Fix dir iteration being broken by concurrent removes #1068
Conversation
When removing a file, we mark all open handles as "removed" (
pair={-1,-1}) to avoid trying to later read metadata that no longer
exists. Unfortunately, this also includes open dir handles that happen
to be pointing at the removed file, causing them to return
LFS_ERR_CORRUPT on the next read.
The good news is this is _not_ actual filesystem corruption, only a
logic error in lfs_dir_read.
We actually already have logic in place to nudge the dir to the next id,
but it was unreachable with the existing logic. I suspect this worked at
one point but was broken during a refactor due to lack of testing.
---
Fortunately, all we need to do is _not_ clobber the handle if the
internal type is a dir. Then the dir-nudging logic can correctly take
over.
I've also added test_dirs_remove_read to test this and prevent another
regression, adapted from tests provided by tpwrules that identified the
original bug.
Found by tpwrules
geky
force-pushed
the
fix-dir-remove-read
branch
from
a39cb26 to
caba4f3
Compare
Tests passed ✓, Code: 17112 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)
|
|
Thanks a bunch, I was able to test in our application with this patch on top of 2.10.1 and it seems to fix the bug we observed. Do you think the case of removing multiple files between directory reads is adequately tested? My tests from the issue do pass after this patch too. |
I'm not opposed to accepting more tests :) The problem is you can always have more testing, and my attention is elsewhere on a changeset that will eventually add more testing (rbyd). So hardening the current state of things is unfortunately not a priority for me right now. I'll respond with more in #1061 (comment) |
|
@tpwrules Sorry about the delay on bringing this in. Unrelated PRs delayed the release a bit. Thanks for raising the issue on this! |
When removing a file, we mark all open handles as "removed" (pair={-1,-1}) to avoid trying to later read metadata that no longer exists. Unfortunately, this also includes open dir handles that happen to be pointing at the removed file, causing them to return LFS_ERR_CORRUPT on the next read.
The good news is this is not actual filesystem corruption, only a logic error in
lfs_dir_read.We actually already have logic in place to nudge the dir to the next id, but it was unreachable with the existing logic. I suspect this worked at one point but was broken during a refactor due to lack of testing.
Fortunately, all we need to do is not clobber the handle if the internal type is a dir. Then the dir-nudging logic can correctly take over.
I've also added
test_dirs_remove_readto test this and prevent another regression, adapted from tests provided by @tpwrules that identified the original bug.Found by @tpwrules
See #1061 for more info