btrfs: send: fix send failure of a subcase of orphan inodes

  From: BingJing Chang <bingjingc@synology.com>

Commit 9ed0a72e5b35 ("btrfs: send: fix failures when processing inodes with
no links") tries to fix all incremental send cases of orphan inodes the
send operation will meet. However, there's still a bug causing the corner
subcase fails with a ENOENT error.

Here's shortened steps of that subcase:

  $ btrfs subvolume create vol
  $ touch vol/foo

  $ btrfs subvolume snapshot -r vol snap1
  $ btrfs subvolume snapshot -r vol snap2

  # Turn the second snapshot to RW mode and delete the file while
  # holding an open file descriptor on it
  $ btrfs property set snap2 ro false
  $ exec 73<snap2/foo
  $ rm snap2/foo

  # Set the second snapshot back to RO mode and do an incremental send
  # with an unusal reverse order
  $ btrfs property set snap2 ro true
  $ btrfs send -p snap2 snap1 > /dev/null
  At subvol snap1
  ERROR: send ioctl failed with -2: No such file or directory

It's subcase 3 of BTRFS_COMPARE_TREE_CHANGED in the commit 9ed0a72e5b35
("btrfs: send: fix failures when processing inodes with no links"). And
it's not a common case. We still have not met it in the real world.
Theoretically, this case can happen in a batch cascading snapshot backup.
In cascading backups, the receive operation in the middle may cause orphan
inodes to appear because of the open file descriptors on the snapshot files
during receiving. And if we don't do the batch snapshot backups in their
creation order, then we can have an inode, which is an orphan in the parent
snapshot but refers to a file in the send snapshot. Since an orphan inode
has no paths, the send operation will fail with a ENOENT error if it
tries to generate a path for it.

In that patch, this subcase will be treated as an inode with a new
generation. However, when the routine tries to delete the old paths in
the parent snapshot, the function process_all_refs() doesn't check whether
there are paths recorded or not before it calls the function
process_recorded_refs(). And the function process_recorded_refs() try
to get the first path in the parent snapshot in the beginning. Since it has
no paths in the parent snapshot, the send operation fails.

To fix this, we can easily put a link count check to avoid entering the
deletion routine like what we do a link count check to avoid creating a
new one. Moreover, we can assume that the function process_all_refs()
can always collect references to process because we know it has a
positive link count.

Signed-off-by: BingJing Chang <bingjingc@synology.com>
---
 fs/btrfs/send.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)
  

On Sun, Oct 16, 2022 at 4:34 PM bingjingc <bingjingc@synology.com> wrote:
>
> From: BingJing Chang <bingjingc@synology.com>
>
> Commit 9ed0a72e5b35 ("btrfs: send: fix failures when processing inodes with
> no links") tries to fix all incremental send cases of orphan inodes the
> send operation will meet. However, there's still a bug causing the corner
> subcase fails with a ENOENT error.
>
> Here's shortened steps of that subcase:
>
>   $ btrfs subvolume create vol
>   $ touch vol/foo
>
>   $ btrfs subvolume snapshot -r vol snap1
>   $ btrfs subvolume snapshot -r vol snap2
>
>   # Turn the second snapshot to RW mode and delete the file while
>   # holding an open file descriptor on it
>   $ btrfs property set snap2 ro false
>   $ exec 73<snap2/foo
>   $ rm snap2/foo
>
>   # Set the second snapshot back to RO mode and do an incremental send
>   # with an unusal reverse order
>   $ btrfs property set snap2 ro true
>   $ btrfs send -p snap2 snap1 > /dev/null
>   At subvol snap1
>   ERROR: send ioctl failed with -2: No such file or directory
>
> It's subcase 3 of BTRFS_COMPARE_TREE_CHANGED in the commit 9ed0a72e5b35
> ("btrfs: send: fix failures when processing inodes with no links"). And
> it's not a common case. We still have not met it in the real world.
> Theoretically, this case can happen in a batch cascading snapshot backup.
> In cascading backups, the receive operation in the middle may cause orphan
> inodes to appear because of the open file descriptors on the snapshot files
> during receiving. And if we don't do the batch snapshot backups in their
> creation order, then we can have an inode, which is an orphan in the parent
> snapshot but refers to a file in the send snapshot. Since an orphan inode
> has no paths, the send operation will fail with a ENOENT error if it
> tries to generate a path for it.
>
> In that patch, this subcase will be treated as an inode with a new
> generation. However, when the routine tries to delete the old paths in
> the parent snapshot, the function process_all_refs() doesn't check whether
> there are paths recorded or not before it calls the function
> process_recorded_refs(). And the function process_recorded_refs() try
> to get the first path in the parent snapshot in the beginning. Since it has
> no paths in the parent snapshot, the send operation fails.
>
> To fix this, we can easily put a link count check to avoid entering the
> deletion routine like what we do a link count check to avoid creating a
> new one. Moreover, we can assume that the function process_all_refs()
> can always collect references to process because we know it has a
> positive link count.
>
> Signed-off-by: BingJing Chang <bingjingc@synology.com>

Looks good.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Please add a test case for fstests later if you can.
Thanks!

> ---
>  fs/btrfs/send.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 4ef4167072b8..1568fa977ca1 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -6665,17 +6665,19 @@ static int changed_inode(struct send_ctx *sctx,
>                         /*
>                          * First, process the inode as if it was deleted.
>                          */
> -                       sctx->cur_inode_gen = right_gen;
> -                       sctx->cur_inode_new = false;
> -                       sctx->cur_inode_deleted = true;
> -                       sctx->cur_inode_size = btrfs_inode_size(
> -                                       sctx->right_path->nodes[0], right_ii);
> -                       sctx->cur_inode_mode = btrfs_inode_mode(
> -                                       sctx->right_path->nodes[0], right_ii);
> -                       ret = process_all_refs(sctx,
> -                                       BTRFS_COMPARE_TREE_DELETED);
> -                       if (ret < 0)
> -                               goto out;
> +                       if (old_nlinks > 0) {
> +                               sctx->cur_inode_gen = right_gen;
> +                               sctx->cur_inode_new = false;
> +                               sctx->cur_inode_deleted = true;
> +                               sctx->cur_inode_size = btrfs_inode_size(
> +                                               sctx->right_path->nodes[0], right_ii);
> +                               sctx->cur_inode_mode = btrfs_inode_mode(
> +                                               sctx->right_path->nodes[0], right_ii);
> +                               ret = process_all_refs(sctx,
> +                                               BTRFS_COMPARE_TREE_DELETED);
> +                               if (ret < 0)
> +                                       goto out;
> +                       }
>
>                         /*
>                          * Now process the inode as if it was new.
> --
> 2.37.1
>
  

On Sun, Oct 16, 2022 at 11:33:46PM +0800, bingjingc wrote:
> From: BingJing Chang <bingjingc@synology.com>
> 
> Commit 9ed0a72e5b35 ("btrfs: send: fix failures when processing inodes with
> no links") tries to fix all incremental send cases of orphan inodes the
> send operation will meet. However, there's still a bug causing the corner
> subcase fails with a ENOENT error.
> 
> Here's shortened steps of that subcase:
> 
>   $ btrfs subvolume create vol
>   $ touch vol/foo
> 
>   $ btrfs subvolume snapshot -r vol snap1
>   $ btrfs subvolume snapshot -r vol snap2
> 
>   # Turn the second snapshot to RW mode and delete the file while
>   # holding an open file descriptor on it
>   $ btrfs property set snap2 ro false
>   $ exec 73<snap2/foo
>   $ rm snap2/foo
> 
>   # Set the second snapshot back to RO mode and do an incremental send
>   # with an unusal reverse order
>   $ btrfs property set snap2 ro true
>   $ btrfs send -p snap2 snap1 > /dev/null
>   At subvol snap1
>   ERROR: send ioctl failed with -2: No such file or directory
> 
> It's subcase 3 of BTRFS_COMPARE_TREE_CHANGED in the commit 9ed0a72e5b35
> ("btrfs: send: fix failures when processing inodes with no links"). And
> it's not a common case. We still have not met it in the real world.
> Theoretically, this case can happen in a batch cascading snapshot backup.
> In cascading backups, the receive operation in the middle may cause orphan
> inodes to appear because of the open file descriptors on the snapshot files
> during receiving. And if we don't do the batch snapshot backups in their
> creation order, then we can have an inode, which is an orphan in the parent
> snapshot but refers to a file in the send snapshot. Since an orphan inode
> has no paths, the send operation will fail with a ENOENT error if it
> tries to generate a path for it.
> 
> In that patch, this subcase will be treated as an inode with a new
> generation. However, when the routine tries to delete the old paths in
> the parent snapshot, the function process_all_refs() doesn't check whether
> there are paths recorded or not before it calls the function
> process_recorded_refs(). And the function process_recorded_refs() try
> to get the first path in the parent snapshot in the beginning. Since it has
> no paths in the parent snapshot, the send operation fails.
> 
> To fix this, we can easily put a link count check to avoid entering the
> deletion routine like what we do a link count check to avoid creating a
> new one. Moreover, we can assume that the function process_all_refs()
> can always collect references to process because we know it has a
> positive link count.
> 
> Signed-off-by: BingJing Chang <bingjingc@synology.com>

Added to misc-next, thanks.