diff mbox series

[4/4] btrfs-progs: Enqueue command if it can't be performed immediately

Message ID 20200825150338.32610-4-rgoldwyn@suse.de (mailing list archive)
State New, archived
Headers show
Series [1/4] btrfs-progs: get_fsid_fd() for getting fsid using fd | expand

Commit Message

Goldwyn Rodrigues Aug. 25, 2020, 3:03 p.m. UTC 
From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Wait for the current exclusive operation to finish before issuing the
command ioctl, so we have a better chance of success.

Q: The resize argument parsing is hackish. Is there a better way to do
this?

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 cmds/device.c     | 56 +++++++++++++++++++++++++++++++++++++++--------
 cmds/filesystem.c | 25 +++++++++++++++++----
 common/sysfs.c    | 26 ++++++++++++++++++++++
 common/utils.h    |  1 +
 4 files changed, 95 insertions(+), 13 deletions(-)

Comments

David Sterba Sept. 2, 2020, 2:11 p.m. UTC | #1 
On Tue, Aug 25, 2020 at 10:03:38AM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Wait for the current exclusive operation to finish before issuing the
> command ioctl, so we have a better chance of success.
> 
> Q: The resize argument parsing is hackish. Is there a better way to do
> this?

You mean parsing in kernel? Progs pass the 1st non-option parameter
without changes, so if you add a new option, the -- separator needs to
be used to make sure the relative size update (eg. -1G) is properly
recognized. This is built in already and should not require anything
special on the option parsing side.

> --- a/cmds/device.c
> +++ b/cmds/device.c
> @@ -49,6 +49,7 @@ static const char * const cmd_device_add_usage[] = {
>  	"",
>  	"-K|--nodiscard    do not perform whole device TRIM on devices that report such capability",
>  	"-f|--force        force overwrite existing filesystem on the disk",
> +	"-q|--enqueue	   enqueue if an exclusive operation is running",

Short for -q should not be used due to confusion with --quiet. Also I
think that --enqueue is not a common action that would need a short
option, the long option is always safe.

> --- a/common/sysfs.c
> +++ b/common/sysfs.c
> @@ -50,3 +50,29 @@ int get_exclusive_operation(int mp_fd, char *val)
>  	val[n - 1] = '\0';
>  	return n;
>  }
> +
> +int sysfs_wait(int fd, int seconds)
> +{
> +	fd_set fds;
> +	struct timeval tv;
> +
> +	FD_ZERO(&fds);
> +	FD_SET(fd, &fds);
> +
> +	tv.tv_sec = seconds;
> +	tv.tv_usec = 0;
> +
> +	return select(fd+1, NULL, NULL, &fds, &tv);

With the short sleep times, do we need to wait using select? Yes this
would return once the notification event is sent but as the sleep time
is 1 second, it could simply be sleep(1) unconditionally.

> +}
> +
> +void wait_for_exclusive_operation(int dirfd)
> +{
> +        char exop[BTRFS_SYSFS_EXOP_SIZE];
> +	int fd;
> +
> +        fd = sysfs_open(dirfd, "exclusive_operation");
> +        while ((sysfs_get_str_fd(fd, exop, BTRFS_SYSFS_EXOP_SIZE) > 0) &&
> +		strncmp(exop, "none", 4))
> +			sysfs_wait(fd, 1);
> +	close(fd);
> +}
> diff --git a/common/utils.h b/common/utils.h
> index be8aab58..f141edb6 100644
> --- a/common/utils.h
> +++ b/common/utils.h
> @@ -155,5 +155,6 @@ int btrfs_warn_multiple_profiles(int fd);
>  
>  #define BTRFS_SYSFS_EXOP_SIZE		16
>  int get_exclusive_operation(int fd, char *val);
> +void wait_for_exclusive_operation(int fd);
>  
>  #endif
> -- 
> 2.26.2
Goldwyn Rodrigues Sept. 2, 2020, 8:45 p.m. UTC | #2 
On 16:11 02/09, David Sterba wrote:
> On Tue, Aug 25, 2020 at 10:03:38AM -0500, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > Wait for the current exclusive operation to finish before issuing the
> > command ioctl, so we have a better chance of success.
> > 
> > Q: The resize argument parsing is hackish. Is there a better way to do
> > this?
> 
> You mean parsing in kernel? Progs pass the 1st non-option parameter
> without changes, so if you add a new option, the -- separator needs to
> be used to make sure the relative size update (eg. -1G) is properly
> recognized. This is built in already and should not require anything
> special on the option parsing side.

Particularly the example you mention, -1G. You answered it as well :)

> 
> > --- a/cmds/device.c
> > +++ b/cmds/device.c
> > @@ -49,6 +49,7 @@ static const char * const cmd_device_add_usage[] = {
> >  	"",
> >  	"-K|--nodiscard    do not perform whole device TRIM on devices that report such capability",
> >  	"-f|--force        force overwrite existing filesystem on the disk",
> > +	"-q|--enqueue	   enqueue if an exclusive operation is running",
> 
> Short for -q should not be used due to confusion with --quiet. Also I
> think that --enqueue is not a common action that would need a short
> option, the long option is always safe.

Yes, and --quit as well. However, just --enqueue is fine.

> 
> > --- a/common/sysfs.c
> > +++ b/common/sysfs.c
> > @@ -50,3 +50,29 @@ int get_exclusive_operation(int mp_fd, char *val)
> >  	val[n - 1] = '\0';
> >  	return n;
> >  }
> > +
> > +int sysfs_wait(int fd, int seconds)
> > +{
> > +	fd_set fds;
> > +	struct timeval tv;
> > +
> > +	FD_ZERO(&fds);
> > +	FD_SET(fd, &fds);
> > +
> > +	tv.tv_sec = seconds;
> > +	tv.tv_usec = 0;
> > +
> > +	return select(fd+1, NULL, NULL, &fds, &tv);
> 
> With the short sleep times, do we need to wait using select? Yes this
> would return once the notification event is sent but as the sleep time
> is 1 second, it could simply be sleep(1) unconditionally.

Yes, the kernel provides the sysfs notification. We could sys_wait() for
longer (with a higher wait parameter) and yet the function would return
the moment the kernel notifies the change. I know we are not using it
now, but it is better to use robust interfaces when a event notification
is provided by the kernel.

> 
> > +}
> > +
> > +void wait_for_exclusive_operation(int dirfd)
> > +{
> > +        char exop[BTRFS_SYSFS_EXOP_SIZE];
> > +	int fd;
> > +
> > +        fd = sysfs_open(dirfd, "exclusive_operation");
> > +        while ((sysfs_get_str_fd(fd, exop, BTRFS_SYSFS_EXOP_SIZE) > 0) &&
> > +		strncmp(exop, "none", 4))
> > +			sysfs_wait(fd, 1);
> > +	close(fd);
> > +}
> > diff --git a/common/utils.h b/common/utils.h
> > index be8aab58..f141edb6 100644
> > --- a/common/utils.h
> > +++ b/common/utils.h
> > @@ -155,5 +155,6 @@ int btrfs_warn_multiple_profiles(int fd);
> >  
> >  #define BTRFS_SYSFS_EXOP_SIZE		16
> >  int get_exclusive_operation(int fd, char *val);
> > +void wait_for_exclusive_operation(int fd);
> >  
> >  #endif
> > -- 
> > 2.26.2
diff mbox series

Patch

diff --git a/cmds/device.c b/cmds/device.c
index 6acd4ae6..12d92dac 100644
--- a/cmds/device.c
+++ b/cmds/device.c
@@ -49,6 +49,7 @@  static const char * const cmd_device_add_usage[] = {
 	"",
 	"-K|--nodiscard    do not perform whole device TRIM on devices that report such capability",
 	"-f|--force        force overwrite existing filesystem on the disk",
+	"-q|--enqueue	   enqueue if an exclusive operation is running",
 	NULL
 };
 
@@ -62,6 +63,7 @@  static int cmd_device_add(const struct cmd_struct *cmd,
 	int force = 0;
 	int last_dev;
 	char exop[BTRFS_SYSFS_EXOP_SIZE];
+	bool enqueue = false;
 
 	optind = 0;
 	while (1) {
@@ -69,10 +71,11 @@  static int cmd_device_add(const struct cmd_struct *cmd,
 		static const struct option long_options[] = {
 			{ "nodiscard", optional_argument, NULL, 'K'},
 			{ "force", no_argument, NULL, 'f'},
+			{ "enqueue", no_argument, NULL, 'q'},
 			{ NULL, 0, NULL, 0}
 		};
 
-		c = getopt_long(argc, argv, "Kf", long_options, NULL);
+		c = getopt_long(argc, argv, "Kfq", long_options, NULL);
 		if (c < 0)
 			break;
 		switch (c) {
@@ -82,6 +85,9 @@  static int cmd_device_add(const struct cmd_struct *cmd,
 		case 'f':
 			force = 1;
 			break;
+		case 'q':
+			enqueue = true;
+			break;
 		default:
 			usage_unknown_option(cmd, argv);
 		}
@@ -98,9 +104,15 @@  static int cmd_device_add(const struct cmd_struct *cmd,
 		return 1;
 
 	if (get_exclusive_operation(fdmnt, exop) > 0 && strcmp(exop, "none")) {
-		error("unable to add device: %s in progress", exop);
-		close_file_or_dir(fdmnt, dirstream);
-		return 1;
+		if (enqueue) {
+			printf("%s in progress. Waiting for %s to finish.\n",
+					exop, exop);
+			wait_for_exclusive_operation(fdmnt);
+		} else {
+			error("unable to add: %s in progress", exop);
+			close_file_or_dir(fdmnt, dirstream);
+			return 1;
+		}
 	}
 
 	for (i = optind; i < last_dev; i++){
@@ -163,8 +175,27 @@  static int _cmd_device_remove(const struct cmd_struct *cmd,
 	int i, fdmnt, ret = 0;
 	DIR	*dirstream = NULL;
 	char exop[BTRFS_SYSFS_EXOP_SIZE];
+	bool enqueue = false;
 
-	clean_args_no_options(cmd, argc, argv);
+
+	while (1) {
+		int c;
+		static const struct option long_options[] = {
+			{ "enqueue", no_argument, NULL, 'q'},
+			{ NULL, 0, NULL, 0}
+		};
+
+		c = getopt_long(argc, argv, "q", long_options, NULL);
+		if (c < 0)
+			break;
+		switch (c) {
+		case 'q':
+			enqueue = true;
+			break;
+		default:
+			usage_unknown_option(cmd, argv);
+		}
+	}
 
 	if (check_argc_min(argc - optind, 2))
 		return 1;
@@ -176,9 +207,15 @@  static int _cmd_device_remove(const struct cmd_struct *cmd,
 		return 1;
 
 	if (get_exclusive_operation(fdmnt, exop) > 0 && strcmp(exop, "none")) {
-		error("unable to remove device: %s in progress", exop);
-		close_file_or_dir(fdmnt, dirstream);
-		return 1;
+		if (enqueue) {
+			printf("%s in progress. Waiting for %s to finish.\n",
+					exop, exop);
+			wait_for_exclusive_operation(fdmnt);
+		} else {
+			error("unable to remove device: %s in progress", exop);
+			close_file_or_dir(fdmnt, dirstream);
+			return 1;
+		}
 	}
 
 	for(i = optind; i < argc - 1; i++) {
@@ -251,7 +288,8 @@  static int _cmd_device_remove(const struct cmd_struct *cmd,
 	"the device.",								\
 	"If 'missing' is specified for <device>, the first device that is",	\
 	"described by the filesystem metadata, but not present at the mount",	\
-	"time will be removed. (only in degraded mode)"
+	"time will be removed. (only in degraded mode)", \
+	"-q|--enqueue	   enqueue if an exclusive operation is running"
 
 static const char * const cmd_device_remove_usage[] = {
 	"btrfs device remove <device>|<devid> [<device>|<devid>...] <path>",
diff --git a/cmds/filesystem.c b/cmds/filesystem.c
index c3efb405..a584b1d3 100644
--- a/cmds/filesystem.c
+++ b/cmds/filesystem.c
@@ -1080,8 +1080,19 @@  static int cmd_filesystem_resize(const struct cmd_struct *cmd,
 	DIR	*dirstream = NULL;
 	struct stat st;
 	char exop[BTRFS_SYSFS_EXOP_SIZE];
+	bool enqueue = false;
 
-	clean_args_no_options_relaxed(cmd, argc, argv);
+	while(1) {
+		int c = getopt(argc - 2, argv, "q");
+		if (c < 0)
+			break;
+
+		switch(c) {
+		case 'q':
+			enqueue = true;
+			break;
+		}
+	}
 
 	if (check_argc_exact(argc - optind, 2))
 		return 1;
@@ -1112,9 +1123,15 @@  static int cmd_filesystem_resize(const struct cmd_struct *cmd,
 		return 1;
 
 	if (get_exclusive_operation(fd, exop) > 0 && strcmp(exop, "none")) {
-		error("unable to resize: %s in progress", exop);
-		close_file_or_dir(fd, dirstream);
-		return 1;
+		if (enqueue) {
+			printf("%s in progress. Waiting for %s to finish.\n",
+					exop, exop);
+			wait_for_exclusive_operation(fd);
+		} else {
+			error("unable to resize: %s in progress", exop);
+			close_file_or_dir(fd, dirstream);
+			return 1;
+		}
 	}
 
 	printf("Resize '%s' of '%s'\n", path, amount);
diff --git a/common/sysfs.c b/common/sysfs.c
index b2c95cfb..3b6df52e 100644
--- a/common/sysfs.c
+++ b/common/sysfs.c
@@ -50,3 +50,29 @@  int get_exclusive_operation(int mp_fd, char *val)
 	val[n - 1] = '\0';
 	return n;
 }
+
+int sysfs_wait(int fd, int seconds)
+{
+	fd_set fds;
+	struct timeval tv;
+
+	FD_ZERO(&fds);
+	FD_SET(fd, &fds);
+
+	tv.tv_sec = seconds;
+	tv.tv_usec = 0;
+
+	return select(fd+1, NULL, NULL, &fds, &tv);
+}
+
+void wait_for_exclusive_operation(int dirfd)
+{
+        char exop[BTRFS_SYSFS_EXOP_SIZE];
+	int fd;
+
+        fd = sysfs_open(dirfd, "exclusive_operation");
+        while ((sysfs_get_str_fd(fd, exop, BTRFS_SYSFS_EXOP_SIZE) > 0) &&
+		strncmp(exop, "none", 4))
+			sysfs_wait(fd, 1);
+	close(fd);
+}
diff --git a/common/utils.h b/common/utils.h
index be8aab58..f141edb6 100644
--- a/common/utils.h
+++ b/common/utils.h
@@ -155,5 +155,6 @@  int btrfs_warn_multiple_profiles(int fd);
 
 #define BTRFS_SYSFS_EXOP_SIZE		16
 int get_exclusive_operation(int fd, char *val);
+void wait_for_exclusive_operation(int fd);
 
 #endif