On Tue, Jan 26, 2021 at 7:19 PM Roman Anasal | BDSU
[off-list ref] wrote:

Am Montag, den 25.01.2021, 20:51 +0000 schrieb Filipe Manana:

quoted

On Mon, Jan 25, 2021 at 7:51 PM Roman Anasal [off-list ref]
wrote:

quoted

This is analogous to the preceding patch ("btrfs: send: fix invalid
commands for inodes with changed type but same gen") but for
changed
rdev:

When doing an incremental send, if a new inode has the same number
as an
inode in the parent subvolume, was created with the same generation
but
has differing rdev it will not be detected as changed and thus not
recreated. This will lead to incorrect results on the receiver
where the
inode will keep the rdev of the inode in the parent subvolume or
even
fail when also the ref is unchanged.

This case does not happen when doing incremental sends with
snapshots
that are kept read-only by the user all the time, but it may happen
if
- a snapshot was modified in the same transaction as its parent
after it
  was created
- the subvol used as parent was created independently from the sent
subvol

Example reproducers:

  # case 1: same ino at same path
  btrfs subvolume create subvol1
  btrfs subvolume create subvol2
  mknod subvol1/a c 1 3
  mknod subvol2/a c 1 5
  btrfs property set subvol1 ro true
  btrfs property set subvol2 ro true
  btrfs send -p subvol1 subvol2 | btrfs receive --dump

The produced tree state here is:
  |-- subvol1
  |   `-- a         (ino 257, c 1,3)
  |
  `-- subvol2
      `-- a         (ino 257, c 1,5)

Where subvol1/a and subvol2/a are character devices with differing
minor
numbers but same inode number and same generation.

Example output of the receive command:
  At subvol subvol2
  snapshot        ./subvol2                       uuid=7513941c-
4ef7-f847-b05e-4fdfe003af7b transid=9 parent_uuid=b66f015b-c226-
2548-9e39-048c7fdbec99 parent_transid=9
  utimes          ./subvol2/                      atime=2021-01-
25T17:14:36+0000 mtime=2021-01-25T17:14:36+0000 ctime=2021-01-
25T17:14:36+0000
  link            ./subvol2/a                     dest=a
  unlink          ./subvol2/a
  utimes          ./subvol2/                      atime=2021-01-
25T17:14:36+0000 mtime=2021-01-25T17:14:36+0000 ctime=2021-01-
25T17:14:36+0000
  utimes          ./subvol2/a                     atime=2021-01-
25T17:14:36+0000 mtime=2021-01-25T17:14:36+0000 ctime=2021-01-
25T17:14:36+0000

=> the `link` command causes the receiver to fail with:
   ERROR: link a -> a failed: File exists

Second example:
  # case 2: same ino at different path
  btrfs subvolume create subvol1
  btrfs subvolume create subvol2
  mknod subvol1/a c 1 3
  mknod subvol2/b c 1 5
  btrfs property set subvol1 ro true
  btrfs property set subvol2 ro true
  btrfs send -p subvol1 subvol2 | btrfs receive --dump

As I've told you before for the v1 patchset from a week or two ago,
this is not a supported scenario for incremental sends.
Incremental sends are meant to be used on RO snapshots of the same
subvolume, and those snapshots must never be changed after they were
created.

Incremental sends were simply not designed for these cases, and can
never be guaranteed to work with such cases.

The bug is not having incremental sends fail right away, with an
explicit error message, when the send and parent roots aren't RO
snapshots of the same subvolume.

I am sorry, I didn't want to anger you or to appear to be just stubborn
by posting this.

As I wrote in the cover letter I am aware that this is not a supported
use case and I understand that that makes the patches likely to be
rejected.

Ok, now I got the cover letter and the remaining v2 patches.
Vger has been having some lag this week, only got the mails during the
last evening.

Thanks.

As said the reason I _wrote_ the patches was simply to learn more about
the btrfs code and its internals and see if I would be able to
understand it enough. The reason I _submitted_ them was just to
document what I found out so others could have a look into it and just
in case it maybe useful at a later time.

I also don't want to claim that these will add full support for sending
unrelated roots - they don't! They only handle those very specific edge
cases I found, which are currently _possible_, although still not
supported.

I took a deeper look into the rest to see if it could be supported:
the comparing algorithm actually works fine, even with completely
unrelated subvolumes (i.e. btrfs_compare_trees, changed_cb,
changed_inode etc.), but the processing of the changes (i.e.
process_recorded_refs etc.) is heavily based on (ino, gen) as
identifying handle, which can not be changed without the high risk of
regression - just as you said in your earlier comments - since side
effects of any changes are hard to see or understand without a very
deep understanding of the whole code; which is why I didn't even try to
touch that parts.

I apologize if I appeared to be stubborn or ignorant of your feedback!
That really wasn't my intent.

quoted

quoted

The produced tree state here is:
  |-- subvol1
  |   `-- a         (ino 257, c 1,3)
  |
  `-- subvol2
      `-- b         (ino 257, c 1,5)

Where subvol1/a and subvol2/b are character devices with differing
minor
numbers but same inode number and same generation.

Example output of the receive command:
  At subvol subvol2
  snapshot        ./subvol2                       uuid=1c175819-
8b97-0046-a20e-5f95e37cbd40 transid=13 parent_uuid=bad4a908-21b4-
6f40-9a08-6b0768346725 parent_transid=13
  utimes          ./subvol2/                      atime=2021-01-
25T17:18:46+0000 mtime=2021-01-25T17:18:46+0000 ctime=2021-01-
25T17:18:46+0000
  link            ./subvol2/b                     dest=a
  unlink          ./subvol2/a
  utimes          ./subvol2/                      atime=2021-01-
25T17:18:46+0000 mtime=2021-01-25T17:18:46+0000 ctime=2021-01-
25T17:18:46+0000
  utimes          ./subvol2/b                     atime=2021-01-
25T17:18:46+0000 mtime=2021-01-25T17:18:46+0000 ctime=2021-01-
25T17:18:46+0000

=> subvol1/a is renamed to subvol2/b instead of recreated to
updated
   rdev which results in received subvol2/b having the wrong minor
   number:

  257 crw-r--r--. 1 root root 1, 3 Jan 25 17:18 subvol2/b

Signed-off-by: Roman Anasal <redacted>
---
v2:
  - add this patch to also handle changed rdev
---
 fs/btrfs/send.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index c8b1f441f..ef544525f 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c

@@ -6263,6 +6263,7 @@ static int changed_inode(struct send_ctx

*sctx,
        struct btrfs_inode_item *right_ii = NULL;
        u64 left_gen = 0;
        u64 right_gen = 0;
+       u64 left_rdev, right_rdev;
        u64 left_type, right_type;

        sctx->cur_ino = key->objectid;

@@ -6285,6 +6286,8 @@ static int changed_inode(struct send_ctx

*sctx,
                                struct btrfs_inode_item);
                left_gen = btrfs_inode_generation(sctx->left_path-

quoted

nodes[0],

                                left_ii);
+               left_rdev = btrfs_inode_rdev(sctx->left_path-

quoted

nodes[0],

+                               left_ii);
        } else {
                right_ii = btrfs_item_ptr(sctx->right_path-

quoted

nodes[0],

                                sctx->right_path->slots[0],

@@ -6300,6 +6303,9 @@ static int changed_inode(struct send_ctx

*sctx,
                right_gen = btrfs_inode_generation(sctx-

quoted

right_path->nodes[0],

                                right_ii);

+               right_rdev = btrfs_inode_rdev(sctx->right_path-

quoted

nodes[0],

+                               right_ii);
+
                left_type = S_IFMT & btrfs_inode_mode(
                                sctx->left_path->nodes[0],
left_ii);
                right_type = S_IFMT & btrfs_inode_mode(

@@ -6310,7 +6316,8 @@ static int changed_inode(struct send_ctx

*sctx,
                 * the inode as deleted+reused because it would
generate a
                 * stream that tries to delete/mkdir the root dir.
                 */
-               if ((left_gen != right_gen || left_type !=
right_type) &&
+               if ((left_gen != right_gen || left_type !=
right_type ||
+                   left_rdev != right_rdev) &&
                    sctx->cur_ino != BTRFS_FIRST_FREE_OBJECTID)
                        sctx->cur_inode_recreated = 1;
        }

@@ -6350,8 +6357,7 @@ static int changed_inode(struct send_ctx

*sctx,
                                sctx->left_path->nodes[0],
left_ii);
                sctx->cur_inode_mode = btrfs_inode_mode(
                                sctx->left_path->nodes[0],
left_ii);
-               sctx->cur_inode_rdev = btrfs_inode_rdev(
-                               sctx->left_path->nodes[0],
left_ii);
+               sctx->cur_inode_rdev = left_rdev;
                if (sctx->cur_ino != BTRFS_FIRST_FREE_OBJECTID)
                        ret = send_create_inode_if_needed(sctx);
        } else if (result == BTRFS_COMPARE_TREE_DELETED) {

@@ -6396,8 +6402,7 @@ static int changed_inode(struct send_ctx

*sctx,
                                        sctx->left_path->nodes[0],
left_ii);
                        sctx->cur_inode_mode = btrfs_inode_mode(
                                        sctx->left_path->nodes[0],
left_ii);
-                       sctx->cur_inode_rdev = btrfs_inode_rdev(
-                                       sctx->left_path->nodes[0],
left_ii);
+                       sctx->cur_inode_rdev = left_rdev;
                        ret = send_create_inode_if_needed(sctx);
                        if (ret < 0)
                                goto out;
--
2.26.2

-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”