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

btrfs: do not wrongly report failure when doing btrfs comparison #438

Conversation

ronnychevalier
Copy link

snapper would always fail using btrfs features to perform a comparison
between two read-only snapshots, and fallback to calling stat on each
file.

The reason was that btrfs_read_and_process_send_stream would first
return 0, because it successfully did its work, and the second time
it would return -ENODATA, because no more data was present in the
stream to be processed.

We now check that we fail due to ENODATA only if it was the first time
we tried to call btrfs_read_and_process_send_stream.

In my case, a comparison took initially ~30s, and came down to ~300ms
with this fix.

snapper would always fail using btrfs features to perform a comparison
between two read-only snapshots, and fallback to calling stat on each
file.

The reason was that btrfs_read_and_process_send_stream would first
return 0, because it successfully did its work, and the second time
it would return -ENODATA, because no more data was present in the
stream to be processed.

We now check that we fail due to ENODATA only if it was the first time
we tried to call btrfs_read_and_process_send_stream.

In my case, a comparison took initially ~30s, and came down to ~300ms
with this fix.
@aschnell
Copy link
Member

Interesting. Do you know whether this is due to a change in (lib)btrfs and if when this happened? I'm reluctant to just "enable" btrfs send again if it did not work for a long time without testing. Maybe other
things also need adoption.

@ronnychevalier
Copy link
Author

Hmm, I just looked at btrfs-progs, and it seems that this change was in April 2017: kdave/btrfs-progs@4b59093#diff-081aa62fa583d438192c7be324495e42

If we want to exclude empty streams we need a way to tell the difference between
btrfs_read_and_process_send_stream() returning 1 because read_buf() did not
detect any data and read_and_process_cmd() returning 1 because honor_end_cmd was
set. Without introducing too many changes the best way to me seems to have
btrfs_read_and_process_send_stream() return -ENODATA in the first case.

@aschnell
Copy link
Member

Thanks for the explanation. We also have a bug for that in openSUSE (https://bugzilla.suse.com/show_bug.cgi?id=1111414).

@kobliha
Copy link
Member

kobliha commented Nov 6, 2018

... and it's planned to be planned hopefully in next development sprint. The problem did not make it to the current (just started) one.

@aschnell
Copy link
Member

Superseded by #472 due to merge conflicts.

Anyway, thanks for the patch.

@aschnell aschnell closed this Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants