mirror of
https://github.com/s3fs-fuse/s3fs-fuse.git
synced 2026-04-25 21:35:58 +03:00
[GH-ISSUE #1453] Corruption following rename() #762
Labels
No labels
bug
bug
dataloss
duplicate
enhancement
feature request
help wanted
invalid
need info
performance
pull-request
question
question
testing
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/s3fs-fuse#762
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @init-js on GitHub (Oct 15, 2020).
Original GitHub issue: https://github.com/s3fs-fuse/s3fs-fuse/issues/1453
Version of s3fs being used (s3fs --version)
Version of fuse being used (pkg-config --modversion fuse, rpm -qi fuse, dpkg -s fuse)
2.9.9Kernel information (uname -r)
4.14.177-139.254.amzn2.x86_64GNU/Linux Distribution, if applicable (cat /etc/os-release)
s3fs command line used, if applicable
/etc/fstab entry, if applicable
s3fs syslog messages (grep s3fs /var/log/syslog, journalctl | grep s3fs, or s3fs outputs)
if you execute s3fs with dbglevel, curldbg option, you can get detail debug messages
Details about issue
It feels like a caching bug is causing s3fs to upload a file filled with zeroes if you trigger a chmod/chown just after a rename.
At the end of the following program, "B" will have the correct length, but its contents will be nothing but nil bytes
\0.(This is assuming /mnt is the fuse mountpoint.)
Note: the target file is not only corrupted when accessed via fuse. If we access the target file out-of-band, via the AWS console, it reveals that the target file is also corrupted in the bucket, at rest. S3fs makes an explicit PUT with nil bytes.
The
chmod 0644call triggers:Then, reading the file at the end of the script (i.e. the last line) triggers:
curl.cpp:PutRequest(3433): uploading... [path=/linux_src/my_target][fd=12][size=11]
curl.cpp:GetObjectRequest(3517): downloading... [path=/linux_src/my_target][fd=12]
Not sure why that last upload needs to happen, but that's where the file gets corrupted. What gets uploaded is a body of 11 nul bytes. The broken upload does not happen if the file has been read between the rename and the chmod.
@gaul commented on GitHub (Oct 16, 2020):
I can reproduce these symptoms although am unsure about the fix:
@ggtakec do you think this is correct?
@init-js commented on GitHub (Oct 16, 2020):
Attaching two traces that show the different sequence of events. The corrupt trace is the script in the original issue post. the "ok" trace is the one where we read the file between the rename and the chmod.
s3fs-ok.trace.log
s3fs-corrupt.trace.log
Aside from the extra obvious read() operation that's added between rename and chmod, the main difference is s3fs's behaviour during the very last read of the script (i.e. after
final contents:).In the OK trace, the last read just hits the cache after chmod() completes? no download nor upload
@init-js commented on GitHub (Nov 5, 2020):
@gaul , thanks for looking into it.
Can you justify your reasoning a bit more for patching that block? It seems strange that the state is incorrect by the time the final read takes place. Ignoring time seems like a patch for this scenario, but I'm not convinced it's a comprehensive fix.
github.com/s3fs-fuse/s3fs-fuse@38e1eaa8a3/src/fdcache_entity.cpp (L432)@bitprophet commented on GitHub (Feb 10, 2021):
I can confirm this bug:
use_cacheSpecifically, I can confirm the proximate trigger of the bug,
though I haven't yet proven the pseudo-workaround of "read before chmod"including that reading before the chmod avoids the issue; I haven't tested the patch (though I'm intending to).My 100% reproducible real world trigger is using
rpm-sign(egrpm --delsign /path/to/s3fs/some.rpm, though I've seen what may be the same issue in a much more infrequent basis during runs ofcreaterepoacross large RPM repositories.To wit:
@bitprophet commented on GitHub (Feb 10, 2021):
Actually I'm not sure the prior comments outlined something I found above: the cache is being filled with the corrupt data by the interplay between the rename and the chmod, and the open() is just flushing that already bogus data upstream.
(Note: the stat cache mirrors the cache in all cases - when the cache is empty the stat cache is also. So at least a disparity there is unlikely to be at fault.)
The more I (admittedly not a C++ or filesystem dev) look at this the more I'm wondering why the rename isn't resulting in a rename in the cache; that appears to be the root of the issue.
The chmod (setattr/getattr underneath, iirc) is probably a second-order bug that might also need fixing (refresh from S3 when cache is empty, instead of spitting out a bunch of zeroed pagelist blocks or whatever it's doing now) but would likely not be triggered if the other is patched.
@gaul commented on GitHub (Feb 11, 2021):
Thanks for digging into this! I will look into this again but it would help greatly if you can send a PR with a minimized and failing test case to
test/integration-test-main.sh@gaul commented on GitHub (Feb 11, 2021):
Also if you are working on a fix, consider the simplest approach of flush or invalidate instead of a more complicated one preserving the cache contents. While performance is important, we can work on that in a subsequent commit.
@gaul commented on GitHub (Feb 11, 2021):
Here is a minimal test case, transliterated from the description from @bitprophet:
@gaul commented on GitHub (Feb 11, 2021):
We can further minimize this by creating the object with the AWS CLI:
s3fs seems to do an unnecessary flush when it does not know anything about the remote object and a user modifies the metadata of it.
@gaul commented on GitHub (Feb 11, 2021):
I can verify that https://github.com/s3fs-fuse/s3fs-fuse/issues/1453#issuecomment-710085421 addresses the symptom with the above test case. I believe that the logic here is incomplete -- it considers that the mtime is not -1 because a file write has occurred but instead it is due to the chmod. @ggtakec what do you think?
@ggtakec commented on GitHub (Feb 17, 2021):
I'm sorry I was late in noticing this issue.
It may work with changing @gaul, but it needs a little more time to check.
But, when I was checking the log file, there was something wrong.
In the process of
s3fs.cpp:rename_object(), the file cache is also moved.Then, it deletes the original file and delete the cache.
However, as you can see in the last line above, it even deleted the file cache of the new file(zzz).
Clearing the Stats cache for new files is the correct behavior. This will be needed if the ctime changes(although but it should put more conditions for stats cache clear).
At this point, I think there was an inconsistency(it seems that the cache was acting as if it existed even though there was no cache file) and it was filled with 0x00 bytes.
After applying and trying the following patches, this issue seems to have been resolved.(I will still test)
I'm sorry, but please take a little more time to completely check the logic and finish the test.
I would be grateful if you could try the above changes.
@ggtakec commented on GitHub (Feb 17, 2021):
After applying the above patch, other tests failed.
I need to investigate a little more.
@ggtakec commented on GitHub (Feb 19, 2021):
@gaul (and all) I found the cause of the problem of this.
(The part that was deleting the cache shown above was one of the factors other than the multipart HEAD request.)
The rename logic internally copies the file and deletes the original file.
The cause of the bug was that s3fs was updating mtime immediately after this copy, at which time a new cache file was created and the contents of the file were null.
I will publicize the fix immediately.
@gaul commented on GitHub (Feb 20, 2021):
@init-js Could you test with the latest master?
@bitprophet commented on GitHub (Feb 24, 2021):
1.89 is already cut but just want to note that the patch which made it into the release, DOES fix my reproducible issue. Thanks so much!
I'm hoping it also fixes my less-reproducible issue (which may be purely metadata modification, no renaming; would guess in that case it's just "metadata-modified files were not in cache to begin with") but time will tell.
@gaul commented on GitHub (Feb 25, 2021):
@bitprophet Thanks for working with us on this issue! Good bug reports make a big difference in finding a fix. If there are other symptoms let's open a new issue and address that separately.