[PATCH 1/2] btrfs-progs: avoid duplicate checks on user provided devid

[PATCH 1/2] btrfs-progs: avoid duplicate checks on user provided devid

[Anand Jain]

kernel is already checking it (rightly), we don't need to check that in the user land.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-replace.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/cmds-replace.c b/cmds-replace.c
index 6ea7c61..0787ce3 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -118,7 +118,6 @@ static int cmd_start_replace(int argc, char **argv)
 	struct btrfs_ioctl_dev_replace_args start_args = {0};
 	struct btrfs_ioctl_dev_replace_args status_args = {0};
 	int ret;
-	int i;
 	int c;
 	int fdmnt = -1;
 	int fdsrcdev = -1;
@@ -214,33 +213,7 @@ static int cmd_start_replace(int argc, char **argv)
 	}
 
 	if (is_numerical(srcdev)) {
-		struct btrfs_ioctl_fs_info_args fi_args;
-		struct btrfs_ioctl_dev_info_args *di_args = NULL;
-
 		start_args.start.srcdevid = arg_strtou64(srcdev);
-
-		ret = get_fs_info(path, &fi_args, &di_args);
-		if (ret) {
-			fprintf(stderr, "ERROR: getting dev info for devstats failed: "
-					"%s\n", strerror(-ret));
-			free(di_args);
-			goto leave_with_error;
-		}
-		if (!fi_args.num_devices) {
-			fprintf(stderr, "ERROR: no devices found\n");
-			free(di_args);
-			goto leave_with_error;
-		}
-
-		for (i = 0; i < fi_args.num_devices; i++)
-			if (start_args.start.srcdevid == di_args[i].devid)
-				break;
-		free(di_args);
-		if (i == fi_args.num_devices) {
-			fprintf(stderr, "Error: '%s' is not a valid devid for filesystem '%s'\n",
-				srcdev, path);
-			goto leave_with_error;
-		}
 	} else {
 		fdsrcdev = open(srcdev, O_RDWR);
 		if (fdsrcdev < 0) {
-- 
2.4.1

[PATCH 2/2] btrfs-progs: use function is_block_device() instead

[Anand Jain]

Here the delete code as below, is trying to check if the provided device
is a block device, there is a function for it. use it.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-replace.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/cmds-replace.c b/cmds-replace.c
index 0787ce3..e6bc27e 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -120,14 +120,12 @@ static int cmd_start_replace(int argc, char **argv)
 	int ret;
 	int c;
 	int fdmnt = -1;
-	int fdsrcdev = -1;
 	int fddstdev = -1;
 	char *path;
 	char *srcdev;
 	char *dstdev = NULL;
 	int avoid_reading_from_srcdev = 0;
 	int force_using_targetdev = 0;
-	struct stat st;
 	u64 dstdev_block_count;
 	int do_not_background = 0;
 	int mixed = 0;
@@ -214,28 +212,9 @@ static int cmd_start_replace(int argc, char **argv)
 
 	if (is_numerical(srcdev)) {
 		start_args.start.srcdevid = arg_strtou64(srcdev);
-	} else {
-		fdsrcdev = open(srcdev, O_RDWR);
-		if (fdsrcdev < 0) {
-			fprintf(stderr, "Error: Unable to open device '%s'\n",
-				srcdev);
-			fprintf(stderr, "\tTry using the devid instead of the path\n");
-			goto leave_with_error;
-		}
-		ret = fstat(fdsrcdev, &st);
-		if (ret) {
-			fprintf(stderr, "Error: Unable to stat '%s'\n", srcdev);
-			goto leave_with_error;
-		}
-		if (!S_ISBLK(st.st_mode)) {
-			fprintf(stderr, "Error: '%s' is not a block device\n",
-				srcdev);
-			goto leave_with_error;
-		}
+	} else if (is_block_device(srcdev)) {
 		strncpy((char *)start_args.start.srcdev_name, srcdev,
 			BTRFS_DEVICE_PATH_NAME_MAX);
-		close(fdsrcdev);
-		fdsrcdev = -1;
 		start_args.start.srcdevid = 0;
 	}
 
@@ -308,8 +287,6 @@ leave_with_error:
 		free(dstdev);
 	if (fdmnt != -1)
 		close(fdmnt);
-	if (fdsrcdev != -1)
-		close(fdsrcdev);
 	if (fddstdev != -1)
 		close(fddstdev);
 	return 1;
-- 
2.4.1

Re: [PATCH 2/2] btrfs-progs: use function is_block_device() instead

[David Sterba]

On Mon, Jun 01, 2015 at 02:25:18PM +0800, Anand Jain wrote:
> Here the delete code as below, is trying to check if the provided device
> is a block device, there is a function for it. use it.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Applied, thanks.

Re: [PATCH 1/2] btrfs-progs: avoid duplicate checks on user provided devid

[David Sterba]

On Mon, Jun 01, 2015 at 02:25:17PM +0800, Anand Jain wrote:
> kernel is already checking it (rightly), we don't need to check that in the user land.

Sometimes it's useful to duplicate the checks in userspace because we
can fail early and return the error message directly, compared to
messages in syslog or a simple errno.

Have you observed that the userspace checks were problematic?

> @@ -214,33 +213,7 @@ static int cmd_start_replace(int argc, char **argv)
>  	}
>  
>  	if (is_numerical(srcdev)) {
> -		struct btrfs_ioctl_fs_info_args fi_args;
> -		struct btrfs_ioctl_dev_info_args *di_args = NULL;
> -
>  		start_args.start.srcdevid = arg_strtou64(srcdev);
> -
> -		ret = get_fs_info(path, &fi_args, &di_args);

This does additional checks, like checking where it's mounted.

Re: [PATCH 1/2] btrfs-progs: avoid duplicate checks on user provided devid

[Anand Jain]

On 06/02/2015 11:12 PM, David Sterba wrote:
> On Mon, Jun 01, 2015 at 02:25:17PM +0800, Anand Jain wrote:
>> kernel is already checking it (rightly), we don't need to check that in the user land.
>
> Sometimes it's useful to duplicate the checks in userspace because we
> can fail early and return the error message directly, compared to
> messages in syslog or a simple errno.

  right. But here what do you feel about the gain obtained vs additional
  code thats required ?

  My take: get_fs_info() calls check_mounted_where() which in turn calls
  btrfs_scan_lblkid(), which is system wide scan of all block devices,
  thats heavy weight.

  My past tests showed visible delay/jitter in the output of
  'btrfs fi show -d' when btrfs scrub is running. The main culprit
  is check_mounted_where calling btrfs_scan_lblkid. so its recommend
  to run get_fs_info() only if required.

> Have you observed that the userspace checks were problematic?

  Nope. Just a cleanup. With this patch cmd_start_replace() would
  should match with cmd_rm_dev().
  As both 'btrfs dev del <devid> ..' and 'btrfs replace start <devid> .."
  intentions are same, so theoretically up to certain extent their codes
  should match.

>> @@ -214,33 +213,7 @@ static int cmd_start_replace(int argc, char **argv)
>>   	}
>>
>>   	if (is_numerical(srcdev)) {
>> -		struct btrfs_ioctl_fs_info_args fi_args;
>> -		struct btrfs_ioctl_dev_info_args *di_args = NULL;
>> -
>>   		start_args.start.srcdevid = arg_strtou64(srcdev);
>> -
>> -		ret = get_fs_info(path, &fi_args, &di_args);
>
> This does additional checks, like checking where it's mounted.

  in this part of the if statement srcdev is <num> and is devid.
  get_fs_info check does not help.

  What did I miss ?

Thanks Anand

> --
> 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
>

Re: [PATCH 1/2] btrfs-progs: avoid duplicate checks on user provided devid

[Anand Jain]

David,

  Kindly do not integrated this patch as of now.
  As I have just found that, the error reporting when
  the default background option is used is not completely
  transparent. I need to fix this before this patch.

Thanks, Anand


On 06/03/2015 03:49 PM, Anand Jain wrote:
>
> On 06/02/2015 11:12 PM, David Sterba wrote:
>> On Mon, Jun 01, 2015 at 02:25:17PM +0800, Anand Jain wrote:
>>> kernel is already checking it (rightly), we don't need to check that
>>> in the user land.
>>
>> Sometimes it's useful to duplicate the checks in userspace because we
>> can fail early and return the error message directly, compared to
>> messages in syslog or a simple errno.
>
>   right. But here what do you feel about the gain obtained vs additional
>   code thats required ?
>
>   My take: get_fs_info() calls check_mounted_where() which in turn calls
>   btrfs_scan_lblkid(), which is system wide scan of all block devices,
>   thats heavy weight.
>
>   My past tests showed visible delay/jitter in the output of
>   'btrfs fi show -d' when btrfs scrub is running. The main culprit
>   is check_mounted_where calling btrfs_scan_lblkid. so its recommend
>   to run get_fs_info() only if required.
>
>> Have you observed that the userspace checks were problematic?
>
>   Nope. Just a cleanup. With this patch cmd_start_replace() would
>   should match with cmd_rm_dev().
>   As both 'btrfs dev del <devid> ..' and 'btrfs replace start <devid> .."
>   intentions are same, so theoretically up to certain extent their codes
>   should match.
>
>>> @@ -214,33 +213,7 @@ static int cmd_start_replace(int argc, char **argv)
>>>       }
>>>
>>>       if (is_numerical(srcdev)) {
>>> -        struct btrfs_ioctl_fs_info_args fi_args;
>>> -        struct btrfs_ioctl_dev_info_args *di_args = NULL;
>>> -
>>>           start_args.start.srcdevid = arg_strtou64(srcdev);
>>> -
>>> -        ret = get_fs_info(path, &fi_args, &di_args);
>>
>> This does additional checks, like checking where it's mounted.
>
>   in this part of the if statement srcdev is <num> and is devid.
>   get_fs_info check does not help.
>
>   What did I miss ?
>
> Thanks Anand
>
>> --
>> 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
>>
> --
> 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

Re: [PATCH 1/2] btrfs-progs: avoid duplicate checks on user provided devid

[David Sterba]

On Thu, Jun 04, 2015 at 11:09:02AM +0800, Anand Jain wrote:
>   Kindly do not integrated this patch as of now.
>   As I have just found that, the error reporting when
>   the default background option is used is not completely
>   transparent. I need to fix this before this patch.

Understood, thanks for the notice.