Bug #53897
closeddiff-iterate can report holes when diffing against the beginning of time (fromsnapname == NULL)
0%
Description
rbd_diff_iterate/rbd_diff_iterate2() API documentation says:
If the source snapshot name is NULL, we interpret that as the beginning of time and return all allocated regions of the image.
Based on this sentence, the user can rightfully assume that the callback would only be invoked for allocated areas (i.e. with exists=true). However, if there is a snapshot which contains a discarded object, diff-iterate reports holes (exists=false) even when diffing against the beginning of time:
$ rbd create --size 16M foo $ sudo rbd map foo /dev/rbd0 $ sudo xfs_io -d -c 'pwrite -b 4M 0 16M' /dev/rbd0 wrote 16777216/16777216 bytes at offset 0 16 MiB, 4 ops; 0.4607 sec (34.726 MiB/sec and 8.6814 ops/sec) $ rbd snap create foo@snap Creating snap: 100% complete...done. $ rbd diff foo Offset Length Type 0 4194304 data 4194304 4194304 data 8388608 4194304 data 12582912 4194304 data $ blkdiscard -o 4M -l 8M /dev/rbd0 $ rbd diff foo Offset Length Type 0 4194304 data 4194304 4194304 zero 8388608 4194304 zero 12582912 4194304 data
Removing such a snapshot makes these "holes" disappear from the report:
$ rbd snap rm foo@snap Removing snap: 100% complete...done. $ rbd diff foo Offset Length Type 0 4194304 data 12582912 4194304 data
If nothing depends on this quirk, it should be fixed. Otherwise, the documentation needs to be updated to mention this.
Updated by Christopher Hoffman about 2 years ago
- Status changed from New to In Progress
- Assignee set to Christopher Hoffman
Updated by Christopher Hoffman over 1 year ago
I don't see anything within ceph code that explicitly relies on showing a zeroed entry. I would lean towards keeping this functionality as some user script may rely on this. If so, updating the documentation would be best.
Updated by Ilya Dryomov 6 months ago
- Blocks Feature #63341: improve rbd_diff_iterate2() performance in fast-diff mode added
Updated by Ilya Dryomov 6 months ago
- Assignee changed from Christopher Hoffman to Ilya Dryomov
- Priority changed from Normal to High
After a deep investigation prompted by https://tracker.ceph.com/issues/63341, I'm convinced that this is an implementation bug. The API documentation is correct (i.e. it's how the code was intended to behave).
Keeping the current behavior and instead adjusting documentation would also preclude some of the performance optimizations for fast-diff mode.
Updated by Ilya Dryomov 5 months ago
Christopher Hoffman wrote:
I don't see anything within ceph code that explicitly relies on showing a zeroed entry.
It turns out that deep-copy started relying on this in https://github.com/ceph/ceph/commit/e5a21e904142f8d728ba5de1eecf940b6b8329b5.
Updated by Ilya Dryomov 5 months ago
- Blocked by Bug #63654: [diff-iterate] ObjectListSnapsRequest's LIST_SNAPS_FLAG_WHOLE_OBJECT behavior is broken added
Updated by Ilya Dryomov 5 months ago
- Related to Bug #63719: [test] scribble()-based DiffIterate tests are too weak added
Updated by Ilya Dryomov 5 months ago
- Related to Bug #63770: [diff-iterate] discards that truncate aren't accounted for by ObjectListSnapsRequest added
Updated by Ilya Dryomov 4 months ago
- Status changed from In Progress to Pending Backport
- Backport set to pacific,quincy,reef
Updated by Backport Bot 4 months ago
- Copied to Backport #63846: pacific: diff-iterate can report holes when diffing against the beginning of time (fromsnapname == NULL) added
Updated by Backport Bot 4 months ago
- Copied to Backport #63847: quincy: diff-iterate can report holes when diffing against the beginning of time (fromsnapname == NULL) added
Updated by Backport Bot 4 months ago
- Copied to Backport #63848: reef: diff-iterate can report holes when diffing against the beginning of time (fromsnapname == NULL) added
Updated by Backport Bot 2 months ago
- Status changed from Pending Backport to Resolved
While running with --resolve-parent, the script "backport-create-issue" noticed that all backports of this issue are in status "Resolved" or "Rejected".