On 06/09/2015 03:19 PM, Jan Stancek wrote:
>
>
>
>
> ----- Original Message -----
>> From: "Stanislav Kholmanskikh" <stanislav.kholmansk...@oracle.com>
>> To: "Cyril Hrubis" <chru...@suse.cz>, "Jan Stancek" <jstan...@redhat.com>
>> Cc: ltp-list@lists.sourceforge.net, "vasily isaenko"
>> <vasily.isae...@oracle.com>
>> Sent: Tuesday, 9 June, 2015 1:51:46 PM
>> Subject: Re: [LTP] [RFC PATCH 5/5] acl_test01: Wait until the loop device is
>> detached
>>
>>
>>
>> On 06/09/2015 01:10 PM, Cyril Hrubis wrote:
>>> Hi!
>>>> If the test is executed on NFS, it may print:
>>>>
>>>> rm: cannot remove `/mnt/ltp-xeJYpESVKz/acltest01.fCogoo4gn0': Directory
>>>> not empty
>>>>
>>>> It seems the following happen:
>>>>
>>>> 1. After 'unmount -d' 'acltest01.fCogoo4gn0/blkext3' is still in use by
>>>> udev.
>>>> See kernel commit a1ecac3b0656a68259927c234e505804d33a7b83
>>>> ("loop: Make explicit loop device destruction lazy")
>>>>
>>>> 2. Due to the "NFS silly rename" feature, command 'rm -f
>>>> acltest01.fCogoo4gn0/blkext3'
>>>> renames 'blkext3' to '.nfs*'
>>>>
>>>> 3. The removal of 'acltest01.fCogoo4gn0' directory fails, because it
>>>> still contains this '.nfs*' file.
>>>>
>>>> Intorucing a wait cycle should fix this problem.
>>>
>>> Hmm, we may want to add this into test.sh because otherwise we will need
>>> to carry this snippet of code in each testcase that works with loop
>>> devices.
>>>
>>> Would calling umount without -d and detach_device() (from tst_device.c
>>> via apicmd) help here? (it seems to fix the very same problem, it this
>>> function does not work, it needs to be fixed anyway)
>>>
>>
>> I think it wouldn't. Please, see below.
>>
>> ## Problem 1 ##
>> Rapid execution of 'losetup /dev/loop0' file + 'losetup -d /dev/loop0'
>> may fail.
>>
>> Reproducer:
>> for i in `seq 20`; do
>> echo "attach"
>> losetup /dev/loop0 file1.img
>> echo "detach"
>> losetup -d /dev/loop0
>> done
>>
>> 1. Kernel versions without the above kernel patch should fail on the
>> "detach" phase, i.e. on
>> if (lo->lo_refcnt > 1) /* we needed one fd for the ioctl */
>> return -EBUSY;
>> in loop_clr_fd().
>>
>> However, I've not tested it by myself.
>>
>> And Jan's commit:
>>
>> commit 554b1793a1aa34f8918e8b1c04d8c6835f4a5710
>> Author: Jan Stancek <jstan...@redhat.com>
>> Date: Tue Jan 20 14:35:49 2015 +0100
>>
>> 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 <jstan...@redhat.com>
>> Acked-by: Cyril Hrubis <chru...@suse.cz>
>>
>> is to fix this scenario.
>>
>> 2. However, newer kernels (i.e. with the above kernel patch) will fail
>> with EBUSY at the "attach" phase, because loop_clr_fd() will always
>> return 0 and mark the loop device for auto clearing, but loop_set_fd()
>> will go this path:
>>
>> error = -EBUSY;
>> if (lo->lo_state != Lo_unbound)
>> goto out_putf;
>>
>> /* ... */
>>
>> out_putf:
>> fput(file);
>> out:
>> /* This is safe: open() is still holding a reference. */
>> module_put(THIS_MODULE);
>> return error;
>>
>> ## Problem 2 - NFS specific ##
>>
>> If we have the above kernel patch, even if 'losetup -d' executed without
>> problems, it doesn't mean that the backend file is not in use.
>>
>> So we need to wait some undetermined time until udev finishes its
>> playing with the loop device, and only after that we can safely remove
>> the file without a risk to be affected by "NFS silly rename".
>>
>> ##
>>
>> I think that to nail down both the problems at once, we may need to do
>> something like this:
>>
>> attach_device(const char *dev)
>> {
>> /* a cycle to verify that dev is not in use.
>> * If it's in use after the cycle, call tst_brkm()
>> */
>> ioctl(dev_fd, LOOP_SET_FD);
>> /* handle ioctl errors */
>> }
>>
>> detach_device(const char *dev)
>> {
>> ioctl(dev_fd, LOOP_CLR_FD);
>> /* ignore the return code */
>>
>> /* a cycle to verify that dev is not in use.
>> * If it's still in use after the cycle, call tst_brkm()
>> */
>> }
>>
>> ioctl(LOOP_GET_STATUS64) looks like a good way to check whether the loop
>> device is in use.
>>
>> What do you think?
>
> A cycle in detach_device() to make sure it's no longer in use
> on kernels which have lazy cleanup seems reasonable to me.
>
> But I'm not following, why is it needed to have same cycle also in
> attach_device()?
Yes, a one more look made me feel that that cycle is not required in
attach_device(), it would be useless.
Sorry for confusion.
>
> Regards,
> Jan
>
------------------------------------------------------------------------------
_______________________________________________
Ltp-list mailing list
Ltp-list@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ltp-list
