Details
- Reviewers
alc markj - Commits
- rGb068bb09a1a8: Add vnode_pager_clean_{a,}sync(9)
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
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? |
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). |
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. |
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. |
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. |
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. |
sys/kern/vfs_subr.c | ||
---|---|---|
4013 | For cleangeneration yes, but not for generation. And it is the later which dirties the object. |