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

[PATCH 0/4] 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. First three patches
make them to common functions and last one is the main part.

Tomohiro Misono (4):
  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  |  77 ++++++++++++++++++++++++++++-----------
 utils.c           | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 utils.h           |  15 ++++++++
 5 files changed, 181 insertions(+), 134 deletions(-)

-- 
2.9.5

[PATCH 1/4] 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>
---
 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 2/4] 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>
---
 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 3/4] 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>
---
 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;
+	int fd;
+	DIR *dirstream;
 };
 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

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

[Qu Wenruo]

On 2017年09月26日 13:45, Misono, Tomohiro wrote:
> 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'.

It is already quite good, good enough for the fix.

However just a small point can be further enhanced, commended below.

> 
> Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.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;
> +	int fd;

Will it be possible that the final fd recorded here is invalid or some 
other reason that we failed to execute SYNC ioctl on that fd, but can 
succeeded with other fd?

In that case, a list of fd will help.

Thanks,
Qu

> +	DIR *dirstream;
>   };
>   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);
> 

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

[Misono, Tomohiro]

On 2017/09/26 22:08, Qu Wenruo wrote:
> 
> 
> On 2017年09月26日 13:45, Misono, Tomohiro wrote:
>> 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'.
> 
> It is already quite good, good enough for the fix.
> 
> However just a small point can be further enhanced, commended below.
> 
>>
>> Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.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;
>> +	int fd;
> 
> Will it be possible that the final fd recorded here is invalid or some 
> other reason that we failed to execute SYNC ioctl on that fd, but can 
> succeeded with other fd?
> 
> In that case, a list of fd will help.
> 
> Thanks,
> Qu
> 
Hello,

I think fd will not be invalidated unless user does because open is
succeeded. Also, if SYNC is failed for one fd, it would fail for other fds
too. So, I think there is no need to keep several fds. What do you think?

By the way, thanks for reviewing whole patches and comments.
I will splits the cleanup for the fourth patch.

Regards,
Tomohiro

>> +	DIR *dirstream;
>>   };
>>   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);
>>
> 
> 

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

[Qu Wenruo]

On 2017年09月27日 08:42, Misono, Tomohiro wrote:
> On 2017/09/26 22:08, Qu Wenruo wrote:
>>
>>
>> On 2017年09月26日 13:45, Misono, Tomohiro wrote:
>>> 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'.
>>
>> It is already quite good, good enough for the fix.
>>
>> However just a small point can be further enhanced, commended below.
>>
>>>
>>> Signed-off-by: Tomohiro Misono <misono.tomohiro@jp.fujitsu.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;
>>> +	int fd;
>>
>> Will it be possible that the final fd recorded here is invalid or some
>> other reason that we failed to execute SYNC ioctl on that fd, but can
>> succeeded with other fd?
>>
>> In that case, a list of fd will help.
>>
>> Thanks,
>> Qu
>>
> Hello,
> 
> I think fd will not be invalidated unless user does because open is
> succeeded. Also, if SYNC is failed for one fd, it would fail for other fds
> too. So, I think there is no need to keep several fds. What do you think?

Makes sense.

So I'm OK using current method.
Feel free to add my reviewed-by tag.

Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>

Thanks,
Qu

> 
> By the way, thanks for reviewing whole patches and comments.
> I will splits the cleanup for the fourth patch.
> 
> Regards,
> Tomohiro
> 
>>> +	DIR *dirstream;
>>>    };
>>>    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);
>>>
>>
>>
> 
> --
> 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
> 

[PATCH 4/4] 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>
---
 cmds-subvolume.c | 77 ++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 56 insertions(+), 21 deletions(-)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 666f6e0..bcbd737 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -263,6 +263,10 @@ 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) {
 		int c;
@@ -279,10 +283,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 +302,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,50 +342,81 @@ again:
 	}
 
 	printf("Delete subvolume (%s): '%s/%s'\n",
-		commit_mode == 2 || (commit_mode == 1 && cnt + 1 == argc)
-		? "commit" : "no-commit", dname, vname);
+		commit_mode == COMMIT_EACH ? "commit" : "no-commit", dname, vname);
 	memset(&args, 0, sizeof(args));
 	strncpy_null(args.name, vname);
 	res = ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &args);
-	if(res < 0 ){
+	if (res < 0) {
 		error("cannot delete '%s/%s': %s", dname, vname,
 			strerror(errno));
 		ret = 1;
 		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",
+			error("unable to wait for commit after deleting '%s': %s",
 				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 == 2 && 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 4/4] btrfs-progs: subvol: fix subvol del --commit-after

[Qu Wenruo]

On 2017年09月26日 13:46, 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>
> ---
>   cmds-subvolume.c | 77 ++++++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 56 insertions(+), 21 deletions(-)
> 
> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> index 666f6e0..bcbd737 100644
> --- a/cmds-subvolume.c
> +++ b/cmds-subvolume.c
> @@ -263,6 +263,10 @@ 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) {
>   		int c;
> @@ -279,10 +283,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;

The commit_mode cleanup can be a separate patch.
(More patches always look cooler)

Other part looks good enough.
Feel free to add my reviewed by tags after splitting the cleanup patch.

Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>

Thanks,
Qu
>   			break;
>   		case 'v':
>   			verbose++;
> @@ -298,7 +302,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,50 +342,81 @@ again:
>   	}
>   
>   	printf("Delete subvolume (%s): '%s/%s'\n",
> -		commit_mode == 2 || (commit_mode == 1 && cnt + 1 == argc)
> -		? "commit" : "no-commit", dname, vname);
> +		commit_mode == COMMIT_EACH ? "commit" : "no-commit", dname, vname);
>   	memset(&args, 0, sizeof(args));
>   	strncpy_null(args.name, vname);
>   	res = ioctl(fd, BTRFS_IOC_SNAP_DESTROY, &args);
> -	if(res < 0 ){
> +	if (res < 0) {
>   		error("cannot delete '%s/%s': %s", dname, vname,
>   			strerror(errno));
>   		ret = 1;
>   		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",
> +			error("unable to wait for commit after deleting '%s': %s",
>   				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 == 2 && 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;
>   }
> 

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

[Qu Wenruo]

On 2017年09月26日 13:44, 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. First three patches
> make them to common functions and last one is the main part.

Much more elegant than my expectation.
Clean and short fix.

Only small suggestion to use fd list other than using the last valid fd 
(for patch 3), and a possible patch split (for patch 4).

Looks good overall.
You can add my reviewed-by tag after splitting cleanup patch.

Good job.

Thanks,
Qu
> 
> Tomohiro Misono (4):
>    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  |  77 ++++++++++++++++++++++++++++-----------
>   utils.c           | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   utils.h           |  15 ++++++++
>   5 files changed, 181 insertions(+), 134 deletions(-)
>