Anand Jain
2013-Mar-21 11:56 UTC
[PATCH] btrfs-progs: make btrfs dev scan multi path aware
We should avoid using non multi-path (mp) path for mp disks
As of now there is no good way (like api) to check that.
A workaround way is to check if the O_EXCL open is unsuccessful.
This is safe since otherwise the BTRFS_IOC_SCAN_DEV ioctl would
fail if the disk-path can not be opened with the flag O_EXCL set.
This patch also includes some (error) print format changes related
to the btrfs dev scan..
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
cmds-device.c | 53 +++++++++++++++++++++++++++++++++++++++--------------
utils.c | 31 ++++++++++++++++++++++++-------
2 files changed, 63 insertions(+), 21 deletions(-)
diff --git a/cmds-device.c b/cmds-device.c
index 41e79d3..b8d05fd 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -185,7 +185,7 @@ static const char * const cmd_scan_dev_usage[] = {
static int cmd_scan_dev(int argc, char **argv)
{
- int i, fd, e;
+ int i, fd, e, ret = 0;
int checklist = 1;
int devstart = 1;
@@ -197,6 +197,21 @@ static int cmd_scan_dev(int argc, char **argv)
devstart += 1;
}
+ fd = open("/dev/btrfs-control", O_RDWR);
+ e = errno;
+ if (fd < 0) {
+ FILE *mfd = popen("lsmod | grep btrfs", "r");
+ char buf[16];
+
+ if (fread (buf, 1, sizeof (buf), mfd) > 0)
+ fprintf(stderr, "ERROR: failed to open "\
+ "/dev/btrfs-control - %s\n", strerror(e));
+ else
+ fprintf(stderr, "ERROR: btrfs kernel module "\
+ "is not loaded\n");
+ return 10;
+ }
+
if(argc<=devstart){
int ret;
@@ -210,20 +225,30 @@ static int cmd_scan_dev(int argc, char **argv)
fprintf(stderr, "ERROR: error %d while scanning\n", ret);
return 18;
}
+ printf("done\n");
return 0;
}
- fd = open("/dev/btrfs-control", O_RDWR);
- if (fd < 0) {
- perror("failed to open /dev/btrfs-control");
- return 10;
- }
-
+ printf("Scanning for Btrfs in\n");
for( i = devstart ; i < argc ; i++ ){
+ int fdt;
struct btrfs_ioctl_vol_args args;
- int ret;
+ printf(" %s ", argv[i]);
+ fflush(stdout);
- printf("Scanning for Btrfs filesystems in
''%s''\n", argv[i]);
+ /*
+ * If for a multipath (mp) disk user provides the
+ * non-mp path then open with flag O_EXCL will fail,
+ * (also ioctl opens with O_EXCL), So test it before
+ * calling ioctl.
+ */
+ fdt = open(argv[i], O_RDONLY|O_EXCL);
+ if (fdt < 0) {
+ perror("ERROR");
+ ret = -1;
+ continue;
+ }
+ close(fdt);
strncpy_null(args.name, argv[i]);
/*
@@ -235,15 +260,15 @@ static int cmd_scan_dev(int argc, char **argv)
e = errno;
if( ret < 0 ){
- close(fd);
- fprintf(stderr, "ERROR: unable to scan the device
''%s'' - %s\n",
- argv[i], strerror(e));
- return 11;
+ fprintf(stderr, "ERROR: unable to scan - %s\n",
+ strerror(e));
+ ret = -1;
}
+ printf("found\n");
}
close(fd);
- return 0;
+ return ret;
}
static const char * const cmd_ready_dev_usage[] = {
diff --git a/utils.c b/utils.c
index 7b028a7..2fc6c32 100644
--- a/utils.c
+++ b/utils.c
@@ -1105,25 +1105,32 @@ again:
if (!S_ISBLK(st.st_mode)) {
continue;
}
- fd = open(fullpath, O_RDONLY);
+ fd = open(fullpath, O_RDONLY|O_EXCL);
if (fd < 0) {
/* ignore the following errors:
ENXIO (device don''t exists)
ENOMEDIUM (No medium found ->
like a cd tray empty)
+ EBUSY (when mp disk is opened
+ using non-mp path).
*/
- if(errno != ENXIO && errno != ENOMEDIUM)
+ if(errno != ENXIO && errno != ENOMEDIUM &&
+ errno != EBUSY)
fprintf(stderr, "failed to read %s: %s\n",
fullpath, strerror(errno));
continue;
}
+ close(fd);
+
+ fd = open(fullpath, O_RDONLY);
ret = btrfs_scan_one_device(fd, fullpath, &tmp_devices,
&num_devices,
BTRFS_SUPER_INFO_OFFSET);
+ close(fd);
+
if (ret == 0 && run_ioctl > 0) {
btrfs_register_one_device(fullpath);
}
- close(fd);
}
if (!list_empty(&pending_list)) {
free(pending);
@@ -1442,19 +1449,29 @@ scan_again:
continue;
}
- fd = open(fullpath, O_RDONLY);
+ /* This will fail for the multi path devices
+ * when non multipath path is used. So we avoid
+ * sending it to the kernel which eventually will
+ * fail.
+ */
+ fd = open(fullpath, O_RDONLY|O_EXCL);
if (fd < 0) {
- fprintf(stderr, "failed to open %s: %s\n",
- fullpath, strerror(errno));
+ if (errno != EBUSY) {
+ fprintf(stderr, "failed to open %s: %s\n",
+ fullpath, strerror(errno));
+ }
continue;
}
+ close(fd);
+
+ fd = open(fullpath, O_RDONLY);
ret = btrfs_scan_one_device(fd, fullpath, &tmp_devices,
&num_devices,
BTRFS_SUPER_INFO_OFFSET);
+ close(fd);
if (ret == 0 && run_ioctl > 0) {
btrfs_register_one_device(fullpath);
}
- close(fd);
}
fclose(proc_partitions);
--
1.8.1.227.g44fe835
--
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
2013-Apr-08 15:22 UTC
Re: [PATCH] btrfs-progs: make btrfs dev scan multi path aware
On Thu, Mar 21, 2013 at 07:56:44PM +0800, Anand Jain wrote:> We should avoid using non multi-path (mp) path for mp disks > As of now there is no good way (like api) to check that. > A workaround way is to check if the O_EXCL open is unsuccessful. > This is safe since otherwise the BTRFS_IOC_SCAN_DEV ioctl would > fail if the disk-path can not be opened with the flag O_EXCL set.Agreed. Alternatively we could try to parse the /sys entries.> --- a/cmds-device.c > +++ b/cmds-device.c > @@ -185,7 +185,7 @@ static const char * const cmd_scan_dev_usage[] = { > > static int cmd_scan_dev(int argc, char **argv) > { > - int i, fd, e; > + int i, fd, e, ret = 0; > int checklist = 1; > int devstart = 1; > > @@ -197,6 +197,21 @@ static int cmd_scan_dev(int argc, char **argv) > devstart += 1; > } > > + fd = open("/dev/btrfs-control", O_RDWR); > + e = errno; > + if (fd < 0) { > + FILE *mfd = popen("lsmod | grep btrfs", "r");Please transform this into C.> + char buf[16]; > + > + if (fread (buf, 1, sizeof (buf), mfd) > 0) > + fprintf(stderr, "ERROR: failed to open "\ > + "/dev/btrfs-control - %s\n", strerror(e)); > + else > + fprintf(stderr, "ERROR: btrfs kernel module "\ > + "is not loaded\n"); > + return 10;We should not be using the old style of error codes in new code, other paths in this functions return -1 .> + }thanks, david -- 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
2013-Apr-09 11:12 UTC
Re: [PATCH] btrfs-progs: make btrfs dev scan multi path aware
On Mon, Apr 08, 2013 at 05:22:38PM +0200, David Sterba wrote:> > + fd = open("/dev/btrfs-control", O_RDWR); > > + e = errno; > > + if (fd < 0) { > > + FILE *mfd = popen("lsmod | grep btrfs", "r"); > Please transform this into C.Actually, what if btrfs is not built as a module? There should be another way to detect that btrfs is compiled in, like opening the control device and reading something back. david -- 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
2013-Apr-10 03:05 UTC
Re: [PATCH] btrfs-progs: make btrfs dev scan multi path aware
On 04/08/2013 11:22 PM, David Sterba wrote:> On Thu, Mar 21, 2013 at 07:56:44PM +0800, Anand Jain wrote: >> We should avoid using non multi-path (mp) path for mp disks >> As of now there is no good way (like api) to check that. >> A workaround way is to check if the O_EXCL open is unsuccessful. >> This is safe since otherwise the BTRFS_IOC_SCAN_DEV ioctl would >> fail if the disk-path can not be opened with the flag O_EXCL set. > > Agreed. Alternatively we could try to parse the /sys entries.sorry to confuse you on this David. hope the below description will clarify.. this patch actually combined two fixes - one as in the subject here, and the other a small fix which is to check if the kernel module is loaded. the later revised patch separated this into two patch-set - v6: access to backup superblock (dt: 04/05/13) - [PATCH 0/9] a bunch of miscellaneous bug fixes (dt: 04/05/13) in the above v6... as indicated I have dropped the [PATCH] btrfs-progs: make btrfs dev scan multi path aware since its found that when btrfs is mounted it would open the dev with O_EXCL as well, so we can''t depend on this workaround. Further original problem related to the multi-path wasn''t reproducible with my above two patch-sets applied (in the same order) on top integration-20130321 . IMO I lost the trigger as I don''t think there is any fix related to multi path. If there is any good reproducible test-case related to multi-path I would dig further. The above patch set viz. "v6: access to backup superblock" and "[PATCH 0/9] a bunch of miscellaneous bug fixes" are important. They bring a lot of stability around the area of mkfs, btrfs fi show, btrfs dev scan. 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
Anand Jain
2013-Apr-10 03:07 UTC
Re: [PATCH] btrfs-progs: make btrfs dev scan multi path aware
On 04/09/2013 07:12 PM, David Sterba wrote:> On Mon, Apr 08, 2013 at 05:22:38PM +0200, David Sterba wrote: >>> + fd = open("/dev/btrfs-control", O_RDWR); >>> + e = errno; >>> + if (fd < 0) { >>> + FILE *mfd = popen("lsmod | grep btrfs", "r"); >> Please transform this into C. > > Actually, what if btrfs is not built as a module? There should be another > way to detect that btrfs is compiled in, like opening the control device > and reading something back.yeah sorry i missed it. Thanks. Let me followup in the latest patch of this. 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