[PATCH v2 0/5] btrfs-progs: subvol: fix del --commit-after

[PATCH v2 0/5] btrfs-progs: subvol: fix del --commit-after

[Misono, Tomohiro]

Fix subvol del --commit-after to work properly:
 - SYNC ioctl will be issued even when last delete is failed
 - SYNC ioctl will be issued on each file system only once in the end

To achieve this, each deleted subvol's (parent's) fsid is checked each
time. If the fsid is seen for the first time, its fd will be kept in order
to issue SYNC ioctl in the end.

There already exists get_fsid() in cmds-property.c and seen_fsid which
keeps fsid in hush function in cmds-filesystem.c. This patch utilizes
them.

First patch is the independent but critical. Current code is reversed in
--commit-after and --commit-each operation. i.e. --commit-after means
--commit-each actually. The patch fix this.

2rd to 4th patches make functions stated above to common and last one is
the main part.

Thanks Qu for reviewing whole patches.

change to v2:
	split the cleanup part of 4th patch and make it the independent patch
	(1st patch in new series).

Tomohiro Misono (5):
  btrfs-progs: subvol: exchange subvol del --commit-after and --commit-each
  btrfs-progs: move get_fsid() to util.c
  btrfs-progs: move seen_fsid to util.c
  btrfs-progs: change seen_fsid to hold fd and DIR*
  btrfs-progs: subvol: fix subvol del --commit-after

 cmds-filesystem.c |  88 +++------------------------------------------
 cmds-property.c   |  30 ----------------
 cmds-subvolume.c  |  72 +++++++++++++++++++++++++++----------
 utils.c           | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 utils.h           |  15 ++++++++
 5 files changed, 179 insertions(+), 131 deletions(-)

-- 
2.9.5

[PATCH v2 1/5] btrfs-progs: subvol: exchange subvol del --commit-after and --commit-each

[Misono, Tomohiro]

Current code is reversed in --commit-after and --commit-each operation.
i.e. --commit-after means --commit-each actually. This patch fix this
and also introduces enum type for more readable code.

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 cmds-subvolume.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 666f6e0..9e561f3 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -263,6 +263,7 @@ static int cmd_subvol_delete(int argc, char **argv)
 	DIR	*dirstream = NULL;
 	int verbose = 0;
 	int commit_mode = 0;
+	enum { COMMIT_AFTER = 1, COMMIT_EACH = 2 };
 
 	while (1) {
 		int c;
@@ -279,10 +280,10 @@ static int cmd_subvol_delete(int argc, char **argv)
 
 		switch(c) {
 		case 'c':
-			commit_mode = 1;
+			commit_mode = COMMIT_AFTER;
 			break;
 		case 'C':
-			commit_mode = 2;
+			commit_mode = COMMIT_EACH;
 			break;
 		case 'v':
 			verbose++;
@@ -298,7 +299,7 @@ static int cmd_subvol_delete(int argc, char **argv)
 	if (verbose > 0) {
 		printf("Transaction commit: %s\n",
 			!commit_mode ? "none (default)" :
-			commit_mode == 1 ? "at the end" : "after each");
+			commit_mode == COMMIT_AFTER ? "at the end" : "after each");
 	}
 
 	cnt = optind;
@@ -338,7 +339,7 @@ again:
 	}
 
 	printf("Delete subvolume (%s): '%s/%s'\n",
-		commit_mode == 2 || (commit_mode == 1 && cnt + 1 == argc)
+		commit_mode == COMMIT_EACH || (commit_mode == COMMIT_AFTER && cnt + 1 == argc)
 		? "commit" : "no-commit", dname, vname);
 	memset(&args, 0, sizeof(args));
 	strncpy_null(args.name, vname);
@@ -350,7 +351,7 @@ again:
 		goto out;
 	}
 
-	if (commit_mode == 1) {
+	if (commit_mode == COMMIT_EACH) {
 		res = wait_for_commit(fd);
 		if (res < 0) {
 			error("unable to wait for commit after '%s': %s",
@@ -373,7 +374,7 @@ out:
 		goto again;
 	}
 
-	if (commit_mode == 2 && fd != -1) {
+	if (commit_mode == COMMIT_AFTER && fd != -1) {
 		res = wait_for_commit(fd);
 		if (res < 0) {
 			error("unable to do final sync after deletion: %s",
-- 
2.9.5

[PATCH v2 2/5] btrfs-progs: move get_fsid() to util.c

[Misono, Tomohiro]

Make get_fsid() to a common function.
This will be used for 'subvol delete --commit-after'.

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 cmds-property.c | 30 ------------------------------
 utils.c         | 31 +++++++++++++++++++++++++++++++
 utils.h         |  1 +
 3 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/cmds-property.c b/cmds-property.c
index 9ae1246..03bafa0 100644
--- a/cmds-property.c
+++ b/cmds-property.c
@@ -48,36 +48,6 @@ static int parse_prop(const char *arg, const struct prop_handler *props,
 	return -1;
 }
 
-static int get_fsid(const char *path, u8 *fsid, int silent)
-{
-	int ret;
-	int fd;
-	struct btrfs_ioctl_fs_info_args args;
-
-	fd = open(path, O_RDONLY);
-	if (fd < 0) {
-		ret = -errno;
-		if (!silent)
-			error("failed to open %s: %s", path,
-				strerror(-ret));
-		goto out;
-	}
-
-	ret = ioctl(fd, BTRFS_IOC_FS_INFO, &args);
-	if (ret < 0) {
-		ret = -errno;
-		goto out;
-	}
-
-	memcpy(fsid, args.fsid, BTRFS_FSID_SIZE);
-	ret = 0;
-
-out:
-	if (fd != -1)
-		close(fd);
-	return ret;
-}
-
 static int check_btrfs_object(const char *object)
 {
 	int ret;
diff --git a/utils.c b/utils.c
index 7a2710f..4a5dc60 100644
--- a/utils.c
+++ b/utils.c
@@ -1758,6 +1758,37 @@ out:
 	return ret;
 }
 
+int get_fsid(const char *path, u8 *fsid, int silent)
+{
+	int ret;
+	int fd;
+	struct btrfs_ioctl_fs_info_args args;
+
+	fd = open(path, O_RDONLY);
+	if (fd < 0) {
+		ret = -errno;
+		if (!silent)
+			error("failed to open %s: %s", path,
+				strerror(-ret));
+		goto out;
+	}
+
+	ret = ioctl(fd, BTRFS_IOC_FS_INFO, &args);
+	if (ret < 0) {
+		ret = -errno;
+		goto out;
+	}
+
+	memcpy(fsid, args.fsid, BTRFS_FSID_SIZE);
+	ret = 0;
+
+out:
+	if (fd != -1)
+		close(fd);
+	return ret;
+}
+
+
 static int group_profile_devs_min(u64 flag)
 {
 	switch (flag & BTRFS_BLOCK_GROUP_PROFILE_MASK) {
diff --git a/utils.h b/utils.h
index d28a05a..b3aabe1 100644
--- a/utils.h
+++ b/utils.h
@@ -100,6 +100,7 @@ int open_file_or_dir3(const char *fname, DIR **dirstream, int open_flags);
 void close_file_or_dir(int fd, DIR *dirstream);
 int get_fs_info(const char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 		struct btrfs_ioctl_dev_info_args **di_ret);
+int get_fsid(const char *path, u8 *fsid, int silent);
 int get_label(const char *btrfs_dev, char *label);
 int set_label(const char *btrfs_dev, const char *label);
 
-- 
2.9.5

[PATCH v2 3/5] btrfs-progs: move seen_fsid to util.c

[Misono, Tomohiro]

Make is_seen_fsid()/add_seen_fsid()/free_seen_fsid() to common functions.
This will be used for 'subvol delete --commit-after'.

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 cmds-filesystem.c | 88 ++++---------------------------------------------------
 utils.c           | 70 +++++++++++++++++++++++++++++++++++++++++++
 utils.h           | 11 +++++++
 3 files changed, 86 insertions(+), 83 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 018857c..c7dae40 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -30,7 +30,6 @@
 
 #include "kerncompat.h"
 #include "ctree.h"
-#include "ioctl.h"
 #include "utils.h"
 #include "volumes.h"
 #include "commands.h"
@@ -43,85 +42,8 @@
  * for btrfs fi show, we maintain a hash of fsids we've already printed.
  * This way we don't print dups if a given FS is mounted more than once.
  */
-#define SEEN_FSID_HASH_SIZE 256
-
-struct seen_fsid {
-	u8 fsid[BTRFS_FSID_SIZE];
-	struct seen_fsid *next;
-};
-
 static struct seen_fsid *seen_fsid_hash[SEEN_FSID_HASH_SIZE] = {NULL,};
 
-static int is_seen_fsid(u8 *fsid)
-{
-	u8 hash = fsid[0];
-	int slot = hash % SEEN_FSID_HASH_SIZE;
-	struct seen_fsid *seen = seen_fsid_hash[slot];
-
-	while (seen) {
-		if (memcmp(seen->fsid, fsid, BTRFS_FSID_SIZE) == 0)
-			return 1;
-
-		seen = seen->next;
-	}
-
-	return 0;
-}
-
-static int add_seen_fsid(u8 *fsid)
-{
-	u8 hash = fsid[0];
-	int slot = hash % SEEN_FSID_HASH_SIZE;
-	struct seen_fsid *seen = seen_fsid_hash[slot];
-	struct seen_fsid *alloc;
-
-	if (!seen)
-		goto insert;
-
-	while (1) {
-		if (memcmp(seen->fsid, fsid, BTRFS_FSID_SIZE) == 0)
-			return -EEXIST;
-
-		if (!seen->next)
-			break;
-
-		seen = seen->next;
-	}
-
-insert:
-
-	alloc = malloc(sizeof(*alloc));
-	if (!alloc)
-		return -ENOMEM;
-
-	alloc->next = NULL;
-	memcpy(alloc->fsid, fsid, BTRFS_FSID_SIZE);
-
-	if (seen)
-		seen->next = alloc;
-	else
-		seen_fsid_hash[slot] = alloc;
-
-	return 0;
-}
-
-static void free_seen_fsid(void)
-{
-	int slot;
-	struct seen_fsid *seen;
-	struct seen_fsid *next;
-
-	for (slot = 0; slot < SEEN_FSID_HASH_SIZE; slot++) {
-		seen = seen_fsid_hash[slot];
-		while (seen) {
-			next = seen->next;
-			free(seen);
-			seen = next;
-		}
-		seen_fsid_hash[slot] = NULL;
-	}
-}
-
 static const char * const filesystem_cmd_group_usage[] = {
 	"btrfs filesystem [<group>] <command> [<args>]",
 	NULL
@@ -355,7 +277,7 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices,
 	u64 devs_found = 0;
 	u64 total;
 
-	if (add_seen_fsid(fs_devices->fsid))
+	if (add_seen_fsid(fs_devices->fsid, seen_fsid_hash))
 		return;
 
 	uuid_unparse(fs_devices->fsid, uuidbuf);
@@ -402,7 +324,7 @@ static int print_one_fs(struct btrfs_ioctl_fs_info_args *fs_info,
 	struct btrfs_ioctl_dev_info_args *tmp_dev_info;
 	int ret;
 
-	ret = add_seen_fsid(fs_info->fsid);
+	ret = add_seen_fsid(fs_info->fsid, seen_fsid_hash);
 	if (ret == -EEXIST)
 		return 0;
 	else if (ret)
@@ -474,7 +396,7 @@ static int btrfs_scan_kernel(void *search, unsigned unit_mode)
 			goto out;
 
 		/* skip all fs already shown as mounted fs */
-		if (is_seen_fsid(fs_info_arg.fsid))
+		if (is_seen_fsid(fs_info_arg.fsid, seen_fsid_hash))
 			continue;
 
 		ret = get_label_mounted(mnt->mnt_dir, label);
@@ -676,7 +598,7 @@ static int search_umounted_fs_uuids(struct list_head *all_uuids,
 		}
 
 		/* skip all fs already shown as mounted fs */
-		if (is_seen_fsid(cur_fs->fsid))
+		if (is_seen_fsid(cur_fs->fsid, seen_fsid_hash))
 			continue;
 
 		fs_copy = calloc(1, sizeof(*fs_copy));
@@ -908,7 +830,7 @@ devs_only:
 		free_fs_devices(fs_devices);
 	}
 out:
-	free_seen_fsid();
+	free_seen_fsid(seen_fsid_hash);
 	return ret;
 }
 
diff --git a/utils.c b/utils.c
index 4a5dc60..f91d41e 100644
--- a/utils.c
+++ b/utils.c
@@ -1788,6 +1788,76 @@ out:
 	return ret;
 }
 
+int is_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[])
+{
+	u8 hash = fsid[0];
+	int slot = hash % SEEN_FSID_HASH_SIZE;
+	struct seen_fsid *seen = seen_fsid_hash[slot];
+
+	while (seen) {
+		if (memcmp(seen->fsid, fsid, BTRFS_FSID_SIZE) == 0)
+			return 1;
+
+		seen = seen->next;
+	}
+
+	return 0;
+}
+
+int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[])
+{
+	u8 hash = fsid[0];
+	int slot = hash % SEEN_FSID_HASH_SIZE;
+	struct seen_fsid *seen = seen_fsid_hash[slot];
+	struct seen_fsid *alloc;
+
+	if (!seen)
+		goto insert;
+
+	while (1) {
+		if (memcmp(seen->fsid, fsid, BTRFS_FSID_SIZE) == 0)
+			return -EEXIST;
+
+		if (!seen->next)
+			break;
+
+		seen = seen->next;
+	}
+
+insert:
+
+	alloc = malloc(sizeof(*alloc));
+	if (!alloc)
+		return -ENOMEM;
+
+	alloc->next = NULL;
+	memcpy(alloc->fsid, fsid, BTRFS_FSID_SIZE);
+
+	if (seen)
+		seen->next = alloc;
+	else
+		seen_fsid_hash[slot] = alloc;
+
+	return 0;
+}
+
+void free_seen_fsid(struct seen_fsid *seen_fsid_hash[])
+{
+	int slot;
+	struct seen_fsid *seen;
+	struct seen_fsid *next;
+
+	for (slot = 0; slot < SEEN_FSID_HASH_SIZE; slot++) {
+		seen = seen_fsid_hash[slot];
+		while (seen) {
+			next = seen->next;
+			free(seen);
+			seen = next;
+		}
+		seen_fsid_hash[slot] = NULL;
+	}
+}
+
 
 static int group_profile_devs_min(u64 flag)
 {
diff --git a/utils.h b/utils.h
index b3aabe1..da34e6c 100644
--- a/utils.h
+++ b/utils.h
@@ -28,6 +28,7 @@
 #include "btrfs-list.h"
 #include "sizes.h"
 #include "messages.h"
+#include "ioctl.h"
 
 #define BTRFS_SCAN_MOUNTED	(1ULL << 0)
 #define BTRFS_SCAN_LBLKID	(1ULL << 1)
@@ -101,6 +102,16 @@ void close_file_or_dir(int fd, DIR *dirstream);
 int get_fs_info(const char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 		struct btrfs_ioctl_dev_info_args **di_ret);
 int get_fsid(const char *path, u8 *fsid, int silent);
+
+#define SEEN_FSID_HASH_SIZE 256
+struct seen_fsid {
+	u8 fsid[BTRFS_FSID_SIZE];
+	struct seen_fsid *next;
+};
+int is_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[]);
+int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[]);
+void free_seen_fsid(struct seen_fsid *seen_fsid_hash[]);
+
 int get_label(const char *btrfs_dev, char *label);
 int set_label(const char *btrfs_dev, const char *label);
 
-- 
2.9.5

[PATCH v2 4/5] btrfs-progs: change seen_fsid to hold fd and DIR*

[Misono, Tomohiro]

Change seen_fsid to hold fd and DIR* in order to keep access to each fs.
This will be used for 'subvol delete --commit-after'.

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 cmds-filesystem.c | 4 ++--
 utils.c           | 6 +++++-
 utils.h           | 5 ++++-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index c7dae40..4bbff43 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -277,7 +277,7 @@ static void print_one_uuid(struct btrfs_fs_devices *fs_devices,
 	u64 devs_found = 0;
 	u64 total;
 
-	if (add_seen_fsid(fs_devices->fsid, seen_fsid_hash))
+	if (add_seen_fsid(fs_devices->fsid, seen_fsid_hash, -1, NULL))
 		return;
 
 	uuid_unparse(fs_devices->fsid, uuidbuf);
@@ -324,7 +324,7 @@ static int print_one_fs(struct btrfs_ioctl_fs_info_args *fs_info,
 	struct btrfs_ioctl_dev_info_args *tmp_dev_info;
 	int ret;
 
-	ret = add_seen_fsid(fs_info->fsid, seen_fsid_hash);
+	ret = add_seen_fsid(fs_info->fsid, seen_fsid_hash, -1, NULL);
 	if (ret == -EEXIST)
 		return 0;
 	else if (ret)
diff --git a/utils.c b/utils.c
index f91d41e..bdfbfe0 100644
--- a/utils.c
+++ b/utils.c
@@ -1804,7 +1804,8 @@ int is_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[])
 	return 0;
 }
 
-int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[])
+int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[],
+		int fd, DIR *dirstream)
 {
 	u8 hash = fsid[0];
 	int slot = hash % SEEN_FSID_HASH_SIZE;
@@ -1832,6 +1833,8 @@ insert:
 
 	alloc->next = NULL;
 	memcpy(alloc->fsid, fsid, BTRFS_FSID_SIZE);
+	alloc->fd = fd;
+	alloc->dirstream = dirstream;
 
 	if (seen)
 		seen->next = alloc;
@@ -1851,6 +1854,7 @@ void free_seen_fsid(struct seen_fsid *seen_fsid_hash[])
 		seen = seen_fsid_hash[slot];
 		while (seen) {
 			next = seen->next;
+			close_file_or_dir(seen->fd, seen->dirstream);
 			free(seen);
 			seen = next;
 		}
diff --git a/utils.h b/utils.h
index da34e6c..bac7688 100644
--- a/utils.h
+++ b/utils.h
@@ -107,9 +107,12 @@ int get_fsid(const char *path, u8 *fsid, int silent);
 struct seen_fsid {
 	u8 fsid[BTRFS_FSID_SIZE];
 	struct seen_fsid *next;
+	DIR *dirstream;
+	int fd;
 };
 int is_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[]);
-int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[]);
+int add_seen_fsid(u8 *fsid, struct seen_fsid *seen_fsid_hash[],
+		int fd, DIR *dirstream);
 void free_seen_fsid(struct seen_fsid *seen_fsid_hash[]);
 
 int get_label(const char *btrfs_dev, char *label);
-- 
2.9.5

[PATCH v2 5/5] btrfs-progs: subvol: fix subvol del --commit-after

[Misono, Tomohiro]

Fix subvol del --commit-after to work properly:
 - SYNC ioctl will be issued even when last delete is failed
 - SYNC ioctl will be issued on each file system only once in the end

To achieve this, get_fsid() and add_seen_fsid() is called after each delete
to keep only one fd for each fs.

In the end, seen_fsid_hash will be traversed and SYNC is issued on each fs.

Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 cmds-subvolume.c | 61 ++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 13 deletions(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 9e561f3..0c8a75f 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -263,6 +263,9 @@ static int cmd_subvol_delete(int argc, char **argv)
 	DIR	*dirstream = NULL;
 	int verbose = 0;
 	int commit_mode = 0;
+	u8 fsid[BTRFS_FSID_SIZE];
+	char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
+	struct seen_fsid *seen_fsid_hash[SEEN_FSID_HASH_SIZE] = {NULL,};
 	enum { COMMIT_AFTER = 1, COMMIT_EACH = 2 };
 
 	while (1) {
@@ -358,31 +361,63 @@ again:
 				path, strerror(errno));
 			ret = 1;
 		}
+	} else if (commit_mode == COMMIT_AFTER) {
+		res = get_fsid(dname, fsid, 0);
+		if (res < 0) {
+			error("unable to get fsid for '%s': %s", path, strerror(-res));
+			error("delete suceeded but commit may not be done in the end");
+			ret = 1;
+			goto out;
+		}
+
+		if (add_seen_fsid(fsid, seen_fsid_hash, fd, dirstream) == 0) {
+			if (verbose > 0) {
+				uuid_unparse(fsid, uuidbuf);
+				printf("  new fs is found for '%s', fsid: %s\n", path, uuidbuf);
+			}
+			// this is the first time a subvolume on this filesystem is deleted
+			// keep fd in order to issue SYNC ioctl in the end
+			goto keep_fd;
+		}
 	}
 
 out:
+	close_file_or_dir(fd, dirstream);
+keep_fd:
+	fd = -1;
+	dirstream = NULL;
 	free(dupdname);
 	free(dupvname);
 	dupdname = NULL;
 	dupvname = NULL;
 	cnt++;
-	if (cnt < argc) {
-		close_file_or_dir(fd, dirstream);
-		/* avoid double free */
-		fd = -1;
-		dirstream = NULL;
+	if (cnt < argc)
 		goto again;
-	}
 
-	if (commit_mode == COMMIT_AFTER && fd != -1) {
-		res = wait_for_commit(fd);
-		if (res < 0) {
-			error("unable to do final sync after deletion: %s",
-				strerror(errno));
-			ret = 1;
+	if (commit_mode == COMMIT_AFTER) {
+		// traverse seen_fsid_hash and issue SYNC ioctl on each filesystem
+		int slot;
+		struct seen_fsid *seen;
+
+		for (slot = 0; slot < SEEN_FSID_HASH_SIZE; slot++) {
+			seen = seen_fsid_hash[slot];
+			while (seen) {
+				res = wait_for_commit(seen->fd);
+				if (res < 0) {
+					uuid_unparse(seen->fsid, uuidbuf);
+					error("unable to do final sync after deletion: %s, fsid: %s",
+						strerror(errno), uuidbuf);
+					ret = 1;
+				} else if (verbose > 0) {
+					uuid_unparse(seen->fsid, uuidbuf);
+					printf("final sync is done for fsid: %s\n", uuidbuf);
+				}
+				seen = seen->next;
+			}
 		}
+		// fd will also be closed in free_seen_fsid
+		free_seen_fsid(seen_fsid_hash);
 	}
-	close_file_or_dir(fd, dirstream);
 
 	return ret;
 }
-- 
2.9.5

Re: [PATCH v2 5/5] btrfs-progs: subvol: fix subvol del --commit-after

[David Sterba]

On Wed, Sep 27, 2017 at 11:03:48AM +0900, Misono, Tomohiro wrote:
> Fix subvol del --commit-after to work properly:
>  - SYNC ioctl will be issued even when last delete is failed
>  - SYNC ioctl will be issued on each file system only once in the end
> 
> To achieve this, get_fsid() and add_seen_fsid() is called after each delete
> to keep only one fd for each fs.
> 
> In the end, seen_fsid_hash will be traversed and SYNC is issued on each fs.
> 
> Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.com>
> Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>

Review comments from me in a form of diff, coding style issues:

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 00931a55807e..69c50387a984 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -265,7 +265,7 @@ static int cmd_subvol_delete(int argc, char **argv)
 	int commit_mode = 0;
 	u8 fsid[BTRFS_FSID_SIZE];
 	char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
-	struct seen_fsid *seen_fsid_hash[SEEN_FSID_HASH_SIZE] = {NULL,};
+	struct seen_fsid *seen_fsid_hash[SEEN_FSID_HASH_SIZE] = { NULL, };
 	enum { COMMIT_AFTER = 1, COMMIT_EACH = 2 };
 
 	while (1) {
@@ -364,8 +364,10 @@ static int cmd_subvol_delete(int argc, char **argv)
 	} else if (commit_mode == COMMIT_AFTER) {
 		res = get_fsid(dname, fsid, 0);
 		if (res < 0) {
-			error("unable to get fsid for '%s': %s", path, strerror(-res));
-			error("delete suceeded but commit may not be done in the end");
+			error("unable to get fsid for '%s': %s",
+				path, strerror(-res));
+			error(
+			"delete suceeded but commit may not be done in the end");
 			ret = 1;
 			goto out;
 		}
@@ -373,10 +375,14 @@ static int cmd_subvol_delete(int argc, char **argv)
 		if (add_seen_fsid(fsid, seen_fsid_hash, fd, dirstream) == 0) {
 			if (verbose > 0) {
 				uuid_unparse(fsid, uuidbuf);
-				printf("  new fs is found for '%s', fsid: %s\n", path, uuidbuf);
+				printf("  new fs is found for '%s', fsid: %s\n",
+						path, uuidbuf);
 			}
-			// this is the first time a subvolume on this filesystem is deleted
-			// keep fd in order to issue SYNC ioctl in the end
+			/*
+			 * This is the first time a subvolume on this
+			 * filesystem is deleted, keep fd in order to issue
+			 * SYNC ioctl in the end
+			 */
 			goto keep_fd;
 		}
 	}
@@ -395,27 +401,32 @@ static int cmd_subvol_delete(int argc, char **argv)
 		goto again;
 
 	if (commit_mode == COMMIT_AFTER) {
-		// traverse seen_fsid_hash and issue SYNC ioctl on each filesystem
 		int slot;
-		struct seen_fsid *seen;
 
+		/*
+		 * Traverse seen_fsid_hash and issue SYNC ioctl on each
+		 * filesystem
+		 */
 		for (slot = 0; slot < SEEN_FSID_HASH_SIZE; slot++) {
-			seen = seen_fsid_hash[slot];
+			struct seen_fsid *seen = seen_fsid_hash[slot];
+
 			while (seen) {
 				res = wait_for_commit(seen->fd);
 				if (res < 0) {
 					uuid_unparse(seen->fsid, uuidbuf);
-					error("unable to do final sync after deletion: %s, fsid: %s",
+					error(
+			"unable to do final sync after deletion: %s, fsid: %s",
 						strerror(errno), uuidbuf);
 					ret = 1;
 				} else if (verbose > 0) {
 					uuid_unparse(seen->fsid, uuidbuf);
-					printf("final sync is done for fsid: %s\n", uuidbuf);
+					printf("final sync is done for fsid: %s\n",
+						uuidbuf);
 				}
 				seen = seen->next;
 			}
 		}
-		// fd will also be closed in free_seen_fsid
+		/* fd will also be closed in free_seen_fsid */
 		free_seen_fsid(seen_fsid_hash);
 	}

Re: [PATCH v2 0/5] btrfs-progs: subvol: fix del --commit-after

[David Sterba]

On Wed, Sep 27, 2017 at 11:00:35AM +0900, Misono, Tomohiro wrote:
> Fix subvol del --commit-after to work properly:
>  - SYNC ioctl will be issued even when last delete is failed
>  - SYNC ioctl will be issued on each file system only once in the end
> 
> To achieve this, each deleted subvol's (parent's) fsid is checked each
> time. If the fsid is seen for the first time, its fd will be kept in order
> to issue SYNC ioctl in the end.
> 
> There already exists get_fsid() in cmds-property.c and seen_fsid which
> keeps fsid in hush function in cmds-filesystem.c. This patch utilizes
> them.
> 
> First patch is the independent but critical. Current code is reversed in
> --commit-after and --commit-each operation. i.e. --commit-after means
> --commit-each actually. The patch fix this.
> 
> 2rd to 4th patches make functions stated above to common and last one is
> the main part.

Thanks for the fix, I'm glad we did not have to remove the option.
Patches applied with some adjustments. Please became more familiar with
the coding style. I'm usually fixing all patches here and there, but
once the amount grows I'd rather spend time on more useful things.

Bothering with coding style is annoying but the point is that the
committed code looks exactly the same no matter when or who authored it.

> Thanks Qu for reviewing whole patches.

Appreciated.