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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| 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. |
| 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. |
| 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. |
| 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. |
| 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. |
| 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? |