Re: [Qemu-devel] [Libguestfs] [PATCH] tests: regressions: make test-big-heap use a temporary empty file

Re: [Qemu-devel] [Libguestfs] [PATCH] tests: regressions: make test-big-heap use a temporary empty file

[Eric Blake]

[adding qemu lists]

On 03/21/2018 07:51 AM, Richard W.M. Jones wrote:
> On Wed, Mar 21, 2018 at 01:44:17PM +0100, Pino Toscano wrote:
>> Newer versions of qemu use file locking for the images by default, and
>> apparently that does not work with /dev/null.  Since this test just
>> calls qemu-img to get the format of an empty image, create a temporary
>> one instead.
> 
> ACK, but feels like this is a bug in qemu-img to me.

You're right that file locking on a character device like /dev/null is 
not going to work as expected, but is it a case where fcntl() actually 
fails, or is it worse where the fcntl() claiming the locks "succeeds" 
but doesn't do what we want?  That is, what were the actual error 
messages you ran into?

Is it something where qemu should instead be checking fstat() results 
and only doing locking on regular files and block devices?  At any rate, 
I agree that qemu itself should behave nicer on attempts to use 
/dev/null as an image.

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

Re: [Qemu-devel] [Libguestfs] [PATCH] tests: regressions: make test-big-heap use a temporary empty file

[Pino Toscano]

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

On Wednesday, 21 March 2018 14:45:38 CET Eric Blake wrote:
> [adding qemu lists]
> 
> On 03/21/2018 07:51 AM, Richard W.M. Jones wrote:
> > On Wed, Mar 21, 2018 at 01:44:17PM +0100, Pino Toscano wrote:
> >> Newer versions of qemu use file locking for the images by default, and
> >> apparently that does not work with /dev/null.  Since this test just
> >> calls qemu-img to get the format of an empty image, create a temporary
> >> one instead.
> > 
> > ACK, but feels like this is a bug in qemu-img to me.
> 
> You're right that file locking on a character device like /dev/null is 
> not going to work as expected, but is it a case where fcntl() actually 
> fails, or is it worse where the fcntl() claiming the locks "succeeds" 
> but doesn't do what we want?  That is, what were the actual error 
> messages you ran into?

$ qemu-img --version
qemu-img version 2.10.1(qemu-2.10.1-2.fc27)
Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers
$ qemu-img info /dev/null 
qemu-img: Could not open '/dev/null': Failed to get "consistent read" lock
Is another process using the image?

-- 
Pino Toscano

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-block] [Libguestfs] [PATCH] tests: regressions: make test-big-heap use a temporary empty file

[Kevin Wolf]

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

Am 21.03.2018 um 14:48 hat Pino Toscano geschrieben:
> On Wednesday, 21 March 2018 14:45:38 CET Eric Blake wrote:
> > [adding qemu lists]
> > 
> > On 03/21/2018 07:51 AM, Richard W.M. Jones wrote:
> > > On Wed, Mar 21, 2018 at 01:44:17PM +0100, Pino Toscano wrote:
> > >> Newer versions of qemu use file locking for the images by default, and
> > >> apparently that does not work with /dev/null.  Since this test just
> > >> calls qemu-img to get the format of an empty image, create a temporary
> > >> one instead.
> > > 
> > > ACK, but feels like this is a bug in qemu-img to me.
> > 
> > You're right that file locking on a character device like /dev/null is 
> > not going to work as expected, but is it a case where fcntl() actually 
> > fails, or is it worse where the fcntl() claiming the locks "succeeds" 
> > but doesn't do what we want?  That is, what were the actual error 
> > messages you ran into?
> 
> $ qemu-img --version
> qemu-img version 2.10.1(qemu-2.10.1-2.fc27)
> Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers
> $ qemu-img info /dev/null 
> qemu-img: Could not open '/dev/null': Failed to get "consistent read" lock
> Is another process using the image?

Not sure where the difference is, but I can't reproduce this on
upstream, neither git master nor the v2.10.1 tag:

$ ./qemu-img --version
qemu-img version 2.10.1 (v2.10.1-dirty)
Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers
$ ./qemu-img info /dev/null
image: /dev/null
file format: raw
virtual size: 0 (0 bytes)
disk size: 0

Also with strace:

open("/dev/null", O_RDONLY|O_CLOEXEC)   = 10
fcntl(10, F_OFD_SETLK, {l_type=F_RDLCK, l_whence=SEEK_SET, l_start=100, l_len=1}) = 0
...

So my kernel (4.15.9-200.fc26.x86_64) seems to have no problems with
locking /dev/null.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [Qemu-block] [Libguestfs] [PATCH] tests: regressions: make test-big-heap use a temporary empty file

[Eric Blake]

On 03/21/2018 03:44 PM, Kevin Wolf wrote:
>>>
>>> You're right that file locking on a character device like /dev/null is
>>> not going to work as expected, but is it a case where fcntl() actually
>>> fails, or is it worse where the fcntl() claiming the locks "succeeds"
>>> but doesn't do what we want?  That is, what were the actual error
>>> messages you ran into?
>>
>> $ qemu-img --version
>> qemu-img version 2.10.1(qemu-2.10.1-2.fc27)
>> Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers
>> $ qemu-img info /dev/null
>> qemu-img: Could not open '/dev/null': Failed to get "consistent read" lock
>> Is another process using the image?
> 
> Not sure where the difference is, but I can't reproduce this on
> upstream, neither git master nor the v2.10.1 tag:

Is it a case where file locking actually works, and more than one 
process is trying to lock /dev/null at once?  (qemu-img info is 
short-lived, but could there be another longer-lived process also using 
/dev/null)?

Does using -r help (if the only reason you're telling qemu-img to 
operate on /dev/null is to probe qemu-img features, can you probe those 
same features without needing to write, which in turn requests less 
locking)?

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

Re: [Qemu-devel] [Qemu-block] [Libguestfs] [PATCH] tests: regressions: make test-big-heap use a temporary empty file

[Richard W.M. Jones]

On Wed, Mar 21, 2018 at 03:58:05PM -0500, Eric Blake wrote:
> On 03/21/2018 03:44 PM, Kevin Wolf wrote:
> >>>
> >>>You're right that file locking on a character device like /dev/null is
> >>>not going to work as expected, but is it a case where fcntl() actually
> >>>fails, or is it worse where the fcntl() claiming the locks "succeeds"
> >>>but doesn't do what we want?  That is, what were the actual error
> >>>messages you ran into?
> >>
> >>$ qemu-img --version
> >>qemu-img version 2.10.1(qemu-2.10.1-2.fc27)
> >>Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers
> >>$ qemu-img info /dev/null
> >>qemu-img: Could not open '/dev/null': Failed to get "consistent read" lock
> >>Is another process using the image?
> >
> >Not sure where the difference is, but I can't reproduce this on
> >upstream, neither git master nor the v2.10.1 tag:
> 
> Is it a case where file locking actually works, and more than one
> process is trying to lock /dev/null at once?  (qemu-img info is
> short-lived, but could there be another longer-lived process also
> using /dev/null)?
> 
> Does using -r help (if the only reason you're telling qemu-img to
> operate on /dev/null is to probe qemu-img features, can you probe
> those same features without needing to write, which in turn requests
> less locking)?

The original test (before Pino's patch) runs ‘qemu-img info /dev/null’
purely as a way to fork qemu-img.  It's a regression test for some
problem we had with the code that confines qemu-img using resource
limits.  So really nothing much to do with qemu-img at all.  The fix
is to create a temporary file and run qemu-img against that.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top