[LTP] [PATCH 0/2] tst_find_backing_dev: fix stat fails /dev/root

[LTP] [PATCH 0/2] tst_find_backing_dev: fix stat fails /dev/root

[Alessandro Carminati]

Working on minor fixes, I noticed a strong dependency from the test suite
and sysfs.
Many tests, including the one that triggered the mail in the first place,
simply terminate broken when no sysfs is there.
The motivation that kept the old montinfo at its place, do not stand, and
in my opinion, the montinfocan be dropped altogether.

The following is an example of what happen to a test that supports both
methods:
in this first, the test uses the uevent strategy

% /usr/lib/ltp-testsuite/testcases/bin/ioctl_loop05
tst_test.c:1363: TINFO: Timeout per run is 0h 05m 00s
tst_test.c:1115: TINFO: ext2/ext3/ext4 is supported by the test
tst_device.c:91: TINFO: Found free device 0 '/dev/loop0'
loop0: detected capacity change from 0 to 2048
tst_device.c:570: TINFO: Device name retrived through uevent
ioctl_loop05.c:126: TINFO: backing dev(/dev/sda) logical_block_size is 512
ioctl_loop05.c:62: TINFO: Without setting lo_offset or sizelimit
ioctl_loop05.c:45: TINFO: In dio mode
ioctl_loop05.c:48: TPASS: lo_flags has LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TPASS: /sys/block/loop0/loop/dio = 1
ioctl_loop05.c:45: TINFO: In non dio mode
ioctl_loop05.c:50: TPASS: lo_flags doesn't have LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TPASS: /sys/block/loop0/loop/dio = 0
ioctl_loop05.c:69: TINFO: With offset equal to logical_block_size
loop0: detected capacity change from 2048 to 2047
ioctl_loop05.c:74: TPASS: LOOP_SET_DIRECT_IO succeeded
ioctl_loop05.c:45: TINFO: In dio mode
ioctl_loop05.c:48: TPASS: lo_flags has LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TPASS: /sys/block/loop0/loop/dio = 1
ioctl_loop05.c:81: TINFO: With nonzero offset less than logical_block_size
ioctl_loop05.c:92: TPASS: LOOP_SET_DIRECT_IO failed as expected: EINVAL (22)

Summary:
passed   8
failed   0
broken   0
skipped  0
warnings 0

in this second, the sysfs is unmounted, and the test uses the backup
mountinfo strategy

% ln -s /dev/sda /dev/root
% umount /sys
% /usr/lib/ltp-testsuite/testcases/bin/ioctl_loop05
tst_test.c:1363: TINFO: Timeout per run is 0h 05m 00s
tst_test.c:1115: TINFO: ext2/ext3/ext4 is supported by the test
tst_device.c:91: TINFO: Found free device 0 '/dev/loop0'
loop0: detected capacity change from 0 to 2048
tst_device.c:570: TINFO: Device name retrived through mountinfo
ioctl_loop05.c:126: TINFO: backing dev(/dev/root) logical_block_size is 512
ioctl_loop05.c:62: TINFO: Without setting lo_offset or sizelimit
ioctl_loop05.c:45: TINFO: In dio mode
ioctl_loop05.c:48: TPASS: lo_flags has LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TBROK: Failed to open FILE '/sys/block/loop0/loop/dio' for reading: ENOENT (2)

Summary:
passed   1
failed   0
broken   1
skipped  0
warnings 0
%

The fact that whithout sysfs the backing device can't be found is not
relevant since the test fails with broken.

As a consequence of the drop of the mountinfo strategy, the API
documentation needs to be updated.

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   | 41 ++++++++++-------------------------------
 2 files changed, 13 insertions(+), 35 deletions(-)

-- 
2.34.1


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

[LTP] [PATCH 1/2] tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent

[Alessandro Carminati]

In some minimal Linux the /dev/root can be missing. The consequence for this
is that mountinfo doesn't contain the correct information.

The unevent file in sysfs is another method to retrieve device info given 
a major/minor.

btrfs subvolumes can be an exception to this rule, but they are not expected
to be used in the current usecase.

This solution requires sysfs to be mounted in /sys, but considering many 
tests depends on the same, including the ioctl_loop05 that triggered this 
patch, this requirement is not too restricted, and the old mountinfo can be 
dropped altogether.

$ sudo /usr/lib/ltp-testsuite/testcases/bin/ioctl_loop05
tst_test.c:1363: TINFO: Timeout per run is 0h 05m 00s
tst_test.c:1115: TINFO: ext2/ext3/ext4 is supported by the test
tst_device.c:91: TINFO: Found free device 0 '/dev/loop0'
loop0: detected capacity change from 0 to 2048
tst_device.c:570: TINFO: Device name retrived through mountinfo
ioctl_loop05.c:126: TINFO: backing dev(/dev/root) logical_block_size is 512
ioctl_loop05.c:62: TINFO: Without setting lo_offset or sizelimit
ioctl_loop05.c:45: TINFO: In dio mode
ioctl_loop05.c:48: TPASS: lo_flags has LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TBROK: Failed to open FILE '/sys/block/loop0/loop/dio' for reading: ENOENT (2)

Summary:
passed   1
failed   0
broken   1
skipped  0
warnings 0

The example here shows what happen if sysfs is not mounted and the
mountinfo method is used: the tests that depends on sysfs fail with
broken and not just warn.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
---
 lib/tst_device.c | 41 ++++++++++-------------------------------
 1 file changed, 10 insertions(+), 31 deletions(-)

diff --git a/lib/tst_device.c b/lib/tst_device.c
index 8419b80c3..4ccb71ac8 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -520,48 +520,27 @@ 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];
+	unsigned int dev_major, dev_minor;
+	char uevent_path[PATH_MAX];
+	char dev_name[NAME_MAX];
 
 	if (stat(path, &buf) < 0)
 		tst_brkm(TWARN | TERRNO, NULL, "stat() failed");
 
+	*dev = '\0';
 	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);
+	sprintf(uevent_path,
+		"/sys/dev/block/%d:%d/uevent", dev_major, dev_minor);
 
-		if (line_mjr == dev_major && line_mnr == dev_minor) {
-			strcpy(dev, pre);
-			break;
-		}
+	if (!access(uevent_path, R_OK)) {
+		FILE_LINES_SCANF(NULL, uevent_path, "DEVNAME=%s", dev_name);
 
-		len = count_match_len(path, mnt_point);
-		if (len > best_match_len) {
-			strcpy(dev, pre);
-			best_match_len = len;
-		}
+		if (dev_name[0])
+			sprintf(dev, "/dev/%s", dev_name);
 	}
 
-	SAFE_FCLOSE(NULL, file);
-
-	if (!*dev)
-		tst_brkm(TBROK, NULL, "Cannot find block device for %s", path);
-
 	if (stat(dev, &buf) < 0)
 		tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);
 
-- 
2.34.1


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

[LTP] [PATCH 2/2] c-test-api: Documentation updated

[Alessandro Carminati]

Since the tst_find_backing_dev logic is changed, the doc is updated
accordingly.

Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
---
 doc/c-test-api.txt | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
index 64ee3397f..b579e85ee 100644
--- a/doc/c-test-api.txt
+++ b/doc/c-test-api.txt
@@ -1067,12 +1067,11 @@ is created for that intention.
 -------------------------------------------------------------------------------
 #include "tst_test.h"
 
-voud tst_find_backing_dev(const char *path, char *dev);
+void tst_find_backing_dev(const char *path, char *dev);
 -------------------------------------------------------------------------------
 
-This function finds the block dev that this path belongs to, it uses stat function
-to get the major/minor number of the path. Then scan them in '/proc/self/mountinfo'
-and list 2th column value after ' - ' string as its block dev if match succeeds.
+This function finds the block dev that this path belongs to, it uses the unevent 
+file in sysfs to find the device name from the major and minor numbers. 
 
 [source,c]
 -------------------------------------------------------------------------------
-- 
2.34.1


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

Re: [LTP] [PATCH 1/2] tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent

[Richard Palethorpe]

Hello,

Alessandro Carminati <alessandro.carminati@gmail.com> writes:

> In some minimal Linux the /dev/root can be missing. The consequence for this
> is that mountinfo doesn't contain the correct information.
>
> The unevent file in sysfs is another method to retrieve device info given 
> a major/minor.
>
> btrfs subvolumes can be an exception to this rule, but they are not expected
> to be used in the current usecase.

Unfortunately this is not true. If you mount /tmp on BTRFS (or set
TMPDIR to some BTRFS mount), then we hit this issue.

>
> This solution requires sysfs to be mounted in /sys, but considering many 
> tests depends on the same, including the ioctl_loop05 that triggered this 
> patch, this requirement is not too restricted, and the old mountinfo can be 
> dropped altogether.

The mountinfo method is not such a maintenance issue that it needs to be
removed IMO. Possibly it could be replaced by tst_stat_mount_dev
instead, but we're cautious about touching this function.

To be clear, I'm not sure anyone compiles Linux without /sys then tries
to run LTP on it. However the fact that it is possible to remove /sys is
another signal (in addition to BTRFS) that the situation is complicated.

>
> $ sudo /usr/lib/ltp-testsuite/testcases/bin/ioctl_loop05
> tst_test.c:1363: TINFO: Timeout per run is 0h 05m 00s
> tst_test.c:1115: TINFO: ext2/ext3/ext4 is supported by the test
> tst_device.c:91: TINFO: Found free device 0 '/dev/loop0'
> loop0: detected capacity change from 0 to 2048
> tst_device.c:570: TINFO: Device name retrived through mountinfo
> ioctl_loop05.c:126: TINFO: backing dev(/dev/root) logical_block_size is 512
> ioctl_loop05.c:62: TINFO: Without setting lo_offset or sizelimit
> ioctl_loop05.c:45: TINFO: In dio mode
> ioctl_loop05.c:48: TPASS: lo_flags has LO_FLAGS_DIRECT_IO flag
> ioctl_loop05.c:52: TBROK: Failed to open FILE '/sys/block/loop0/loop/dio' for reading: ENOENT (2)
>
> Summary:
> passed   1
> failed   0
> broken   1
> skipped  0
> warnings 0
>
> The example here shows what happen if sysfs is not mounted and the
> mountinfo method is used: the tests that depends on sysfs fail with
> broken and not just warn.
>
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
> ---
>  lib/tst_device.c | 41 ++++++++++-------------------------------
>  1 file changed, 10 insertions(+), 31 deletions(-)
>
> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index 8419b80c3..4ccb71ac8 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -520,48 +520,27 @@ 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];
> +	unsigned int dev_major, dev_minor;
> +	char uevent_path[PATH_MAX];
> +	char dev_name[NAME_MAX];
>  
>  	if (stat(path, &buf) < 0)
>  		tst_brkm(TWARN | TERRNO, NULL, "stat() failed");
>  
> +	*dev = '\0';
>  	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);
> +	sprintf(uevent_path,
> +		"/sys/dev/block/%d:%d/uevent", dev_major, dev_minor);
>  
> -		if (line_mjr == dev_major && line_mnr == dev_minor) {
> -			strcpy(dev, pre);
> -			break;
> -		}
> +	if (!access(uevent_path, R_OK)) {
> +		FILE_LINES_SCANF(NULL, uevent_path, "DEVNAME=%s", dev_name);
>  
> -		len = count_match_len(path, mnt_point);
> -		if (len > best_match_len) {
> -			strcpy(dev, pre);
> -			best_match_len = len;
> -		}
> +		if (dev_name[0])
> +			sprintf(dev, "/dev/%s", dev_name);
>  	}
>  
> -	SAFE_FCLOSE(NULL, file);
> -
> -	if (!*dev)
> -		tst_brkm(TBROK, NULL, "Cannot find block device for %s", path);
> -
>  	if (stat(dev, &buf) < 0)
>  		tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH 1/2] tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent

[Alessandro Carminati]

[-- Attachment #1.1: Type: text/plain, Size: 11948 bytes --]

Hello Richard,





Il giorno gio 27 ott 2022 alle ore 11:16 Richard Palethorpe <
rpalethorpe@suse.de> ha scritto:

> Hello,
>
> Alessandro Carminati <alessandro.carminati@gmail.com> writes:
>
> > In some minimal Linux the /dev/root can be missing. The consequence for
> this
> > is that mountinfo doesn't contain the correct information.
> >
> > The unevent file in sysfs is another method to retrieve device info
> given
> > a major/minor.
> >
> > btrfs subvolumes can be an exception to this rule, but they are not
> expected
> > to be used in the current usecase.
>
> Unfortunately this is not true. If you mount /tmp on BTRFS (or set
> TMPDIR to some BTRFS mount), then we hit this issue.
>

This is also true if you use the mountinfo strategy.
I ran a few tests on my test environment, and I could see that the
ioctl_loop05
on btrfs filesystem doesn't work either with the mountinfo strategy.

Here is a sample running the original (with a few additional debug output)
test
using the mountinfo strategy on ext3:

VFS: Mounted root (ext2 filesystem) on device 8:0.
devtmpfs: mounted
Freeing unused kernel image (initmem) memory: 956K
Write protecting the kernel read-only data: 14336k
Freeing unused kernel image (text/rodata gap) memory: 2044K
Freeing unused kernel image (rodata/data gap) memory: 48K
Run /sbin/init as init process
random: fast init done
EXT4-fs (sda): re-mounted. Opts: (null). Quota mode: disabled.
Starting syslogd: OK
Starting klogd: OK
Running sysctl: OK
Saving random seed: random: dd: uninitialized urandom read (512 bytes read)
OK
Starting network: Waiting for interface eth0 to appear...............
timeout!
run-parts: /etc/network/if-pre-up.d/wait_iface: exit status 1
FAIL

Welcome to Buildroot
buildroot login: root
# /usr/lib/ltp-testsuite/testcases/bin/ioctl_loop05
tst_test.c:1363: TINFO: Timeout per run is 0h 05m 00s
tst_test.c:1115: TINFO: ext2/ext3/ext4 is supported by the test
tst_device.c:88: TINFO: Found free device 0 '/dev/loop0'
loop0: detected capacity change from 0 to 2048
tst_device.c:522: TINFO: Debug: Device numbers: 8/0
tst_device.c:527: TINFO: Debug: 14 1 8:0 / / rw,relatime - ext2 /dev/root rw
tst_device.c:543: TINFO: Debug: devneme=/dev/root
tst_device.c:548: TWARN: stat(/dev/root) failed: ENOENT (2)

Summary:
passed   0
failed   0
broken   0
skipped  0
warnings 1
# ln -s /dev/sda /dev/root
# /usr/lib/ltp-testsuite/testcases/bin/ioctl_loop05
tst_test.c:1363: TINFO: Timeout per run is 0h 05m 00s
tst_test.c:1115: TINFO: ext2/ext3/ext4 is supported by the test
tst_device.c:88: TINFO: Found free device 0 '/dev/loop0'
loop0: detected capacity change from 0 to 2048
tst_device.c:522: TINFO: Debug: Device numbers: 8/0
tst_device.c:527: TINFO: Debug: 14 1 8:0 / / rw,relatime - ext2 /dev/root rw
tst_device.c:543: TINFO: Debug: devneme=/dev/root
ioctl_loop05.c:126: TINFO: backing dev(/dev/root) logical_block_size is 512
ioctl_loop05.c:62: TINFO: Without setting lo_offset or sizelimit
ioctl_loop05.c:45: TINFO: In dio mode
ioctl_loop05.c:48: TPASS: lo_flags has LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TPASS: /sys/block/loop0/loop/dio = 1
ioctl_loop05.c:45: TINFO: In non dio mode
ioctl_loop05.c:50: TPASS: lo_flags doesn't have LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TPASS: /sys/block/loop0/loop/dio = 0
ioctl_loop05.c:69: TINFO: With offset equal to logical_block_size
loop0: detected capacity change from 2048 to 2047
ioctl_loop05.c:74: TPASS: LOOP_SET_DIRECT_IO succeeded
ioctl_loop05.c:45: TINFO: In dio mode
ioctl_loop05.c:48: TPASS: lo_flags has LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TPASS: /sys/block/loop0/loop/dio = 1
ioctl_loop05.c:81: TINFO: With nonzero offset less than logical_block_size
ioctl_loop05.c:92: TPASS: LOOP_SET_DIRECT_IO failed as expected: EINVAL (22)

Summary:
passed   8
failed   0
broken   0
skipped  0
warnings 0

here the same system on a btrfs

VFS: Mounted root (btrfs filesystem) on device 0:13.
random: fast init done
devtmpfs: mounted
Freeing unused kernel image (initmem) memory: 956K
Write protecting the kernel read-only data: 14336k
Freeing unused kernel image (text/rodata gap) memory: 2044K
Freeing unused kernel image (rodata/data gap) memory: 48K
Run /sbin/init as init process
BTRFS info (device sda): disk space caching is enabled
Starting syslogd: OK
Starting klogd: OK
Running sysctl: OK
Saving random seed: random: dd: uninitialized urandom read (512 bytes read)
OK
Starting network: Waiting for interface eth0 to appear...............
timeout!
run-parts: /etc/network/if-pre-up.d/wait_iface: exit status 1
FAIL

Welcome to Buildroot
buildroot login: root
# kill 71
# umount /tmp
# umount /run
# /usr/lib/ltp-testsuite/testcases/bin/ioctl_loop05
tst_test.c:1363: TINFO: Timeout per run is 0h 05m 00s
tst_test.c:1115: TINFO: btrfs is supported by the test
tst_device.c:88: TINFO: Found free device 0 '/dev/loop0'
loop0: detected capacity change from 0 to 2048
tst_device.c:522: TINFO: Debug: Device numbers: 0/14
tst_device.c:527: TINFO: Debug: 15 1 0:13 / / rw,relatime - btrfs /dev/root
rw,space_cache,subvolid=5,subvol=/
tst_device.c:527: TINFO: Debug: 16 15 0:5 / /dev rw,relatime - devtmpfs
devtmpfs rw,size=505416k,nr_inodes=126354,mode=755
tst_device.c:527: TINFO: Debug: 14 15 0:15 / /proc rw,relatime - proc proc
rw
tst_device.c:527: TINFO: Debug: 17 16 0:16 / /dev/pts rw,relatime - devpts
devpts rw,gid=5,mode=620,ptmxmode=666
tst_device.c:527: TINFO: Debug: 18 16 0:17 / /dev/shm rw,relatime - tmpfs
tmpfs rw,mode=777
tst_device.c:527: TINFO: Debug: 21 15 0:20 / /sys rw,relatime - sysfs sysfs
rw
tst_device.c:543: TINFO: Debug: devneme=
tst_device.c:545: TBROK: Cannot find block device for
/tmp/iocz4RUVG/test.img

Summary:
passed   0
failed   0
broken   1
skipped  0
warnings 0
# ln -s /dev/sda /dev/root
# /usr/lib/ltp-testsuite/testcases/bin/ioctl_loop05
tst_test.c:1363: TINFO: Timeout per run is 0h 05m 00s
tst_test.c:1115: TINFO: btrfs is supported by the test
tst_device.c:88: TINFO: Found free device 0 '/dev/loop0'
loop0: detected capacity change from 0 to 2048
tst_device.c:522: TINFO: Debug: Device numbers: 0/14
tst_device.c:527: TINFO: Debug: 15 1 0:13 / / rw,relatime - btrfs /dev/root
rw,space_cache,subvolid=5,subvol=/
tst_device.c:527: TINFO: Debug: 16 15 0:5 / /dev rw,relatime - devtmpfs
devtmpfs rw,size=505416k,nr_inodes=126354,mode=755
tst_device.c:527: TINFO: Debug: 14 15 0:15 / /proc rw,relatime - proc proc
rw
tst_device.c:527: TINFO: Debug: 17 16 0:16 / /dev/pts rw,relatime - devpts
devpts rw,gid=5,mode=620,ptmxmode=666
tst_device.c:527: TINFO: Debug: 18 16 0:17 / /dev/shm rw,relatime - tmpfs
tmpfs rw,mode=777
tst_device.c:527: TINFO: Debug: 21 15 0:20 / /sys rw,relatime - sysfs sysfs
rw
tst_device.c:543: TINFO: Debug: devneme=
tst_device.c:545: TBROK: Cannot find block device for
/tmp/ioctVo52r/test.img

Summary:
passed   0
failed   0
broken   1
skipped  0
warnings 0
#

a stat on the /, results in a couple of minor/major that is different from
the
couple listed in the mountinfo. This leads, as a result, a test failure.


> >
> > This solution requires sysfs to be mounted in /sys, but considering many
> > tests depends on the same, including the ioctl_loop05 that triggered
> this
> > patch, this requirement is not too restricted, and the old mountinfo can
> be
> > dropped altogether.
>
> The mountinfo method is not such a maintenance issue that it needs to be
> removed IMO. Possibly it could be replaced by tst_stat_mount_dev
> instead, but we're cautious about touching this function.
>

tst_find_backing_dev(), the function that is the target of this patch,
seems to
be referenced in only a couple of points in all the LTP test suite.
One place is in the ioctl_loop05 test, the other reference I found is in
the
lib/tst_device.c - tst_dev_block_size().  tst_dev_block_size() is a
function
that seems not to be referenced by any test.


>
> To be clear, I'm not sure anyone compiles Linux without /sys then tries
> to run LTP on it. However the fact that it is possible to remove /sys is
> another signal (in addition to BTRFS) that the situation is complicated.
>

Indeed, if we want to have a test that works in all the possible scenarios,
it needs additional work.
But IMHO, keeping the mountinfo strategy gives no advantage over not
having it.


> >
> > $ sudo /usr/lib/ltp-testsuite/testcases/bin/ioctl_loop05
> > tst_test.c:1363: TINFO: Timeout per run is 0h 05m 00s
> > tst_test.c:1115: TINFO: ext2/ext3/ext4 is supported by the test
> > tst_device.c:91: TINFO: Found free device 0 '/dev/loop0'
> > loop0: detected capacity change from 0 to 2048
> > tst_device.c:570: TINFO: Device name retrived through mountinfo
> > ioctl_loop05.c:126: TINFO: backing dev(/dev/root) logical_block_size is
> 512
> > ioctl_loop05.c:62: TINFO: Without setting lo_offset or sizelimit
> > ioctl_loop05.c:45: TINFO: In dio mode
> > ioctl_loop05.c:48: TPASS: lo_flags has LO_FLAGS_DIRECT_IO flag
> > ioctl_loop05.c:52: TBROK: Failed to open FILE
> '/sys/block/loop0/loop/dio' for reading: ENOENT (2)
> >
> > Summary:
> > passed   1
> > failed   0
> > broken   1
> > skipped  0
> > warnings 0
> >
> > The example here shows what happen if sysfs is not mounted and the
> > mountinfo method is used: the tests that depends on sysfs fail with
> > broken and not just warn.
> >
> > Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> > Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
> > ---
> >  lib/tst_device.c | 41 ++++++++++-------------------------------
> >  1 file changed, 10 insertions(+), 31 deletions(-)
> >
> > diff --git a/lib/tst_device.c b/lib/tst_device.c
> > index 8419b80c3..4ccb71ac8 100644
> > --- a/lib/tst_device.c
> > +++ b/lib/tst_device.c
> > @@ -520,48 +520,27 @@ 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];
> > +     unsigned int dev_major, dev_minor;
> > +     char uevent_path[PATH_MAX];
> > +     char dev_name[NAME_MAX];
> >
> >       if (stat(path, &buf) < 0)
> >               tst_brkm(TWARN | TERRNO, NULL, "stat() failed");
> >
> > +     *dev = '\0';
> >       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);
> > +     sprintf(uevent_path,
> > +             "/sys/dev/block/%d:%d/uevent", dev_major, dev_minor);
> >
> > -             if (line_mjr == dev_major && line_mnr == dev_minor) {
> > -                     strcpy(dev, pre);
> > -                     break;
> > -             }
> > +     if (!access(uevent_path, R_OK)) {
> > +             FILE_LINES_SCANF(NULL, uevent_path, "DEVNAME=%s",
> dev_name);
> >
> > -             len = count_match_len(path, mnt_point);
> > -             if (len > best_match_len) {
> > -                     strcpy(dev, pre);
> > -                     best_match_len = len;
> > -             }
> > +             if (dev_name[0])
> > +                     sprintf(dev, "/dev/%s", dev_name);
> >       }
> >
> > -     SAFE_FCLOSE(NULL, file);
> > -
> > -     if (!*dev)
> > -             tst_brkm(TBROK, NULL, "Cannot find block device for %s",
> path);
> > -
> >       if (stat(dev, &buf) < 0)
> >               tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);
>
>
> --
> Thank you,
> Richard.
>

Thank you for your time.
Alessandro

[-- Attachment #1.2: Type: text/html, Size: 15292 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

Re: [LTP] [PATCH 1/2] tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent

[Richard Palethorpe]

Hello,

Alessandro Carminati <alessandro.carminati@gmail.com> writes:

> Hello Richard,
>
> Il giorno gio 27 ott 2022 alle ore 11:16 Richard Palethorpe <rpalethorpe@suse.de> ha scritto:
>
>  Hello,
>
>  Alessandro Carminati <alessandro.carminati@gmail.com> writes:
>
>  > In some minimal Linux the /dev/root can be missing. The consequence for this
>  > is that mountinfo doesn't contain the correct information.
>  >
>  > The unevent file in sysfs is another method to retrieve device info given 
>  > a major/minor.
>  >
>  > btrfs subvolumes can be an exception to this rule, but they are not expected
>  > to be used in the current usecase.
>
>  Unfortunately this is not true. If you mount /tmp on BTRFS (or set
>  TMPDIR to some BTRFS mount), then we hit this issue.
>
> This is also true if you use the mountinfo strategy.
> I ran a few tests on my test environment, and I could see that the ioctl_loop05 
> on btrfs filesystem doesn't work either with the mountinfo strategy.

OK, so to summarise niether strategy works when the FS is BTRFS and init
has not done something about /dev/root.

I suppose part of the problem is BTRFS can span multiple devices
(IIUC). So there is no definite single backing device.

The command "btrfs devices stat <path>", uses the BTRFS_IOC_DEV_INFO
ioctl to get backing device paths.

>
>  >
>  > This solution requires sysfs to be mounted in /sys, but considering many 
>  > tests depends on the same, including the ioctl_loop05 that triggered this 
>  > patch, this requirement is not too restricted, and the old mountinfo can be 
>  > dropped altogether.
>
>  The mountinfo method is not such a maintenance issue that it needs to be
>  removed IMO. Possibly it could be replaced by tst_stat_mount_dev
>  instead, but we're cautious about touching this function.
>
> tst_find_backing_dev(), the function that is the target of this patch, seems to 
> be referenced in only a couple of points in all the LTP test suite.
> One place is in the ioctl_loop05 test, the other reference I found is in the 
> lib/tst_device.c - tst_dev_block_size().  tst_dev_block_size() is a function 
> that seems not to be referenced by any test.
>  
>  
>  To be clear, I'm not sure anyone compiles Linux without /sys then tries
>  to run LTP on it. However the fact that it is possible to remove /sys is
>  another signal (in addition to BTRFS) that the situation is complicated.
>
> Indeed, if we want to have a test that works in all the possible scenarios, 
> it needs additional work. 
> But IMHO, keeping the mountinfo strategy gives no advantage over not 
> having it. 
>

Well it allows the test to work on BTRFS in most situations. Possibly if
we find that the FS is BTRFS, the device is /dev/root and it doesn't
exist. Then we should call tst_brk(TCONF...

AFAICT what the test actually wants is the block device sector size
(BLKSSZGET). Possibly this can be retrieved with the BTRFS_IOC_FS_INFO
ioctl.

The final option would be to try using some other BTRFS specific ioctl
to get the backing device(s). If there is more than one then just return
the first. That's probably not worth the effort for the current test,
but I think it is quite likely this issue will arise again. io_control01
has a similar requirement.

-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH 1/2] tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent

[Alessandro Carminati]

Hello Richard,

Il giorno lun 31 ott 2022 alle ore 13:54 Richard Palethorpe
<rpalethorpe@suse.de> ha scritto:
>
> Hello,
>
> Alessandro Carminati <alessandro.carminati@gmail.com> writes:
>
> > Hello Richard,
> >
> > Il giorno gio 27 ott 2022 alle ore 11:16 Richard Palethorpe <rpalethorpe@suse.de> ha scritto:
> >
> >  Hello,
> >
> >  Alessandro Carminati <alessandro.carminati@gmail.com> writes:
> >
> >  > In some minimal Linux the /dev/root can be missing. The consequence for this
> >  > is that mountinfo doesn't contain the correct information.
> >  >
> >  > The unevent file in sysfs is another method to retrieve device info given
> >  > a major/minor.
> >  >
> >  > btrfs subvolumes can be an exception to this rule, but they are not expected
> >  > to be used in the current usecase.
> >
> >  Unfortunately this is not true. If you mount /tmp on BTRFS (or set
> >  TMPDIR to some BTRFS mount), then we hit this issue.
> >
> > This is also true if you use the mountinfo strategy.
> > I ran a few tests on my test environment, and I could see that the ioctl_loop05
> > on btrfs filesystem doesn't work either with the mountinfo strategy.
>
> OK, so to summarise niether strategy works when the FS is BTRFS and init
> has not done something about /dev/root.
>
> I suppose part of the problem is BTRFS can span multiple devices
> (IIUC). So there is no definite single backing device.
>
> The command "btrfs devices stat <path>", uses the BTRFS_IOC_DEV_INFO
> ioctl to get backing device paths.

Yes, use the ioctl could be a solution for btrfs.
I ran a little test to understand what filesystem I can expect.
ioctl_loop05 is supported on a few filesystems, I tried listing them,
and on a few, I run the test:

| Filesystem    | tested | notes
+---------------+--------+----------------------------
|nfs            | no     | not tested, no idea.
|9p             | no     | not tested, no idea.
|ramfs          | no     | theoretically possible to have root on
ramfs, but I think it very unlikely
|btrfs          | yes    | both strategies have problems with it.
|xfs            | yes    | tested ok.
|ext2/ext3/ext4 | yes    | tested ok.
|minix          | yes    | max size is 64MB, possible but not
probable. Stat on mounted fs shows minor/major corresponding to
backend device.
|udf            | no     | optical media filesystem. Very unlikely to
have it as root filesystem. Stat on mounted fs shows minor/major
corresponding to backend device.
|sysv           | no     | not tested, no idea.
|ufs            | no     | not tested, no idea.
|f2fs           | yes    | tested ok.
|nilfs          | yes    | tested ok.
|exofs          | no     | it has been removed from kernel supported
FS since 5.1
|fuse           | no     | Unlikely.
|vfat           | no     | I'm not aware you can mount vfat as rootfs.
Since it lacks on a lot of features, it is unlikely having it as
rootfs.
|exfat          | no     | not tested, no idea.

It seems to me likely that btrfs is the only filesystem, among the
supported, that can present virtual devices with different
minor/major.
I'm not 100% sure, but I would bet on the fact that we can identify
these btrfs scenarios just by looking at the major number. If it is 0
we are dealing with it.
That been said, if major == 0 we can extract the backing device by
using the ioct, as you suggested:
        struct btrfs_ioctl_dev_info_args di_args = {0};
        ioctl(fd, BTRFS_IOC_DEV_INFO, &di_args)
and use di_args.path

Do you see any downside on such approach?

>
> >
> >  >
> >  > This solution requires sysfs to be mounted in /sys, but considering many
> >  > tests depends on the same, including the ioctl_loop05 that triggered this
> >  > patch, this requirement is not too restricted, and the old mountinfo can be
> >  > dropped altogether.
> >
> >  The mountinfo method is not such a maintenance issue that it needs to be
> >  removed IMO. Possibly it could be replaced by tst_stat_mount_dev
> >  instead, but we're cautious about touching this function.
> >
> > tst_find_backing_dev(), the function that is the target of this patch, seems to
> > be referenced in only a couple of points in all the LTP test suite.
> > One place is in the ioctl_loop05 test, the other reference I found is in the
> > lib/tst_device.c - tst_dev_block_size().  tst_dev_block_size() is a function
> > that seems not to be referenced by any test.
> >
> >
> >  To be clear, I'm not sure anyone compiles Linux without /sys then tries
> >  to run LTP on it. However the fact that it is possible to remove /sys is
> >  another signal (in addition to BTRFS) that the situation is complicated.
> >
> > Indeed, if we want to have a test that works in all the possible scenarios,
> > it needs additional work.
> > But IMHO, keeping the mountinfo strategy gives no advantage over not
> > having it.
> >
>
> Well it allows the test to work on BTRFS in most situations. Possibly if
> we find that the FS is BTRFS, the device is /dev/root and it doesn't
> exist. Then we should call tst_brk(TCONF...
>

I ran a multiple tests with btrfs, and the mountinfo strategy.
I didn't find a single scenario where it succeded.
In my situation I always had that the backend device presnted a
different couple minor/major with the one listed by mountinfo.
Could you suggest a btrfs scenario where the mountinfo strategy ends
with success?

> AFAICT what the test actually wants is the block device sector size
> (BLKSSZGET). Possibly this can be retrieved with the BTRFS_IOC_FS_INFO
> ioctl.
>
> The final option would be to try using some other BTRFS specific ioctl
> to get the backing device(s). If there is more than one then just return
> the first. That's probably not worth the effort for the current test,
> but I think it is quite likely this issue will arise again. io_control01
> has a similar requirement.
>
> --
> Thank you,
> Richard.

Cheers
Alessandro Carminati

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

Re: [LTP] [PATCH 1/2] tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent

[Richard Palethorpe]

Hello,

Alessandro Carminati <alessandro.carminati@gmail.com> writes:

> Hello Richard,
>
> Il giorno lun 31 ott 2022 alle ore 13:54 Richard Palethorpe
> <rpalethorpe@suse.de> ha scritto:
>>
>> Hello,
>>
>> Alessandro Carminati <alessandro.carminati@gmail.com> writes:
>>
>> > Hello Richard,
>> >
>> > Il giorno gio 27 ott 2022 alle ore 11:16 Richard Palethorpe <rpalethorpe@suse.de> ha scritto:
>> >
>> >  Hello,
>> >
>> >  Alessandro Carminati <alessandro.carminati@gmail.com> writes:
>> >
>> >  > In some minimal Linux the /dev/root can be missing. The consequence for this
>> >  > is that mountinfo doesn't contain the correct information.
>> >  >
>> >  > The unevent file in sysfs is another method to retrieve device info given
>> >  > a major/minor.
>> >  >
>> >  > btrfs subvolumes can be an exception to this rule, but they are not expected
>> >  > to be used in the current usecase.
>> >
>> >  Unfortunately this is not true. If you mount /tmp on BTRFS (or set
>> >  TMPDIR to some BTRFS mount), then we hit this issue.
>> >
>> > This is also true if you use the mountinfo strategy.
>> > I ran a few tests on my test environment, and I could see that the ioctl_loop05
>> > on btrfs filesystem doesn't work either with the mountinfo strategy.
>>
>> OK, so to summarise niether strategy works when the FS is BTRFS and init
>> has not done something about /dev/root.
>>
>> I suppose part of the problem is BTRFS can span multiple devices
>> (IIUC). So there is no definite single backing device.
>>
>> The command "btrfs devices stat <path>", uses the BTRFS_IOC_DEV_INFO
>> ioctl to get backing device paths.
>
> Yes, use the ioctl could be a solution for btrfs.
> I ran a little test to understand what filesystem I can expect.
> ioctl_loop05 is supported on a few filesystems, I tried listing them,
> and on a few, I run the test:

Interesting, thanks.

>
> | Filesystem    | tested | notes
> +---------------+--------+----------------------------
> |nfs            | no     | not tested, no idea.
> |9p             | no     | not tested, no idea.

Indeed I have no idea what running LTP on networked FS (including Ceph)
produces.

> |ramfs          | no     | theoretically possible to have root on
> ramfs, but I think it very unlikely
> |btrfs          | yes    | both strategies have problems with it.
> |xfs            | yes    | tested ok.
> |ext2/ext3/ext4 | yes    | tested ok.
> |minix          | yes    | max size is 64MB, possible but not
> probable. Stat on mounted fs shows minor/major corresponding to
> backend device.
> |udf            | no     | optical media filesystem. Very unlikely to
> have it as root filesystem. Stat on mounted fs shows minor/major
> corresponding to backend device.
> |sysv           | no     | not tested, no idea.
> |ufs            | no     | not tested, no idea.
> |f2fs           | yes    | tested ok.
> |nilfs          | yes    | tested ok.
> |exofs          | no     | it has been removed from kernel supported
> FS since 5.1
> |fuse           | no     | Unlikely.
> |vfat           | no     | I'm not aware you can mount vfat as rootfs.
> Since it lacks on a lot of features, it is unlikely having it as
> rootfs.
> |exfat          | no     | not tested, no idea.
>
> It seems to me likely that btrfs is the only filesystem, among the
> supported, that can present virtual devices with different
> minor/major.

I suppose CephFS would also, but we make no attempt to support that in
LTP.

> I'm not 100% sure, but I would bet on the fact that we can identify
> these btrfs scenarios just by looking at the major number. If it is 0
> we are dealing with it.
> That been said, if major == 0 we can extract the backing device by
> using the ioct, as you suggested:
>         struct btrfs_ioctl_dev_info_args di_args = {0};
>         ioctl(fd, BTRFS_IOC_DEV_INFO, &di_args)
> and use di_args.path
>
> Do you see any downside on such approach?

Only the complication of adding a special approach for BTRFS. If we find
more FS with this issue, then we may have to add code for them also.

I would accept this approach though, because it is more direct and it's
not clear that scanning mountinfo will work either.

>
>>
>> >
>> >  >
>> >  > This solution requires sysfs to be mounted in /sys, but considering many
>> >  > tests depends on the same, including the ioctl_loop05 that triggered this
>> >  > patch, this requirement is not too restricted, and the old mountinfo can be
>> >  > dropped altogether.
>> >
>> >  The mountinfo method is not such a maintenance issue that it needs to be
>> >  removed IMO. Possibly it could be replaced by tst_stat_mount_dev
>> >  instead, but we're cautious about touching this function.
>> >
>> > tst_find_backing_dev(), the function that is the target of this patch, seems to
>> > be referenced in only a couple of points in all the LTP test suite.
>> > One place is in the ioctl_loop05 test, the other reference I found is in the
>> > lib/tst_device.c - tst_dev_block_size().  tst_dev_block_size() is a function
>> > that seems not to be referenced by any test.
>> >
>> >
>> >  To be clear, I'm not sure anyone compiles Linux without /sys then tries
>> >  to run LTP on it. However the fact that it is possible to remove /sys is
>> >  another signal (in addition to BTRFS) that the situation is complicated.
>> >
>> > Indeed, if we want to have a test that works in all the possible scenarios,
>> > it needs additional work.
>> > But IMHO, keeping the mountinfo strategy gives no advantage over not
>> > having it.
>> >
>>
>> Well it allows the test to work on BTRFS in most situations. Possibly if
>> we find that the FS is BTRFS, the device is /dev/root and it doesn't
>> exist. Then we should call tst_brk(TCONF...
>>
>
> I ran a multiple tests with btrfs, and the mountinfo strategy.
> I didn't find a single scenario where it succeded.
> In my situation I always had that the backend device presnted a
> different couple minor/major with the one listed by mountinfo.
> Could you suggest a btrfs scenario where the mountinfo strategy ends
> with success?

The major and minor are never correct in mountinfo, but the device path
is correct most of the time. This can be used to get the real major and
minor if necessary or get the sector size as in this case.

To verify that it works, you can run the test with TMPDIR set to a path
on BTRFS. For e.g. using a loop device with BTRFS.

$ cd /tmp
$ fallocate -l 300MiB btrfs.img
$ losetup -f btrfs.img
$ mkfs.btrfs /dev/loop0
$ mkdir btrfs
$ mount -t btrfs /dev/loop0 btrfs
$ export TMPDIR=/tmp/btrfs
$ ioctl_loop05

>
>> AFAICT what the test actually wants is the block device sector size
>> (BLKSSZGET). Possibly this can be retrieved with the BTRFS_IOC_FS_INFO
>> ioctl.
>>
>> The final option would be to try using some other BTRFS specific ioctl
>> to get the backing device(s). If there is more than one then just return
>> the first. That's probably not worth the effort for the current test,
>> but I think it is quite likely this issue will arise again. io_control01
>> has a similar requirement.
>>
>> --
>> Thank you,
>> Richard.
>
> Cheers
> Alessandro Carminati


-- 
Thank you,
Richard.

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

[LTP] [PATCH 0/2] tst_find_backing_dev: fix stat fails /dev/root

[Alessandro Carminati]

This tst_find_backing_dev patch version adds support for btrfs
filesystems.
Strategy here is to understand if the subject is btrfs or not.
btrfs uses virtual device, and the major is 0. In all other test
supported file systems the major is always non zero.

If non btrfs it proceed as the previous version using sysfs minor
and major.
If btrfs it uses the BTRFS_IOC_FS_INFO ioctl to get it FS uuid.

non btrfs:
----------------------------------------------------------------
EXT4-fs (sda): re-mounted. Opts: (null). Quota mode: none.
Welcome to Buildroot
buildroot login: root
# /usr/lib/ltp-testsuite/testcases/bin/ioctl_loop05
tst_test.c:1363: TINFO: Timeout per run is 0h 05m 00s
tst_test.c:1115: TINFO: ext2/ext3/ext4 is supported by the test
tst_device.c:90: TINFO: Found free device 0 '/dev/loop0'
loop0: detected capacity change from 0 to 2048
tst_device.c:579: TINFO: Device numbers: 8/0
tst_device.c:611: TINFO: Use uevent strategy
ioctl_loop05.c:126: TINFO: backing dev(/dev/sda) logical_block_size is 512
ioctl_loop05.c:62: TINFO: Without setting lo_offset or sizelimit
ioctl_loop05.c:45: TINFO: In dio mode
ioctl_loop05.c:48: TPASS: lo_flags has LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TPASS: /sys/block/loop0/loop/dio = 1
ioctl_loop05.c:45: TINFO: In non dio mode
ioctl_loop05.c:50: TPASS: lo_flags doesn't have LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TPASS: /sys/block/loop0/loop/dio = 0
ioctl_loop05.c:69: TINFO: With offset equal to logical_block_size
loop0: detected capacity change from 2048 to 2047
ioctl_loop05.c:74: TPASS: LOOP_SET_DIRECT_IO succeeded
ioctl_loop05.c:45: TINFO: In dio mode
ioctl_loop05.c:48: TPASS: lo_flags has LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TPASS: /sys/block/loop0/loop/dio = 1
ioctl_loop05.c:81: TINFO: With nonzero offset less than logical_block_size
ioctl_loop05.c:92: TPASS: LOOP_SET_DIRECT_IO failed as expected: EINVAL (22)

Summary:
passed   8
failed   0
broken   0
skipped  0
warnings 0
#

btrfs
----------------------------------------------------------------
BTRFS info (device sda): disk space caching is enabled
Welcome to Buildroot
buildroot login: root
# /usr/lib/ltp-testsuite/testcases/bin/ioctl_loop05
tst_test.c:1363: TINFO: Timeout per run is 0h 05m 00s
tst_test.c:1115: TINFO: btrfs is supported by the test
tst_device.c:90: TINFO: Found free device 0 '/dev/loop0'
loop0: detected capacity change from 0 to 2048
tst_device.c:579: TINFO: Device numbers: 0/15
tst_device.c:582: TINFO: Use BTRFS specific strategy
ioctl_loop05.c:126: TINFO: backing dev(/dev/sda) logical_block_size is 512
ioctl_loop05.c:62: TINFO: Without setting lo_offset or sizelimit
ioctl_loop05.c:45: TINFO: In dio mode
ioctl_loop05.c:48: TPASS: lo_flags has LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TPASS: /sys/block/loop0/loop/dio = 1
ioctl_loop05.c:45: TINFO: In non dio mode
ioctl_loop05.c:50: TPASS: lo_flags doesn't have LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TPASS: /sys/block/loop0/loop/dio = 0
ioctl_loop05.c:69: TINFO: With offset equal to logical_block_size
loop0: detected capacity change from 2048 to 2047
ioctl_loop05.c:74: TPASS: LOOP_SET_DIRECT_IO succeeded
ioctl_loop05.c:45: TINFO: In dio mode
ioctl_loop05.c:48: TPASS: lo_flags has LO_FLAGS_DIRECT_IO flag
ioctl_loop05.c:52: TPASS: /sys/block/loop0/loop/dio = 1
ioctl_loop05.c:81: TINFO: With nonzero offset less than logical_block_size
ioctl_loop05.c:87: TPASS: LOOP_SET_DIRECT_IO succeeded, offset is ignored

Summary:
passed   8
failed   0
broken   0
skipped  0
warnings 0
#

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   | 86 ++++++++++++++++++++++++++++++----------------
 2 files changed, 62 insertions(+), 31 deletions(-)

-- 
2.34.1


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

[LTP] [PATCH 1/2] tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent

[Alessandro Carminati]

In some minimal Linux, the /dev/root can be missing. The consequence of
this is that mountinfo doesn't contain the correct information. btrfs
file systems are yet another point of trouble for this function.

The unevent file in sysfs is another method to retrieve device info
using the sysfs.

btrfs file systems are special from the device name retrieval, and in
place of use of the minor/major they are approached by using the uuid.
In the end, btrfs strategy is a slightly modified version of the same
unevent strategy.

Non btrfs look in "/sys/dev/block/%d:%d/uevent" major, minor
btrfs look in /sys/fs/btrfs/%s/devices/%s/uevent, uuid, devname

The btrfs handling requires BTRFS specific ioctl for finding the
file system uuid, and for this reason, btrfs/ioctl.h is needed.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
---
 lib/tst_device.c | 86 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 57 insertions(+), 29 deletions(-)

diff --git a/lib/tst_device.c b/lib/tst_device.c
index 8419b80c3..4c2bde846 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -33,6 +33,8 @@
 #include <stdint.h>
 #include <inttypes.h>
 #include <sys/sysmacros.h>
+#include <btrfs/ioctl.h>
+#include <dirent.h>
 #include "lapi/syscalls.h"
 #include "test.h"
 #include "safe_macros.h"
@@ -45,6 +47,7 @@
 
 #define DEV_FILE "test_dev.img"
 #define DEV_SIZE_MB 300u
+#define UUID_STR_SZ 37
 
 static char dev_path[1024];
 static int device_acquired;
@@ -519,48 +522,73 @@ static int count_match_len(const char *first, const char *second)
 void tst_find_backing_dev(const char *path, char *dev)
 {
 	struct stat buf;
+	struct btrfs_ioctl_fs_info_args args = {0};
+	struct dirent *d;
 	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];
+	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 = '\0';
 	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");
+		dir = opendir("/");
+		fd = dirfd(dir);
+		if (!ioctl(fd, BTRFS_IOC_FS_INFO, &args)) {
+			sprintf(btrfs_uuid_str,
+				"%02x%02x%02x%02x-%02x%02x-"
+				"%02x%02x-%02x%02x-"
+				"%02x%02x%02x%02x%02x%02x",
+				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;
+		closedir(dir);
+		dir = opendir(bdev_path);
+		if (!dir){
+			tst_brkm(TWARN | TERRNO, NULL,
+				"can't opendir %d", dir);
+		}
+		while (d = readdir(dir)){
+			if (d->d_name[0]!='.')
+				break;
+		}
+		uevent_path[0]='\0';
+		closedir(dir);
+		if (d!=NULL) {
+			sprintf(uevent_path, "%s/%s/uevent",
+				bdev_path, d->d_name);
 		}
+	} 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);
+	}
 
 	if (stat(dev, &buf) < 0)
 		tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);
-- 
2.34.1


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

[LTP] [PATCH 2/2] c-test-api: Documentation updated

[Alessandro Carminati]

Since the tst_find_backing_dev logic is changed, the doc is updated
accordingly.

Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
---
 doc/c-test-api.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
index 64ee3397f..a7888c242 100644
--- a/doc/c-test-api.txt
+++ b/doc/c-test-api.txt
@@ -1071,8 +1071,11 @@ voud tst_find_backing_dev(const char *path, char *dev);
 -------------------------------------------------------------------------------
 
 This function finds the block dev that this path belongs to, it uses stat function
-to get the major/minor number of the path. Then scan them in '/proc/self/mountinfo'
-and list 2th column value after ' - ' string as its block dev if match succeeds.
+to get the major/minor number of the path. 
+This function finds the block dev that this path belongs to, it uses the unevent 
+file in sysfs to find the device name. It needs to discriminate between btrfs
+and not btrfs. For non btrfs filesystems it uses the minor, major numbers. For
+btrfs it uses the fs uuid.
 
 [source,c]
 -------------------------------------------------------------------------------
-- 
2.34.1


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

Re: [LTP] [PATCH 1/2] tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent

[Richard Palethorpe]

Hello,

Alessandro Carminati <alessandro.carminati@gmail.com> writes:

> In some minimal Linux, the /dev/root can be missing. The consequence of
> this is that mountinfo doesn't contain the correct information. btrfs
> file systems are yet another point of trouble for this function.
>
> The unevent file in sysfs is another method to retrieve device info
> using the sysfs.
>
> btrfs file systems are special from the device name retrieval, and in
> place of use of the minor/major they are approached by using the uuid.
> In the end, btrfs strategy is a slightly modified version of the same
> unevent strategy.
>
> Non btrfs look in "/sys/dev/block/%d:%d/uevent" major, minor
> btrfs look in /sys/fs/btrfs/%s/devices/%s/uevent, uuid, devname
>
> The btrfs handling requires BTRFS specific ioctl for finding the
> file system uuid, and for this reason, btrfs/ioctl.h is needed.

Fundamentally this looks like a much better solution. However there are
a bunch of details to work out.

>
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
> ---
>  lib/tst_device.c | 86 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 57 insertions(+), 29 deletions(-)
>
> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index 8419b80c3..4c2bde846 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -33,6 +33,8 @@
>  #include <stdint.h>
>  #include <inttypes.h>
>  #include <sys/sysmacros.h>
> +#include <btrfs/ioctl.h>

This header won't exist on a lot of systems
(e.g. https://github.com/richiejp/ltp/actions/runs/3384063391/jobs/5620599611). Instead
we can import <linux/btrfs.h> which is in the kernel headers and we
already rely on <linux/loop.h>.

It seems though that the definitions we need are 10+ years old and not
arch or config specific, so we dont' have to add them to
lapi/btrfs.h. Unless it turns out Android removes them or something like
that.

So I think it is safe just to switch this to linux/btrfs.h.

> +#include <dirent.h>
>  #include "lapi/syscalls.h"
>  #include "test.h"
>  #include "safe_macros.h"
> @@ -45,6 +47,7 @@
>  
>  #define DEV_FILE "test_dev.img"
>  #define DEV_SIZE_MB 300u
> +#define UUID_STR_SZ 37
>  
>  static char dev_path[1024];
>  static int device_acquired;
> @@ -519,48 +522,73 @@ static int count_match_len(const char *first, const char *second)
>  void tst_find_backing_dev(const char *path, char *dev)
>  {
>  	struct stat buf;
> +	struct btrfs_ioctl_fs_info_args args = {0};
> +	struct dirent *d;
>  	FILE *file;

This var is now unused. Please look at the compiler warnings (and/or
configure your IDE to display them inline with LSP/clangd or similar).

Please silence any warnings relating to your changes otherwise serious
errors get lost in the noise.

> -	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];
> +	char uevent_path[PATH_MAX];
> +	char dev_name[NAME_MAX];

It seems NAME_MAX either does not exist on Alpine linux (perhaps due to
muslc) or needs an extra explicit include.

https://github.com/richiejp/ltp/actions/runs/3384078253/jobs/5620630058

> +	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 = '\0';

Very minor nit, but moving this around creates unecessary churn when
doing git-blame.

>  	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");
> +		dir = opendir("/");

What happens if TMPDIR is not on the same mount as '/'?

Also consider what the user will see if opendir fails with for
e.g. EPERM or ENOMEM. (there is SAFE_OPENDIR).

> +		fd = dirfd(dir);

As we only want the fd, we could call open (SAFE_OPEN)
instead. Incredibly dirfd can fail, so it saves some effort.

> +		if (!ioctl(fd, BTRFS_IOC_FS_INFO, &args)) {

What happens if the test author allows this function to be called on
tmpfs, rootfs, etc.? Or if the FS is valid, but has the same issue as
BTRFS.

Especially considering this may be discovered in an environment where
it's difficult to trace the test.

> +			sprintf(btrfs_uuid_str,
> +				"%02x%02x%02x%02x-%02x%02x-"
> +				"%02x%02x-%02x%02x-"
> +				"%02x%02x%02x%02x%02x%02x",
> +				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);

LTP uses the kernel C style. If you run 'make check-tst_device' in lib
it will run checkpatch.pl and display the formatting issues.

(you can ignore errors not related to your changes).

>  		}
> -
> -		len = count_match_len(path, mnt_point);
> -		if (len > best_match_len) {
> -			strcpy(dev, pre);
> -			best_match_len = len;
> +		closedir(dir);
> +		dir = opendir(bdev_path);
> +		if (!dir){
> +			tst_brkm(TWARN | TERRNO, NULL,
> +				"can't opendir %d", dir);

This isn't a valid format string and this should be TBROK. However it
can all be replaced with SAFE_OPENDIR.

> +		}
> +		while (d = readdir(dir)){

SAFE_READDIR

> +			if (d->d_name[0]!='.')
> +				break;

I suppose that given the present usage, it doesn't matter if there is
more than one backing dev. However we should at least print an info
message if more than one is detected.

> +		}
> +		uevent_path[0]='\0';
> +		closedir(dir);

This frees the memory pointed to by *d and results in a use-after-free
below. I recommend compiling with -fsanatize=undefined,address
(e.g. ./configure CFLAGS="-fsanatize=undefined,address") during
development.

Then instead of getting random segfaults you always get:

=298==ERROR: AddressSanitizer: heap-use-after-free on address 0x62d00000a473 at pc 0x7f1d7077357e bp 0x7ffea26903b0 sp 0x7ffea268fb60
READ of size 6 at 0x62d00000a473 thread T0
    #0 0x7f1d7077357d  (/lib64/libasan.so.8+0x7657d)
    #1 0x7f1d707744ba in __interceptor_vsprintf (/lib64/libasan.so.8+0x774ba)
    #2 0x7f1d7077460e in __interceptor_sprintf (/lib64/libasan.so.8+0x7760e)
    #3 0x413909 in tst_find_backing_dev /home/rich/qa/ltp/lib/tst_device.c:576
    #4 0x40916f in setup /home/rich/qa/ltp/testcases/kernel/syscalls/ioctl/ioctl_loop05.c:128
    #5 0x41ad47 in do_test_setup /home/rich/qa/ltp/lib/tst_test.c:1285
    #6 0x41ad47 in testrun /home/rich/qa/ltp/lib/tst_test.c:1412
    #7 0x41ad47 in fork_testrun /home/rich/qa/ltp/lib/tst_test.c:1558
    #8 0x421670 in tst_run_tcases /home/rich/qa/ltp/lib/tst_test.c:1651
    #9 0x407c6e in main ../../../../include/tst_test.h:379
    #10 0x7f1d6fef75af in __libc_start_call_main (/lib64/libc.so.6+0x275af)
    #11 0x7f1d6fef7678 in __libc_start_main_impl (/lib64/libc.so.6+0x27678)
    #12 0x4084a4 in _start ../sysdeps/x86_64/start.S:115

0x62d00000a473 is located 115 bytes inside of 32816-byte region [0x62d00000a400,0x62d000012430)
freed by thread T0 here:
    #0 0x7f1d707b80e8  (/lib64/libasan.so.8+0xbb0e8)
    #1 0x7f1d6ffa87f0 in __closedir (/lib64/libc.so.6+0xd87f0)

previously allocated by thread T0 here:
    #0 0x7f1d707b940f in __interceptor_malloc (/lib64/libasan.so.8+0xbc40f)
    #1 0x7f1d6ffa8674 in __alloc_dir (/lib64/libc.so.6+0xd8674)

> +		if (d!=NULL) {

Also not kernel style and there is SAFE_CLOSEDIR.

> +			sprintf(uevent_path, "%s/%s/uevent",
> +				bdev_path, d->d_name);
>  		}

If d == NULL then what does that mean?

> +	} 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)) {

What is the user going to see if this happens?

N.B if we have no options left then we want to fail as fast as possible
with any information that is easily available. This can save a lot of
time remotely debugging/tracing test systems when a simple log message
could have made the issue clear.

> +		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);
> +	}
>  
>  	if (stat(dev, &buf) < 0)
>  		tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);


-- 
Thank you,
Richard.

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

[LTP] [PATCH 0/2] Fix tst_find_backing_dev adding BTRFS support and /dev/root missing case

[Alessandro Carminati]

Hello Richard,

Thanks for the detailed review.
If in the future I want to contribute more to the LTP project, I need
to provide myself with a CI pipeline like yours.
I appreciated the review that was very detailed, but I couldn't address
a single comment.

>> +		if (!ioctl(fd, BTRFS_IOC_FS_INFO, &args)) {
>What happens if the test author allows this function to be called on
tmpfs, rootfs, etc.? Or if the FS is valid, but has the same issue as
BTRFS.

I have gone thorough all the file systems supported by LTP at this stage,
and I noticed that BTRFS is the only file system that owns this
singularity.
In addition to this, I also dared to assume that if device major number
is == 0 then the test is facing the BTRFS.
This assumption might not be true in general, but I tested it to be
true in the test supported file system.
Is your comment referring to this?


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   | 87 ++++++++++++++++++++++++++++++----------------
 2 files changed, 63 insertions(+), 31 deletions(-)

-- 
2.34.1


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

[LTP] [PATCH 1/2] tst_find_backing_dev: Get dev name from /sys/*/uevent

[Alessandro Carminati]

In some minimal Linux, the /dev/root can be missing. The consequence of
this is that mountinfo doesn't contain the correct information. btrfs
file systems are yet another point of trouble for this function.

The unevent file in sysfs is another method to retrieve device info
using the sysfs.

btrfs file systems are special from the device name retrieval, and in
place of use of the minor/major they are approached by using the uuid.
In the end, btrfs strategy is a slightly modified version of the same
unevent strategy.

Non btrfs look in "/sys/dev/block/%d:%d/uevent" major, minor
btrfs look in /sys/fs/btrfs/%s/devices/%s/uevent, uuid, devname

The btrfs handling requires BTRFS specific ioctl for finding the
file system uuid, and for this reason, btrfs/ioctl.h is needed.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
---
 lib/tst_device.c | 87 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 58 insertions(+), 29 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);
-- 
2.34.1


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

[LTP] [PATCH 2/2] c-test-api: Documentation updated

[Alessandro Carminati]

Since the tst_find_backing_dev logic is changed, the doc is updated
accordingly.

Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
---
 doc/c-test-api.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
index 64ee3397f..a7888c242 100644
--- a/doc/c-test-api.txt
+++ b/doc/c-test-api.txt
@@ -1071,8 +1071,11 @@ voud tst_find_backing_dev(const char *path, char *dev);
 -------------------------------------------------------------------------------
 
 This function finds the block dev that this path belongs to, it uses stat function
-to get the major/minor number of the path. Then scan them in '/proc/self/mountinfo'
-and list 2th column value after ' - ' string as its block dev if match succeeds.
+to get the major/minor number of the path. 
+This function finds the block dev that this path belongs to, it uses the unevent 
+file in sysfs to find the device name. It needs to discriminate between btrfs
+and not btrfs. For non btrfs filesystems it uses the minor, major numbers. For
+btrfs it uses the fs uuid.
 
 [source,c]
 -------------------------------------------------------------------------------
-- 
2.34.1


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

Re: [LTP] [PATCH 0/2] Fix tst_find_backing_dev adding BTRFS support and /dev/root missing case

[Richard Palethorpe]

Hello,

Alessandro Carminati <alessandro.carminati@gmail.com> writes:

> Hello Richard,
>
> Thanks for the detailed review.
> If in the future I want to contribute more to the LTP project, I need
> to provide myself with a CI pipeline like yours.

If you fork the project in Github and create a new branch then the CI
will run on the commits you push to GH.

> I appreciated the review that was very detailed, but I couldn't address
> a single comment.
>
>>> +		if (!ioctl(fd, BTRFS_IOC_FS_INFO, &args)) {
>>What happens if the test author allows this function to be called on
> tmpfs, rootfs, etc.? Or if the FS is valid, but has the same issue as
> BTRFS.
>
> I have gone thorough all the file systems supported by LTP at this stage,
> and I noticed that BTRFS is the only file system that owns this
> singularity.

tmpfs doesn't have a backing device and we support it? E.g.

40 60 0:38 / /tmp rw,nosuid,nodev shared:19 - tmpfs tmpfs rw,nr_inodes=1048576,inode64

So this function shouldn't be called on it and it is not in the test
currently effected[1]. However if the test author does it by accident (99%
chance of happening) then we want a sensible error message.

> In addition to this, I also dared to assume that if device major number
> is == 0 then the test is facing the BTRFS.
> This assumption might not be true in general, but I tested it to be
> true in the test supported file system.
> Is your comment referring to this?

I think it is absolutely correct that we shouldn't design for
requirements we presently don't have. However:

1. It's highly probable this function will be misused.
2. It's relatively easy to guard against.

>
>
> 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   | 87 ++++++++++++++++++++++++++++++----------------
>  2 files changed, 63 insertions(+), 31 deletions(-)

[1]: The test skips it

.skip_filesystems = (const char *const []) {
	"tmpfs",
	"overlayfs",
	NULL
},

Also note that you can run LTP on any filesystem. You just need to set
the appropriate env vars.


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH 1/2] tst_find_backing_dev: Get dev name from /sys/*/uevent

[Richard Palethorpe]

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.

-- 
Thank you,
Richard.

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

[LTP] [PATCH 0/2] Fix tst_find_backing_dev adding BTRFS support and /dev/root missing case

[Alessandro Carminati]

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(-)

-- 
2.34.1


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

[LTP] [PATCH 1/2] tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent

[Alessandro Carminati]

In some minimal Linux, the /dev/root can be missing. The consequence of
this is that mountinfo doesn't contain the correct information. btrfs
file systems are yet another point of trouble for this function.

The unevent file in sysfs is another method to retrieve device info
using the sysfs.

btrfs file systems are special from the device name retrieval, and in
place of use of the minor/major they are approached by using the uuid.
In the end, btrfs strategy is a slightly modified version of the same
unevent strategy.

Non btrfs look in "/sys/dev/block/%d:%d/uevent" major, minor
btrfs look in /sys/fs/btrfs/%s/devices/%s/uevent, uuid, devname

The btrfs handling requires BTRFS specific ioctl for finding the
file system uuid, and for this reason, btrfs/ioctl.h is needed.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
---
 lib/tst_device.c | 91 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 61 insertions(+), 30 deletions(-)

diff --git a/lib/tst_device.c b/lib/tst_device.c
index 8419b80c3..054e39bcd 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,74 @@ 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;
-		}
-
-		len = count_match_len(path, mnt_point);
-		if (len > best_match_len) {
-			strcpy(dev, pre);
-			best_match_len = len;
+	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);
+		} else {
+			tst_brkm(TBROK, NULL, "BTRFS ioctl failed. Is %s on a tmpfs?", 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, 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);
-- 
2.34.1


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

[LTP] [PATCH 2/2] c-test-api: Documentation updated

[Alessandro Carminati]

Since the tst_find_backing_dev logic is changed, the doc is updated
accordingly.

Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
---
 doc/c-test-api.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
index 64ee3397f..a7888c242 100644
--- a/doc/c-test-api.txt
+++ b/doc/c-test-api.txt
@@ -1071,8 +1071,11 @@ voud tst_find_backing_dev(const char *path, char *dev);
 -------------------------------------------------------------------------------
 
 This function finds the block dev that this path belongs to, it uses stat function
-to get the major/minor number of the path. Then scan them in '/proc/self/mountinfo'
-and list 2th column value after ' - ' string as its block dev if match succeeds.
+to get the major/minor number of the path. 
+This function finds the block dev that this path belongs to, it uses the unevent 
+file in sysfs to find the device name. It needs to discriminate between btrfs
+and not btrfs. For non btrfs filesystems it uses the minor, major numbers. For
+btrfs it uses the fs uuid.
 
 [source,c]
 -------------------------------------------------------------------------------
-- 
2.34.1


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

Re: [LTP] [PATCH 1/2] tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent

[Richard Palethorpe]

Hello,

I'd like to merge, but discovered some more issues that requuire more
than a fixup before merge.

Also please use the -v flag in git format-patch to version the patches
after the first revision. I don't mind which version you start at now.

Alessandro Carminati <alessandro.carminati@gmail.com> writes:

> In some minimal Linux, the /dev/root can be missing. The consequence of
> this is that mountinfo doesn't contain the correct information. btrfs
> file systems are yet another point of trouble for this function.
>
> The unevent file in sysfs is another method to retrieve device info
> using the sysfs.
>
> btrfs file systems are special from the device name retrieval, and in
> place of use of the minor/major they are approached by using the uuid.
> In the end, btrfs strategy is a slightly modified version of the same
> unevent strategy.
>
> Non btrfs look in "/sys/dev/block/%d:%d/uevent" major, minor
> btrfs look in /sys/fs/btrfs/%s/devices/%s/uevent, uuid, devname
>
> The btrfs handling requires BTRFS specific ioctl for finding the
> file system uuid, and for this reason, btrfs/ioctl.h is needed.
>
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
> ---
>  lib/tst_device.c | 91 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 61 insertions(+), 30 deletions(-)
>
> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index 8419b80c3..054e39bcd 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,74 @@ 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;
> -		}
> -
> -		len = count_match_len(path, mnt_point);
> -		if (len > best_match_len) {
> -			strcpy(dev, pre);
> -			best_match_len = len;
> +	if (dev_major == 0) {
> +		tst_resm(TINFO, "Use BTRFS specific strategy");
> +
> +		fd = SAFE_OPEN(NULL, dirname(path), O_DIRECTORY);

There are two problems here. One is simple and that dirname can modify
path, but path is a const pointer (compiler should warn about dropping
const modifiers). The simple solution is just to copy path into a buffer.

Secondly ioctl_loop05 passes the path to an image, but the self test in
/lib/newlib_tests/tst_device.c passes the mount point. So unless I am
mistaken dirname will return the dir below the mount point which is
wrong.

One option is to try opening path as a dir first and if that fails, use
dirname to get the containing folder. Changeing ioctl_loop05 would also
be valid.

> +		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 {
> +			tst_brkm(TBROK, NULL, "BTRFS ioctl failed. Is %s
> on a tmpfs?", path);
Need TERRNO here and/or check that the errorno is ENOTTY otherwise the
hint makes no sense.
> +			}
> +		SAFE_CLOSE(NULL, fd);
> +		dir = SAFE_OPENDIR(NULL, bdev_path);
> +		while (d = SAFE_READDIR(NULL, dir)) {
> +			if (d->d_name[0]!='.')

There are a few formatting errors like the missing spaces around !=.

Run make check-tst_device in the lib dir and see the kernel style
guidelines.

> +				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");

Still need to print some information about where we are looking (bdev_path).

> +			}
> +		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);

Also we can use (TBROK | TERRNO) here as access sets that.

> +		}

make check somehow missing this. The } is indented too far.

>  
>  	if (stat(dev, &buf) < 0)
>  		tst_brkm(TWARN | TERRNO, NULL, "stat(%s) failed", dev);



-- 
Thank you,
Richard.

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

[LTP] [PATCH v6 0/2] tst_find_backing_dev: fix stat fails /dev/root

[Alessandro Carminati]

This version fixes:
* dirname can modify path: as suggested in the review a new buffer is
reserved in the stack and the path string is copied there.
* Secondly ioctl_loop05 passes the path to an image, but the self
test in /lib/newlib_tests/tst_device.c passes the mount point. I
didn't notice this in the test, however I fixed this case. In place
of trying open the file as suggested I preferred to use
S_ISREG(buf.st_mode) using the stat data already present.
As side note, since the /lib/newlib_tests/tst_device.c depends on
mkfs.ex2 external executable, its results depends on the mkfs.ext2
implementation. Using the busybox mkfs.ext2 implementation the test
fails out of the box since a problem with the loop block size.
* TERRNO flag missing in the error messages fixed.
* formatting errors fixed .
* else statement bracket indentation have been adjusted.




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   | 95 ++++++++++++++++++++++++++++++++--------------
 2 files changed, 71 insertions(+), 31 deletions(-)

-- 
2.34.1


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

[LTP] [PATCH v6 1/2] tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent

[Alessandro Carminati]

In some minimal Linux, the /dev/root can be missing. The consequence of
this is that mountinfo doesn't contain the correct information. btrfs
file systems are yet another point of trouble for this function.

The unevent file in sysfs is another method to retrieve device info
using the sysfs.

btrfs file systems are special from the device name retrieval, and in
place of use of the minor/major they are approached by using the uuid.
In the end, btrfs strategy is a slightly modified version of the same
unevent strategy.

Non btrfs look in "/sys/dev/block/%d:%d/uevent" major, minor
btrfs look in /sys/fs/btrfs/%s/devices/%s/uevent, uuid, devname

The btrfs handling requires BTRFS specific ioctl for finding the
file system uuid, and for this reason, btrfs/ioctl.h is needed.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
---
 lib/tst_device.c | 95 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 66 insertions(+), 29 deletions(-)

diff --git a/lib/tst_device.c b/lib/tst_device.c
index 8419b80c3..85acdc4f2 100644
--- a/lib/tst_device.c
+++ b/lib/tst_device.c
@@ -33,6 +33,8 @@
 #include <stdint.h>
 #include <inttypes.h>
 #include <sys/sysmacros.h>
+#include <linux/btrfs.h>
+#include <linux/limits.h>
 #include "lapi/syscalls.h"
 #include "test.h"
 #include "safe_macros.h"
@@ -45,6 +47,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 +523,81 @@ 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+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");
+	strncpy(tmp_path, path, PATH_MAX-1);
+	tmp_path[PATH_MAX-1] = '\0';
+	if (S_ISREG(buf.st_mode))
+		dirname(tmp_path);
 
 	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, 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 failed with %d.", errno);
 		}
-
-		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 | TERRNO, NULL, "No backining 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);
+	} 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);
-- 
2.34.1


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

[LTP] [PATCH v6 2/2] c-test-api: Documentation updated

[Alessandro Carminati]

Since the tst_find_backing_dev logic is changed, the doc is updated
accordingly.

Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
---
 doc/c-test-api.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
index 64ee3397f..a7888c242 100644
--- a/doc/c-test-api.txt
+++ b/doc/c-test-api.txt
@@ -1071,8 +1071,11 @@ voud tst_find_backing_dev(const char *path, char *dev);
 -------------------------------------------------------------------------------
 
 This function finds the block dev that this path belongs to, it uses stat function
-to get the major/minor number of the path. Then scan them in '/proc/self/mountinfo'
-and list 2th column value after ' - ' string as its block dev if match succeeds.
+to get the major/minor number of the path. 
+This function finds the block dev that this path belongs to, it uses the unevent 
+file in sysfs to find the device name. It needs to discriminate between btrfs
+and not btrfs. For non btrfs filesystems it uses the minor, major numbers. For
+btrfs it uses the fs uuid.
 
 [source,c]
 -------------------------------------------------------------------------------
-- 
2.34.1


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

Re: [LTP] [PATCH v6 1/2] tst_find_backing_dev: Get dev name from /sys/dev/block/*/uevent

[Richard Palethorpe]

Hello,

Pushed with minor changes, thanks!

I added some whitespace and replaced my signoff tag with reviewed by.

Alessandro Carminati <alessandro.carminati@gmail.com> writes:

> +			if (errno == ENOTTY)
> +				tst_brkm(TBROK | TERRNO, NULL, "BTRFS ioctl failed. Is %s on a tmpfs?", path);
> +			tst_brkm(TBROK | TERRNO, NULL, "BTRFS ioctl
> failed with %d.", errno);

TERRNO prints the errno, instead I added the tmp_path.

>  
> -	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);

GCC 12 complains about a null ptr deref here. Seems unlikely to happen,
but I added the nonnull attribute to silence the warning.

> +	} 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);


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH v6 2/2] c-test-api: Documentation updated

[Richard Palethorpe]

Hello,

Merged, thanks!

I removed some trailing whitespace.

Alessandro Carminati <alessandro.carminati@gmail.com> writes:

> Since the tst_find_backing_dev logic is changed, the doc is updated
> accordingly.
>
> Signed-off-by: Alessandro Carminati <alessandro.carminati@gmail.com>
> ---
>  doc/c-test-api.txt | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/doc/c-test-api.txt b/doc/c-test-api.txt
> index 64ee3397f..a7888c242 100644
> --- a/doc/c-test-api.txt
> +++ b/doc/c-test-api.txt
> @@ -1071,8 +1071,11 @@ voud tst_find_backing_dev(const char *path, char *dev);
>  -------------------------------------------------------------------------------
>  
>  This function finds the block dev that this path belongs to, it uses stat function
> -to get the major/minor number of the path. Then scan them in '/proc/self/mountinfo'
> -and list 2th column value after ' - ' string as its block dev if match succeeds.
> +to get the major/minor number of the path. 
> +This function finds the block dev that this path belongs to, it uses the unevent

There are trailing whitespace on these lines

> +file in sysfs to find the device name. It needs to discriminate between btrfs
> +and not btrfs. For non btrfs filesystems it uses the minor, major numbers. For
> +btrfs it uses the fs uuid.
>  
>  [source,c]
>  -------------------------------------------------------------------------------


-- 
Thank you,
Richard.

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