From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp1040.oracle.com ([156.151.31.81]:35051 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750959AbbFDDJK (ORCPT ); Wed, 3 Jun 2015 23:09:10 -0400 Message-ID: <556FC14E.1030700@oracle.com> Date: Thu, 04 Jun 2015 11:09:02 +0800 From: Anand Jain MIME-Version: 1.0 To: dsterba@suse.cz, linux-btrfs@vger.kernel.org Subject: Re: [PATCH 1/2] btrfs-progs: avoid duplicate checks on user provided devid References: <1433139918-820-1-git-send-email-anand.jain@oracle.com> <20150602151234.GH6761@twin.jikos.cz> <556EB19D.6010807@oracle.com> In-Reply-To: <556EB19D.6010807@oracle.com> Content-Type: text/plain; charset=windows-1252; format=flowed Sender: linux-btrfs-owner@vger.kernel.org List-ID: 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 ..' and 'btrfs replace start .." > 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 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