| Message ID | 20220406110837.14773-1-mdoucha@suse.cz |
|---|---|
| State | Deferred |
| Headers | show |
| Series | syscalls: Check for leftover partition info in loopdev ioctl tests | expand |
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
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
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.
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.
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 = {
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(-)