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

[Anand Jain <anand.jain@oracle.com>]

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