Page MenuHomeFreeBSD

Add vnode_pager_clean(9)
ClosedPublic

Authored by kib on Jan 8 2024, 5:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 15, 10:27 PM
Unknown Object (File)
Thu, Nov 14, 8:49 PM
Unknown Object (File)
Fri, Nov 8, 5:06 AM
Unknown Object (File)
Wed, Nov 6, 10:29 AM
Unknown Object (File)
Sat, Nov 2, 9:36 PM
Unknown Object (File)
Oct 2 2024, 8:05 AM
Unknown Object (File)
Sep 24 2024, 4:07 AM
Unknown Object (File)
Sep 23 2024, 8:05 PM

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib requested review of this revision.Jan 8 2024, 5:39 AM
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 added inline comments.
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 marked 2 inline comments as done.Jan 8 2024, 4:48 PM
kib added inline comments.
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.

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
254

There's no need for retv at all.

sys/vm/vnode_pager.c
1698

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

1706

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 marked 4 inline comments as done.

Switch to vnode_pager_clean_sync/async

sys/kern/vfs_subr.c
4113

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.

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
1722

Extra ws.

kib marked 3 inline comments as done.Jan 11 2024, 3:37 PM
kib added inline comments.
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 marked an inline comment as done.

Bump __FreeBSD_version

markj added inline comments.
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.

This revision is now accepted and ready to land.Jan 11 2024, 3:49 PM
sys/kern/vfs_subr.c
4013

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

This revision was automatically updated to reflect the committed changes.