Add vnode_pager_clean(9)

[asomers]

sys/fs/nfsclient/nfs_clbio.c

1439

Now, with this change, we'll call newnfs_sigintr even if vp->v_bufobj.bo_object is NULL. Is that intended?

[olce]

sys/fs/nfsclient/nfs_clbio.c

1439

Not familiar with this code, but shouldn't the return code of vnode_pager_clean() (formerly, that of vm_object_page_clean()) be also taken into account in the test guarding the goto out; block? (not related to this change; just a remark in passing).

[kib]

sys/fs/nfsclient/nfs_clbio.c

1439

IMO either of conditions do not matter. We try to ensure that signals interrupt NFS ops for soft mounts, but never promised that we must do any progress before the interrupt.

[markj]

I would just have separate functions, vnode_pager_clean() and vnode_pager_clean_async(), which internally specify the OBJPC_* flags. In particular, only OBJPC_SYNC is needed, and it's likely to stay that way. Using the OBJPC_* flags in the vnode_pager namespace feels strange.

sys/fs/nfsclient/nfs_clnode.c

256

There's no need for retv at all.

sys/vm/vnode_pager.c

1697

Now that almost all callers are converted, it's easy to change the return type to bool.

1705

What's the purpose of having ign_nosync at all? There are two callers that specify false - they can just test for VV_NOSYNC directly.

[kib]

Switch to vnode_pager_clean_sync/async

[kib]

sys/kern/vfs_subr.c

4112

I think this should be vnode_pager_clean_sync(), to make the requirements on VOP_FSYNC() less severe. I will do it as a follow-up.

[markj]

sys/contrib/openzfs/include/os/freebsd/spl/sys/vnode.h

108

We'd need a __FreeBSD_version bump in order to upstream this.

sys/kern/vfs_subr.c

4013

Now we have to acquire the object lock before checking whether the object might be dirty. Can we avoid this?

sys/ufs/ffs/ffs_rawread.c

137

The mightbedirty check is lost - is that intentional? vm_object_page_clean() will check but this requires the object lock. Oh, we check it earlier, ok.

sys/vm/vnode_pager.c

1721

Extra ws.

[kib]

sys/kern/vfs_subr.c

4013

I think that this is not too important. Inactivation is the costly operation by itself, we manipulate a lot of locks there. I can add a special variant that checks mightbedirty() in unlocked manner, but do not see much sense. In fact this clears a (rare) race.

sys/ufs/ffs/ffs_rawread.c

137

Again it is racy and it might be better to remove the unlocked check. If we loose the race, the data could be corrupted.

[kib]

Bump __FreeBSD_version

[markj]

sys/kern/vfs_subr.c

4013

Hmm, what's the race? I believe the vnode lock is required to bump cleangeneration, and here we hold the exclusive lock.

[kib]

sys/kern/vfs_subr.c

4013

For cleangeneration yes, but not for generation. And it is the later which dirties the object.