[bug] error communication from kernel to userland

[bug] error communication from kernel to userland

[Anand Jain]

Hi,

  the below example tells a problem..

  ----
  # btrfs dev del missing /btrfs
  ERROR: error removing the device 'missing' - Invalid argument

  # dmesg | tail
  ::
  [ 4295.258686] btrfs: unable to go below two devices on raid1
  #
  -----
  as of now the more accurate errors are being logged to dmesg,
  which a normal user might not notice and there are few other
  btrfs cli cases similar to this.

  We need to get the kernel errors to the useland as part the
  cli being used.

  However the btrfs is relaying on the limited set of errno
  defines which is insufficient / inappropriate in the
  btrfs context.

  As a solution we have 2 choices broadly.. as of now

  - Update the errno with more FS and RAID related defines

  - Get the error string as part of ioctl arg.

  - ?


  any comments / questions / views / ideas are welcome..


Thanks, Anand

device delete to get errors from the kernel

[Anand Jain]

  As showed in the previous email in this thread, we need to get
  the error string from the kernel to the cli to improve the
  usability of the product.
  As also said, I was looking at two way which I think we could
  do this, here I take the 2nd approach which is to pass the error
  string though the ioctl args. Which leads to change in the
  ioctl-structure. But..

  This comes with a caveat that both btrfs-progs and btrfs-kernel
  patch together either must be applied or removed.

   [PATCH] btrfs-progs: device delete to get errors from the kernel
   [PATCH] btrfs: device delete to get errors from the kernel

  Which is not a developer/integrator friendly rule. If there
  is anything I could do to help on this pls do let me know.

  There are quite a number of cli which needs passing of the
  the error string from the kernel to cli. Which I plan to work
  once we finalize the approach to address this issue.

  Thanks for your time to review this.

Anand

[PATCH] btrfs-progs: device delete to get errors from the kernel

[Anand Jain]

add another parameter to the ioctl arg structure to carry the error string

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-device.c | 13 +++++++++----
 ioctl.h       |  9 ++++++++-
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/cmds-device.c b/cmds-device.c
index 41e79d3..3cf96db 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -157,15 +157,20 @@ static int cmd_rm_dev(int argc, char **argv)
 	}
 
 	for(i=1 ; i < argc - 1; i++ ){
-		struct	btrfs_ioctl_vol_args arg;
+		struct	btrfs_ioctl_dev_args arg;
 		int	res;
 
+		memset(&arg, 0, sizeof(arg));
 		strncpy_null(arg.name, argv[i]);
 		res = ioctl(fdmnt, BTRFS_IOC_RM_DEV, &arg);
 		e = errno;
-		if(res<0){
-			fprintf(stderr, "ERROR: error removing the device '%s' - %s\n",
-				argv[i], strerror(e));
+		if (res < 0) {
+			if (strlen(arg.ret_err_str))
+				fprintf(stderr, "ERROR: error removing the device '%s' - %s\n",
+					argv[i], arg.ret_err_str);
+			else
+				fprintf(stderr, "ERROR: error removing the device '%s' - %s\n",
+					argv[i], strerror(e));
 			ret++;
 		}
 	}
diff --git a/ioctl.h b/ioctl.h
index 1ee631a..b55bcb7 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -36,6 +36,13 @@ struct btrfs_ioctl_vol_args {
 	char name[BTRFS_PATH_NAME_MAX + 1];
 };
 
+#define BTRFS_IOCTL_ERR_LEN 256
+struct btrfs_ioctl_dev_args {
+	__s64 fd;
+	char name[BTRFS_PATH_NAME_MAX + 1];
+	char ret_err_str[BTRFS_IOCTL_ERR_LEN];
+};
+
 #define BTRFS_DEVICE_PATH_NAME_MAX 1024
 
 #define BTRFS_SUBVOL_CREATE_ASYNC	(1ULL << 0)
@@ -467,7 +474,7 @@ struct btrfs_ioctl_clone_range_args {
 #define BTRFS_IOC_CLONE        _IOW(BTRFS_IOCTL_MAGIC, 9, int)
 #define BTRFS_IOC_ADD_DEV _IOW(BTRFS_IOCTL_MAGIC, 10, \
 				   struct btrfs_ioctl_vol_args)
-#define BTRFS_IOC_RM_DEV _IOW(BTRFS_IOCTL_MAGIC, 11, \
+#define BTRFS_IOC_RM_DEV _IOWR(BTRFS_IOCTL_MAGIC, 11, \
 				   struct btrfs_ioctl_vol_args)
 #define BTRFS_IOC_BALANCE _IOW(BTRFS_IOCTL_MAGIC, 12, \
 				   struct btrfs_ioctl_vol_args)
-- 
1.8.1.227.g44fe835

[PATCH] btrfs: device delete to get errors from the kernel

[Anand Jain]

adds a parameter in the ioctl arg struct to carry the error string

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ioctl.c           | 42 +++++++++++++++++++++++++++---------------
 fs/btrfs/volumes.c         | 29 +++++++++++++++--------------
 fs/btrfs/volumes.h         |  2 +-
 include/uapi/linux/btrfs.h |  9 ++++++++-
 4 files changed, 51 insertions(+), 31 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 898c572..da9cba5 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2337,7 +2337,8 @@ out:
 static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 {
 	struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
-	struct btrfs_ioctl_vol_args *vol_args;
+	struct btrfs_ioctl_dev_args *dev_args = NULL;
+	char ret_err[BTRFS_IOCTL_ERR_LEN];
 	int ret;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -2347,27 +2348,38 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 	if (ret)
 		return ret;
 
-	if (atomic_xchg(&root->fs_info->mutually_exclusive_operation_running,
-			1)) {
-		pr_info("btrfs: dev add/delete/balance/replace/resize operation in progress\n");
-		mnt_drop_write_file(file);
-		return -EINVAL;
+	dev_args = memdup_user(arg, sizeof(*dev_args));
+	if (IS_ERR(dev_args)) {
+		ret = PTR_ERR(dev_args);
+		goto out;
 	}
 
-	mutex_lock(&root->fs_info->volume_mutex);
-	vol_args = memdup_user(arg, sizeof(*vol_args));
-	if (IS_ERR(vol_args)) {
-		ret = PTR_ERR(vol_args);
+	dev_args->name[BTRFS_PATH_NAME_MAX] = '\0';
+	memset(ret_err, 0, BTRFS_IOCTL_ERR_LEN);
+
+	if (atomic_xchg(&root->fs_info->mutually_exclusive_operation_running,
+			1)) {
+		strncpy(dev_args->ret_err_str,
+			"add/delete/balance/replace/resize operation in progress\n",
+			BTRFS_IOCTL_ERR_LEN);
+		if (copy_to_user(arg, dev_args, sizeof(dev_args)))
+			ret = -EFAULT;
+		ret = -EINVAL;
 		goto out;
 	}
 
-	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
-	ret = btrfs_rm_device(root, vol_args->name);
-
-	kfree(vol_args);
-out:
+	mutex_lock(&root->fs_info->volume_mutex);
+	ret = btrfs_rm_device(root, dev_args->name, ret_err);
 	mutex_unlock(&root->fs_info->volume_mutex);
 	atomic_set(&root->fs_info->mutually_exclusive_operation_running, 0);
+	if (ret) {
+		strncpy(dev_args->ret_err_str, ret_err,
+			BTRFS_IOCTL_ERR_LEN);
+		if (copy_to_user(arg, dev_args, sizeof(*dev_args)))
+			ret = -EFAULT;
+	}
+out:
+	kfree(dev_args);
 	mnt_drop_write_file(file);
 	return ret;
 }
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5989a92..12c02d7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1426,7 +1426,7 @@ out:
 	return ret;
 }
 
-int btrfs_rm_device(struct btrfs_root *root, char *device_path)
+int btrfs_rm_device(struct btrfs_root *root, char *device_path, char *ret_err)
 {
 	struct btrfs_device *device;
 	struct btrfs_device *next_device;
@@ -1461,30 +1461,30 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 	btrfs_dev_replace_unlock(&root->fs_info->dev_replace);
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID10) && num_devices <= 4) {
-		printk(KERN_ERR "btrfs: unable to go below four devices "
-		       "on raid10\n");
+		strncpy(ret_err, "unable to go below four devices on raid10",
+				BTRFS_IOCTL_ERR_LEN);
 		ret = -EINVAL;
 		goto out;
 	}
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID1) && num_devices <= 2) {
-		printk(KERN_ERR "btrfs: unable to go below two "
-		       "devices on raid1\n");
+		strncpy(ret_err, "unable to go below two devices on raid1",
+				BTRFS_IOCTL_ERR_LEN);
 		ret = -EINVAL;
 		goto out;
 	}
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID5) &&
 	    root->fs_info->fs_devices->rw_devices <= 2) {
-		printk(KERN_ERR "btrfs: unable to go below two "
-		       "devices on raid5\n");
+		strncpy(ret_err, "unable to go below two devices on raid5",
+				BTRFS_IOCTL_ERR_LEN);
 		ret = -EINVAL;
 		goto out;
 	}
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID6) &&
 	    root->fs_info->fs_devices->rw_devices <= 3) {
-		printk(KERN_ERR "btrfs: unable to go below three "
-		       "devices on raid6\n");
+		strncpy(ret_err, "unable to go below three devices on raid6",
+				BTRFS_IOCTL_ERR_LEN);
 		ret = -EINVAL;
 		goto out;
 	}
@@ -1511,8 +1511,8 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 		bh = NULL;
 		disk_super = NULL;
 		if (!device) {
-			printk(KERN_ERR "btrfs: no missing devices found to "
-			       "remove\n");
+			strncpy(ret_err, "no missing devices found to remove",
+				BTRFS_IOCTL_ERR_LEN);
 			goto out;
 		}
 	} else {
@@ -1534,14 +1534,15 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 	}
 
 	if (device->is_tgtdev_for_dev_replace) {
-		pr_err("btrfs: unable to remove the dev_replace target dev\n");
+		strncpy(ret_err, "unable to remove the dev_replace target dev",
+			BTRFS_IOCTL_ERR_LEN);
 		ret = -EINVAL;
 		goto error_brelse;
 	}
 
 	if (device->writeable && root->fs_info->fs_devices->rw_devices == 1) {
-		printk(KERN_ERR "btrfs: unable to remove the only writeable "
-		       "device\n");
+		strncpy(ret_err, "unable to remove the only writeable device",
+			BTRFS_IOCTL_ERR_LEN);
 		ret = -EINVAL;
 		goto error_brelse;
 	}
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 062d860..94dc9f2 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -287,7 +287,7 @@ int btrfs_find_device_by_path(struct btrfs_root *root, char *device_path,
 int btrfs_add_device(struct btrfs_trans_handle *trans,
 		     struct btrfs_root *root,
 		     struct btrfs_device *device);
-int btrfs_rm_device(struct btrfs_root *root, char *device_path);
+int btrfs_rm_device(struct btrfs_root *root, char *device_path, char *ret_err);
 void btrfs_cleanup_fs_uuids(void);
 int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
 int btrfs_grow_device(struct btrfs_trans_handle *trans,
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index fa3a5f9..b2fc716 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -31,6 +31,13 @@ struct btrfs_ioctl_vol_args {
 	char name[BTRFS_PATH_NAME_MAX + 1];
 };
 
+#define BTRFS_IOCTL_ERR_LEN 256
+struct btrfs_ioctl_dev_args {
+	__s64 fd;
+	char name[BTRFS_PATH_NAME_MAX + 1];
+	char ret_err_str[BTRFS_IOCTL_ERR_LEN];
+};
+
 #define BTRFS_DEVICE_PATH_NAME_MAX 1024
 
 #define BTRFS_SUBVOL_CREATE_ASYNC	(1ULL << 0)
@@ -442,7 +449,7 @@ struct btrfs_ioctl_send_args {
 #define BTRFS_IOC_CLONE        _IOW(BTRFS_IOCTL_MAGIC, 9, int)
 #define BTRFS_IOC_ADD_DEV _IOW(BTRFS_IOCTL_MAGIC, 10, \
 				   struct btrfs_ioctl_vol_args)
-#define BTRFS_IOC_RM_DEV _IOW(BTRFS_IOCTL_MAGIC, 11, \
+#define BTRFS_IOC_RM_DEV _IOWR(BTRFS_IOCTL_MAGIC, 11, \
 				   struct btrfs_ioctl_vol_args)
 #define BTRFS_IOC_BALANCE _IOW(BTRFS_IOCTL_MAGIC, 12, \
 				   struct btrfs_ioctl_vol_args)
-- 
1.8.1.227.g44fe835

Re: device delete to get errors from the kernel

[Stefan Behrens]

On Fri, 26 Apr 2013 17:39:16 +0800, Anand Jain wrote:
> 
>  As showed in the previous email in this thread, we need to get
>  the error string from the kernel to the cli to improve the
>  usability of the product.
>  As also said, I was looking at two way which I think we could
>  do this, here I take the 2nd approach which is to pass the error
>  string though the ioctl args. Which leads to change in the
>  ioctl-structure. But..
> 
>  This comes with a caveat that both btrfs-progs and btrfs-kernel
>  patch together either must be applied or removed.
> 
>   [PATCH] btrfs-progs: device delete to get errors from the kernel
>   [PATCH] btrfs: device delete to get errors from the kernel
> 
>  Which is not a developer/integrator friendly rule. If there
>  is anything I could do to help on this pls do let me know.
> 
>  There are quite a number of cli which needs passing of the
>  the error string from the kernel to cli. Which I plan to work
>  once we finalize the approach to address this issue.
> 
>  Thanks for your time to review this.

The regular procedure is to define an enumeration of numerical error
codes. And in user mode, somewhere in the btrfslib you add a function to
map the numerical error code to a string. I mean, you don't export a
string in first place, you export something numerical that allows
applications to evaluate the specific error code.

One use case where numerical error codes are better would be that a user
friendly GUI replaces the kernel developer assigned error strings with
friendly error strings for real human beings, maybe including advices,
how to avoid that error, how to go on.

About the issue you mentioned, that you need to change the kernel and
the user mode at the same time:
You can keep it compatible. Just do not delete the old kernel interface.
The user mode program could try the new interface first, and if it
fails, fall back to the old interface. You can use the same ioctl number
for both ways if you change the length of the ioctl parameter.

Re: [PATCH] btrfs: device delete to get errors from the kernel

[Josef Bacik]

On Fri, Apr 26, 2013 at 03:41:15AM -0600, Anand Jain wrote:
> adds a parameter in the ioctl arg struct to carry the error string
> 

I don't like this approach, we are API'ifying strings coming back from the
kernel.

> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/ioctl.c           | 42 +++++++++++++++++++++++++++---------------
>  fs/btrfs/volumes.c         | 29 +++++++++++++++--------------
>  fs/btrfs/volumes.h         |  2 +-
>  include/uapi/linux/btrfs.h |  9 ++++++++-
>  4 files changed, 51 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 898c572..da9cba5 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2337,7 +2337,8 @@ out:
>  static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
i>  {
>  	struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
> -	struct btrfs_ioctl_vol_args *vol_args;
> +	struct btrfs_ioctl_dev_args *dev_args = NULL;
> +	char ret_err[BTRFS_IOCTL_ERR_LEN];
>  	int ret;
>  
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -2347,27 +2348,38 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
>  	if (ret)
>  		return ret;
>  
> -	if (atomic_xchg(&root->fs_info->mutually_exclusive_operation_running,
> -			1)) {
> -		pr_info("btrfs: dev add/delete/balance/replace/resize operation in progress\n");
> -		mnt_drop_write_file(file);
> -		return -EINVAL;
> +	dev_args = memdup_user(arg, sizeof(*dev_args));
> +	if (IS_ERR(dev_args)) {
> +		ret = PTR_ERR(dev_args);
> +		goto out;
>  	}
>  
> -	mutex_lock(&root->fs_info->volume_mutex);
> -	vol_args = memdup_user(arg, sizeof(*vol_args));
> -	if (IS_ERR(vol_args)) {
> -		ret = PTR_ERR(vol_args);
> +	dev_args->name[BTRFS_PATH_NAME_MAX] = '\0';
> +	memset(ret_err, 0, BTRFS_IOCTL_ERR_LEN);
> +
> +	if (atomic_xchg(&root->fs_info->mutually_exclusive_operation_running,
> +			1)) {
> +		strncpy(dev_args->ret_err_str,
> +			"add/delete/balance/replace/resize operation in progress\n",
> +			BTRFS_IOCTL_ERR_LEN);
> +		if (copy_to_user(arg, dev_args, sizeof(dev_args)))
> +			ret = -EFAULT;
> +		ret = -EINVAL;
>  		goto out;
>  	}
>  
> -	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
> -	ret = btrfs_rm_device(root, vol_args->name);
> -
> -	kfree(vol_args);
> -out:
> +	mutex_lock(&root->fs_info->volume_mutex);
> +	ret = btrfs_rm_device(root, dev_args->name, ret_err);
>  	mutex_unlock(&root->fs_info->volume_mutex);
>  	atomic_set(&root->fs_info->mutually_exclusive_operation_running, 0);
> +	if (ret) {
> +		strncpy(dev_args->ret_err_str, ret_err,
> +			BTRFS_IOCTL_ERR_LEN);
> +		if (copy_to_user(arg, dev_args, sizeof(*dev_args)))
> +			ret = -EFAULT;
> +	}
> +out:
> +	kfree(dev_args);
>  	mnt_drop_write_file(file);
>  	return ret;
>  }
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 5989a92..12c02d7 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1426,7 +1426,7 @@ out:
>  	return ret;
>  }
>  
> -int btrfs_rm_device(struct btrfs_root *root, char *device_path)
> +int btrfs_rm_device(struct btrfs_root *root, char *device_path, char *ret_err)
>  {
>  	struct btrfs_device *device;
>  	struct btrfs_device *next_device;
> @@ -1461,30 +1461,30 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
>  	btrfs_dev_replace_unlock(&root->fs_info->dev_replace);
>  
>  	if ((all_avail & BTRFS_BLOCK_GROUP_RAID10) && num_devices <= 4) {
> -		printk(KERN_ERR "btrfs: unable to go below four devices "
> -		       "on raid10\n");
> +		strncpy(ret_err, "unable to go below four devices on raid10",
> +				BTRFS_IOCTL_ERR_LEN);
>  		ret = -EINVAL;
>  		goto out;
>  	}
>  
>  	if ((all_avail & BTRFS_BLOCK_GROUP_RAID1) && num_devices <= 2) {
> -		printk(KERN_ERR "btrfs: unable to go below two "
> -		       "devices on raid1\n");
> +		strncpy(ret_err, "unable to go below two devices on raid1",
> +				BTRFS_IOCTL_ERR_LEN);
>  		ret = -EINVAL;
>  		goto out;
>  	}
>  
>  	if ((all_avail & BTRFS_BLOCK_GROUP_RAID5) &&
>  	    root->fs_info->fs_devices->rw_devices <= 2) {
> -		printk(KERN_ERR "btrfs: unable to go below two "
> -		       "devices on raid5\n");
> +		strncpy(ret_err, "unable to go below two devices on raid5",
> +				BTRFS_IOCTL_ERR_LEN);
>  		ret = -EINVAL;
>  		goto out;
>  	}
>  	if ((all_avail & BTRFS_BLOCK_GROUP_RAID6) &&
>  	    root->fs_info->fs_devices->rw_devices <= 3) {
> -		printk(KERN_ERR "btrfs: unable to go below three "
> -		       "devices on raid6\n");
> +		strncpy(ret_err, "unable to go below three devices on raid6",
> +				BTRFS_IOCTL_ERR_LEN);
>  		ret = -EINVAL;
>  		goto out;
>  	}
> @@ -1511,8 +1511,8 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
>  		bh = NULL;
>  		disk_super = NULL;
>  		if (!device) {
> -			printk(KERN_ERR "btrfs: no missing devices found to "
> -			       "remove\n");
> +			strncpy(ret_err, "no missing devices found to remove",
> +				BTRFS_IOCTL_ERR_LEN);
>  			goto out;
>  		}
>  	} else {
> @@ -1534,14 +1534,15 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
>  	}
>  
>  	if (device->is_tgtdev_for_dev_replace) {
> -		pr_err("btrfs: unable to remove the dev_replace target dev\n");
> +		strncpy(ret_err, "unable to remove the dev_replace target dev",
> +			BTRFS_IOCTL_ERR_LEN);
>  		ret = -EINVAL;
>  		goto error_brelse;
>  	}
>  
>  	if (device->writeable && root->fs_info->fs_devices->rw_devices == 1) {
> -		printk(KERN_ERR "btrfs: unable to remove the only writeable "
> -		       "device\n");
> +		strncpy(ret_err, "unable to remove the only writeable device",
> +			BTRFS_IOCTL_ERR_LEN);
>  		ret = -EINVAL;
>  		goto error_brelse;
>  	}
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 062d860..94dc9f2 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -287,7 +287,7 @@ int btrfs_find_device_by_path(struct btrfs_root *root, char *device_path,
>  int btrfs_add_device(struct btrfs_trans_handle *trans,
>  		     struct btrfs_root *root,
>  		     struct btrfs_device *device);
> -int btrfs_rm_device(struct btrfs_root *root, char *device_path);
> +int btrfs_rm_device(struct btrfs_root *root, char *device_path, char *ret_err);
>  void btrfs_cleanup_fs_uuids(void);
>  int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
>  int btrfs_grow_device(struct btrfs_trans_handle *trans,
> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> index fa3a5f9..b2fc716 100644
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -31,6 +31,13 @@ struct btrfs_ioctl_vol_args {
>  	char name[BTRFS_PATH_NAME_MAX + 1];
>  };
>  
> +#define BTRFS_IOCTL_ERR_LEN 256
> +struct btrfs_ioctl_dev_args {
> +	__s64 fd;
> +	char name[BTRFS_PATH_NAME_MAX + 1];
> +	char ret_err_str[BTRFS_IOCTL_ERR_LEN];
> +};
> +
>  #define BTRFS_DEVICE_PATH_NAME_MAX 1024
>  
>  #define BTRFS_SUBVOL_CREATE_ASYNC	(1ULL << 0)
> @@ -442,7 +449,7 @@ struct btrfs_ioctl_send_args {
>  #define BTRFS_IOC_CLONE        _IOW(BTRFS_IOCTL_MAGIC, 9, int)
>  #define BTRFS_IOC_ADD_DEV _IOW(BTRFS_IOCTL_MAGIC, 10, \
>  				   struct btrfs_ioctl_vol_args)
> -#define BTRFS_IOC_RM_DEV _IOW(BTRFS_IOCTL_MAGIC, 11, \
> +#define BTRFS_IOC_RM_DEV _IOWR(BTRFS_IOCTL_MAGIC, 11, \
>  				   struct btrfs_ioctl_vol_args)

And I may be braindead from the weeks of bugfixing, but you change this to take
btrfs_ioctl_dev_args but you haven't actually changed it here in the _IOWR.  So
no this is a pretty solid ioctl(), it's been around forever, we're not changing
its signature to send back strings.  If you want to make this give you more
specific errors then adjust the errno's we send back and make the userspace util
translate those to their appropriate error.  Thanks,

Josef

Re: [PATCH] btrfs: device delete to get errors from the kernel

[Anand Jain]

>  If you want to make this give you more
> specific errors then adjust the errno's we send back and make the userspace util
> translate those to their appropriate error.  Thanks,

  Thats the approach #1 as listed before in this mail thread,
  let me give a try.

Anand

Re: device delete to get errors from the kernel

[Anand Jain]

  Thanks Stefan.

::

> About the issue you mentioned, that you need to change the kernel and
> the user mode at the same time:
> You can keep it compatible. Just do not delete the old kernel interface.
> The user mode program could try the new interface first, and if it
> fails, fall back to the old interface. You can use the same ioctl number
> for both ways if you change the length of the ioctl parameter.

  Here there won't be a definite failure as such,
  kernel could send or read garbage if the old and
  new structures don't match.

  The problem with most of the ioctl api structures
  are they don't (already) share the struct len. So
  it won't be possible to know which version of the
  structure is being used by the other party?

Anand

Re: device delete to get errors from the kernel

[Anand Jain]

   Thanks for the comments. V2 is out. Here I followed
   the choice #1 as review suggested. which is to update
   the error code. However I have them in uapi/linux/btrfs.h
   (not in errno.h though I meant it to be there in the first
   place).

   pls do let me know your review comments.

Thanks, Anand


On 04/26/2013 05:39 PM, Anand Jain wrote:
>
>   As showed in the previous email in this thread, we need to get
>   the error string from the kernel to the cli to improve the
>   usability of the product.
>   As also said, I was looking at two way which I think we could
>   do this, here I take the 2nd approach which is to pass the error
>   string though the ioctl args. Which leads to change in the
>   ioctl-structure. But..
>
>   This comes with a caveat that both btrfs-progs and btrfs-kernel
>   patch together either must be applied or removed.
>
>    [PATCH] btrfs-progs: device delete to get errors from the kernel
>    [PATCH] btrfs: device delete to get errors from the kernel
>
>   Which is not a developer/integrator friendly rule. If there
>   is anything I could do to help on this pls do let me know.
>
>   There are quite a number of cli which needs passing of the
>   the error string from the kernel to cli. Which I plan to work
>   once we finalize the approach to address this issue.
>
>   Thanks for your time to review this.
>
> Anand

[PATCH v2] btrfs: device delete to get errors from the kernel

[Anand Jain]

v1->v2:
introduce error codes for the device mgmt usage

v1:
adds a parameter in the ioctl arg struct to carry the error string

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ioctl.c           | 22 +++++++++++-----------
 fs/btrfs/volumes.c         | 26 +++++++-------------------
 include/uapi/linux/btrfs.h | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 30 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 898c572..e6d21ec 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2347,14 +2347,6 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 	if (ret)
 		return ret;
 
-	if (atomic_xchg(&root->fs_info->mutually_exclusive_operation_running,
-			1)) {
-		pr_info("btrfs: dev add/delete/balance/replace/resize operation in progress\n");
-		mnt_drop_write_file(file);
-		return -EINVAL;
-	}
-
-	mutex_lock(&root->fs_info->volume_mutex);
 	vol_args = memdup_user(arg, sizeof(*vol_args));
 	if (IS_ERR(vol_args)) {
 		ret = PTR_ERR(vol_args);
@@ -2362,12 +2354,20 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 	}
 
 	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
-	ret = btrfs_rm_device(root, vol_args->name);
 
-	kfree(vol_args);
-out:
+	if (atomic_xchg(&root->fs_info->mutually_exclusive_operation_running,
+			1)) {
+		ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
+		goto out;
+	}
+
+	mutex_lock(&root->fs_info->volume_mutex);
+	ret = btrfs_rm_device(root, vol_args->name);
 	mutex_unlock(&root->fs_info->volume_mutex);
 	atomic_set(&root->fs_info->mutually_exclusive_operation_running, 0);
+
+out:
+	kfree(vol_args);
 	mnt_drop_write_file(file);
 	return ret;
 }
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5989a92..22c702e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1461,31 +1461,23 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 	btrfs_dev_replace_unlock(&root->fs_info->dev_replace);
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID10) && num_devices <= 4) {
-		printk(KERN_ERR "btrfs: unable to go below four devices "
-		       "on raid10\n");
-		ret = -EINVAL;
+		ret = BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET;
 		goto out;
 	}
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID1) && num_devices <= 2) {
-		printk(KERN_ERR "btrfs: unable to go below two "
-		       "devices on raid1\n");
-		ret = -EINVAL;
+		ret = BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET;
 		goto out;
 	}
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID5) &&
 	    root->fs_info->fs_devices->rw_devices <= 2) {
-		printk(KERN_ERR "btrfs: unable to go below two "
-		       "devices on raid5\n");
-		ret = -EINVAL;
+		ret = BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET;
 		goto out;
 	}
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID6) &&
 	    root->fs_info->fs_devices->rw_devices <= 3) {
-		printk(KERN_ERR "btrfs: unable to go below three "
-		       "devices on raid6\n");
-		ret = -EINVAL;
+		ret = BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET;
 		goto out;
 	}
 
@@ -1511,8 +1503,7 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 		bh = NULL;
 		disk_super = NULL;
 		if (!device) {
-			printk(KERN_ERR "btrfs: no missing devices found to "
-			       "remove\n");
+			ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
 			goto out;
 		}
 	} else {
@@ -1534,15 +1525,12 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 	}
 
 	if (device->is_tgtdev_for_dev_replace) {
-		pr_err("btrfs: unable to remove the dev_replace target dev\n");
-		ret = -EINVAL;
+		ret = BTRFS_ERROR_DEV_TGT_REPLACE;
 		goto error_brelse;
 	}
 
 	if (device->writeable && root->fs_info->fs_devices->rw_devices == 1) {
-		printk(KERN_ERR "btrfs: unable to remove the only writeable "
-		       "device\n");
-		ret = -EINVAL;
+		ret = BTRFS_ERROR_DEV_ONLY_WRITABLE;
 		goto error_brelse;
 	}
 
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index fa3a5f9..dcde338 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -511,4 +511,40 @@ struct btrfs_ioctl_send_args {
 #define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
 				    struct btrfs_ioctl_dev_replace_args)
 
+enum btrfs_err_code {
+	notused,
+	BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_TGT_REPLACE,
+	BTRFS_ERROR_DEV_MISSING_NOT_FOUND,
+	BTRFS_ERROR_DEV_ONLY_WRITABLE,
+	BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS
+};
+
+static inline char *btrfs_err_str(enum btrfs_err_code err_code)
+{
+	switch (err_code) {
+		case BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET:
+			return "unable to go below two devices on raid1";
+		case BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET:
+			return "unable to go below four devices on raid10";
+		case BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET:
+			return "unable to go below two devices on raid5";
+		case BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET:
+			return "unable to go below three devices on raid6";
+		case BTRFS_ERROR_DEV_TGT_REPLACE:
+			return "unable to remove the dev_replace target dev";
+		case BTRFS_ERROR_DEV_MISSING_NOT_FOUND:
+			return "no missing devices found to remove";
+		case BTRFS_ERROR_DEV_ONLY_WRITABLE:
+			return "unable to remove the only writeable device";
+		case BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS:
+			return "add/delete/balance/replace/resize operation "\
+				"in progress";
+		default:
+			return NULL;
+	}
+}
 #endif /* _UAPI_LINUX_BTRFS_H */
-- 
1.8.1.227.g44fe835

[PATCH v2] btrfs-progs: device delete to get errors from the kernel

[Anand Jain]

v1->v2:
introduce error codes for the device mgmt usage

v1:
add another parameter to the ioctl arg structure to carry the error string

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-device.c | 10 ++++++++--
 ioctl.h       | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/cmds-device.c b/cmds-device.c
index 41e79d3..bc81937 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -163,8 +163,14 @@ static int cmd_rm_dev(int argc, char **argv)
 		strncpy_null(arg.name, argv[i]);
 		res = ioctl(fdmnt, BTRFS_IOC_RM_DEV, &arg);
 		e = errno;
-		if(res<0){
-			fprintf(stderr, "ERROR: error removing the device '%s' - %s\n",
+		if (res > 0) {
+			fprintf(stderr,
+				"ERROR: error removing the device '%s' - %s\n",
+				argv[i], btrfs_err_str(res));
+			ret++;
+		} else if (res < 0) {
+			fprintf(stderr,
+				"ERROR: error removing the device '%s' - %s\n",
 				argv[i], strerror(e));
 			ret++;
 		}
diff --git a/ioctl.h b/ioctl.h
index 1ee631a..63d5831 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -537,6 +537,43 @@ struct btrfs_ioctl_clone_range_args {
 #define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
 				    struct btrfs_ioctl_dev_replace_args)
 
+enum btrfs_err_code {
+	notused,
+	BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_TGT_REPLACE,
+	BTRFS_ERROR_DEV_MISSING_NOT_FOUND,
+	BTRFS_ERROR_DEV_ONLY_WRITABLE,
+	BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS
+};
+
+static inline char *btrfs_err_str(enum btrfs_err_code err_code)
+{
+	switch (err_code) {
+		case BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET:
+			return "unable to go below two devices on raid1";
+		case BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET:
+			return "unable to go below four devices on raid10";
+		case BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET:
+			return "unable to go below two devices on raid5";
+		case BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET:
+			return "unable to go below three devices on raid6";
+		case BTRFS_ERROR_DEV_TGT_REPLACE:
+			return "unable to remove the dev_replace target dev";
+		case BTRFS_ERROR_DEV_MISSING_NOT_FOUND:
+			return "no missing devices found to remove";
+		case BTRFS_ERROR_DEV_ONLY_WRITABLE:
+			return "unable to remove the only writeable device";
+		case BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS:
+			return "add/delete/balance/replace/resize operation "\
+				"in progress";
+		default:
+			return NULL;
+	}
+}
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.8.1.227.g44fe835

Re: [PATCH v2] btrfs: device delete to get errors from the kernel

[Josef Bacik]

On Tue, Apr 30, 2013 at 07:19:40AM -0600, Anand Jain wrote:
> v1->v2:
> introduce error codes for the device mgmt usage
> 
> v1:
> adds a parameter in the ioctl arg struct to carry the error string
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---

I need a proper log for this patch.  Thanks,

Josef

[PATCH v3] btrfs: device delete to get errors from the kernel

[Anand Jain]

when user runs command btrfs dev del the raid requisite error if any
goes to the /var/log/messages, its not good idea to clutter messages
with these user (knowledge) errors, further user don't have to review
the system messages to know problem with the cli it should be dropped
to the user as part of the cli return.

to bring this feature created a set of the ERROR defined
BTRFS_ERROR_DEV* error codes and created their error string.

I expect this enum to be added with other error which we might
want to communicate to the user land

v3:
moved the code with in the file no logical change

v1->v2:
introduce error codes for the device mgmt usage

v1:
adds a parameter in the ioctl arg struct to carry the error string

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ioctl.c           | 22 +++++++++++-----------
 fs/btrfs/volumes.c         | 26 +++++++-------------------
 include/uapi/linux/btrfs.h | 41 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 58 insertions(+), 31 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 898c572..e6d21ec 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2347,14 +2347,6 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 	if (ret)
 		return ret;
 
-	if (atomic_xchg(&root->fs_info->mutually_exclusive_operation_running,
-			1)) {
-		pr_info("btrfs: dev add/delete/balance/replace/resize operation in progress\n");
-		mnt_drop_write_file(file);
-		return -EINVAL;
-	}
-
-	mutex_lock(&root->fs_info->volume_mutex);
 	vol_args = memdup_user(arg, sizeof(*vol_args));
 	if (IS_ERR(vol_args)) {
 		ret = PTR_ERR(vol_args);
@@ -2362,12 +2354,20 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 	}
 
 	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
-	ret = btrfs_rm_device(root, vol_args->name);
 
-	kfree(vol_args);
-out:
+	if (atomic_xchg(&root->fs_info->mutually_exclusive_operation_running,
+			1)) {
+		ret = BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
+		goto out;
+	}
+
+	mutex_lock(&root->fs_info->volume_mutex);
+	ret = btrfs_rm_device(root, vol_args->name);
 	mutex_unlock(&root->fs_info->volume_mutex);
 	atomic_set(&root->fs_info->mutually_exclusive_operation_running, 0);
+
+out:
+	kfree(vol_args);
 	mnt_drop_write_file(file);
 	return ret;
 }
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5989a92..22c702e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1461,31 +1461,23 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 	btrfs_dev_replace_unlock(&root->fs_info->dev_replace);
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID10) && num_devices <= 4) {
-		printk(KERN_ERR "btrfs: unable to go below four devices "
-		       "on raid10\n");
-		ret = -EINVAL;
+		ret = BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET;
 		goto out;
 	}
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID1) && num_devices <= 2) {
-		printk(KERN_ERR "btrfs: unable to go below two "
-		       "devices on raid1\n");
-		ret = -EINVAL;
+		ret = BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET;
 		goto out;
 	}
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID5) &&
 	    root->fs_info->fs_devices->rw_devices <= 2) {
-		printk(KERN_ERR "btrfs: unable to go below two "
-		       "devices on raid5\n");
-		ret = -EINVAL;
+		ret = BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET;
 		goto out;
 	}
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID6) &&
 	    root->fs_info->fs_devices->rw_devices <= 3) {
-		printk(KERN_ERR "btrfs: unable to go below three "
-		       "devices on raid6\n");
-		ret = -EINVAL;
+		ret = BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET;
 		goto out;
 	}
 
@@ -1511,8 +1503,7 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 		bh = NULL;
 		disk_super = NULL;
 		if (!device) {
-			printk(KERN_ERR "btrfs: no missing devices found to "
-			       "remove\n");
+			ret = BTRFS_ERROR_DEV_MISSING_NOT_FOUND;
 			goto out;
 		}
 	} else {
@@ -1534,15 +1525,12 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 	}
 
 	if (device->is_tgtdev_for_dev_replace) {
-		pr_err("btrfs: unable to remove the dev_replace target dev\n");
-		ret = -EINVAL;
+		ret = BTRFS_ERROR_DEV_TGT_REPLACE;
 		goto error_brelse;
 	}
 
 	if (device->writeable && root->fs_info->fs_devices->rw_devices == 1) {
-		printk(KERN_ERR "btrfs: unable to remove the only writeable "
-		       "device\n");
-		ret = -EINVAL;
+		ret = BTRFS_ERROR_DEV_ONLY_WRITABLE;
 		goto error_brelse;
 	}
 
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index fa3a5f9..408374a 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -423,6 +423,46 @@ struct btrfs_ioctl_send_args {
 	__u64 reserved[4];		/* in */
 };
 
+/* Error codes as returned by the kernel */
+enum btrfs_err_code {
+	notused,
+	BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_TGT_REPLACE,
+	BTRFS_ERROR_DEV_MISSING_NOT_FOUND,
+	BTRFS_ERROR_DEV_ONLY_WRITABLE,
+	BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS
+};
+/* An error code to error string mapping for the kernel
+*  error codes
+*/
+static inline char *btrfs_err_str(enum btrfs_err_code err_code)
+{
+	switch (err_code) {
+		case BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET:
+			return "unable to go below two devices on raid1";
+		case BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET:
+			return "unable to go below four devices on raid10";
+		case BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET:
+			return "unable to go below two devices on raid5";
+		case BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET:
+			return "unable to go below three devices on raid6";
+		case BTRFS_ERROR_DEV_TGT_REPLACE:
+			return "unable to remove the dev_replace target dev";
+		case BTRFS_ERROR_DEV_MISSING_NOT_FOUND:
+			return "no missing devices found to remove";
+		case BTRFS_ERROR_DEV_ONLY_WRITABLE:
+			return "unable to remove the only writeable device";
+		case BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS:
+			return "add/delete/balance/replace/resize operation "\
+				"in progress";
+		default:
+			return NULL;
+	}
+}
+
 #define BTRFS_IOC_SNAP_CREATE _IOW(BTRFS_IOCTL_MAGIC, 1, \
 				   struct btrfs_ioctl_vol_args)
 #define BTRFS_IOC_DEFRAG _IOW(BTRFS_IOCTL_MAGIC, 2, \
@@ -510,5 +550,4 @@ struct btrfs_ioctl_send_args {
 				      struct btrfs_ioctl_get_dev_stats)
 #define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
 				    struct btrfs_ioctl_dev_replace_args)
-
 #endif /* _UAPI_LINUX_BTRFS_H */
-- 
1.8.1.227.g44fe835

[PATCH v3] btrfs-progs: device delete to get errors from the kernel

[Anand Jain]

when user runs command btrfs dev del the raid requisite error if any
goes to the /var/log/messages, its not good idea to clutter messages
with these user (knowledge) errors, further user don't have to review
the system messages to know problem with the cli it should be dropped
to the user as part of the cli return.

to bring this feature created a set of the ERROR defined
BTRFS_ERROR_DEV* error codes and created their error string.

I expect this enum to be added with other error which we might
want to communicate to the user land

v3:
moved the code with in the file no logical change

v1->v2:
introduce error codes for the device mgmt usage

v1:
add another parameter to the ioctl arg structure to carry the error string

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-device.c | 10 ++++++++--
 ioctl.h       | 43 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/cmds-device.c b/cmds-device.c
index 41e79d3..bc81937 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -163,8 +163,14 @@ static int cmd_rm_dev(int argc, char **argv)
 		strncpy_null(arg.name, argv[i]);
 		res = ioctl(fdmnt, BTRFS_IOC_RM_DEV, &arg);
 		e = errno;
-		if(res<0){
-			fprintf(stderr, "ERROR: error removing the device '%s' - %s\n",
+		if (res > 0) {
+			fprintf(stderr,
+				"ERROR: error removing the device '%s' - %s\n",
+				argv[i], btrfs_err_str(res));
+			ret++;
+		} else if (res < 0) {
+			fprintf(stderr,
+				"ERROR: error removing the device '%s' - %s\n",
 				argv[i], strerror(e));
 			ret++;
 		}
diff --git a/ioctl.h b/ioctl.h
index 1ee631a..897dd26 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -441,6 +441,48 @@ struct btrfs_ioctl_qgroup_create_args {
 	__u64 create;
 	__u64 qgroupid;
 };
+
+/* Error codes as returned by the kernel */
+enum btrfs_err_code {
+	notused,
+	BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET,
+	BTRFS_ERROR_DEV_TGT_REPLACE,
+	BTRFS_ERROR_DEV_MISSING_NOT_FOUND,
+	BTRFS_ERROR_DEV_ONLY_WRITABLE,
+	BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS
+};
+
+/* An error code to error string mapping for the kernel
+*  error codes
+*/
+static inline char *btrfs_err_str(enum btrfs_err_code err_code)
+{
+	switch (err_code) {
+		case BTRFS_ERROR_DEV_RAID1_MIN_NOT_MET:
+			return "unable to go below two devices on raid1";
+		case BTRFS_ERROR_DEV_RAID10_MIN_NOT_MET:
+			return "unable to go below four devices on raid10";
+		case BTRFS_ERROR_DEV_RAID5_MIN_NOT_MET:
+			return "unable to go below two devices on raid5";
+		case BTRFS_ERROR_DEV_RAID6_MIN_NOT_MET:
+			return "unable to go below three devices on raid6";
+		case BTRFS_ERROR_DEV_TGT_REPLACE:
+			return "unable to remove the dev_replace target dev";
+		case BTRFS_ERROR_DEV_MISSING_NOT_FOUND:
+			return "no missing devices found to remove";
+		case BTRFS_ERROR_DEV_ONLY_WRITABLE:
+			return "unable to remove the only writeable device";
+		case BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS:
+			return "add/delete/balance/replace/resize operation "\
+				"in progress";
+		default:
+			return NULL;
+	}
+}
+
 #define BTRFS_IOC_SNAP_CREATE _IOW(BTRFS_IOCTL_MAGIC, 1, \
 				   struct btrfs_ioctl_vol_args)
 #define BTRFS_IOC_DEFRAG _IOW(BTRFS_IOCTL_MAGIC, 2, \
@@ -536,7 +578,6 @@ struct btrfs_ioctl_clone_range_args {
 				      struct btrfs_ioctl_get_dev_stats)
 #define BTRFS_IOC_DEV_REPLACE _IOWR(BTRFS_IOCTL_MAGIC, 53, \
 				    struct btrfs_ioctl_dev_replace_args)
-
 #ifdef __cplusplus
 }
 #endif
-- 
1.8.1.227.g44fe835