Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix msync04 race #1157

Open
pevik opened this issue May 20, 2024 · 4 comments
Open

Fix msync04 race #1157

pevik opened this issue May 20, 2024 · 4 comments
Labels

Comments

@pevik
Copy link
Member

pevik commented May 20, 2024

msync04 suffers races. According to Jan Kara nothing guarantees that the page is not written out before get_dirty_page() manages to read the page state, test should be reworked to verify the page contents is on disk when it finds dirty bit isn't set anymore:

What would be a more sensible test is that msync is indeed persisting the data. So something like: mmap file, write to mmap, msync, abort fs, mount fs again, check the data is there. We do have framework for stuff like this in fstests (but we don't test msync AFAIK, only fsync), not sure if LTP has something for this as well [1]).

=> We should find out how fstests do filesystem abort. And also evaluate if it'd make more sense to add msync support to fstests.

[1] https://lore.kernel.org/ltp/[email protected]/

@pevik pevik added the bug label May 20, 2024
@pevik pevik pinned this issue May 21, 2024
@coolgw
Copy link
Contributor

coolgw commented May 27, 2024

@coolgw
Copy link
Contributor

coolgw commented May 28, 2024

New version of patch needed, updating.

@pevik
Copy link
Member Author

pevik commented May 28, 2024

We merge improvement (hopefully) in 66517b8, but there are still some TODO from Jan Kara (https://lore.kernel.org/ltp/20240528095030.valplwc5t3m3betn@quack3/):

  • Use at least direct IO to verify the data is stored on disk now (maybe I was wrong and 66517b8 just turns test into false positive)
  • Or add support to LTP to abort fs and remount it
  • Also support for msync() could be added to fstests (https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/) to where is infrastructure for abort fs and remount it, but they don't test msync().

@metan-ucw FYI

@pevik
Copy link
Member Author

pevik commented Jun 17, 2024

At least verification for storing data on the disc was added in 21be0b1.

@metan-ucw Do we want to have this open for looking into abort fs or we are done? I'm not sure if anybody spents time to implement it.

@pevik pevik unpinned this issue Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants