Re: locking patches for ufs_rename

[David Holland <dholland-tech%netbsd.org@localhost>]

On Wed, Sep 30, 2009 at 04:24:20PM +0100, Mindaugas Rasiukevicius wrote:
 > However, please do not send massive attachments to the list, rather upload
 > them and provide links. :)

Yeah, I didn't realize how big it was until after I noticed it had
gotten stuck on the way to the list. :-/

 > > The first patch is naming reform to abolish the alphabet soup of tdvp,
 > > tvp, tdp, ip, xp, and so forth. This has been checked to make only
 > > very minor changes to the generated .o files.
 > 
 > Sounds reasonable.  Can we do it in the following order: separate these
 > routines into a separate ufs_rename.c file, apply cosmetic/name changes
 > and then do functional changes.

I'm not convinced it's worth putting ufs_rename in its own file.
Despite the huge block comment, my version is quite a bit shorter than
the previous version. :-)

The separate wapbl copy should just be killed entirely.

 > > Since I've pretty much rewritten the whole thing the diffs may not be
 > > the easiest to read, so I've also included copies of the updated
 > > functions, plus the new ufs_parentcheck() that replaces ufs_checkpath().
 > 
 > I tend to think that we should write proper test cases and regression tests
 > for this (also, attempt to generate some races).  It requires additional
 > thought and work, but it will be useful not only for revamp, but also any
 > further changes to rename (including non-ufs rename).

We have some, and I have some others that I need to find time to
atfify.

On the other hand, I also don't entirely want to commit them until we
expect the kernel to survive running them on the common fses. ;-/

 > Now some comments on the actual patch:
 > 
 > >     * Due to oddities in namei and in the fs-independent code,
 > >     * currently we are called with references to all four vnodes,
 > >     * but with only toparent_vnode locked.
 > 
 > This comment is wrong i.e. tochild_vnode (if not NULL) is also locked.

good call, I apparently just flatly misread vnode_if.src.

 > >    // XXX do we need this?
 > >    //from_name->cn_flags &= ~SAVESTART;
 > >    //to_name->cn_flags &= ~SAVESTART;
 > >    // XXX or this form instead?
 > >    //from_name->cn_flags &= ~(MODMASK | SAVESTART);
 > >    //from_name->cn_flags |= LOCKPARENT | LOCKLEAF;
 > 
 > It does not seem that namei() would change these flags, but not sure if
 > convention is to rely on this.  However, if we rely, then these should
 > definitely be KASSERT()s.

relookup() specifically does not care, but I'm not (yet) sure what all
30 or so VOP_LOOKUP implementations might do. I'm going to be figuring
this out eventually; in the meantime I think as long as it runs and
the fs remains unmountable that it's best to leave the flags alone.

 > >            /* XXX see note above */
 > >            if (fromchild_vnode == ap->a_tvp) {
 > >                    VOP_UNLOCK(fromchild_vnode, 0);
 > >                    do_race_condition = 1;
 > >            }
 > 
 > Hmm, that is slightly confusing.  Since fromparent_vnode was unlocked, is
 > there anything who prevents from other thread doing, let's say, rename on
 > "old" a_tvp, and creating a hardling to fromchild_vnode in tochild_vnode's
 > path?  If not (unless you serialize entire ufs_rename(), but that is not
 > what we want), following relookup() will lock against oneself.

It is what we want; there's only one rename at a time per volume. This
is necessary for at least two other major reasons. (Go reread the big
block comment. :-) )

The case where do_race_condition is nonzero *is* a race condition. At
the time I wrote it I was reasonably convinced it wouldn't, by racing,
cause a deadlock.

Fixing it requires fixing the locking model for VOP_LOOKUP, which I'm
going to be doing but not within the next couple weeks and not for
5.1.

(It also requires changing vget() to not return locked vnodes. Is
there any real reason we do this? It seems entirely bogus to me.)

 > Should be if (do_race_condition == 0) for fromchild_vnode, otherwise error
 > path unlocks twice.  Same bug in (fromparent_vnode != toparent_vnode) path.

Indeed.

 > I did not glance much on further code, but I think having one separated
 > step "lock all" is the right way.  Further code should work, assuming old
 > logic is correct.

right...

-- 
David A. Holland
dholland%netbsd.org@localhost