| Message ID | 1363867004-22912-1-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | Under Review, archived |
| Headers | show |
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
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
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
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
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);
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(-)