[PATCH v3 0/6] Btrfs: incremental send, fix serval case failure

[PATCH v3 0/6] Btrfs: incremental send, fix serval case failure

[robbieko]

From: Robbie Ko <robbieko@synology.com>

Patch for fix btrfs incremental send.
These patches base on v4.8.0-rc8

V3: Improve the change log
V2: Add a new patch "add generation check in existence demtermination for
    the parent directory"

Robbie Ko (6):
  Btrfs: incremental send, fix failure to rename with the name collision
  Btrfs: incremental send, fix invalid path for truncate operations
  Btrfs: incremental send, fix not necessary waiting for rmdir operation
  Btrfs: incremental send, fix invalid path for rmdir operations
  Btrfs: incremental send, fix invalid rename operations
  Btrfs: incremental send, fix invalid utime operations

 fs/btrfs/send.c | 61 ++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 47 insertions(+), 14 deletions(-)

-- 
1.9.1

[PATCH v3 1/6] Btrfs: incremental send, fix failure to rename with the name collision

[robbieko]

From: Robbie Ko <robbieko@synology.com>

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

Example scenario:
Parent snapshot:
|.                (ino 256, gen 5)
|---- a1/         (ino 257, gen 5)
|---- a2/         (ino 258, gen 5)

Send snapshot:
|.                (ino 256, gen 7)
|---- a2          (ino 257, gen 7)

rmdir a1
mkfile o257-7-0
rename o257-7-0 -> a2
ERROR: rename o257-7-0 -> a2 failed: Is a directory

While computing the send stream the following steps happen:

1) delete a1;

2) mkfile o257-7-0;

3) rename o257-7-0->a2;

In step 3 we will check whether it will lead to overwrite.

The generation number of inode 257's parent (ino 256) in send snapshot
is 7, which is inconsistent with the one in parent snapshot (gen 5).
For the general parent directory except inode 256, if its generation
is not identical between parent and send snapshot, it will be removed
then created. Thus it's not possible to happen overwrite under the new
directory. However for the special inode 256, the above logic does not
work since it is a subvolume. So overwrite check is required for the
inode 256.
Fix by skipping generation inconsistency check for inode 256.

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

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index a87675f..2060e75 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1681,6 +1681,9 @@ static int is_inode_existent(struct send_ctx *sctx, u64 ino, u64 gen)
 {
 	int ret;
 
+	if (ino == BTRFS_FIRST_FREE_OBJECTID)
+		return 1;
+
 	ret = get_cur_inode_state(sctx, ino, gen);
 	if (ret < 0)
 		goto out;
@@ -1866,7 +1869,7 @@ static int will_overwrite_ref(struct send_ctx *sctx, u64 dir, u64 dir_gen,
 	 * not deleted and then re-created, if it was then we have no overwrite
 	 * and we can just unlink this entry.
 	 */
-	if (sctx->parent_root) {
+	if (sctx->parent_root && dir != BTRFS_FIRST_FREE_OBJECTID) {
 		ret = get_inode_info(sctx->parent_root, dir, NULL, &gen, NULL,
 				     NULL, NULL, NULL);
 		if (ret < 0 && ret != -ENOENT)
-- 
1.9.1

[PATCH v3 2/6] Btrfs: incremental send, fix invalid path for truncate operations

[robbieko]

From: Robbie Ko <robbieko@synology.com>

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

Example scenario:
Parent snapshot:
|---- dir258/ (ino 258, gen 15, dir)
    |--- dir257/ (ino 257, gen 15, dir)
|---- dir259/ (ino 259, gen 15, dir)

Send snapshot:
|---- file258 (ino 258, gen 21, file)
|---- new_dir259/ (ino 259, gen 21, dir)
    |--- dir257/ (ino 257, gen 15, dir)

utimes
mkdir o259-21-0
rename dir258 -> o258-15-0
utimes
mkfile o258-21-0
rename o258-21-0 -> file258
utimes
truncate o258-21-0 size=0
ERROR: truncate o258-21-0 failed: No such file or directory

While computing the send stream the following steps happen:

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

2) Later when processing inode 258, we delay rmdir operation for dir258
   because it's not empty, and then orphanize it.

3) After we create o258-21-0 and rename it to file258, we get ENOENT on
   truncate file258. The reason is that the dir258 with inode 258 is
   waiting for rmdir operation and file258's inode is also 258, then
   get_current_path called before truncate will return a unique name.

Fix this by adding generation check for the inode waiting for rmdir
operation.

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

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 2060e75..22eca86 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -311,7 +311,7 @@ static int is_waiting_for_move(struct send_ctx *sctx, u64 ino);
 static struct waiting_dir_move *
 get_waiting_dir_move(struct send_ctx *sctx, u64 ino);
 
-static int is_waiting_for_rm(struct send_ctx *sctx, u64 dir_ino);
+static int is_waiting_for_rm(struct send_ctx *sctx, u64 dir_ino, u64 dir_gen);
 
 static int need_send_hole(struct send_ctx *sctx)
 {
@@ -2292,7 +2292,7 @@ static int get_cur_path(struct send_ctx *sctx, u64 ino, u64 gen,
 
 		fs_path_reset(name);
 
-		if (is_waiting_for_rm(sctx, ino)) {
+		if (is_waiting_for_rm(sctx, ino, gen)) {
 			ret = gen_unique_name(sctx, ino, gen, name);
 			if (ret < 0)
 				goto out;
@@ -2899,11 +2899,11 @@ get_orphan_dir_info(struct send_ctx *sctx, u64 dir_ino)
 	return NULL;
 }
 
-static int is_waiting_for_rm(struct send_ctx *sctx, u64 dir_ino)
+static int is_waiting_for_rm(struct send_ctx *sctx, u64 dir_ino, u64 dir_gen)
 {
 	struct orphan_dir_info *odi = get_orphan_dir_info(sctx, dir_ino);
 
-	return odi != NULL;
+	return (odi != NULL && odi->gen == dir_gen);
 }
 
 static void free_orphan_dir_info(struct send_ctx *sctx,
@@ -3166,7 +3166,7 @@ static int path_loop(struct send_ctx *sctx, struct fs_path *name,
 	while (ino != BTRFS_FIRST_FREE_OBJECTID) {
 		fs_path_reset(name);
 
-		if (is_waiting_for_rm(sctx, ino))
+		if (is_waiting_for_rm(sctx, ino, gen))
 			break;
 		if (is_waiting_for_move(sctx, ino)) {
 			if (*ancestor_ino == 0)
-- 
1.9.1

[PATCH v3 3/6] Btrfs: incremental send, fix not necessary waiting for rmdir operation

[robbieko]

From: Robbie Ko <robbieko@synology.com>

Under certain situations, an incremental send operation can
delay rmdir operation when processing inode 258, but it is
not necessary, because dir258 is empty.

Example scenario:
Parent snapshot:
|---- dir258/ (ino 258, gen 27)
    |---- dir257/ (ino 257, gen 27)
|---- dir259/ (ino 259, gen 27)

Send snapshot:
|---- new_dir259/ (ino 259, gen 38)
    |---- new_dir258/ (ino 258, gen 38)
        |---- new_dir257/ (ino 257, gen 38)

rmdir dir258/dir257
mkdir o257-38-0
mkdir o258-38-0
chown o257-38-0 - uid=0, gid=0
chmod o257-38-0 - mode=0755
rename dir258 -> o258-27-0  -----> dir is empty, but waiting o257-38-0
mkdir o259-38-0
chown o258-38-0 - uid=0, gid=0
chmod o258-38-0 - mode=0755
rmdir dir259
rename o259-38-0 -> new_dir259
utimes
chown new_dir259 - uid=0, gid=0
chmod new_dir259 - mode=0755
rename o258-38-0 -> new_dir259/new_dir258
utimes new_dir259/new_dir258
utimes new_dir259
rename o257-38-0 -> new_dir259/new_dir258/new_dir257
rmdir o258-27-0             -----> when o257-38-0 finish, delete
utimes new_dir259/new_dir258/new_dir257
utimes new_dir259/new_dir258
utimes new_dir259

While computing the send stream the following steps happen:

1) While processing inode 257 we delete dir258/dir257 immediately.
   After create o257-38-0, we then delay its rename operation
   because its new parent in the send snapshot, inode 258,
   was not yet processed and therefore not yet renamed.

2) On processing inode 258, we need to check if all under files are done
   before rmdir dir258. However, the waiting o257-38-0 (ino 257) will
   mislead the check making it think dir257 (ino 257) is still there.
   dir258 will be orphanized at first but in fact it has become empty.

Fix this by adding generation to waiting_dir_move struct, so can_rmdir
can use it to correct the result.

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

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 22eca86..eaf1c92 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -237,6 +237,7 @@ struct pending_dir_move {
 struct waiting_dir_move {
 	struct rb_node node;
 	u64 ino;
+	u64 gen;
 	/*
 	 * There might be some directory that could not be removed because it
 	 * was waiting for this directory inode to be moved first. Therefore
@@ -2930,6 +2931,7 @@ static int can_rmdir(struct send_ctx *sctx, u64 dir, u64 dir_gen,
 	struct btrfs_key found_key;
 	struct btrfs_key loc;
 	struct btrfs_dir_item *di;
+	u64 gen;
 
 	/*
 	 * Don't try to rmdir the top/root subvolume dir.
@@ -2969,8 +2971,13 @@ static int can_rmdir(struct send_ctx *sctx, u64 dir, u64 dir_gen,
 				struct btrfs_dir_item);
 		btrfs_dir_item_key_to_cpu(path->nodes[0], di, &loc);
 
+		ret = get_inode_info(root, loc.objectid, NULL, &gen, NULL,
+				     NULL, NULL, NULL);
+		if (ret < 0)
+			goto out;
+
 		dm = get_waiting_dir_move(sctx, loc.objectid);
-		if (dm) {
+		if (dm && dm->gen == gen) {
 			struct orphan_dir_info *odi;
 
 			odi = add_orphan_dir_info(sctx, dir);
@@ -3010,7 +3017,8 @@ static int is_waiting_for_move(struct send_ctx *sctx, u64 ino)
 	return entry != NULL;
 }
 
-static int add_waiting_dir_move(struct send_ctx *sctx, u64 ino, bool orphanized)
+static int add_waiting_dir_move(struct send_ctx *sctx, u64 ino, u64 gen,
+		     bool orphanized)
 {
 	struct rb_node **p = &sctx->waiting_dir_moves.rb_node;
 	struct rb_node *parent = NULL;
@@ -3020,6 +3028,7 @@ static int add_waiting_dir_move(struct send_ctx *sctx, u64 ino, bool orphanized)
 	if (!dm)
 		return -ENOMEM;
 	dm->ino = ino;
+	dm->gen = gen;
 	dm->rmdir_ino = 0;
 	dm->orphanized = orphanized;
 
@@ -3117,7 +3126,7 @@ static int add_pending_dir_move(struct send_ctx *sctx,
 			goto out;
 	}
 
-	ret = add_waiting_dir_move(sctx, pm->ino, is_orphan);
+	ret = add_waiting_dir_move(sctx, pm->ino, pm->gen, is_orphan);
 	if (ret)
 		goto out;
 
-- 
1.9.1

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

[robbieko]

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(+)

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

[PATCH v3 5/6] Btrfs: incremental send, fix invalid rename operations

[robbieko]

From: Robbie Ko <robbieko@synology.com>

Under certain situations, an incremental send operation can
a rename operation that will make the receiving end fail when
attempting to execute it, because the source has been deleted.

Example scenario:
Parent snapshot:
|---- d1/              (ino 257, gen 44)
|---- d4/              (ino 258, gen 44)
|---- d3/              (ino 259, gen 44)

Send snapshot:
|---- d1/              (ino 258, gen 47)
|---- d4/              (ino 260, gen 47)
    |---- d3/          (ino 259, gen 47)
        |---- d1/          (ino 257, gen 47)

rmdir d1
mkdir o257-47-0
mkdir o259-47-0
chown o257-47-0 - uid=0, gid=0
chmod o257-47-0 - mode=0755
rmdir d4
mkdir o258-47-0
rename d1 -> o257-44-0
ERROR: rename d1 -> o257-44-0 failed: No such file or directory

While computing the send stream the following steps happen:

1) While processing inode 257 we remove the d1 (ino 257, gen 44),
   create o257-47-0 and o259-47-0, and delay o257-47-0 rename operation
   because its new parent in the send snapshot, inode 259, was not yet
   processed and therefore not yet renamed;

2) Later when processing on inode 258, after we create o258-47-0 before
   rename it to d1, we need to check whether it will overwrite others.
   It shows it will overwrite to d1 (inode 257, gen 44) so needs to
   rename d1 (inode 257, gen 44) to unique name. But it was previously
   removed, which leads to rename failed. The reason why we thought
   d1 (inode 257, gen 44) would be overwirtten is inode 257 with diffent
   generation 47 is waiting for move, then we are mislead they are the
   same since we miss the generation check for them.

Fix this by adding generation check for the inode if it is waiting for
move.

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

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 139f492..81a2bee 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1857,6 +1857,7 @@ static int will_overwrite_ref(struct send_ctx *sctx, u64 dir, u64 dir_gen,
 	u64 gen;
 	u64 other_inode = 0;
 	u8 other_type = 0;
+	struct waiting_dir_move *dm = NULL;
 
 	if (!sctx->parent_root)
 		goto out;
@@ -1898,11 +1899,15 @@ static int will_overwrite_ref(struct send_ctx *sctx, u64 dir, u64 dir_gen,
 	 * overwrite anything at this point in time.
 	 */
 	if (other_inode > sctx->send_progress ||
-	    is_waiting_for_move(sctx, other_inode)) {
+		((dm = get_waiting_dir_move(sctx, other_inode)) != NULL)) {
 		ret = get_inode_info(sctx->parent_root, other_inode, NULL,
 				who_gen, NULL, NULL, NULL, NULL);
 		if (ret < 0)
 			goto out;
+		if (dm && dm->gen != *who_gen) {
+			ret = 0;
+			goto out;
+		}
 
 		ret = 1;
 		*who_ino = other_inode;
-- 
1.9.1

[PATCH v3 6/6] Btrfs: incremental send, fix invalid utime operations

[robbieko]

From: Robbie Ko <robbieko@synology.com>

Under certain situations, an incremental send operation can
a utime operation that will make the receiving end fail when
attempting to execute it, because the path has been deleted.

Exampla scenario:
Parent snapshot:
|---- dir258/         (ino 258, gen 7, dir)
    |--- dir257/      (ino 257, gen 7, dir)
|---- dir259/         (ino 259, gen 7, dir)

Send snapshot:
|---- file258         (ino 258, gen 10, file)
|---- new_dir259/     (ino 259, gen 10, dir)
    |--- dir257/      (ino 257, gen 7, dir)

mkdir o259-10-0
rename dir258 -> o258-7-0
utimes
mkfile o258-10-0
rename o258-10-0 -> file258
utimes
truncate file258 size=0
chown file258 - uid=0, gid=0
chmod file258 - mode=0644
utimes file258
rmdir dir259
utimes
rename o259-10-0 -> new_dir259
utimes
chown new_dir259 - uid=0, gid=0
chmod new_dir259 - mode=0755
rename o258-7-0/dir257 -> new_dir259/dir257
rmdir o258-7-0
utimes new_dir259/dir257
utimes o258-7-0
ERROR: utimes o258-7-0 failed: No such file or directory

While computing the send stream the following steps happen:

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

2) Later when processing inode 258 we orphanize dir258, and rename it to
   o258-7-0, because under dir258 there is a dir257 waiting to be moved;

3) Finally, when 259 is finished processing we rename o258-7-0/dir257 to
   new_dir259/dir257, and then remove directory o258-7-0. After updating
   the time in the two parents if it's exist, we only check whether
   the inode number exists in the send snapshot, which result in utimes
   error, because old parent has been deleted;

Fix this by adding generation check in existence demtermination for
the parent directory.

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

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 81a2bee..b4a4724 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -3338,14 +3338,17 @@ finish:
 		/*
 		 * The parent inode might have been deleted in the send snapshot
 		 */
+		u64 gen;
 		ret = get_inode_info(sctx->send_root, cur->dir, NULL,
-				     NULL, NULL, NULL, NULL, NULL);
-		if (ret == -ENOENT) {
+				     &gen, NULL, NULL, NULL, NULL);
+
+		if (ret < 0 && ret != -ENOENT)
+			goto out;
+
+		if (ret == -ENOENT || gen != cur->dir_gen) {
 			ret = 0;
 			continue;
 		}
-		if (ret < 0)
-			goto out;
 
 		ret = send_utimes(sctx, cur->dir, cur->dir_gen);
 		if (ret < 0)
-- 
1.9.1

Re: [PATCH v3 1/6] Btrfs: incremental send, fix failure to rename with the name collision

[Filipe Manana]

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

Missing some word after the word "can".

> a rename operation that will make the receiving end fail when
> attempting to execute it, because the target is exist.
>
> Example scenario:
> Parent snapshot:

Two consecutive sentences ending with a colon is odd.

> |.                (ino 256, gen 5)

. is not a child of anything, shouldn't have the | preceding it.

> |---- a1/         (ino 257, gen 5)
> |---- a2/         (ino 258, gen 5)
>
> Send snapshot:
> |.                (ino 256, gen 7)

Same here.

> |---- a2          (ino 257, gen 7)
>
> rmdir a1
> mkfile o257-7-0
> rename o257-7-0 -> a2
> ERROR: rename o257-7-0 -> a2 failed: Is a directory

Alright what is this? So you just paste here the output of "btrfs
receive -vv" without mentioning where it comes from.
Let the reader guess and make some sense of it.

>
> While computing the send stream the following steps happen:
>
> 1) delete a1;
>
> 2) mkfile o257-7-0;
>
> 3) rename o257-7-0->a2;
>
> In step 3 we will check whether it will lead to overwrite.
>
> The generation number of inode 257's parent (ino 256) in send snapshot
> is 7, which is inconsistent with the one in parent snapshot (gen 5).
> For the general parent directory except inode 256, if its generation
> is not identical between parent and send snapshot, it will be removed
> then created. Thus it's not possible to happen overwrite under the new
> directory. However for the special inode 256, the above logic does not
> work since it is a subvolume. So overwrite check is required for the
> inode 256.
> Fix by skipping generation inconsistency check for inode 256.

I've heavily reworded the explanation and made other changes to the
changelog myself.
You should have given an example on how to create the snapshots so
that it's clear how to get different inodes with the same number but
different generations.

I've pushed into an integration branch in my git repository with the
goal of including it for 4.11:

https://git.kernel.org/cgit/linux/kernel/git/fdmanana/linux.git/log/?h=integration-4.11

The subject was also changed to "Btrfs: send, fix failure to rename
top level inode due to name collision".

So for this patch, you don't need to do anything else.
Thanks.

>
> Signed-off-by: Robbie Ko <robbieko@synology.com>
> ---
> V3: improve the change log
>  fs/btrfs/send.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index a87675f..2060e75 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -1681,6 +1681,9 @@ static int is_inode_existent(struct send_ctx *sctx, u64 ino, u64 gen)
>  {
>         int ret;
>
> +       if (ino == BTRFS_FIRST_FREE_OBJECTID)
> +               return 1;
> +
>         ret = get_cur_inode_state(sctx, ino, gen);
>         if (ret < 0)
>                 goto out;
> @@ -1866,7 +1869,7 @@ static int will_overwrite_ref(struct send_ctx *sctx, u64 dir, u64 dir_gen,
>          * not deleted and then re-created, if it was then we have no overwrite
>          * and we can just unlink this entry.
>          */
> -       if (sctx->parent_root) {
> +       if (sctx->parent_root && dir != BTRFS_FIRST_FREE_OBJECTID) {
>                 ret = get_inode_info(sctx->parent_root, dir, NULL, &gen, NULL,
>                                      NULL, NULL, NULL);
>                 if (ret < 0 && ret != -ENOENT)
> --
> 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



-- 
Filipe David Manana,

"People will forget what you said,
 people will forget what you did,
 but people will never forget how you made them feel."

Re: [PATCH v3 2/6] Btrfs: incremental send, fix invalid path for truncate operations

[Filipe Manana]

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", without it the phrase
doesn't make any sense.
Tip: don't copy paste change logs and then modify them for each patch,
it's a lot easier to make errors like that.

> a truncate operation that will make the receiving end fail when
> attempting to execute it, because the path is not exist.

The problem is generating wrong paths, and that can be for any
operation, not just truncates.
The subject should reflect that. As it is, it is misleading.

>
> Example scenario:
> Parent snapshot:
> |---- dir258/ (ino 258, gen 15, dir)
>     |--- dir257/ (ino 257, gen 15, dir)
> |---- dir259/ (ino 259, gen 15, dir)
>
> Send snapshot:
> |---- file258 (ino 258, gen 21, file)
> |---- new_dir259/ (ino 259, gen 21, dir)
>     |--- dir257/ (ino 257, gen 15, dir)
>
> utimes
> mkdir o259-21-0
> rename dir258 -> o258-15-0
> utimes
> mkfile o258-21-0
> rename o258-21-0 -> file258
> utimes
> truncate o258-21-0 size=0
> ERROR: truncate o258-21-0 failed: No such file or directory
>
> While computing the send stream the following steps happen:
>
> 1) While processing inode 257, we create o259-21-0 and delay
>    dir257 rename operation, because its new parent in the send
>    snapshot, inode 259, was not yet processed and therefore not
>    yet renamed.

The problem is exactly here. There's no need to delay the rename of
257, because inode 259 is a new inode.
The whole logic for delaying renames is meant to be applied when
there's really a need to do so, when the child and parent inodes exist
in both snapshots.
So the problem is fully fixed by not delaying the rename for 257,
doing this also avoids the extra complexity and the need for all the
patches in your patchset except for patches 1/6 and 4/6.

I've added such a fix into my 4.11 integration branch and just sent it
to the list too.
It's here:

https://git.kernel.org/cgit/linux/kernel/git/fdmanana/linux.git/commit/?h=integration-4.11&id=b271d6c72e1dc34e93ff6035096e29d45ffa933e

thanks



>
> 2) Later when processing inode 258, we delay rmdir operation for dir258
>    because it's not empty, and then orphanize it.
>
> 3) After we create o258-21-0 and rename it to file258, we get ENOENT on
>    truncate file258. The reason is that the dir258 with inode 258 is
>    waiting for rmdir operation and file258's inode is also 258, then
>    get_current_path called before truncate will return a unique name.
>
> Fix this by adding generation check for the inode waiting for rmdir
> operation.
>
> Signed-off-by: Robbie Ko <robbieko@synology.com>
> ---
> V3: improve the change log
>  fs/btrfs/send.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 2060e75..22eca86 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -311,7 +311,7 @@ static int is_waiting_for_move(struct send_ctx *sctx, u64 ino);
>  static struct waiting_dir_move *
>  get_waiting_dir_move(struct send_ctx *sctx, u64 ino);
>
> -static int is_waiting_for_rm(struct send_ctx *sctx, u64 dir_ino);
> +static int is_waiting_for_rm(struct send_ctx *sctx, u64 dir_ino, u64 dir_gen);
>
>  static int need_send_hole(struct send_ctx *sctx)
>  {
> @@ -2292,7 +2292,7 @@ static int get_cur_path(struct send_ctx *sctx, u64 ino, u64 gen,
>
>                 fs_path_reset(name);
>
> -               if (is_waiting_for_rm(sctx, ino)) {
> +               if (is_waiting_for_rm(sctx, ino, gen)) {
>                         ret = gen_unique_name(sctx, ino, gen, name);
>                         if (ret < 0)
>                                 goto out;
> @@ -2899,11 +2899,11 @@ get_orphan_dir_info(struct send_ctx *sctx, u64 dir_ino)
>         return NULL;
>  }
>
> -static int is_waiting_for_rm(struct send_ctx *sctx, u64 dir_ino)
> +static int is_waiting_for_rm(struct send_ctx *sctx, u64 dir_ino, u64 dir_gen)
>  {
>         struct orphan_dir_info *odi = get_orphan_dir_info(sctx, dir_ino);
>
> -       return odi != NULL;
> +       return (odi != NULL && odi->gen == dir_gen);
>  }
>
>  static void free_orphan_dir_info(struct send_ctx *sctx,
> @@ -3166,7 +3166,7 @@ static int path_loop(struct send_ctx *sctx, struct fs_path *name,
>         while (ino != BTRFS_FIRST_FREE_OBJECTID) {
>                 fs_path_reset(name);
>
> -               if (is_waiting_for_rm(sctx, ino))
> +               if (is_waiting_for_rm(sctx, ino, gen))
>                         break;
>                 if (is_waiting_for_move(sctx, ino)) {
>                         if (*ancestor_ino == 0)
> --
> 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



-- 
Filipe David Manana,

"People will forget what you said,
 people will forget what you did,
 but people will never forget how you made them feel."

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

[Filipe Manana]

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



-- 
Filipe David Manana,

"People will forget what you said,
 people will forget what you did,
 but people will never forget how you made them feel."

Re: [PATCH v3 0/6] Btrfs: incremental send, fix serval case failure

[David Sterba]

On Thu, Jan 05, 2017 at 04:24:54PM +0800, robbieko wrote:
> From: Robbie Ko <robbieko@synology.com>
> 
> Patch for fix btrfs incremental send.
> These patches base on v4.8.0-rc8

Is this a typo or did you really base the patches on 4.8-rc8? At the
moment we're nearing the v4.11 development cycle, so patches should be
eg. based on a recent 4.10-rc from the master branch. Let me know
privately if you need more information on the development cycle and
workflow.

> V3: Improve the change log

Please take Filipe's advice and comments seriously. Changelogs are
important part of the patches, namely for fixes that are not obvious and
fix corner cases. Writing good changelogs takes time to learn, you can
find examples in the log of send.c .

Re: [PATCH v3 0/6] Btrfs: incremental send, fix serval case failure

[Giuseppe Della Bianca]

Hi.

This looks bit like at my issue with the differential sending/receiving
and snapshot deleting.

Thanks for the test cases and for the patches.


Regards. 

Gdb

>>robbieko Thu, 05 Jan 2017 00:47:54 -0800


>>From: Robbie Ko <robbi...@synology.com>

>>Patch for fix btrfs incremental send.
>>These patches base on v4.8.0-rc8
>>V3: Improve the change log
>>V2: Add a new patch "add generation check in existence demtermination for
    the parent directory"

>>Robbie Ko (6):
>>  Btrfs: incremental send, fix failure to rename with the name collision
>>  Btrfs: incremental send, fix invalid path for truncate operations
>>  Btrfs: incremental send, fix not necessary waiting for rmdir operation
>>  Btrfs: incremental send, fix invalid path for rmdir operations
>>  Btrfs: incremental send, fix invalid rename operations
>>  Btrfs: incremental send, fix invalid utime operations
]zac[