| Message ID | 20221104044149.655317-2-alessandro.carminati@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix tst_find_backing_dev adding BTRFS support and /dev/root missing case | expand |
Hello, Just a couple of points. Alessandro Carminati <alessandro.carminati@gmail.com> writes: > + if (dev_major == 0) { > + tst_resm(TINFO, "Use BTRFS specific strategy"); > + > + fd = SAFE_OPEN(NULL, dirname(path), O_DIRECTORY); > + if (!ioctl(fd, BTRFS_IOC_FS_INFO, &args)) { If this is tmpfs, then this ioctl will fail with ENOTTY, but it is not checked for. > + sprintf(btrfs_uuid_str, > + UUID_FMT, > + args.fsid[0], args.fsid[1], > + args.fsid[2], args.fsid[3], > + args.fsid[4], args.fsid[5], > + args.fsid[6], args.fsid[7], > + args.fsid[8], args.fsid[9], > + args.fsid[10], args.fsid[11], > + args.fsid[12], args.fsid[13], > + args.fsid[14], args.fsid[15]); > + sprintf(bdev_path, > + "/sys/fs/btrfs/%s/devices", btrfs_uuid_str); > } > - > - len = count_match_len(path, mnt_point); > - if (len > best_match_len) { > - strcpy(dev, pre); > - best_match_len = len; > + SAFE_CLOSE(NULL, fd); > + dir = SAFE_OPENDIR(NULL, bdev_path); So then down here we get a failure with ENOENT or some similar confusing message. Ideally we want to tell the user that the FS has an anonymous backing dev, but that it is not BTRFS. Maybe also hint that they need to add the FS to tst_test.skip_filesystems if it is has no backing device (e.g. tmpfs). As this is the most likely cause given ioctl_loop05's history. > + while (d = SAFE_READDIR(NULL, dir)) { > + if (d->d_name[0]!='.') > + break; > } > + uevent_path[0] = '\0'; > + if (d) { > + sprintf(uevent_path, "%s/%s/uevent", > + bdev_path, d->d_name); > + } else { > + tst_brkm(TBROK, NULL, "No backining device > found"); We can print bdev path here. Then the user knows where to look without much effort. Marking this as changes requested in patchwork.
The test is specifically forbidden on tmpfs, but I agree with you that may be some weird scenarios where the major is 0 and the filesystem is not BTRFS. As predicted, if for example the ioctl_loop05 test is forced to run on tmpfs, I found no way to make it if not change the code at tst_fs_in_skiplist, you have a confusing error message: tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s tst_test.c:1278: TINFO: tmpfs is supported by the test tst_device.c:94: TINFO: Found free device 0 '/dev/loop0' loop0: detected capacity change from 0 to 2048 tst_device.c:545: TINFO: Use BTRFS specific strategy tst_device.c:563: TBROK: opendir() failed: ENOENT (2) LTP_SINGLE_FS_TYPE, if it is the env variable that was mentioned on the previous mail in this thread, seems not to affect the fs skiplist. By the way, I modified the code to produce a TBROK message if the ioctl fails. This version of the patch produces: tst_test.c:1526: TINFO: Timeout per run is 0h 00m 30s tst_test.c:1278: TINFO: tmpfs is supported by the test tst_device.c:94: TINFO: Found free device 0 '/dev/loop0' loop0: detected capacity change from 0 to 2048 tst_device.c:545: TINFO: Use BTRFS specific strategy tst_device.c:562: TBROK: BTRFS ioctl failed. Is /tmp/iocVvqexV on a tmpfs? Thanks Alessandro Alessandro Carminati (2): tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent c-test-api: Documentation updated doc/c-test-api.txt | 7 +++- lib/tst_device.c | 91 +++++++++++++++++++++++++++++++--------------- 2 files changed, 66 insertions(+), 32 deletions(-)
diff --git a/lib/tst_device.c b/lib/tst_device.c index 8419b80c3..e9933955f 100644 --- a/lib/tst_device.c +++ b/lib/tst_device.c @@ -33,6 +33,9 @@ #include <stdint.h> #include <inttypes.h> #include <sys/sysmacros.h> +#include <linux/btrfs.h> +#include <linux/limits.h> +#include <dirent.h> #include "lapi/syscalls.h" #include "test.h" #include "safe_macros.h" @@ -45,6 +48,8 @@ #define DEV_FILE "test_dev.img" #define DEV_SIZE_MB 300u +#define UUID_STR_SZ 37 +#define UUID_FMT "%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x" static char dev_path[1024]; static int device_acquired; @@ -519,48 +524,72 @@ static int count_match_len(const char *first, const char *second) void tst_find_backing_dev(const char *path, char *dev) { struct stat buf; - FILE *file; - char line[PATH_MAX]; - char *pre = NULL; - char *next = NULL; - unsigned int dev_major, dev_minor, line_mjr, line_mnr; - unsigned int len, best_match_len = 1; - char mnt_point[PATH_MAX]; + struct btrfs_ioctl_fs_info_args args = {0}; + struct dirent *d; + char uevent_path[PATH_MAX]; + char dev_name[NAME_MAX]; + char bdev_path[PATH_MAX]; + char btrfs_uuid_str[UUID_STR_SZ]; + DIR *dir; + unsigned int dev_major, dev_minor; + int fd; if (stat(path, &buf) < 0) tst_brkm(TWARN | TERRNO, NULL, "stat() failed"); dev_major = major(buf.st_dev); dev_minor = minor(buf.st_dev); - file = SAFE_FOPEN(NULL, "/proc/self/mountinfo", "r"); *dev = '\0'; - while (fgets(line, sizeof(line), file)) { - if (sscanf(line, "%*d %*d %d:%d %*s %s", - &line_mjr, &line_mnr, mnt_point) != 3) - continue; - - pre = strstr(line, " - "); - pre = strtok_r(pre, " ", &next); - pre = strtok_r(NULL, " ", &next); - pre = strtok_r(NULL, " ", &next); - - if (line_mjr == dev_major && line_mnr == dev_minor) { - strcpy(dev, pre); - break; + if (dev_major == 0) { + tst_resm(TINFO, "Use BTRFS specific strategy"); + + fd = SAFE_OPEN(NULL, dirname(path), O_DIRECTORY); + if (!ioctl(fd, BTRFS_IOC_FS_INFO, &args)) { + sprintf(btrfs_uuid_str, + UUID_FMT, + args.fsid[0], args.fsid[1], + args.fsid[2], args.fsid[3], + args.fsid[4], args.fsid[5], + args.fsid[6], args.fsid[7], + args.fsid[8], args.fsid[9], + args.fsid[10], args.fsid[11], + args.fsid[12], args.fsid[13], + args.fsid[14], args.fsid[15]); + sprintf(bdev_path, + "/sys/fs/btrfs/%s/devices", btrfs_uuid_str); } - - len = count_match_len(path, mnt_point); - if (len > best_match_len) { - strcpy(dev, pre); - best_match_len = len; + SAFE_CLOSE(NULL, fd); + dir = SAFE_OPENDIR(NULL, bdev_path); + while (d = SAFE_READDIR(NULL, dir)) { + if (d->d_name[0]!='.') + break; } + uevent_path[0] = '\0'; + if (d) { + sprintf(uevent_path, "%s/%s/uevent", + bdev_path, d->d_name); + } else { + tst_brkm(TBROK, NULL, "No backining device found"); + } + if (SAFE_READDIR(NULL, dir)) + tst_resm(TINFO, "Warning: used first of multiple backing device."); + SAFE_CLOSEDIR(NULL, dir); + } else { + + tst_resm(TINFO, "Use uevent strategy"); + sprintf(uevent_path, + "/sys/dev/block/%d:%d/uevent", dev_major, dev_minor); } - SAFE_FCLOSE(NULL, file); + if (!access(uevent_path, R_OK)) { + FILE_LINES_SCANF(NULL, uevent_path, "DEVNAME=%s", dev_name); - if (!*dev) - tst_brkm(TBROK, NULL, "Cannot find block device for %s", path); + if (dev_name[0]) + sprintf(dev, "/dev/%s", dev_name); + } else { + tst_brkm(TBROK, NULL, "uevent file (%s) access failed", uevent_path); + } if (stat(dev, &buf) < 0) tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);