[OE-core] [PATCH] mtools: fix race issue while mtools invoked frequently

Fri Aug 24 14:18:41 UTC 2018

On 08/23/2018 11:06 PM, Hongxu Jia wrote:
> On 2018年08月24日 04:47, Randy MacLeod wrote:
>> On 08/22/2018 05:16 AM, Hongxu Jia wrote:
>>> While invoking mtools frequently, the unblocking request
>>
>> s/unblocking/non-blocking/
>>
>> I would read 'unblocking' as a request that results in the
>> system becoming unblocked whereas non-blocking is more
>> commonly used and refers to function calls that will
>> not result in calling blocking system calls.
>>
> 
> OK, thanks  to point it out
> 
>> Indeed the NB in LOCK_NB stand for non-blocking.
>>
>>> caused race issue. Here is an example of syslinux
>>> [snip]
>>> dd if=/dev/zero of=floppy.img bs=1024 count=144
>>> losetup /dev/loop1 floppy.img
>>> mkdosfs /dev/loop1
>>> syslinux -i /dev/loop1
>>> |plain floppy: device "/proc/6351/fd/3" busy (Resource temporarily 
>>> unavailable):
>>> |Cannot initialize 'S:'
>>> |Bad target s:/ldlinux.sys
>>> [snip]
>>>
>>> The idea is from:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1235016
>>> https://groups.google.com/a/chromium.org/forum/#!msg/chromium-os-dev/bRPUCFHoBTQ/ZjB8kjjx1vUJ 
>>>
>>
>> I read though most of that and it seems that people think
>> that this may be a work-around. What do you think?
>>
>> If I were used to using mtools and it never blocked,
>> the I'd certainly be upset that we patched it to block.
>> lockdev.h gets used by:
>>
>> $ grep -r "lockdev.h" mtools-4.0.18/
>> mtools-4.0.18/plain_io.c:#include "lockdev.h"
>> mtools-4.0.18/floppyd.c:#include "lockdev.h"
>>
>> so until you can explain what the root cause of the
>> problem is more clearly, and why this change won't cause
>> problems for users of mtools, I think you should continue
>> to work on this problem.
>>
>> Or maybe the commit just needs a more details technical
>> explanation that summarizes the long thread that you linked
>> to...
>>
> 
>  From the thread, and my confirm, the last update of mtools is 2013-01-09,
> and Ubuntu 1604/1804 has the same issue on `syslinux -i /dev/loop1'.
> 
> With some digging on mtools, the user of mtools usually have a mtools.conf
> to customize it, and use `nolock' for specific device to workaround
> flock invoking (such as `drive c: file="/dev/sda1" nolock=1'). but 
> unfortunately
> syslinux does not follow the rule (it is hardly to know `/proc/***/fd/3' 
> before
> invoking syslinux )
> 
> [snip]
>   42         if(IS_NOLOCK(dev))
>   43                 return 0;
>   44
>   45 #if (defined(HAVE_FLOCK) && defined (LOCK_EX) && defined(LOCK_NB))
>   46         if (flock(fd, (mode ? LOCK_EX : LOCK_SH)) < 0)
> [snip]
> 
> On the other side, the error of `plain floppy: device "***" busy' 
> exactly comes
> from the error of flock invoking (not actual resource busy), so the fix 
> is safe
> for re-entry of mtools
> [snip]
>          /* lock the device on writes */
>          if (locked && lock_dev(This->fd, mode == O_RDWR, dev)) {
>                  if(errmsg)
> #ifdef HAVE_SNPRINTF
>                          snprintf(errmsg,199,
>                                  "plain floppy: device \"%s\" busy (%s):",
>                                  dev ? dev->name : "unknown", 
> strerror(errno));
> [snip]

Ok, thanks for explaining that.

> 
> I think long time no maintenance of mtools, and not to fix this 
> obviously issue.

Yeah, they don't even have a git repo for the project.

Thanks,
../Randy

> 
> //Hongxu
> 
>> ../Randy
>>
>>
>>>
>>> Signed-off-by: Hongxu Jia <hongxu.jia at windriver.com>
>>> ---
>>>   ...01-remove-LOCK_NB-to-use-blocking-request.patch | 44 
>>> ++++++++++++++++++++++
>>>   meta/recipes-devtools/mtools/mtools_4.0.18.bb      |  1 +
>>>   2 files changed, 45 insertions(+)
>>>   create mode 100644 
>>> meta/recipes-devtools/mtools/mtools/0001-remove-LOCK_NB-to-use-blocking-request.patch 
>>>
>>>
>>> diff --git 
>>> a/meta/recipes-devtools/mtools/mtools/0001-remove-LOCK_NB-to-use-blocking-request.patch 
>>> b/meta/recipes-devtools/mtools/mtools/0001-remove-LOCK_NB-to-use-blocking-request.patch 
>>>
>>> new file mode 100644
>>> index 0000000..47385a5
>>> --- /dev/null
>>> +++ 
>>> b/meta/recipes-devtools/mtools/mtools/0001-remove-LOCK_NB-to-use-blocking-request.patch 
>>>
>>> @@ -0,0 +1,44 @@
>>> +From 5bdbfe0a63fed48104b17412854b26ee2275869a Mon Sep 17 00:00:00 2001
>>> +From: Hongxu Jia <hongxu.jia at windriver.com>
>>> +Date: Wed, 22 Aug 2018 16:54:39 +0800
>>> +Subject: [PATCH] remove LOCK_NB to use blocking request
>>> +
>>> +While invoking mtools frequently, the unblocking request
>>> +caused race issue. Here is an example of syslinux
>>> +[snip]
>>> +dd if=/dev/zero of=floppy.img bs=1024 count=144
>>> +losetup /dev/loop1 floppy.img
>>> +mkdosfs /dev/loop1
>>> +syslinux -i /dev/loop1
>>> +|plain floppy: device "/proc/6351/fd/3" busy (Resource temporarily 
>>> unavailable):
>>> +|Cannot initialize 'S:'
>>> +|Bad target s:/ldlinux.sys
>>> +[snip]
>>> +
>>> +The idea is from:
>>> +https://bugzilla.redhat.com/show_bug.cgi?id=1235016
>>> +https://groups.google.com/a/chromium.org/forum/#!msg/chromium-os-dev/bRPUCFHoBTQ/ZjB8kjjx1vUJ 
>>>
>>> +
>>> +Upstream-Status: Pending
>>> +
>>> +Signed-off-by: Hongxu Jia <hongxu.jia at windriver.com>
>>> +---
>>> + lockdev.h | 2 +-
>>> + 1 file changed, 1 insertion(+), 1 deletion(-)
>>> +
>>> +diff --git a/lockdev.h b/lockdev.h
>>> +index 4467bc2..5a135ad 100644
>>> +--- a/lockdev.h
>>> ++++ b/lockdev.h
>>> +@@ -43,7 +43,7 @@ int lock_dev(int fd, int mode, struct device *dev)
>>> +         return 0;
>>> +
>>> + #if (defined(HAVE_FLOCK) && defined (LOCK_EX) && defined(LOCK_NB))
>>> +-    if (flock(fd, (mode ? LOCK_EX : LOCK_SH)|LOCK_NB) < 0)
>>> ++    if (flock(fd, (mode ? LOCK_EX : LOCK_SH)) < 0)
>>
>>> + #else /* FLOCK */
>>> +
>>> + #if (defined(HAVE_LOCKF) && defined(F_TLOCK))
>>> +--
>>> +2.7.4
>>> +
>>> diff --git a/meta/recipes-devtools/mtools/mtools_4.0.18.bb 
>>> b/meta/recipes-devtools/mtools/mtools_4.0.18.bb
>>> index dcd32ed..91f7b7c 100644
>>> --- a/meta/recipes-devtools/mtools/mtools_4.0.18.bb
>>> +++ b/meta/recipes-devtools/mtools/mtools_4.0.18.bb
>>> @@ -31,6 +31,7 @@ SRC_URI = "${GNU_MIRROR}/mtools/mtools-${PV}.tar.bz2 \
>>>              file://mtools-makeinfo.patch \
>>>              file://no-x11.gplv3.patch \
>>> file://0001-Continue-even-if-fs-size-is-not-divisible-by-sectors.patch \
>>> + file://0001-remove-LOCK_NB-to-use-blocking-request.patch \
>>>              "
>>>     SRC_URI_append_class-native = " 
>>> file://disable-hardcoded-configs.patch"
>>>
>>
>>
> 

-- 
# Randy MacLeod
# Wind River Linux