linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] btrfs: send: correctly recreate changed inodes
@ 2021-01-11 19:02 Roman Anasal
  2021-01-11 19:02 ` [PATCH 1/2] btrfs: send: rename send_ctx.cur_inode_new_gen to cur_inode_recreated Roman Anasal
  2021-01-11 19:02 ` [PATCH 2/2] btrfs: send: fix invalid commands for inodes with changed type but same gen Roman Anasal
  0 siblings, 2 replies; 15+ messages in thread
From: Roman Anasal @ 2021-01-11 19:02 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Roman Anasal

Trying to incremental send and receive some subvolumes I repeatedly ran into
errors with btrfs-receive failing. Taking a deeper look into the issue showed
that btrfs-send was producing (semantically) invalid command streams. I could
pin down the conditions of this happening to having inodes with the same number
and the same generation but different inode type in the parent and the sent
subvolume.

The cause of this was that the kernel code for btrfs-send did not check if the
type of the inode changed but only recreated the inode when the generation
changed, too. Having the two inodes originate in the same generation though
would produce a command stream that causes the receiver to fail.

This small patch set adds the check for the same type and causes a deleted and
create of the inode if necessary.
For this the first patch renames send_ctx.cur_inode_new_gen to
cur_inode_recreated because that is what this currently _actually_ is used for:
detecting whether an inode was/should be recreated.
I deemed this refactoring/renaming to be appropriate because the second patch
then adds another condition that will set cur_inode_recreated - i.e. if the
inode types differ.


Looking through the code and developing this patch I also identified an
assumption that seems to be hard coded into many places and that probably caused
other bugs, too, e.g. [1]:

That people would only ever incremental send read-only snapshots of the provided
parent snapshot that they're based off.

But this assumption fails/causes bugs when send/receive is used with:
1. snapshots that were modified after their creation
2. subvolumes that were created independently and are not descendants of each
   other

(Although 2. may look like total nonsense it actually makes sense for
independent subvolumes that share some extents e.g. by cp --reflink or some
other means of deduplication.)

Thus I suspect there to be further hidden bugs for very rare edge cases
especially where the generation number is used to check for differences -
because taking 1./2. into account the generations between the subvolume and
parent may arbitrarily be smaller, equal or greater and as such does not qualify
as a good indicator for change detection.
The same is true for the inode numbers.

This makes it also connected to [1]:
"btrfs: send: fix wrong file path when there is an inode with a pending rmdir"

[1] https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git/commit/?id=0b3f407e6728d990ae1630a02c7b952c21c288d3


Roman Anasal (2):
  btrfs: send: rename send_ctx.cur_inode_new_gen to cur_inode_recreated
  btrfs: send: fix invalid commands for inodes with changed type but
    same gen

 fs/btrfs/send.c | 47 +++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

--
2.26.2

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-01-12 15:09 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-11 19:02 [PATCH 0/2] btrfs: send: correctly recreate changed inodes Roman Anasal
2021-01-11 19:02 ` [PATCH 1/2] btrfs: send: rename send_ctx.cur_inode_new_gen to cur_inode_recreated Roman Anasal
2021-01-11 19:02 ` [PATCH 2/2] btrfs: send: fix invalid commands for inodes with changed type but same gen Roman Anasal
2021-01-11 19:30   ` Andrei Borzenkov
2021-01-11 20:53     ` Roman Anasal | BDSU
2021-01-12 11:27       ` Filipe Manana
2021-01-12 12:07         ` Graham Cobb
2021-01-12 12:30           ` Filipe Manana
2021-01-12 13:10         ` Roman Anasal | BDSU
2021-01-12 13:54           ` Filipe Manana
2021-01-12 14:37             ` Roman Anasal | BDSU
2021-01-12 15:08               ` Filipe Manana
2021-01-11 21:15   ` David Sterba
2021-01-11 21:31     ` Roman Anasal | BDSU
2021-01-11 21:19   ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).