page_cache: eliminate PageWriteGuard
#5119
Conversation
1624 tests run: 1547 passed, 0 failed, 77 skipped (full report)The comment gets automatically updated with the latest test results
7d79ace at 2023-08-28T09:30:36.897Z :recycle: |
problame
requested review from
conradludgate,
skyzh and
hlinnaka
and removed request for
a team,
conradludgate and
skyzh
There was a problem hiding this comment.
I don't know, the new interface doesn't feel very nice to me. I think the PageWriteGuard was a fine approach. Was the motivation just to simplify things? I like that this is less lines of code, but other than that I'm not sure it's really simpler.
If we turn this async in the future, we'll need to add an async variant that takes a Future, as you said. PageWriteGuard gives the caller more flexibility, and I don't think it's any more complicated in the callers either.
| /// lock_for_read() return value | ||
| pub enum ReadBufResult<'a> { | ||
| Found(PageReadGuard<'a>), | ||
| NotFound(PageWriteGuard<'a>), | ||
| MissFilled, |
There was a problem hiding this comment.
In my head, this becomes "misfilled", i.e. filled incorrectly.
| @@ -511,7 +445,14 @@ impl PageCache { | |||
| /// } | |||
There was a problem hiding this comment.
Please update the comment above
I find the usage a bit weird now. Could lock_for_read() always return PageReadGuard<'a> ? I.e it could do the looping and retrying.
| Already, | ||
| Inserted, |
There was a problem hiding this comment.
These need more comments and/or clearer names too, IMHO
Motivation is the first point in #5111 The second point in #5111 is addressed by my #5122 which is stacked on top of this PR here. |
|
FTR, as the Also, personally I prefer the approach to just set the counter to 0 to this PR, like suggested in #5111. |
|
Also, for the eventual switch to io-uring, having an owned buffer handle is required. PageWriteGuard is fit for that purpose. So, let's leave it and close this PR. |
Since my #4994 , the page cache doesn't support mutating pages anymore.
The remaining role of
PageWriteGuardis that of a fill handler. We can useFnOnceclosures (or in an aysnc future: aFuture) for that.So, make the change now.