[LTP] [PATCH] syscalls: Check for leftover partition info in loopdev ioctl tests

[LTP] [PATCH] syscalls: Check for leftover partition info in loopdev ioctl tests

[Martin Doucha]

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

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

Re: [LTP] [PATCH] syscalls: Check for leftover partition info in loopdev ioctl tests

[Petr Vorel]

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

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

Re: [LTP] [PATCH] syscalls: Check for leftover partition info in loopdev ioctl tests

[Cyril Hrubis]

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

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] syscalls: Check for leftover partition info in loopdev ioctl tests

[Martin Doucha]

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.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic

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

Re: [LTP] [PATCH] syscalls: Check for leftover partition info in loopdev ioctl tests

[Richard Palethorpe]

Hello,

Martin Doucha <mdoucha@suse.cz> writes:

> 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.

I'm trying to cleanup patchwork and I'm not sure what the resolution to
this was?

If this has not been resolved elsewhere and nobody wants to work on this
further then I would be in favor of merging the patch. The information
is then available for others to investigate.

-- 
Thank you,
Richard.

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