diff mbox series

syscalls: Check for leftover partition info in loopdev ioctl tests

Message ID 20220406110837.14773-1-mdoucha@suse.cz
State New
Headers show
Series syscalls: Check for leftover partition info in loopdev ioctl tests | expand

Commit Message

Martin Doucha April 6, 2022, 11:08 a.m. UTC
Due to a kernel bug, successful ioctl09 and ioctl_loop01 test runs
sometimes leave behind stale partition info on the loop device they used,
which then causes mkfs.vfat to fail in later tests. Check that partition
info was properly removed in cleanup.

Signed-off-by: Martin Doucha <mdoucha@suse.cz>
---

This does not fix the mkfs.vfat failures but it makes the true cause visible.
We could add a workaround for the mkfs.vfat failures by simply initializing
the loop device with the LO_FLAGS_PARTSCAN flag by default, or at least when
stale partition info is found by tst_find_free_loopdev().

 testcases/kernel/syscalls/ioctl/ioctl09.c      | 12 +++++++++++-
 testcases/kernel/syscalls/ioctl/ioctl_loop01.c |  6 ++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

Comments

Petr Vorel April 19, 2022, 7:09 a.m. UTC | #1
Hi Martin,

> Due to a kernel bug, successful ioctl09 and ioctl_loop01 test runs
> sometimes leave behind stale partition info on the loop device they used,
> which then causes mkfs.vfat to fail in later tests. Check that partition
> info was properly removed in cleanup.

Thanks!

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

Kind regards,
Petr
Cyril Hrubis April 25, 2022, 3:21 p.m. UTC | #2
Hi!
> Due to a kernel bug, successful ioctl09 and ioctl_loop01 test runs
> sometimes leave behind stale partition info on the loop device they used,
> which then causes mkfs.vfat to fail in later tests. Check that partition
> info was properly removed in cleanup.
> 
> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
> ---
> 
> This does not fix the mkfs.vfat failures but it makes the true cause visible.
> We could add a workaround for the mkfs.vfat failures by simply initializing
> the loop device with the LO_FLAGS_PARTSCAN flag by default, or at least when
> stale partition info is found by tst_find_free_loopdev().

I guess that it would be cleaner to put the stale partition info
detection into the loop library. We can print a warning there and then
do the workaround.

Also do we want to add a regression test for the stale partition info?
Should be easy enough. Or at least add the hash of the kernel commit
that fixed it to the ioctl tests?

>  testcases/kernel/syscalls/ioctl/ioctl09.c      | 12 +++++++++++-
>  testcases/kernel/syscalls/ioctl/ioctl_loop01.c |  6 ++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl09.c b/testcases/kernel/syscalls/ioctl/ioctl09.c
> index 9728ecb9c..09867a5c5 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl09.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl09.c
> @@ -19,7 +19,7 @@
>         ({ value ? TST_RETVAL_EQ0(x) : TST_RETVAL_NOTNULL(x); })
>  
>  static char dev_path[1024];
> -static int dev_num, attach_flag, dev_fd;
> +static int dev_num = -1, attach_flag, dev_fd;
>  static char loop_partpath[1026], sys_loop_partpath[1026];
>  
>  static void change_partition(const char *const cmd[])
> @@ -102,6 +102,16 @@ static void cleanup(void)
>  		SAFE_CLOSE(dev_fd);
>  	if (attach_flag)
>  		tst_detach_device(dev_path);
> +
> +	if (dev_num < 0)
> +		return;
> +
> +	sprintf(sys_loop_partpath, "/sys/block/loop%d/loop%dp1", dev_num,
> +		dev_num);
> +	sprintf(loop_partpath, "%sp1", dev_path);
> +
> +	if (!access(sys_loop_partpath, F_OK) || !access(loop_partpath, F_OK))
> +		tst_res(TWARN, "Partition info was not cleared from loop dev");
>  }
>  
>  static struct tst_test test = {
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop01.c b/testcases/kernel/syscalls/ioctl/ioctl_loop01.c
> index 734d803d5..17168ae04 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl_loop01.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl_loop01.c
> @@ -135,6 +135,12 @@ static void cleanup(void)
>  		SAFE_CLOSE(dev_fd);
>  	if (attach_flag)
>  		tst_detach_device(dev_path);
> +
> +	if (!*sys_loop_partpath || !*loop_partpath)
> +		return;
> +
> +	if (!access(sys_loop_partpath, F_OK) || !access(loop_partpath, F_OK))
> +		tst_res(TWARN, "Partition info was not cleared from loop dev");
>  }
>  
>  static struct tst_test test = {
> -- 
> 2.35.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp
Martin Doucha April 26, 2022, 4:14 p.m. UTC | #3
On 25. 04. 22 17:21, Cyril Hrubis wrote:
> Hi!
>> Due to a kernel bug, successful ioctl09 and ioctl_loop01 test runs
>> sometimes leave behind stale partition info on the loop device they used,
>> which then causes mkfs.vfat to fail in later tests. Check that partition
>> info was properly removed in cleanup.
>>
>> Signed-off-by: Martin Doucha <mdoucha@suse.cz>
>> ---
>>
>> This does not fix the mkfs.vfat failures but it makes the true cause visible.
>> We could add a workaround for the mkfs.vfat failures by simply initializing
>> the loop device with the LO_FLAGS_PARTSCAN flag by default, or at least when
>> stale partition info is found by tst_find_free_loopdev().
> 
> I guess that it would be cleaner to put the stale partition info
> detection into the loop library. We can print a warning there and then
> do the workaround.

The workaround needs to be added into tst_attach_device(). It doesn't
make sense to add it to test cleanup, in part because
tst_detach_device() can and occasionally does fail on timeout.

On the other hand, we need a cleanup check in ioctl tests which create
partitions on loop devices, otherwise they'll leave garbage behind silently.

> Also do we want to add a regression test for the stale partition info?
> Should be easy enough. Or at least add the hash of the kernel commit
> that fixed it to the ioctl tests?

I haven't investigated deep enough to find out how to reliably trigger
the bug or which patch fixed it (if any). Regression test would be nice
but it's not a trivial task at the moment.
diff mbox series

Patch

diff --git a/testcases/kernel/syscalls/ioctl/ioctl09.c b/testcases/kernel/syscalls/ioctl/ioctl09.c
index 9728ecb9c..09867a5c5 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl09.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl09.c
@@ -19,7 +19,7 @@ 
        ({ value ? TST_RETVAL_EQ0(x) : TST_RETVAL_NOTNULL(x); })
 
 static char dev_path[1024];
-static int dev_num, attach_flag, dev_fd;
+static int dev_num = -1, attach_flag, dev_fd;
 static char loop_partpath[1026], sys_loop_partpath[1026];
 
 static void change_partition(const char *const cmd[])
@@ -102,6 +102,16 @@  static void cleanup(void)
 		SAFE_CLOSE(dev_fd);
 	if (attach_flag)
 		tst_detach_device(dev_path);
+
+	if (dev_num < 0)
+		return;
+
+	sprintf(sys_loop_partpath, "/sys/block/loop%d/loop%dp1", dev_num,
+		dev_num);
+	sprintf(loop_partpath, "%sp1", dev_path);
+
+	if (!access(sys_loop_partpath, F_OK) || !access(loop_partpath, F_OK))
+		tst_res(TWARN, "Partition info was not cleared from loop dev");
 }
 
 static struct tst_test test = {
diff --git a/testcases/kernel/syscalls/ioctl/ioctl_loop01.c b/testcases/kernel/syscalls/ioctl/ioctl_loop01.c
index 734d803d5..17168ae04 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl_loop01.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl_loop01.c
@@ -135,6 +135,12 @@  static void cleanup(void)
 		SAFE_CLOSE(dev_fd);
 	if (attach_flag)
 		tst_detach_device(dev_path);
+
+	if (!*sys_loop_partpath || !*loop_partpath)
+		return;
+
+	if (!access(sys_loop_partpath, F_OK) || !access(loop_partpath, F_OK))
+		tst_res(TWARN, "Partition info was not cleared from loop dev");
 }
 
 static struct tst_test test = {