[LTP] [PATCH v3 0/3] tst_device: add support for overlayfs

[LTP] [PATCH v3 0/3] tst_device: add support for overlayfs

[Jeff Moyer]

When running ltp-aiodio on an overlay file system, the following error
occurs:

tst_tmpdir.c:316: TINFO: Using /mnt/fstests/TEST_DIR/ovl-mnt/ltp-hSHEHy5M0s/LTP_aio4q4GMW as tmpdir (overlayfs filesystem)
tst_test.c:1888: TINFO: LTP version: 20220121-2271-g91a10df22
tst_test.c:1892: TINFO: Tested kernel: vendor kernel
tst_test.c:1723: TINFO: Timeout per run is 0h 40m 00s
aiocp.c:211: TINFO: Maximum AIO blocks: 65536
tst_device.c:551: TINFO: Use BTRFS specific strategy
tst_device.c:569: TBROK: BTRFS ioctl failed. Is . on a tmpfs?: ENOTTY (25)

The issue is that stat(2) on an overlayfs mount point will return a
major device number of 0.  The code assumes this is btrfs, and tries
to issue a btrfs-specific ioctl.  When that fails, the final message is
printed that suggests this might be tmpfs.

I modified the code to use statfs(2) to get the file system type, and
use that to determine which file system specific code to call.
Finally, I added code to parse out the upper directory from the
overlayfs mount options using a combination of /proc/self/mountinfo
and /proc/self/mounts, as explained in patch 3.  stat(2) is then
called on the upper directory to get to the underlying device node.

I tested this on xfs, btrfs, a btrfs subvolume and overlayfs.  Review
of the series is greatly appreciated.

Thanks in advance!
Jeff

[PATCH v3 1/3] lib/tst_device.c: factor out btrfs-specific logic from
[PATCH v3 2/3] lib/tst_device.c: check for BTRFS_SUPER_MAGIC instead
[PATCH v3 3/3] tst_find_backing_dev(): add support for overlayfs
---
Changes since v2:
- Address review comments from Petr:
  - Use test-defined magic numbers
  - #include statfs.h instead of vfs.h.
  - Don't split long strings across lines
- Add Reviewed-by tag for patch 1.

Changes since v1:
- Remove dependency on libmount in patch 3.  Patches 1 and 2 are
  unchanged.


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 1/3] lib/tst_device.c: factor out btrfs-specific logic from tst_find_backing_dev

[Jeff Moyer]

In preparation for handling overlayfs (which also has a major device
number of 0), factor out the btrfs logic.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
 lib/tst_device.c | 104 +++++++++++++++++++++++++----------------------
 1 file changed, 55 insertions(+), 49 deletions(-)

diff --git a/lib/tst_device.c b/lib/tst_device.c
index 723f6ca06..70234a83c 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -520,20 +520,68 @@ unsigned long tst_dev_bytes_written(const char *dev)
 	return dev_bytes_written;
 }
 
+static void btrfs_get_uevent_path(char *tmp_path, char *uevent_path)
+{
+	int fd;
+	struct btrfs_ioctl_fs_info_args args = {0};
+	char btrfs_uuid_str[UUID_STR_SZ];
+	struct dirent *d;
+	char bdev_path[PATH_MAX];
+	DIR *dir;
+
+	tst_resm(TINFO, "Use BTRFS specific strategy");
+
+	fd = SAFE_OPEN(NULL, tmp_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);
+	} else {
+		if (errno == ENOTTY)
+			tst_brkm(TBROK | TERRNO, NULL, "BTRFS ioctl failed. Is %s on a tmpfs?", tmp_path);
+
+		tst_brkm(TBROK | TERRNO, NULL, "BTRFS ioctl on %s failed.", tmp_path);
+	}
+	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 | TERRNO, NULL, "No backing device found while looking in %s.", bdev_path);
+	}
+
+	if (SAFE_READDIR(NULL, dir))
+		tst_resm(TINFO, "Warning: used first of multiple backing device.");
+
+	SAFE_CLOSEDIR(NULL, dir);
+}
+
 __attribute__((nonnull))
 void tst_find_backing_dev(const char *path, char *dev, size_t dev_size)
 {
 	struct stat buf;
-	struct btrfs_ioctl_fs_info_args args = {0};
-	struct dirent *d;
 	char uevent_path[PATH_MAX+PATH_MAX+10]; //10 is for the static uevent path
 	char dev_name[NAME_MAX];
-	char bdev_path[PATH_MAX];
 	char tmp_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");
@@ -548,49 +596,7 @@ void tst_find_backing_dev(const char *path, char *dev, size_t dev_size)
 	*dev = '\0';
 
 	if (dev_major == 0) {
-		tst_resm(TINFO, "Use BTRFS specific strategy");
-
-		fd = SAFE_OPEN(NULL, tmp_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);
-		} else {
-			if (errno == ENOTTY)
-				tst_brkm(TBROK | TERRNO, NULL, "BTRFS ioctl failed. Is %s on a tmpfs?", path);
-
-			tst_brkm(TBROK | TERRNO, NULL, "BTRFS ioctl on %s failed.", tmp_path);
-		}
-		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 | TERRNO, NULL, "No backing device found while looking in %s.", bdev_path);
-		}
-
-		if (SAFE_READDIR(NULL, dir))
-			tst_resm(TINFO, "Warning: used first of multiple backing device.");
-
-		SAFE_CLOSEDIR(NULL, dir);
+		btrfs_get_uevent_path(tmp_path, uevent_path);
 	} else {
 		tst_resm(TINFO, "Use uevent strategy");
 		sprintf(uevent_path,
-- 
2.43.5


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 1/3] lib/tst_device.c: factor out btrfs-specific logic from tst_find_backing_dev

[Cyril Hrubis]

Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 2/3] lib/tst_device.c: check for BTRFS_SUPER_MAGIC instead of device major of 0

[Jeff Moyer]

stat() may return a major number of 0 in st_dev for any number of
pseudo file systems.  Check for the exact file system instead.  There
should be no change in behavior with this patch.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
v3: Address review comments from Petr:
    - use test-defined file system magic numbers
    - use statfs.h instead of vfs.h
v2: unchanged
---
 lib/tst_device.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/lib/tst_device.c b/lib/tst_device.c
index 70234a83c..76c3a3e1e 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -17,6 +17,7 @@
 #include <sys/sysmacros.h>
 #include <linux/btrfs.h>
 #include <linux/limits.h>
+#include <sys/statfs.h>
 #include "lapi/syscalls.h"
 #include "test.h"
 #include "safe_macros.h"
@@ -546,9 +547,6 @@ static void btrfs_get_uevent_path(char *tmp_path, char *uevent_path)
 		sprintf(bdev_path,
 			"/sys/fs/btrfs/%s/devices", btrfs_uuid_str);
 	} else {
-		if (errno == ENOTTY)
-			tst_brkm(TBROK | TERRNO, NULL, "BTRFS ioctl failed. Is %s on a tmpfs?", tmp_path);
-
 		tst_brkm(TBROK | TERRNO, NULL, "BTRFS ioctl on %s failed.", tmp_path);
 	}
 	SAFE_CLOSE(NULL, fd);
@@ -578,6 +576,7 @@ __attribute__((nonnull))
 void tst_find_backing_dev(const char *path, char *dev, size_t dev_size)
 {
 	struct stat buf;
+	struct statfs fsbuf;
 	char uevent_path[PATH_MAX+PATH_MAX+10]; //10 is for the static uevent path
 	char dev_name[NAME_MAX];
 	char tmp_path[PATH_MAX];
@@ -595,8 +594,13 @@ void tst_find_backing_dev(const char *path, char *dev, size_t dev_size)
 	dev_minor = minor(buf.st_dev);
 	*dev = '\0';
 
-	if (dev_major == 0) {
+	if (statfs(path, &fsbuf) < 0)
+		tst_brkm(TBROK | TERRNO, NULL, "statfs() failed");
+
+	if (fsbuf.f_type == TST_BTRFS_MAGIC) {
 		btrfs_get_uevent_path(tmp_path, uevent_path);
+	} else if (dev_major == 0) {
+		tst_brkm(TBROK, NULL, "%s resides on an unsupported pseudo-file system.", path);
 	} else {
 		tst_resm(TINFO, "Use uevent strategy");
 		sprintf(uevent_path,
-- 
2.43.5


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 2/3] lib/tst_device.c: check for BTRFS_SUPER_MAGIC instead of device major of 0

[Petr Vorel]

Hi Jeff,

> stat() may return a major number of 0 in st_dev for any number of
> pseudo file systems.  Check for the exact file system instead.  There
> should be no change in behavior with this patch.

LGTM, thank you!
Reviewed-by: Petr Vorel <pvorel@suse.cz>

FYI I'll apply very minor formatting fix before merge.

Kind regards,
Petr

+++ lib/tst_device.c
@@ -559,12 +559,10 @@ static void btrfs_get_uevent_path(char *tmp_path, char *uevent_path)
 
 	uevent_path[0] = '\0';
 
-	if (d) {
-		sprintf(uevent_path, "%s/%s/uevent",
-			bdev_path, d->d_name);
-	} else {
-		tst_brkm(TBROK | TERRNO, NULL, "No backing device found while looking in %s.", bdev_path);
-	}
+	if (d)
+		sprintf(uevent_path, "%s/%s/uevent", bdev_path, d->d_name);
+	else
+		tst_brkm(TBROK | TERRNO, NULL, "No backing device found while looking in %s", bdev_path);
 
 	if (SAFE_READDIR(NULL, dir))
 		tst_resm(TINFO, "Warning: used first of multiple backing device.");
@@ -600,7 +598,7 @@ void tst_find_backing_dev(const char *path, char *dev, size_t dev_size)
 	if (fsbuf.f_type == TST_BTRFS_MAGIC) {
 		btrfs_get_uevent_path(tmp_path, uevent_path);
 	} else if (dev_major == 0) {
-		tst_brkm(TBROK, NULL, "%s resides on an unsupported pseudo-file system.", path);
+		tst_brkm(TBROK, NULL, "%s resides on an unsupported pseudo-file system", path);
 	} else {
 		tst_resm(TINFO, "Use uevent strategy");
 		sprintf(uevent_path,


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 2/3] lib/tst_device.c: check for BTRFS_SUPER_MAGIC instead of device major of 0

[Jeff Moyer]

Petr Vorel <pvorel@suse.cz> writes:

> Hi Jeff,
>
>> stat() may return a major number of 0 in st_dev for any number of
>> pseudo file systems.  Check for the exact file system instead.  There
>> should be no change in behavior with this patch.
>
> LGTM, thank you!
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
> FYI I'll apply very minor formatting fix before merge.

Yup, that looks fine to me.  Thanks for taking care of that!

-Jeff

> +++ lib/tst_device.c
> @@ -559,12 +559,10 @@ static void btrfs_get_uevent_path(char *tmp_path, char *uevent_path)
>  
>  	uevent_path[0] = '\0';
>  
> -	if (d) {
> -		sprintf(uevent_path, "%s/%s/uevent",
> -			bdev_path, d->d_name);
> -	} else {
> -		tst_brkm(TBROK | TERRNO, NULL, "No backing device found while looking in %s.", bdev_path);
> -	}
> +	if (d)
> +		sprintf(uevent_path, "%s/%s/uevent", bdev_path, d->d_name);
> +	else
> +		tst_brkm(TBROK | TERRNO, NULL, "No backing device found while looking in %s", bdev_path);
>  
>  	if (SAFE_READDIR(NULL, dir))
>  		tst_resm(TINFO, "Warning: used first of multiple backing device.");
> @@ -600,7 +598,7 @@ void tst_find_backing_dev(const char *path, char *dev, size_t dev_size)
>  	if (fsbuf.f_type == TST_BTRFS_MAGIC) {
>  		btrfs_get_uevent_path(tmp_path, uevent_path);
>  	} else if (dev_major == 0) {
> -		tst_brkm(TBROK, NULL, "%s resides on an unsupported pseudo-file system.", path);
> +		tst_brkm(TBROK, NULL, "%s resides on an unsupported pseudo-file system", path);
>  	} else {
>  		tst_resm(TINFO, "Use uevent strategy");
>  		sprintf(uevent_path,


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 2/3] lib/tst_device.c: check for BTRFS_SUPER_MAGIC instead of device major of 0

[Cyril Hrubis]

Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 2/3] lib/tst_device.c: check for BTRFS_SUPER_MAGIC instead of device major of 0

[Petr Vorel]

Hi all,

FYI I merged first and this (second) commit.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v3 3/3] tst_find_backing_dev(): add support for overlayfs

[Jeff Moyer]

Add checks for overlayfs in tst_find_backing_dev.  As with btrfs, only
a single device is checked (the upper one) and returned from
tst_find_backing_dev().

The implementation uses both /proc/self/mountinfo and /proc/self/mounts.
The former is used to map a device to a mountpoint, and the latter is
used to get the file system options for the mountpoint.  All of the
information is present in mountinfo, but the file format is more complex,
and there are no glibc helpers for parsing it.

The '#define _GNU_SOURCE' was added for the use of the strchrnul(3)
function.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

---
v3: Address review comments from Petr:
    - Don't split long strings across lines
    - use TST_OVERLAYFS_MAGIC instead of the kernel's definition
    Note that I did not use SAFE_SSCANF, as tst_device.c uses the old
    style safe macros, and that function is not covered.
v2: Don't use libmount.  Instead, map from device number to mount-point
    using /proc/self/mountinfo, and then use the mntent.h helpers to get
    the mount options for the mountpoint from /proc/self/mounts.
---
 lib/tst_device.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)

diff --git a/lib/tst_device.c b/lib/tst_device.c
index 76c3a3e1e..29dc6974f 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -3,6 +3,7 @@
  * Copyright (C) 2014 Cyril Hrubis chrubis@suse.cz
  */
 
+#define _GNU_SOURCE
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <sys/ioctl.h>
@@ -572,6 +573,136 @@ static void btrfs_get_uevent_path(char *tmp_path, char *uevent_path)
 	SAFE_CLOSEDIR(NULL, dir);
 }
 
+static char *overlay_mount_from_dev(dev_t dev)
+{
+	unsigned dev_major, dev_minor, mnt_major, mnt_minor;
+	FILE *fp;
+	char line[PATH_MAX];
+	char *mountpoint;
+	int ret;
+
+	dev_major = major(dev);
+	dev_minor = minor(dev);
+
+	fp = SAFE_FOPEN(NULL, "/proc/self/mountinfo", "r");
+	while (fgets(line, sizeof(line), fp) != NULL) {
+		ret = sscanf(line, "%*d %*d %u:%u %*s %ms",
+			     &mnt_major, &mnt_minor, &mountpoint);
+		if (ret != 3)
+			tst_brkm(TBROK, NULL,
+				 "failed to parse mountinfo line: \"%s\"",
+				 line);
+		if (mnt_major == dev_major && mnt_minor == dev_minor)
+			break;
+		free(mountpoint);
+		mountpoint = NULL;
+	}
+	SAFE_FCLOSE(NULL, fp);
+	if (!mountpoint)
+		tst_brkm(TBROK, NULL,
+			 "Unable to find mount entry for device %u:%u\n",
+			 dev_major, dev_minor);
+
+	return mountpoint;
+}
+
+static char *overlay_get_upperdir(char *mountpoint)
+{
+	FILE *mntf;
+	struct mntent *mnt;
+	char *optstr, *optstart, *optend;
+	char *upperdir = NULL;
+
+	mntf = setmntent("/proc/self/mounts", "r");
+	if (!mntf)
+		tst_brkm(TBROK | TERRNO, NULL, "Can't open /proc/self/mounts");
+
+	while ((mnt = getmntent(mntf)) != NULL) {
+		if (strncmp(mnt->mnt_dir, mountpoint, strlen(mountpoint)))
+			continue;
+
+		if (strncmp(mnt->mnt_type, "overlay", strlen("overlay")))
+			tst_brkm(TBROK, NULL,
+				 "expected overlayfs on mount point \"%s\", but it is of type %s.",
+				 mountpoint, mnt->mnt_type);
+
+		optstr = hasmntopt(mnt, "upperdir");
+		if (optstr) {
+			optstart = strchr(optstr, '=');
+			optstart++;
+			optend = strchrnul(optstr, ',');
+			upperdir = calloc(optend - optstart + 1, 1);
+			memcpy(upperdir, optstart, optend - optstart);
+			break;
+		} else {
+			tst_brkm(TBROK, NULL,
+				 "mount point %s does not contain an upperdir",
+				 mountpoint);
+		}
+	}
+	endmntent(mntf);
+
+	if (!upperdir)
+		tst_brkm(TBROK, NULL,
+			 "Unable to find mount point \"%s\" in mount table",
+			 mountpoint);
+
+	return upperdir;
+}
+
+/*
+ * To get from a file or directory on an overlayfs to a device
+ * for an underlying mountpoint requires the following steps:
+ *
+ * 1. stat() the pathname and pick out st_dev.
+ * 2. use the st_dev to look up the mount point of the file
+ *    system in /proc/self/mountinfo
+ *
+ * Because 'mountinfo' is a much more complicated file format than
+ * 'mounts', we switch to looking up the mount point in /proc/mounts,
+ * and use the mntent.h helpers to parse the entries.
+ *
+ * 3. Using getmntent(), find the entry for the mount point identified
+ *    in step 2.
+ * 4. Call hasmntopt() to find the upperdir option, and parse that
+ *    option to get to the path name for the upper directory.
+ * 5. Call stat on the upper directory.  This should now contain
+ *    the major and minor number for the underlying device (assuming
+ *    that there aren't nested overlay file systems).
+ * 6. Populate the uevent path.
+ *
+ * Example /proc/self/mountinfo line for overlayfs:
+ *    471 69 0:48 / /tmp rw,relatime shared:242 - overlay overlay rw,seclabel,lowerdir=/tmp,upperdir=/mnt/upper/upper,workdir=/mnt/upper/work,uuid=null
+ *
+ * See section 3.5 of the kernel's Documentation/filesystems/proc.rst file
+ * for a detailed explanation of the mountinfo format.
+ */
+static void overlay_get_uevent_path(char *tmp_path, char *uevent_path)
+{
+	int ret;
+	struct stat st;
+	char *mountpoint, *upperdir;
+
+	tst_resm(TINFO, "Use OVERLAYFS specific strategy");
+
+	ret = stat(tmp_path, &st);
+	if (ret)
+		tst_brkm(TBROK | TERRNO, NULL, "stat failed");
+
+	mountpoint = overlay_mount_from_dev(st.st_dev);
+	upperdir = overlay_get_upperdir(mountpoint);
+	free(mountpoint);
+
+	ret = stat(upperdir, &st);
+	free(upperdir);
+	if (ret)
+		tst_brkm(TBROK | TERRNO, NULL, "stat failed");
+
+	tst_resm(TINFO, "Warning: used first of multiple backing devices.");
+	sprintf(uevent_path, "/sys/dev/block/%d:%d/uevent",
+		major(st.st_dev), minor(st.st_dev));
+}
+
 __attribute__((nonnull))
 void tst_find_backing_dev(const char *path, char *dev, size_t dev_size)
 {
@@ -599,6 +730,8 @@ void tst_find_backing_dev(const char *path, char *dev, size_t dev_size)
 
 	if (fsbuf.f_type == TST_BTRFS_MAGIC) {
 		btrfs_get_uevent_path(tmp_path, uevent_path);
+	} else if (fsbuf.f_type == TST_OVERLAYFS_MAGIC) {
+		overlay_get_uevent_path(tmp_path, uevent_path);
 	} else if (dev_major == 0) {
 		tst_brkm(TBROK, NULL, "%s resides on an unsupported pseudo-file system.", path);
 	} else {
-- 
2.43.5


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 3/3] tst_find_backing_dev(): add support for overlayfs

[Petr Vorel]

Hi Jeff,


> Add checks for overlayfs in tst_find_backing_dev.  As with btrfs, only
> a single device is checked (the upper one) and returned from
> tst_find_backing_dev().

> The implementation uses both /proc/self/mountinfo and /proc/self/mounts.
> The former is used to map a device to a mountpoint, and the latter is
> used to get the file system options for the mountpoint.  All of the
> information is present in mountinfo, but the file format is more complex,
> and there are no glibc helpers for parsing it.

> The '#define _GNU_SOURCE' was added for the use of the strchrnul(3)
> function.

> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

> ---
> v3: Address review comments from Petr:
>     - Don't split long strings across lines
>     - use TST_OVERLAYFS_MAGIC instead of the kernel's definition
>     Note that I did not use SAFE_SSCANF, as tst_device.c uses the old
>     style safe macros, and that function is not covered.
> v2: Don't use libmount.  Instead, map from device number to mount-point
>     using /proc/self/mountinfo, and then use the mntent.h helpers to get
>     the mount options for the mountpoint from /proc/self/mounts.

LGTM, thanks for a very nice work!

Reviewed-by: Petr Vorel <pvorel@suse.cz>

I'd prefer others have look into it before merging.

Again, I'll apply minor formatting changes before merge (using SAFE_STAT(),
moving else branch after break to it's own and checkpatch.pl fixes).

Kind regards,
Petr

+++ lib/tst_device.c
@@ -575,7 +573,7 @@ static void btrfs_get_uevent_path(char *tmp_path, char *uevent_path)
 
 static char *overlay_mount_from_dev(dev_t dev)
 {
-	unsigned dev_major, dev_minor, mnt_major, mnt_minor;
+	unsigned int dev_major, dev_minor, mnt_major, mnt_minor;
 	FILE *fp;
 	char line[PATH_MAX];
 	char *mountpoint;
@@ -627,6 +625,7 @@ static char *overlay_get_upperdir(char *mountpoint)
 				 mountpoint, mnt->mnt_type);
 
 		optstr = hasmntopt(mnt, "upperdir");
+
 		if (optstr) {
 			optstart = strchr(optstr, '=');
 			optstart++;
@@ -634,11 +633,11 @@ static char *overlay_get_upperdir(char *mountpoint)
 			upperdir = calloc(optend - optstart + 1, 1);
 			memcpy(upperdir, optstart, optend - optstart);
 			break;
-		} else {
-			tst_brkm(TBROK, NULL,
-				 "mount point %s does not contain an upperdir",
-				 mountpoint);
 		}
+
+		tst_brkm(TBROK, NULL,
+			 "mount point %s does not contain an upperdir",
+			 mountpoint);
 	}
 	endmntent(mntf);
 
@@ -679,26 +678,21 @@ static char *overlay_get_upperdir(char *mountpoint)
  */
 static void overlay_get_uevent_path(char *tmp_path, char *uevent_path)
 {
-	int ret;
 	struct stat st;
 	char *mountpoint, *upperdir;
 
 	tst_resm(TINFO, "Use OVERLAYFS specific strategy");
 
-	ret = stat(tmp_path, &st);
-	if (ret)
-		tst_brkm(TBROK | TERRNO, NULL, "stat failed");
+	SAFE_STAT(NULL, tmp_path, &st);
 
 	mountpoint = overlay_mount_from_dev(st.st_dev);
 	upperdir = overlay_get_upperdir(mountpoint);
 	free(mountpoint);
 
-	ret = stat(upperdir, &st);
+	SAFE_STAT(NULL, upperdir, &st);
 	free(upperdir);
-	if (ret)
-		tst_brkm(TBROK | TERRNO, NULL, "stat failed");
 
-	tst_resm(TINFO, "Warning: used first of multiple backing devices.");
+	tst_resm(TINFO, "WARNING: used first of multiple backing devices");
 	sprintf(uevent_path, "/sys/dev/block/%d:%d/uevent",
 		major(st.st_dev), minor(st.st_dev));
 }

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 3/3] tst_find_backing_dev(): add support for overlayfs

[Jeff Moyer]

Hi, Petr,

Petr Vorel <pvorel@suse.cz> writes:

> Hi Jeff,
>
>
>> Add checks for overlayfs in tst_find_backing_dev.  As with btrfs, only
>> a single device is checked (the upper one) and returned from
>> tst_find_backing_dev().
>
>> The implementation uses both /proc/self/mountinfo and /proc/self/mounts.
>> The former is used to map a device to a mountpoint, and the latter is
>> used to get the file system options for the mountpoint.  All of the
>> information is present in mountinfo, but the file format is more complex,
>> and there are no glibc helpers for parsing it.
>
>> The '#define _GNU_SOURCE' was added for the use of the strchrnul(3)
>> function.
>
>> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
>
>> ---
>> v3: Address review comments from Petr:
>>     - Don't split long strings across lines
>>     - use TST_OVERLAYFS_MAGIC instead of the kernel's definition
>>     Note that I did not use SAFE_SSCANF, as tst_device.c uses the old
>>     style safe macros, and that function is not covered.
>> v2: Don't use libmount.  Instead, map from device number to mount-point
>>     using /proc/self/mountinfo, and then use the mntent.h helpers to get
>>     the mount options for the mountpoint from /proc/self/mounts.
>
> LGTM, thanks for a very nice work!
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>
> I'd prefer others have look into it before merging.

Sure, any review is appreciated.

> Again, I'll apply minor formatting changes before merge (using SAFE_STAT(),
> moving else branch after break to it's own and checkpatch.pl fixes).

It looks fine, but I will make a couple of observations.

> @@ -634,11 +633,11 @@ static char *overlay_get_upperdir(char *mountpoint)
>  			upperdir = calloc(optend - optstart + 1, 1);
>  			memcpy(upperdir, optstart, optend - optstart);
>  			break;
> -		} else {
> -			tst_brkm(TBROK, NULL,
> -				 "mount point %s does not contain an upperdir",
> -				 mountpoint);
>  		}
> +
> +		tst_brkm(TBROK, NULL,
> +			 "mount point %s does not contain an upperdir",
> +			 mountpoint);

This is technically different, but I don't think it matters.  All
overlay mount points need an upperdir, so it is valid to error out here.

>  	}
>  	endmntent(mntf);
>  
> @@ -679,26 +678,21 @@ static char *overlay_get_upperdir(char *mountpoint)
>   */
>  static void overlay_get_uevent_path(char *tmp_path, char *uevent_path)
>  {
> -	int ret;
>  	struct stat st;
>  	char *mountpoint, *upperdir;
>  
>  	tst_resm(TINFO, "Use OVERLAYFS specific strategy");
>  
> -	ret = stat(tmp_path, &st);
> -	if (ret)
> -		tst_brkm(TBROK | TERRNO, NULL, "stat failed");
> +	SAFE_STAT(NULL, tmp_path, &st);

Sorry for not using SAFE_STAT.  I don't know how I missed that.  Thanks
again for the review and for fixing up these issues.

Cheers,
Jeff


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 3/3] tst_find_backing_dev(): add support for overlayfs

[Petr Vorel]

Hi Jeff,
> > LGTM, thanks for a very nice work!

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>

> > I'd prefer others have look into it before merging.

> Sure, any review is appreciated.

> > Again, I'll apply minor formatting changes before merge (using SAFE_STAT(),
> > moving else branch after break to it's own and checkpatch.pl fixes).

> It looks fine, but I will make a couple of observations.

> > @@ -634,11 +633,11 @@ static char *overlay_get_upperdir(char *mountpoint)
> >  			upperdir = calloc(optend - optstart + 1, 1);
> >  			memcpy(upperdir, optstart, optend - optstart);
> >  			break;
> > -		} else {
> > -			tst_brkm(TBROK, NULL,
> > -				 "mount point %s does not contain an upperdir",
> > -				 mountpoint);
> >  		}
> > +
> > +		tst_brkm(TBROK, NULL,
> > +			 "mount point %s does not contain an upperdir",
> > +			 mountpoint);

> This is technically different, but I don't think it matters.  All
> overlay mount points need an upperdir, so it is valid to error out here.

FYI my point here was to change:

if (...) {
	foo ...
	break;
} else {
	bar ...
}

to:

if (...) {
	foo ...
	break;
}

bar ...

(IMHO slightly readable + checkpatch.pl prefers it.)
Did I overlook something?

> >  	}
> >  	endmntent(mntf);

> > @@ -679,26 +678,21 @@ static char *overlay_get_upperdir(char *mountpoint)
> >   */
> >  static void overlay_get_uevent_path(char *tmp_path, char *uevent_path)
> >  {
> > -	int ret;
> >  	struct stat st;
> >  	char *mountpoint, *upperdir;

> >  	tst_resm(TINFO, "Use OVERLAYFS specific strategy");

> > -	ret = stat(tmp_path, &st);
> > -	if (ret)
> > -		tst_brkm(TBROK | TERRNO, NULL, "stat failed");
> > +	SAFE_STAT(NULL, tmp_path, &st);

> Sorry for not using SAFE_STAT.  I don't know how I missed that.  Thanks
> again for the review and for fixing up these issues.

Nah, not a big deal. The patchset is very nice, thanks for that!

Kind regards,
Petr

> Cheers,
> Jeff


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 3/3] tst_find_backing_dev(): add support for overlayfs

[Cyril Hrubis]

Hi!
> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index 76c3a3e1e..29dc6974f 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -3,6 +3,7 @@
>   * Copyright (C) 2014 Cyril Hrubis chrubis@suse.cz
>   */
>  
> +#define _GNU_SOURCE
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <sys/ioctl.h>
> @@ -572,6 +573,136 @@ static void btrfs_get_uevent_path(char *tmp_path, char *uevent_path)
>  	SAFE_CLOSEDIR(NULL, dir);
>  }
>  
> +static char *overlay_mount_from_dev(dev_t dev)
> +{
> +	unsigned dev_major, dev_minor, mnt_major, mnt_minor;
> +	FILE *fp;
> +	char line[PATH_MAX];

PATH_MAX does not really make any sense here. It's as good as any other
number so I would just hardcode 4096 here.

> +	char *mountpoint;
> +	int ret;
> +
> +	dev_major = major(dev);
> +	dev_minor = minor(dev);
> +
> +	fp = SAFE_FOPEN(NULL, "/proc/self/mountinfo", "r");
> +	while (fgets(line, sizeof(line), fp) != NULL) {
> +		ret = sscanf(line, "%*d %*d %u:%u %*s %ms",
> +			     &mnt_major, &mnt_minor, &mountpoint);
> +		if (ret != 3)
> +			tst_brkm(TBROK, NULL,
> +				 "failed to parse mountinfo line: \"%s\"",
                                                                   ^
                We usually use ' instead of " inside of strings in LTP.


> +				 line);
> +		if (mnt_major == dev_major && mnt_minor == dev_minor)
> +			break;
> +		free(mountpoint);
> +		mountpoint = NULL;
> +	}
> +	SAFE_FCLOSE(NULL, fp);
> +	if (!mountpoint)
> +		tst_brkm(TBROK, NULL,
> +			 "Unable to find mount entry for device %u:%u\n",
                                                                      ^
							No newlines in
							tst_*()
							messages please.
> +			 dev_major, dev_minor);
> +
> +	return mountpoint;
> +}
> +
> +static char *overlay_get_upperdir(char *mountpoint)
> +{
> +	FILE *mntf;
> +	struct mntent *mnt;
> +	char *optstr, *optstart, *optend;
> +	char *upperdir = NULL;
> +
> +	mntf = setmntent("/proc/self/mounts", "r");
> +	if (!mntf)
> +		tst_brkm(TBROK | TERRNO, NULL, "Can't open /proc/self/mounts");
> +
> +	while ((mnt = getmntent(mntf)) != NULL) {
> +		if (strncmp(mnt->mnt_dir, mountpoint, strlen(mountpoint)))
> +			continue;

Why strncmp() here? Isn't this possibly generating false positives in
the case that we there is more mounts that have the same prefix that
matches mountpoint?

> +		if (strncmp(mnt->mnt_type, "overlay", strlen("overlay")))
> +			tst_brkm(TBROK, NULL,
> +				 "expected overlayfs on mount point \"%s\", but it is of type %s.",
> +				 mountpoint, mnt->mnt_type);

Here as well, I suppose that the probability of false positive here is
close to zero, but I do not see the reason for strncmp() here either.

> +		optstr = hasmntopt(mnt, "upperdir");
> +		if (optstr) {
> +			optstart = strchr(optstr, '=');
> +			optstart++;
> +			optend = strchrnul(optstr, ',');
> +			upperdir = calloc(optend - optstart + 1, 1);
> +			memcpy(upperdir, optstart, optend - optstart);

Isn't this just a complicated way how to re-implement strndup()?

> +			break;
> +		} else {
> +			tst_brkm(TBROK, NULL,
> +				 "mount point %s does not contain an upperdir",
> +				 mountpoint);
> +		}
> +	}
> +	endmntent(mntf);
> +
> +	if (!upperdir)
> +		tst_brkm(TBROK, NULL,
> +			 "Unable to find mount point \"%s\" in mount table",
> +			 mountpoint);
> +
> +	return upperdir;
> +}
> +
> +/*
> + * To get from a file or directory on an overlayfs to a device
> + * for an underlying mountpoint requires the following steps:
> + *
> + * 1. stat() the pathname and pick out st_dev.
> + * 2. use the st_dev to look up the mount point of the file
> + *    system in /proc/self/mountinfo
> + *
> + * Because 'mountinfo' is a much more complicated file format than
> + * 'mounts', we switch to looking up the mount point in /proc/mounts,
> + * and use the mntent.h helpers to parse the entries.
> + *
> + * 3. Using getmntent(), find the entry for the mount point identified
> + *    in step 2.
> + * 4. Call hasmntopt() to find the upperdir option, and parse that
> + *    option to get to the path name for the upper directory.
> + * 5. Call stat on the upper directory.  This should now contain
> + *    the major and minor number for the underlying device (assuming
> + *    that there aren't nested overlay file systems).
> + * 6. Populate the uevent path.
> + *
> + * Example /proc/self/mountinfo line for overlayfs:
> + *    471 69 0:48 / /tmp rw,relatime shared:242 - overlay overlay rw,seclabel,lowerdir=/tmp,upperdir=/mnt/upper/upper,workdir=/mnt/upper/work,uuid=null
> + *
> + * See section 3.5 of the kernel's Documentation/filesystems/proc.rst file
> + * for a detailed explanation of the mountinfo format.
> + */
> +static void overlay_get_uevent_path(char *tmp_path, char *uevent_path)
> +{
> +	int ret;
> +	struct stat st;
> +	char *mountpoint, *upperdir;
> +
> +	tst_resm(TINFO, "Use OVERLAYFS specific strategy");
> +
> +	ret = stat(tmp_path, &st);
> +	if (ret)
> +		tst_brkm(TBROK | TERRNO, NULL, "stat failed");
> +
> +	mountpoint = overlay_mount_from_dev(st.st_dev);
> +	upperdir = overlay_get_upperdir(mountpoint);
> +	free(mountpoint);

Since the mntpoint is only intermediate result, why can't we pass the
st.dev to the overlay_get_upperdir() and call overlay_mount_from_dev()
from there?

> +	ret = stat(upperdir, &st);
> +	free(upperdir);
> +	if (ret)
> +		tst_brkm(TBROK | TERRNO, NULL, "stat failed");
> +
> +	tst_resm(TINFO, "Warning: used first of multiple backing devices.");
> +	sprintf(uevent_path, "/sys/dev/block/%d:%d/uevent",
> +		major(st.st_dev), minor(st.st_dev));
> +}
> +
>  __attribute__((nonnull))
>  void tst_find_backing_dev(const char *path, char *dev, size_t dev_size)
>  {
> @@ -599,6 +730,8 @@ void tst_find_backing_dev(const char *path, char *dev, size_t dev_size)
>  
>  	if (fsbuf.f_type == TST_BTRFS_MAGIC) {
>  		btrfs_get_uevent_path(tmp_path, uevent_path);
> +	} else if (fsbuf.f_type == TST_OVERLAYFS_MAGIC) {
> +		overlay_get_uevent_path(tmp_path, uevent_path);
>  	} else if (dev_major == 0) {
>  		tst_brkm(TBROK, NULL, "%s resides on an unsupported pseudo-file system.", path);
>  	} else {
> -- 
> 2.43.5
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v3 3/3] tst_find_backing_dev(): add support for overlayfs

[Jeff Moyer]

Hi, Cyril,

Thanks for the review.  Comments inline.

Cyril Hrubis <chrubis@suse.cz> writes:

>> +static char *overlay_mount_from_dev(dev_t dev)
>> +{
>> +	unsigned dev_major, dev_minor, mnt_major, mnt_minor;
>> +	FILE *fp;
>> +	char line[PATH_MAX];
>
> PATH_MAX does not really make any sense here. It's as good as any other
> number so I would just hardcode 4096 here.

Agreed.  Also, the theoretical max is beyond 4k, but it shouldn't be a
practical issue.  I did struggle with plucking a value out of thin air,
here.

>> +		if (ret != 3)
>> +			tst_brkm(TBROK, NULL,
>> +				 "failed to parse mountinfo line: \"%s\"",
>                                                                    ^
>                 We usually use ' instead of " inside of strings in LTP.

Ok.

>> +	if (!mountpoint)
>> +		tst_brkm(TBROK, NULL,
>> +			 "Unable to find mount entry for device %u:%u\n",
>                                                                       ^
> 							No newlines in
> 							tst_*()
> 							messages please.

Oops!

>> +	while ((mnt = getmntent(mntf)) != NULL) {
>> +		if (strncmp(mnt->mnt_dir, mountpoint, strlen(mountpoint)))
>> +			continue;
>
> Why strncmp() here? Isn't this possibly generating false positives in
> the case that we there is more mounts that have the same prefix that
> matches mountpoint?

Yes, good point.  Thanks!

>> +		if (strncmp(mnt->mnt_type, "overlay", strlen("overlay")))
>> +			tst_brkm(TBROK, NULL,
>> +				 "expected overlayfs on mount point \"%s\", but it is of type %s.",
>> +				 mountpoint, mnt->mnt_type);
>
> Here as well, I suppose that the probability of false positive here is
> close to zero, but I do not see the reason for strncmp() here either.

Agreed.

>> +		optstr = hasmntopt(mnt, "upperdir");
>> +		if (optstr) {
>> +			optstart = strchr(optstr, '=');
>> +			optstart++;
>> +			optend = strchrnul(optstr, ',');
>> +			upperdir = calloc(optend - optstart + 1, 1);
>> +			memcpy(upperdir, optstart, optend - optstart);
>
> Isn't this just a complicated way how to re-implement strndup()?

Yes.  :)  I'll fix that up.

>> +static void overlay_get_uevent_path(char *tmp_path, char *uevent_path)
>> +{
>> +	int ret;
>> +	struct stat st;
>> +	char *mountpoint, *upperdir;
>> +
>> +	tst_resm(TINFO, "Use OVERLAYFS specific strategy");
>> +
>> +	ret = stat(tmp_path, &st);
>> +	if (ret)
>> +		tst_brkm(TBROK | TERRNO, NULL, "stat failed");
>> +
>> +	mountpoint = overlay_mount_from_dev(st.st_dev);
>> +	upperdir = overlay_get_upperdir(mountpoint);
>> +	free(mountpoint);
>
> Since the mntpoint is only intermediate result, why can't we pass the
> st.dev to the overlay_get_upperdir() and call overlay_mount_from_dev()
> from there?

It makes more logical sense to me to pass a mount point to that
function.  Another argument against changing would be that
overlay_get_upperdir is already pretty large.  However, if you feel
strongly about it, I can certainly change it.

Thanks again for the thorough review!

Cheers,
Jeff


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp