LinuxLists.cc - [PATCH v2 0/2] btrfs: send: fix failures when processing inodes with no links

2022-08-11 10:17:33

Subject: [PATCH v2 0/2] btrfs: send: fix failures when processing inodes with no links

2022-08-11 10:17:42

Subject: [PATCH v2 1/2] btrfs: send: refactor get_inode_info()

From: BingJing Chang <[email protected]>

Refactor get_inode_info() to populate all wanted fields on an output
structure. Besides, also introduce a helper function called
get_inode_gen(), which is majorly used.

Reviewed-by: Robbie Ko <[email protected]>
Signed-off-by: BingJing Chang <[email protected]>
---
fs/btrfs/send.c | 154 +++++++++++++++++++++++++-----------------------
1 file changed, 79 insertions(+), 75 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index e7671afcee4f..f8d77a33b9b7 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -842,17 +842,31 @@ static int send_rmdir(struct send_ctx *sctx, struct fs_path *path)
return ret;
}

+struct btrfs_inode_info {
+ u64 size;
+ u64 gen;
+ u64 mode;
+ u64 uid;
+ u64 gid;
+ u64 rdev;
+ u64 attr;
+};
+
/*
* Helper function to retrieve some fields from an inode item.
*/
-static int __get_inode_info(struct btrfs_root *root, struct btrfs_path *path,
- u64 ino, u64 *size, u64 *gen, u64 *mode, u64 *uid,
- u64 *gid, u64 *rdev, u64 *fileattr)
+static int get_inode_info(struct btrfs_root *root, u64 ino,
+ struct btrfs_inode_info *info)
{
int ret;
+ struct btrfs_path *path;
struct btrfs_inode_item *ii;
struct btrfs_key key;

+ path = alloc_path_for_send();
+ if (!path)
+ return -ENOMEM;
+
key.objectid = ino;
key.type = BTRFS_INODE_ITEM_KEY;
key.offset = 0;
@@ -860,47 +874,42 @@ static int __get_inode_info(struct btrfs_root *root, struct btrfs_path *path,
if (ret) {
if (ret > 0)
ret = -ENOENT;
- return ret;
+ goto out;
}

+ if (!info)
+ goto out;
+
ii = btrfs_item_ptr(path->nodes[0], path->slots[0],
struct btrfs_inode_item);
- if (size)
- *size = btrfs_inode_size(path->nodes[0], ii);
- if (gen)
- *gen = btrfs_inode_generation(path->nodes[0], ii);
- if (mode)
- *mode = btrfs_inode_mode(path->nodes[0], ii);
- if (uid)
- *uid = btrfs_inode_uid(path->nodes[0], ii);
- if (gid)
- *gid = btrfs_inode_gid(path->nodes[0], ii);
- if (rdev)
- *rdev = btrfs_inode_rdev(path->nodes[0], ii);
+ info->size = btrfs_inode_size(path->nodes[0], ii);
+ info->gen = btrfs_inode_generation(path->nodes[0], ii);
+ info->mode = btrfs_inode_mode(path->nodes[0], ii);
+ info->uid = btrfs_inode_uid(path->nodes[0], ii);
+ info->gid = btrfs_inode_gid(path->nodes[0], ii);
+ info->rdev = btrfs_inode_rdev(path->nodes[0], ii);
/*
* Transfer the unchanged u64 value of btrfs_inode_item::flags, that's
* otherwise logically split to 32/32 parts.
*/
- if (fileattr)
- *fileattr = btrfs_inode_flags(path->nodes[0], ii);
+ info->attr = btrfs_inode_flags(path->nodes[0], ii);

+out:
+ btrfs_free_path(path);
return ret;
}

-static int get_inode_info(struct btrfs_root *root,
- u64 ino, u64 *size, u64 *gen,
- u64 *mode, u64 *uid, u64 *gid,
- u64 *rdev, u64 *fileattr)
+static int get_inode_gen(struct btrfs_root *root, u64 ino, u64 *gen)
{
- struct btrfs_path *path;
int ret;
+ struct btrfs_inode_info info;

- path = alloc_path_for_send();
- if (!path)
- return -ENOMEM;
- ret = __get_inode_info(root, path, ino, size, gen, mode, uid, gid,
- rdev, fileattr);
- btrfs_free_path(path);
+ if (!gen)
+ return -EPERM;
+
+ ret = get_inode_info(root, ino, &info);
+ if (!ret)
+ *gen = info.gen;
return ret;
}

@@ -1644,8 +1653,7 @@ static int get_cur_inode_state(struct send_ctx *sctx, u64 ino, u64 gen)
u64 left_gen;
u64 right_gen;

- ret = get_inode_info(sctx->send_root, ino, NULL, &left_gen, NULL, NULL,
- NULL, NULL, NULL);
+ ret = get_inode_gen(sctx->send_root, ino, &left_gen);
if (ret < 0 && ret != -ENOENT)
goto out;
left_ret = ret;
@@ -1653,8 +1661,7 @@ static int get_cur_inode_state(struct send_ctx *sctx, u64 ino, u64 gen)
if (!sctx->parent_root) {
right_ret = -ENOENT;
} else {
- ret = get_inode_info(sctx->parent_root, ino, NULL, &right_gen,
- NULL, NULL, NULL, NULL, NULL);
+ ret = get_inode_gen(sctx->parent_root, ino, &right_gen);
if (ret < 0 && ret != -ENOENT)
goto out;
right_ret = ret;
@@ -1816,8 +1823,7 @@ static int get_first_ref(struct btrfs_root *root, u64 ino,
btrfs_release_path(path);

if (dir_gen) {
- ret = get_inode_info(root, parent_dir, NULL, dir_gen, NULL,
- NULL, NULL, NULL, NULL);
+ ret = get_inode_gen(root, parent_dir, dir_gen);
if (ret < 0)
goto out;
}
@@ -1874,6 +1880,7 @@ static int will_overwrite_ref(struct send_ctx *sctx, u64 dir, u64 dir_gen,
int ret = 0;
u64 gen;
u64 other_inode = 0;
+ struct btrfs_inode_info info;

if (!sctx->parent_root)
goto out;
@@ -1888,8 +1895,7 @@ static int will_overwrite_ref(struct send_ctx *sctx, u64 dir, u64 dir_gen,
* and we can just unlink this entry.
*/
if (sctx->parent_root && dir != BTRFS_FIRST_FREE_OBJECTID) {
- ret = get_inode_info(sctx->parent_root, dir, NULL, &gen, NULL,
- NULL, NULL, NULL, NULL);
+ ret = get_inode_gen(sctx->parent_root, dir, &gen);
if (ret < 0 && ret != -ENOENT)
goto out;
if (ret) {
@@ -1916,13 +1922,14 @@ static int will_overwrite_ref(struct send_ctx *sctx, u64 dir, u64 dir_gen,
*/
if (other_inode > sctx->send_progress ||
is_waiting_for_move(sctx, other_inode)) {
- ret = get_inode_info(sctx->parent_root, other_inode, NULL,
- who_gen, who_mode, NULL, NULL, NULL, NULL);
+ ret = get_inode_info(sctx->parent_root, other_inode, &info);
if (ret < 0)
goto out;

ret = 1;
*who_ino = other_inode;
+ *who_gen = info.gen;
+ *who_mode = info.mode;
} else {
ret = 0;
}
@@ -1955,8 +1962,7 @@ static int did_overwrite_ref(struct send_ctx *sctx,
goto out;

if (dir != BTRFS_FIRST_FREE_OBJECTID) {
- ret = get_inode_info(sctx->send_root, dir, NULL, &gen, NULL,
- NULL, NULL, NULL, NULL);
+ ret = get_inode_gen(sctx->send_root, dir, &gen);
if (ret < 0 && ret != -ENOENT)
goto out;
if (ret) {
@@ -1978,8 +1984,7 @@ static int did_overwrite_ref(struct send_ctx *sctx,
goto out;
}

- ret = get_inode_info(sctx->send_root, ow_inode, NULL, &gen, NULL, NULL,
- NULL, NULL, NULL);
+ ret = get_inode_gen(sctx->send_root, ow_inode, &gen);
if (ret < 0)
goto out;

@@ -2645,6 +2650,7 @@ static int send_create_inode(struct send_ctx *sctx, u64 ino)
int ret = 0;
struct fs_path *p;
int cmd;
+ struct btrfs_inode_info info;
u64 gen;
u64 mode;
u64 rdev;
@@ -2656,10 +2662,12 @@ static int send_create_inode(struct send_ctx *sctx, u64 ino)
return -ENOMEM;

if (ino != sctx->cur_ino) {
- ret = get_inode_info(sctx->send_root, ino, NULL, &gen, &mode,
- NULL, NULL, &rdev, NULL);
+ ret = get_inode_info(sctx->send_root, ino, &info);
if (ret < 0)
goto out;
+ gen = info.gen;
+ mode = info.mode;
+ rdev = info.rdev;
} else {
gen = sctx->cur_inode_gen;
mode = sctx->cur_inode_mode;
@@ -3359,8 +3367,7 @@ static int apply_dir_move(struct send_ctx *sctx, struct pending_dir_move *pm)
/*
* The parent inode might have been deleted in the send snapshot
*/
- ret = get_inode_info(sctx->send_root, cur->dir, NULL,
- NULL, NULL, NULL, NULL, NULL, NULL);
+ ret = get_inode_info(sctx->send_root, cur->dir, NULL);
if (ret == -ENOENT) {
ret = 0;
continue;
@@ -3534,12 +3541,10 @@ static int wait_for_dest_dir_move(struct send_ctx *sctx,
goto out;
}

- ret = get_inode_info(sctx->parent_root, di_key.objectid, NULL,
- &left_gen, NULL, NULL, NULL, NULL, NULL);
+ ret = get_inode_gen(sctx->parent_root, di_key.objectid, &left_gen);
if (ret < 0)
goto out;
- ret = get_inode_info(sctx->send_root, di_key.objectid, NULL,
- &right_gen, NULL, NULL, NULL, NULL, NULL);
+ ret = get_inode_gen(sctx->send_root, di_key.objectid, &right_gen);
if (ret < 0) {
if (ret == -ENOENT)
ret = 0;
@@ -3669,8 +3674,7 @@ static int is_ancestor(struct btrfs_root *root,
cur_offset = item_size;
}

- ret = get_inode_info(root, parent, NULL, &parent_gen,
- NULL, NULL, NULL, NULL, NULL);
+ ret = get_inode_gen(root, parent, &parent_gen);
if (ret < 0)
goto out;
ret = check_ino_in_path(root, ino1, ino1_gen,
@@ -3760,9 +3764,8 @@ static int wait_for_parent_move(struct send_ctx *sctx,
memcmp(path_before->start, path_after->start, len1))) {
u64 parent_ino_gen;

- ret = get_inode_info(sctx->parent_root, ino, NULL,
- &parent_ino_gen, NULL, NULL, NULL,
- NULL, NULL);
+ ret = get_inode_gen(sctx->parent_root, ino,
+ &parent_ino_gen);
if (ret < 0)
goto out;
if (ino_gen == parent_ino_gen) {
@@ -4441,8 +4444,7 @@ static int record_new_ref_if_needed(int num, u64 dir, int index,
struct recorded_ref *ref;
u64 dir_gen;

- ret = get_inode_info(sctx->send_root, dir, NULL, &dir_gen, NULL,
- NULL, NULL, NULL, NULL);
+ ret = get_inode_gen(sctx->send_root, dir, &dir_gen);
if (ret < 0)
goto out;

@@ -4472,8 +4474,7 @@ static int record_deleted_ref_if_needed(int num, u64 dir, int index,
struct recorded_ref *ref;
u64 dir_gen;

- ret = get_inode_info(sctx->parent_root, dir, NULL, &dir_gen, NULL,
- NULL, NULL, NULL, NULL);
+ ret = get_inode_gen(sctx->parent_root, dir, &dir_gen);
if (ret < 0)
goto out;

@@ -5056,8 +5057,7 @@ static int send_clone(struct send_ctx *sctx,
TLV_PUT_PATH(sctx, BTRFS_SEND_A_PATH, p);

if (clone_root->root == sctx->send_root) {
- ret = get_inode_info(sctx->send_root, clone_root->ino, NULL,
- &gen, NULL, NULL, NULL, NULL, NULL);
+ ret = get_inode_gen(sctx->send_root, clone_root->ino, &gen);
if (ret < 0)
goto out;
ret = get_cur_path(sctx, clone_root->ino, gen, p);
@@ -5536,6 +5536,7 @@ static int clone_range(struct send_ctx *sctx, struct btrfs_path *dst_path,
struct btrfs_path *path;
struct btrfs_key key;
int ret;
+ struct btrfs_inode_info info;
u64 clone_src_i_size = 0;

/*
@@ -5565,12 +5566,11 @@ static int clone_range(struct send_ctx *sctx, struct btrfs_path *dst_path,
* There are inodes that have extents that lie behind its i_size. Don't
* accept clones from these extents.
*/
- ret = __get_inode_info(clone_root->root, path, clone_root->ino,
- &clone_src_i_size, NULL, NULL, NULL, NULL, NULL,
- NULL);
+ ret = get_inode_info(clone_root->root, clone_root->ino, &info);
btrfs_release_path(path);
if (ret < 0)
goto out;
+ clone_src_i_size = info.size;

/*
* We can't send a clone operation for the entire range if we find
@@ -6259,6 +6259,7 @@ static int process_recorded_refs_if_needed(struct send_ctx *sctx, int at_end,
static int finish_inode_if_needed(struct send_ctx *sctx, int at_end)
{
int ret = 0;
+ struct btrfs_inode_info info;
u64 left_mode;
u64 left_uid;
u64 left_gid;
@@ -6301,11 +6302,13 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end)
goto out;
if (!at_end && sctx->cmp_key->objectid == sctx->cur_ino)
goto out;
-
- ret = get_inode_info(sctx->send_root, sctx->cur_ino, NULL, NULL,
- &left_mode, &left_uid, &left_gid, NULL, &left_fileattr);
+ ret = get_inode_info(sctx->send_root, sctx->cur_ino, &info);
if (ret < 0)
goto out;
+ left_mode = info.mode;
+ left_uid = info.uid;
+ left_gid = info.gid;
+ left_fileattr = info.attr;

if (!sctx->parent_root || sctx->cur_inode_new) {
need_chown = 1;
@@ -6316,11 +6319,14 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end)
} else {
u64 old_size;

- ret = get_inode_info(sctx->parent_root, sctx->cur_ino,
- &old_size, NULL, &right_mode, &right_uid,
- &right_gid, NULL, &right_fileattr);
+ ret = get_inode_info(sctx->parent_root, sctx->cur_ino, &info);
if (ret < 0)
goto out;
+ old_size = info.size;
+ right_mode = info.mode;
+ right_uid = info.uid;
+ right_gid = info.gid;
+ right_fileattr = info.attr;

if (left_uid != right_uid || left_gid != right_gid)
need_chown = 1;
@@ -6790,13 +6796,11 @@ static int dir_changed(struct send_ctx *sctx, u64 dir)
u64 orig_gen, new_gen;
int ret;

- ret = get_inode_info(sctx->send_root, dir, NULL, &new_gen, NULL, NULL,
- NULL, NULL, NULL);
+ ret = get_inode_gen(sctx->send_root, dir, &new_gen);
if (ret)
return ret;

- ret = get_inode_info(sctx->parent_root, dir, NULL, &orig_gen, NULL,
- NULL, NULL, NULL, NULL);
+ ret = get_inode_gen(sctx->parent_root, dir, &orig_gen);
if (ret)
return ret;

--
2.37.1

2022-08-11 10:17:59

by bingjingc

[permalink] [raw]

Subject: [PATCH v2 2/2] btrfs: send: fix failures when processing inodes with no links

From: BingJing Chang <[email protected]>

There is a bug causing send failures when processing an orphan directory
with no links. In commit 46b2f4590aab ("Btrfs: fix send failure when root
has deleted files still open")', the orphan inode issue was addressed. The
send operation fails with a ENOENT error because of any attempts to
generate a path for the inode with a link count of zero. Therefore, in that
patch, sctx->ignore_cur_inode was introduced to be set if the current inode
has a link count of zero for bypassing some unnecessary steps. And a helper
function btrfs_unlink_all_paths() was introduced and called to clean up old
paths found in the parent snapshot. However, not only regular files but
also directories can be orphan inodes. So if the send operation meets an
orphan directory, it will issue a wrong unlink command for that directory
now. Soon the receive operation fails with a EISDIR error. Besides, the
send operation also fails with a ENOENT error later when it tries to
generate a path of it.

Similar example but making an orphan dir for an incremental send:

$ btrfs subvolume create vol
$ mkdir vol/dir
$ touch vol/dir/foo

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

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

# Set the second snapshot back to RO mode and do an incremental send.
$ btrfs property set snap2 ro true
$ mkdir receive_dir
$ btrfs send snap2 -p snap1 | btrfs receive receive_dir/
At subvol snap2
At snapshot snap2
ERROR: send ioctl failed with -2: No such file or directory
ERROR: unlink dir failed. Is a directory

Actually, orphan inodes are more common use cases in cascading backups.
(Please see the illustration below.) In a cascading backup, a user wants
to replicate a couple of snapshots from Machine A to Machine B and from
Machine B to Machine C. Machine B doesn't take any RO snapshots for
sending. All a receiver does is create an RW snapshot of its parent
snapshot, apply the send stream and turn it into RO mode at the end. Even
if all paths of some inodes are deleted in applying the send stream, these
inodes would not be deleted and become orphans after changing the subvolume
from RW to RO. Moreover, orphan inodes can occur not only in send snapshots
but also in parent snapshots because Machine B may do a batch replication
of a couple of snapshots.

An illustration for cascading backups:
Machine A (snapshot {1..n}) --> Machine B --> Machine C

The intuition to solve the problem is to delete all the items of orphan
inodes before using these snapshots for sending. I used to think that the
reasonable timing for doing that is during the ioctl of changing the
subvolume from RW to RO because it sounds good that we will not modify the
fs tree of a RO snapshot anymore. However, attempting to do the orphan
cleanup in the ioctl would be pointless. Because if someone is holding an
open file descriptor on the inode, the reference count of the inode will
never drop to 0. Then iput() cannot trigger eviction, which finally deletes
all the items of it. So we try to extend the original patch to handle
orphans in send/parent snapshots. Here are several cases that need to be
considered:

Case 1: BTRFS_COMPARE_TREE_NEW
| send snapshot | action
--------------------------------
nlink | 0 | ignore

In case 1, when we get a BTRFS_COMPARE_TREE_NEW tree comparison result,
it means that a new inode is found in the send snapshot and it doesn't
appear in the parent snapshot. Since this inode has a link count of zero
(It's an orphan and there're no paths for it.), we can leverage
sctx->ignore_cur_inode in the original patch to prevent it from being
created.

Case 2: BTRFS_COMPARE_TREE_DELETED
| parent snapshot | action
----------------------------------
nlink | 0 | as usual

In case 2, when we get a BTRFS_COMPARE_TREE_DELETED tree comparison
result, it means that the inode only appears in the parent snapshot.
As usual, the send operation will try to delete all its paths. However,
this inode has a link count of zero, so no paths of it will be found. No
deletion operations will be issued. We don't need to change any logic.

Case 3: BTRFS_COMPARE_TREE_CHANGED
| | parent snapshot | send snapshot | action
-----------------------------------------------------------------------
subcase 1 | nlink | 0 | 0 | ignore
subcase 2 | nlink | >0 | 0 | new_gen(deletion)
subcase 3 | nlink | 0 | >0 | new_gen(creation)

In case 3, when we get a BTRFS_COMPARE_TREE_CHANGED tree comparison result,
it means that the inode appears in both snapshots. Here're three subcases.

First, if the inode has link counts of zero in both snapshots. Since there
are no paths for this inode in (source/destination) parent snapshots and we
don't care about whether there is also an orphan inode in destination or
not, we can set sctx->ignore_cur_inode on to prevent it from being created.

For the second and the third subcases, if there're paths in one snapshot
and there're no paths in the other snapshot for this inode. We can treat
this inode as a new generation. We can also leverage the logic handling a
new generation of an inode with small adjustments. Then it will delete all
old paths and create a new inode with new attributes and paths only when
there's a positive link count in the send snapshot. In subcase 2, the
send operation only needs to delete all old paths as in the parent
snapshot. But it may require more operations for a directory to remove its
old paths. If a not-empty directory is going to be deleted (because it has
a link count of zero in the send snapshot) but there're files/directories
with bigger inode numbers under it, the send operation will need to rename
it to its orphan name first. After processing and deleting the last item
under this directory, the send operation will check this directory, aka
the parent directory of the last item, again and issue a rmdir operation
to remove it finally. Therefore, we also need to treat inodes with a link
count of zero as if they didn't exist in get_cur_inode_state(), which is
used in process_recorded_refs(). By doing this, when reviewing a directory
with orphan names after the last item under it has been deleted, the send
operation now can properly issue a rmdir operation. Otherwise, without
doing this, the orphan directory with an orphan name would be kept here
at the end due to the existing inode with a link count of zero being found.
In subcase 3, as in case 2, no old paths would be found, so no deletion
operations will be issued. The send operation will only create a new one
for that inode.

Note that subcase 3 is not a common case. That's because it's easy to
reduce the hard links of an inode, but once all valid paths are removed,
there're no valid paths for creating other hard links. The only way to do
that is trying to send an older snapshot after a newer snapshot has been
sent.

Cc: <[email protected]> # 4.9: 46b2f4590aab: Btrfs: fix send
failure when root has deleted files still open
Cc: <[email protected]> # 4.9: 71ecfc133b03: btrfs: send:
introduce recorded_ref_alloc and recorded_ref_free
Cc: <[email protected]> # 4.9: 3aa5bd367fa5: btrfs: send: fix
sending link commands for existing file paths
Cc: <[email protected]> # 4.9: 0d8869fb6b6f8: btrfs: send: always
use the rbtree based inode ref management infrastructure
Reviewed-by: Robbie Ko <[email protected]>
Signed-off-by: BingJing Chang <[email protected]>
---
fs/btrfs/send.c | 214 +++++++++++++++++++-----------------------------
1 file changed, 85 insertions(+), 129 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index f8d77a33b9b7..6ab1ba66ff4b 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -850,6 +850,7 @@ struct btrfs_inode_info {
u64 gid;
u64 rdev;
u64 attr;
+ u64 nlink;
};

/*
@@ -888,6 +889,7 @@ static int get_inode_info(struct btrfs_root *root, u64 ino,
info->uid = btrfs_inode_uid(path->nodes[0], ii);
info->gid = btrfs_inode_gid(path->nodes[0], ii);
info->rdev = btrfs_inode_rdev(path->nodes[0], ii);
+ info->nlink = btrfs_inode_nlink(path->nodes[0], ii);
/*
* Transfer the unchanged u64 value of btrfs_inode_item::flags, that's
* otherwise logically split to 32/32 parts.
@@ -1652,19 +1654,22 @@ static int get_cur_inode_state(struct send_ctx *sctx, u64 ino, u64 gen)
int right_ret;
u64 left_gen;
u64 right_gen;
+ struct btrfs_inode_info info;

- ret = get_inode_gen(sctx->send_root, ino, &left_gen);
+ ret = get_inode_info(sctx->send_root, ino, &info);
if (ret < 0 && ret != -ENOENT)
goto out;
- left_ret = ret;
+ left_ret = (info.nlink == 0) ? -ENOENT : ret;
+ left_gen = info.gen;

if (!sctx->parent_root) {
right_ret = -ENOENT;
} else {
- ret = get_inode_gen(sctx->parent_root, ino, &right_gen);
+ ret = get_inode_info(sctx->parent_root, ino, &info);
if (ret < 0 && ret != -ENOENT)
goto out;
- right_ret = ret;
+ right_ret = (info.nlink == 0) ? -ENOENT : ret;
+ right_gen = info.gen;
}

if (!left_ret && !right_ret) {
@@ -6413,86 +6418,6 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end)
return ret;
}

-struct parent_paths_ctx {
- struct list_head *refs;
- struct send_ctx *sctx;
-};
-
-static int record_parent_ref(int num, u64 dir, int index, struct fs_path *name,
- void *ctx)
-{
- struct parent_paths_ctx *ppctx = ctx;
-
- /*
- * Pass 0 as the generation for the directory, we don't care about it
- * here as we have no new references to add, we just want to delete all
- * references for an inode.
- */
- return record_ref_in_tree(&ppctx->sctx->rbtree_deleted_refs, ppctx->refs,
- name, dir, 0, ppctx->sctx);
-}
-
-/*
- * Issue unlink operations for all paths of the current inode found in the
- * parent snapshot.
- */
-static int btrfs_unlink_all_paths(struct send_ctx *sctx)
-{
- LIST_HEAD(deleted_refs);
- struct btrfs_path *path;
- struct btrfs_root *root = sctx->parent_root;
- struct btrfs_key key;
- struct btrfs_key found_key;
- struct parent_paths_ctx ctx;
- int iter_ret = 0;
- int ret;
-
- path = alloc_path_for_send();
- if (!path)
- return -ENOMEM;
-
- key.objectid = sctx->cur_ino;
- key.type = BTRFS_INODE_REF_KEY;
- key.offset = 0;
-
- ctx.refs = &deleted_refs;
- ctx.sctx = sctx;
-
- btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
- if (found_key.objectid != key.objectid)
- break;
- if (found_key.type != key.type &&
- found_key.type != BTRFS_INODE_EXTREF_KEY)
- break;
-
- ret = iterate_inode_ref(root, path, &found_key, 1,
- record_parent_ref, &ctx);
- if (ret < 0)
- goto out;
- }
- /* Catch error found during iteration */
- if (iter_ret < 0) {
- ret = iter_ret;
- goto out;
- }
-
- while (!list_empty(&deleted_refs)) {
- struct recorded_ref *ref;
-
- ref = list_first_entry(&deleted_refs, struct recorded_ref, list);
- ret = send_unlink(sctx, ref->full_path);
- if (ret < 0)
- goto out;
- recorded_ref_free(ref);
- }
- ret = 0;
-out:
- btrfs_free_path(path);
- if (ret)
- __free_recorded_refs(&deleted_refs);
- return ret;
-}
-
static void close_current_inode(struct send_ctx *sctx)
{
u64 i_size;
@@ -6583,25 +6508,37 @@ static int changed_inode(struct send_ctx *sctx,
* file descriptor against it or turning a RO snapshot into RW mode,
* keep an open file descriptor against a file, delete it and then
* turn the snapshot back to RO mode before using it for a send
- * operation. So if we find such cases, ignore the inode and all its
- * items completely if it's a new inode, or if it's a changed inode
- * make sure all its previous paths (from the parent snapshot) are all
- * unlinked and all other the inode items are ignored.
+ * operation. The former is what the receiver operation does.
+ * Therefore, if we want to send these snapshots soon after they're
+ * received, we need to handle orphan inodes as well. Moreover,
+ * orphans can appear not only in the send snapshot but also in the
+ * parent snapshot. Here are several cases:
+ *
+ * Case 1: BTRFS_COMPARE_TREE_NEW
+ * | send snapshot | action
+ * --------------------------------
+ * nlink | 0 | ignore
+ *
+ * Case 2: BTRFS_COMPARE_TREE_DELETED
+ * | parent snapshot | action
+ * ----------------------------------
+ * nlink | 0 | as usual
+ * Note: No unlinks will be sent because there're no paths for it.
+ *
+ * Case 3: BTRFS_COMPARE_TREE_CHANGED
+ * | | parent snapshot | send snapshot | action
+ * -----------------------------------------------------------------------
+ * subcase 1 | nlink | 0 | 0 | ignore
+ * subcase 2 | nlink | >0 | 0 | new_gen(deletion)
+ * subcase 3 | nlink | 0 | >0 | new_gen(creation)
+ *
*/
- if (result == BTRFS_COMPARE_TREE_NEW ||
- result == BTRFS_COMPARE_TREE_CHANGED) {
- u32 nlinks;
-
- nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii);
- if (nlinks == 0) {
+ if (result == BTRFS_COMPARE_TREE_NEW) {
+ if (btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii) ==
+ 0) {
sctx->ignore_cur_inode = true;
- if (result == BTRFS_COMPARE_TREE_CHANGED)
- ret = btrfs_unlink_all_paths(sctx);
goto out;
}
- }
-
- if (result == BTRFS_COMPARE_TREE_NEW) {
sctx->cur_inode_gen = left_gen;
sctx->cur_inode_new = true;
sctx->cur_inode_deleted = false;
@@ -6622,6 +6559,18 @@ static int changed_inode(struct send_ctx *sctx,
sctx->cur_inode_mode = btrfs_inode_mode(
sctx->right_path->nodes[0], right_ii);
} else if (result == BTRFS_COMPARE_TREE_CHANGED) {
+ u32 new_nlinks, old_nlinks;
+
+ new_nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0],
+ left_ii);
+ old_nlinks = btrfs_inode_nlink(sctx->right_path->nodes[0],
+ right_ii);
+ if (new_nlinks == 0 && old_nlinks == 0) {
+ sctx->ignore_cur_inode = true;
+ goto out;
+ } else if (new_nlinks == 0 || old_nlinks == 0) {
+ sctx->cur_inode_new_gen = 1;
+ }
/*
* We need to do some special handling in case the inode was
* reported as changed with a changed generation number. This
@@ -6648,38 +6597,45 @@ static int changed_inode(struct send_ctx *sctx,
/*
* Now process the inode as if it was new.
*/
- sctx->cur_inode_gen = left_gen;
- sctx->cur_inode_new = true;
- sctx->cur_inode_deleted = false;
- sctx->cur_inode_size = btrfs_inode_size(
- 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);
- ret = send_create_inode_if_needed(sctx);
- if (ret < 0)
- goto out;
+ if (new_nlinks > 0) {
+ sctx->cur_inode_gen = left_gen;
+ sctx->cur_inode_new = true;
+ sctx->cur_inode_deleted = false;
+ sctx->cur_inode_size = btrfs_inode_size(
+ 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);
+ ret = send_create_inode_if_needed(sctx);
+ if (ret < 0)
+ goto out;

- ret = process_all_refs(sctx, BTRFS_COMPARE_TREE_NEW);
- if (ret < 0)
- goto out;
- /*
- * Advance send_progress now as we did not get into
- * process_recorded_refs_if_needed in the new_gen case.
- */
- sctx->send_progress = sctx->cur_ino + 1;
+ ret = process_all_refs(sctx,
+ BTRFS_COMPARE_TREE_NEW);
+ if (ret < 0)
+ goto out;
+ /*
+ * Advance send_progress now as we did not get
+ * into process_recorded_refs_if_needed in the
+ * new_gen case.
+ */
+ sctx->send_progress = sctx->cur_ino + 1;

- /*
- * Now process all extents and xattrs of the inode as if
- * they were all new.
- */
- ret = process_all_extents(sctx);
- if (ret < 0)
- goto out;
- ret = process_all_new_xattrs(sctx);
- if (ret < 0)
- goto out;
+ /*
+ * Now process all extents and xattrs of the
+ * inode as if they were all new.
+ */
+ ret = process_all_extents(sctx);
+ if (ret < 0)
+ goto out;
+ ret = process_all_new_xattrs(sctx);
+ if (ret < 0)
+ goto out;
+ }
} else {
sctx->cur_inode_gen = left_gen;
sctx->cur_inode_new = false;
--
2.37.1

2022-08-11 12:29:18

by Filipe Manana

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] btrfs: send: fix failures when processing inodes with no links

On Thu, Aug 11, 2022 at 11:09 AM bingjingc <[email protected]> wrote:
>
> From: BingJing Chang <[email protected]>
>
> There is a bug causing send failures when processing an orphan directory
> with no links. In commit 46b2f4590aab ("Btrfs: fix send failure when root
> has deleted files still open")', the orphan inode issue was addressed. The
> send operation fails with a ENOENT error because of any attempts to
> generate a path for the inode with a link count of zero. Therefore, in that
> patch, sctx->ignore_cur_inode was introduced to be set if the current inode
> has a link count of zero for bypassing some unnecessary steps. And a helper
> function btrfs_unlink_all_paths() was introduced and called to clean up old
> paths found in the parent snapshot. However, not only regular files but
> also directories can be orphan inodes. So if the send operation meets an
> orphan directory, it will issue a wrong unlink command for that directory
> now. Soon the receive operation fails with a EISDIR error. Besides, the
> send operation also fails with a ENOENT error later when it tries to
> generate a path of it.
>
> Similar example but making an orphan dir for an incremental send:
>
> $ btrfs subvolume create vol
> $ mkdir vol/dir
> $ touch vol/dir/foo
>
> $ btrfs subvolume snapshot -r vol snap1
> $ btrfs subvolume snapshot -r vol snap2
>
> # Turn the second snapshot to RW mode and delete the whole dir while
> # holding an open file descriptor on it.
> $ btrfs property set snap2 ro false
> $ exec 73<snap2/dir
> $ rm -rf snap2/dir
>
> # Set the second snapshot back to RO mode and do an incremental send.
> $ btrfs property set snap2 ro true
> $ mkdir receive_dir
> $ btrfs send snap2 -p snap1 | btrfs receive receive_dir/
> At subvol snap2
> At snapshot snap2
> ERROR: send ioctl failed with -2: No such file or directory
> ERROR: unlink dir failed. Is a directory
>
> Actually, orphan inodes are more common use cases in cascading backups.
> (Please see the illustration below.) In a cascading backup, a user wants
> to replicate a couple of snapshots from Machine A to Machine B and from
> Machine B to Machine C. Machine B doesn't take any RO snapshots for
> sending. All a receiver does is create an RW snapshot of its parent
> snapshot, apply the send stream and turn it into RO mode at the end. Even
> if all paths of some inodes are deleted in applying the send stream, these
> inodes would not be deleted and become orphans after changing the subvolume
> from RW to RO. Moreover, orphan inodes can occur not only in send snapshots
> but also in parent snapshots because Machine B may do a batch replication
> of a couple of snapshots.
>
> An illustration for cascading backups:
> Machine A (snapshot {1..n}) --> Machine B --> Machine C
>
> The intuition to solve the problem is to delete all the items of orphan
> inodes before using these snapshots for sending. I used to think that the
> reasonable timing for doing that is during the ioctl of changing the
> subvolume from RW to RO because it sounds good that we will not modify the
> fs tree of a RO snapshot anymore. However, attempting to do the orphan
> cleanup in the ioctl would be pointless. Because if someone is holding an
> open file descriptor on the inode, the reference count of the inode will
> never drop to 0. Then iput() cannot trigger eviction, which finally deletes
> all the items of it. So we try to extend the original patch to handle
> orphans in send/parent snapshots. Here are several cases that need to be
> considered:
>
> Case 1: BTRFS_COMPARE_TREE_NEW
> | send snapshot | action
> --------------------------------
> nlink | 0 | ignore
>
> In case 1, when we get a BTRFS_COMPARE_TREE_NEW tree comparison result,
> it means that a new inode is found in the send snapshot and it doesn't
> appear in the parent snapshot. Since this inode has a link count of zero
> (It's an orphan and there're no paths for it.), we can leverage
> sctx->ignore_cur_inode in the original patch to prevent it from being
> created.
>
> Case 2: BTRFS_COMPARE_TREE_DELETED
> | parent snapshot | action
> ----------------------------------
> nlink | 0 | as usual
>
> In case 2, when we get a BTRFS_COMPARE_TREE_DELETED tree comparison
> result, it means that the inode only appears in the parent snapshot.
> As usual, the send operation will try to delete all its paths. However,
> this inode has a link count of zero, so no paths of it will be found. No
> deletion operations will be issued. We don't need to change any logic.
>
> Case 3: BTRFS_COMPARE_TREE_CHANGED
> | | parent snapshot | send snapshot | action
> -----------------------------------------------------------------------
> subcase 1 | nlink | 0 | 0 | ignore
> subcase 2 | nlink | >0 | 0 | new_gen(deletion)
> subcase 3 | nlink | 0 | >0 | new_gen(creation)
>
> In case 3, when we get a BTRFS_COMPARE_TREE_CHANGED tree comparison result,
> it means that the inode appears in both snapshots. Here're three subcases.
>
> First, if the inode has link counts of zero in both snapshots. Since there
> are no paths for this inode in (source/destination) parent snapshots and we
> don't care about whether there is also an orphan inode in destination or
> not, we can set sctx->ignore_cur_inode on to prevent it from being created.
>
> For the second and the third subcases, if there're paths in one snapshot
> and there're no paths in the other snapshot for this inode. We can treat
> this inode as a new generation. We can also leverage the logic handling a
> new generation of an inode with small adjustments. Then it will delete all
> old paths and create a new inode with new attributes and paths only when
> there's a positive link count in the send snapshot. In subcase 2, the
> send operation only needs to delete all old paths as in the parent
> snapshot. But it may require more operations for a directory to remove its
> old paths. If a not-empty directory is going to be deleted (because it has
> a link count of zero in the send snapshot) but there're files/directories
> with bigger inode numbers under it, the send operation will need to rename
> it to its orphan name first. After processing and deleting the last item
> under this directory, the send operation will check this directory, aka
> the parent directory of the last item, again and issue a rmdir operation
> to remove it finally. Therefore, we also need to treat inodes with a link
> count of zero as if they didn't exist in get_cur_inode_state(), which is
> used in process_recorded_refs(). By doing this, when reviewing a directory
> with orphan names after the last item under it has been deleted, the send
> operation now can properly issue a rmdir operation. Otherwise, without
> doing this, the orphan directory with an orphan name would be kept here
> at the end due to the existing inode with a link count of zero being found.
> In subcase 3, as in case 2, no old paths would be found, so no deletion
> operations will be issued. The send operation will only create a new one
> for that inode.
>
> Note that subcase 3 is not a common case. That's because it's easy to
> reduce the hard links of an inode, but once all valid paths are removed,
> there're no valid paths for creating other hard links. The only way to do
> that is trying to send an older snapshot after a newer snapshot has been
> sent.
>
> Cc: <[email protected]> # 4.9: 46b2f4590aab: Btrfs: fix send
> failure when root has deleted files still open
> Cc: <[email protected]> # 4.9: 71ecfc133b03: btrfs: send:
> introduce recorded_ref_alloc and recorded_ref_free
> Cc: <[email protected]> # 4.9: 3aa5bd367fa5: btrfs: send: fix
> sending link commands for existing file paths
> Cc: <[email protected]> # 4.9: 0d8869fb6b6f8: btrfs: send: always
> use the rbtree based inode ref management infrastructure

Btw, lines with CC, Fixes, etc, tags should not be broken even if they
are wider than 74 characters.

So, in v1 when I gave you that example of CC stable tags, it wasn't
meant for you to literally copy-paste them.

First I asked if the purpose of the original Fixes tag was to backport
the fix to stable releases.
Was that the intention? You didn't provide an answer about that.

Then I told if that was the case, the proper way would be adding CC
stable tags and listing any
dependencies. I gave those 4 as examples with commits that are fairly
recent and obvious dependencies,
but I also said that probably there's a lot more missing - especially
if we want to backport to as far as 4.9.

Even with just those 4 dependencies, some of those commits are fairly
large, and that may be frowned upon
according to stable backport rules (listed at
https://www.kernel.org/doc/Documentation/process/stable-kernel-rules.rst).
For e.g., patches with over 100 lines changed.

Now, did you actually verify if there were more dependencies? (and test)
And do you really want to go as far as 4.9 (currently the oldest
stable release)?
I seriously doubt that those 4 commits are the only dependencies in
order to be able to cleanly backport to 4.9 and other old branches.

It may be better to backport only to a few younger stable branches, or
just provide later a version of the patch to
apply to each desired stable branch (once the fix is in Linus' tree
and in a -rc release).

If you are not interested in backporting to stable or don't have the
time to verify the dependencies and test, then just remove all the
stable tags.
Just leave a fixes tag:

Fixes: 31db9f7c23fbf7 ("Btrfs: introduce BTRFS_IOC_SEND for btrfs send/receive")

Also, please don't forget to send a test case for fstests, covering as
many cases as possible (not just the example
at the beginning of the changelog).

Thanks.

> Reviewed-by: Robbie Ko <[email protected]>
> Signed-off-by: BingJing Chang <[email protected]>
> ---
> fs/btrfs/send.c | 214 +++++++++++++++++++-----------------------------
> 1 file changed, 85 insertions(+), 129 deletions(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index f8d77a33b9b7..6ab1ba66ff4b 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -850,6 +850,7 @@ struct btrfs_inode_info {
> u64 gid;
> u64 rdev;
> u64 attr;
> + u64 nlink;
> };
>
> /*
> @@ -888,6 +889,7 @@ static int get_inode_info(struct btrfs_root *root, u64 ino,
> info->uid = btrfs_inode_uid(path->nodes[0], ii);
> info->gid = btrfs_inode_gid(path->nodes[0], ii);
> info->rdev = btrfs_inode_rdev(path->nodes[0], ii);
> + info->nlink = btrfs_inode_nlink(path->nodes[0], ii);
> /*
> * Transfer the unchanged u64 value of btrfs_inode_item::flags, that's
> * otherwise logically split to 32/32 parts.
> @@ -1652,19 +1654,22 @@ static int get_cur_inode_state(struct send_ctx *sctx, u64 ino, u64 gen)
> int right_ret;
> u64 left_gen;
> u64 right_gen;
> + struct btrfs_inode_info info;
>
> - ret = get_inode_gen(sctx->send_root, ino, &left_gen);
> + ret = get_inode_info(sctx->send_root, ino, &info);
> if (ret < 0 && ret != -ENOENT)
> goto out;
> - left_ret = ret;
> + left_ret = (info.nlink == 0) ? -ENOENT : ret;
> + left_gen = info.gen;
>
> if (!sctx->parent_root) {
> right_ret = -ENOENT;
> } else {
> - ret = get_inode_gen(sctx->parent_root, ino, &right_gen);
> + ret = get_inode_info(sctx->parent_root, ino, &info);
> if (ret < 0 && ret != -ENOENT)
> goto out;
> - right_ret = ret;
> + right_ret = (info.nlink == 0) ? -ENOENT : ret;
> + right_gen = info.gen;
> }
>
> if (!left_ret && !right_ret) {
> @@ -6413,86 +6418,6 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end)
> return ret;
> }
>
> -struct parent_paths_ctx {
> - struct list_head *refs;
> - struct send_ctx *sctx;
> -};
> -
> -static int record_parent_ref(int num, u64 dir, int index, struct fs_path *name,
> - void *ctx)
> -{
> - struct parent_paths_ctx *ppctx = ctx;
> -
> - /*
> - * Pass 0 as the generation for the directory, we don't care about it
> - * here as we have no new references to add, we just want to delete all
> - * references for an inode.
> - */
> - return record_ref_in_tree(&ppctx->sctx->rbtree_deleted_refs, ppctx->refs,
> - name, dir, 0, ppctx->sctx);
> -}
> -
> -/*
> - * Issue unlink operations for all paths of the current inode found in the
> - * parent snapshot.
> - */
> -static int btrfs_unlink_all_paths(struct send_ctx *sctx)
> -{
> - LIST_HEAD(deleted_refs);
> - struct btrfs_path *path;
> - struct btrfs_root *root = sctx->parent_root;
> - struct btrfs_key key;
> - struct btrfs_key found_key;
> - struct parent_paths_ctx ctx;
> - int iter_ret = 0;
> - int ret;
> -
> - path = alloc_path_for_send();
> - if (!path)
> - return -ENOMEM;
> -
> - key.objectid = sctx->cur_ino;
> - key.type = BTRFS_INODE_REF_KEY;
> - key.offset = 0;
> -
> - ctx.refs = &deleted_refs;
> - ctx.sctx = sctx;
> -
> - btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
> - if (found_key.objectid != key.objectid)
> - break;
> - if (found_key.type != key.type &&
> - found_key.type != BTRFS_INODE_EXTREF_KEY)
> - break;
> -
> - ret = iterate_inode_ref(root, path, &found_key, 1,
> - record_parent_ref, &ctx);
> - if (ret < 0)
> - goto out;
> - }
> - /* Catch error found during iteration */
> - if (iter_ret < 0) {
> - ret = iter_ret;
> - goto out;
> - }
> -
> - while (!list_empty(&deleted_refs)) {
> - struct recorded_ref *ref;
> -
> - ref = list_first_entry(&deleted_refs, struct recorded_ref, list);
> - ret = send_unlink(sctx, ref->full_path);
> - if (ret < 0)
> - goto out;
> - recorded_ref_free(ref);
> - }
> - ret = 0;
> -out:
> - btrfs_free_path(path);
> - if (ret)
> - __free_recorded_refs(&deleted_refs);
> - return ret;
> -}
> -
> static void close_current_inode(struct send_ctx *sctx)
> {
> u64 i_size;
> @@ -6583,25 +6508,37 @@ static int changed_inode(struct send_ctx *sctx,
> * file descriptor against it or turning a RO snapshot into RW mode,
> * keep an open file descriptor against a file, delete it and then
> * turn the snapshot back to RO mode before using it for a send
> - * operation. So if we find such cases, ignore the inode and all its
> - * items completely if it's a new inode, or if it's a changed inode
> - * make sure all its previous paths (from the parent snapshot) are all
> - * unlinked and all other the inode items are ignored.
> + * operation. The former is what the receiver operation does.
> + * Therefore, if we want to send these snapshots soon after they're
> + * received, we need to handle orphan inodes as well. Moreover,
> + * orphans can appear not only in the send snapshot but also in the
> + * parent snapshot. Here are several cases:
> + *
> + * Case 1: BTRFS_COMPARE_TREE_NEW
> + * | send snapshot | action
> + * --------------------------------
> + * nlink | 0 | ignore
> + *
> + * Case 2: BTRFS_COMPARE_TREE_DELETED
> + * | parent snapshot | action
> + * ----------------------------------
> + * nlink | 0 | as usual
> + * Note: No unlinks will be sent because there're no paths for it.
> + *
> + * Case 3: BTRFS_COMPARE_TREE_CHANGED
> + * | | parent snapshot | send snapshot | action
> + * -----------------------------------------------------------------------
> + * subcase 1 | nlink | 0 | 0 | ignore
> + * subcase 2 | nlink | >0 | 0 | new_gen(deletion)
> + * subcase 3 | nlink | 0 | >0 | new_gen(creation)
> + *
> */
> - if (result == BTRFS_COMPARE_TREE_NEW ||
> - result == BTRFS_COMPARE_TREE_CHANGED) {
> - u32 nlinks;
> -
> - nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii);
> - if (nlinks == 0) {
> + if (result == BTRFS_COMPARE_TREE_NEW) {
> + if (btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii) ==
> + 0) {
> sctx->ignore_cur_inode = true;
> - if (result == BTRFS_COMPARE_TREE_CHANGED)
> - ret = btrfs_unlink_all_paths(sctx);
> goto out;
> }
> - }
> -
> - if (result == BTRFS_COMPARE_TREE_NEW) {
> sctx->cur_inode_gen = left_gen;
> sctx->cur_inode_new = true;
> sctx->cur_inode_deleted = false;
> @@ -6622,6 +6559,18 @@ static int changed_inode(struct send_ctx *sctx,
> sctx->cur_inode_mode = btrfs_inode_mode(
> sctx->right_path->nodes[0], right_ii);
> } else if (result == BTRFS_COMPARE_TREE_CHANGED) {
> + u32 new_nlinks, old_nlinks;
> +
> + new_nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0],
> + left_ii);
> + old_nlinks = btrfs_inode_nlink(sctx->right_path->nodes[0],
> + right_ii);
> + if (new_nlinks == 0 && old_nlinks == 0) {
> + sctx->ignore_cur_inode = true;
> + goto out;
> + } else if (new_nlinks == 0 || old_nlinks == 0) {
> + sctx->cur_inode_new_gen = 1;
> + }
> /*
> * We need to do some special handling in case the inode was
> * reported as changed with a changed generation number. This
> @@ -6648,38 +6597,45 @@ static int changed_inode(struct send_ctx *sctx,
> /*
> * Now process the inode as if it was new.
> */
> - sctx->cur_inode_gen = left_gen;
> - sctx->cur_inode_new = true;
> - sctx->cur_inode_deleted = false;
> - sctx->cur_inode_size = btrfs_inode_size(
> - 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);
> - ret = send_create_inode_if_needed(sctx);
> - if (ret < 0)
> - goto out;
> + if (new_nlinks > 0) {
> + sctx->cur_inode_gen = left_gen;
> + sctx->cur_inode_new = true;
> + sctx->cur_inode_deleted = false;
> + sctx->cur_inode_size = btrfs_inode_size(
> + 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);
> + ret = send_create_inode_if_needed(sctx);
> + if (ret < 0)
> + goto out;
>
> - ret = process_all_refs(sctx, BTRFS_COMPARE_TREE_NEW);
> - if (ret < 0)
> - goto out;
> - /*
> - * Advance send_progress now as we did not get into
> - * process_recorded_refs_if_needed in the new_gen case.
> - */
> - sctx->send_progress = sctx->cur_ino + 1;
> + ret = process_all_refs(sctx,
> + BTRFS_COMPARE_TREE_NEW);
> + if (ret < 0)
> + goto out;
> + /*
> + * Advance send_progress now as we did not get
> + * into process_recorded_refs_if_needed in the
> + * new_gen case.
> + */
> + sctx->send_progress = sctx->cur_ino + 1;
>
> - /*
> - * Now process all extents and xattrs of the inode as if
> - * they were all new.
> - */
> - ret = process_all_extents(sctx);
> - if (ret < 0)
> - goto out;
> - ret = process_all_new_xattrs(sctx);
> - if (ret < 0)
> - goto out;
> + /*
> + * Now process all extents and xattrs of the
> + * inode as if they were all new.
> + */
> + ret = process_all_extents(sctx);
> + if (ret < 0)
> + goto out;
> + ret = process_all_new_xattrs(sctx);
> + if (ret < 0)
> + goto out;
> + }
> } else {
> sctx->cur_inode_gen = left_gen;
> sctx->cur_inode_new = false;
> --
> 2.37.1
>

2022-08-12 14:47:09

by bingjing chang

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] btrfs: send: fix failures when processing inodes with no links

Filipe Manana <[email protected]> 於 2022年8月11日週四晚上8:00寫道：
>
> On Thu, Aug 11, 2022 at 11:09 AM bingjingc <[email protected]> wrote:
> >
> > From: BingJing Chang <[email protected]>
> >
> > There is a bug causing send failures when processing an orphan directory
> > with no links. In commit 46b2f4590aab ("Btrfs: fix send failure when root
> > has deleted files still open")', the orphan inode issue was addressed. The
> > send operation fails with a ENOENT error because of any attempts to
> > generate a path for the inode with a link count of zero. Therefore, in that
> > patch, sctx->ignore_cur_inode was introduced to be set if the current inode
> > has a link count of zero for bypassing some unnecessary steps. And a helper
> > function btrfs_unlink_all_paths() was introduced and called to clean up old
> > paths found in the parent snapshot. However, not only regular files but
> > also directories can be orphan inodes. So if the send operation meets an
> > orphan directory, it will issue a wrong unlink command for that directory
> > now. Soon the receive operation fails with a EISDIR error. Besides, the
> > send operation also fails with a ENOENT error later when it tries to
> > generate a path of it.
> >
> > Similar example but making an orphan dir for an incremental send:
> >
> > $ btrfs subvolume create vol
> > $ mkdir vol/dir
> > $ touch vol/dir/foo
> >
> > $ btrfs subvolume snapshot -r vol snap1
> > $ btrfs subvolume snapshot -r vol snap2
> >
> > # Turn the second snapshot to RW mode and delete the whole dir while
> > # holding an open file descriptor on it.
> > $ btrfs property set snap2 ro false
> > $ exec 73<snap2/dir
> > $ rm -rf snap2/dir
> >
> > # Set the second snapshot back to RO mode and do an incremental send.
> > $ btrfs property set snap2 ro true
> > $ mkdir receive_dir
> > $ btrfs send snap2 -p snap1 | btrfs receive receive_dir/
> > At subvol snap2
> > At snapshot snap2
> > ERROR: send ioctl failed with -2: No such file or directory
> > ERROR: unlink dir failed. Is a directory
> >
> > Actually, orphan inodes are more common use cases in cascading backups.
> > (Please see the illustration below.) In a cascading backup, a user wants
> > to replicate a couple of snapshots from Machine A to Machine B and from
> > Machine B to Machine C. Machine B doesn't take any RO snapshots for
> > sending. All a receiver does is create an RW snapshot of its parent
> > snapshot, apply the send stream and turn it into RO mode at the end. Even
> > if all paths of some inodes are deleted in applying the send stream, these
> > inodes would not be deleted and become orphans after changing the subvolume
> > from RW to RO. Moreover, orphan inodes can occur not only in send snapshots
> > but also in parent snapshots because Machine B may do a batch replication
> > of a couple of snapshots.
> >
> > An illustration for cascading backups:
> > Machine A (snapshot {1..n}) --> Machine B --> Machine C
> >
> > The intuition to solve the problem is to delete all the items of orphan
> > inodes before using these snapshots for sending. I used to think that the
> > reasonable timing for doing that is during the ioctl of changing the
> > subvolume from RW to RO because it sounds good that we will not modify the
> > fs tree of a RO snapshot anymore. However, attempting to do the orphan
> > cleanup in the ioctl would be pointless. Because if someone is holding an
> > open file descriptor on the inode, the reference count of the inode will
> > never drop to 0. Then iput() cannot trigger eviction, which finally deletes
> > all the items of it. So we try to extend the original patch to handle
> > orphans in send/parent snapshots. Here are several cases that need to be
> > considered:
> >
> > Case 1: BTRFS_COMPARE_TREE_NEW
> > | send snapshot | action
> > --------------------------------
> > nlink | 0 | ignore
> >
> > In case 1, when we get a BTRFS_COMPARE_TREE_NEW tree comparison result,
> > it means that a new inode is found in the send snapshot and it doesn't
> > appear in the parent snapshot. Since this inode has a link count of zero
> > (It's an orphan and there're no paths for it.), we can leverage
> > sctx->ignore_cur_inode in the original patch to prevent it from being
> > created.
> >
> > Case 2: BTRFS_COMPARE_TREE_DELETED
> > | parent snapshot | action
> > ----------------------------------
> > nlink | 0 | as usual
> >
> > In case 2, when we get a BTRFS_COMPARE_TREE_DELETED tree comparison
> > result, it means that the inode only appears in the parent snapshot.
> > As usual, the send operation will try to delete all its paths. However,
> > this inode has a link count of zero, so no paths of it will be found. No
> > deletion operations will be issued. We don't need to change any logic.
> >
> > Case 3: BTRFS_COMPARE_TREE_CHANGED
> > | | parent snapshot | send snapshot | action
> > -----------------------------------------------------------------------
> > subcase 1 | nlink | 0 | 0 | ignore
> > subcase 2 | nlink | >0 | 0 | new_gen(deletion)
> > subcase 3 | nlink | 0 | >0 | new_gen(creation)
> >
> > In case 3, when we get a BTRFS_COMPARE_TREE_CHANGED tree comparison result,
> > it means that the inode appears in both snapshots. Here're three subcases.
> >
> > First, if the inode has link counts of zero in both snapshots. Since there
> > are no paths for this inode in (source/destination) parent snapshots and we
> > don't care about whether there is also an orphan inode in destination or
> > not, we can set sctx->ignore_cur_inode on to prevent it from being created.
> >
> > For the second and the third subcases, if there're paths in one snapshot
> > and there're no paths in the other snapshot for this inode. We can treat
> > this inode as a new generation. We can also leverage the logic handling a
> > new generation of an inode with small adjustments. Then it will delete all
> > old paths and create a new inode with new attributes and paths only when
> > there's a positive link count in the send snapshot. In subcase 2, the
> > send operation only needs to delete all old paths as in the parent
> > snapshot. But it may require more operations for a directory to remove its
> > old paths. If a not-empty directory is going to be deleted (because it has
> > a link count of zero in the send snapshot) but there're files/directories
> > with bigger inode numbers under it, the send operation will need to rename
> > it to its orphan name first. After processing and deleting the last item
> > under this directory, the send operation will check this directory, aka
> > the parent directory of the last item, again and issue a rmdir operation
> > to remove it finally. Therefore, we also need to treat inodes with a link
> > count of zero as if they didn't exist in get_cur_inode_state(), which is
> > used in process_recorded_refs(). By doing this, when reviewing a directory
> > with orphan names after the last item under it has been deleted, the send
> > operation now can properly issue a rmdir operation. Otherwise, without
> > doing this, the orphan directory with an orphan name would be kept here
> > at the end due to the existing inode with a link count of zero being found.
> > In subcase 3, as in case 2, no old paths would be found, so no deletion
> > operations will be issued. The send operation will only create a new one
> > for that inode.
> >
> > Note that subcase 3 is not a common case. That's because it's easy to
> > reduce the hard links of an inode, but once all valid paths are removed,
> > there're no valid paths for creating other hard links. The only way to do
> > that is trying to send an older snapshot after a newer snapshot has been
> > sent.
> >
> > Cc: <[email protected]> # 4.9: 46b2f4590aab: Btrfs: fix send
> > failure when root has deleted files still open
> > Cc: <[email protected]> # 4.9: 71ecfc133b03: btrfs: send:
> > introduce recorded_ref_alloc and recorded_ref_free
> > Cc: <[email protected]> # 4.9: 3aa5bd367fa5: btrfs: send: fix
> > sending link commands for existing file paths
> > Cc: <[email protected]> # 4.9: 0d8869fb6b6f8: btrfs: send: always
> > use the rbtree based inode ref management infrastructure
>
> Btw, lines with CC, Fixes, etc, tags should not be broken even if they
> are wider than 74 characters.
>

Okay, thank you for telling me that.

> So, in v1 when I gave you that example of CC stable tags, it wasn't
> meant for you to literally copy-paste them.
>
> First I asked if the purpose of the original Fixes tag was to backport
> the fix to stable releases.
> Was that the intention? You didn't provide an answer about that.
>

Oh, I misunderstood your suggestion. I'm sorry about that.
Our intention is to report this bug and try to provide a reasonable and
acceptable fix for it. Backporting is not our goal.

> Then I told if that was the case, the proper way would be adding CC
> stable tags and listing any
> dependencies. I gave those 4 as examples with commits that are fairly
> recent and obvious dependencies,
> but I also said that probably there's a lot more missing - especially
> if we want to backport to as far as 4.9.
>

> Even with just those 4 dependencies, some of those commits are fairly
> large, and that may be frowned upon
> according to stable backport rules (listed at
> https://www.kernel.org/doc/Documentation/process/stable-kernel-rules.rst).
> For e.g., patches with over 100 lines changed.
>
> Now, did you actually verify if there were more dependencies? (and test)
> And do you really want to go as far as 4.9 (currently the oldest
> stable release)?

No, I didn't. I used to think the CC tag was a very cool feature, which
just putting a few commits lets backport easily when I read your mail,
so I copied and pasted these 4 commits in the beginning of revising
the patch v2. However, I'm wrong.

> I seriously doubt that those 4 commits are the only dependencies in
> order to be able to cleanly backport to 4.9 and other old branches.
>
> It may be better to backport only to a few younger stable branches, or
> just provide later a version of the patch to
> apply to each desired stable branch (once the fix is in Linus' tree
> and in a -rc release).
>
> If you are not interested in backporting to stable or don't have the
> time to verify the dependencies and test, then just remove all the
> stable tags.
> Just leave a fixes tag:
>
> Fixes: 31db9f7c23fbf7 ("Btrfs: introduce BTRFS_IOC_SEND for btrfs send/receive")
>

Since backporting is not our goal. I will just leave the fix tag here.

> Also, please don't forget to send a test case for fstests, covering as
> many cases as possible (not just the example
> at the beginning of the changelog).
>

Okay, I will submit a test case covering all cases.
Because I still need to spend time learning how to use the fssum utility
for the last test case you reviewed, so I will submit the test case later.

Thanks.

> Thanks.
>
> > Reviewed-by: Robbie Ko <[email protected]>
> > Signed-off-by: BingJing Chang <[email protected]>
> > ---
> > fs/btrfs/send.c | 214 +++++++++++++++++++-----------------------------
> > 1 file changed, 85 insertions(+), 129 deletions(-)
> >
> > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> > index f8d77a33b9b7..6ab1ba66ff4b 100644
> > --- a/fs/btrfs/send.c
> > +++ b/fs/btrfs/send.c
> > @@ -850,6 +850,7 @@ struct btrfs_inode_info {
> > u64 gid;
> > u64 rdev;
> > u64 attr;
> > + u64 nlink;
> > };
> >
> > /*
> > @@ -888,6 +889,7 @@ static int get_inode_info(struct btrfs_root *root, u64 ino,
> > info->uid = btrfs_inode_uid(path->nodes[0], ii);
> > info->gid = btrfs_inode_gid(path->nodes[0], ii);
> > info->rdev = btrfs_inode_rdev(path->nodes[0], ii);
> > + info->nlink = btrfs_inode_nlink(path->nodes[0], ii);
> > /*
> > * Transfer the unchanged u64 value of btrfs_inode_item::flags, that's
> > * otherwise logically split to 32/32 parts.
> > @@ -1652,19 +1654,22 @@ static int get_cur_inode_state(struct send_ctx *sctx, u64 ino, u64 gen)
> > int right_ret;
> > u64 left_gen;
> > u64 right_gen;
> > + struct btrfs_inode_info info;
> >
> > - ret = get_inode_gen(sctx->send_root, ino, &left_gen);
> > + ret = get_inode_info(sctx->send_root, ino, &info);
> > if (ret < 0 && ret != -ENOENT)
> > goto out;
> > - left_ret = ret;
> > + left_ret = (info.nlink == 0) ? -ENOENT : ret;
> > + left_gen = info.gen;
> >
> > if (!sctx->parent_root) {
> > right_ret = -ENOENT;
> > } else {
> > - ret = get_inode_gen(sctx->parent_root, ino, &right_gen);
> > + ret = get_inode_info(sctx->parent_root, ino, &info);
> > if (ret < 0 && ret != -ENOENT)
> > goto out;
> > - right_ret = ret;
> > + right_ret = (info.nlink == 0) ? -ENOENT : ret;
> > + right_gen = info.gen;
> > }
> >
> > if (!left_ret && !right_ret) {
> > @@ -6413,86 +6418,6 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end)
> > return ret;
> > }
> >
> > -struct parent_paths_ctx {
> > - struct list_head *refs;
> > - struct send_ctx *sctx;
> > -};
> > -
> > -static int record_parent_ref(int num, u64 dir, int index, struct fs_path *name,
> > - void *ctx)
> > -{
> > - struct parent_paths_ctx *ppctx = ctx;
> > -
> > - /*
> > - * Pass 0 as the generation for the directory, we don't care about it
> > - * here as we have no new references to add, we just want to delete all
> > - * references for an inode.
> > - */
> > - return record_ref_in_tree(&ppctx->sctx->rbtree_deleted_refs, ppctx->refs,
> > - name, dir, 0, ppctx->sctx);
> > -}
> > -
> > -/*
> > - * Issue unlink operations for all paths of the current inode found in the
> > - * parent snapshot.
> > - */
> > -static int btrfs_unlink_all_paths(struct send_ctx *sctx)
> > -{
> > - LIST_HEAD(deleted_refs);
> > - struct btrfs_path *path;
> > - struct btrfs_root *root = sctx->parent_root;
> > - struct btrfs_key key;
> > - struct btrfs_key found_key;
> > - struct parent_paths_ctx ctx;
> > - int iter_ret = 0;
> > - int ret;
> > -
> > - path = alloc_path_for_send();
> > - if (!path)
> > - return -ENOMEM;
> > -
> > - key.objectid = sctx->cur_ino;
> > - key.type = BTRFS_INODE_REF_KEY;
> > - key.offset = 0;
> > -
> > - ctx.refs = &deleted_refs;
> > - ctx.sctx = sctx;
> > -
> > - btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
> > - if (found_key.objectid != key.objectid)
> > - break;
> > - if (found_key.type != key.type &&
> > - found_key.type != BTRFS_INODE_EXTREF_KEY)
> > - break;
> > -
> > - ret = iterate_inode_ref(root, path, &found_key, 1,
> > - record_parent_ref, &ctx);
> > - if (ret < 0)
> > - goto out;
> > - }
> > - /* Catch error found during iteration */
> > - if (iter_ret < 0) {
> > - ret = iter_ret;
> > - goto out;
> > - }
> > -
> > - while (!list_empty(&deleted_refs)) {
> > - struct recorded_ref *ref;
> > -
> > - ref = list_first_entry(&deleted_refs, struct recorded_ref, list);
> > - ret = send_unlink(sctx, ref->full_path);
> > - if (ret < 0)
> > - goto out;
> > - recorded_ref_free(ref);
> > - }
> > - ret = 0;
> > -out:
> > - btrfs_free_path(path);
> > - if (ret)
> > - __free_recorded_refs(&deleted_refs);
> > - return ret;
> > -}
> > -
> > static void close_current_inode(struct send_ctx *sctx)
> > {
> > u64 i_size;
> > @@ -6583,25 +6508,37 @@ static int changed_inode(struct send_ctx *sctx,
> > * file descriptor against it or turning a RO snapshot into RW mode,
> > * keep an open file descriptor against a file, delete it and then
> > * turn the snapshot back to RO mode before using it for a send
> > - * operation. So if we find such cases, ignore the inode and all its
> > - * items completely if it's a new inode, or if it's a changed inode
> > - * make sure all its previous paths (from the parent snapshot) are all
> > - * unlinked and all other the inode items are ignored.
> > + * operation. The former is what the receiver operation does.
> > + * Therefore, if we want to send these snapshots soon after they're
> > + * received, we need to handle orphan inodes as well. Moreover,
> > + * orphans can appear not only in the send snapshot but also in the
> > + * parent snapshot. Here are several cases:
> > + *
> > + * Case 1: BTRFS_COMPARE_TREE_NEW
> > + * | send snapshot | action
> > + * --------------------------------
> > + * nlink | 0 | ignore
> > + *
> > + * Case 2: BTRFS_COMPARE_TREE_DELETED
> > + * | parent snapshot | action
> > + * ----------------------------------
> > + * nlink | 0 | as usual
> > + * Note: No unlinks will be sent because there're no paths for it.
> > + *
> > + * Case 3: BTRFS_COMPARE_TREE_CHANGED
> > + * | | parent snapshot | send snapshot | action
> > + * -----------------------------------------------------------------------
> > + * subcase 1 | nlink | 0 | 0 | ignore
> > + * subcase 2 | nlink | >0 | 0 | new_gen(deletion)
> > + * subcase 3 | nlink | 0 | >0 | new_gen(creation)
> > + *
> > */
> > - if (result == BTRFS_COMPARE_TREE_NEW ||
> > - result == BTRFS_COMPARE_TREE_CHANGED) {
> > - u32 nlinks;
> > -
> > - nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii);
> > - if (nlinks == 0) {
> > + if (result == BTRFS_COMPARE_TREE_NEW) {
> > + if (btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii) ==
> > + 0) {
> > sctx->ignore_cur_inode = true;
> > - if (result == BTRFS_COMPARE_TREE_CHANGED)
> > - ret = btrfs_unlink_all_paths(sctx);
> > goto out;
> > }
> > - }
> > -
> > - if (result == BTRFS_COMPARE_TREE_NEW) {
> > sctx->cur_inode_gen = left_gen;
> > sctx->cur_inode_new = true;
> > sctx->cur_inode_deleted = false;
> > @@ -6622,6 +6559,18 @@ static int changed_inode(struct send_ctx *sctx,
> > sctx->cur_inode_mode = btrfs_inode_mode(
> > sctx->right_path->nodes[0], right_ii);
> > } else if (result == BTRFS_COMPARE_TREE_CHANGED) {
> > + u32 new_nlinks, old_nlinks;
> > +
> > + new_nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0],
> > + left_ii);
> > + old_nlinks = btrfs_inode_nlink(sctx->right_path->nodes[0],
> > + right_ii);
> > + if (new_nlinks == 0 && old_nlinks == 0) {
> > + sctx->ignore_cur_inode = true;
> > + goto out;
> > + } else if (new_nlinks == 0 || old_nlinks == 0) {
> > + sctx->cur_inode_new_gen = 1;
> > + }
> > /*
> > * We need to do some special handling in case the inode was
> > * reported as changed with a changed generation number. This
> > @@ -6648,38 +6597,45 @@ static int changed_inode(struct send_ctx *sctx,
> > /*
> > * Now process the inode as if it was new.
> > */
> > - sctx->cur_inode_gen = left_gen;
> > - sctx->cur_inode_new = true;
> > - sctx->cur_inode_deleted = false;
> > - sctx->cur_inode_size = btrfs_inode_size(
> > - 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);
> > - ret = send_create_inode_if_needed(sctx);
> > - if (ret < 0)
> > - goto out;
> > + if (new_nlinks > 0) {
> > + sctx->cur_inode_gen = left_gen;
> > + sctx->cur_inode_new = true;
> > + sctx->cur_inode_deleted = false;
> > + sctx->cur_inode_size = btrfs_inode_size(
> > + 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);
> > + ret = send_create_inode_if_needed(sctx);
> > + if (ret < 0)
> > + goto out;
> >
> > - ret = process_all_refs(sctx, BTRFS_COMPARE_TREE_NEW);
> > - if (ret < 0)
> > - goto out;
> > - /*
> > - * Advance send_progress now as we did not get into
> > - * process_recorded_refs_if_needed in the new_gen case.
> > - */
> > - sctx->send_progress = sctx->cur_ino + 1;
> > + ret = process_all_refs(sctx,
> > + BTRFS_COMPARE_TREE_NEW);
> > + if (ret < 0)
> > + goto out;
> > + /*
> > + * Advance send_progress now as we did not get
> > + * into process_recorded_refs_if_needed in the
> > + * new_gen case.
> > + */
> > + sctx->send_progress = sctx->cur_ino + 1;
> >
> > - /*
> > - * Now process all extents and xattrs of the inode as if
> > - * they were all new.
> > - */
> > - ret = process_all_extents(sctx);
> > - if (ret < 0)
> > - goto out;
> > - ret = process_all_new_xattrs(sctx);
> > - if (ret < 0)
> > - goto out;
> > + /*
> > + * Now process all extents and xattrs of the
> > + * inode as if they were all new.
> > + */
> > + ret = process_all_extents(sctx);
> > + if (ret < 0)
> > + goto out;
> > + ret = process_all_new_xattrs(sctx);
> > + if (ret < 0)
> > + goto out;
> > + }
> > } else {
> > sctx->cur_inode_gen = left_gen;
> > sctx->cur_inode_new = false;
> > --
> > 2.37.1
> >

2022-08-12 16:40:00

by Filipe Manana

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] btrfs: send: fix failures when processing inodes with no links

On Fri, Aug 12, 2022 at 3:36 PM bingjing chang <[email protected]> wrote:
>
> Filipe Manana <[email protected]> 於 2022年8月11日週四晚上8:00寫道：
> >
> > On Thu, Aug 11, 2022 at 11:09 AM bingjingc <[email protected]> wrote:
> > >
> > > From: BingJing Chang <[email protected]>
> > >
> > > There is a bug causing send failures when processing an orphan directory
> > > with no links. In commit 46b2f4590aab ("Btrfs: fix send failure when root
> > > has deleted files still open")', the orphan inode issue was addressed. The
> > > send operation fails with a ENOENT error because of any attempts to
> > > generate a path for the inode with a link count of zero. Therefore, in that
> > > patch, sctx->ignore_cur_inode was introduced to be set if the current inode
> > > has a link count of zero for bypassing some unnecessary steps. And a helper
> > > function btrfs_unlink_all_paths() was introduced and called to clean up old
> > > paths found in the parent snapshot. However, not only regular files but
> > > also directories can be orphan inodes. So if the send operation meets an
> > > orphan directory, it will issue a wrong unlink command for that directory
> > > now. Soon the receive operation fails with a EISDIR error. Besides, the
> > > send operation also fails with a ENOENT error later when it tries to
> > > generate a path of it.
> > >
> > > Similar example but making an orphan dir for an incremental send:
> > >
> > > $ btrfs subvolume create vol
> > > $ mkdir vol/dir
> > > $ touch vol/dir/foo
> > >
> > > $ btrfs subvolume snapshot -r vol snap1
> > > $ btrfs subvolume snapshot -r vol snap2
> > >
> > > # Turn the second snapshot to RW mode and delete the whole dir while
> > > # holding an open file descriptor on it.
> > > $ btrfs property set snap2 ro false
> > > $ exec 73<snap2/dir
> > > $ rm -rf snap2/dir
> > >
> > > # Set the second snapshot back to RO mode and do an incremental send.
> > > $ btrfs property set snap2 ro true
> > > $ mkdir receive_dir
> > > $ btrfs send snap2 -p snap1 | btrfs receive receive_dir/
> > > At subvol snap2
> > > At snapshot snap2
> > > ERROR: send ioctl failed with -2: No such file or directory
> > > ERROR: unlink dir failed. Is a directory
> > >
> > > Actually, orphan inodes are more common use cases in cascading backups.
> > > (Please see the illustration below.) In a cascading backup, a user wants
> > > to replicate a couple of snapshots from Machine A to Machine B and from
> > > Machine B to Machine C. Machine B doesn't take any RO snapshots for
> > > sending. All a receiver does is create an RW snapshot of its parent
> > > snapshot, apply the send stream and turn it into RO mode at the end. Even
> > > if all paths of some inodes are deleted in applying the send stream, these
> > > inodes would not be deleted and become orphans after changing the subvolume
> > > from RW to RO. Moreover, orphan inodes can occur not only in send snapshots
> > > but also in parent snapshots because Machine B may do a batch replication
> > > of a couple of snapshots.
> > >
> > > An illustration for cascading backups:
> > > Machine A (snapshot {1..n}) --> Machine B --> Machine C
> > >
> > > The intuition to solve the problem is to delete all the items of orphan
> > > inodes before using these snapshots for sending. I used to think that the
> > > reasonable timing for doing that is during the ioctl of changing the
> > > subvolume from RW to RO because it sounds good that we will not modify the
> > > fs tree of a RO snapshot anymore. However, attempting to do the orphan
> > > cleanup in the ioctl would be pointless. Because if someone is holding an
> > > open file descriptor on the inode, the reference count of the inode will
> > > never drop to 0. Then iput() cannot trigger eviction, which finally deletes
> > > all the items of it. So we try to extend the original patch to handle
> > > orphans in send/parent snapshots. Here are several cases that need to be
> > > considered:
> > >
> > > Case 1: BTRFS_COMPARE_TREE_NEW
> > > | send snapshot | action
> > > --------------------------------
> > > nlink | 0 | ignore
> > >
> > > In case 1, when we get a BTRFS_COMPARE_TREE_NEW tree comparison result,
> > > it means that a new inode is found in the send snapshot and it doesn't
> > > appear in the parent snapshot. Since this inode has a link count of zero
> > > (It's an orphan and there're no paths for it.), we can leverage
> > > sctx->ignore_cur_inode in the original patch to prevent it from being
> > > created.
> > >
> > > Case 2: BTRFS_COMPARE_TREE_DELETED
> > > | parent snapshot | action
> > > ----------------------------------
> > > nlink | 0 | as usual
> > >
> > > In case 2, when we get a BTRFS_COMPARE_TREE_DELETED tree comparison
> > > result, it means that the inode only appears in the parent snapshot.
> > > As usual, the send operation will try to delete all its paths. However,
> > > this inode has a link count of zero, so no paths of it will be found. No
> > > deletion operations will be issued. We don't need to change any logic.
> > >
> > > Case 3: BTRFS_COMPARE_TREE_CHANGED
> > > | | parent snapshot | send snapshot | action
> > > -----------------------------------------------------------------------
> > > subcase 1 | nlink | 0 | 0 | ignore
> > > subcase 2 | nlink | >0 | 0 | new_gen(deletion)
> > > subcase 3 | nlink | 0 | >0 | new_gen(creation)
> > >
> > > In case 3, when we get a BTRFS_COMPARE_TREE_CHANGED tree comparison result,
> > > it means that the inode appears in both snapshots. Here're three subcases.
> > >
> > > First, if the inode has link counts of zero in both snapshots. Since there
> > > are no paths for this inode in (source/destination) parent snapshots and we
> > > don't care about whether there is also an orphan inode in destination or
> > > not, we can set sctx->ignore_cur_inode on to prevent it from being created.
> > >
> > > For the second and the third subcases, if there're paths in one snapshot
> > > and there're no paths in the other snapshot for this inode. We can treat
> > > this inode as a new generation. We can also leverage the logic handling a
> > > new generation of an inode with small adjustments. Then it will delete all
> > > old paths and create a new inode with new attributes and paths only when
> > > there's a positive link count in the send snapshot. In subcase 2, the
> > > send operation only needs to delete all old paths as in the parent
> > > snapshot. But it may require more operations for a directory to remove its
> > > old paths. If a not-empty directory is going to be deleted (because it has
> > > a link count of zero in the send snapshot) but there're files/directories
> > > with bigger inode numbers under it, the send operation will need to rename
> > > it to its orphan name first. After processing and deleting the last item
> > > under this directory, the send operation will check this directory, aka
> > > the parent directory of the last item, again and issue a rmdir operation
> > > to remove it finally. Therefore, we also need to treat inodes with a link
> > > count of zero as if they didn't exist in get_cur_inode_state(), which is
> > > used in process_recorded_refs(). By doing this, when reviewing a directory
> > > with orphan names after the last item under it has been deleted, the send
> > > operation now can properly issue a rmdir operation. Otherwise, without
> > > doing this, the orphan directory with an orphan name would be kept here
> > > at the end due to the existing inode with a link count of zero being found.
> > > In subcase 3, as in case 2, no old paths would be found, so no deletion
> > > operations will be issued. The send operation will only create a new one
> > > for that inode.
> > >
> > > Note that subcase 3 is not a common case. That's because it's easy to
> > > reduce the hard links of an inode, but once all valid paths are removed,
> > > there're no valid paths for creating other hard links. The only way to do
> > > that is trying to send an older snapshot after a newer snapshot has been
> > > sent.
> > >
> > > Cc: <[email protected]> # 4.9: 46b2f4590aab: Btrfs: fix send
> > > failure when root has deleted files still open
> > > Cc: <[email protected]> # 4.9: 71ecfc133b03: btrfs: send:
> > > introduce recorded_ref_alloc and recorded_ref_free
> > > Cc: <[email protected]> # 4.9: 3aa5bd367fa5: btrfs: send: fix
> > > sending link commands for existing file paths
> > > Cc: <[email protected]> # 4.9: 0d8869fb6b6f8: btrfs: send: always
> > > use the rbtree based inode ref management infrastructure
> >
> > Btw, lines with CC, Fixes, etc, tags should not be broken even if they
> > are wider than 74 characters.
> >
>
> Okay, thank you for telling me that.
>
> > So, in v1 when I gave you that example of CC stable tags, it wasn't
> > meant for you to literally copy-paste them.
> >
> > First I asked if the purpose of the original Fixes tag was to backport
> > the fix to stable releases.
> > Was that the intention? You didn't provide an answer about that.
> >
>
> Oh, I misunderstood your suggestion. I'm sorry about that.
> Our intention is to report this bug and try to provide a reasonable and
> acceptable fix for it. Backporting is not our goal.
>
> > Then I told if that was the case, the proper way would be adding CC
> > stable tags and listing any
> > dependencies. I gave those 4 as examples with commits that are fairly
> > recent and obvious dependencies,
> > but I also said that probably there's a lot more missing - especially
> > if we want to backport to as far as 4.9.
> >
>
> > Even with just those 4 dependencies, some of those commits are fairly
> > large, and that may be frowned upon
> > according to stable backport rules (listed at
> > https://www.kernel.org/doc/Documentation/process/stable-kernel-rules.rst).
> > For e.g., patches with over 100 lines changed.
> >
> > Now, did you actually verify if there were more dependencies? (and test)
> > And do you really want to go as far as 4.9 (currently the oldest
> > stable release)?
>
> No, I didn't. I used to think the CC tag was a very cool feature, which
> just putting a few commits lets backport easily when I read your mail,
> so I copied and pasted these 4 commits in the beginning of revising
> the patch v2. However, I'm wrong.
>
> > I seriously doubt that those 4 commits are the only dependencies in
> > order to be able to cleanly backport to 4.9 and other old branches.
> >
> > It may be better to backport only to a few younger stable branches, or
> > just provide later a version of the patch to
> > apply to each desired stable branch (once the fix is in Linus' tree
> > and in a -rc release).
> >
> > If you are not interested in backporting to stable or don't have the
> > time to verify the dependencies and test, then just remove all the
> > stable tags.
> > Just leave a fixes tag:
> >
> > Fixes: 31db9f7c23fbf7 ("Btrfs: introduce BTRFS_IOC_SEND for btrfs send/receive")
> >
>
> Since backporting is not our goal. I will just leave the fix tag here.
>
> > Also, please don't forget to send a test case for fstests, covering as
> > many cases as possible (not just the example
> > at the beginning of the changelog).
> >
>
> Okay, I will submit a test case covering all cases.
> Because I still need to spend time learning how to use the fssum utility
> for the last test case you reviewed, so I will submit the test case later.

There's nothing special about fssum in order to use it.

Call it once on the original snapshot/subvolume to generate a digest
based on the tree and
save it to a file. The digest is based on all data and metadata of
every inode accessible through the given path.

After receiving the send stream, call fssum again to compute the
digest on the resulting snapshot and compare it
with the digest that was computed and saved before - if they match
then send/receive produced the correct
results, otherwise it didn't and the test should fail.

There are plenty of send/receive test cases using fssum, just look at
them (btrfs/241 for e.g.).

>
> Thanks.
>
> > Thanks.
> >
> > > Reviewed-by: Robbie Ko <[email protected]>
> > > Signed-off-by: BingJing Chang <[email protected]>
> > > ---
> > > fs/btrfs/send.c | 214 +++++++++++++++++++-----------------------------
> > > 1 file changed, 85 insertions(+), 129 deletions(-)
> > >
> > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> > > index f8d77a33b9b7..6ab1ba66ff4b 100644
> > > --- a/fs/btrfs/send.c
> > > +++ b/fs/btrfs/send.c
> > > @@ -850,6 +850,7 @@ struct btrfs_inode_info {
> > > u64 gid;
> > > u64 rdev;
> > > u64 attr;
> > > + u64 nlink;
> > > };
> > >
> > > /*
> > > @@ -888,6 +889,7 @@ static int get_inode_info(struct btrfs_root *root, u64 ino,
> > > info->uid = btrfs_inode_uid(path->nodes[0], ii);
> > > info->gid = btrfs_inode_gid(path->nodes[0], ii);
> > > info->rdev = btrfs_inode_rdev(path->nodes[0], ii);
> > > + info->nlink = btrfs_inode_nlink(path->nodes[0], ii);
> > > /*
> > > * Transfer the unchanged u64 value of btrfs_inode_item::flags, that's
> > > * otherwise logically split to 32/32 parts.
> > > @@ -1652,19 +1654,22 @@ static int get_cur_inode_state(struct send_ctx *sctx, u64 ino, u64 gen)
> > > int right_ret;
> > > u64 left_gen;
> > > u64 right_gen;
> > > + struct btrfs_inode_info info;
> > >
> > > - ret = get_inode_gen(sctx->send_root, ino, &left_gen);
> > > + ret = get_inode_info(sctx->send_root, ino, &info);
> > > if (ret < 0 && ret != -ENOENT)
> > > goto out;
> > > - left_ret = ret;
> > > + left_ret = (info.nlink == 0) ? -ENOENT : ret;
> > > + left_gen = info.gen;
> > >
> > > if (!sctx->parent_root) {
> > > right_ret = -ENOENT;
> > > } else {
> > > - ret = get_inode_gen(sctx->parent_root, ino, &right_gen);
> > > + ret = get_inode_info(sctx->parent_root, ino, &info);
> > > if (ret < 0 && ret != -ENOENT)
> > > goto out;
> > > - right_ret = ret;
> > > + right_ret = (info.nlink == 0) ? -ENOENT : ret;
> > > + right_gen = info.gen;
> > > }
> > >
> > > if (!left_ret && !right_ret) {
> > > @@ -6413,86 +6418,6 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end)
> > > return ret;
> > > }
> > >
> > > -struct parent_paths_ctx {
> > > - struct list_head *refs;
> > > - struct send_ctx *sctx;
> > > -};
> > > -
> > > -static int record_parent_ref(int num, u64 dir, int index, struct fs_path *name,
> > > - void *ctx)
> > > -{
> > > - struct parent_paths_ctx *ppctx = ctx;
> > > -
> > > - /*
> > > - * Pass 0 as the generation for the directory, we don't care about it
> > > - * here as we have no new references to add, we just want to delete all
> > > - * references for an inode.
> > > - */
> > > - return record_ref_in_tree(&ppctx->sctx->rbtree_deleted_refs, ppctx->refs,
> > > - name, dir, 0, ppctx->sctx);
> > > -}
> > > -
> > > -/*
> > > - * Issue unlink operations for all paths of the current inode found in the
> > > - * parent snapshot.
> > > - */
> > > -static int btrfs_unlink_all_paths(struct send_ctx *sctx)
> > > -{
> > > - LIST_HEAD(deleted_refs);
> > > - struct btrfs_path *path;
> > > - struct btrfs_root *root = sctx->parent_root;
> > > - struct btrfs_key key;
> > > - struct btrfs_key found_key;
> > > - struct parent_paths_ctx ctx;
> > > - int iter_ret = 0;
> > > - int ret;
> > > -
> > > - path = alloc_path_for_send();
> > > - if (!path)
> > > - return -ENOMEM;
> > > -
> > > - key.objectid = sctx->cur_ino;
> > > - key.type = BTRFS_INODE_REF_KEY;
> > > - key.offset = 0;
> > > -
> > > - ctx.refs = &deleted_refs;
> > > - ctx.sctx = sctx;
> > > -
> > > - btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
> > > - if (found_key.objectid != key.objectid)
> > > - break;
> > > - if (found_key.type != key.type &&
> > > - found_key.type != BTRFS_INODE_EXTREF_KEY)
> > > - break;
> > > -
> > > - ret = iterate_inode_ref(root, path, &found_key, 1,
> > > - record_parent_ref, &ctx);
> > > - if (ret < 0)
> > > - goto out;
> > > - }
> > > - /* Catch error found during iteration */
> > > - if (iter_ret < 0) {
> > > - ret = iter_ret;
> > > - goto out;
> > > - }
> > > -
> > > - while (!list_empty(&deleted_refs)) {
> > > - struct recorded_ref *ref;
> > > -
> > > - ref = list_first_entry(&deleted_refs, struct recorded_ref, list);
> > > - ret = send_unlink(sctx, ref->full_path);
> > > - if (ret < 0)
> > > - goto out;
> > > - recorded_ref_free(ref);
> > > - }
> > > - ret = 0;
> > > -out:
> > > - btrfs_free_path(path);
> > > - if (ret)
> > > - __free_recorded_refs(&deleted_refs);
> > > - return ret;
> > > -}
> > > -
> > > static void close_current_inode(struct send_ctx *sctx)
> > > {
> > > u64 i_size;
> > > @@ -6583,25 +6508,37 @@ static int changed_inode(struct send_ctx *sctx,
> > > * file descriptor against it or turning a RO snapshot into RW mode,
> > > * keep an open file descriptor against a file, delete it and then
> > > * turn the snapshot back to RO mode before using it for a send
> > > - * operation. So if we find such cases, ignore the inode and all its
> > > - * items completely if it's a new inode, or if it's a changed inode
> > > - * make sure all its previous paths (from the parent snapshot) are all
> > > - * unlinked and all other the inode items are ignored.
> > > + * operation. The former is what the receiver operation does.
> > > + * Therefore, if we want to send these snapshots soon after they're
> > > + * received, we need to handle orphan inodes as well. Moreover,
> > > + * orphans can appear not only in the send snapshot but also in the
> > > + * parent snapshot. Here are several cases:
> > > + *
> > > + * Case 1: BTRFS_COMPARE_TREE_NEW
> > > + * | send snapshot | action
> > > + * --------------------------------
> > > + * nlink | 0 | ignore
> > > + *
> > > + * Case 2: BTRFS_COMPARE_TREE_DELETED
> > > + * | parent snapshot | action
> > > + * ----------------------------------
> > > + * nlink | 0 | as usual
> > > + * Note: No unlinks will be sent because there're no paths for it.
> > > + *
> > > + * Case 3: BTRFS_COMPARE_TREE_CHANGED
> > > + * | | parent snapshot | send snapshot | action
> > > + * -----------------------------------------------------------------------
> > > + * subcase 1 | nlink | 0 | 0 | ignore
> > > + * subcase 2 | nlink | >0 | 0 | new_gen(deletion)
> > > + * subcase 3 | nlink | 0 | >0 | new_gen(creation)
> > > + *
> > > */
> > > - if (result == BTRFS_COMPARE_TREE_NEW ||
> > > - result == BTRFS_COMPARE_TREE_CHANGED) {
> > > - u32 nlinks;
> > > -
> > > - nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii);
> > > - if (nlinks == 0) {
> > > + if (result == BTRFS_COMPARE_TREE_NEW) {
> > > + if (btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii) ==
> > > + 0) {
> > > sctx->ignore_cur_inode = true;
> > > - if (result == BTRFS_COMPARE_TREE_CHANGED)
> > > - ret = btrfs_unlink_all_paths(sctx);
> > > goto out;
> > > }
> > > - }
> > > -
> > > - if (result == BTRFS_COMPARE_TREE_NEW) {
> > > sctx->cur_inode_gen = left_gen;
> > > sctx->cur_inode_new = true;
> > > sctx->cur_inode_deleted = false;
> > > @@ -6622,6 +6559,18 @@ static int changed_inode(struct send_ctx *sctx,
> > > sctx->cur_inode_mode = btrfs_inode_mode(
> > > sctx->right_path->nodes[0], right_ii);
> > > } else if (result == BTRFS_COMPARE_TREE_CHANGED) {
> > > + u32 new_nlinks, old_nlinks;
> > > +
> > > + new_nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0],
> > > + left_ii);
> > > + old_nlinks = btrfs_inode_nlink(sctx->right_path->nodes[0],
> > > + right_ii);
> > > + if (new_nlinks == 0 && old_nlinks == 0) {
> > > + sctx->ignore_cur_inode = true;
> > > + goto out;
> > > + } else if (new_nlinks == 0 || old_nlinks == 0) {
> > > + sctx->cur_inode_new_gen = 1;
> > > + }
> > > /*
> > > * We need to do some special handling in case the inode was
> > > * reported as changed with a changed generation number. This
> > > @@ -6648,38 +6597,45 @@ static int changed_inode(struct send_ctx *sctx,
> > > /*
> > > * Now process the inode as if it was new.
> > > */
> > > - sctx->cur_inode_gen = left_gen;
> > > - sctx->cur_inode_new = true;
> > > - sctx->cur_inode_deleted = false;
> > > - sctx->cur_inode_size = btrfs_inode_size(
> > > - 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);
> > > - ret = send_create_inode_if_needed(sctx);
> > > - if (ret < 0)
> > > - goto out;
> > > + if (new_nlinks > 0) {
> > > + sctx->cur_inode_gen = left_gen;
> > > + sctx->cur_inode_new = true;
> > > + sctx->cur_inode_deleted = false;
> > > + sctx->cur_inode_size = btrfs_inode_size(
> > > + 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);
> > > + ret = send_create_inode_if_needed(sctx);
> > > + if (ret < 0)
> > > + goto out;
> > >
> > > - ret = process_all_refs(sctx, BTRFS_COMPARE_TREE_NEW);
> > > - if (ret < 0)
> > > - goto out;
> > > - /*
> > > - * Advance send_progress now as we did not get into
> > > - * process_recorded_refs_if_needed in the new_gen case.
> > > - */
> > > - sctx->send_progress = sctx->cur_ino + 1;
> > > + ret = process_all_refs(sctx,
> > > + BTRFS_COMPARE_TREE_NEW);
> > > + if (ret < 0)
> > > + goto out;
> > > + /*
> > > + * Advance send_progress now as we did not get
> > > + * into process_recorded_refs_if_needed in the
> > > + * new_gen case.
> > > + */
> > > + sctx->send_progress = sctx->cur_ino + 1;
> > >
> > > - /*
> > > - * Now process all extents and xattrs of the inode as if
> > > - * they were all new.
> > > - */
> > > - ret = process_all_extents(sctx);
> > > - if (ret < 0)
> > > - goto out;
> > > - ret = process_all_new_xattrs(sctx);
> > > - if (ret < 0)
> > > - goto out;
> > > + /*
> > > + * Now process all extents and xattrs of the
> > > + * inode as if they were all new.
> > > + */
> > > + ret = process_all_extents(sctx);
> > > + if (ret < 0)
> > > + goto out;
> > > + ret = process_all_new_xattrs(sctx);
> > > + if (ret < 0)
> > > + goto out;
> > > + }
> > > } else {
> > > sctx->cur_inode_gen = left_gen;
> > > sctx->cur_inode_new = false;
> > > --
> > > 2.37.1
> > >

2022-08-18 14:22:49

by David Sterba

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] btrfs: send: fix failures when processing inodes with no links

On Fri, Aug 12, 2022 at 10:36:38PM +0800, bingjing chang wrote:
> > I seriously doubt that those 4 commits are the only dependencies in
> > order to be able to cleanly backport to 4.9 and other old branches.
> >
> > It may be better to backport only to a few younger stable branches, or
> > just provide later a version of the patch to
> > apply to each desired stable branch (once the fix is in Linus' tree
> > and in a -rc release).
> >
> > If you are not interested in backporting to stable or don't have the
> > time to verify the dependencies and test, then just remove all the
> > stable tags.
> > Just leave a fixes tag:
> >
> > Fixes: 31db9f7c23fbf7 ("Btrfs: introduce BTRFS_IOC_SEND for btrfs send/receive")
>
> Since backporting is not our goal. I will just leave the fix tag here.

This Fixes: points to the original send patch, so that's not really
useful, otherwise if there's a target stable release where the patches
still apply cleanly, or with minimal conflicts it's sufficient to add a
CC: stable tag it's good to have it.

2022-09-22 10:43:50

by Filipe Manana

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] btrfs: send: fix failures when processing inodes with no links

,

On Fri, Aug 12, 2022 at 3:36 PM bingjing chang <[email protected]> wrote:
>
> Filipe Manana <[email protected]> 於 2022年8月11日週四晚上8:00寫道：
> >
> > On Thu, Aug 11, 2022 at 11:09 AM bingjingc <[email protected]> wrote:
> > >
> > > From: BingJing Chang <[email protected]>
> > >
> > > There is a bug causing send failures when processing an orphan directory
> > > with no links. In commit 46b2f4590aab ("Btrfs: fix send failure when root
> > > has deleted files still open")', the orphan inode issue was addressed. The
> > > send operation fails with a ENOENT error because of any attempts to
> > > generate a path for the inode with a link count of zero. Therefore, in that
> > > patch, sctx->ignore_cur_inode was introduced to be set if the current inode
> > > has a link count of zero for bypassing some unnecessary steps. And a helper
> > > function btrfs_unlink_all_paths() was introduced and called to clean up old
> > > paths found in the parent snapshot. However, not only regular files but
> > > also directories can be orphan inodes. So if the send operation meets an
> > > orphan directory, it will issue a wrong unlink command for that directory
> > > now. Soon the receive operation fails with a EISDIR error. Besides, the
> > > send operation also fails with a ENOENT error later when it tries to
> > > generate a path of it.
> > >
> > > Similar example but making an orphan dir for an incremental send:
> > >
> > > $ btrfs subvolume create vol
> > > $ mkdir vol/dir
> > > $ touch vol/dir/foo
> > >
> > > $ btrfs subvolume snapshot -r vol snap1
> > > $ btrfs subvolume snapshot -r vol snap2
> > >
> > > # Turn the second snapshot to RW mode and delete the whole dir while
> > > # holding an open file descriptor on it.
> > > $ btrfs property set snap2 ro false
> > > $ exec 73<snap2/dir
> > > $ rm -rf snap2/dir
> > >
> > > # Set the second snapshot back to RO mode and do an incremental send.
> > > $ btrfs property set snap2 ro true
> > > $ mkdir receive_dir
> > > $ btrfs send snap2 -p snap1 | btrfs receive receive_dir/
> > > At subvol snap2
> > > At snapshot snap2
> > > ERROR: send ioctl failed with -2: No such file or directory
> > > ERROR: unlink dir failed. Is a directory
> > >
> > > Actually, orphan inodes are more common use cases in cascading backups.
> > > (Please see the illustration below.) In a cascading backup, a user wants
> > > to replicate a couple of snapshots from Machine A to Machine B and from
> > > Machine B to Machine C. Machine B doesn't take any RO snapshots for
> > > sending. All a receiver does is create an RW snapshot of its parent
> > > snapshot, apply the send stream and turn it into RO mode at the end. Even
> > > if all paths of some inodes are deleted in applying the send stream, these
> > > inodes would not be deleted and become orphans after changing the subvolume
> > > from RW to RO. Moreover, orphan inodes can occur not only in send snapshots
> > > but also in parent snapshots because Machine B may do a batch replication
> > > of a couple of snapshots.
> > >
> > > An illustration for cascading backups:
> > > Machine A (snapshot {1..n}) --> Machine B --> Machine C
> > >
> > > The intuition to solve the problem is to delete all the items of orphan
> > > inodes before using these snapshots for sending. I used to think that the
> > > reasonable timing for doing that is during the ioctl of changing the
> > > subvolume from RW to RO because it sounds good that we will not modify the
> > > fs tree of a RO snapshot anymore. However, attempting to do the orphan
> > > cleanup in the ioctl would be pointless. Because if someone is holding an
> > > open file descriptor on the inode, the reference count of the inode will
> > > never drop to 0. Then iput() cannot trigger eviction, which finally deletes
> > > all the items of it. So we try to extend the original patch to handle
> > > orphans in send/parent snapshots. Here are several cases that need to be
> > > considered:
> > >
> > > Case 1: BTRFS_COMPARE_TREE_NEW
> > > | send snapshot | action
> > > --------------------------------
> > > nlink | 0 | ignore
> > >
> > > In case 1, when we get a BTRFS_COMPARE_TREE_NEW tree comparison result,
> > > it means that a new inode is found in the send snapshot and it doesn't
> > > appear in the parent snapshot. Since this inode has a link count of zero
> > > (It's an orphan and there're no paths for it.), we can leverage
> > > sctx->ignore_cur_inode in the original patch to prevent it from being
> > > created.
> > >
> > > Case 2: BTRFS_COMPARE_TREE_DELETED
> > > | parent snapshot | action
> > > ----------------------------------
> > > nlink | 0 | as usual
> > >
> > > In case 2, when we get a BTRFS_COMPARE_TREE_DELETED tree comparison
> > > result, it means that the inode only appears in the parent snapshot.
> > > As usual, the send operation will try to delete all its paths. However,
> > > this inode has a link count of zero, so no paths of it will be found. No
> > > deletion operations will be issued. We don't need to change any logic.
> > >
> > > Case 3: BTRFS_COMPARE_TREE_CHANGED
> > > | | parent snapshot | send snapshot | action
> > > -----------------------------------------------------------------------
> > > subcase 1 | nlink | 0 | 0 | ignore
> > > subcase 2 | nlink | >0 | 0 | new_gen(deletion)
> > > subcase 3 | nlink | 0 | >0 | new_gen(creation)
> > >
> > > In case 3, when we get a BTRFS_COMPARE_TREE_CHANGED tree comparison result,
> > > it means that the inode appears in both snapshots. Here're three subcases.
> > >
> > > First, if the inode has link counts of zero in both snapshots. Since there
> > > are no paths for this inode in (source/destination) parent snapshots and we
> > > don't care about whether there is also an orphan inode in destination or
> > > not, we can set sctx->ignore_cur_inode on to prevent it from being created.
> > >
> > > For the second and the third subcases, if there're paths in one snapshot
> > > and there're no paths in the other snapshot for this inode. We can treat
> > > this inode as a new generation. We can also leverage the logic handling a
> > > new generation of an inode with small adjustments. Then it will delete all
> > > old paths and create a new inode with new attributes and paths only when
> > > there's a positive link count in the send snapshot. In subcase 2, the
> > > send operation only needs to delete all old paths as in the parent
> > > snapshot. But it may require more operations for a directory to remove its
> > > old paths. If a not-empty directory is going to be deleted (because it has
> > > a link count of zero in the send snapshot) but there're files/directories
> > > with bigger inode numbers under it, the send operation will need to rename
> > > it to its orphan name first. After processing and deleting the last item
> > > under this directory, the send operation will check this directory, aka
> > > the parent directory of the last item, again and issue a rmdir operation
> > > to remove it finally. Therefore, we also need to treat inodes with a link
> > > count of zero as if they didn't exist in get_cur_inode_state(), which is
> > > used in process_recorded_refs(). By doing this, when reviewing a directory
> > > with orphan names after the last item under it has been deleted, the send
> > > operation now can properly issue a rmdir operation. Otherwise, without
> > > doing this, the orphan directory with an orphan name would be kept here
> > > at the end due to the existing inode with a link count of zero being found.
> > > In subcase 3, as in case 2, no old paths would be found, so no deletion
> > > operations will be issued. The send operation will only create a new one
> > > for that inode.
> > >
> > > Note that subcase 3 is not a common case. That's because it's easy to
> > > reduce the hard links of an inode, but once all valid paths are removed,
> > > there're no valid paths for creating other hard links. The only way to do
> > > that is trying to send an older snapshot after a newer snapshot has been
> > > sent.
> > >
> > > Cc: <[email protected]> # 4.9: 46b2f4590aab: Btrfs: fix send
> > > failure when root has deleted files still open
> > > Cc: <[email protected]> # 4.9: 71ecfc133b03: btrfs: send:
> > > introduce recorded_ref_alloc and recorded_ref_free
> > > Cc: <[email protected]> # 4.9: 3aa5bd367fa5: btrfs: send: fix
> > > sending link commands for existing file paths
> > > Cc: <[email protected]> # 4.9: 0d8869fb6b6f8: btrfs: send: always
> > > use the rbtree based inode ref management infrastructure
> >
> > Btw, lines with CC, Fixes, etc, tags should not be broken even if they
> > are wider than 74 characters.
> >
>
> Okay, thank you for telling me that.
>
> > So, in v1 when I gave you that example of CC stable tags, it wasn't
> > meant for you to literally copy-paste them.
> >
> > First I asked if the purpose of the original Fixes tag was to backport
> > the fix to stable releases.
> > Was that the intention? You didn't provide an answer about that.
> >
>
> Oh, I misunderstood your suggestion. I'm sorry about that.
> Our intention is to report this bug and try to provide a reasonable and
> acceptable fix for it. Backporting is not our goal.
>
> > Then I told if that was the case, the proper way would be adding CC
> > stable tags and listing any
> > dependencies. I gave those 4 as examples with commits that are fairly
> > recent and obvious dependencies,
> > but I also said that probably there's a lot more missing - especially
> > if we want to backport to as far as 4.9.
> >
>
> > Even with just those 4 dependencies, some of those commits are fairly
> > large, and that may be frowned upon
> > according to stable backport rules (listed at
> > https://www.kernel.org/doc/Documentation/process/stable-kernel-rules.rst).
> > For e.g., patches with over 100 lines changed.
> >
> > Now, did you actually verify if there were more dependencies? (and test)
> > And do you really want to go as far as 4.9 (currently the oldest
> > stable release)?
>
> No, I didn't. I used to think the CC tag was a very cool feature, which
> just putting a few commits lets backport easily when I read your mail,
> so I copied and pasted these 4 commits in the beginning of revising
> the patch v2. However, I'm wrong.
>
> > I seriously doubt that those 4 commits are the only dependencies in
> > order to be able to cleanly backport to 4.9 and other old branches.
> >
> > It may be better to backport only to a few younger stable branches, or
> > just provide later a version of the patch to
> > apply to each desired stable branch (once the fix is in Linus' tree
> > and in a -rc release).
> >
> > If you are not interested in backporting to stable or don't have the
> > time to verify the dependencies and test, then just remove all the
> > stable tags.
> > Just leave a fixes tag:
> >
> > Fixes: 31db9f7c23fbf7 ("Btrfs: introduce BTRFS_IOC_SEND for btrfs send/receive")
> >
>
> Since backporting is not our goal. I will just leave the fix tag here.
>
> > Also, please don't forget to send a test case for fstests, covering as
> > many cases as possible (not just the example
> > at the beginning of the changelog).
> >
>
> Okay, I will submit a test case covering all cases.
> Because I still need to spend time learning how to use the fssum utility
> for the last test case you reviewed, so I will submit the test case later.

BingJing, any progress with the test case?
We would love to have that in fstests to help prevent regressions in the future.

Thanks.

>
> Thanks.
>
> > Thanks.
> >
> > > Reviewed-by: Robbie Ko <[email protected]>
> > > Signed-off-by: BingJing Chang <[email protected]>
> > > ---
> > > fs/btrfs/send.c | 214 +++++++++++++++++++-----------------------------
> > > 1 file changed, 85 insertions(+), 129 deletions(-)
> > >
> > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> > > index f8d77a33b9b7..6ab1ba66ff4b 100644
> > > --- a/fs/btrfs/send.c
> > > +++ b/fs/btrfs/send.c
> > > @@ -850,6 +850,7 @@ struct btrfs_inode_info {
> > > u64 gid;
> > > u64 rdev;
> > > u64 attr;
> > > + u64 nlink;
> > > };
> > >
> > > /*
> > > @@ -888,6 +889,7 @@ static int get_inode_info(struct btrfs_root *root, u64 ino,
> > > info->uid = btrfs_inode_uid(path->nodes[0], ii);
> > > info->gid = btrfs_inode_gid(path->nodes[0], ii);
> > > info->rdev = btrfs_inode_rdev(path->nodes[0], ii);
> > > + info->nlink = btrfs_inode_nlink(path->nodes[0], ii);
> > > /*
> > > * Transfer the unchanged u64 value of btrfs_inode_item::flags, that's
> > > * otherwise logically split to 32/32 parts.
> > > @@ -1652,19 +1654,22 @@ static int get_cur_inode_state(struct send_ctx *sctx, u64 ino, u64 gen)
> > > int right_ret;
> > > u64 left_gen;
> > > u64 right_gen;
> > > + struct btrfs_inode_info info;
> > >
> > > - ret = get_inode_gen(sctx->send_root, ino, &left_gen);
> > > + ret = get_inode_info(sctx->send_root, ino, &info);
> > > if (ret < 0 && ret != -ENOENT)
> > > goto out;
> > > - left_ret = ret;
> > > + left_ret = (info.nlink == 0) ? -ENOENT : ret;
> > > + left_gen = info.gen;
> > >
> > > if (!sctx->parent_root) {
> > > right_ret = -ENOENT;
> > > } else {
> > > - ret = get_inode_gen(sctx->parent_root, ino, &right_gen);
> > > + ret = get_inode_info(sctx->parent_root, ino, &info);
> > > if (ret < 0 && ret != -ENOENT)
> > > goto out;
> > > - right_ret = ret;
> > > + right_ret = (info.nlink == 0) ? -ENOENT : ret;
> > > + right_gen = info.gen;
> > > }
> > >
> > > if (!left_ret && !right_ret) {
> > > @@ -6413,86 +6418,6 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end)
> > > return ret;
> > > }
> > >
> > > -struct parent_paths_ctx {
> > > - struct list_head *refs;
> > > - struct send_ctx *sctx;
> > > -};
> > > -
> > > -static int record_parent_ref(int num, u64 dir, int index, struct fs_path *name,
> > > - void *ctx)
> > > -{
> > > - struct parent_paths_ctx *ppctx = ctx;
> > > -
> > > - /*
> > > - * Pass 0 as the generation for the directory, we don't care about it
> > > - * here as we have no new references to add, we just want to delete all
> > > - * references for an inode.
> > > - */
> > > - return record_ref_in_tree(&ppctx->sctx->rbtree_deleted_refs, ppctx->refs,
> > > - name, dir, 0, ppctx->sctx);
> > > -}
> > > -
> > > -/*
> > > - * Issue unlink operations for all paths of the current inode found in the
> > > - * parent snapshot.
> > > - */
> > > -static int btrfs_unlink_all_paths(struct send_ctx *sctx)
> > > -{
> > > - LIST_HEAD(deleted_refs);
> > > - struct btrfs_path *path;
> > > - struct btrfs_root *root = sctx->parent_root;
> > > - struct btrfs_key key;
> > > - struct btrfs_key found_key;
> > > - struct parent_paths_ctx ctx;
> > > - int iter_ret = 0;
> > > - int ret;
> > > -
> > > - path = alloc_path_for_send();
> > > - if (!path)
> > > - return -ENOMEM;
> > > -
> > > - key.objectid = sctx->cur_ino;
> > > - key.type = BTRFS_INODE_REF_KEY;
> > > - key.offset = 0;
> > > -
> > > - ctx.refs = &deleted_refs;
> > > - ctx.sctx = sctx;
> > > -
> > > - btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
> > > - if (found_key.objectid != key.objectid)
> > > - break;
> > > - if (found_key.type != key.type &&
> > > - found_key.type != BTRFS_INODE_EXTREF_KEY)
> > > - break;
> > > -
> > > - ret = iterate_inode_ref(root, path, &found_key, 1,
> > > - record_parent_ref, &ctx);
> > > - if (ret < 0)
> > > - goto out;
> > > - }
> > > - /* Catch error found during iteration */
> > > - if (iter_ret < 0) {
> > > - ret = iter_ret;
> > > - goto out;
> > > - }
> > > -
> > > - while (!list_empty(&deleted_refs)) {
> > > - struct recorded_ref *ref;
> > > -
> > > - ref = list_first_entry(&deleted_refs, struct recorded_ref, list);
> > > - ret = send_unlink(sctx, ref->full_path);
> > > - if (ret < 0)
> > > - goto out;
> > > - recorded_ref_free(ref);
> > > - }
> > > - ret = 0;
> > > -out:
> > > - btrfs_free_path(path);
> > > - if (ret)
> > > - __free_recorded_refs(&deleted_refs);
> > > - return ret;
> > > -}
> > > -
> > > static void close_current_inode(struct send_ctx *sctx)
> > > {
> > > u64 i_size;
> > > @@ -6583,25 +6508,37 @@ static int changed_inode(struct send_ctx *sctx,
> > > * file descriptor against it or turning a RO snapshot into RW mode,
> > > * keep an open file descriptor against a file, delete it and then
> > > * turn the snapshot back to RO mode before using it for a send
> > > - * operation. So if we find such cases, ignore the inode and all its
> > > - * items completely if it's a new inode, or if it's a changed inode
> > > - * make sure all its previous paths (from the parent snapshot) are all
> > > - * unlinked and all other the inode items are ignored.
> > > + * operation. The former is what the receiver operation does.
> > > + * Therefore, if we want to send these snapshots soon after they're
> > > + * received, we need to handle orphan inodes as well. Moreover,
> > > + * orphans can appear not only in the send snapshot but also in the
> > > + * parent snapshot. Here are several cases:
> > > + *
> > > + * Case 1: BTRFS_COMPARE_TREE_NEW
> > > + * | send snapshot | action
> > > + * --------------------------------
> > > + * nlink | 0 | ignore
> > > + *
> > > + * Case 2: BTRFS_COMPARE_TREE_DELETED
> > > + * | parent snapshot | action
> > > + * ----------------------------------
> > > + * nlink | 0 | as usual
> > > + * Note: No unlinks will be sent because there're no paths for it.
> > > + *
> > > + * Case 3: BTRFS_COMPARE_TREE_CHANGED
> > > + * | | parent snapshot | send snapshot | action
> > > + * -----------------------------------------------------------------------
> > > + * subcase 1 | nlink | 0 | 0 | ignore
> > > + * subcase 2 | nlink | >0 | 0 | new_gen(deletion)
> > > + * subcase 3 | nlink | 0 | >0 | new_gen(creation)
> > > + *
> > > */
> > > - if (result == BTRFS_COMPARE_TREE_NEW ||
> > > - result == BTRFS_COMPARE_TREE_CHANGED) {
> > > - u32 nlinks;
> > > -
> > > - nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii);
> > > - if (nlinks == 0) {
> > > + if (result == BTRFS_COMPARE_TREE_NEW) {
> > > + if (btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii) ==
> > > + 0) {
> > > sctx->ignore_cur_inode = true;
> > > - if (result == BTRFS_COMPARE_TREE_CHANGED)
> > > - ret = btrfs_unlink_all_paths(sctx);
> > > goto out;
> > > }
> > > - }
> > > -
> > > - if (result == BTRFS_COMPARE_TREE_NEW) {
> > > sctx->cur_inode_gen = left_gen;
> > > sctx->cur_inode_new = true;
> > > sctx->cur_inode_deleted = false;
> > > @@ -6622,6 +6559,18 @@ static int changed_inode(struct send_ctx *sctx,
> > > sctx->cur_inode_mode = btrfs_inode_mode(
> > > sctx->right_path->nodes[0], right_ii);
> > > } else if (result == BTRFS_COMPARE_TREE_CHANGED) {
> > > + u32 new_nlinks, old_nlinks;
> > > +
> > > + new_nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0],
> > > + left_ii);
> > > + old_nlinks = btrfs_inode_nlink(sctx->right_path->nodes[0],
> > > + right_ii);
> > > + if (new_nlinks == 0 && old_nlinks == 0) {
> > > + sctx->ignore_cur_inode = true;
> > > + goto out;
> > > + } else if (new_nlinks == 0 || old_nlinks == 0) {
> > > + sctx->cur_inode_new_gen = 1;
> > > + }
> > > /*
> > > * We need to do some special handling in case the inode was
> > > * reported as changed with a changed generation number. This
> > > @@ -6648,38 +6597,45 @@ static int changed_inode(struct send_ctx *sctx,
> > > /*
> > > * Now process the inode as if it was new.
> > > */
> > > - sctx->cur_inode_gen = left_gen;
> > > - sctx->cur_inode_new = true;
> > > - sctx->cur_inode_deleted = false;
> > > - sctx->cur_inode_size = btrfs_inode_size(
> > > - 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);
> > > - ret = send_create_inode_if_needed(sctx);
> > > - if (ret < 0)
> > > - goto out;
> > > + if (new_nlinks > 0) {
> > > + sctx->cur_inode_gen = left_gen;
> > > + sctx->cur_inode_new = true;
> > > + sctx->cur_inode_deleted = false;
> > > + sctx->cur_inode_size = btrfs_inode_size(
> > > + 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);
> > > + ret = send_create_inode_if_needed(sctx);
> > > + if (ret < 0)
> > > + goto out;
> > >
> > > - ret = process_all_refs(sctx, BTRFS_COMPARE_TREE_NEW);
> > > - if (ret < 0)
> > > - goto out;
> > > - /*
> > > - * Advance send_progress now as we did not get into
> > > - * process_recorded_refs_if_needed in the new_gen case.
> > > - */
> > > - sctx->send_progress = sctx->cur_ino + 1;
> > > + ret = process_all_refs(sctx,
> > > + BTRFS_COMPARE_TREE_NEW);
> > > + if (ret < 0)
> > > + goto out;
> > > + /*
> > > + * Advance send_progress now as we did not get
> > > + * into process_recorded_refs_if_needed in the
> > > + * new_gen case.
> > > + */
> > > + sctx->send_progress = sctx->cur_ino + 1;
> > >
> > > - /*
> > > - * Now process all extents and xattrs of the inode as if
> > > - * they were all new.
> > > - */
> > > - ret = process_all_extents(sctx);
> > > - if (ret < 0)
> > > - goto out;
> > > - ret = process_all_new_xattrs(sctx);
> > > - if (ret < 0)
> > > - goto out;
> > > + /*
> > > + * Now process all extents and xattrs of the
> > > + * inode as if they were all new.
> > > + */
> > > + ret = process_all_extents(sctx);
> > > + if (ret < 0)
> > > + goto out;
> > > + ret = process_all_new_xattrs(sctx);
> > > + if (ret < 0)
> > > + goto out;
> > > + }
> > > } else {
> > > sctx->cur_inode_gen = left_gen;
> > > sctx->cur_inode_new = false;
> > > --
> > > 2.37.1
> > >

2022-09-23 14:43:03

by bingjing chang

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] btrfs: send: fix failures when processing inodes with no links

Filipe Manana <[email protected]> 於 2022年9月22日週四下午6:08寫道：
>
> ,
>
> On Fri, Aug 12, 2022 at 3:36 PM bingjing chang <[email protected]> wrote:
> >
> > Filipe Manana <[email protected]> 於 2022年8月11日週四晚上8:00寫道：
> > >
> > > On Thu, Aug 11, 2022 at 11:09 AM bingjingc <[email protected]> wrote:
> > > >
> > > > From: BingJing Chang <[email protected]>
> > > >
> > > > There is a bug causing send failures when processing an orphan directory
> > > > with no links. In commit 46b2f4590aab ("Btrfs: fix send failure when root
> > > > has deleted files still open")', the orphan inode issue was addressed. The
> > > > send operation fails with a ENOENT error because of any attempts to
> > > > generate a path for the inode with a link count of zero. Therefore, in that
> > > > patch, sctx->ignore_cur_inode was introduced to be set if the current inode
> > > > has a link count of zero for bypassing some unnecessary steps. And a helper
> > > > function btrfs_unlink_all_paths() was introduced and called to clean up old
> > > > paths found in the parent snapshot. However, not only regular files but
> > > > also directories can be orphan inodes. So if the send operation meets an
> > > > orphan directory, it will issue a wrong unlink command for that directory
> > > > now. Soon the receive operation fails with a EISDIR error. Besides, the
> > > > send operation also fails with a ENOENT error later when it tries to
> > > > generate a path of it.
> > > >
> > > > Similar example but making an orphan dir for an incremental send:
> > > >
> > > > $ btrfs subvolume create vol
> > > > $ mkdir vol/dir
> > > > $ touch vol/dir/foo
> > > >
> > > > $ btrfs subvolume snapshot -r vol snap1
> > > > $ btrfs subvolume snapshot -r vol snap2
> > > >
> > > > # Turn the second snapshot to RW mode and delete the whole dir while
> > > > # holding an open file descriptor on it.
> > > > $ btrfs property set snap2 ro false
> > > > $ exec 73<snap2/dir
> > > > $ rm -rf snap2/dir
> > > >
> > > > # Set the second snapshot back to RO mode and do an incremental send.
> > > > $ btrfs property set snap2 ro true
> > > > $ mkdir receive_dir
> > > > $ btrfs send snap2 -p snap1 | btrfs receive receive_dir/
> > > > At subvol snap2
> > > > At snapshot snap2
> > > > ERROR: send ioctl failed with -2: No such file or directory
> > > > ERROR: unlink dir failed. Is a directory
> > > >
> > > > Actually, orphan inodes are more common use cases in cascading backups.
> > > > (Please see the illustration below.) In a cascading backup, a user wants
> > > > to replicate a couple of snapshots from Machine A to Machine B and from
> > > > Machine B to Machine C. Machine B doesn't take any RO snapshots for
> > > > sending. All a receiver does is create an RW snapshot of its parent
> > > > snapshot, apply the send stream and turn it into RO mode at the end. Even
> > > > if all paths of some inodes are deleted in applying the send stream, these
> > > > inodes would not be deleted and become orphans after changing the subvolume
> > > > from RW to RO. Moreover, orphan inodes can occur not only in send snapshots
> > > > but also in parent snapshots because Machine B may do a batch replication
> > > > of a couple of snapshots.
> > > >
> > > > An illustration for cascading backups:
> > > > Machine A (snapshot {1..n}) --> Machine B --> Machine C
> > > >
> > > > The intuition to solve the problem is to delete all the items of orphan
> > > > inodes before using these snapshots for sending. I used to think that the
> > > > reasonable timing for doing that is during the ioctl of changing the
> > > > subvolume from RW to RO because it sounds good that we will not modify the
> > > > fs tree of a RO snapshot anymore. However, attempting to do the orphan
> > > > cleanup in the ioctl would be pointless. Because if someone is holding an
> > > > open file descriptor on the inode, the reference count of the inode will
> > > > never drop to 0. Then iput() cannot trigger eviction, which finally deletes
> > > > all the items of it. So we try to extend the original patch to handle
> > > > orphans in send/parent snapshots. Here are several cases that need to be
> > > > considered:
> > > >
> > > > Case 1: BTRFS_COMPARE_TREE_NEW
> > > > | send snapshot | action
> > > > --------------------------------
> > > > nlink | 0 | ignore
> > > >
> > > > In case 1, when we get a BTRFS_COMPARE_TREE_NEW tree comparison result,
> > > > it means that a new inode is found in the send snapshot and it doesn't
> > > > appear in the parent snapshot. Since this inode has a link count of zero
> > > > (It's an orphan and there're no paths for it.), we can leverage
> > > > sctx->ignore_cur_inode in the original patch to prevent it from being
> > > > created.
> > > >
> > > > Case 2: BTRFS_COMPARE_TREE_DELETED
> > > > | parent snapshot | action
> > > > ----------------------------------
> > > > nlink | 0 | as usual
> > > >
> > > > In case 2, when we get a BTRFS_COMPARE_TREE_DELETED tree comparison
> > > > result, it means that the inode only appears in the parent snapshot.
> > > > As usual, the send operation will try to delete all its paths. However,
> > > > this inode has a link count of zero, so no paths of it will be found. No
> > > > deletion operations will be issued. We don't need to change any logic.
> > > >
> > > > Case 3: BTRFS_COMPARE_TREE_CHANGED
> > > > | | parent snapshot | send snapshot | action
> > > > -----------------------------------------------------------------------
> > > > subcase 1 | nlink | 0 | 0 | ignore
> > > > subcase 2 | nlink | >0 | 0 | new_gen(deletion)
> > > > subcase 3 | nlink | 0 | >0 | new_gen(creation)
> > > >
> > > > In case 3, when we get a BTRFS_COMPARE_TREE_CHANGED tree comparison result,
> > > > it means that the inode appears in both snapshots. Here're three subcases.
> > > >
> > > > First, if the inode has link counts of zero in both snapshots. Since there
> > > > are no paths for this inode in (source/destination) parent snapshots and we
> > > > don't care about whether there is also an orphan inode in destination or
> > > > not, we can set sctx->ignore_cur_inode on to prevent it from being created.
> > > >
> > > > For the second and the third subcases, if there're paths in one snapshot
> > > > and there're no paths in the other snapshot for this inode. We can treat
> > > > this inode as a new generation. We can also leverage the logic handling a
> > > > new generation of an inode with small adjustments. Then it will delete all
> > > > old paths and create a new inode with new attributes and paths only when
> > > > there's a positive link count in the send snapshot. In subcase 2, the
> > > > send operation only needs to delete all old paths as in the parent
> > > > snapshot. But it may require more operations for a directory to remove its
> > > > old paths. If a not-empty directory is going to be deleted (because it has
> > > > a link count of zero in the send snapshot) but there're files/directories
> > > > with bigger inode numbers under it, the send operation will need to rename
> > > > it to its orphan name first. After processing and deleting the last item
> > > > under this directory, the send operation will check this directory, aka
> > > > the parent directory of the last item, again and issue a rmdir operation
> > > > to remove it finally. Therefore, we also need to treat inodes with a link
> > > > count of zero as if they didn't exist in get_cur_inode_state(), which is
> > > > used in process_recorded_refs(). By doing this, when reviewing a directory
> > > > with orphan names after the last item under it has been deleted, the send
> > > > operation now can properly issue a rmdir operation. Otherwise, without
> > > > doing this, the orphan directory with an orphan name would be kept here
> > > > at the end due to the existing inode with a link count of zero being found.
> > > > In subcase 3, as in case 2, no old paths would be found, so no deletion
> > > > operations will be issued. The send operation will only create a new one
> > > > for that inode.
> > > >
> > > > Note that subcase 3 is not a common case. That's because it's easy to
> > > > reduce the hard links of an inode, but once all valid paths are removed,
> > > > there're no valid paths for creating other hard links. The only way to do
> > > > that is trying to send an older snapshot after a newer snapshot has been
> > > > sent.
> > > >
> > > > Cc: <[email protected]> # 4.9: 46b2f4590aab: Btrfs: fix send
> > > > failure when root has deleted files still open
> > > > Cc: <[email protected]> # 4.9: 71ecfc133b03: btrfs: send:
> > > > introduce recorded_ref_alloc and recorded_ref_free
> > > > Cc: <[email protected]> # 4.9: 3aa5bd367fa5: btrfs: send: fix
> > > > sending link commands for existing file paths
> > > > Cc: <[email protected]> # 4.9: 0d8869fb6b6f8: btrfs: send: always
> > > > use the rbtree based inode ref management infrastructure
> > >
> > > Btw, lines with CC, Fixes, etc, tags should not be broken even if they
> > > are wider than 74 characters.
> > >
> >
> > Okay, thank you for telling me that.
> >
> > > So, in v1 when I gave you that example of CC stable tags, it wasn't
> > > meant for you to literally copy-paste them.
> > >
> > > First I asked if the purpose of the original Fixes tag was to backport
> > > the fix to stable releases.
> > > Was that the intention? You didn't provide an answer about that.
> > >
> >
> > Oh, I misunderstood your suggestion. I'm sorry about that.
> > Our intention is to report this bug and try to provide a reasonable and
> > acceptable fix for it. Backporting is not our goal.
> >
> > > Then I told if that was the case, the proper way would be adding CC
> > > stable tags and listing any
> > > dependencies. I gave those 4 as examples with commits that are fairly
> > > recent and obvious dependencies,
> > > but I also said that probably there's a lot more missing - especially
> > > if we want to backport to as far as 4.9.
> > >
> >
> > > Even with just those 4 dependencies, some of those commits are fairly
> > > large, and that may be frowned upon
> > > according to stable backport rules (listed at
> > > https://www.kernel.org/doc/Documentation/process/stable-kernel-rules.rst).
> > > For e.g., patches with over 100 lines changed.
> > >
> > > Now, did you actually verify if there were more dependencies? (and test)
> > > And do you really want to go as far as 4.9 (currently the oldest
> > > stable release)?
> >
> > No, I didn't. I used to think the CC tag was a very cool feature, which
> > just putting a few commits lets backport easily when I read your mail,
> > so I copied and pasted these 4 commits in the beginning of revising
> > the patch v2. However, I'm wrong.
> >
> > > I seriously doubt that those 4 commits are the only dependencies in
> > > order to be able to cleanly backport to 4.9 and other old branches.
> > >
> > > It may be better to backport only to a few younger stable branches, or
> > > just provide later a version of the patch to
> > > apply to each desired stable branch (once the fix is in Linus' tree
> > > and in a -rc release).
> > >
> > > If you are not interested in backporting to stable or don't have the
> > > time to verify the dependencies and test, then just remove all the
> > > stable tags.
> > > Just leave a fixes tag:
> > >
> > > Fixes: 31db9f7c23fbf7 ("Btrfs: introduce BTRFS_IOC_SEND for btrfs send/receive")
> > >
> >
> > Since backporting is not our goal. I will just leave the fix tag here.
> >
> > > Also, please don't forget to send a test case for fstests, covering as
> > > many cases as possible (not just the example
> > > at the beginning of the changelog).
> > >
> >
> > Okay, I will submit a test case covering all cases.
> > Because I still need to spend time learning how to use the fssum utility
> > for the last test case you reviewed, so I will submit the test case later.
>
> BingJing, any progress with the test case?
> We would love to have that in fstests to help prevent regressions in the future.
>
> Thanks.
>

Hi Filipe,

I used to wait the git pull of my patch to the master branch, then I can
attach the commit uuid in tests as before. However, it seems that there're
troubles for the maintainer to pull all of the bug fixes into the new
kernel 6.0 at once.

Okay, I will try to create a test covering the cases this weekend.
Sorry for the inconvenience made.

Thanks.

> >
> > Thanks.
> >
> > > Thanks.
> > >
> > > > Reviewed-by: Robbie Ko <[email protected]>
> > > > Signed-off-by: BingJing Chang <[email protected]>
> > > > ---
> > > > fs/btrfs/send.c | 214 +++++++++++++++++++-----------------------------
> > > > 1 file changed, 85 insertions(+), 129 deletions(-)
> > > >
> > > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> > > > index f8d77a33b9b7..6ab1ba66ff4b 100644
> > > > --- a/fs/btrfs/send.c
> > > > +++ b/fs/btrfs/send.c
> > > > @@ -850,6 +850,7 @@ struct btrfs_inode_info {
> > > > u64 gid;
> > > > u64 rdev;
> > > > u64 attr;
> > > > + u64 nlink;
> > > > };
> > > >
> > > > /*
> > > > @@ -888,6 +889,7 @@ static int get_inode_info(struct btrfs_root *root, u64 ino,
> > > > info->uid = btrfs_inode_uid(path->nodes[0], ii);
> > > > info->gid = btrfs_inode_gid(path->nodes[0], ii);
> > > > info->rdev = btrfs_inode_rdev(path->nodes[0], ii);
> > > > + info->nlink = btrfs_inode_nlink(path->nodes[0], ii);
> > > > /*
> > > > * Transfer the unchanged u64 value of btrfs_inode_item::flags, that's
> > > > * otherwise logically split to 32/32 parts.
> > > > @@ -1652,19 +1654,22 @@ static int get_cur_inode_state(struct send_ctx *sctx, u64 ino, u64 gen)
> > > > int right_ret;
> > > > u64 left_gen;
> > > > u64 right_gen;
> > > > + struct btrfs_inode_info info;
> > > >
> > > > - ret = get_inode_gen(sctx->send_root, ino, &left_gen);
> > > > + ret = get_inode_info(sctx->send_root, ino, &info);
> > > > if (ret < 0 && ret != -ENOENT)
> > > > goto out;
> > > > - left_ret = ret;
> > > > + left_ret = (info.nlink == 0) ? -ENOENT : ret;
> > > > + left_gen = info.gen;
> > > >
> > > > if (!sctx->parent_root) {
> > > > right_ret = -ENOENT;
> > > > } else {
> > > > - ret = get_inode_gen(sctx->parent_root, ino, &right_gen);
> > > > + ret = get_inode_info(sctx->parent_root, ino, &info);
> > > > if (ret < 0 && ret != -ENOENT)
> > > > goto out;
> > > > - right_ret = ret;
> > > > + right_ret = (info.nlink == 0) ? -ENOENT : ret;
> > > > + right_gen = info.gen;
> > > > }
> > > >
> > > > if (!left_ret && !right_ret) {
> > > > @@ -6413,86 +6418,6 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end)
> > > > return ret;
> > > > }
> > > >
> > > > -struct parent_paths_ctx {
> > > > - struct list_head *refs;
> > > > - struct send_ctx *sctx;
> > > > -};
> > > > -
> > > > -static int record_parent_ref(int num, u64 dir, int index, struct fs_path *name,
> > > > - void *ctx)
> > > > -{
> > > > - struct parent_paths_ctx *ppctx = ctx;
> > > > -
> > > > - /*
> > > > - * Pass 0 as the generation for the directory, we don't care about it
> > > > - * here as we have no new references to add, we just want to delete all
> > > > - * references for an inode.
> > > > - */
> > > > - return record_ref_in_tree(&ppctx->sctx->rbtree_deleted_refs, ppctx->refs,
> > > > - name, dir, 0, ppctx->sctx);
> > > > -}
> > > > -
> > > > -/*
> > > > - * Issue unlink operations for all paths of the current inode found in the
> > > > - * parent snapshot.
> > > > - */
> > > > -static int btrfs_unlink_all_paths(struct send_ctx *sctx)
> > > > -{
> > > > - LIST_HEAD(deleted_refs);
> > > > - struct btrfs_path *path;
> > > > - struct btrfs_root *root = sctx->parent_root;
> > > > - struct btrfs_key key;
> > > > - struct btrfs_key found_key;
> > > > - struct parent_paths_ctx ctx;
> > > > - int iter_ret = 0;
> > > > - int ret;
> > > > -
> > > > - path = alloc_path_for_send();
> > > > - if (!path)
> > > > - return -ENOMEM;
> > > > -
> > > > - key.objectid = sctx->cur_ino;
> > > > - key.type = BTRFS_INODE_REF_KEY;
> > > > - key.offset = 0;
> > > > -
> > > > - ctx.refs = &deleted_refs;
> > > > - ctx.sctx = sctx;
> > > > -
> > > > - btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
> > > > - if (found_key.objectid != key.objectid)
> > > > - break;
> > > > - if (found_key.type != key.type &&
> > > > - found_key.type != BTRFS_INODE_EXTREF_KEY)
> > > > - break;
> > > > -
> > > > - ret = iterate_inode_ref(root, path, &found_key, 1,
> > > > - record_parent_ref, &ctx);
> > > > - if (ret < 0)
> > > > - goto out;
> > > > - }
> > > > - /* Catch error found during iteration */
> > > > - if (iter_ret < 0) {
> > > > - ret = iter_ret;
> > > > - goto out;
> > > > - }
> > > > -
> > > > - while (!list_empty(&deleted_refs)) {
> > > > - struct recorded_ref *ref;
> > > > -
> > > > - ref = list_first_entry(&deleted_refs, struct recorded_ref, list);
> > > > - ret = send_unlink(sctx, ref->full_path);
> > > > - if (ret < 0)
> > > > - goto out;
> > > > - recorded_ref_free(ref);
> > > > - }
> > > > - ret = 0;
> > > > -out:
> > > > - btrfs_free_path(path);
> > > > - if (ret)
> > > > - __free_recorded_refs(&deleted_refs);
> > > > - return ret;
> > > > -}
> > > > -
> > > > static void close_current_inode(struct send_ctx *sctx)
> > > > {
> > > > u64 i_size;
> > > > @@ -6583,25 +6508,37 @@ static int changed_inode(struct send_ctx *sctx,
> > > > * file descriptor against it or turning a RO snapshot into RW mode,
> > > > * keep an open file descriptor against a file, delete it and then
> > > > * turn the snapshot back to RO mode before using it for a send
> > > > - * operation. So if we find such cases, ignore the inode and all its
> > > > - * items completely if it's a new inode, or if it's a changed inode
> > > > - * make sure all its previous paths (from the parent snapshot) are all
> > > > - * unlinked and all other the inode items are ignored.
> > > > + * operation. The former is what the receiver operation does.
> > > > + * Therefore, if we want to send these snapshots soon after they're
> > > > + * received, we need to handle orphan inodes as well. Moreover,
> > > > + * orphans can appear not only in the send snapshot but also in the
> > > > + * parent snapshot. Here are several cases:
> > > > + *
> > > > + * Case 1: BTRFS_COMPARE_TREE_NEW
> > > > + * | send snapshot | action
> > > > + * --------------------------------
> > > > + * nlink | 0 | ignore
> > > > + *
> > > > + * Case 2: BTRFS_COMPARE_TREE_DELETED
> > > > + * | parent snapshot | action
> > > > + * ----------------------------------
> > > > + * nlink | 0 | as usual
> > > > + * Note: No unlinks will be sent because there're no paths for it.
> > > > + *
> > > > + * Case 3: BTRFS_COMPARE_TREE_CHANGED
> > > > + * | | parent snapshot | send snapshot | action
> > > > + * -----------------------------------------------------------------------
> > > > + * subcase 1 | nlink | 0 | 0 | ignore
> > > > + * subcase 2 | nlink | >0 | 0 | new_gen(deletion)
> > > > + * subcase 3 | nlink | 0 | >0 | new_gen(creation)
> > > > + *
> > > > */
> > > > - if (result == BTRFS_COMPARE_TREE_NEW ||
> > > > - result == BTRFS_COMPARE_TREE_CHANGED) {
> > > > - u32 nlinks;
> > > > -
> > > > - nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii);
> > > > - if (nlinks == 0) {
> > > > + if (result == BTRFS_COMPARE_TREE_NEW) {
> > > > + if (btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii) ==
> > > > + 0) {
> > > > sctx->ignore_cur_inode = true;
> > > > - if (result == BTRFS_COMPARE_TREE_CHANGED)
> > > > - ret = btrfs_unlink_all_paths(sctx);
> > > > goto out;
> > > > }
> > > > - }
> > > > -
> > > > - if (result == BTRFS_COMPARE_TREE_NEW) {
> > > > sctx->cur_inode_gen = left_gen;
> > > > sctx->cur_inode_new = true;
> > > > sctx->cur_inode_deleted = false;
> > > > @@ -6622,6 +6559,18 @@ static int changed_inode(struct send_ctx *sctx,
> > > > sctx->cur_inode_mode = btrfs_inode_mode(
> > > > sctx->right_path->nodes[0], right_ii);
> > > > } else if (result == BTRFS_COMPARE_TREE_CHANGED) {
> > > > + u32 new_nlinks, old_nlinks;
> > > > +
> > > > + new_nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0],
> > > > + left_ii);
> > > > + old_nlinks = btrfs_inode_nlink(sctx->right_path->nodes[0],
> > > > + right_ii);
> > > > + if (new_nlinks == 0 && old_nlinks == 0) {
> > > > + sctx->ignore_cur_inode = true;
> > > > + goto out;
> > > > + } else if (new_nlinks == 0 || old_nlinks == 0) {
> > > > + sctx->cur_inode_new_gen = 1;
> > > > + }
> > > > /*
> > > > * We need to do some special handling in case the inode was
> > > > * reported as changed with a changed generation number. This
> > > > @@ -6648,38 +6597,45 @@ static int changed_inode(struct send_ctx *sctx,
> > > > /*
> > > > * Now process the inode as if it was new.
> > > > */
> > > > - sctx->cur_inode_gen = left_gen;
> > > > - sctx->cur_inode_new = true;
> > > > - sctx->cur_inode_deleted = false;
> > > > - sctx->cur_inode_size = btrfs_inode_size(
> > > > - 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);
> > > > - ret = send_create_inode_if_needed(sctx);
> > > > - if (ret < 0)
> > > > - goto out;
> > > > + if (new_nlinks > 0) {
> > > > + sctx->cur_inode_gen = left_gen;
> > > > + sctx->cur_inode_new = true;
> > > > + sctx->cur_inode_deleted = false;
> > > > + sctx->cur_inode_size = btrfs_inode_size(
> > > > + 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);
> > > > + ret = send_create_inode_if_needed(sctx);
> > > > + if (ret < 0)
> > > > + goto out;
> > > >
> > > > - ret = process_all_refs(sctx, BTRFS_COMPARE_TREE_NEW);
> > > > - if (ret < 0)
> > > > - goto out;
> > > > - /*
> > > > - * Advance send_progress now as we did not get into
> > > > - * process_recorded_refs_if_needed in the new_gen case.
> > > > - */
> > > > - sctx->send_progress = sctx->cur_ino + 1;
> > > > + ret = process_all_refs(sctx,
> > > > + BTRFS_COMPARE_TREE_NEW);
> > > > + if (ret < 0)
> > > > + goto out;
> > > > + /*
> > > > + * Advance send_progress now as we did not get
> > > > + * into process_recorded_refs_if_needed in the
> > > > + * new_gen case.
> > > > + */
> > > > + sctx->send_progress = sctx->cur_ino + 1;
> > > >
> > > > - /*
> > > > - * Now process all extents and xattrs of the inode as if
> > > > - * they were all new.
> > > > - */
> > > > - ret = process_all_extents(sctx);
> > > > - if (ret < 0)
> > > > - goto out;
> > > > - ret = process_all_new_xattrs(sctx);
> > > > - if (ret < 0)
> > > > - goto out;
> > > > + /*
> > > > + * Now process all extents and xattrs of the
> > > > + * inode as if they were all new.
> > > > + */
> > > > + ret = process_all_extents(sctx);
> > > > + if (ret < 0)
> > > > + goto out;
> > > > + ret = process_all_new_xattrs(sctx);
> > > > + if (ret < 0)
> > > > + goto out;
> > > > + }
> > > > } else {
> > > > sctx->cur_inode_gen = left_gen;
> > > > sctx->cur_inode_new = false;
> > > > --
> > > > 2.37.1
> > > >

2022-09-23 16:10:14

by Filipe Manana

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] btrfs: send: fix failures when processing inodes with no links

On Fri, Sep 23, 2022 at 3:16 PM bingjing chang <[email protected]> wrote:
>
> Filipe Manana <[email protected]> 於 2022年9月22日週四下午6:08寫道：
> >
> > ,
> >
> > On Fri, Aug 12, 2022 at 3:36 PM bingjing chang <[email protected]> wrote:
> > >
> > > Filipe Manana <[email protected]> 於 2022年8月11日週四晚上8:00寫道：
> > > >
> > > > On Thu, Aug 11, 2022 at 11:09 AM bingjingc <[email protected]> wrote:
> > > > >
> > > > > From: BingJing Chang <[email protected]>
> > > > >
> > > > > There is a bug causing send failures when processing an orphan directory
> > > > > with no links. In commit 46b2f4590aab ("Btrfs: fix send failure when root
> > > > > has deleted files still open")', the orphan inode issue was addressed. The
> > > > > send operation fails with a ENOENT error because of any attempts to
> > > > > generate a path for the inode with a link count of zero. Therefore, in that
> > > > > patch, sctx->ignore_cur_inode was introduced to be set if the current inode
> > > > > has a link count of zero for bypassing some unnecessary steps. And a helper
> > > > > function btrfs_unlink_all_paths() was introduced and called to clean up old
> > > > > paths found in the parent snapshot. However, not only regular files but
> > > > > also directories can be orphan inodes. So if the send operation meets an
> > > > > orphan directory, it will issue a wrong unlink command for that directory
> > > > > now. Soon the receive operation fails with a EISDIR error. Besides, the
> > > > > send operation also fails with a ENOENT error later when it tries to
> > > > > generate a path of it.
> > > > >
> > > > > Similar example but making an orphan dir for an incremental send:
> > > > >
> > > > > $ btrfs subvolume create vol
> > > > > $ mkdir vol/dir
> > > > > $ touch vol/dir/foo
> > > > >
> > > > > $ btrfs subvolume snapshot -r vol snap1
> > > > > $ btrfs subvolume snapshot -r vol snap2
> > > > >
> > > > > # Turn the second snapshot to RW mode and delete the whole dir while
> > > > > # holding an open file descriptor on it.
> > > > > $ btrfs property set snap2 ro false
> > > > > $ exec 73<snap2/dir
> > > > > $ rm -rf snap2/dir
> > > > >
> > > > > # Set the second snapshot back to RO mode and do an incremental send.
> > > > > $ btrfs property set snap2 ro true
> > > > > $ mkdir receive_dir
> > > > > $ btrfs send snap2 -p snap1 | btrfs receive receive_dir/
> > > > > At subvol snap2
> > > > > At snapshot snap2
> > > > > ERROR: send ioctl failed with -2: No such file or directory
> > > > > ERROR: unlink dir failed. Is a directory
> > > > >
> > > > > Actually, orphan inodes are more common use cases in cascading backups.
> > > > > (Please see the illustration below.) In a cascading backup, a user wants
> > > > > to replicate a couple of snapshots from Machine A to Machine B and from
> > > > > Machine B to Machine C. Machine B doesn't take any RO snapshots for
> > > > > sending. All a receiver does is create an RW snapshot of its parent
> > > > > snapshot, apply the send stream and turn it into RO mode at the end. Even
> > > > > if all paths of some inodes are deleted in applying the send stream, these
> > > > > inodes would not be deleted and become orphans after changing the subvolume
> > > > > from RW to RO. Moreover, orphan inodes can occur not only in send snapshots
> > > > > but also in parent snapshots because Machine B may do a batch replication
> > > > > of a couple of snapshots.
> > > > >
> > > > > An illustration for cascading backups:
> > > > > Machine A (snapshot {1..n}) --> Machine B --> Machine C
> > > > >
> > > > > The intuition to solve the problem is to delete all the items of orphan
> > > > > inodes before using these snapshots for sending. I used to think that the
> > > > > reasonable timing for doing that is during the ioctl of changing the
> > > > > subvolume from RW to RO because it sounds good that we will not modify the
> > > > > fs tree of a RO snapshot anymore. However, attempting to do the orphan
> > > > > cleanup in the ioctl would be pointless. Because if someone is holding an
> > > > > open file descriptor on the inode, the reference count of the inode will
> > > > > never drop to 0. Then iput() cannot trigger eviction, which finally deletes
> > > > > all the items of it. So we try to extend the original patch to handle
> > > > > orphans in send/parent snapshots. Here are several cases that need to be
> > > > > considered:
> > > > >
> > > > > Case 1: BTRFS_COMPARE_TREE_NEW
> > > > > | send snapshot | action
> > > > > --------------------------------
> > > > > nlink | 0 | ignore
> > > > >
> > > > > In case 1, when we get a BTRFS_COMPARE_TREE_NEW tree comparison result,
> > > > > it means that a new inode is found in the send snapshot and it doesn't
> > > > > appear in the parent snapshot. Since this inode has a link count of zero
> > > > > (It's an orphan and there're no paths for it.), we can leverage
> > > > > sctx->ignore_cur_inode in the original patch to prevent it from being
> > > > > created.
> > > > >
> > > > > Case 2: BTRFS_COMPARE_TREE_DELETED
> > > > > | parent snapshot | action
> > > > > ----------------------------------
> > > > > nlink | 0 | as usual
> > > > >
> > > > > In case 2, when we get a BTRFS_COMPARE_TREE_DELETED tree comparison
> > > > > result, it means that the inode only appears in the parent snapshot.
> > > > > As usual, the send operation will try to delete all its paths. However,
> > > > > this inode has a link count of zero, so no paths of it will be found. No
> > > > > deletion operations will be issued. We don't need to change any logic.
> > > > >
> > > > > Case 3: BTRFS_COMPARE_TREE_CHANGED
> > > > > | | parent snapshot | send snapshot | action
> > > > > -----------------------------------------------------------------------
> > > > > subcase 1 | nlink | 0 | 0 | ignore
> > > > > subcase 2 | nlink | >0 | 0 | new_gen(deletion)
> > > > > subcase 3 | nlink | 0 | >0 | new_gen(creation)
> > > > >
> > > > > In case 3, when we get a BTRFS_COMPARE_TREE_CHANGED tree comparison result,
> > > > > it means that the inode appears in both snapshots. Here're three subcases.
> > > > >
> > > > > First, if the inode has link counts of zero in both snapshots. Since there
> > > > > are no paths for this inode in (source/destination) parent snapshots and we
> > > > > don't care about whether there is also an orphan inode in destination or
> > > > > not, we can set sctx->ignore_cur_inode on to prevent it from being created.
> > > > >
> > > > > For the second and the third subcases, if there're paths in one snapshot
> > > > > and there're no paths in the other snapshot for this inode. We can treat
> > > > > this inode as a new generation. We can also leverage the logic handling a
> > > > > new generation of an inode with small adjustments. Then it will delete all
> > > > > old paths and create a new inode with new attributes and paths only when
> > > > > there's a positive link count in the send snapshot. In subcase 2, the
> > > > > send operation only needs to delete all old paths as in the parent
> > > > > snapshot. But it may require more operations for a directory to remove its
> > > > > old paths. If a not-empty directory is going to be deleted (because it has
> > > > > a link count of zero in the send snapshot) but there're files/directories
> > > > > with bigger inode numbers under it, the send operation will need to rename
> > > > > it to its orphan name first. After processing and deleting the last item
> > > > > under this directory, the send operation will check this directory, aka
> > > > > the parent directory of the last item, again and issue a rmdir operation
> > > > > to remove it finally. Therefore, we also need to treat inodes with a link
> > > > > count of zero as if they didn't exist in get_cur_inode_state(), which is
> > > > > used in process_recorded_refs(). By doing this, when reviewing a directory
> > > > > with orphan names after the last item under it has been deleted, the send
> > > > > operation now can properly issue a rmdir operation. Otherwise, without
> > > > > doing this, the orphan directory with an orphan name would be kept here
> > > > > at the end due to the existing inode with a link count of zero being found.
> > > > > In subcase 3, as in case 2, no old paths would be found, so no deletion
> > > > > operations will be issued. The send operation will only create a new one
> > > > > for that inode.
> > > > >
> > > > > Note that subcase 3 is not a common case. That's because it's easy to
> > > > > reduce the hard links of an inode, but once all valid paths are removed,
> > > > > there're no valid paths for creating other hard links. The only way to do
> > > > > that is trying to send an older snapshot after a newer snapshot has been
> > > > > sent.
> > > > >
> > > > > Cc: <[email protected]> # 4.9: 46b2f4590aab: Btrfs: fix send
> > > > > failure when root has deleted files still open
> > > > > Cc: <[email protected]> # 4.9: 71ecfc133b03: btrfs: send:
> > > > > introduce recorded_ref_alloc and recorded_ref_free
> > > > > Cc: <[email protected]> # 4.9: 3aa5bd367fa5: btrfs: send: fix
> > > > > sending link commands for existing file paths
> > > > > Cc: <[email protected]> # 4.9: 0d8869fb6b6f8: btrfs: send: always
> > > > > use the rbtree based inode ref management infrastructure
> > > >
> > > > Btw, lines with CC, Fixes, etc, tags should not be broken even if they
> > > > are wider than 74 characters.
> > > >
> > >
> > > Okay, thank you for telling me that.
> > >
> > > > So, in v1 when I gave you that example of CC stable tags, it wasn't
> > > > meant for you to literally copy-paste them.
> > > >
> > > > First I asked if the purpose of the original Fixes tag was to backport
> > > > the fix to stable releases.
> > > > Was that the intention? You didn't provide an answer about that.
> > > >
> > >
> > > Oh, I misunderstood your suggestion. I'm sorry about that.
> > > Our intention is to report this bug and try to provide a reasonable and
> > > acceptable fix for it. Backporting is not our goal.
> > >
> > > > Then I told if that was the case, the proper way would be adding CC
> > > > stable tags and listing any
> > > > dependencies. I gave those 4 as examples with commits that are fairly
> > > > recent and obvious dependencies,
> > > > but I also said that probably there's a lot more missing - especially
> > > > if we want to backport to as far as 4.9.
> > > >
> > >
> > > > Even with just those 4 dependencies, some of those commits are fairly
> > > > large, and that may be frowned upon
> > > > according to stable backport rules (listed at
> > > > https://www.kernel.org/doc/Documentation/process/stable-kernel-rules.rst).
> > > > For e.g., patches with over 100 lines changed.
> > > >
> > > > Now, did you actually verify if there were more dependencies? (and test)
> > > > And do you really want to go as far as 4.9 (currently the oldest
> > > > stable release)?
> > >
> > > No, I didn't. I used to think the CC tag was a very cool feature, which
> > > just putting a few commits lets backport easily when I read your mail,
> > > so I copied and pasted these 4 commits in the beginning of revising
> > > the patch v2. However, I'm wrong.
> > >
> > > > I seriously doubt that those 4 commits are the only dependencies in
> > > > order to be able to cleanly backport to 4.9 and other old branches.
> > > >
> > > > It may be better to backport only to a few younger stable branches, or
> > > > just provide later a version of the patch to
> > > > apply to each desired stable branch (once the fix is in Linus' tree
> > > > and in a -rc release).
> > > >
> > > > If you are not interested in backporting to stable or don't have the
> > > > time to verify the dependencies and test, then just remove all the
> > > > stable tags.
> > > > Just leave a fixes tag:
> > > >
> > > > Fixes: 31db9f7c23fbf7 ("Btrfs: introduce BTRFS_IOC_SEND for btrfs send/receive")
> > > >
> > >
> > > Since backporting is not our goal. I will just leave the fix tag here.
> > >
> > > > Also, please don't forget to send a test case for fstests, covering as
> > > > many cases as possible (not just the example
> > > > at the beginning of the changelog).
> > > >
> > >
> > > Okay, I will submit a test case covering all cases.
> > > Because I still need to spend time learning how to use the fssum utility
> > > for the last test case you reviewed, so I will submit the test case later.
> >
> > BingJing, any progress with the test case?
> > We would love to have that in fstests to help prevent regressions in the future.
> >
> > Thanks.
> >
>
> Hi Filipe,
>
> I used to wait the git pull of my patch to the master branch, then I can
> attach the commit uuid in tests as before. However, it seems that there're
> troubles for the maintainer to pull all of the bug fixes into the new
> kernel 6.0 at once.

Ah, yes, it will only be in 6.1, so it should land in Linus' tree in
about 2 weeks maybe (we are at 6.0-rc6, next week is likely the final
rc7).
If you want to wait for that, it's fine. Just wanted to check if you
needed any help and that it wasn't forgotten.

Thanks!

>
> Okay, I will try to create a test covering the cases this weekend.
> Sorry for the inconvenience made.
>
> Thanks.
>
> > >
> > > Thanks.
> > >
> > > > Thanks.
> > > >
> > > > > Reviewed-by: Robbie Ko <[email protected]>
> > > > > Signed-off-by: BingJing Chang <[email protected]>
> > > > > ---
> > > > > fs/btrfs/send.c | 214 +++++++++++++++++++-----------------------------
> > > > > 1 file changed, 85 insertions(+), 129 deletions(-)
> > > > >
> > > > > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> > > > > index f8d77a33b9b7..6ab1ba66ff4b 100644
> > > > > --- a/fs/btrfs/send.c
> > > > > +++ b/fs/btrfs/send.c
> > > > > @@ -850,6 +850,7 @@ struct btrfs_inode_info {
> > > > > u64 gid;
> > > > > u64 rdev;
> > > > > u64 attr;
> > > > > + u64 nlink;
> > > > > };
> > > > >
> > > > > /*
> > > > > @@ -888,6 +889,7 @@ static int get_inode_info(struct btrfs_root *root, u64 ino,
> > > > > info->uid = btrfs_inode_uid(path->nodes[0], ii);
> > > > > info->gid = btrfs_inode_gid(path->nodes[0], ii);
> > > > > info->rdev = btrfs_inode_rdev(path->nodes[0], ii);
> > > > > + info->nlink = btrfs_inode_nlink(path->nodes[0], ii);
> > > > > /*
> > > > > * Transfer the unchanged u64 value of btrfs_inode_item::flags, that's
> > > > > * otherwise logically split to 32/32 parts.
> > > > > @@ -1652,19 +1654,22 @@ static int get_cur_inode_state(struct send_ctx *sctx, u64 ino, u64 gen)
> > > > > int right_ret;
> > > > > u64 left_gen;
> > > > > u64 right_gen;
> > > > > + struct btrfs_inode_info info;
> > > > >
> > > > > - ret = get_inode_gen(sctx->send_root, ino, &left_gen);
> > > > > + ret = get_inode_info(sctx->send_root, ino, &info);
> > > > > if (ret < 0 && ret != -ENOENT)
> > > > > goto out;
> > > > > - left_ret = ret;
> > > > > + left_ret = (info.nlink == 0) ? -ENOENT : ret;
> > > > > + left_gen = info.gen;
> > > > >
> > > > > if (!sctx->parent_root) {
> > > > > right_ret = -ENOENT;
> > > > > } else {
> > > > > - ret = get_inode_gen(sctx->parent_root, ino, &right_gen);
> > > > > + ret = get_inode_info(sctx->parent_root, ino, &info);
> > > > > if (ret < 0 && ret != -ENOENT)
> > > > > goto out;
> > > > > - right_ret = ret;
> > > > > + right_ret = (info.nlink == 0) ? -ENOENT : ret;
> > > > > + right_gen = info.gen;
> > > > > }
> > > > >
> > > > > if (!left_ret && !right_ret) {
> > > > > @@ -6413,86 +6418,6 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end)
> > > > > return ret;
> > > > > }
> > > > >
> > > > > -struct parent_paths_ctx {
> > > > > - struct list_head *refs;
> > > > > - struct send_ctx *sctx;
> > > > > -};
> > > > > -
> > > > > -static int record_parent_ref(int num, u64 dir, int index, struct fs_path *name,
> > > > > - void *ctx)
> > > > > -{
> > > > > - struct parent_paths_ctx *ppctx = ctx;
> > > > > -
> > > > > - /*
> > > > > - * Pass 0 as the generation for the directory, we don't care about it
> > > > > - * here as we have no new references to add, we just want to delete all
> > > > > - * references for an inode.
> > > > > - */
> > > > > - return record_ref_in_tree(&ppctx->sctx->rbtree_deleted_refs, ppctx->refs,
> > > > > - name, dir, 0, ppctx->sctx);
> > > > > -}
> > > > > -
> > > > > -/*
> > > > > - * Issue unlink operations for all paths of the current inode found in the
> > > > > - * parent snapshot.
> > > > > - */
> > > > > -static int btrfs_unlink_all_paths(struct send_ctx *sctx)
> > > > > -{
> > > > > - LIST_HEAD(deleted_refs);
> > > > > - struct btrfs_path *path;
> > > > > - struct btrfs_root *root = sctx->parent_root;
> > > > > - struct btrfs_key key;
> > > > > - struct btrfs_key found_key;
> > > > > - struct parent_paths_ctx ctx;
> > > > > - int iter_ret = 0;
> > > > > - int ret;
> > > > > -
> > > > > - path = alloc_path_for_send();
> > > > > - if (!path)
> > > > > - return -ENOMEM;
> > > > > -
> > > > > - key.objectid = sctx->cur_ino;
> > > > > - key.type = BTRFS_INODE_REF_KEY;
> > > > > - key.offset = 0;
> > > > > -
> > > > > - ctx.refs = &deleted_refs;
> > > > > - ctx.sctx = sctx;
> > > > > -
> > > > > - btrfs_for_each_slot(root, &key, &found_key, path, iter_ret) {
> > > > > - if (found_key.objectid != key.objectid)
> > > > > - break;
> > > > > - if (found_key.type != key.type &&
> > > > > - found_key.type != BTRFS_INODE_EXTREF_KEY)
> > > > > - break;
> > > > > -
> > > > > - ret = iterate_inode_ref(root, path, &found_key, 1,
> > > > > - record_parent_ref, &ctx);
> > > > > - if (ret < 0)
> > > > > - goto out;
> > > > > - }
> > > > > - /* Catch error found during iteration */
> > > > > - if (iter_ret < 0) {
> > > > > - ret = iter_ret;
> > > > > - goto out;
> > > > > - }
> > > > > -
> > > > > - while (!list_empty(&deleted_refs)) {
> > > > > - struct recorded_ref *ref;
> > > > > -
> > > > > - ref = list_first_entry(&deleted_refs, struct recorded_ref, list);
> > > > > - ret = send_unlink(sctx, ref->full_path);
> > > > > - if (ret < 0)
> > > > > - goto out;
> > > > > - recorded_ref_free(ref);
> > > > > - }
> > > > > - ret = 0;
> > > > > -out:
> > > > > - btrfs_free_path(path);
> > > > > - if (ret)
> > > > > - __free_recorded_refs(&deleted_refs);
> > > > > - return ret;
> > > > > -}
> > > > > -
> > > > > static void close_current_inode(struct send_ctx *sctx)
> > > > > {
> > > > > u64 i_size;
> > > > > @@ -6583,25 +6508,37 @@ static int changed_inode(struct send_ctx *sctx,
> > > > > * file descriptor against it or turning a RO snapshot into RW mode,
> > > > > * keep an open file descriptor against a file, delete it and then
> > > > > * turn the snapshot back to RO mode before using it for a send
> > > > > - * operation. So if we find such cases, ignore the inode and all its
> > > > > - * items completely if it's a new inode, or if it's a changed inode
> > > > > - * make sure all its previous paths (from the parent snapshot) are all
> > > > > - * unlinked and all other the inode items are ignored.
> > > > > + * operation. The former is what the receiver operation does.
> > > > > + * Therefore, if we want to send these snapshots soon after they're
> > > > > + * received, we need to handle orphan inodes as well. Moreover,
> > > > > + * orphans can appear not only in the send snapshot but also in the
> > > > > + * parent snapshot. Here are several cases:
> > > > > + *
> > > > > + * Case 1: BTRFS_COMPARE_TREE_NEW
> > > > > + * | send snapshot | action
> > > > > + * --------------------------------
> > > > > + * nlink | 0 | ignore
> > > > > + *
> > > > > + * Case 2: BTRFS_COMPARE_TREE_DELETED
> > > > > + * | parent snapshot | action
> > > > > + * ----------------------------------
> > > > > + * nlink | 0 | as usual
> > > > > + * Note: No unlinks will be sent because there're no paths for it.
> > > > > + *
> > > > > + * Case 3: BTRFS_COMPARE_TREE_CHANGED
> > > > > + * | | parent snapshot | send snapshot | action
> > > > > + * -----------------------------------------------------------------------
> > > > > + * subcase 1 | nlink | 0 | 0 | ignore
> > > > > + * subcase 2 | nlink | >0 | 0 | new_gen(deletion)
> > > > > + * subcase 3 | nlink | 0 | >0 | new_gen(creation)
> > > > > + *
> > > > > */
> > > > > - if (result == BTRFS_COMPARE_TREE_NEW ||
> > > > > - result == BTRFS_COMPARE_TREE_CHANGED) {
> > > > > - u32 nlinks;
> > > > > -
> > > > > - nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii);
> > > > > - if (nlinks == 0) {
> > > > > + if (result == BTRFS_COMPARE_TREE_NEW) {
> > > > > + if (btrfs_inode_nlink(sctx->left_path->nodes[0], left_ii) ==
> > > > > + 0) {
> > > > > sctx->ignore_cur_inode = true;
> > > > > - if (result == BTRFS_COMPARE_TREE_CHANGED)
> > > > > - ret = btrfs_unlink_all_paths(sctx);
> > > > > goto out;
> > > > > }
> > > > > - }
> > > > > -
> > > > > - if (result == BTRFS_COMPARE_TREE_NEW) {
> > > > > sctx->cur_inode_gen = left_gen;
> > > > > sctx->cur_inode_new = true;
> > > > > sctx->cur_inode_deleted = false;
> > > > > @@ -6622,6 +6559,18 @@ static int changed_inode(struct send_ctx *sctx,
> > > > > sctx->cur_inode_mode = btrfs_inode_mode(
> > > > > sctx->right_path->nodes[0], right_ii);
> > > > > } else if (result == BTRFS_COMPARE_TREE_CHANGED) {
> > > > > + u32 new_nlinks, old_nlinks;
> > > > > +
> > > > > + new_nlinks = btrfs_inode_nlink(sctx->left_path->nodes[0],
> > > > > + left_ii);
> > > > > + old_nlinks = btrfs_inode_nlink(sctx->right_path->nodes[0],
> > > > > + right_ii);
> > > > > + if (new_nlinks == 0 && old_nlinks == 0) {
> > > > > + sctx->ignore_cur_inode = true;
> > > > > + goto out;
> > > > > + } else if (new_nlinks == 0 || old_nlinks == 0) {
> > > > > + sctx->cur_inode_new_gen = 1;
> > > > > + }
> > > > > /*
> > > > > * We need to do some special handling in case the inode was
> > > > > * reported as changed with a changed generation number. This
> > > > > @@ -6648,38 +6597,45 @@ static int changed_inode(struct send_ctx *sctx,
> > > > > /*
> > > > > * Now process the inode as if it was new.
> > > > > */
> > > > > - sctx->cur_inode_gen = left_gen;
> > > > > - sctx->cur_inode_new = true;
> > > > > - sctx->cur_inode_deleted = false;
> > > > > - sctx->cur_inode_size = btrfs_inode_size(
> > > > > - 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);
> > > > > - ret = send_create_inode_if_needed(sctx);
> > > > > - if (ret < 0)
> > > > > - goto out;
> > > > > + if (new_nlinks > 0) {
> > > > > + sctx->cur_inode_gen = left_gen;
> > > > > + sctx->cur_inode_new = true;
> > > > > + sctx->cur_inode_deleted = false;
> > > > > + sctx->cur_inode_size = btrfs_inode_size(
> > > > > + 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);
> > > > > + ret = send_create_inode_if_needed(sctx);
> > > > > + if (ret < 0)
> > > > > + goto out;
> > > > >
> > > > > - ret = process_all_refs(sctx, BTRFS_COMPARE_TREE_NEW);
> > > > > - if (ret < 0)
> > > > > - goto out;
> > > > > - /*
> > > > > - * Advance send_progress now as we did not get into
> > > > > - * process_recorded_refs_if_needed in the new_gen case.
> > > > > - */
> > > > > - sctx->send_progress = sctx->cur_ino + 1;
> > > > > + ret = process_all_refs(sctx,
> > > > > + BTRFS_COMPARE_TREE_NEW);
> > > > > + if (ret < 0)
> > > > > + goto out;
> > > > > + /*
> > > > > + * Advance send_progress now as we did not get
> > > > > + * into process_recorded_refs_if_needed in the
> > > > > + * new_gen case.
> > > > > + */
> > > > > + sctx->send_progress = sctx->cur_ino + 1;
> > > > >
> > > > > - /*
> > > > > - * Now process all extents and xattrs of the inode as if
> > > > > - * they were all new.
> > > > > - */
> > > > > - ret = process_all_extents(sctx);
> > > > > - if (ret < 0)
> > > > > - goto out;
> > > > > - ret = process_all_new_xattrs(sctx);
> > > > > - if (ret < 0)
> > > > > - goto out;
> > > > > + /*
> > > > > + * Now process all extents and xattrs of the
> > > > > + * inode as if they were all new.
> > > > > + */
> > > > > + ret = process_all_extents(sctx);
> > > > > + if (ret < 0)
> > > > > + goto out;
> > > > > + ret = process_all_new_xattrs(sctx);
> > > > > + if (ret < 0)
> > > > > + goto out;
> > > > > + }
> > > > > } else {
> > > > > sctx->cur_inode_gen = left_gen;
> > > > > sctx->cur_inode_new = false;
> > > > > --
> > > > > 2.37.1
> > > > >