--LpQ9ahxlCli8rRTG
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 05, 2007 at 08:44:01AM -0400, Greg Troxel wrote:
>=20
> > Simply put, any time you have locks, you have to have a locking order.=
=20
> > Since most lookups progress from root down, the locking order we have i=
s=20
> > from the root vnode down. Since ".." really is a vnode higher up, you=
=20
> > can't lock ".." whie you have a lock on your directory as that's not=20
> > locking from the top down. So you release your lock, lock "..", then=20
> > re-lock ".".
>=20
> Thanks - I do follow that we need to have an order to avoid deadlocks,
> and what our order is.  What's not 100% clear in my head is the exact
> relationship between the interlock and the vnode lock, which has to be
> held for each possible operation, and mostly what can happen after vref
> but without holding the vnode lock.

Sorry!

Interlock is fine. You don't take other locks while holding the interlock.=
=20
You at most trade the interlock on a vnode for the lock on said vnode. And=
=20
the loking code handles this correctly.

> >>   for i in `cd /usr/bin && ls`; do cp -pf /usr/bin/$i ../$i & done
> >> and=20
> >>   for i in `cd .. && ls`; do cat ../$i  > /dev/null & done
> >
> > How many concurrent processes will this fire up? If you used to die at=
=20
> > this and now don't, that's an improvement.
>=20
> Hundreds.   I didn't save my script from before, but I think that's what
> I was doing.

Ok.

> >> I know that wrstuden@ in the past said that LK_RETRY was not correct.
> >
> > I personally think it's vile for this.
>=20
> Without LK_RETRY, and maybe even with, the caller has to check the
> vn_lock return value and do something.  In vfs_lookup, if we fail to
> lock the child, we can return an error.  But, since we have a vref at
> this point, it seems this should never happen, so maybe it should be a
> panic.

It's unlikely at the moment. If you have a ref, the one thing that could=20
happen is a VOP_REVOKE(), but I don't think you can do that on a=20
directory.

Oh, the one other thing that might happen some day is that we add code to=
=20
handle forcible file system unmount. In such a case, we would probably=20
want to revoke all the vnodes. So you could have a reference on something=
=20
then have it die on you.

> In the case of relocking the parent, we also have a vref (or rather our
> caller must), so can that ever reasonably fail?  Should I panic if
> vn_lock (w/o LK_RETRY) fails?

Don't panic. Return an error. This case (".." lookup) is why I added=20
PDIRUNLOCK. I am not sure if chuq removed it.

> > You NEVER grab a lock while getting the new vnode? If so, then this is =
ok.=20
> > If you grab an intermediate lock, you probably should do the ISDOTDOT=
=20
> > dance.
>=20
> Yes, that's how the code goes.  It uses coda's name cache or venus, and
> those come back with a ref but no locks.

Then you're fine.

> > Pick an ordering and stick with it. It doesn't matter what it is, beyon=
d=20
> > the fact some orderings are easier to work with than others.
> >
> > The same ordering as VFS is probably good, though.
>=20
> I agree in general; the problem is that I don't understand how coda is
> dealing with locking in the non-vfs part of the code.

Oh! Sorry! I mistook where the misunderstanding is.

> > My thought is that having either one of them set should inhibit caching=
.=20
> > When is CODA_NOCACHE set?
>=20
> When venus sets it before returning a cnode.
>=20
> I don't understand if the coda namecache is merely duplicative of the
> regular name_cache, or somehow different.

Ok. I don't know.

> >>      /*
> >> -     * If the lookup went well, we need to lock the child.
> >> +     * If the lookup succeeded, we must generally lock the returned
> >> +     * vnode.  This could be a ., .., or normal lookup.  See
> >> +     * vnodeops(9) for the details.
> >
> > I really think you need to start this dance sooner.
>=20
> Because it's unsafe to be holding just a vref without a lock?  Or
> because you think this might deadlock?  Or something else?
> (Sorry, I'm really beyond my level of solid understanding here.)

If there really are no hierarchical locks in the venus code, then you're=20
right that you don't need to do locking sooner. I was concerend there=20
might be locks in the venus code that end up leading to deadlock=20
potential.

> OK, I will commit this, and work from there.  The old code is clearly
> wrong, and this works.

Great!

Take care,

Bill

--LpQ9ahxlCli8rRTG
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (NetBSD)

iD8DBQFGFSRYWz+3JHUci9cRAukIAKCI65oipL5yMUwgiI7GUKqytwmJAwCghOaJ
sBwgB5GLRws/7+9K4DZ6BKo=
=eSsF
-----END PGP SIGNATURE-----

--LpQ9ahxlCli8rRTG--