| Message ID | 201302140753.AA00021@FM-323941448.jp.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Thu, Feb 14, 2013 at 04:53:04PM +0900, Tsutomu Itoh wrote: > Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> Worked for me here in these situations (ie. mkfs failed, filenames specified given as simple path or with mixture of ./ and ../ leading to the device): * mounted /dev/sdx * /dev/sdx part of a multi-device mounted fs * /dev/sdx swap device * file as a swap device * swap files with funny names: \ \040 \134 \888 <space> Tested-by: David Sterba <dsterba@suse.cz> -- 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 2013/02/14 09:53 AM, Tsutomu Itoh wrote: > + if (ret < 0) { > + fprintf(stderr, "error checking %s status: %s\n", file, > + strerror(-ret)); > + exit(1); > + } > > ... > > + /* check if the device is busy */ > + fd = open(file, O_RDWR|O_EXCL); > + if (fd < 0) { > + fprintf(stderr, "unable to open %s: %s\n", file, > + strerror(errno)); > + exit(1); > + } This is fine and works (as tested by David) - but I'm not sure if the below suggestions from Zach were taken into account. 1. If the check with "open(file, O_RDWR|O_EXCL)" shows that the device is available, there's no point in checking if it is mounted as a swap device. A preliminary check using this could precede all other checks which should be skipped if it shows success. 2. If there's an error checking the status (for example lets say /proc/swaps is deprecated), we should print the informational message but not error out. On 2013/02/13 11:58 AM, Zach Brown wrote: > - First always open with O_EXCL. If it succeeds then there's no reason > to check /proc/swaps at all. (Maybe it doesn't need to try > check_mounted() there either? Not sure if it's protecting against > accidentally mounting mounted shared storage or not.) > > ... > > - At no point is failure of any of the /proc/swaps parsing fatal. It'd > carry on ignoring errors until it doesnt have work to do. It'd only > ever print the nice message when it finds a match.
On 2013/03/04 1:47, Brendan Hide wrote: > On 2013/02/14 09:53 AM, Tsutomu Itoh wrote: >> + if (ret < 0) { >> + fprintf(stderr, "error checking %s status: %s\n", file, >> + strerror(-ret)); >> + exit(1); >> + } >> >> ... >> >> + /* check if the device is busy */ >> + fd = open(file, O_RDWR|O_EXCL); >> + if (fd < 0) { >> + fprintf(stderr, "unable to open %s: %s\n", file, >> + strerror(errno)); >> + exit(1); >> + } > This is fine and works (as tested by David) - but I'm not sure if the below suggestions from Zach were taken into account. > > 1. If the check with "open(file, O_RDWR|O_EXCL)" shows that the device is > available, there's no point in checking if it is mounted as a swap device. > A preliminary check using this could precede all other checks which should > be skipped if it shows success. > > 2. If there's an error checking the status (for example lets say /proc/swaps > is deprecated), we should print the informational message but not error out. If "open(file, O_RDWR|O_EXCL)" failed, we should output an appropriate message why it failed. So, I'm testing is_swap_device() and check_mounted() first. But is_swap_device() is not perfect, so I'm trying to open O_EXCL after all other tests, as a last safety check. Thanks, Tsutomu > > On 2013/02/13 11:58 AM, Zach Brown wrote: >> - First always open with O_EXCL. If it succeeds then there's no reason >> to check /proc/swaps at all. (Maybe it doesn't need to try >> check_mounted() there either? Not sure if it's protecting against >> accidentally mounting mounted shared storage or not.) >> >> ... >> >> - At no point is failure of any of the /proc/swaps parsing fatal. It'd >> carry on ignoring errors until it doesnt have work to do. It'd only >> ever print the nice message when it finds a match. > > -- 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/mkfs.c b/mkfs.c index 2d3c2af..2210312 100644 --- a/mkfs.c +++ b/mkfs.c @@ -1366,6 +1366,16 @@ int main(int ac, char **av) if (source_dir == 0) { file = av[optind++]; + ret = is_swap_device(file); + if (ret < 0) { + fprintf(stderr, "error checking %s status: %s\n", file, + strerror(-ret)); + exit(1); + } + if (ret == 1) { + fprintf(stderr, "%s is a swap device\n", file); + exit(1); + } ret = check_mounted(file); if (ret < 0) { fprintf(stderr, "error checking %s mount status\n", file); @@ -1376,9 +1386,23 @@ int main(int ac, char **av) exit(1); } ac--; + /* check if the device is busy */ + fd = open(file, O_RDWR|O_EXCL); + if (fd < 0) { + fprintf(stderr, "unable to open %s: %s\n", file, + strerror(errno)); + exit(1); + } + close(fd); + /* + * open again without O_EXCL so that the problem should not + * occur by the following processing. + * (btrfs_register_one_device() fails if O_EXCL is on) + */ fd = open(file, O_RDWR); if (fd < 0) { - fprintf(stderr, "unable to open %s\n", file); + fprintf(stderr, "unable to open %s: %s\n", file, + strerror(errno)); exit(1); } first_file = file; @@ -1461,6 +1485,16 @@ int main(int ac, char **av) int old_mixed = mixed; file = av[optind++]; + ret = is_swap_device(file); + if (ret < 0) { + fprintf(stderr, "error checking %s status: %s\n", file, + strerror(-ret)); + exit(1); + } + if (ret == 1) { + fprintf(stderr, "%s is a swap device\n", file); + exit(1); + } ret = check_mounted(file); if (ret < 0) { fprintf(stderr, "error checking %s mount status\n", @@ -1471,9 +1505,23 @@ int main(int ac, char **av) fprintf(stderr, "%s is mounted\n", file); exit(1); } + /* check if the device is busy */ + fd = open(file, O_RDWR|O_EXCL); + if (fd < 0) { + fprintf(stderr, "unable to open %s: %s\n", file, + strerror(errno)); + exit(1); + } + close(fd); + /* + * open again without O_EXCL so that the problem should not + * occur by the following processing. + * (btrfs_register_one_device() fails if O_EXCL is on) + */ fd = open(file, O_RDWR); if (fd < 0) { - fprintf(stderr, "unable to open %s\n", file); + fprintf(stderr, "unable to open %s: %s\n", file, + strerror(errno)); exit(1); } ret = btrfs_device_already_in_root(root, fd, diff --git a/utils.c b/utils.c index f9ee812..891edcc 100644 --- a/utils.c +++ b/utils.c @@ -1386,3 +1386,78 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, return 0; } + +#define isoctal(c) (((c) & ~7) == '0') + +static inline void translate(char *f, char *t) +{ + while (*f != '\0') { + if (*f == '\\' && + isoctal(f[1]) && isoctal(f[2]) && isoctal(f[3])) { + *t++ = 64*(f[1] & 7) + 8*(f[2] & 7) + (f[3] & 7); + f += 4; + } else + *t++ = *f++; + } + *t = '\0'; + return; +} + +/* + * Checks if the swap device. + * Returns 1 if swap device, < 0 on error or 0 if not swap device. + */ +int is_swap_device(const char *file) +{ + FILE *f; + struct stat st_buf; + dev_t dev; + ino_t ino = 0; + char tmp[PATH_MAX]; + char buf[PATH_MAX]; + char *cp; + int ret = 0; + + if (stat(file, &st_buf) < 0) + return -errno; + if (S_ISBLK(st_buf.st_mode)) + dev = st_buf.st_rdev; + else if (S_ISREG(st_buf.st_mode)) { + dev = st_buf.st_dev; + ino = st_buf.st_ino; + } else + return 0; + + if ((f = fopen("/proc/swaps", "r")) == NULL) + return 0; + + /* skip the first line */ + if (fgets(tmp, sizeof(tmp), f) == NULL) + goto out; + + while (fgets(tmp, sizeof(tmp), f) != NULL) { + if ((cp = strchr(tmp, ' ')) != NULL) + *cp = '\0'; + if ((cp = strchr(tmp, '\t')) != NULL) + *cp = '\0'; + translate(tmp, buf); + if (stat(buf, &st_buf) != 0) + continue; + if (S_ISBLK(st_buf.st_mode)) { + if (dev == st_buf.st_rdev) { + ret = 1; + break; + } + } else if (S_ISREG(st_buf.st_mode)) { + if (dev == st_buf.st_dev && ino == st_buf.st_ino) { + ret = 1; + break; + } + } + } + +out: + fclose(f); + + return ret; +} diff --git a/utils.h b/utils.h index bbcaf6a..60a0fea 100644 --- a/utils.h +++ b/utils.h @@ -55,6 +55,7 @@ int get_fs_info(int fd, char *path, struct btrfs_ioctl_fs_info_args *fi_args, struct btrfs_ioctl_dev_info_args **di_ret); char *__strncpy__null(char *dest, const char *src, size_t n); +int is_swap_device(const char *file); /* Helper to always get proper size of the destination string */ #define strncpy_null(dest, src) __strncpy__null(dest, src, sizeof(dest))
Currently, the following commands succeed. # cat /proc/swaps Filename Type Size Used Priority /dev/sda3 partition 8388604 0 -1 /dev/sdc8 partition 9765884 0 -2 # mkfs.btrfs /dev/sdc8 WARNING! - Btrfs v0.20-rc1-165-g82ac345 IS EXPERIMENTAL WARNING! - see http://btrfs.wiki.kernel.org before using fs created label (null) on /dev/sdc8 nodesize 4096 leafsize 4096 sectorsize 4096 size 9.31GB Btrfs v0.20-rc1-165-g82ac345 # btrfs fi sh /dev/sdc8 Label: none uuid: fc0bdbd0-7eed-460f-b4e9-131273b66df2 Total devices 1 FS bytes used 28.00KB devid 1 size 9.31GB used 989.62MB path /dev/sdc8 Btrfs v0.20-rc1-165-g82ac345 # But we should check out the swap device. Fixed it. Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com> --- (this patch is based on Chris's raid56-experimental branch) v2 -> v3: - Open again without O_EXCL so that the problem should not occur (btrfs_register_one_device() fails if O_EXCL is on) v1 -> v2: - O_EXCL is specified at open(), as a last safety check - Check when the swap device is a file is added - Translate '\nnn'(octal) to character Thank you Eric, David, Zach and others. --- mkfs.c | 52 +++++++++++++++++++++++++++++++++++++++++++-- utils.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ utils.h | 1 + 3 files changed, 126 insertions(+), 2 deletions(-) -- 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