[PATCH 0/2] btrfs: send: fix a bug that sending unlink commands for directories

[PATCH 0/2] btrfs: send: fix a bug that sending unlink commands for directories

[bingjingc]

From: BingJing Chang <bingjingc@synology.com>

There is a bug sending unlink commands for directories. In
commit 46b2f4590aab ("Btrfs: fix send failure when root has deleted files
still open")', orphan inode issue was addressed. There're no reference
paths for these orphan inodes, so the send operation fails with an ENOENT
error. 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 reference paths found in the parent
snapshot. However, not only regular files but also directories can be
orphan inodes. So if it meets an orphan directory, a wrong unlink command
for this directory will be issued. Soon the unlink command fails with an
EISDIR error.


BingJing Chang (2):
  btrfs: send: refactor get_inode_info()
  btrfs: send: fix a bug that sending unlink commands for directories

 fs/btrfs/send.c | 390 +++++++++++++++++++++++-------------------------
 1 file changed, 189 insertions(+), 201 deletions(-)

-- 
2.37.1

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

[bingjingc]

From: BingJing Chang <bingjingc@synology.com>

Refactor get_inode_info() with request flags and output structure supports.
Besides, also introduce a helper function called get_inode_gen(), which
is majorly used.

Reviewed-by: Robbie Ko <robbieko@synology.com>
Signed-off-by: BingJing Chang <bingjingc@synology.com>
---
 fs/btrfs/send.c | 176 ++++++++++++++++++++++++++++--------------------
 1 file changed, 102 insertions(+), 74 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index e7671afcee4f..84b09d428ca3 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -842,17 +842,41 @@ static int send_rmdir(struct send_ctx *sctx, struct fs_path *path)
 	return ret;
 }
 
+enum inode_field {
+	INODE_SIZE = 0x1,
+	INODE_GEN = 0x1 << 1,
+	INODE_MODE = 0x1 << 2,
+	INODE_UID = 0x1 << 3,
+	INODE_GID = 0x1 << 4,
+	INODE_RDEV = 0x1 << 5,
+	INODE_ATTR = 0x1 << 6,
+};
+
+struct 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, unsigned int flags,
+			  struct 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 +884,49 @@ 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);
+	if (flags & INODE_SIZE)
+		info->size = btrfs_inode_size(path->nodes[0], ii);
+	if (flags & INODE_GEN)
+		info->gen = btrfs_inode_generation(path->nodes[0], ii);
+	if (flags & INODE_MODE)
+		info->mode = btrfs_inode_mode(path->nodes[0], ii);
+	if (flags & INODE_UID)
+		info->uid = btrfs_inode_uid(path->nodes[0], ii);
+	if (flags & INODE_GID)
+		info->gid = btrfs_inode_gid(path->nodes[0], ii);
+	if (flags & INODE_RDEV)
+		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);
+	if (flags & INODE_ATTR)
+		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 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, INODE_GEN, &info);
+	if (!ret)
+		*gen = info.gen;
 	return ret;
 }
 
@@ -1644,8 +1670,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 +1678,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 +1840,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 +1897,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 inode_info info;
 
 	if (!sctx->parent_root)
 		goto out;
@@ -1888,8 +1912,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 +1939,15 @@ 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,
+				     INODE_GEN|INODE_MODE, &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 +1980,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 +2002,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 +2668,7 @@ static int send_create_inode(struct send_ctx *sctx, u64 ino)
 	int ret = 0;
 	struct fs_path *p;
 	int cmd;
+	struct inode_info info;
 	u64 gen;
 	u64 mode;
 	u64 rdev;
@@ -2656,10 +2680,13 @@ 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,
+				     INODE_GEN|INODE_MODE|INODE_RDEV, &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 +3386,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, 0, NULL);
 		if (ret == -ENOENT) {
 			ret = 0;
 			continue;
@@ -3534,12 +3560,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 +3693,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 +3783,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 +4463,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 +4493,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 +5076,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 +5555,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 inode_info info;
 	u64 clone_src_i_size = 0;
 
 	/*
@@ -5565,12 +5585,12 @@ 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, INODE_SIZE,
+			     &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 +6279,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 inode_info info;
 	u64 left_mode;
 	u64 left_uid;
 	u64 left_gid;
@@ -6301,11 +6322,15 @@ 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,
+			     INODE_MODE|INODE_UID|INODE_GID|INODE_ATTR,
+			     &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;
@@ -6317,10 +6342,15 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end)
 		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);
+				     INODE_SIZE|INODE_MODE|INODE_UID|
+				     INODE_GID|INODE_ATTR, &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 +6820,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

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

[Filipe Manana]

On Mon, Aug 8, 2022 at 11:27 AM bingjingc <bingjingc@synology.com> wrote:
>
> From: BingJing Chang <bingjingc@synology.com>
>
> Refactor get_inode_info() with request flags and output structure supports.
> Besides, also introduce a helper function called get_inode_gen(), which
> is majorly used.
>
> Reviewed-by: Robbie Ko <robbieko@synology.com>
> Signed-off-by: BingJing Chang <bingjingc@synology.com>
> ---
>  fs/btrfs/send.c | 176 ++++++++++++++++++++++++++++--------------------
>  1 file changed, 102 insertions(+), 74 deletions(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index e7671afcee4f..84b09d428ca3 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -842,17 +842,41 @@ static int send_rmdir(struct send_ctx *sctx, struct fs_path *path)
>         return ret;
>  }
>
> +enum inode_field {
> +       INODE_SIZE = 0x1,
> +       INODE_GEN = 0x1 << 1,
> +       INODE_MODE = 0x1 << 2,
> +       INODE_UID = 0x1 << 3,
> +       INODE_GID = 0x1 << 4,
> +       INODE_RDEV = 0x1 << 5,
> +       INODE_ATTR = 0x1 << 6,
> +};

0x1 is a bit odd, we normally define flags like: "1U <<  ..."

> +
> +struct inode_info {
> +       u64 size;
> +       u64 gen;
> +       u64 mode;
> +       u64 uid;
> +       u64 gid;
> +       u64 rdev;
> +       u64 attr;
> +};

The name of the structure and enums should ideally have a "btrfs_" and
"BTRFS_" prefix, as those names
are a bit too generic, prone for future conflicts.

> +
>  /*
>   * 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, unsigned int flags,
> +                         struct 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 +884,49 @@ 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);
> +       if (flags & INODE_SIZE)
> +               info->size = btrfs_inode_size(path->nodes[0], ii);
> +       if (flags & INODE_GEN)
> +               info->gen = btrfs_inode_generation(path->nodes[0], ii);
> +       if (flags & INODE_MODE)
> +               info->mode = btrfs_inode_mode(path->nodes[0], ii);
> +       if (flags & INODE_UID)
> +               info->uid = btrfs_inode_uid(path->nodes[0], ii);
> +       if (flags & INODE_GID)
> +               info->gid = btrfs_inode_gid(path->nodes[0], ii);
> +       if (flags & INODE_RDEV)
> +               info->rdev = btrfs_inode_rdev(path->nodes[0], ii);

To make things more simple, we can get away with the flags.

Just populate all the fields of the structure unconditionally.
Less code, it also makes the call sites more readable by avoiding long
bitwise oring expressions,
and it's really cheap to get all the fields anyway.

>         /*
>          * 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);
> +       if (flags & INODE_ATTR)
> +               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 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, INODE_GEN, &info);
> +       if (!ret)
> +               *gen = info.gen;
>         return ret;
>  }
>
> @@ -1644,8 +1670,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 +1678,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 +1840,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 +1897,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 inode_info info;
>
>         if (!sctx->parent_root)
>                 goto out;
> @@ -1888,8 +1912,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 +1939,15 @@ 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,
> +                                    INODE_GEN|INODE_MODE, &info);

In the future, for bitwise OR, please add a space before and after the
operator, it makes things
easier to the eyes (INODE_GEN | INODE_MODE vs INODE_GEN|INODE_MODE).

Otherwise, it looks good. get_inode_info() definitely has too many
optional arguments.
Thanks.

>                 if (ret < 0)
>                         goto out;
>
>                 ret = 1;
>                 *who_ino = other_inode;
> +               *who_gen = info.gen;
> +               *who_mode = info.mode;
>         } else {
>                 ret = 0;
>         }
> @@ -1955,8 +1980,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 +2002,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 +2668,7 @@ static int send_create_inode(struct send_ctx *sctx, u64 ino)
>         int ret = 0;
>         struct fs_path *p;
>         int cmd;
> +       struct inode_info info;
>         u64 gen;
>         u64 mode;
>         u64 rdev;
> @@ -2656,10 +2680,13 @@ 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,
> +                                    INODE_GEN|INODE_MODE|INODE_RDEV, &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 +3386,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, 0, NULL);
>                 if (ret == -ENOENT) {
>                         ret = 0;
>                         continue;
> @@ -3534,12 +3560,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 +3693,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 +3783,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 +4463,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 +4493,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 +5076,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 +5555,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 inode_info info;
>         u64 clone_src_i_size = 0;
>
>         /*
> @@ -5565,12 +5585,12 @@ 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, INODE_SIZE,
> +                            &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 +6279,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 inode_info info;
>         u64 left_mode;
>         u64 left_uid;
>         u64 left_gid;
> @@ -6301,11 +6322,15 @@ 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,
> +                            INODE_MODE|INODE_UID|INODE_GID|INODE_ATTR,
> +                            &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;
> @@ -6317,10 +6342,15 @@ static int finish_inode_if_needed(struct send_ctx *sctx, int at_end)
>                 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);
> +                                    INODE_SIZE|INODE_MODE|INODE_UID|
> +                                    INODE_GID|INODE_ATTR, &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 +6820,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
>

[PATCH 2/2] btrfs: send: fix a bug that sending unlink commands for directories

[bingjingc]

From: BingJing Chang <bingjingc@synology.com>

There is a bug sending unlink commands for directories. In
commit 46b2f4590aab ("Btrfs: fix send failure when root has deleted files
still open")', orphan inode issue was addressed. There're no reference
paths for these orphan inodes, so the send operation fails with an ENOENT
error. 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 reference paths found in the parent
snapshot. However, not only regular files but also directories can be
orphan inodes. So if it meets an orphan directory, a wrong unlink command
for this directory will be issued. Soon the unlink command fails with an
EISDIR error.

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
  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 clone an RW subvolume from its parent
snapshot, replay the changes and turn it into RO mode at the end. After
all reference paths of some inodes are deleted in applying changes,
there's no guarantee that orphan inodes will be cleaned up after subvolumes
are turned into RO mode. 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 do orphan cleanups before using
these snapshots for sending. The more reasonable timing is doing orphan
cleanups during the ioctl of turning an RW subvolume into an RO snapshot.
And it's also because an RO snapshot is regarded that the fs tree has
already frozen and will never be adjusted anymore. However, that would make
the work of property changes more complicated than expected. And the
btrfs_orphan_cleanup() is also not promised to remove all orphans
successfully. 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 reference paths.), 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 reference paths.
However, this inode has a link count of zero, so no reference paths will
be found. No deletion operations will be issued. We don't need to change
any logics.

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 reference 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 reference paths in one
snapshot and there're no reference paths in the other snapshot. 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 reference paths and create a new inode with new attributes
and paths only when there's a positive link count in the send snapshot.
Unlike regular files, which just require unlink operations for deleting
their old reference paths, a directory may require more operations to
remove its old reference paths. For a non-empty directory, the send
operation will rename it first and finally issue a rmdir operation to
remove it after the last item under this directory has been deleted.
Therefore, we also need to modify the existence definition of inodes to
exclude orphan inodes in get_cur_inode_state(), which is used in
process_recorded_refs(). Then it can properly issue the rmdir operation
after the last item in the directory has been removed.

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 a older snapshot after a newer snapshot has been
sent.

Fixes: 46b2f4590aab ("Btrfs: fix send failure when root has deleted files still open")
Reviewed-by: Robbie Ko <robbieko@synology.com>
Signed-off-by: BingJing Chang <bingjingc@synology.com>
---
 fs/btrfs/send.c | 218 ++++++++++++++++++++----------------------------
 1 file changed, 89 insertions(+), 129 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 84b09d428ca3..351fb7cdec4c 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -850,6 +850,7 @@ enum inode_field {
 	INODE_GID = 0x1 << 4,
 	INODE_RDEV = 0x1 << 5,
 	INODE_ATTR = 0x1 << 6,
+	INODE_NLINK = 0x1 << 7,
 };
 
 struct inode_info {
@@ -860,6 +861,7 @@ struct inode_info {
 	u64 gid;
 	u64 rdev;
 	u64 attr;
+	u64 nlink;
 };
 
 /*
@@ -904,6 +906,8 @@ static int get_inode_info(struct btrfs_root *root, u64 ino, unsigned int flags,
 		info->gid = btrfs_inode_gid(path->nodes[0], ii);
 	if (flags & INODE_RDEV)
 		info->rdev = btrfs_inode_rdev(path->nodes[0], ii);
+	if (flags & INODE_NLINK)
+		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.
@@ -1669,19 +1673,24 @@ static int get_cur_inode_state(struct send_ctx *sctx, u64 ino, u64 gen)
 	int right_ret;
 	u64 left_gen;
 	u64 right_gen;
+	struct inode_info info;
 
-	ret = get_inode_gen(sctx->send_root, ino, &left_gen);
+	ret = get_inode_info(sctx->send_root, ino, INODE_GEN|INODE_NLINK,
+			     &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,
+				     INODE_GEN|INODE_NLINK, &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) {
@@ -6437,86 +6446,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;
@@ -6607,25 +6536,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 bacause there're no reference paths.
+	 *
+	 * 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;
@@ -6646,6 +6587,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
@@ -6672,38 +6625,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

Re: [PATCH 2/2] btrfs: send: fix a bug that sending unlink commands for directories

[Filipe Manana]

On Mon, Aug 8, 2022 at 11:27 AM bingjingc <bingjingc@synology.com> wrote:
>
> From: BingJing Chang <bingjingc@synology.com>
>

Reading the subject is confusing, something sounds off:

"btrfs: send: fix a bug that sending unlink commands for directories"

May I suggest something more clear like:

"btrfs: send: fix failures when processing inodes with no links"

Since it does not only address the case of directories but also for
regular files on the parent snapshot,
as you mention further below in the changelog.

> There is a bug sending unlink commands for directories. In
> commit 46b2f4590aab ("Btrfs: fix send failure when root has deleted files
> still open")', orphan inode issue was addressed. There're no reference
> paths for these orphan inodes, so the send operation fails with an ENOENT
> error. 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 reference paths found in the parent
> snapshot. However, not only regular files but also directories can be
> orphan inodes. So if it meets an orphan directory, a wrong unlink command
> for this directory will be issued. Soon the unlink command fails with an
> EISDIR error.

So, there are two things happening:

1) Send fails with -ENOENT, because it tries to generate a path for a directory
that has no links. This is just like the case of a regular file before
commit 46b2f4590aab.

2) Before the send ioctl failed with -ENOENT, it generated an unlink
command for the
directory. This is the only part you mention.

>
> 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
>   ERROR: unlink dir failed. Is a directory

So running your reproducer, I get send failing with -ENOENT, but oddly in your
output that doesn't happen. Perhaps that's when running with some
custom and specific
synology codebase? Or you unintentionally omitted a part of the output?

This is what I get:

$ cat test.sh
#!/bin/bash

DEV=/dev/sdj
MNT=/mnt/sdj

umount $DEV &>/dev/null
mkfs.btrfs -f $DEV 2>/dev/null
mount $DEV $MNT

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

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

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

# Set the second snapshot back to RO mode and do an incremental send.
btrfs property set $MNT/snap2 ro true
mkdir $MNT/receive_dir
btrfs send $MNT/snap2 -p $MNT/snap1 | btrfs receive $MNT/receive_dir/

$ ./test.sh
(...)
Create subvolume '/mnt/sdj/vol'
Create a readonly snapshot of '/mnt/sdj/vol' in '/mnt/sdj/snap1'
Create a readonly snapshot of '/mnt/sdj/vol' in '/mnt/sdj/snap2'
At subvol /mnt/sdj/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 clone an RW subvolume from its parent

What do you mean by cloning a RW subvolume?
Create a (RW) snapshot of it? Please make it more clear in the next
patch version.

> snapshot, replay the changes and turn it into RO mode at the end. After
> all reference paths of some inodes are deleted in applying changes,

"reference paths" is confusing, just say paths only.

"in applying changes" -> "when applying the send stream"

> there's no guarantee that orphan inodes will be cleaned up after subvolumes
> are turned into RO mode.

Orphan cleanup is not supposed to happen when changing the subvolume
state from RW to RO (and vice versa).
Once an inode's link count drops to 0, it's marked as an orphan for
cleanup, but the cleanup only happens later,
more on this below.

> 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 do orphan cleanups before using
> these snapshots for sending. The more reasonable timing is doing orphan
> cleanups during the ioctl of turning an RW subvolume into an RO snapshot.

No, attempting to do orphan cleanup in the ioctl would be pointless.
More details below.

The cleanup, deleting the inode's items etc, can not happen while
someone is using a file descriptor for the inode,
which is precisely the case in the reproducer above.

It will happen only once the reference count on the inode drops to 0
(after the last file descriptor is closed).
The final iput() triggers the eviction, which will delete all the
inode items, etc, if the inode still has a link count of 0.

> And it's also because an RO snapshot is regarded that the fs tree has
> already frozen and will never be adjusted anymore.

We can do orphan cleanup on a RO subvolume/snapshot - that's not a
problem. We only skip orphan cleanup
when the fs is mounted in RO mode. It's just pointless for the type of
reproducer you provided, see below.

> However, that would make
> the work of property changes more complicated than expected. And the
> btrfs_orphan_cleanup() is also not promised to remove all orphans
> successfully.

In this case btrfs_orphan_cleanup() would never trigger the cleanup of
the inode because there's still
a process holding a file descriptor open, so the iput() done by
btrfs_orphan_cleanup() is not the final
iput - therefore it doesn't not trigger eviction, which is what
deletes all the items of the inode.

For that test, what will trigger the cleanup is the closing of the
file descriptor - that triggers the final iput,
which triggers eviction and that finally triggers the deletions of all
the items that belong to the inode, etc.

> 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 reference paths.), we can leverage

Please don't use the term "reference paths", it's confusing. Just say
it has no links,
or that it doesn't have any inode reference items.

> 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 reference paths.

"all its reference paths" -> "all its paths"

> However, this inode has a link count of zero, so no reference paths will

Same as before and same note for everywhere else in the changelog and code
comments.

> be found. No deletion operations will be issued. We don't need to change
> any logics.
>
> 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 reference 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 reference paths in one
> snapshot and there're no reference paths in the other snapshot. 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 reference paths and create a new inode with new attributes
> and paths only when there's a positive link count in the send snapshot.
> Unlike regular files, which just require unlink operations for deleting
> their old reference paths, a directory may require more operations to
> remove its old reference paths. For a non-empty directory, the send
> operation will rename it first and finally issue a rmdir operation to
> remove it after the last item under this directory has been deleted.

How can we have a non-empty directory with a link count of 0?
To be able to rmdir a directory (even if some process is holding an
open file descriptor on it), one
has to delete all its dentries first - there's no way around this,
it's enforced in generic code, outside btrfs.

So I don't understand your suggestion that we can have a snapshot with
a non-empty directory
that has a link count of 0.

> Therefore, we also need to modify the existence definition of inodes to

"existence definition of inodes" - this is super confusing. I can
understand it only
by looking at the code changes. You mean, to treat inodes with a link count of 0
as if they didn't exist.

Maybe just say "... we also need to consider inodes without links as deleted".

> exclude orphan inodes in get_cur_inode_state(), which is used in
> process_recorded_refs(). Then it can properly issue the rmdir operation
> after the last item in the directory has been removed.
>
> 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 a older snapshot after a newer snapshot has been
> sent.

>
> Fixes: 46b2f4590aab ("Btrfs: fix send failure when root has deleted files still open")

So the Fixes tag is meant to identify a commit that introduced a bug.

That commit only addressed one case of inodes with no links - when
they are regular files and in the send snapshot only.
The other cases you identified didn't work before that commit - they
have never worked since send was introduced in 2012 (commit
31db9f7c23fbf7e95026143f79645de6507b583b).

If your intention is to have this backported, and simply mark that
commit as a dependency, then use a proper CC tag as mentioned at:

https://www.kernel.org/doc/html/v4.10/process/stable-kernel-rules.html#option-3

Like this I suppose:

Cc: <stable@vger.kernel.org> # 4.9: 46b2f4590aab: Btrfs: fix send
failure when root has deleted files still open
Cc: <stable@vger.kernel.org> # 4.9: 71ecfc133b03: btrfs: send:
introduce recorded_ref_alloc and recorded_ref_free
Cc: <stable@vger.kernel.org> # 4.9: 3aa5bd367fa5: btrfs: send: fix
sending link commands for existing file paths
Cc: <stable@vger.kernel.org> # 4.9: 0d8869fb6b6f8: btrfs: send: always
use the rbtree based inode ref management infrastructure

Maybe there are other patches that are needed for a clean backport,
but those are the only ones I have in mind because they are recent.
Otherwise we would have to change the version of the patch submitted
for stable releases.

> Reviewed-by: Robbie Ko <robbieko@synology.com>
> Signed-off-by: BingJing Chang <bingjingc@synology.com>
> ---
>  fs/btrfs/send.c | 218 ++++++++++++++++++++----------------------------
>  1 file changed, 89 insertions(+), 129 deletions(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 84b09d428ca3..351fb7cdec4c 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -850,6 +850,7 @@ enum inode_field {
>         INODE_GID = 0x1 << 4,
>         INODE_RDEV = 0x1 << 5,
>         INODE_ATTR = 0x1 << 6,
> +       INODE_NLINK = 0x1 << 7,
>  };
>
>  struct inode_info {
> @@ -860,6 +861,7 @@ struct inode_info {
>         u64 gid;
>         u64 rdev;
>         u64 attr;
> +       u64 nlink;
>  };
>
>  /*
> @@ -904,6 +906,8 @@ static int get_inode_info(struct btrfs_root *root, u64 ino, unsigned int flags,
>                 info->gid = btrfs_inode_gid(path->nodes[0], ii);
>         if (flags & INODE_RDEV)
>                 info->rdev = btrfs_inode_rdev(path->nodes[0], ii);
> +       if (flags & INODE_NLINK)
> +               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.
> @@ -1669,19 +1673,24 @@ static int get_cur_inode_state(struct send_ctx *sctx, u64 ino, u64 gen)
>         int right_ret;
>         u64 left_gen;
>         u64 right_gen;
> +       struct inode_info info;
>
> -       ret = get_inode_gen(sctx->send_root, ino, &left_gen);
> +       ret = get_inode_info(sctx->send_root, ino, INODE_GEN|INODE_NLINK,
> +                            &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,
> +                                    INODE_GEN|INODE_NLINK, &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) {
> @@ -6437,86 +6446,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;
> @@ -6607,25 +6536,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 bacause there're no reference paths.

bacause -> because

The code looks good to me, it's a good solution.

Can you please send a test case for fstests covering all these scenarios?
I.e. directory with a link count of 0, like the reproducer in the
changelog, parent snapshot with an inode have 0 links as well, etc.

Thanks.

> +        *
> +        * 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;
> @@ -6646,6 +6587,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
> @@ -6672,38 +6625,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
>

Re: [PATCH 2/2] btrfs: send: fix a bug that sending unlink commands for directories

[bingjing chang]

Hi Filipe,

Thank you for your review and the detailed comments.
I will submit a patch v2 to fix unclear sentences and problem you mentioned.

Filipe Manana <fdmanana@kernel.org> 於 2022年8月9日 週二 凌晨12:23寫道：
>
> On Mon, Aug 8, 2022 at 11:27 AM bingjingc <bingjingc@synology.com> wrote:
> >
> > From: BingJing Chang <bingjingc@synology.com>
> >
>
> Reading the subject is confusing, something sounds off:
>
> "btrfs: send: fix a bug that sending unlink commands for directories"
>
> May I suggest something more clear like:
>
> "btrfs: send: fix failures when processing inodes with no links"
>
> Since it does not only address the case of directories but also for
> regular files on the parent snapshot,
> as you mention further below in the changelog.

Yes, I tried to handle all orphan cases I can come up with.
Your suggested subject is better and clear. Thank you for the subject.

>
> > There is a bug sending unlink commands for directories. In
> > commit 46b2f4590aab ("Btrfs: fix send failure when root has deleted files
> > still open")', orphan inode issue was addressed. There're no reference
> > paths for these orphan inodes, so the send operation fails with an ENOENT
> > error. 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 reference paths found in the parent
> > snapshot. However, not only regular files but also directories can be
> > orphan inodes. So if it meets an orphan directory, a wrong unlink command
> > for this directory will be issued. Soon the unlink command fails with an
> > EISDIR error.
>
> So, there are two things happening:
>
> 1) Send fails with -ENOENT, because it tries to generate a path for a directory
> that has no links. This is just like the case of a regular file before
> commit 46b2f4590aab.
>
> 2) Before the send ioctl failed with -ENOENT, it generated an unlink
> command for the
> directory. This is the only part you mention.
>

Yes, that's all what happened. I should have mentioned that the send
operation would also fail with -ENOENT. But I only addressed the first
problem the receive operation met. I will mention both in the patch v2.

> >
> > 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
> >   ERROR: unlink dir failed. Is a directory
>
> So running your reproducer, I get send failing with -ENOENT, but oddly in your
> output that doesn't happen. Perhaps that's when running with some
> custom and specific
> synology codebase? Or you unintentionally omitted a part of the output?
>
I tried to reproduce the problem in my Ubuntu machine and made a mistake.
Since I just wanted to address only the unlink issue in patch v1, I guess that
I piped the stderr to /dev/null for the send operation like:
$ btrfs send snap2 -p snap1 2>/dev/null | btrfs receive receive_dir/
But I made a mistake removing the pipe command from the example.

> This is what I get:
>
> $ cat test.sh
> #!/bin/bash
>
> DEV=/dev/sdj
> MNT=/mnt/sdj
>
> umount $DEV &>/dev/null
> mkfs.btrfs -f $DEV 2>/dev/null
> mount $DEV $MNT
>
> btrfs subvolume create $MNT/vol
> mkdir $MNT/vol/dir
> touch $MNT/vol/dir/foo
>
> btrfs subvolume snapshot -r $MNT/vol $MNT/snap1
> btrfs subvolume snapshot -r $MNT/vol $MNT/snap2
>
> # Turn the second snapshot to RW mode and delete the whole dir while
> # holding an open file descriptor on it.
> btrfs property set $MNT/snap2 ro false
> exec 73<$MNT/snap2/dir
> rm -rf $MNT/snap2/dir
>
> # Set the second snapshot back to RO mode and do an incremental send.
> btrfs property set $MNT/snap2 ro true
> mkdir $MNT/receive_dir
> btrfs send $MNT/snap2 -p $MNT/snap1 | btrfs receive $MNT/receive_dir/
>
> $ ./test.sh
> (...)
> Create subvolume '/mnt/sdj/vol'
> Create a readonly snapshot of '/mnt/sdj/vol' in '/mnt/sdj/snap1'
> Create a readonly snapshot of '/mnt/sdj/vol' in '/mnt/sdj/snap2'
> At subvol /mnt/sdj/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 clone an RW subvolume from its parent
>
> What do you mean by cloning a RW subvolume?
> Create a (RW) snapshot of it? Please make it more clear in the next
> patch version.
>
> > snapshot, replay the changes and turn it into RO mode at the end. After
> > all reference paths of some inodes are deleted in applying changes,
>
> "reference paths" is confusing, just say paths only.
>
> "in applying changes" -> "when applying the send stream"
>
> > there's no guarantee that orphan inodes will be cleaned up after subvolumes
> > are turned into RO mode.
>
> Orphan cleanup is not supposed to happen when changing the subvolume
> state from RW to RO (and vice versa).
> Once an inode's link count drops to 0, it's marked as an orphan for
> cleanup, but the cleanup only happens later,
> more on this below.
>
> > 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 do orphan cleanups before using
> > these snapshots for sending. The more reasonable timing is doing orphan
> > cleanups during the ioctl of turning an RW subvolume into an RO snapshot.
>
> No, attempting to do orphan cleanup in the ioctl would be pointless.
> More details below.
>
> The cleanup, deleting the inode's items etc, can not happen while
> someone is using a file descriptor for the inode,
> which is precisely the case in the reproducer above.
>
> It will happen only once the reference count on the inode drops to 0
> (after the last file descriptor is closed).
> The final iput() triggers the eviction, which will delete all the
> inode items, etc, if the inode still has a link count of 0.
>
> > And it's also because an RO snapshot is regarded that the fs tree has
> > already frozen and will never be adjusted anymore.
>
> We can do orphan cleanup on a RO subvolume/snapshot - that's not a
> problem. We only skip orphan cleanup
> when the fs is mounted in RO mode. It's just pointless for the type of
> reproducer you provided, see below.
>

Thank you for the detailed explanation. Saying doing orphan cleanup here
is not clear. The actual goal is to delete all the items of orphan inodes
before using these snapshots for sending. And the evictions of them
are not promised after doing an orphan cleanup because there can be
some file descriptors still hold on those inodes. So you said attempting to
do orphan cleanup in the ioctl would be pointless. I will revised this
paragraph.

> > However, that would make
> > the work of property changes more complicated than expected. And the
> > btrfs_orphan_cleanup() is also not promised to remove all orphans
> > successfully.
>
> In this case btrfs_orphan_cleanup() would never trigger the cleanup of
> the inode because there's still
> a process holding a file descriptor open, so the iput() done by
> btrfs_orphan_cleanup() is not the final
> iput - therefore it doesn't not trigger eviction, which is what
> deletes all the items of the inode.
>
> For that test, what will trigger the cleanup is the closing of the
> file descriptor - that triggers the final iput,
> which triggers eviction and that finally triggers the deletions of all
> the items that belong to the inode, etc.
>
> > 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 reference paths.), we can leverage
>
> Please don't use the term "reference paths", it's confusing. Just say
> it has no links,
> or that it doesn't have any inode reference items.

Indeed, I will revise all of them in patch v2.

>
> > 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 reference paths.
>
> "all its reference paths" -> "all its paths"
>
> > However, this inode has a link count of zero, so no reference paths will
>
> Same as before and same note for everywhere else in the changelog and code
> comments.
>
> > be found. No deletion operations will be issued. We don't need to change
> > any logics.
> >
> > 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 reference 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 reference paths in one
> > snapshot and there're no reference paths in the other snapshot. 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 reference paths and create a new inode with new attributes
> > and paths only when there's a positive link count in the send snapshot.
> > Unlike regular files, which just require unlink operations for deleting
> > their old reference paths, a directory may require more operations to
> > remove its old reference paths. For a non-empty directory, the send
> > operation will rename it first and finally issue a rmdir operation to
> > remove it after the last item under this directory has been deleted.
>
> How can we have a non-empty directory with a link count of 0?
> To be able to rmdir a directory (even if some process is holding an
> open file descriptor on it), one
> has to delete all its dentries first - there's no way around this,
> it's enforced in generic code, outside btrfs.
>
> So I don't understand your suggestion that we can have a snapshot with
> a non-empty directory
> that has a link count of 0.

Sorry, it's unclear here. I will revise the sentences here. In subcase 2,
we wants to delete all old paths in the parent snapshot for an inode
because it becomes an orphan (with a link count of 0) in the send
snapshot. But if the inode is a non-empty dir in the parent snapshot,
the send operation may need to issue more operations for deleting
all old paths against a non-empty dir as in the parent snapshot.

>
> > Therefore, we also need to modify the existence definition of inodes to
>
> "existence definition of inodes" - this is super confusing. I can
> understand it only
> by looking at the code changes. You mean, to treat inodes with a link count of 0
> as if they didn't exist.
>
> Maybe just say "... we also need to consider inodes without links as deleted".
>
> > exclude orphan inodes in get_cur_inode_state(), which is used in
> > process_recorded_refs(). Then it can properly issue the rmdir operation
> > after the last item in the directory has been removed.
> >
> > 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 a older snapshot after a newer snapshot has been
> > sent.
>
> >
> > Fixes: 46b2f4590aab ("Btrfs: fix send failure when root has deleted files still open")
>
> So the Fixes tag is meant to identify a commit that introduced a bug.
>
> That commit only addressed one case of inodes with no links - when
> they are regular files and in the send snapshot only.
> The other cases you identified didn't work before that commit - they
> have never worked since send was introduced in 2012 (commit
> 31db9f7c23fbf7e95026143f79645de6507b583b).
>
> If your intention is to have this backported, and simply mark that
> commit as a dependency, then use a proper CC tag as mentioned at:
>
> https://www.kernel.org/doc/html/v4.10/process/stable-kernel-rules.html#option-3
>
> Like this I suppose:
>
> Cc: <stable@vger.kernel.org> # 4.9: 46b2f4590aab: Btrfs: fix send
> failure when root has deleted files still open
> Cc: <stable@vger.kernel.org> # 4.9: 71ecfc133b03: btrfs: send:
> introduce recorded_ref_alloc and recorded_ref_free
> Cc: <stable@vger.kernel.org> # 4.9: 3aa5bd367fa5: btrfs: send: fix
> sending link commands for existing file paths
> Cc: <stable@vger.kernel.org> # 4.9: 0d8869fb6b6f8: btrfs: send: always
> use the rbtree based inode ref management infrastructure
>
> Maybe there are other patches that are needed for a clean backport,
> but those are the only ones I have in mind because they are recent.
> Otherwise we would have to change the version of the patch submitted
> for stable releases.
>
Thank you for the suggestions.

> > Reviewed-by: Robbie Ko <robbieko@synology.com>
> > Signed-off-by: BingJing Chang <bingjingc@synology.com>
> > ---
> >  fs/btrfs/send.c | 218 ++++++++++++++++++++----------------------------
> >  1 file changed, 89 insertions(+), 129 deletions(-)
> >
> > diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> > index 84b09d428ca3..351fb7cdec4c 100644
> > --- a/fs/btrfs/send.c
> > +++ b/fs/btrfs/send.c
> > @@ -850,6 +850,7 @@ enum inode_field {
> >         INODE_GID = 0x1 << 4,
> >         INODE_RDEV = 0x1 << 5,
> >         INODE_ATTR = 0x1 << 6,
> > +       INODE_NLINK = 0x1 << 7,
> >  };
> >
> >  struct inode_info {
> > @@ -860,6 +861,7 @@ struct inode_info {
> >         u64 gid;
> >         u64 rdev;
> >         u64 attr;
> > +       u64 nlink;
> >  };
> >
> >  /*
> > @@ -904,6 +906,8 @@ static int get_inode_info(struct btrfs_root *root, u64 ino, unsigned int flags,
> >                 info->gid = btrfs_inode_gid(path->nodes[0], ii);
> >         if (flags & INODE_RDEV)
> >                 info->rdev = btrfs_inode_rdev(path->nodes[0], ii);
> > +       if (flags & INODE_NLINK)
> > +               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.
> > @@ -1669,19 +1673,24 @@ static int get_cur_inode_state(struct send_ctx *sctx, u64 ino, u64 gen)
> >         int right_ret;
> >         u64 left_gen;
> >         u64 right_gen;
> > +       struct inode_info info;
> >
> > -       ret = get_inode_gen(sctx->send_root, ino, &left_gen);
> > +       ret = get_inode_info(sctx->send_root, ino, INODE_GEN|INODE_NLINK,
> > +                            &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,
> > +                                    INODE_GEN|INODE_NLINK, &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) {
> > @@ -6437,86 +6446,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;
> > @@ -6607,25 +6536,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 bacause there're no reference paths.
>
> bacause -> because
>
> The code looks good to me, it's a good solution.
>
> Can you please send a test case for fstests covering all these scenarios?
> I.e. directory with a link count of 0, like the reproducer in the
> changelog, parent snapshot with an inode have 0 links as well, etc.

Okay. I will also submit a test case covering all of them.

>
> Thanks.
>
> > +        *
> > +        * 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;
> > @@ -6646,6 +6587,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
> > @@ -6672,38 +6625,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
> >