linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/6] Btrfs: incremental send, fix serval case for inode 256 and generation
@ 2016-10-28  1:40 robbieko
  2016-10-28  1:40 ` [PATCH v2 1/6] Btrfs: incremental send, do not skip generation inconsistency check for inode 256 robbieko
                   ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: robbieko @ 2016-10-28  1:40 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Robbie Ko

From: Robbie Ko <robbieko@synology.com>

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

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

Robbie Ko (6):
  Btrfs: incremental send, do not skip generation inconsistency check
    for inode 256.
  Btrfs: incremental send, add generation check for the inode waiting
    for rmdir operation.
  Btrfs: incremental send, add generation for waiting_dir_move struct
    and check it in can_rmdir.
  Btrfs: incremental send, skip check overwritten if parents' generation
    are different
  Btrfs: incremental send, add generation check for inode is waiting for
    move.
  Btrfs: incremental send, add generation check in existence
    demtermination for the parent directory

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

-- 
1.9.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/6] Btrfs: incremental send, do not skip generation inconsistency check for inode 256.
  2016-10-28  1:40 [PATCH v2 0/6] Btrfs: incremental send, fix serval case for inode 256 and generation robbieko
@ 2016-10-28  1:40 ` robbieko
  2016-10-28  1:40 ` [PATCH v2 2/6] Btrfs: incremental send, add generation check for the inode waiting for rmdir operation robbieko
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: robbieko @ 2016-10-28  1:40 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Robbie Ko

From: Robbie Ko <robbieko@synology.com>

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>
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/6] Btrfs: incremental send, add generation check for the inode waiting for rmdir operation.
  2016-10-28  1:40 [PATCH v2 0/6] Btrfs: incremental send, fix serval case for inode 256 and generation robbieko
  2016-10-28  1:40 ` [PATCH v2 1/6] Btrfs: incremental send, do not skip generation inconsistency check for inode 256 robbieko
@ 2016-10-28  1:40 ` robbieko
  2016-10-28  1:40 ` [PATCH v2 3/6] Btrfs: incremental send, add generation for waiting_dir_move struct and check it in can_rmdir robbieko
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: robbieko @ 2016-10-28  1:40 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Robbie Ko

From: Robbie Ko <robbieko@synology.com>

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>
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/6] Btrfs: incremental send, add generation for waiting_dir_move struct and check it in can_rmdir.
  2016-10-28  1:40 [PATCH v2 0/6] Btrfs: incremental send, fix serval case for inode 256 and generation robbieko
  2016-10-28  1:40 ` [PATCH v2 1/6] Btrfs: incremental send, do not skip generation inconsistency check for inode 256 robbieko
  2016-10-28  1:40 ` [PATCH v2 2/6] Btrfs: incremental send, add generation check for the inode waiting for rmdir operation robbieko
@ 2016-10-28  1:40 ` robbieko
  2016-10-28 15:13   ` Filipe Manana
  2016-10-28  1:40 ` [PATCH v2 4/6] Btrfs: incremental send, skip check overwritten if parents' generation are different robbieko
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 8+ messages in thread
From: robbieko @ 2016-10-28  1:40 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Robbie Ko

From: Robbie Ko <robbieko@synology.com>

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>
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 4/6] Btrfs: incremental send, skip check overwritten if parents' generation are different
  2016-10-28  1:40 [PATCH v2 0/6] Btrfs: incremental send, fix serval case for inode 256 and generation robbieko
                   ` (2 preceding siblings ...)
  2016-10-28  1:40 ` [PATCH v2 3/6] Btrfs: incremental send, add generation for waiting_dir_move struct and check it in can_rmdir robbieko
@ 2016-10-28  1:40 ` robbieko
  2016-10-28  1:40 ` [PATCH v2 5/6] Btrfs: incremental send, add generation check for inode is waiting for move robbieko
  2016-10-28  1:40 ` [PATCH v2 6/6] Btrfs: incremental send, add generation check in existence demtermination for the parent directory robbieko
  5 siblings, 0 replies; 8+ messages in thread
From: robbieko @ 2016-10-28  1:40 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Robbie Ko

From: Robbie Ko <robbieko@synology.com>

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>
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 5/6] Btrfs: incremental send, add generation check for inode is waiting for move.
  2016-10-28  1:40 [PATCH v2 0/6] Btrfs: incremental send, fix serval case for inode 256 and generation robbieko
                   ` (3 preceding siblings ...)
  2016-10-28  1:40 ` [PATCH v2 4/6] Btrfs: incremental send, skip check overwritten if parents' generation are different robbieko
@ 2016-10-28  1:40 ` robbieko
  2016-10-28  1:40 ` [PATCH v2 6/6] Btrfs: incremental send, add generation check in existence demtermination for the parent directory robbieko
  5 siblings, 0 replies; 8+ messages in thread
From: robbieko @ 2016-10-28  1:40 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Robbie Ko

From: Robbie Ko <robbieko@synology.com>

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>
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 6/6] Btrfs: incremental send, add generation check in existence demtermination for the parent directory
  2016-10-28  1:40 [PATCH v2 0/6] Btrfs: incremental send, fix serval case for inode 256 and generation robbieko
                   ` (4 preceding siblings ...)
  2016-10-28  1:40 ` [PATCH v2 5/6] Btrfs: incremental send, add generation check for inode is waiting for move robbieko
@ 2016-10-28  1:40 ` robbieko
  5 siblings, 0 replies; 8+ messages in thread
From: robbieko @ 2016-10-28  1:40 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Robbie Ko

From: Robbie Ko <robbieko@synology.com>

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>
---
 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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 3/6] Btrfs: incremental send, add generation for waiting_dir_move struct and check it in can_rmdir.
  2016-10-28  1:40 ` [PATCH v2 3/6] Btrfs: incremental send, add generation for waiting_dir_move struct and check it in can_rmdir robbieko
@ 2016-10-28 15:13   ` Filipe Manana
  0 siblings, 0 replies; 8+ messages in thread
From: Filipe Manana @ 2016-10-28 15:13 UTC (permalink / raw)
  To: robbieko; +Cc: linux-btrfs@vger.kernel.org

On Fri, Oct 28, 2016 at 2:40 AM, robbieko <robbieko@synology.com> wrote:
> From: Robbie Ko <robbieko@synology.com>
>
> 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.

Fix what?
Your changelog does not say what is the problem. Does the send
operation fails? Or is it the receiver that fails for example due to
an operation using a wrong path name or attempting to rmdir a
non-empty directory? Or is the problem something else, like for
example are you optimizing something like avoiding unnecessary
renames?

Also the subject is not a summary of what is being fixed (e.g. "btrfs:
incremental send, fix invalid rmdir operations"). Instead you say what
code changes you do... and not what problem they are solving.

I haven't read all the other patches you sent along (and probably
won't have the time today), but by just looking at their subjects I
can infer the same comments apply.




>
> Signed-off-by: Robbie Ko <robbieko@synology.com>
> ---

After this line (---) you should mention what changed between patch
versions, see [1]

[1] https://btrfs.wiki.kernel.org/index.php/Writing_patch_for_btrfs#Updating_patches


thanks


>  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
>
> --
> 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."

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-10-28 15:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-28  1:40 [PATCH v2 0/6] Btrfs: incremental send, fix serval case for inode 256 and generation robbieko
2016-10-28  1:40 ` [PATCH v2 1/6] Btrfs: incremental send, do not skip generation inconsistency check for inode 256 robbieko
2016-10-28  1:40 ` [PATCH v2 2/6] Btrfs: incremental send, add generation check for the inode waiting for rmdir operation robbieko
2016-10-28  1:40 ` [PATCH v2 3/6] Btrfs: incremental send, add generation for waiting_dir_move struct and check it in can_rmdir robbieko
2016-10-28 15:13   ` Filipe Manana
2016-10-28  1:40 ` [PATCH v2 4/6] Btrfs: incremental send, skip check overwritten if parents' generation are different robbieko
2016-10-28  1:40 ` [PATCH v2 5/6] Btrfs: incremental send, add generation check for inode is waiting for move robbieko
2016-10-28  1:40 ` [PATCH v2 6/6] Btrfs: incremental send, add generation check in existence demtermination for the parent directory robbieko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).