svn commit: r363482 - in head/sys: kern sys

[Conrad Meyer]

Hi Konstantin,

On Tue, Jul 28, 2020 at 11:42 AM Konstantin Belousov
<kostikbel at gmail.com> wrote:
>
> On Fri, Jul 24, 2020 at 05:34:05PM +0000, Conrad Meyer wrote:
> > ...
> > --- head/sys/kern/vfs_bio.c   Fri Jul 24 17:32:10 2020        (r363481)
> > +++ head/sys/kern/vfs_bio.c   Fri Jul 24 17:34:04 2020        (r363482)
> > @@ -3849,7 +3849,7 @@ getblkx(struct vnode *vp, daddr_t blkno, daddr_t dblkn
> > ...
> > +     /* Attempt lockless lookup first. */
> > +     bp = gbincore_unlocked(bo, blkno);
> > +     if (bp == NULL)
> > +             goto newbuf_unlocked;
> > +
> > +     lockflags = LK_EXCLUSIVE | LK_SLEEPFAIL |
> > +         ((flags & GB_LOCK_NOWAIT) ? LK_NOWAIT : 0);
> > +
> > +     error = BUF_TIMELOCK(bp, lockflags, NULL, "getblku", slpflag,
> > +         slptimeo);
> I realized that this is not safe.  There is an ordering between buffer
> types that defines which order buffer locks should obey.  For instance,
> on UFS the critical order is inode buffer -> snaplk -> cg buffer, or
> data block -> indirect data block.  Since buffer identity can change under
> us, we might end up waiting for a lock of type that is incompatible with
> the currently owned lock.
>
> I think the easiest fix is to use LK_NOWAIT always, after all it is lockless
> path.  ERESTART/EINTR checks below than can be removed.

Thanks, that makes sense to me.  Please see https://reviews.freebsd.org/D25898 .

(For the UFS scenario, I think this requires an on-disk sector
changing identity from one kind to another?  I believe lblknos are
mostly statically typed in UFS, but it could happen with data blocks
and indirect blocks?  Of course, UFS is not the only filesystem.)

Best regards,
Conrad