diff mbox

[v3,4/6] Btrfs: incremental send, fix invalid path for rmdir operations

Message ID 1483604700-21017-5-git-send-email-robbieko@synology.com (mailing list archive)
State New, archived
Headers show

Commit Message

robbieko Jan. 5, 2017, 8:24 a.m. UTC
From: Robbie Ko <robbieko@synology.com>

Under certain situations, an incremental send operation can
a rmdir operation that will make the receiving end fail when
attempting to execute it, because the path is not exist.

Example scenario:
Parent snapshot:
|---- d259_old/         (ino 259, gen 96)
    |---- d1/           (ino 258, gen 96)
|---- f                 (ino 257, gen 96)

Send snapshot:
|---- d258/             (ino 258, gen 98)
|---- d259/             (ino 259, gen 98)
    |---- d1/           (ino 257, gen 98)

unlink f
mkdir o257-98-0
mkdir o259-98-0
chown o257-98-0 - uid=0, gid=0
chmod o257-98-0 - mode=0755
rmdir o258-96-0
ERROR: rmdir o258-96-0 failed: No such file or directory

While computing the send stream the following steps happen:

1) While processing inode 257 we create o257-98-0 and o259-98-0,
   then delay o257-98-0 rename operation because its new parent
   in the send snapshot, inode 259, was not yet processed and therefore
   not yet renamed;

2) Later we want to delete d1 (ino 258, gen 96) while processing inode
   258. In order to get its path for delete, we need to check if it is
   overwritten in the send snapshot. And we find it is overwritten so
   we delete it via unique name , which leads to error. The reason is
   we will find out d1 is under parent directory (inode 259) in the send
   snapshot, and for this case, because d1(inode 257, gen 98) is not same
   as d1 (inode 258, gen 96), we conclude d1 has been overwritten.

Fix this by adding generation check for the parent directory. Because
both parent directory are not identical, we can just skip the overwrite
check. In addition, inode 256 should not check for this since it is a
subvolume.

Signed-off-by: Robbie Ko <robbieko@synology.com>
---
V3: improve the change log
 fs/btrfs/send.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Filipe Manana Jan. 19, 2017, 12:06 p.m. UTC | #1
On Thu, Jan 5, 2017 at 8:24 AM, robbieko <robbieko@synology.com> wrote:
> From: Robbie Ko <robbieko@synology.com>
>
> Under certain situations, an incremental send operation can

Again, missing some word after the word "can".
Copy pasting change logs is not that good....

> a rmdir operation that will make the receiving end fail when
> attempting to execute it, because the path is not exist.
>
> Example scenario:
> Parent snapshot:
> |---- d259_old/         (ino 259, gen 96)
>     |---- d1/           (ino 258, gen 96)
> |---- f                 (ino 257, gen 96)
>
> Send snapshot:
> |---- d258/             (ino 258, gen 98)
> |---- d259/             (ino 259, gen 98)
>     |---- d1/           (ino 257, gen 98)

Please try to align things for easier readability.

>
> unlink f
> mkdir o257-98-0
> mkdir o259-98-0
> chown o257-98-0 - uid=0, gid=0
> chmod o257-98-0 - mode=0755
> rmdir o258-96-0
> ERROR: rmdir o258-96-0 failed: No such file or directory

Same comment as before (patch 1/6). Don't just paste the output of
receive -vv out of nowhere without mentioning what it is.

>
> While computing the send stream the following steps happen:
>
> 1) While processing inode 257 we create o257-98-0 and o259-98-0,
>    then delay o257-98-0 rename operation because its new parent
>    in the send snapshot, inode 259, was not yet processed and therefore
>    not yet renamed;
>
> 2) Later we want to delete d1 (ino 258, gen 96) while processing inode
>    258. In order to get its path for delete, we need to check if it is
>    overwritten in the send snapshot. And we find it is overwritten so
>    we delete it via unique name , which leads to error. The reason is
>    we will find out d1 is under parent directory (inode 259) in the send
>    snapshot, and for this case, because d1(inode 257, gen 98) is not same
>    as d1 (inode 258, gen 96), we conclude d1 has been overwritten.
>
> Fix this by adding generation check for the parent directory. Because
> both parent directory are not identical, we can just skip the overwrite
> check. In addition, inode 256 should not check for this since it is a
> subvolume.

I've heavily reworded the change log and included the patch in my 4.11
integration branch.

Thanks.

>
> Signed-off-by: Robbie Ko <robbieko@synology.com>
> ---
> V3: improve the change log
>  fs/btrfs/send.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index eaf1c92..139f492 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -1938,6 +1938,19 @@ static int did_overwrite_ref(struct send_ctx *sctx,
>         if (ret <= 0)
>                 goto out;
>
> +       if (dir != BTRFS_FIRST_FREE_OBJECTID) {
> +               ret = get_inode_info(sctx->send_root, dir, NULL, &gen, NULL,
> +                                    NULL, NULL, NULL);
> +               if (ret < 0 && ret != -ENOENT)
> +                       goto out;
> +               if (ret) {
> +                       ret = 0;
> +                       goto out;
> +               }
> +               if (gen != dir_gen)
> +                       goto out;
> +       }
> +
>         /* check if the ref was overwritten by another ref */
>         ret = lookup_dir_item_inode(sctx->send_root, dir, name, name_len,
>                         &ow_inode, &other_type);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index eaf1c92..139f492 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1938,6 +1938,19 @@  static int did_overwrite_ref(struct send_ctx *sctx,
 	if (ret <= 0)
 		goto out;
 
+	if (dir != BTRFS_FIRST_FREE_OBJECTID) {
+		ret = get_inode_info(sctx->send_root, dir, NULL, &gen, NULL,
+				     NULL, NULL, NULL);
+		if (ret < 0 && ret != -ENOENT)
+			goto out;
+		if (ret) {
+			ret = 0;
+			goto out;
+		}
+		if (gen != dir_gen)
+			goto out;
+	}
+
 	/* check if the ref was overwritten by another ref */
 	ret = lookup_dir_item_inode(sctx->send_root, dir, name, name_len,
 			&ow_inode, &other_type);