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

Message ID 1433139918-820-1-git-send-email-anand.jain@oracle.com
State New
Headers show

Commit Message

Anand Jain June 1, 2015, 6:25 a.m. UTC
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(-)

Comments

David Sterba June 2, 2015, 3:12 p.m. UTC | #1
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.
--
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
Anand Jain June 3, 2015, 7:49 a.m. UTC | #2
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
Anand Jain June 4, 2015, 3:09 a.m. UTC | #3
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
--
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
David Sterba June 5, 2015, 3:36 p.m. UTC | #4
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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