Page MenuHomeFreeBSD

Drop the object lock in vfs_bio and cluster
ClosedPublic

Authored by jeff on Sep 10 2019, 8:33 PM.

Details

Summary

Now that page busy/valid/dirty and object pip can be inspected and modified without the object lock quite a few places in vfs no longer require the object lock. This patch converts all of the code in vfs to either elide the object lock or use a read lock in many places.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jeff created this revision.Sep 10 2019, 8:33 PM
jeff retitled this revision from Drop the object lock where it is no longer needed. to Drop the object lock in vfs_bio and cluster.Oct 15 2019, 3:54 AM
jeff edited the summary of this revision. (Show Details)
jeff added reviewers: kib, markj, alc.
jeff added inline comments.Oct 15 2019, 3:56 AM
sys/kern/vfs_bio.c
3674 ↗(On Diff #61915)

I deleted this condition because I didn't want to protect the flag with the object lock. However, I now believe a racy flag check is safe because we hold the busy lock on the constituent pages in these cases. I could re-institute it here although it does not seem like a significant burden to check dirty.

4721 ↗(On Diff #61915)

I think this is simply an optimization of the general case below for single pages. I don't believe that is common or necessary. I would appreciate another set of eyes to verify.

kib added inline comments.Oct 16 2019, 12:35 PM
sys/kern/vfs_bio.c
4721 ↗(On Diff #61915)

It seems that indeed, this case is just an optimization. The code initially appeared in some Dyson' commit from 1995 r7694 without a useful commit message.

sys/kern/vfs_cluster.c
552 ↗(On Diff #61915)

I think this (and similar) asserts should be replaced by checks that bo_object == b_pages[i]->object.

jeff added a comment.Oct 16 2019, 6:41 PM

Any comment on the MIGHTBEDIRTY check?

kib added inline comments.Oct 16 2019, 6:55 PM
sys/kern/vfs_bio.c
3674 ↗(On Diff #61915)

OBJ_MIGHTBEDIRTY means that there might be (but not necessary are) dirty pages in the object, so checking the flag should be an optimization always.

jeff added inline comments.Oct 16 2019, 7:02 PM
sys/kern/vfs_bio.c
3674 ↗(On Diff #61915)

I understand that but in this case we no longer hold the object lock. So the check would race. I think this could be safe because we hold the page busy. So if there is a race to set the flag it can't involve this set of pages. If there is a race to clear the flag it similarly can't involve these pages and we may simply do extra work.

It seems like a small optimization to avoid this code given the amount of page iteration that is done in general. The flag check will rely on somewhat obscure synchronization that may be fragile. This is why I removed it.

kib added inline comments.Oct 16 2019, 7:18 PM
sys/kern/vfs_bio.c
3674 ↗(On Diff #61915)

From this PoV it seems to be more correct to not check the flag than to check it. But I think that it is only advisory and at worst the conversion of dirty pages to dirty buffers should happen somewhat later.

jeff added inline comments.Oct 21 2019, 11:58 PM
sys/kern/vfs_bio.c
3674 ↗(On Diff #61915)

I feel ambivalent really. I would like to go forward with the patch as-is. If we find a workload where it really matters that we reduce the cpu time here we can look at the precise synchronization guarantees provided for MIGHTBEDIRTY optimizations. If you are ok with that can you approve of the commit?

kib accepted this revision.Oct 22 2019, 12:04 AM
This revision is now accepted and ready to land.Oct 22 2019, 12:04 AM