Re: [LTP] [PATCH v2] lib: tst_device: Use exponential backoff for loop device detach

[Petr Vorel <pvorel@suse.cz>]

Hi Seba,

> detach_loop_fd() retries LOOP_CLR_FD 40 times in 50ms steps, a fixed
> 2s timeout that ignores LTP_TIMEOUT_MUL. When another process still
> holds the loop device at cleanup for longer than that, e.g. on a
> loaded machine, the test ends with an unwanted
> "ioctl(/dev/loopN, LOOP_CLR_FD, 0) no ENXIO for too long" TWARN.

> Use TST_RETRY_FN_EXP_BACKOFF() instead, as attach already does since
> d90d20046 ("lib: Use backoff polling to wait for loop device nodes").
> The detach finishes quickly when the device is released right away,
> polling slows down when it is not, and the total timeout (about 2s by
> default) is scaled by tst_multiply_timeout().

Out of curiosity, which of ioctl test is affected by this on a slow worker?
Baremetal issue?

BTW ioctl_loop01 is not working with -i (not caused by this patch):

# ./testcases/kernel/syscalls/ioctl/ioctl_loop01
...
tst_buffers.c:57: TINFO: Test is using guarded buffers
ioctl_loop01.c:82: TPASS: /sys/block/loop0/loop/partscan = 0
ioctl_loop01.c:83: TPASS: /sys/block/loop0/loop/autoclear = 0
ioctl_loop01.c:84: TPASS: /sys/block/loop0/loop/backing_file = '/tmp/LTP_iocMhKZxH/test.img'
ioctl_loop01.c:57: TPASS: get expected lo_flag 12
ioctl_loop01.c:59: TPASS: /sys/block/loop0/loop/partscan = 1
ioctl_loop01.c:60: TPASS: /sys/block/loop0/loop/autoclear = 1
ioctl_loop01.c:69: TPASS: access /dev/loop0p1 succeeds
ioctl_loop01.c:75: TPASS: access /sys/block/loop0/loop0p1 succeeds
ioctl_loop01.c:90: TINFO: Test flag can be clear
ioctl_loop01.c:57: TPASS: get expected lo_flag 8
ioctl_loop01.c:59: TPASS: /sys/block/loop0/loop/partscan = 1
ioctl_loop01.c:60: TPASS: /sys/block/loop0/loop/autoclear = 0
ioctl_loop01.c:69: TPASS: access /dev/loop0p1 succeeds
ioctl_loop01.c:75: TPASS: access /sys/block/loop0/loop0p1 succeeds
ioctl_loop01.c:82: TBROK: Failed to open FILE '/sys/block/loop0/loop/partscan' for reading: ENOENT (2)

> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> Signed-off-by: Sebastian Chlad <sebastianchlad@gmail.com>
> ---
> Changes in v2:
> - Use TST_RETRY_FN_EXP_BACKOFF() instead of scaling the retry count
>   with tst_multiply_timeout() (Cyril).
> - Link to v1:
>   https://lore.kernel.org/ltp/20261022115410.23845-1-sebastian.chlad@suse.com/

>  lib/tst_device.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)

> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index 00304711b..bf77c9ded 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -249,9 +249,14 @@ uint64_t tst_get_device_size(const char *dev_path)
>  	return size/1024/1024;
>  }

> +static int loop_clr_fd_busy(int dev_fd)
> +{
> +	return !ioctl(dev_fd, LOOP_CLR_FD, 0) || errno == EBUSY;
> +}
> +
>  static int detach_loop_fd(const char *dev, int *dev_fd)
>  {
> -	int ret, i, retval = 1;
> +	int ret, retval = 1;

>  	/* keep trying to clear LOOPDEV until we get ENXIO, a quick succession
>  	 * of attach/detach might not give udev enough time to complete
> @@ -259,22 +264,19 @@ static int detach_loop_fd(const char *dev, int *dev_fd)
>  	 * Since 18048c1af783 ("loop: Fix a race between loop detach and loop open")
>  	 * device is detached only after last close.
>  	 */
> -	for (i = 0; i < 40; i++) {
> -		ret = ioctl(*dev_fd, LOOP_CLR_FD, 0);
> +	ret = TST_RETRY_FN_EXP_BACKOFF(loop_clr_fd_busy(*dev_fd),
> +				       TST_RETVAL_EQ0, 2);

> -		if (ret && (errno == ENXIO)) {
> -			retval = 0;
> -			goto exit;
> -		}
> -
> -		if (ret && (errno != EBUSY)) {
> -			tst_resm(TWARN,
> -				 "ioctl(%s, LOOP_CLR_FD, 0) unexpectedly failed with: %s",
> -				 dev, tst_strerrno(errno));
> -			goto exit;
> -		}
> +	if (!ret && (errno == ENXIO)) {
> +		retval = 0;
> +		goto exit;
> +	}

> -		usleep(50000);
> +	if (!ret) {
> +		tst_resm(TWARN,
> +			 "ioctl(%s, LOOP_CLR_FD, 0) unexpectedly failed with: %s",
> +			 dev, tst_strerrno(errno));

nit: While you at it, could you change to use TERRNO?

		tst_resm(TWARN | TERRNO,
			 "ioctl(%s, LOOP_CLR_FD, 0) failed, dev);

But maybe it can be done as an separate effort (there are more tst_strerrno()
calls).

The rest LGTM. Thanks!

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

Kind regards,
Petr

> +		goto exit;
>  	}

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