All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
@ 2018-07-19  3:41 Fam Zheng
  2018-07-19 17:57 ` [Qemu-devel] [Qemu-block] " John Snow
  2018-07-21 21:08 ` [Qemu-devel] " Max Reitz
  0 siblings, 2 replies; 13+ messages in thread
From: Fam Zheng @ 2018-07-19  3:41 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Max Reitz, qemu-block

On my Fedora 28, /dev/null is locked by some other process (couldn't
inspect it due to the current lslocks limitation), so iotests 226 fails
with some unexpected image locking errors because it uses qemu-io to
open it.

Actually it's safe to not use any lock on /dev/null or /dev/zero.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/file-posix.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 60af4b3d51..8bf034108a 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
         s->use_lock = false;
         break;
     case ON_OFF_AUTO_AUTO:
-        s->use_lock = qemu_has_ofd_lock();
+        if (!strcmp(filename, "/dev/null") ||
+                   !strcmp(filename, "/dev/zero")) {
+            s->use_lock = false;
+        } else {
+            s->use_lock = qemu_has_ofd_lock();
+        }
         break;
     default:
         abort();
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
  2018-07-19  3:41 [Qemu-devel] [PATCH] block: Don't lock /dev/null and /dev/zero automatically Fam Zheng
@ 2018-07-19 17:57 ` John Snow
  2018-07-20  8:24   ` Fam Zheng
  2018-07-20 16:34   ` Eric Blake
  2018-07-21 21:08 ` [Qemu-devel] " Max Reitz
  1 sibling, 2 replies; 13+ messages in thread
From: John Snow @ 2018-07-19 17:57 UTC (permalink / raw)
  To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz



On 07/18/2018 11:41 PM, Fam Zheng wrote:
> On my Fedora 28, /dev/null is locked by some other process (couldn't
> inspect it due to the current lslocks limitation), so iotests 226 fails
> with some unexpected image locking errors because it uses qemu-io to
> open it.
> 
> Actually it's safe to not use any lock on /dev/null or /dev/zero.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/file-posix.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 60af4b3d51..8bf034108a 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>          s->use_lock = false;
>          break;
>      case ON_OFF_AUTO_AUTO:
> -        s->use_lock = qemu_has_ofd_lock();
> +        if (!strcmp(filename, "/dev/null") ||
> +                   !strcmp(filename, "/dev/zero")) {
> +            s->use_lock = false;
> +        } else {
> +            s->use_lock = qemu_has_ofd_lock();
> +        }
>          break;
>      default:
>          abort();
> 

I apparently caused a lot of failures with 226, oops!

Should we instead modify the test in this case to not attempt to take a
lock on a device we know cannot meaningfully store state, or is it your
preference to attempt to maintain such a list in the raw driver itself?

I guess we never want QEMU to try to lock things like /dev/zero, but I
don't know if there are more such pseudo-devices we should never try to
lock and if so, what common property unifies them such that we don't
have to maintain a list.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
  2018-07-19 17:57 ` [Qemu-devel] [Qemu-block] " John Snow
@ 2018-07-20  8:24   ` Fam Zheng
  2018-07-20 16:35     ` Eric Blake
  2018-07-20 18:56     ` John Snow
  2018-07-20 16:34   ` Eric Blake
  1 sibling, 2 replies; 13+ messages in thread
From: Fam Zheng @ 2018-07-20  8:24 UTC (permalink / raw)
  To: John Snow; +Cc: qemu-devel, Kevin Wolf, qemu-block, Max Reitz

On Thu, 07/19 13:57, John Snow wrote:
> Should we instead modify the test in this case to not attempt to take a
> lock on a device we know cannot meaningfully store state, or is it your
> preference to attempt to maintain such a list in the raw driver itself?
> 
> I guess we never want QEMU to try to lock things like /dev/zero, but I
> don't know if there are more such pseudo-devices we should never try to
> lock and if so, what common property unifies them such that we don't
> have to maintain a list.

They are 0 sized anyway so I only expect them to be used in test cases like the
one we're fixing. So this really doesn't matter.  An exceptional one would be
/dev/nullb* but that is not used in real world either.

I'm not trying to maintain a complete list (e.g. /dev/urandom and /dev/nullb*
are missing), this patch is just trying to make writing test code easier.

Fam

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
  2018-07-19 17:57 ` [Qemu-devel] [Qemu-block] " John Snow
  2018-07-20  8:24   ` Fam Zheng
@ 2018-07-20 16:34   ` Eric Blake
  1 sibling, 0 replies; 13+ messages in thread
From: Eric Blake @ 2018-07-20 16:34 UTC (permalink / raw)
  To: John Snow, Fam Zheng, qemu-devel; +Cc: Kevin Wolf, qemu-block, Max Reitz

On 07/19/2018 12:57 PM, John Snow wrote:
> 
> Should we instead modify the test in this case to not attempt to take a
> lock on a device we know cannot meaningfully store state, or is it your
> preference to attempt to maintain such a list in the raw driver itself?
> 
> I guess we never want QEMU to try to lock things like /dev/zero, but I
> don't know if there are more such pseudo-devices we should never try to
> lock and if so, what common property unifies them such that we don't
> have to maintain a list.

One potential common property: /dev/zero and /dev/null are character 
devices, rather than block devices.  Character devices in general don't 
make much sense for holding stateful images, so it may be as simple as 
gating our locking based on fstat() distinguishing which type of file we 
are accessing.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
  2018-07-20  8:24   ` Fam Zheng
@ 2018-07-20 16:35     ` Eric Blake
  2018-07-21  6:26       ` Fam Zheng
  2018-07-20 18:56     ` John Snow
  1 sibling, 1 reply; 13+ messages in thread
From: Eric Blake @ 2018-07-20 16:35 UTC (permalink / raw)
  To: Fam Zheng, John Snow; +Cc: Kevin Wolf, qemu-devel, qemu-block, Max Reitz

On 07/20/2018 03:24 AM, Fam Zheng wrote:
> On Thu, 07/19 13:57, John Snow wrote:
>> Should we instead modify the test in this case to not attempt to take a
>> lock on a device we know cannot meaningfully store state, or is it your
>> preference to attempt to maintain such a list in the raw driver itself?
>>
>> I guess we never want QEMU to try to lock things like /dev/zero, but I
>> don't know if there are more such pseudo-devices we should never try to
>> lock and if so, what common property unifies them such that we don't
>> have to maintain a list.
> 
> They are 0 sized anyway so I only expect them to be used in test cases like the
> one we're fixing. So this really doesn't matter.  An exceptional one would be
> /dev/nullb* but that is not used in real world either.

I'm not familiar with /dev/nullb* - is that a typo?

$ ll /dev/nullb*
ls: cannot access '/dev/nullb*': No such file or directory


> 
> I'm not trying to maintain a complete list (e.g. /dev/urandom and /dev/nullb*
> are missing), this patch is just trying to make writing test code easier.

/dev/urandom is also a character device, and also fits in my heuristic 
that we likely only care about locking of block devices.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
  2018-07-20  8:24   ` Fam Zheng
  2018-07-20 16:35     ` Eric Blake
@ 2018-07-20 18:56     ` John Snow
  1 sibling, 0 replies; 13+ messages in thread
From: John Snow @ 2018-07-20 18:56 UTC (permalink / raw)
  To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, qemu-block, Max Reitz



On 07/20/2018 04:24 AM, Fam Zheng wrote:
> On Thu, 07/19 13:57, John Snow wrote:
>> Should we instead modify the test in this case to not attempt to take a
>> lock on a device we know cannot meaningfully store state, or is it your
>> preference to attempt to maintain such a list in the raw driver itself?
>>
>> I guess we never want QEMU to try to lock things like /dev/zero, but I
>> don't know if there are more such pseudo-devices we should never try to
>> lock and if so, what common property unifies them such that we don't
>> have to maintain a list.
> 
> They are 0 sized anyway so I only expect them to be used in test cases like the
> one we're fixing. So this really doesn't matter.  An exceptional one would be
> /dev/nullb* but that is not used in real world either.
> 

I agree in general; I'm questioning somewhat if we ought to just patch
the test instead of the driver, given that we can't really be consistent
or accurate about when we decide not to take the lock in the driver;
unless we use something like Eric's suggestion (we don't take locks on
char devices, maybe?)

> I'm not trying to maintain a complete list (e.g. /dev/urandom and /dev/nullb*
> are missing), this patch is just trying to make writing test code easier.
> 

Yeah, it's the little quality-of-life band-aid that I'm wondering if we
ought to stick in here. Granted, this fixes a test that's broken, so...

Reviewed-by: John Snow <jsnow@redhat.com>

I'll let Kevin decide if we ought to patch it nicer than this.

--js

> Fam
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
  2018-07-20 16:35     ` Eric Blake
@ 2018-07-21  6:26       ` Fam Zheng
  0 siblings, 0 replies; 13+ messages in thread
From: Fam Zheng @ 2018-07-21  6:26 UTC (permalink / raw)
  To: Eric Blake; +Cc: jsnow, Kevin Wolf, QEMU Developers, qemu block, Max Reitz

On Sat, Jul 21, 2018 at 12:35 AM Eric Blake <eblake@redhat.com> wrote:
>
> On 07/20/2018 03:24 AM, Fam Zheng wrote:
> I'm not familiar with /dev/nullb* - is that a typo?
>
> $ ll /dev/nullb*
> ls: cannot access '/dev/nullb*': No such file or directory

You probably have figured out already but just in case: it's kernel
null_blk mod.

Fam

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
  2018-07-19  3:41 [Qemu-devel] [PATCH] block: Don't lock /dev/null and /dev/zero automatically Fam Zheng
  2018-07-19 17:57 ` [Qemu-devel] [Qemu-block] " John Snow
@ 2018-07-21 21:08 ` Max Reitz
  2018-07-22  2:37   ` Fam Zheng
  1 sibling, 1 reply; 13+ messages in thread
From: Max Reitz @ 2018-07-21 21:08 UTC (permalink / raw)
  To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, qemu-block

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On 2018-07-19 05:41, Fam Zheng wrote:
> On my Fedora 28, /dev/null is locked by some other process (couldn't
> inspect it due to the current lslocks limitation), so iotests 226 fails
> with some unexpected image locking errors because it uses qemu-io to
> open it.
> 
> Actually it's safe to not use any lock on /dev/null or /dev/zero.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/file-posix.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 60af4b3d51..8bf034108a 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>          s->use_lock = false;
>          break;
>      case ON_OFF_AUTO_AUTO:
> -        s->use_lock = qemu_has_ofd_lock();
> +        if (!strcmp(filename, "/dev/null") ||
> +                   !strcmp(filename, "/dev/zero")) {

I’m not sure I like a strcmp() based on filename (though it should work
for all of the cases where someone would want to specify either of those
for a qemu block device).  Isn’t there some specific major/minor number
we can use to check for these two files?  Or are those Linux-specific?

I could also imagine just not locking any host_device, but since people
do use qcow2 immediately on block devices, maybe we do want to lock them.

Finally, if really all you are trying to do is to make test code easier,
then I don’t know exactly why.  Just disabling locking in 226 shouldn’t
be too hard.

Max

> +            s->use_lock = false;
> +        } else {
> +            s->use_lock = qemu_has_ofd_lock();
> +        }
>          break;
>      default:
>          abort();
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
  2018-07-21 21:08 ` [Qemu-devel] " Max Reitz
@ 2018-07-22  2:37   ` Fam Zheng
  2018-07-22 14:06     ` Max Reitz
  0 siblings, 1 reply; 13+ messages in thread
From: Fam Zheng @ 2018-07-22  2:37 UTC (permalink / raw)
  To: Max Reitz; +Cc: QEMU Developers, Kevin Wolf, qemu block

On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote:
>
> On 2018-07-19 05:41, Fam Zheng wrote:
> > On my Fedora 28, /dev/null is locked by some other process (couldn't
> > inspect it due to the current lslocks limitation), so iotests 226 fails
> > with some unexpected image locking errors because it uses qemu-io to
> > open it.
> >
> > Actually it's safe to not use any lock on /dev/null or /dev/zero.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/file-posix.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 60af4b3d51..8bf034108a 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> >          s->use_lock = false;
> >          break;
> >      case ON_OFF_AUTO_AUTO:
> > -        s->use_lock = qemu_has_ofd_lock();
> > +        if (!strcmp(filename, "/dev/null") ||
> > +                   !strcmp(filename, "/dev/zero")) {
>
> I’m not sure I like a strcmp() based on filename (though it should work
> for all of the cases where someone would want to specify either of those
> for a qemu block device).  Isn’t there some specific major/minor number
> we can use to check for these two files?  Or are those Linux-specific?

Yeah, I guess major/minor numbers are Linux-specific.

>
> I could also imagine just not locking any host_device, but since people
> do use qcow2 immediately on block devices, maybe we do want to lock them.

That's right.

>
> Finally, if really all you are trying to do is to make test code easier,
> then I don’t know exactly why.  Just disabling locking in 226 shouldn’t
> be too hard.

The tricky thing is in remembering to do that for future test cases.
My machine seems to be somehow different than John's so that my 226
cannot lock /dev/null, but I'm not sure that is the case for other
releases, distros or system instances.

Fam

>
> Max
>
> > +            s->use_lock = false;
> > +        } else {
> > +            s->use_lock = qemu_has_ofd_lock();
> > +        }
> >          break;
> >      default:
> >          abort();
> >
>
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
  2018-07-22  2:37   ` Fam Zheng
@ 2018-07-22 14:06     ` Max Reitz
  2018-07-23  1:56       ` Fam Zheng
  0 siblings, 1 reply; 13+ messages in thread
From: Max Reitz @ 2018-07-22 14:06 UTC (permalink / raw)
  To: Fam Zheng; +Cc: QEMU Developers, Kevin Wolf, qemu block

[-- Attachment #1: Type: text/plain, Size: 2378 bytes --]

On 2018-07-22 04:37, Fam Zheng wrote:
> On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote:
>>
>> On 2018-07-19 05:41, Fam Zheng wrote:
>>> On my Fedora 28, /dev/null is locked by some other process (couldn't
>>> inspect it due to the current lslocks limitation), so iotests 226 fails
>>> with some unexpected image locking errors because it uses qemu-io to
>>> open it.
>>>
>>> Actually it's safe to not use any lock on /dev/null or /dev/zero.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>  block/file-posix.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>> index 60af4b3d51..8bf034108a 100644
>>> --- a/block/file-posix.c
>>> +++ b/block/file-posix.c
>>> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>>>          s->use_lock = false;
>>>          break;
>>>      case ON_OFF_AUTO_AUTO:
>>> -        s->use_lock = qemu_has_ofd_lock();
>>> +        if (!strcmp(filename, "/dev/null") ||
>>> +                   !strcmp(filename, "/dev/zero")) {
>>
>> I’m not sure I like a strcmp() based on filename (though it should work
>> for all of the cases where someone would want to specify either of those
>> for a qemu block device).  Isn’t there some specific major/minor number
>> we can use to check for these two files?  Or are those Linux-specific?
> 
> Yeah, I guess major/minor numbers are Linux-specific.
> 
>>
>> I could also imagine just not locking any host_device, but since people
>> do use qcow2 immediately on block devices, maybe we do want to lock them.
> 
> That's right.
> 
>>
>> Finally, if really all you are trying to do is to make test code easier,
>> then I don’t know exactly why.  Just disabling locking in 226 shouldn’t
>> be too hard.
> 
> The tricky thing is in remembering to do that for future test cases.
> My machine seems to be somehow different than John's so that my 226
> cannot lock /dev/null, but I'm not sure that is the case for other
> releases, distros or system instances.

Usually we don’t need to use /dev/null, though, because null-co:// is
better suited for most purposes.  This is a very specific test of host
devices.

Maybe we should start a doc file with common good practices about
writing iotests?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
  2018-07-22 14:06     ` Max Reitz
@ 2018-07-23  1:56       ` Fam Zheng
  2018-07-23 14:37         ` Max Reitz
  0 siblings, 1 reply; 13+ messages in thread
From: Fam Zheng @ 2018-07-23  1:56 UTC (permalink / raw)
  To: Max Reitz; +Cc: QEMU Developers, Kevin Wolf, qemu block

On Sun, Jul 22, 2018 at 10:06 PM Max Reitz <mreitz@redhat.com> wrote:
>
> On 2018-07-22 04:37, Fam Zheng wrote:
> > On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote:
> >>
> >> On 2018-07-19 05:41, Fam Zheng wrote:
> >>> On my Fedora 28, /dev/null is locked by some other process (couldn't
> >>> inspect it due to the current lslocks limitation), so iotests 226 fails
> >>> with some unexpected image locking errors because it uses qemu-io to
> >>> open it.
> >>>
> >>> Actually it's safe to not use any lock on /dev/null or /dev/zero.
> >>>
> >>> Signed-off-by: Fam Zheng <famz@redhat.com>
> >>> ---
> >>>  block/file-posix.c | 7 ++++++-
> >>>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/block/file-posix.c b/block/file-posix.c
> >>> index 60af4b3d51..8bf034108a 100644
> >>> --- a/block/file-posix.c
> >>> +++ b/block/file-posix.c
> >>> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> >>>          s->use_lock = false;
> >>>          break;
> >>>      case ON_OFF_AUTO_AUTO:
> >>> -        s->use_lock = qemu_has_ofd_lock();
> >>> +        if (!strcmp(filename, "/dev/null") ||
> >>> +                   !strcmp(filename, "/dev/zero")) {
> >>
> >> I’m not sure I like a strcmp() based on filename (though it should work
> >> for all of the cases where someone would want to specify either of those
> >> for a qemu block device).  Isn’t there some specific major/minor number
> >> we can use to check for these two files?  Or are those Linux-specific?
> >
> > Yeah, I guess major/minor numbers are Linux-specific.
> >
> >>
> >> I could also imagine just not locking any host_device, but since people
> >> do use qcow2 immediately on block devices, maybe we do want to lock them.
> >
> > That's right.
> >
> >>
> >> Finally, if really all you are trying to do is to make test code easier,
> >> then I don’t know exactly why.  Just disabling locking in 226 shouldn’t
> >> be too hard.
> >
> > The tricky thing is in remembering to do that for future test cases.
> > My machine seems to be somehow different than John's so that my 226
> > cannot lock /dev/null, but I'm not sure that is the case for other
> > releases, distros or system instances.
>
> Usually we don’t need to use /dev/null, though, because null-co:// is
> better suited for most purposes.  This is a very specific test of host
> devices.
>
> Maybe we should start a doc file with common good practices about
> writing iotests?

Yes, mentioning using pseudo devices in docs/devel/testing.rst would
probably be a good idea. So is my understanding right that you prefer
fixing the test case and discard this patch? If so I'll send another
version together with the doc update.

Fam

>
> Max
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
  2018-07-23  1:56       ` Fam Zheng
@ 2018-07-23 14:37         ` Max Reitz
  2018-07-24  1:17           ` Fam Zheng
  0 siblings, 1 reply; 13+ messages in thread
From: Max Reitz @ 2018-07-23 14:37 UTC (permalink / raw)
  To: Fam Zheng; +Cc: QEMU Developers, Kevin Wolf, qemu block

[-- Attachment #1: Type: text/plain, Size: 3137 bytes --]

On 2018-07-23 03:56, Fam Zheng wrote:
> On Sun, Jul 22, 2018 at 10:06 PM Max Reitz <mreitz@redhat.com> wrote:
>>
>> On 2018-07-22 04:37, Fam Zheng wrote:
>>> On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote:
>>>>
>>>> On 2018-07-19 05:41, Fam Zheng wrote:
>>>>> On my Fedora 28, /dev/null is locked by some other process (couldn't
>>>>> inspect it due to the current lslocks limitation), so iotests 226 fails
>>>>> with some unexpected image locking errors because it uses qemu-io to
>>>>> open it.
>>>>>
>>>>> Actually it's safe to not use any lock on /dev/null or /dev/zero.
>>>>>
>>>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>>>> ---
>>>>>  block/file-posix.c | 7 ++++++-
>>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>>>> index 60af4b3d51..8bf034108a 100644
>>>>> --- a/block/file-posix.c
>>>>> +++ b/block/file-posix.c
>>>>> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>>>>>          s->use_lock = false;
>>>>>          break;
>>>>>      case ON_OFF_AUTO_AUTO:
>>>>> -        s->use_lock = qemu_has_ofd_lock();
>>>>> +        if (!strcmp(filename, "/dev/null") ||
>>>>> +                   !strcmp(filename, "/dev/zero")) {
>>>>
>>>> I’m not sure I like a strcmp() based on filename (though it should work
>>>> for all of the cases where someone would want to specify either of those
>>>> for a qemu block device).  Isn’t there some specific major/minor number
>>>> we can use to check for these two files?  Or are those Linux-specific?
>>>
>>> Yeah, I guess major/minor numbers are Linux-specific.
>>>
>>>>
>>>> I could also imagine just not locking any host_device, but since people
>>>> do use qcow2 immediately on block devices, maybe we do want to lock them.
>>>
>>> That's right.
>>>
>>>>
>>>> Finally, if really all you are trying to do is to make test code easier,
>>>> then I don’t know exactly why.  Just disabling locking in 226 shouldn’t
>>>> be too hard.
>>>
>>> The tricky thing is in remembering to do that for future test cases.
>>> My machine seems to be somehow different than John's so that my 226
>>> cannot lock /dev/null, but I'm not sure that is the case for other
>>> releases, distros or system instances.
>>
>> Usually we don’t need to use /dev/null, though, because null-co:// is
>> better suited for most purposes.  This is a very specific test of host
>> devices.
>>
>> Maybe we should start a doc file with common good practices about
>> writing iotests?
> 
> Yes, mentioning using pseudo devices in docs/devel/testing.rst would
> probably be a good idea. So is my understanding right that you prefer
> fixing the test case and discard this patch? If so I'll send another
> version together with the doc update.

I personally would prefer fixing the test and not modifying the code,
yes.  But I'm aware that it is a personal opinion.

I think another alternative would be to not lock character devices, as I
don't suppose anyone is going to use them for anything serious.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] block: Don't lock /dev/null and /dev/zero automatically
  2018-07-23 14:37         ` Max Reitz
@ 2018-07-24  1:17           ` Fam Zheng
  0 siblings, 0 replies; 13+ messages in thread
From: Fam Zheng @ 2018-07-24  1:17 UTC (permalink / raw)
  To: Max Reitz; +Cc: QEMU Developers, Kevin Wolf, qemu block

On Mon, Jul 23, 2018 at 10:37 PM Max Reitz <mreitz@redhat.com> wrote:
>
> On 2018-07-23 03:56, Fam Zheng wrote:
> > On Sun, Jul 22, 2018 at 10:06 PM Max Reitz <mreitz@redhat.com> wrote:
> >>
> >> On 2018-07-22 04:37, Fam Zheng wrote:
> >>> On Sun, Jul 22, 2018 at 5:08 AM Max Reitz <mreitz@redhat.com> wrote:
> >>>>
> >>>> On 2018-07-19 05:41, Fam Zheng wrote:
> >>>>> On my Fedora 28, /dev/null is locked by some other process (couldn't
> >>>>> inspect it due to the current lslocks limitation), so iotests 226 fails
> >>>>> with some unexpected image locking errors because it uses qemu-io to
> >>>>> open it.
> >>>>>
> >>>>> Actually it's safe to not use any lock on /dev/null or /dev/zero.
> >>>>>
> >>>>> Signed-off-by: Fam Zheng <famz@redhat.com>
> >>>>> ---
> >>>>>  block/file-posix.c | 7 ++++++-
> >>>>>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/block/file-posix.c b/block/file-posix.c
> >>>>> index 60af4b3d51..8bf034108a 100644
> >>>>> --- a/block/file-posix.c
> >>>>> +++ b/block/file-posix.c
> >>>>> @@ -503,7 +503,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> >>>>>          s->use_lock = false;
> >>>>>          break;
> >>>>>      case ON_OFF_AUTO_AUTO:
> >>>>> -        s->use_lock = qemu_has_ofd_lock();
> >>>>> +        if (!strcmp(filename, "/dev/null") ||
> >>>>> +                   !strcmp(filename, "/dev/zero")) {
> >>>>
> >>>> I’m not sure I like a strcmp() based on filename (though it should work
> >>>> for all of the cases where someone would want to specify either of those
> >>>> for a qemu block device).  Isn’t there some specific major/minor number
> >>>> we can use to check for these two files?  Or are those Linux-specific?
> >>>
> >>> Yeah, I guess major/minor numbers are Linux-specific.
> >>>
> >>>>
> >>>> I could also imagine just not locking any host_device, but since people
> >>>> do use qcow2 immediately on block devices, maybe we do want to lock them.
> >>>
> >>> That's right.
> >>>
> >>>>
> >>>> Finally, if really all you are trying to do is to make test code easier,
> >>>> then I don’t know exactly why.  Just disabling locking in 226 shouldn’t
> >>>> be too hard.
> >>>
> >>> The tricky thing is in remembering to do that for future test cases.
> >>> My machine seems to be somehow different than John's so that my 226
> >>> cannot lock /dev/null, but I'm not sure that is the case for other
> >>> releases, distros or system instances.
> >>
> >> Usually we don’t need to use /dev/null, though, because null-co:// is
> >> better suited for most purposes.  This is a very specific test of host
> >> devices.
> >>
> >> Maybe we should start a doc file with common good practices about
> >> writing iotests?
> >
> > Yes, mentioning using pseudo devices in docs/devel/testing.rst would
> > probably be a good idea. So is my understanding right that you prefer
> > fixing the test case and discard this patch? If so I'll send another
> > version together with the doc update.
>
> I personally would prefer fixing the test and not modifying the code,
> yes.  But I'm aware that it is a personal opinion.

Sure, and you are the maintainer, so why not follow what you think. :)

Fam

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2018-07-24  1:18 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-19  3:41 [Qemu-devel] [PATCH] block: Don't lock /dev/null and /dev/zero automatically Fam Zheng
2018-07-19 17:57 ` [Qemu-devel] [Qemu-block] " John Snow
2018-07-20  8:24   ` Fam Zheng
2018-07-20 16:35     ` Eric Blake
2018-07-21  6:26       ` Fam Zheng
2018-07-20 18:56     ` John Snow
2018-07-20 16:34   ` Eric Blake
2018-07-21 21:08 ` [Qemu-devel] " Max Reitz
2018-07-22  2:37   ` Fam Zheng
2018-07-22 14:06     ` Max Reitz
2018-07-23  1:56       ` Fam Zheng
2018-07-23 14:37         ` Max Reitz
2018-07-24  1:17           ` Fam Zheng

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.