| Message ID | 20221016153346.2794-1-bingjingc@synology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: send: fix send failure of a subcase of orphan inodes | expand |
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.
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.