The blocking there is not needed.

I think the rule should be that any failure on lockless path for buffer lookup results in locked retry. Imagine that buffer was reclaimed meantime, then you would not reach the check for bufobj/blkno at all.

Why do you think it should be handled there ?

return ()

will fix

We can abort without retry for EINTR/ERESTART, right? Retrying in all other cases makes sense to me.

will fix

Huh. I think you should use lockmgr() directly there, to not touch buffer we do not intend to operate on, at all.

I must admit that 'unlocked' suffix is misleading.

I would name it BUF_UNLOCK_RAW()

Will change.

Will change

If you invert the condition, the control flow can be simplified a bit:

if (bp->b_bufobj == && bp->b_lblkno == blkno)
    goto foundbuf_fastpath;
/* It changed, fall back to locked lookup. */
BUF_UNLOCK_RAW(bp);
loop:

Missing parens.

Sure, will do.

Suppose a buf moved from dirty to clean here, after checking the clean queue, before checking the dirty queue. No buf is found. Is SMR intended to avoid that, or is it only consistent per queue?

We found, presumably, that getblk(GB_NOCREAT) gets the wrong result because of this.

SMR only helps you on a per-queue basis. I think maybe ideally there would be a single pctrie here and some other mechanism for efficiently tracking dirty bufs, but I don't know how realistic that is.

Probably the right fix for getblk GB_NOCREAT is to re-check with the lock if unlocked lookup returned NULL. Something like:

	bp = gbincore_unlocked(bo, blkno);
	if (bp == NULL) {
		if (flags & GB_NOCREAT)
			goto loop;
		else
			goto newbuf_unlocked;
	}

(In the !GB_NOCREAT case, we might see the same race from gbincore_unlocked, but it is harmless: we'd create a useless buf at the same lbn, re-lock to attempt to insert it, and discover we'd lost the race. That race can happen even without SMR-gbincore.)

Suraj came up with the same patch idea. I'm wondering if it could be more efficient to just try gbincore_unlocked a second time rather than wait for the bo lock, in the GB_NOCREAT case.

Ah, cool. I hope Suraj is doing well. Spinning locklessly again (GB_NOCREAT case) might be worth doing, but you'll still need the same locked fallback if you continue to get NULL (for correctness). If you should spin, and for how long, probably depends on how many GB_NOCREAT getblk requests will already exist in the bufq and be transitioning queues, vs true-positive misses in your workload. (And relative costs of a few lockless rechecks vs taking the bufobj lock.)

https://reviews.freebsd.org/D28375