[PATCH] btrfs: introduce BTRFS_IOC_CHECK_DEV_EXCL_OPS ioctl to check dev excl op

[PATCH] btrfs: introduce BTRFS_IOC_CHECK_DEV_EXCL_OPS ioctl to check dev excl op

[Anand Jain]

This patch provides an ioctl to check if the FS is performing
any device exclusive operations like device add remove balance
etc. Basically any operation which will set
fs_info->mutually_exclusive_operation_running to true

This will be necessary for any user cli which has to know
the state before putting long running commands to background.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ioctl.c           |   10 ++++++++++
 include/uapi/linux/btrfs.h |    1 +
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 89f346c..3544a90 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4420,6 +4420,14 @@ out_unlock:
 	return ret;
 }
 
+static int btrfs_ioctl_check_dev_excl_op(struct btrfs_root *root,
+		void __user *arg)
+{
+	if (atomic_read(&root->fs_info->mutually_exclusive_operation_running))
+		return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
+	return 0;
+}
+
 long btrfs_ioctl(struct file *file, unsigned int
 		cmd, unsigned long arg)
 {
@@ -4532,6 +4540,8 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_set_fslabel(file, argp);
 	case BTRFS_IOC_FILE_EXTENT_SAME:
 		return btrfs_ioctl_file_extent_same(file, argp);
+	case BTRFS_IOC_CHECK_DEV_EXCL_OPS:
+		return btrfs_ioctl_check_dev_excl_op(root, NULL);
 	}
 
 	return -ENOTTY;
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index 90d7bd9..a4fa6eb 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -606,5 +606,6 @@ static inline char *btrfs_err_str(enum btrfs_err_code err_code)
 				    struct btrfs_ioctl_dev_replace_args)
 #define BTRFS_IOC_FILE_EXTENT_SAME _IOWR(BTRFS_IOCTL_MAGIC, 54, \
 					 struct btrfs_ioctl_same_args)
+#define BTRFS_IOC_CHECK_DEV_EXCL_OPS _IO(BTRFS_IOCTL_MAGIC, 56)
 
 #endif /* _UAPI_LINUX_BTRFS_H */
-- 
1.7.1

[PATCH] btrfs-progs: replace fails start but in the background

[Anand Jain]

when the balance is running, the replace start ioctl
fails (for the right reasons). but since the cli has
put ioctl thread to background (for right reasons)
the user won't know that cli failed to start.

so before cli goes to the background, it should check
if mutually_exclusive_operation_running is not held.

this is done by newly introduced ioctl
 BTRFS_IOC_CHECK_DEV_EXCL_OPS by the following kernel patch:

btrfs: introduce BTRFS_IOC_CHECK_DEV_EXCL_OPS ioctl to check dev excl op

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-replace.c |   14 ++++++++++++++
 ioctl.h        |    1 +
 utils.c        |   15 +++++++++++++++
 utils.h        |    1 +
 4 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/cmds-replace.c b/cmds-replace.c
index e3ff695..0b2cbc8 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -203,6 +203,20 @@ static int cmd_start_replace(int argc, char **argv)
 		goto leave_with_error;
 	}
 
+	/* check if there is some other device exclusive
+	 * operation running in the FS which won't let this replace
+	 * to run. And ENOTTY is when older kernel doesn't support
+	 * lock checking ioctl
+	 */
+	ret = is_dev_excl_op_free(fdmnt);
+	if (ret && ret != -ENOTTY) {
+		fprintf(stderr,
+			"ERROR: replace start failed on \"%s\" - %s\n",
+			path,
+			ret > 0 ? btrfs_err_str(ret) : strerror(-ret));
+		goto leave_with_error;
+	}
+
 	srcdev = argv[optind];
 	dstdev = argv[optind + 1];
 
diff --git a/ioctl.h b/ioctl.h
index e959720..afadcbc 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -598,6 +598,7 @@ struct btrfs_ioctl_clone_range_args {
 #define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
 				    struct btrfs_ioctl_dev_replace_args)
 #define BTRFS_IOC_DEDUP_CTL _IOW(BTRFS_IOCTL_MAGIC, 55, int)
+#define BTRFS_IOC_CHECK_DEV_EXCL_OPS _IO(BTRFS_IOCTL_MAGIC, 56)
 
 #ifdef __cplusplus
 }
diff --git a/utils.c b/utils.c
index 422530d..f0f7719 100644
--- a/utils.c
+++ b/utils.c
@@ -1992,3 +1992,18 @@ int is_vol_small(char *file)
 		return 0;
 	}
 }
+
+/* Returns:
+ * BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS:
+ *	If the device is locked to prevent other device operations
+ * 	from the user cli like device add remove replace balance etc..
+ * < 0:
+ *	For any other error including if kernel don't support the
+ *	ioctl (-ENOTTY)
+ */
+int is_dev_excl_op_free(int fd)
+{
+	int ret;
+	ret = ioctl(fd, BTRFS_IOC_CHECK_DEV_EXCL_OPS, NULL);
+	return ret > 0 ? ret : -errno;
+}
diff --git a/utils.h b/utils.h
index eb6fba3..a1e5d67 100644
--- a/utils.h
+++ b/utils.h
@@ -83,4 +83,5 @@ int test_num_disk_vs_raid(u64 metadata_profile, u64 data_profile,
 	u64 dev_cnt, int mixed, char *estr);
 int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
 int is_vol_small(char *file);
+int is_dev_excl_op_free(int fd);
 #endif
-- 
1.7.1

[PATCH] btrfs-progs: replace fails start but in the background v2

[Anand Jain]

when the balance is running, the replace start ioctl
fails (for the right reasons). but since the cli has
put ioctl thread to background (for right reasons)
the user won't know that cli failed to start.

so before cli goes to the background, it should check
if mutually_exclusive_operation_running is not held.

this is done by newly introduced ioctl
 BTRFS_IOC_CHECK_DEV_EXCL_OPS by the following kernel patch:

btrfs: introduce BTRFS_IOC_CHECK_DEV_EXCL_OPS ioctl to check dev excl op

v2:
 fix checkpatch.pl warnings as spotted by Wang

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-device.c  |    4 ++--
 cmds-replace.c |   14 ++++++++++++++
 ioctl.h        |    1 +
 man/btrfs.8.in |    4 ++--
 utils.c        |   15 +++++++++++++++
 utils.h        |    1 +
 6 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/cmds-device.c b/cmds-device.c
index 8446502..27f7f84 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -45,7 +45,7 @@ static const char * const device_cmd_group_usage[] = {
 };
 
 static const char * const cmd_add_dev_usage[] = {
-	"btrfs device add <device> [<device>...] <path>",
+	"btrfs device add [-f] <device> [<device>...] <path>",
 	"Add a device to a filesystem",
 	NULL
 };
@@ -85,7 +85,7 @@ static int cmd_add_dev(int argc, char **argv)
 		int	devfd, res;
 		u64 dev_block_count = 0;
 		int mixed = 0;
-
+printf("asj: add %s\n", argv[i]);
 		res = test_dev_for_mkfs(argv[i], force, estr);
 		if (res) {
 			fprintf(stderr, "%s", estr);
diff --git a/cmds-replace.c b/cmds-replace.c
index e3ff695..0b2cbc8 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -203,6 +203,20 @@ static int cmd_start_replace(int argc, char **argv)
 		goto leave_with_error;
 	}
 
+	/* check if there is some other device exclusive
+	 * operation running in the FS which won't let this replace
+	 * to run. And ENOTTY is when older kernel doesn't support
+	 * lock checking ioctl
+	 */
+	ret = is_dev_excl_op_free(fdmnt);
+	if (ret && ret != -ENOTTY) {
+		fprintf(stderr,
+			"ERROR: replace start failed on \"%s\" - %s\n",
+			path,
+			ret > 0 ? btrfs_err_str(ret) : strerror(-ret));
+		goto leave_with_error;
+	}
+
 	srcdev = argv[optind];
 	dstdev = argv[optind + 1];
 
diff --git a/ioctl.h b/ioctl.h
index e959720..afadcbc 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -598,6 +598,7 @@ struct btrfs_ioctl_clone_range_args {
 #define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
 				    struct btrfs_ioctl_dev_replace_args)
 #define BTRFS_IOC_DEDUP_CTL _IOW(BTRFS_IOCTL_MAGIC, 55, int)
+#define BTRFS_IOC_CHECK_DEV_EXCL_OPS _IO(BTRFS_IOCTL_MAGIC, 56)
 
 #ifdef __cplusplus
 }
diff --git a/man/btrfs.8.in b/man/btrfs.8.in
index d493d7e..0cca5c3 100644
--- a/man/btrfs.8.in
+++ b/man/btrfs.8.in
@@ -47,7 +47,7 @@ btrfs \- control a btrfs filesystem
 \fBbtrfs\fP \fB[filesystem] balance status\fP [-v] \fI<path>\fP
 .PP
 .PP
-\fBbtrfs\fP \fBdevice add\fP \fI<device>\fP [\fI<device>...\fP] \fI<path>\fP
+\fBbtrfs\fP \fBdevice add\fP [-f] \fI<device>\fP [\fI<device>...\fP] \fI<path>\fP
 .PP
 \fBbtrfs\fP \fBdevice delete\fP \fI<device>\fP [\fI<device>...\fP] \fI<path>\fP
 .PP
@@ -386,7 +386,7 @@ be verbose
 .RE
 .TP
 
-\fBdevice add\fR\fI <dev> \fP[\fI<dev>...\fP] \fI<path>\fR
+\fBdevice add\fR [-f] \fI <dev> \fP[\fI<dev>...\fP] \fI<path>\fR
 Add device(s) to the filesystem identified by \fI<path>\fR.
 .TP
 
diff --git a/utils.c b/utils.c
index b039a03..c53b7f1 100644
--- a/utils.c
+++ b/utils.c
@@ -1993,3 +1993,18 @@ int is_vol_small(char *file)
 		return 0;
 	}
 }
+
+/* Returns:
+ * BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS:
+ *	If the device is locked to prevent other device operations
+ *	from the user cli like device add remove replace balance etc..
+ * < 0:
+ *	For any other error including if kernel don't support the
+ *	ioctl (-ENOTTY)
+ */
+int is_dev_excl_op_free(int fd)
+{
+	int ret;
+	ret = ioctl(fd, BTRFS_IOC_CHECK_DEV_EXCL_OPS, NULL);
+	return ret > 0 ? ret : -errno;
+}
diff --git a/utils.h b/utils.h
index eb6fba3..a1e5d67 100644
--- a/utils.h
+++ b/utils.h
@@ -83,4 +83,5 @@ int test_num_disk_vs_raid(u64 metadata_profile, u64 data_profile,
 	u64 dev_cnt, int mixed, char *estr);
 int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
 int is_vol_small(char *file);
+int is_dev_excl_op_free(int fd);
 #endif
-- 
1.7.1

Re: [PATCH] btrfs-progs: replace fails start but in the background v2

[Anand Jain]

  this there is commit error. Kindly ignore this.
  I have sent out a new patch for this.

Thanks, Anand

On 09/02/2013 10:55 AM, Anand Jain wrote:
> when the balance is running, the replace start ioctl
> fails (for the right reasons). but since the cli has
> put ioctl thread to background (for right reasons)
> the user won't know that cli failed to start.
>
> so before cli goes to the background, it should check
> if mutually_exclusive_operation_running is not held.
>
> this is done by newly introduced ioctl
>   BTRFS_IOC_CHECK_DEV_EXCL_OPS by the following kernel patch:
>
> btrfs: introduce BTRFS_IOC_CHECK_DEV_EXCL_OPS ioctl to check dev excl op
>
> v2:
>   fix checkpatch.pl warnings as spotted by Wang
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>   cmds-device.c  |    4 ++--
>   cmds-replace.c |   14 ++++++++++++++
>   ioctl.h        |    1 +
>   man/btrfs.8.in |    4 ++--
>   utils.c        |   15 +++++++++++++++
>   utils.h        |    1 +
>   6 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/cmds-device.c b/cmds-device.c
> index 8446502..27f7f84 100644
> --- a/cmds-device.c
> +++ b/cmds-device.c
> @@ -45,7 +45,7 @@ static const char * const device_cmd_group_usage[] = {
>   };
>
>   static const char * const cmd_add_dev_usage[] = {
> -	"btrfs device add <device> [<device>...] <path>",
> +	"btrfs device add [-f] <device> [<device>...] <path>",
>   	"Add a device to a filesystem",
>   	NULL
>   };
> @@ -85,7 +85,7 @@ static int cmd_add_dev(int argc, char **argv)
>   		int	devfd, res;
>   		u64 dev_block_count = 0;
>   		int mixed = 0;
> -
> +printf("asj: add %s\n", argv[i]);
>   		res = test_dev_for_mkfs(argv[i], force, estr);
>   		if (res) {
>   			fprintf(stderr, "%s", estr);
> diff --git a/cmds-replace.c b/cmds-replace.c
> index e3ff695..0b2cbc8 100644
> --- a/cmds-replace.c
> +++ b/cmds-replace.c
> @@ -203,6 +203,20 @@ static int cmd_start_replace(int argc, char **argv)
>   		goto leave_with_error;
>   	}
>
> +	/* check if there is some other device exclusive
> +	 * operation running in the FS which won't let this replace
> +	 * to run. And ENOTTY is when older kernel doesn't support
> +	 * lock checking ioctl
> +	 */
> +	ret = is_dev_excl_op_free(fdmnt);
> +	if (ret && ret != -ENOTTY) {
> +		fprintf(stderr,
> +			"ERROR: replace start failed on \"%s\" - %s\n",
> +			path,
> +			ret > 0 ? btrfs_err_str(ret) : strerror(-ret));
> +		goto leave_with_error;
> +	}
> +
>   	srcdev = argv[optind];
>   	dstdev = argv[optind + 1];
>
> diff --git a/ioctl.h b/ioctl.h
> index e959720..afadcbc 100644
> --- a/ioctl.h
> +++ b/ioctl.h
> @@ -598,6 +598,7 @@ struct btrfs_ioctl_clone_range_args {
>   #define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
>   				    struct btrfs_ioctl_dev_replace_args)
>   #define BTRFS_IOC_DEDUP_CTL _IOW(BTRFS_IOCTL_MAGIC, 55, int)
> +#define BTRFS_IOC_CHECK_DEV_EXCL_OPS _IO(BTRFS_IOCTL_MAGIC, 56)
>
>   #ifdef __cplusplus
>   }
> diff --git a/man/btrfs.8.in b/man/btrfs.8.in
> index d493d7e..0cca5c3 100644
> --- a/man/btrfs.8.in
> +++ b/man/btrfs.8.in
> @@ -47,7 +47,7 @@ btrfs \- control a btrfs filesystem
>   \fBbtrfs\fP \fB[filesystem] balance status\fP [-v] \fI<path>\fP
>   .PP
>   .PP
> -\fBbtrfs\fP \fBdevice add\fP \fI<device>\fP [\fI<device>...\fP] \fI<path>\fP
> +\fBbtrfs\fP \fBdevice add\fP [-f] \fI<device>\fP [\fI<device>...\fP] \fI<path>\fP
>   .PP
>   \fBbtrfs\fP \fBdevice delete\fP \fI<device>\fP [\fI<device>...\fP] \fI<path>\fP
>   .PP
> @@ -386,7 +386,7 @@ be verbose
>   .RE
>   .TP
>
> -\fBdevice add\fR\fI <dev> \fP[\fI<dev>...\fP] \fI<path>\fR
> +\fBdevice add\fR [-f] \fI <dev> \fP[\fI<dev>...\fP] \fI<path>\fR
>   Add device(s) to the filesystem identified by \fI<path>\fR.
>   .TP
>
> diff --git a/utils.c b/utils.c
> index b039a03..c53b7f1 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1993,3 +1993,18 @@ int is_vol_small(char *file)
>   		return 0;
>   	}
>   }
> +
> +/* Returns:
> + * BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS:
> + *	If the device is locked to prevent other device operations
> + *	from the user cli like device add remove replace balance etc..
> + * < 0:
> + *	For any other error including if kernel don't support the
> + *	ioctl (-ENOTTY)
> + */
> +int is_dev_excl_op_free(int fd)
> +{
> +	int ret;
> +	ret = ioctl(fd, BTRFS_IOC_CHECK_DEV_EXCL_OPS, NULL);
> +	return ret > 0 ? ret : -errno;
> +}
> diff --git a/utils.h b/utils.h
> index eb6fba3..a1e5d67 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -83,4 +83,5 @@ int test_num_disk_vs_raid(u64 metadata_profile, u64 data_profile,
>   	u64 dev_cnt, int mixed, char *estr);
>   int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
>   int is_vol_small(char *file);
> +int is_dev_excl_op_free(int fd);
>   #endif
>

[PATCH] btrfs-progs: replace fails start but in the background

[Anand Jain]

when the balance is running, the replace start ioctl
fails (for the right reasons). but since the cli has
put ioctl thread to background (for right reasons)
the user won't know that cli failed to start.

so before cli goes to the background, it should check
if mutually_exclusive_operation_running is not held.

this is done by newly introduced ioctl
 BTRFS_IOC_CHECK_DEV_EXCL_OPS by the following kernel patch:

btrfs: introduce BTRFS_IOC_CHECK_DEV_EXCL_OPS ioctl to check dev excl op

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-replace.c |   14 ++++++++++++++
 ioctl.h        |    1 +
 utils.c        |   15 +++++++++++++++
 utils.h        |    1 +
 4 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/cmds-replace.c b/cmds-replace.c
index e89f8cc..916d997 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -203,6 +203,20 @@ static int cmd_start_replace(int argc, char **argv)
 		goto leave_with_error;
 	}
 
+	/* check if there is some other device exclusive
+	 * operation running in the FS which won't let this replace
+	 * to run. And ENOTTY is when older kernel doesn't support
+	 * lock checking ioctl
+	 */
+	ret = is_dev_excl_op_free(fdmnt);
+	if (ret && ret != -ENOTTY) {
+		fprintf(stderr,
+			"ERROR: replace start failed on \"%s\" - %s\n",
+			path,
+			ret > 0 ? btrfs_err_str(ret) : strerror(-ret));
+		goto leave_with_error;
+	}
+
 	srcdev = argv[optind];
 	dstdev = argv[optind + 1];
 
diff --git a/ioctl.h b/ioctl.h
index e959720..afadcbc 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -598,6 +598,7 @@ struct btrfs_ioctl_clone_range_args {
 #define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
 				    struct btrfs_ioctl_dev_replace_args)
 #define BTRFS_IOC_DEDUP_CTL _IOW(BTRFS_IOCTL_MAGIC, 55, int)
+#define BTRFS_IOC_CHECK_DEV_EXCL_OPS _IO(BTRFS_IOCTL_MAGIC, 56)
 
 #ifdef __cplusplus
 }
diff --git a/utils.c b/utils.c
index 82672a7..54a80a6 100644
--- a/utils.c
+++ b/utils.c
@@ -1998,3 +1998,18 @@ int is_vol_small(char *file)
 		return 0;
 	}
 }
+
+/* Returns:
+ * BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS:
+ *	If the device is locked to prevent other device operations
+ *	from the user cli like device add remove replace balance etc..
+ * < 0:
+ *	For any other error including if kernel don't support the
+ *	ioctl (-ENOTTY)
+ */
+int is_dev_excl_op_free(int fd)
+{
+	int ret;
+	ret = ioctl(fd, BTRFS_IOC_CHECK_DEV_EXCL_OPS, NULL);
+	return ret > 0 ? ret : -errno;
+}
diff --git a/utils.h b/utils.h
index 6446e0a..5ef728e 100644
--- a/utils.h
+++ b/utils.h
@@ -83,4 +83,5 @@ int test_num_disk_vs_raid(u64 metadata_profile, u64 data_profile,
 	u64 dev_cnt, int mixed, char *estr);
 int is_vol_small(char *file);
 int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
+int is_dev_excl_op_free(int fd);
 #endif
-- 
1.7.1

Re: [PATCH] btrfs: introduce BTRFS_IOC_CHECK_DEV_EXCL_OPS ioctl to check dev excl op

[Stefan Behrens]

On Wed, 21 Aug 2013 21:10:42 +0800, Anand Jain wrote:
> This patch provides an ioctl to check if the FS is performing
> any device exclusive operations like device add remove balance
> etc. Basically any operation which will set
> fs_info->mutually_exclusive_operation_running to true
> 
> This will be necessary for any user cli which has to know
> the state before putting long running commands to background.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/ioctl.c           |   10 ++++++++++
>  include/uapi/linux/btrfs.h |    1 +
>  2 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 89f346c..3544a90 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4420,6 +4420,14 @@ out_unlock:
>  	return ret;
>  }
>  
> +static int btrfs_ioctl_check_dev_excl_op(struct btrfs_root *root,
> +		void __user *arg)
> +{
> +	if (atomic_read(&root->fs_info->mutually_exclusive_operation_running))
> +		return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
> +	return 0;
> +}
> +
>  long btrfs_ioctl(struct file *file, unsigned int
>  		cmd, unsigned long arg)
>  {
> @@ -4532,6 +4540,8 @@ long btrfs_ioctl(struct file *file, unsigned int
>  		return btrfs_ioctl_set_fslabel(file, argp);
>  	case BTRFS_IOC_FILE_EXTENT_SAME:
>  		return btrfs_ioctl_file_extent_same(file, argp);
> +	case BTRFS_IOC_CHECK_DEV_EXCL_OPS:
> +		return btrfs_ioctl_check_dev_excl_op(root, NULL);
>  	}
>  
>  	return -ENOTTY;
> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> index 90d7bd9..a4fa6eb 100644
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -606,5 +606,6 @@ static inline char *btrfs_err_str(enum btrfs_err_code err_code)
>  				    struct btrfs_ioctl_dev_replace_args)
>  #define BTRFS_IOC_FILE_EXTENT_SAME _IOWR(BTRFS_IOCTL_MAGIC, 54, \
>  					 struct btrfs_ioctl_same_args)
> +#define BTRFS_IOC_CHECK_DEV_EXCL_OPS _IO(BTRFS_IOCTL_MAGIC, 56)
>  
>  #endif /* _UAPI_LINUX_BTRFS_H */
> 

IMHO this is just a workaround for a design flaw.

Now it is like this (in the replace CLI without the "do not background"
option):
1. in user mode, check that the ioctl will succeed, exit with failure if
the checks fail
2. fork
3. exit(0) the parent
4. the backgrounded child executes the ioctl, the result is _ignored_
5. the user _has to_ check the status (progress, completion, errors) by
calling 'btrfs replace status' which can optionally block until the
replace procedure is finished, or optionally periodically print the
status. The exit value of 'btrfs replace status' is set depending on
whether there had been errors.

Step #1 is incomplete and can never be complete due to race conditions
and because the checks in the kernel could be enhanced without updating
the progs. Your patch is trying to improve step #1, to keep up with the
checks in the kernel code.

With the "do not background" option for btrfs replace it looks like this:
1. check that the ioctl will succeed (just because the code is shared
with the case that the "do not background" option is not set)
2. execute the ioctl, the result is used to generate error messages and
to build the exit code, the process does not terminate before the
procedure is finished
3. the user _can_ check the status (progress, completion, errors) by
calling 'btrfs replace status'

I'd prefer it like this in both cases:
1. execute the ioctl (don't check anything in user mode before), set a
flag whether the "do not background" option is set
2. in the kernel code inside the ioctl, perform all checks until there
is no more regular reason to fail
3. in case of errors, return from the ioctl with an error code
4. if backgrounding was requested, return from the ioctl indicating that
all checks had succeeded
5. if the ioctl failed, print an error message and set the exit value
accordingly
6. if the ioctl succeeded and backgrounding was requested, background in
user mode and call the ioctl again, this time with the "do not
background" option

This way we would still have the issue with the possible race, but at
least the checks would be done only in one place. And the additional
ioctl(CHECK_DEV_EXCL_OPS) would be avoided.

Re: [PATCH] btrfs: introduce BTRFS_IOC_CHECK_DEV_EXCL_OPS ioctl to check dev excl op

[anand jain]

Thanks for reviewing. Comments below.

> IMHO this is just a workaround for a design flaw.

  Its a simple fix on the lines of current design.

> Now it is like this (in the replace CLI without the "do not background"
> option):
> 1. in user mode, check that the ioctl will succeed, exit with failure if
> the checks fail
> 2. fork
> 3. exit(0) the parent
> 4. the backgrounded child executes the ioctl, the result is _ignored_
> 5. the user _has to_ check the status (progress, completion, errors) by
> calling 'btrfs replace status' which can optionally block until the
> replace procedure is finished, or optionally periodically print the
> status. The exit value of 'btrfs replace status' is set depending on
> whether there had been errors.

  Yes thats the current design.

> Step #1 is incomplete and can never be complete due to race conditions

> and because the checks in the kernel could be enhanced without updating
> the progs. Your patch is trying to improve step #1, to keep up with the
> checks in the kernel code.

  its an overhead, something like design drawback


> With the "do not background" option for btrfs replace it looks like this:
> 1. check that the ioctl will succeed (just because the code is shared
> with the case that the "do not background" option is not set)
> 2. execute the ioctl, the result is used to generate error messages and
> to build the exit code, the process does not terminate before the
> procedure is finished
> 3. the user _can_ check the status (progress, completion, errors) by
> calling 'btrfs replace status'

  Ok. Code can be bit optimized.

> I'd prefer it like this in both cases:
> 1. execute the ioctl (don't check anything in user mode before), set a
> flag whether the "do not background" option is set
> 2. in the kernel code inside the ioctl, perform all checks until there
> is no more regular reason to fail
> 3. in case of errors, return from the ioctl with an error code
> 4. if backgrounding was requested, return from the ioctl indicating that
> all checks had succeeded
> 5. if the ioctl failed, print an error message and set the exit value
> accordingly

> 6. if the ioctl succeeded and backgrounding was requested, background in
> user mode and call the ioctl again, this time with the "do not
> background" option

> This way we would still have the issue with the possible race, but at
> least the checks would be done only in one place. And the additional
> ioctl(CHECK_DEV_EXCL_OPS) would be avoided.

  yeah this can be done for the long term it helps to reduce the
  overhead of maintaining two similar codes. But CHECK_DEV_EXCL_OPS
  helps to solve an easily seen bug[1] with minimal impact.
  Further CHECK_DEV_EXCL_OPS will be useful to fix issues with
  other cross vol operations.

  [1]
  test case:
   terminal1: Run balance;
   terminal2: Run replace (without -B)
   problem: though replace fails, its unreported

Thanks, Anand

Re: [PATCH] btrfs: introduce BTRFS_IOC_CHECK_DEV_EXCL_OPS ioctl to check dev excl op

[Stefan Behrens]

On Thu, 22 Aug 2013 17:51:37 +0800, anand jain wrote:
> 
> Thanks for reviewing. Comments below.
> 
>> IMHO this is just a workaround for a design flaw.
> 
>  Its a simple fix on the lines of current design.
> 
>> Now it is like this (in the replace CLI without the "do not background"
>> option):
>> 1. in user mode, check that the ioctl will succeed, exit with failure if
>> the checks fail
>> 2. fork
>> 3. exit(0) the parent
>> 4. the backgrounded child executes the ioctl, the result is _ignored_
>> 5. the user _has to_ check the status (progress, completion, errors) by
>> calling 'btrfs replace status' which can optionally block until the
>> replace procedure is finished, or optionally periodically print the
>> status. The exit value of 'btrfs replace status' is set depending on
>> whether there had been errors.
> 
>  Yes thats the current design.
> 
>> Step #1 is incomplete and can never be complete due to race conditions
> 
>> and because the checks in the kernel could be enhanced without updating
>> the progs. Your patch is trying to improve step #1, to keep up with the
>> checks in the kernel code.
> 
>  its an overhead, something like design drawback
> 
> 
>> With the "do not background" option for btrfs replace it looks like this:
>> 1. check that the ioctl will succeed (just because the code is shared
>> with the case that the "do not background" option is not set)
>> 2. execute the ioctl, the result is used to generate error messages and
>> to build the exit code, the process does not terminate before the
>> procedure is finished
>> 3. the user _can_ check the status (progress, completion, errors) by
>> calling 'btrfs replace status'
> 
>  Ok. Code can be bit optimized.
> 
>> I'd prefer it like this in both cases:
>> 1. execute the ioctl (don't check anything in user mode before), set a
>> flag whether the "do not background" option is set
>> 2. in the kernel code inside the ioctl, perform all checks until there
>> is no more regular reason to fail
>> 3. in case of errors, return from the ioctl with an error code
>> 4. if backgrounding was requested, return from the ioctl indicating that
>> all checks had succeeded
>> 5. if the ioctl failed, print an error message and set the exit value
>> accordingly
> 
>> 6. if the ioctl succeeded and backgrounding was requested, background in
>> user mode and call the ioctl again, this time with the "do not
>> background" option
> 
>> This way we would still have the issue with the possible race, but at
>> least the checks would be done only in one place. And the additional
>> ioctl(CHECK_DEV_EXCL_OPS) would be avoided.
> 
>  yeah this can be done for the long term it helps to reduce the
>  overhead of maintaining two similar codes. But CHECK_DEV_EXCL_OPS
>  helps to solve an easily seen bug[1] with minimal impact.
>  Further CHECK_DEV_EXCL_OPS will be useful to fix issues with
>  other cross vol operations.

Which issues? Please share the details.
The other commands do not background and do report errors.

If the issue is that the error values for the
exclusive-operation-in-progress thing are not consistent (we have
-EINVAL, -EINPROGRESS and BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS,
depending on the ioctl), then fix the return values.


>  [1]
>  test case:
>   terminal1: Run balance;
>   terminal2: Run replace (without -B)
>   problem: though replace fails, its unreported
>

Re: [PATCH] btrfs: introduce BTRFS_IOC_CHECK_DEV_EXCL_OPS ioctl to check dev excl op

[Anand Jain]

>>   Further CHECK_DEV_EXCL_OPS will be useful to fix issues with
>>   other cross vol operations.
>
> Which issues? Please share the details.
> The other commands do not background and do report errors.

  I am assuming potentially required (to background) for balance,
  remove and resize.

Thanks, Anand

[PATCH] btrfs-progs: replace fails start but in the background

[Anand Jain]

when the balance is running, the replace start ioctl
fails (for the right reasons). but since the cli has
put ioctl thread to background (for right reasons)
the user won't know that cli failed to start.

so before cli goes to the background, it should check
if mutually_exclusive_operation_running is not held.

this is done by newly introduced ioctl
 BTRFS_IOC_CHECK_DEV_EXCL_OPS by the following kernel patch:

btrfs: introduce BTRFS_IOC_CHECK_DEV_EXCL_OPS ioctl to check dev excl op

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-replace.c | 14 ++++++++++++++
 ioctl.h        |  1 +
 utils.c        | 15 +++++++++++++++
 utils.h        |  1 +
 4 files changed, 31 insertions(+)

diff --git a/cmds-replace.c b/cmds-replace.c
index a31d77e..f69c5ce 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -203,6 +203,20 @@ static int cmd_start_replace(int argc, char **argv)
 		goto leave_with_error;
 	}
 
+	/* check if there is some other device exclusive
+	 * operation running in the FS which won't let this replace
+	 * to run. And ENOTTY is when older kernel doesn't support
+	 * lock checking ioctl
+	 */
+	ret = is_dev_excl_op_free(fdmnt);
+	if (ret && ret != -ENOTTY) {
+		fprintf(stderr,
+			"ERROR: replace start failed on \"%s\" - %s\n",
+			path,
+			ret > 0 ? btrfs_err_str(ret) : strerror(-ret));
+		goto leave_with_error;
+	}
+
 	srcdev = argv[optind];
 	dstdev = argv[optind + 1];
 
diff --git a/ioctl.h b/ioctl.h
index c0dcc06..7df7fc9 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -598,6 +598,7 @@ struct btrfs_ioctl_clone_range_args {
 #define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
 				    struct btrfs_ioctl_dev_replace_args)
 #define BTRFS_IOC_DEDUP_CTL _IOW(BTRFS_IOCTL_MAGIC, 55, int)
+#define BTRFS_IOC_CHECK_DEV_EXCL_OPS _IO(BTRFS_IOCTL_MAGIC, 56)
 
 #ifdef __cplusplus
 }
diff --git a/utils.c b/utils.c
index 02a2658..22c3310 100644
--- a/utils.c
+++ b/utils.c
@@ -2004,3 +2004,18 @@ int is_vol_small(char *file)
 		return 0;
 	}
 }
+
+/* Returns:
+ * BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS:
+ *	If the device is locked to prevent other device operations
+ *	from the user cli like device add remove replace balance etc..
+ * < 0:
+ *	For any other error including if kernel don't support the
+ *	ioctl (-ENOTTY)
+ */
+int is_dev_excl_op_free(int fd)
+{
+	int ret;
+	ret = ioctl(fd, BTRFS_IOC_CHECK_DEV_EXCL_OPS, NULL);
+	return ret > 0 ? ret : -errno;
+}
diff --git a/utils.h b/utils.h
index 616bae1..6952d34 100644
--- a/utils.h
+++ b/utils.h
@@ -85,4 +85,5 @@ int is_vol_small(char *file);
 int csum_tree_block(struct btrfs_root *root, struct extent_buffer *buf,
 			   int verify);
 int get_btrfs_mount(const char *dev, char *mp, size_t mp_size);
+int is_dev_excl_op_free(int fd);
 #endif
-- 
1.8.4.rc4.1.g0d8beaa