diff mbox

[for-2.10] qemu-options: Document the -drive locking parameter.

Message ID 20170906085006.26983-1-rjones@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Richard W.M. Jones Sept. 6, 2017, 8:50 a.m. UTC
Commit 16b48d5d66d2 ("file-posix: Add 'locking' option") added this
option, but as it was not documented in the -help output it was not
easily possible to tell if a particular qemu binary supports it.

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 qemu-options.hx | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Daniel P. Berrangé Sept. 6, 2017, 8:59 a.m. UTC | #1
The 2.10 versio nis already released. Did you mean that you wanted
this in the stable branch ?  If so, then CC qemu-stable@nongnu.org

On Wed, Sep 06, 2017 at 09:50:06AM +0100, Richard W.M. Jones wrote:
> Commit 16b48d5d66d2 ("file-posix: Add 'locking' option") added this
> option, but as it was not documented in the -help output it was not
> easily possible to tell if a particular qemu binary supports it.

NB, nothing should be parsing -help output to look for features
anymore. Is there really no other way to detect this feature ?

> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  qemu-options.hx | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9f6e2adfff..f8f95eb498 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -796,7 +796,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
>      "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
>      "       [,serial=s][,addr=A][,rerror=ignore|stop|report]\n"
>      "       [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
> -    "       [,readonly=on|off][,copy-on-read=on|off]\n"
> +    "       [,readonly=on|off][,copy-on-read=on|off][,locking=off|auto|on]\n"
>      "       [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
>      "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
>      "       [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n"

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Daniel
Richard W.M. Jones Sept. 6, 2017, 9:04 a.m. UTC | #2
On Wed, Sep 06, 2017 at 09:59:02AM +0100, Daniel P. Berrange wrote:
> The 2.10 versio nis already released. Did you mean that you wanted
> this in the stable branch ?  If so, then CC qemu-stable@nongnu.org
> 
> On Wed, Sep 06, 2017 at 09:50:06AM +0100, Richard W.M. Jones wrote:
> > Commit 16b48d5d66d2 ("file-posix: Add 'locking' option") added this
> > option, but as it was not documented in the -help output it was not
> > easily possible to tell if a particular qemu binary supports it.
> 
> NB, nothing should be parsing -help output to look for features
> anymore. Is there really no other way to detect this feature ?

It can be found from monitor output, but parsing -help output really
is easier in some cases.  In any case it's good to document it for end users.

Rich.

> > Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> > ---
> >  qemu-options.hx | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 9f6e2adfff..f8f95eb498 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -796,7 +796,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
> >      "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
> >      "       [,serial=s][,addr=A][,rerror=ignore|stop|report]\n"
> >      "       [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
> > -    "       [,readonly=on|off][,copy-on-read=on|off]\n"
> > +    "       [,readonly=on|off][,copy-on-read=on|off][,locking=off|auto|on]\n"
> >      "       [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
> >      "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
> >      "       [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n"
> 
> Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Richard W.M. Jones Sept. 6, 2017, 9:31 a.m. UTC | #3
On Wed, Sep 06, 2017 at 09:50:06AM +0100, Richard W.M. Jones wrote:
> Commit 16b48d5d66d2 ("file-posix: Add 'locking' option") added this
> option, but as it was not documented in the -help output it was not
> easily possible to tell if a particular qemu binary supports it.

My apologies, I think this patch is wrong.  I'm going to rethink it.

Rich.
Kevin Wolf Sept. 6, 2017, 10:19 a.m. UTC | #4
Am 06.09.2017 um 10:50 hat Richard W.M. Jones geschrieben:
> Commit 16b48d5d66d2 ("file-posix: Add 'locking' option") added this
> option, but as it was not documented in the -help output it was not
> easily possible to tell if a particular qemu binary supports it.
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  qemu-options.hx | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 9f6e2adfff..f8f95eb498 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -796,7 +796,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
>      "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
>      "       [,serial=s][,addr=A][,rerror=ignore|stop|report]\n"
>      "       [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
> -    "       [,readonly=on|off][,copy-on-read=on|off]\n"
> +    "       [,readonly=on|off][,copy-on-read=on|off][,locking=off|auto|on]\n"
>      "       [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
>      "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
>      "       [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n"

'locking' is a driver-specific option and not universally available for
all images, so it shouldn't be included here.

Of course, you're right that driver-specific options should be
documented somewhere, and currently that's only in the QAPI schema for
blockdev-add, which isn't quite satisfying. Maybe adding them to the
'qemu-block-drivers' man page could work (which currently only contains
'qemu-img create' options).

Kevin
Richard W.M. Jones Sept. 6, 2017, 10:44 a.m. UTC | #5
On Wed, Sep 06, 2017 at 12:19:05PM +0200, Kevin Wolf wrote:
> Am 06.09.2017 um 10:50 hat Richard W.M. Jones geschrieben:
> > Commit 16b48d5d66d2 ("file-posix: Add 'locking' option") added this
> > option, but as it was not documented in the -help output it was not
> > easily possible to tell if a particular qemu binary supports it.
> > 
> > Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> > ---
> >  qemu-options.hx | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 9f6e2adfff..f8f95eb498 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -796,7 +796,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
> >      "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
> >      "       [,serial=s][,addr=A][,rerror=ignore|stop|report]\n"
> >      "       [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
> > -    "       [,readonly=on|off][,copy-on-read=on|off]\n"
> > +    "       [,readonly=on|off][,copy-on-read=on|off][,locking=off|auto|on]\n"
> >      "       [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
> >      "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
> >      "       [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n"
> 
> 'locking' is a driver-specific option and not universally available for
> all images, so it shouldn't be included here.

Indeed this patch is wrong, please ignore it.

However I couldn't work out the incantation to disable locking for a
qcow2 overlay backed by a file which is locked by another qemu
process:

  ...
      -drive file=/home/rjones/d/libguestfs/tmp/libguestfsSOXEiU/overlay1,cache=unsafe,format=qcow2,file.locking=off,id=hd0,if=none \
      -device scsi-hd,drive=hd0 \
  ...
  qemu-system-x86_64: -device scsi-hd,drive=hd0: Failed to get shared "write" lock
  Is another process using the image?
  ...

I'm guessing I need another level of indirection to get to the backing
file, but file.file.locking=off did not work either.

I think the error message there is wrong as well since it doesn't
refer to the right command line option nor tell you which file is
locked.

Rich.
Kevin Wolf Sept. 6, 2017, 11:38 a.m. UTC | #6
Am 06.09.2017 um 12:44 hat Richard W.M. Jones geschrieben:
> On Wed, Sep 06, 2017 at 12:19:05PM +0200, Kevin Wolf wrote:
> > Am 06.09.2017 um 10:50 hat Richard W.M. Jones geschrieben:
> > > Commit 16b48d5d66d2 ("file-posix: Add 'locking' option") added this
> > > option, but as it was not documented in the -help output it was not
> > > easily possible to tell if a particular qemu binary supports it.
> > > 
> > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> > > ---
> > >  qemu-options.hx | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/qemu-options.hx b/qemu-options.hx
> > > index 9f6e2adfff..f8f95eb498 100644
> > > --- a/qemu-options.hx
> > > +++ b/qemu-options.hx
> > > @@ -796,7 +796,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
> > >      "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
> > >      "       [,serial=s][,addr=A][,rerror=ignore|stop|report]\n"
> > >      "       [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
> > > -    "       [,readonly=on|off][,copy-on-read=on|off]\n"
> > > +    "       [,readonly=on|off][,copy-on-read=on|off][,locking=off|auto|on]\n"
> > >      "       [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
> > >      "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
> > >      "       [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n"
> > 
> > 'locking' is a driver-specific option and not universally available for
> > all images, so it shouldn't be included here.
> 
> Indeed this patch is wrong, please ignore it.
> 
> However I couldn't work out the incantation to disable locking for a
> qcow2 overlay backed by a file which is locked by another qemu
> process:
> 
>   ...
>       -drive file=/home/rjones/d/libguestfs/tmp/libguestfsSOXEiU/overlay1,cache=unsafe,format=qcow2,file.locking=off,id=hd0,if=none \
>       -device scsi-hd,drive=hd0 \
>   ...
>   qemu-system-x86_64: -device scsi-hd,drive=hd0: Failed to get shared "write" lock
>   Is another process using the image?
>   ...

This command line fragment looks correct to me. For me, it seems to
work. I'm starting a first qemu in the background with default locking
options:

    $ x86_64-softmmu/qemu-system-x86_64 -hda /tmp/test.qcow2

And then starting a second one with a command line resembling yours:

    $ x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi \
      -drive file=/tmp/test.qcow2,cache=unsafe,format=qcow2,file.locking=off,id=hd0,if=none \
      -device scsi-hd,drive=hd0

This works for me. If I set file.locking=auto, it fails as expected, so
it's not that the locking isn't working for me at all.

> I'm guessing I need another level of indirection to get to the backing
> file, but file.file.locking=off did not work either.

No, that would be too much. Without a prefix you configure the qcow2
layer, with the 'file.' prefix you configure the file-posix one. There
is no further nested node.

> I think the error message there is wrong as well since it doesn't
> refer to the right command line option nor tell you which file is
> locked.

This is interesting, I do get -drive in my error message with
locking=auto.

I could see how your message makes sense with -blockdev because I think
then the image file is really only locked as soon as it is used (and
only with the permissions that that user requires), so -device would in
fact be where it happens and the error message would reflect that. But
with -drive, we already request some permissions while opening the
image, so I would definitely expect -drive in your error message.

Kevin
Richard W.M. Jones Sept. 12, 2017, 9:45 a.m. UTC | #7
On Wed, Sep 06, 2017 at 01:38:45PM +0200, Kevin Wolf wrote:
> This command line fragment looks correct to me. For me, it seems to
> work. I'm starting a first qemu in the background with default locking
> options:
> 
>     $ x86_64-softmmu/qemu-system-x86_64 -hda /tmp/test.qcow2
> 
> And then starting a second one with a command line resembling yours:
> 
>     $ x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi \
>       -drive file=/tmp/test.qcow2,cache=unsafe,format=qcow2,file.locking=off,id=hd0,if=none \
>       -device scsi-hd,drive=hd0

The problem is with overlays, where file.locking doesn't propagate to
the backing file.  Thus:

  $ qemu-system-x86_64 -drive file=backing,format=raw

while in another terminal:

  $ qemu-img create -b backing -f qcow2 overlay
  $ qemu-system-x86_64 -drive file=overlay,format=qcow2,file.locking=off
  qemu-system-x86_64: Failed to get shared "write" lock
  Is another process using the image?

After some experimentation, I came up with this command which works:

  $ qemu-system-x86_64 -drive file.file.filename=overlay,file.driver=qcow2,file.backing.file.locking=off

Rich.
Kevin Wolf Sept. 12, 2017, 11:32 a.m. UTC | #8
Am 12.09.2017 um 11:45 hat Richard W.M. Jones geschrieben:
> On Wed, Sep 06, 2017 at 01:38:45PM +0200, Kevin Wolf wrote:
> > This command line fragment looks correct to me. For me, it seems to
> > work. I'm starting a first qemu in the background with default locking
> > options:
> > 
> >     $ x86_64-softmmu/qemu-system-x86_64 -hda /tmp/test.qcow2
> > 
> > And then starting a second one with a command line resembling yours:
> > 
> >     $ x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi \
> >       -drive file=/tmp/test.qcow2,cache=unsafe,format=qcow2,file.locking=off,id=hd0,if=none \
> >       -device scsi-hd,drive=hd0
> 
> The problem is with overlays, where file.locking doesn't propagate to
> the backing file.  Thus:
> 
>   $ qemu-system-x86_64 -drive file=backing,format=raw
> 
> while in another terminal:
> 
>   $ qemu-img create -b backing -f qcow2 overlay
>   $ qemu-system-x86_64 -drive file=overlay,format=qcow2,file.locking=off
>   qemu-system-x86_64: Failed to get shared "write" lock
>   Is another process using the image?

locking=off isn't the right tool for the case. Try this:

$ qemu-system-x86_64 -drive file=overlay,if=none -device virtio-blk-pci,drive=none0,share-rw=on

Unless you're doing really evil things, just telling qemu that your
guest can cope with concurrent writers to the same image is enough. This
propagates through the whole chain as appropriate.

Kevin
Richard W.M. Jones Sept. 12, 2017, 11:43 a.m. UTC | #9
On Tue, Sep 12, 2017 at 01:32:05PM +0200, Kevin Wolf wrote:
> Am 12.09.2017 um 11:45 hat Richard W.M. Jones geschrieben:
> > On Wed, Sep 06, 2017 at 01:38:45PM +0200, Kevin Wolf wrote:
> > > This command line fragment looks correct to me. For me, it seems to
> > > work. I'm starting a first qemu in the background with default locking
> > > options:
> > > 
> > >     $ x86_64-softmmu/qemu-system-x86_64 -hda /tmp/test.qcow2
> > > 
> > > And then starting a second one with a command line resembling yours:
> > > 
> > >     $ x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi \
> > >       -drive file=/tmp/test.qcow2,cache=unsafe,format=qcow2,file.locking=off,id=hd0,if=none \
> > >       -device scsi-hd,drive=hd0
> > 
> > The problem is with overlays, where file.locking doesn't propagate to
> > the backing file.  Thus:
> > 
> >   $ qemu-system-x86_64 -drive file=backing,format=raw
> > 
> > while in another terminal:
> > 
> >   $ qemu-img create -b backing -f qcow2 overlay
> >   $ qemu-system-x86_64 -drive file=overlay,format=qcow2,file.locking=off
> >   qemu-system-x86_64: Failed to get shared "write" lock
> >   Is another process using the image?
> 
> locking=off isn't the right tool for the case. Try this:
> 
> $ qemu-system-x86_64 -drive file=overlay,if=none -device virtio-blk-pci,drive=none0,share-rw=on
> 
> Unless you're doing really evil things, just telling qemu that your
> guest can cope with concurrent writers to the same image is enough. This
> propagates through the whole chain as appropriate.

Our guest certainly *cannot* cope with multiple writers to the backing
disk (file "raw" in my example).  In fact that would be a disaster.

The overlay protects the backing disk from ever seeing any writes.

In our case because the initial qemu instance (which we don't control)
opened the disk ("raw") with an exclusive lock, our only choice for
monitoring that disk is to turn off locking.

Rich.
Kevin Wolf Sept. 12, 2017, 12:20 p.m. UTC | #10
Am 12.09.2017 um 13:43 hat Richard W.M. Jones geschrieben:
> On Tue, Sep 12, 2017 at 01:32:05PM +0200, Kevin Wolf wrote:
> > Am 12.09.2017 um 11:45 hat Richard W.M. Jones geschrieben:
> > > On Wed, Sep 06, 2017 at 01:38:45PM +0200, Kevin Wolf wrote:
> > > > This command line fragment looks correct to me. For me, it seems to
> > > > work. I'm starting a first qemu in the background with default locking
> > > > options:
> > > > 
> > > >     $ x86_64-softmmu/qemu-system-x86_64 -hda /tmp/test.qcow2
> > > > 
> > > > And then starting a second one with a command line resembling yours:
> > > > 
> > > >     $ x86_64-softmmu/qemu-system-x86_64 -device virtio-scsi \
> > > >       -drive file=/tmp/test.qcow2,cache=unsafe,format=qcow2,file.locking=off,id=hd0,if=none \
> > > >       -device scsi-hd,drive=hd0
> > > 
> > > The problem is with overlays, where file.locking doesn't propagate to
> > > the backing file.  Thus:
> > > 
> > >   $ qemu-system-x86_64 -drive file=backing,format=raw
> > > 
> > > while in another terminal:
> > > 
> > >   $ qemu-img create -b backing -f qcow2 overlay
> > >   $ qemu-system-x86_64 -drive file=overlay,format=qcow2,file.locking=off
> > >   qemu-system-x86_64: Failed to get shared "write" lock
> > >   Is another process using the image?
> > 
> > locking=off isn't the right tool for the case. Try this:
> > 
> > $ qemu-system-x86_64 -drive file=overlay,if=none -device virtio-blk-pci,drive=none0,share-rw=on
> > 
> > Unless you're doing really evil things, just telling qemu that your
> > guest can cope with concurrent writers to the same image is enough. This
> > propagates through the whole chain as appropriate.
> 
> Our guest certainly *cannot* cope with multiple writers to the backing
> disk (file "raw" in my example).  In fact that would be a disaster.

Your guest (the libguestfs one with the overlay) can cope with multiple
writers to its disk. Or probably it can't, but you treat it as if it
could and insist that this is correct enough. Otherwise you wouldn't be
able to use a raw image that another VM writes to as its backing file.

> The overlay protects the backing disk from ever seeing any writes.

This is why the backing file is opened read-only and therefore
compatible with the initial qemu instance that requires exclusive write
access.

This is all correctly represented in the locking. You wouldn't be able
to directly use "raw" even with share-rw=on because the initial qemu
instance doesn't support shared write access. But it works for a backing
file.

> In our case because the initial qemu instance (which we don't control)
> opened the disk ("raw") with an exclusive lock, our only choice for
> monitoring that disk is to turn off locking.

No, you just need to make sure that the libguestfs instance doesn't
need write access to the image of an exclusive writer. Which you already
do.

The only locking problem that you need to solve is that your libguestfs
VM doesn't forbid other writers to its backing file. And this is exactly
what share-rw=on achieves.

Kevin
diff mbox

Patch

diff --git a/qemu-options.hx b/qemu-options.hx
index 9f6e2adfff..f8f95eb498 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -796,7 +796,7 @@  DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
     "       [,serial=s][,addr=A][,rerror=ignore|stop|report]\n"
     "       [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
-    "       [,readonly=on|off][,copy-on-read=on|off]\n"
+    "       [,readonly=on|off][,copy-on-read=on|off][,locking=off|auto|on]\n"
     "       [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
     "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
     "       [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n"