| Message ID | 20170906085006.26983-1-rjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 :|
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.
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
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.
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
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.
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
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.
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 --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"
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(-)