[PATCH 1/4] btrfs-progs: Fix infinite loop of btrfs subvolumn sync

[PATCH 1/4] btrfs-progs: Fix infinite loop of btrfs subvolumn sync

[Zhao Lei]

We can trigger the bug by following operation:
  (no wait between commands 3~5)
  btrfs subvolume create /mnt/btrfs/mysubvol
  btrfs subvolume snapshot /mnt/btrfs/mysubvol /mnt/btrfs/mysubvol_snap
  btrfs subvolume delete /mnt/btrfs/mysubvol_snap
  btrfs subvolume delete /mnt/btrfs/mysubvol
  btrfs subvolume sync /mnt/btrfs
The last command will not exit.

Reason:
  List of "deleted subvolumes" are not currectly set.

  It caused by a typo of value assign, in detail:
  *ids[idx] = sh->offset;
  should be:
  (*ids)[idx] = sh->offset;
  So only first element is set to right memory address.

  If there are multiple "deleted subvolumes", program will
  keep wait.

Above typo also caused some segment fault in my test.

This patch fixed above bug.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 cmds-subvolume.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 61f08f5..8e8d019 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -1207,7 +1207,7 @@ static int enumerate_dead_subvols(int fd, int count, u64 **ids)
 			off += sizeof(*sh);
 
 			if (sh->type == BTRFS_ORPHAN_ITEM_KEY) {
-				*ids[idx] = sh->offset;
+				(*ids)[idx] = sh->offset;
 				idx++;
 				if (idx >= count) {
 					u64 *newids;
-- 
1.8.5.1

[PATCH 2/4] btrfs-progs: Simplify memory allocation for enumerate_dead_subvols

[Zhao Lei]

No need prepare memory for enumerate_dead_subvols() in caller, and pass
additional argument for allocated length.

Just do every thing inside enumerate_dead_subvols(), it will not
increase malloc count, but make code simple.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 cmds-subvolume.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 8e8d019..0d83bd5 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -1169,12 +1169,13 @@ again:
  * Enumerate all dead subvolumes that exist in the filesystem.
  * Fill @ids and reallocate to bigger size if needed.
  */
-static int enumerate_dead_subvols(int fd, int count, u64 **ids)
+static int enumerate_dead_subvols(int fd, u64 **ids)
 {
 	int ret;
 	struct btrfs_ioctl_search_args args;
 	struct btrfs_ioctl_search_key *sk = &args.key;
 	int idx = 0;
+	int count = 0;
 
 	memset(&args, 0, sizeof(args));
 
@@ -1189,6 +1190,7 @@ static int enumerate_dead_subvols(int fd, int count, u64 **ids)
 	sk->max_transid = (u64)-1;
 	sk->nr_items = 4096;
 
+	*ids = NULL;
 	while (1) {
 		struct btrfs_ioctl_search_header *sh;
 		unsigned long off;
@@ -1207,8 +1209,6 @@ static int enumerate_dead_subvols(int fd, int count, u64 **ids)
 			off += sizeof(*sh);
 
 			if (sh->type == BTRFS_ORPHAN_ITEM_KEY) {
-				(*ids)[idx] = sh->offset;
-				idx++;
 				if (idx >= count) {
 					u64 *newids;
 
@@ -1218,6 +1218,8 @@ static int enumerate_dead_subvols(int fd, int count, u64 **ids)
 						return -ENOMEM;
 					*ids = newids;
 				}
+				(*ids)[idx] = sh->offset;
+				idx++;
 			}
 			off += sh->len;
 
@@ -1284,14 +1286,7 @@ static int cmd_subvol_sync(int argc, char **argv)
 
 	id_count = argc - optind;
 	if (!id_count) {
-		id_count = SUBVOL_ID_BATCH;
-		ids = (u64*)malloc(id_count * sizeof(u64));
-		if (!ids) {
-			fprintf(stderr, "ERROR: not enough memory\n");
-			ret = 1;
-			goto out;
-		}
-		id_count = enumerate_dead_subvols(fd, id_count, &ids);
+		id_count = enumerate_dead_subvols(fd, &ids);
 		if (id_count < 0) {
 			fprintf(stderr, "ERROR: can't enumerate dead subvolumes: %s\n",
 					strerror(-id_count));
-- 
1.8.5.1

[PATCH 3/4] btrfs-progs: Fix wrong return value of wait_for_subvolume_cleaning()

[Zhao Lei]

Reproduce:
 # btrfs subvolume sync /mnt/btrfs
 Subvolume id 323 is gone
 # echo $?
 1
 #

Reason:
 wait_for_subvolume_cleaning() return !0 in right case, because
 value of ret is set to "is subvolume clean" state before return.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 cmds-subvolume.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 0d83bd5..0b398f0 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -66,7 +66,7 @@ static int is_subvolume_cleaned(int fd, u64 subvolid)
 static int wait_for_subvolume_cleaning(int fd, int count, u64 *ids,
 		int sleep_interval)
 {
-	int ret = 0;
+	int ret;
 	int remaining;
 	int i;
 
@@ -92,6 +92,8 @@ static int wait_for_subvolume_cleaning(int fd, int count, u64 *ids,
 			break;
 		sleep(sleep_interval);
 	}
+
+	ret = 0;
 out:
 	return ret;
 }
-- 
1.8.5.1

[PATCH 4/4] btrfs-progs: Simplify all-subvolumn-clean condition for wait_for_subvolume_cleaning

[Zhao Lei]

Instead of using a dirty-subvolumn-counter in old code, this patch
turn to use a simple and direct way:
  If (not dirty-subvolumn found in current loop) {
      return all_clean;
  }

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 cmds-subvolume.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 0b398f0..20e2c01 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -67,11 +67,11 @@ static int wait_for_subvolume_cleaning(int fd, int count, u64 *ids,
 		int sleep_interval)
 {
 	int ret;
-	int remaining;
+	int dirty_cnt;
 	int i;
 
-	remaining = count;
 	while (1) {
+		dirty_cnt = 0;
 		for (i = 0; i < count; i++) {
 			if (!ids[i])
 				continue;
@@ -80,22 +80,21 @@ static int wait_for_subvolume_cleaning(int fd, int count, u64 *ids,
 				fprintf(stderr,
 					"ERROR: can't perform the search - %s\n",
 					strerror(-ret));
-				goto out;
+				return ret;
 			}
 			if (ret) {
 				printf("Subvolume id %llu is gone\n", ids[i]);
 				ids[i] = 0;
-				remaining--;
+			} else {
+				dirty_cnt++;
 			}
 		}
-		if (!remaining)
+		if (dirty_cnt == 0)
 			break;
 		sleep(sleep_interval);
 	}
 
-	ret = 0;
-out:
-	return ret;
+	return 0;
 }
 
 static const char * const subvolume_cmd_group_usage[] = {
-- 
1.8.5.1

Re: [PATCH 1/4] btrfs-progs: Fix infinite loop of btrfs subvolumn sync

[David Sterba]

On Wed, Aug 26, 2015 at 10:03:36PM +0800, Zhao Lei wrote:
> We can trigger the bug by following operation:
>   (no wait between commands 3~5)
>   btrfs subvolume create /mnt/btrfs/mysubvol
>   btrfs subvolume snapshot /mnt/btrfs/mysubvol /mnt/btrfs/mysubvol_snap
>   btrfs subvolume delete /mnt/btrfs/mysubvol_snap
>   btrfs subvolume delete /mnt/btrfs/mysubvol
>   btrfs subvolume sync /mnt/btrfs
> The last command will not exit.
> 
> Reason:
>   List of "deleted subvolumes" are not currectly set.
> 
>   It caused by a typo of value assign, in detail:
>   *ids[idx] = sh->offset;
>   should be:
>   (*ids)[idx] = sh->offset;
>   So only first element is set to right memory address.
> 
>   If there are multiple "deleted subvolumes", program will
>   keep wait.
> 
> Above typo also caused some segment fault in my test.
> 
> This patch fixed above bug.
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>

Applied, thanks.

Re: [PATCH 3/4] btrfs-progs: Fix wrong return value of wait_for_subvolume_cleaning()

[David Sterba]

On Wed, Aug 26, 2015 at 10:03:38PM +0800, Zhao Lei wrote:
> Reproduce:
>  # btrfs subvolume sync /mnt/btrfs
>  Subvolume id 323 is gone
>  # echo $?
>  1
>  #
> 
> Reason:
>  wait_for_subvolume_cleaning() return !0 in right case, because
>  value of ret is set to "is subvolume clean" state before return.
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>

Applied, thanks.

Re: [PATCH 2/4] btrfs-progs: Simplify memory allocation for enumerate_dead_subvols

[David Sterba]

On Wed, Aug 26, 2015 at 10:03:37PM +0800, Zhao Lei wrote:
> No need prepare memory for enumerate_dead_subvols() in caller, and pass
> additional argument for allocated length.
> 
> Just do every thing inside enumerate_dead_subvols(), it will not
> increase malloc count, but make code simple.
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>

Applied, thanks.

Re: [PATCH 4/4] btrfs-progs: Simplify all-subvolumn-clean condition for wait_for_subvolume_cleaning

[David Sterba]

On Wed, Aug 26, 2015 at 10:03:39PM +0800, Zhao Lei wrote:
> Instead of using a dirty-subvolumn-counter in old code, this patch
> turn to use a simple and direct way:
>   If (not dirty-subvolumn found in current loop) {
>       return all_clean;
>   }
> 
> Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>

Applied with minor modifications, thanks.