[LTP] [PATCH 1/2] ioctl09: wait for udevd complete the uevent handling

[LTP] [PATCH 1/2] ioctl09: wait for udevd complete the uevent handling

[Li Wang]

This is quite necessary to wait for uevent handing settled before
checking the partition is taken effect.

  `udevadm monitor` listening from another terminal:

  KERNEL[447.077683] change   /devices/virtual/block/loop0 (block)
  KERNEL[447.077939] add      /devices/virtual/block/loop0/loop0p1 (block)
  KERNEL[447.077999] add      /devices/virtual/block/loop0/loop0p2 (block)
  UDEV  [447.081138] change   /devices/virtual/block/loop0 (block)
  UDEV  [447.083490] add      /devices/virtual/block/loop0/loop0p1 (block)
  UDEV  [447.086898] add      /devices/virtual/block/loop0/loop0p2 (block)

Which helps to get rid of such boring failures:

  ioctl09.c:44: TPASS: access /sys/block/loop0/loop0p1 succeeds
  ioctl09.c:52: TPASS: access /dev/loop0p1 succeeds
  ioctl09.c:47: TFAIL: access /sys/block/loop0/loop0p2 fails
  ioctl09.c:55: TFAIL: access /dev/loop0p2 fails

I manually confirmed those failures disappeared from a reproducible system
even with running 10000 times.

Signed-off-by: Li Wang <liwang@redhat.com>
---
 testcases/kernel/syscalls/ioctl/ioctl09.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/testcases/kernel/syscalls/ioctl/ioctl09.c b/testcases/kernel/syscalls/ioctl/ioctl09.c
index 9728ecb9c..533a4a5d0 100644
--- a/testcases/kernel/syscalls/ioctl/ioctl09.c
+++ b/testcases/kernel/syscalls/ioctl/ioctl09.c
@@ -58,6 +58,10 @@ static void check_partition(int part_num, bool value)
 
 static void verify_ioctl(void)
 {
+	char cmd[BUFSIZ];
+
+	sprintf(cmd, "udevadm settle --exit-if-exists=%s", dev_path);
+
 	const char *const cmd_parted_old[] = {"parted", "-s", "test.img",
 					      "mklabel", "msdos", "mkpart",
 					      "primary", "ext4", "1M", "10M",
@@ -75,11 +79,13 @@ static void verify_ioctl(void)
 
 	loopinfo.lo_flags =  LO_FLAGS_PARTSCAN;
 	SAFE_IOCTL(dev_fd, LOOP_SET_STATUS, &loopinfo);
+	tst_system(cmd);
 	check_partition(1, true);
 	check_partition(2, false);
 
 	change_partition(cmd_parted_new);
 	TST_RETRY_FUNC(ioctl(dev_fd, BLKRRPART, 0), TST_RETVAL_EQ0);
+	tst_system(cmd);
 	check_partition(1, true);
 	check_partition(2, true);
 
-- 
2.31.1


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

[LTP] [PATCH 2/2] mkswap01: wait for the triggered events to complete

[Li Wang]

Signed-off-by: Li Wang <liwang@redhat.com>
---
 testcases/commands/mkswap/mkswap01.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testcases/commands/mkswap/mkswap01.sh b/testcases/commands/mkswap/mkswap01.sh
index f6494f6e3..cb3563b49 100755
--- a/testcases/commands/mkswap/mkswap01.sh
+++ b/testcases/commands/mkswap/mkswap01.sh
@@ -128,7 +128,7 @@ mkswap_test()
 		return
 	fi
 
-	udevadm trigger --name-match=$TST_DEVICE
+	udevadm trigger --name-match=$TST_DEVICE --settle
 
 	if [ -n "$device" ]; then
 		mkswap_verify "$mkswap_op" "$op_arg" "$device" "$size" "$dev_file"
-- 
2.31.1


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

Re: [LTP] [PATCH 2/2] mkswap01: wait for the triggered events to complete

[Cyril Hrubis]

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

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 2/2] mkswap01: wait for the triggered events to complete

[Petr Vorel]

Hi all,

> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>  testcases/commands/mkswap/mkswap01.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

> diff --git a/testcases/commands/mkswap/mkswap01.sh b/testcases/commands/mkswap/mkswap01.sh
> index f6494f6e3..cb3563b49 100755
> --- a/testcases/commands/mkswap/mkswap01.sh
> +++ b/testcases/commands/mkswap/mkswap01.sh
> @@ -128,7 +128,7 @@ mkswap_test()
>  		return
>  	fi

> -	udevadm trigger --name-match=$TST_DEVICE
> +	udevadm trigger --name-match=$TST_DEVICE --settle

>  	if [ -n "$device" ]; then
>  		mkswap_verify "$mkswap_op" "$op_arg" "$device" "$size" "$dev_file"

--settle option for udevadm trigger has been added in v238 (in 2018) [1].
This mean on SLES 12-SP5, RHEL-7.9, 18.04 LTS bionic, ... we get:

trigger: unrecognized option '--settle'

Do we ignore backward compatibility hoping that all distros aren't tested with
newer LTP?

Kind regards,
Petr

[1] https://github.com/systemd/systemd/commit/792cc203a67edb201073351f5c766fce3d5eab45

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

Re: [LTP] [PATCH 2/2] mkswap01: wait for the triggered events to complete

[Li Wang]

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

On Thu, Apr 21, 2022 at 4:56 AM Petr Vorel <pvorel@suse.cz> wrote:


>
> > -     udevadm trigger --name-match=$TST_DEVICE
> > +     udevadm trigger --name-match=$TST_DEVICE --settle
>
> >       if [ -n "$device" ]; then
> >               mkswap_verify "$mkswap_op" "$op_arg" "$device" "$size"
> "$dev_file"
>
> --settle option for udevadm trigger has been added in v238 (in 2018) [1].
> This mean on SLES 12-SP5, RHEL-7.9, 18.04 LTS bionic, ... we get:
>
> trigger: unrecognized option '--settle'
>

Thanks for pointing out this, Petr.

>
> Do we ignore backward compatibility hoping that all distros aren't tested
> with
> newer LTP?
>

Actually, we only use fixed older-version of LTP for long-term supported
distros (e.g RHEL6/7) testing, as it falls into maintaining phase and no new
features are added in. So this won't be a problem for us.

But if we consider fully backward compatibility of newer LTP for old
distros,
this is a burden to maintaining work. We might need to make a balance on
the patch accept or reject.

With regard to this simple patch, if you think it's a problem to SLES
12-SP5, I'm
fine to NAK and rewrite with another way (at least for ioctl09 I will do
that).

But if you're OK with making use of fixed LTP on older distros, feel free
to apply this one :).

-- 
Regards,
Li Wang

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

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


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

Re: [LTP] [PATCH 2/2] mkswap01: wait for the triggered events to complete

[Li Wang]

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

On Thu, Apr 21, 2022 at 10:18 AM Li Wang <liwang@redhat.com> wrote:

>
>
> On Thu, Apr 21, 2022 at 4:56 AM Petr Vorel <pvorel@suse.cz> wrote:
>
>
>>
>> > -     udevadm trigger --name-match=$TST_DEVICE
>> > +     udevadm trigger --name-match=$TST_DEVICE --settle
>>
>> >       if [ -n "$device" ]; then
>> >               mkswap_verify "$mkswap_op" "$op_arg" "$device" "$size"
>> "$dev_file"
>>
>> --settle option for udevadm trigger has been added in v238 (in 2018) [1].
>> This mean on SLES 12-SP5, RHEL-7.9, 18.04 LTS bionic, ... we get:
>>
>> trigger: unrecognized option '--settle'
>>
>
> Thanks for pointing out this, Petr.
>
>>
>> Do we ignore backward compatibility hoping that all distros aren't tested
>> with
>> newer LTP?
>>
>
> Actually, we only use fixed older-version of LTP for long-term supported
> distros (e.g RHEL6/7) testing, as it falls into maintaining phase and no
> new
> features are added in. So this won't be a problem for us.
>
> But if we consider fully backward compatibility of newer LTP for old
> distros,
> this is a burden to maintaining work. We might need to make a balance on
> the patch accept or reject.
>
> With regard to this simple patch, if you think it's a problem to SLES
> 12-SP5, I'm
> fine to NAK and rewrite with another way (at least for ioctl09 I will do
> that).
>
> But if you're OK with making use of fixed LTP on older distros, feel free
> to apply this one :).
>


Or, just go with the traditional way for compatibility:

--- a/testcases/commands/mkswap/mkswap01.sh
+++ b/testcases/commands/mkswap/mkswap01.sh
@@ -128,7 +128,8 @@ mkswap_test()
                return
        fi

-       udevadm trigger --name-match=$TST_DEVICE --settle
+       udevadm trigger --name-match=$TST_DEVICE
+       udevadm settle --exit-if-exists==$TST_DEVICE

        if [ -n "$device" ]; then
                mkswap_verify "$mkswap_op" "$op_arg" "$device" "$size"
"$dev_file"


-- 
Regards,
Li Wang

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

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


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

Re: [LTP] [PATCH 2/2] mkswap01: wait for the triggered events to complete

[Petr Vorel]

> On Thu, Apr 21, 2022 at 10:18 AM Li Wang <liwang@redhat.com> wrote:



> > On Thu, Apr 21, 2022 at 4:56 AM Petr Vorel <pvorel@suse.cz> wrote:



> >> > -     udevadm trigger --name-match=$TST_DEVICE
> >> > +     udevadm trigger --name-match=$TST_DEVICE --settle

> >> >       if [ -n "$device" ]; then
> >> >               mkswap_verify "$mkswap_op" "$op_arg" "$device" "$size"
> >> "$dev_file"

> >> --settle option for udevadm trigger has been added in v238 (in 2018) [1].
> >> This mean on SLES 12-SP5, RHEL-7.9, 18.04 LTS bionic, ... we get:

> >> trigger: unrecognized option '--settle'


> > Thanks for pointing out this, Petr.


> >> Do we ignore backward compatibility hoping that all distros aren't tested
> >> with
> >> newer LTP?


> > Actually, we only use fixed older-version of LTP for long-term supported
> > distros (e.g RHEL6/7) testing, as it falls into maintaining phase and no
> > new
> > features are added in. So this won't be a problem for us.

> > But if we consider fully backward compatibility of newer LTP for old
> > distros,
> > this is a burden to maintaining work. We might need to make a balance on
> > the patch accept or reject.

> > With regard to this simple patch, if you think it's a problem to SLES
> > 12-SP5, I'm
> > fine to NAK and rewrite with another way (at least for ioctl09 I will do
> > that).

> > But if you're OK with making use of fixed LTP on older distros, feel free
> > to apply this one :).



> Or, just go with the traditional way for compatibility:

> --- a/testcases/commands/mkswap/mkswap01.sh
> +++ b/testcases/commands/mkswap/mkswap01.sh
> @@ -128,7 +128,8 @@ mkswap_test()
>                 return
>         fi

> -       udevadm trigger --name-match=$TST_DEVICE --settle
> +       udevadm trigger --name-match=$TST_DEVICE
> +       udevadm settle --exit-if-exists==$TST_DEVICE

IMHO that'd be better (--exit-if-exists was added in 2009 in udev 174).

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

Kind regards,
Petr

>         if [ -n "$device" ]; then
>                 mkswap_verify "$mkswap_op" "$op_arg" "$device" "$size"
> "$dev_file"

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

Re: [LTP] [PATCH 1/2] ioctl09: wait for udevd complete the uevent handling

[Cyril Hrubis]

Hi!
> This is quite necessary to wait for uevent handing settled before
> checking the partition is taken effect.
> 
>   `udevadm monitor` listening from another terminal:
> 
>   KERNEL[447.077683] change   /devices/virtual/block/loop0 (block)
>   KERNEL[447.077939] add      /devices/virtual/block/loop0/loop0p1 (block)
>   KERNEL[447.077999] add      /devices/virtual/block/loop0/loop0p2 (block)
>   UDEV  [447.081138] change   /devices/virtual/block/loop0 (block)
>   UDEV  [447.083490] add      /devices/virtual/block/loop0/loop0p1 (block)
>   UDEV  [447.086898] add      /devices/virtual/block/loop0/loop0p2 (block)
> 
> Which helps to get rid of such boring failures:
> 
>   ioctl09.c:44: TPASS: access /sys/block/loop0/loop0p1 succeeds
>   ioctl09.c:52: TPASS: access /dev/loop0p1 succeeds
>   ioctl09.c:47: TFAIL: access /sys/block/loop0/loop0p2 fails
>   ioctl09.c:55: TFAIL: access /dev/loop0p2 fails
> 
> I manually confirmed those failures disappeared from a reproducible system
> even with running 10000 times.
> 
> Signed-off-by: Li Wang <liwang@redhat.com>
> ---
>  testcases/kernel/syscalls/ioctl/ioctl09.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/testcases/kernel/syscalls/ioctl/ioctl09.c b/testcases/kernel/syscalls/ioctl/ioctl09.c
> index 9728ecb9c..533a4a5d0 100644
> --- a/testcases/kernel/syscalls/ioctl/ioctl09.c
> +++ b/testcases/kernel/syscalls/ioctl/ioctl09.c
> @@ -58,6 +58,10 @@ static void check_partition(int part_num, bool value)
>  
>  static void verify_ioctl(void)
>  {
> +	char cmd[BUFSIZ];
> +
> +	sprintf(cmd, "udevadm settle --exit-if-exists=%s", dev_path);
> +
>  	const char *const cmd_parted_old[] = {"parted", "-s", "test.img",
>  					      "mklabel", "msdos", "mkpart",
>  					      "primary", "ext4", "1M", "10M",
> @@ -75,11 +79,13 @@ static void verify_ioctl(void)
>  
>  	loopinfo.lo_flags =  LO_FLAGS_PARTSCAN;
>  	SAFE_IOCTL(dev_fd, LOOP_SET_STATUS, &loopinfo);
> +	tst_system(cmd);
>  	check_partition(1, true);
>  	check_partition(2, false);
>  
>  	change_partition(cmd_parted_new);
>  	TST_RETRY_FUNC(ioctl(dev_fd, BLKRRPART, 0), TST_RETVAL_EQ0);
> +	tst_system(cmd);
>  	check_partition(1, true);
>  	check_partition(2, true);

We allready use TST_RETRY_FN_EXP_BACKOFF() in there with 30 second
timeout isn't that enough? And if it isn't wouldn't simply increasing
the timeout to a minute or two fix the issue?

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 1/2] ioctl09: wait for udevd complete the uevent handling

[Li Wang]

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

Cyril Hrubis <chrubis@suse.cz> wrote:



>
> >  static void verify_ioctl(void)
> >  {
> > +     char cmd[BUFSIZ];
> > +
> > +     sprintf(cmd, "udevadm settle --exit-if-exists=%s", dev_path);
> > +
> >       const char *const cmd_parted_old[] = {"parted", "-s", "test.img",
> >                                             "mklabel", "msdos", "mkpart",
> >                                             "primary", "ext4", "1M",
> "10M",
> > @@ -75,11 +79,13 @@ static void verify_ioctl(void)
> >
> >       loopinfo.lo_flags =  LO_FLAGS_PARTSCAN;
> >       SAFE_IOCTL(dev_fd, LOOP_SET_STATUS, &loopinfo);
> > +     tst_system(cmd);
> >       check_partition(1, true);
> >       check_partition(2, false);
> >
> >       change_partition(cmd_parted_new);
> >       TST_RETRY_FUNC(ioctl(dev_fd, BLKRRPART, 0), TST_RETVAL_EQ0);
> > +     tst_system(cmd);
> >       check_partition(1, true);
> >       check_partition(2, true);
>
> We allready use TST_RETRY_FN_EXP_BACKOFF() in there with 30 second
>

I have to say, for most systems 30 sec is long enough, but we definitely got
sporadically fails in larger-scale automation testing.


timeout isn't that enough? And if it isn't wouldn't simply increasing
> the timeout to a minute or two fix the issue?
>


That should be better, I just have a try on my reproducible system,
but it does not work with enlarged to 180 seconds.

ioctl09.c:52: TPASS: access /dev/loop0p1 succeeds
octl09.c:47: TFAIL: access /sys/block/loop0/loop0p2 fails
Test timeouted, sending SIGKILL!
tst_test.c:1509: TINFO: If you are running on slow machine, try exporting
LTP_TIMEOUT_MUL > 1
tst_test.c:1511: TBROK: Test killed! (timeout?)


Note, the `udevadm settle` uses 180s as default timeout as well,
but it can work, I will look into udevadm.c to see if that does
something additional besides the waiting.

If so, we might need to remove the TST_RETRY_FN_EXP_BACKOFF
from this test.

-- 
Regards,
Li Wang

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

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


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

Re: [LTP] [PATCH 1/2] ioctl09: wait for udevd complete the uevent handling

[Cyril Hrubis]

Hi!
> timeout isn't that enough? And if it isn't wouldn't simply increasing
> > the timeout to a minute or two fix the issue?
> >
> 
> 
> That should be better, I just have a try on my reproducible system,
> but it does not work with enlarged to 180 seconds.

You have to set the test timeout to be four times of this timeout
otherwise it will report that the test had timeouted sporadically.

> ioctl09.c:52: TPASS: access /dev/loop0p1 succeeds
> octl09.c:47: TFAIL: access /sys/block/loop0/loop0p2 fails
> Test timeouted, sending SIGKILL!
> tst_test.c:1509: TINFO: If you are running on slow machine, try exporting
> LTP_TIMEOUT_MUL > 1
> tst_test.c:1511: TBROK: Test killed! (timeout?)
> 
> 
> Note, the `udevadm settle` uses 180s as default timeout as well,
> but it can work, I will look into udevadm.c to see if that does
> something additional besides the waiting.

I guess that the difference is that when we wait on a socket actively we
return sooner than with the exp backoff. By definition the exp backoff
may wait for twice as long as the udevadm and because of that the test
may actually timeout because we waited just a little bit longer.

> If so, we might need to remove the TST_RETRY_FN_EXP_BACKOFF
> from this test.

That would be valid option, remove the exp backoff and wait activelly.
Btw we already have an infrastructure for matching kernel event uevents
in the kernel/uevent/ directory. If we split the uevent.h there into a
header and libs/ltpuevent/ we could simply wait for the event by filling
in the uevent_desc structure and calling wait_for_uevents().

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 1/2] ioctl09: wait for udevd complete the uevent handling

[Li Wang]

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

On Wed, Apr 20, 2022 at 8:50 PM Cyril Hrubis <chrubis@suse.cz> wrote:


>
> > Note, the `udevadm settle` uses 180s as default timeout as well,
> > but it can work, I will look into udevadm.c to see if that does
> > something additional besides the waiting.
>
> I guess that the difference is that when we wait on a socket actively we
> return sooner than with the exp backoff. By definition the exp backoff
> may wait for twice as long as the udevadm and because of that the test
> may actually timeout because we waited just a little bit longer.
>

That's right, I overlooked that exp backoff time is longer than the set
value.



>
> > If so, we might need to remove the TST_RETRY_FN_EXP_BACKOFF
> > from this test.
>
> That would be valid option, remove the exp backoff and wait activelly.
>

Yes, wait actively and confirm event handling is completed.
(`udevadm settle` is the direct way I can think of now)



> Btw we already have an infrastructure for matching kernel event uevents
> in the kernel/uevent/ directory. If we split the uevent.h there into a
> header and libs/ltpuevent/ we could simply wait for the event by filling
> in the uevent_desc structure and calling wait_for_uevents().
>

Good to know this, I have looked through that but am afraid it can't
fully satisfy our requirements.

Because the wait_for_uevents is only falling into an infinite loop
and returns if detecting the expected UEVENT (which was sent
from the kernel function kobject_uevent_env).

The key point here is, we have to confirm `udevd` completed the UEVENT
handling, otherwise only detecting event exists is not enough, there is
still
potentially get the race condition again if the event queue there.


-- 
Regards,
Li Wang

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

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


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