For the unr(9) changes, did you provide an interface that accepts an arbitrary lock_object, the way _sleep(), _callout_init_lock(), etc. work?

I disagree with the change. If locking at hand is a problem the code should be reworked to not need it in the first place.

I will note most consumers don't care for a specific error code, just whether it is a device or not.

One trivial idea would be to add a flag to ->v_irflag maintained by devfs. For non-VCHR there is already nothing to do. For VCHR I would note devices have a backpointer to all associates vnodes. Should the vnode point to a "disk" it can get a VIRF_ISDISK flag or similar. Should anything go wrong with the device, pointers get NULLified and whatnot, a loop over said vnodes can unset the flag. This would satisify almost everyone.

The remaining cases can call the routine in the current form. Should there be a frequent consumer which also wants a specific error further changes can be made.

To extend the point the fast path would be +/-:

diff --git a/sys/kern/vfs_subr.c b/sys/kern/vfs_subr.c
index 2df609d9118..0bb97e981ae 100644
--- a/sys/kern/vfs_subr.c
+++ b/sys/kern/vfs_subr.c
@@ -4977,8 +4977,15 @@ vn_need_pageq_flush(struct vnode *vp)
 /*
  * Check if vnode represents a disk device
  */
+bool
+vn_isdisk(struct vnode *vp)
+{
+
+       return ((vp->v_irflag & VIRF_ISDISK) != 0);
+}
+
 int
-vn_isdisk(struct vnode *vp, int *errp)
+vn_isdisk_error(struct vnode *vp, int *errp)
 {
        int error;
 
@@ -4994,10 +5001,11 @@ vn_isdisk(struct vnode *vp, int *errp)
                error = ENXIO;
        else if (!(vp->v_rdev->si_devsw->d_flags & D_DISK))
                error = ENOTBLK;
+       if (error == 0)
+               MPASS(vn_isdisk(vp));
        dev_unlock();
 out:
-       if (errp != NULL)
-               *errp = error;
+       *errp = error;
        return (error == 0);
 }

with everyone passing NULL for error sticking to one-argument vn_isdisk (can even become an inline). The few other cases can use vn_isdisk_error.

Of course the real work is making devfs cooperate here, but I think this is the way to go.

I presume the problematic consumer in your workload is vfs_bio.c. It rolls with:

bsize = vn_isdisk(vp, NULL) ? DEV_BSIZE : bo->bo_bsize;

is the call really necessary to begin with? And is DEV_BSIZE really the right thing to use here? Perhaps the right size can already be there in bo_bsize?

In D26086#579112, @markj wrote:

For the unr(9) changes, did you provide an interface that accepts an arbitrary lock_object, the way _sleep(), _callout_init_lock(), etc. work?

Yes, as a static helper inside subr_unr.c; not a published API.

In D26086#579115, @mjg wrote:

I disagree with the change. If locking at hand is a problem the code should be reworked to not need it in the first place.

Sure, that's fine. Setting some flag on the vnode is not too bad; the harder part is invalidation when disks go away (if we move away from the dev_lock synchronization currently used for both destroy_devl and vn_isdisk).

I will note most consumers don't care for a specific error code, just whether it is a device or not.

Agreed, that's all that really matters.

One trivial idea would be to add a flag to ->v_irflag maintained by devfs. For non-VCHR there is already nothing to do. For VCHR I would note devices have a backpointer to all associates vnodes.

What's the backpointer?

In D26086#579135, @mjg wrote:

Of course the real work is making devfs cooperate here

Right.

In D26086#579164, @mjg wrote:

I presume the problematic consumer in your workload is vfs_bio.c. It rolls with:

Yeah, there are about four consumers in vfs_bio and they are all likely hot in our workloads. getblk/brelse paths.

bsize = vn_isdisk(vp, NULL) ? DEV_BSIZE : bo->bo_bsize;

is the call really necessary to begin with? And is DEV_BSIZE really the right thing to use here? Perhaps the right size can already be there in bo_bsize?

DEV_BSIZE is really the right thing, although I don't remember why bo_bsize wasn't set to DEV_BSIZE instead. Maybe we can set bo_bsize to match DEV_BSIZE when initializing bufobjs for disks. I don't know.

Either way, there are a few other consumers that wouldn't be addressed by fixing bo_bsize, so we need a cheap version of this anyway.

Some historical context around mismatched bo_bsize: https://svnweb.freebsd.org/base?view=revision&revision=206097

In D26086#579224, @cem wrote:

In D26086#579115, @mjg wrote:

One trivial idea would be to add a flag to ->v_irflag maintained by devfs. For non-VCHR there is already nothing to do. For VCHR I would note devices have a backpointer to all associates vnodes.

What's the backpointer?

Is that de->de_vnode / vp->v_data? Yeah, that works.

We already similarly grab VV_ISTTY out of d_flags & D_TTY.

See devfs_revoke. All vnodes from all mount points need to be updated. Some refactoring may be needed to make it feasible.

Alternatively, if that runs into too many woes, as a half measure the flag could be implemented in struct cdev. Then vn_isdisk would get away with taking vnode interlock.

I looked at all vn_isdisk() consumers in vfs_bio.c, ufs/ffs, and vm/. As was correctly noted, vn_isdisk() provides one of three results: disk/not disk/device destroyed. I believe that 'device destroyed' answer is accidental to the way vn_isdisk() currently works and in fact is badly handled for vfs_bio.c and vm/vnode_pager.c. The only place where 'disk destroyed' (AKA ENXIO) result could have some relevance is ffs mount code, where it short-circuits whole mount.

For vfs_bio.c, in fact taking the ENXIO case into account, as it is done now, causes consistency issues. Similarly, for vnode_pager.c, vn_isdisk() is used to gate creation of the devvp v_object where I do not see a reason to deny it after ENXIO.

So I think that mjg' idea of VIRF_ISDISK flag is worth trying, and the complex synchronization issues could be ignored for hot path because hot path really better served by consistent answer than by the notice that devvp was destroy_dev'ed.

In D26086#579112, @markj wrote:

For the unr(9) changes, did you provide an interface that accepts an arbitrary lock_object, the way _sleep(), _callout_init_lock(), etc. work?

I don't have any use for the unr changes, but in case they are interesting to someone in the future:

https://people.freebsd.org/~cem/unr.lock.abstraction.1of2.patch
https://people.freebsd.org/~cem/unr.lock.abstraction.2of2.patch