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

[Hongxu Jia]

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]

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

//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"
>>
>
>