Re: locking patches for ufs_rename

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

David Holland <dholland-tech%netbsd.org@localhost> wrote:
> The attached patches correct the locking in ufs_rename and
> ufs_wapbl_rename. They have been tested lightly and work in the simple
> cases; I will beat on them harder before committing.

Thanks for taking this, I think it is a valuable piece of work.

However, please do not send massive attachments to the list, rather upload
them and provide links. :)

> 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.

> 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).

> Any testing or auditing/reviewing would be appreciated.
> 

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.
Code should be fixed (unlock the vnode, amend error paths) accordingly.
Failure/error paths (from fail_withlocks) re-checked.

>       // 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.

>               /* 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.

Same question applies to the (fromparent_vnode != toparent_vnode) path.

>               if (error && error != ENOENT) {
>                       /*VOP_UNLOCK(toparent_vnode); -- same as fromparent */
>                       VOP_UNLOCK(fromchild_vnode, 0);
>                       VOP_UNLOCK(fromparent_vnode, 0);
>                       goto fail_nolocks;
>               }

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

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.

-- 
Mindaugas