svn commit: r255797 - head/sys/kern

[Konstantin Belousov]

On Sun, Sep 22, 2013 at 01:14:21PM -0700, Matthew Fleming wrote:
> 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?

Does it ? If the lock is dropped, the code is indeed in trouble.
Please note that LK_NOWAIT is specified for upgrade, and I believe
that this causes lockmgr to return with EBUSY without dropping
the lock.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20130922/09ce0448/attachment.sig>