svn commit: r255797 - head/sys/kern

[Matthew Fleming]

On Sun, Sep 22, 2013 at 12:23 PM, Konstantin Belousov <kib at freebsd.org>wrote:

> Author: kib
> Date: Sun Sep 22 19:23:48 2013
> New Revision: 255797
> URL: http://svnweb.freebsd.org/changeset/base/255797
>
> Log:
>   Increase the chance of the buffer write from the bufdaemon helper
>   context to succeed.  If the locked vnode which owns the buffer to be
>   written is shared locked, try the non-blocking upgrade of the lock to
>   exclusive.
>
>   PR:   kern/178997
>   Reported and tested by:       Klaus Weber <
> fbsd-bugs-2013-1 at unix-admin.de>
>   Sponsored by: The FreeBSD Foundation
>   MFC after:    1 week
>   Approved by:  re (marius)
>
> Modified:
>   head/sys/kern/vfs_bio.c
>
> Modified: head/sys/kern/vfs_bio.c
>
> ==============================================================================
> --- head/sys/kern/vfs_bio.c     Sun Sep 22 19:15:24 2013        (r255796)
> +++ head/sys/kern/vfs_bio.c     Sun Sep 22 19:23:48 2013        (r255797)
> @@ -2624,6 +2624,8 @@ flushbufqueues(struct vnode *lvp, int ta
>         int hasdeps;
>         int flushed;
>         int queue;
> +       int error;
> +       bool unlock;
>
>         flushed = 0;
>         queue = QUEUE_DIRTY;
> @@ -2699,7 +2701,16 @@ flushbufqueues(struct vnode *lvp, int ta
>                         BUF_UNLOCK(bp);
>                         continue;
>                 }
> -               if (vn_lock(vp, LK_EXCLUSIVE | LK_NOWAIT | LK_CANRECURSE)
> == 0) {
> +               if (lvp == NULL) {
> +                       unlock = true;
> +                       error = vn_lock(vp, LK_EXCLUSIVE | LK_NOWAIT);
> +               } else {
> +                       ASSERT_VOP_LOCKED(vp, "getbuf");
> +                       unlock = false;
> +                       error = VOP_ISLOCKED(vp) == LK_EXCLUSIVE ? 0 :
> +                           vn_lock(vp, LK_UPGRADE | LK_NOWAIT);
>

I don't think this is quite right.

When the lock is held shared, and VOP_LOCK is implemented by lockmgr(9),
(i.e. all in-tree filesystems?), LK_UPGRADE may drop the lock, and not
reacquire it.  This would happen when the vnode is locked shared, the
upgrade fails (2 shared owners), then lockmgr(9) will try to lock EX, which
will also fail (still one shared owner).  The caller's lock is no longer
held.

Doesn't that scenario (LK_UPGRADE failing) cause problems both for the
caller (unexpected unlock) and for flushbufqueues(), which expects the
vnode lock to be held since lvp is non-NULL?

Thanks,
matthew