Re: [LTP] [PATCH] runltp: export initialized LTP_DEV

Re: [LTP] [PATCH] runltp: export initialized LTP_DEV

[Cyril Hrubis]

> commit 5c7c3fb219957484db92111d423e4e2553fb580b removed initialization
> of empty LTP_DEV from setup(), but it also caused that LTP_DEV
> is no longer exported when it's initialized in set_block_device().
> 
> This causes every test to attempt to get loop device on its own,
> which is not necessary and it also makes tests sporadically fail with EBUSY:
>   creat06     9  TBROK  :  tst_device.c:156: ioctl(/dev/loop1, LOOP_CLR_FD, 0) failed: EBUSY
>   creat06    10  TBROK  :  tst_device.c:156: Remaining cases broken
> 
> The cause of EBUSY is unknown. Cyril suggested it might be gvfsd-trash,
> however I don't have such process and still get the failures sporadically.

The EBUSY I've seen was from umount() rather that from ioctl() and was
caused by stat() from gvfsd-trash. I'm sure of it.

In that case one of the testcases fails to umount the device as:

acct01      0  TWARN  :  acct01.c:225: umount device:/dev/loop1 failed: errno=EBUSY(16): Device or resource busy

I've never seen EBUSY from the ioctl() that removes the loop device,
that looks like a kernel bug to me.

> This patch restores behavior from ltp-20140828, where LTP_DEV is exported
> and tst_acquire_device() will use that instead of trying to acquire/release
> loopdev on its own.

That wouldn't do because that breaks the exectuion when device was not
passed to runltp. What about exporting it but only when it's non-empty?

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] runltp: export initialized LTP_DEV

[Cyril Hrubis]

Hi!
> > This patch restores behavior from ltp-20140828, where LTP_DEV is exported
> > and tst_acquire_device() will use that instead of trying to acquire/release
> > loopdev on its own.
> 
> That wouldn't do because that breaks the exectuion when device was not
> passed to runltp. What about exporting it but only when it's non-empty?

To explain it better. That breaks execution on systems that has no
support for loop devices (not compiled in kernel), because the function
to prepare loop devices fails and LTP_DEV ends up exported as empty
string.

The library then pickups the empty string and tries to format it with
mkfs.

Looking at the code now, the code seems to be prepared for that option
the LTP_DEV should have been exported only if the create_block() function
was sucessfull but the code does apparently does not work for the case
above.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] runltp: export initialized LTP_DEV

[Cyril Hrubis]

Hi!
> This patch restores behavior from ltp-20140828, where LTP_DEV is exported
> and tst_acquire_device() will use that instead of trying to acquire/release
> loopdev on its own.

Sorry for my confusion. This patch is correct as it moves the export to
the right places. I didn't read till it carefully and thought this
simply reverts the change, sorry.

The change is acked, but I think that it merely avoids the EBUSY from
the ioctl() and that there is a real bug somewhere, likely some race
condition in kernel.

> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  runltp |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/runltp b/runltp
> index e803898..8116306 100755
> --- a/runltp
> +++ b/runltp
> @@ -1011,10 +1011,10 @@ set_block_device()
>              echo "Tests which require block device are disabled."
>              echo "You can specify it with option -b"
>  	else
> -            LTP_DEV=$DEVICE
> +            export LTP_DEV=$DEVICE
>          fi
>      else
> -        LTP_DEV=$DEVICE
> +        export LTP_DEV=$DEVICE
>      fi
>  }
>  
> -- 
> 1.7.1
> 

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] runltp: export initialized LTP_DEV

[Cyril Hrubis]

Hi!
> ---
> commit 5c7c3fb219957484db92111d423e4e2553fb580b removed initialization
> of empty LTP_DEV from setup(), but it also caused that LTP_DEV
> is no longer exported when it's initialized in set_block_device().
> 
> This causes every test to attempt to get loop device on its own,
> which is not necessary and may presumably sporadically cause issues
> on older buggy kernels during cleanup of loop device.
> 
> This patch makes LTP_DEV exported (when it's not empty) and
> tst_acquire_device() will use that instead of trying to acquire/release
> loop device on its own for each test.
> ---

Looks good.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list

Re: [LTP] [PATCH] runltp: export initialized LTP_DEV

[Cyril Hrubis]

Hi!
> So, it looks like this is not bug after all, but udev rule that keeps
> firing "blkid":
> 
> /lib/udev/rules.d/60-persistent-storage.rules:84
>   KERNEL!="xvd*|sr*", IMPORT{program}="/sbin/blkid -o udev -p $tempnode"
>     /sbin/blkid -o udev -p /dev/loop0
> 
> If attach/detach happens too quickly, blkid might still hold reference
> and loop_clr_fd() then complains with EBUSY:
>         if (lo->lo_refcnt > 1)  /* we needed one fd for the ioctl */
>                 return -EBUSY;
>
> The only idea I have is to sleep/retry for a short period - see attached patch.

Sounds reasonable to me.

I'm thinking of fixing the umount problem with the gvfsd-trash the same
way.

> From 0ae0f16500cc48795287076edb2b725025466a0f Mon Sep 17 00:00:00 2001
> Message-Id: <0ae0f16500cc48795287076edb2b725025466a0f.1421834144.git.jstancek@redhat.com>
> From: Jan Stancek <jstancek@redhat.com>
> Date: Tue, 20 Jan 2015 14:35:49 +0100
> Subject: [PATCH] tst_device: sleep/retry if LOOP_CLR_FD fails with EBUSY
> 
> Rapid succession of device attach/detach may lead to EBUSY
> from ioctl(LOOP_CLR_FD), because udev rules might still be
> running.
> 
> Sleep/retry for a short period if LOOP_CLR_FD fails with EBUSY.
> 
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  lib/tst_device.c |   25 ++++++++++++++++++-------
>  1 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/tst_device.c b/lib/tst_device.c
> index 6dff1ba..edf0bb4 100644
> --- a/lib/tst_device.c
> +++ b/lib/tst_device.c
> @@ -144,19 +144,30 @@ static void attach_device(void (*cleanup_fn)(void),
>  
>  static void detach_device(void (*cleanup_fn)(void), const char *dev)
>  {
> -	int dev_fd, err;
> +	int dev_fd, err, i;
>  
>  	dev_fd = SAFE_OPEN(cleanup_fn, dev, O_RDONLY);
>  
> -	if (ioctl(dev_fd, LOOP_CLR_FD, 0) < 0) {
> -		err = errno;
> -		close(dev_fd);
> -		tst_brkm(TBROK, cleanup_fn,
> -		         "ioctl(%s, LOOP_CLR_FD, 0) failed: %s",
> -			 dev, tst_strerrno(err));
> +	/* keep trying to clear LOOPDEV fd if EBUSY, a quick succession
> +	 * of attach/detach might not give udev enough time to complete */
> +	for (i = 0; i < 40; i++) {
> +		if (ioctl(dev_fd, LOOP_CLR_FD, 0) == 0) {
> +			close(dev_fd);
> +			return;
> +		}
> +		if (errno != EBUSY) {
> +			err = errno;
> +			close(dev_fd);
> +			tst_brkm(TBROK, cleanup_fn,
> +				"ioctl(%s, LOOP_CLR_FD, 0) failed: %s",
> +				dev, tst_strerrno(err));
> +		}
> +		usleep(50000);
>  	}
>  
>  	close(dev_fd);
> +	tst_brkm(TBROK, cleanup_fn,
> +		"ioctl(%s, LOOP_CLR_FD, 0) EBUSY for too long",	dev);
>  }

Looks good to me.

-- 
Cyril Hrubis
chrubis@suse.cz

------------------------------------------------------------------------------
New Year. New Location. New Benefits. New Data Center in Ashburn, VA.
GigeNET is offering a free month of service with a new server in Ashburn.
Choose from 2 high performing configs, both with 100TB of bandwidth.
Higher redundancy.Lower latency.Increased capacity.Completely compliant.
http://p.sf.net/sfu/gigenet
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list