Re: [Qemu-devel] Disable image locking for snapshot drive?
On Thu, 07/20 21:49, Andrew Baumann via Qemu-devel wrote:
> > From: Fam Zheng [mailto:f...@redhat.com]
> > Sent: Wednesday, 19 July 2017 23:53
> >
> > On Tue, 07/18 16:19, Andrew Baumann wrote:
> > > > From: Eric Blake [mailto:ebl...@redhat.com]
> > > > Sent: Tuesday, 18 July 2017 8:07
> > > > On 07/17/2017 07:33 PM, John Snow wrote:
> > > > > On 07/17/2017 07:30 PM, Andrew Baumann via Qemu-devel wrote:
> > > > >> I'm running a recent Linux build of qemu on Windows Subsystem for
> > Linux
> > > > (WSL) which doesn't appear to implement file locking:
> > > > >>
> > > > >> $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 -
> > device
> > > > virtio-blk-pci,drive=hd0
> > > > >> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to
> > > > unlock byte 100
> > > >
> > > > Does WSL implement fcntl(F_SETLK) but not fcntl(F_OFD_SETLK)?
> > >
> > > Yes, this appears to be the case (there's also one report that it's
> > > broken):
> > > https://github.com/Microsoft/BashOnWindows/issues/1927
> >
> > What does fcntl(F_OFD_SETLK) return? If it is -ENOTSUP, we can probably
> > detect
> > that and disable locking. Can you try the patch pasted in the end?
>
> It returns -EINVAL. Your patch works, if I change the error code. Maybe
> testing for either ENOTSUP or EINVAL, and only allowing the fallback if the
> user didn't force locking on, would be a reasonable compromise?
I'm less comfortable with treating -EINVAL as "not supported".
A better fallback simply disabling is probably F_SETLK. Anyway I think a message
to stderr should be printed like the "#ifndef F_OFD_SETLK" case.
I'll work on a formal patch which does that.
Thanks,
Fam
>
> Cheers,
> Andrew
>
> > ---
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index cfbb236f6f..0be5bbbd53 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -493,6 +493,12 @@ static int raw_open_common(BlockDriverState *bs,
> > QDict *options,
> > }
> > s->fd = fd;
> >
> > +if (s->use_lock) {
> > +int ret0 = qemu_unlock_fd(fd, 0, 0);
> > +if (ret0 == -ENOTSUP) {
> > +s->use_lock = false;
> > +}
> > +}
> > s->lock_fd = -1;
> > if (s->use_lock) {
> > fd = qemu_open(filename, s->open_flags);
>
Re: [Qemu-devel] Disable image locking for snapshot drive?
> From: Fam Zheng [mailto:f...@redhat.com]
> Sent: Wednesday, 19 July 2017 23:53
>
> On Tue, 07/18 16:19, Andrew Baumann wrote:
> > > From: Eric Blake [mailto:ebl...@redhat.com]
> > > Sent: Tuesday, 18 July 2017 8:07
> > > On 07/17/2017 07:33 PM, John Snow wrote:
> > > > On 07/17/2017 07:30 PM, Andrew Baumann via Qemu-devel wrote:
> > > >> I'm running a recent Linux build of qemu on Windows Subsystem for
> Linux
> > > (WSL) which doesn't appear to implement file locking:
> > > >>
> > > >> $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 -
> device
> > > virtio-blk-pci,drive=hd0
> > > >> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to
> > > unlock byte 100
> > >
> > > Does WSL implement fcntl(F_SETLK) but not fcntl(F_OFD_SETLK)?
> >
> > Yes, this appears to be the case (there's also one report that it's broken):
> > https://github.com/Microsoft/BashOnWindows/issues/1927
>
> What does fcntl(F_OFD_SETLK) return? If it is -ENOTSUP, we can probably
> detect
> that and disable locking. Can you try the patch pasted in the end?
It returns -EINVAL. Your patch works, if I change the error code. Maybe testing
for either ENOTSUP or EINVAL, and only allowing the fallback if the user didn't
force locking on, would be a reasonable compromise?
Cheers,
Andrew
> ---
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index cfbb236f6f..0be5bbbd53 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -493,6 +493,12 @@ static int raw_open_common(BlockDriverState *bs,
> QDict *options,
> }
> s->fd = fd;
>
> +if (s->use_lock) {
> +int ret0 = qemu_unlock_fd(fd, 0, 0);
> +if (ret0 == -ENOTSUP) {
> +s->use_lock = false;
> +}
> +}
> s->lock_fd = -1;
> if (s->use_lock) {
> fd = qemu_open(filename, s->open_flags);
Re: [Qemu-devel] Disable image locking for snapshot drive?
On Tue, 07/18 16:19, Andrew Baumann wrote:
> > From: Eric Blake [mailto:ebl...@redhat.com]
> > Sent: Tuesday, 18 July 2017 8:07
> > On 07/17/2017 07:33 PM, John Snow wrote:
> > > On 07/17/2017 07:30 PM, Andrew Baumann via Qemu-devel wrote:
> > >> I'm running a recent Linux build of qemu on Windows Subsystem for Linux
> > (WSL) which doesn't appear to implement file locking:
> > >>
> > >> $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 -device
> > virtio-blk-pci,drive=hd0
> > >> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to
> > unlock byte 100
> >
> > Does WSL implement fcntl(F_SETLK) but not fcntl(F_OFD_SETLK)?
>
> Yes, this appears to be the case (there's also one report that it's broken):
> https://github.com/Microsoft/BashOnWindows/issues/1927
What does fcntl(F_OFD_SETLK) return? If it is -ENOTSUP, we can probably detect
that and disable locking. Can you try the patch pasted in the end?
>
> > We
> > already have code in place for compiling when F_OFD_SETLK is not
> > supported (which makes lock=auto do nothing, and issues a warning that
> > F_SETLK locks may be ineffective when locks are explicitly requested),
> > do we need to just expand that code into a runtime test of whether
> > F_OFD_SETLK appears to be unsupported?
>
> That would be a nice fix, and it would avoid the need for yet another flag. On
> the other hand, WSL is aiming for ABI compatibility, so they should get around
> to implementing F_OFD_SETLK et al eventually.
>
> Even if this were fixed in QEMU or implemented in WSL, shouldn't there to be a
> way to turn snapshot file locking off on a per-drive basis?
A snapshot file is temporary and unlinked immediately, so applying a lock or not
doesn't matter that much to deserve an option for that.
---
diff --git a/block/file-posix.c b/block/file-posix.c
index cfbb236f6f..0be5bbbd53 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -493,6 +493,12 @@ static int raw_open_common(BlockDriverState *bs, QDict
*options,
}
s->fd = fd;
+if (s->use_lock) {
+int ret0 = qemu_unlock_fd(fd, 0, 0);
+if (ret0 == -ENOTSUP) {
+s->use_lock = false;
+}
+}
s->lock_fd = -1;
if (s->use_lock) {
fd = qemu_open(filename, s->open_flags);
Re: [Qemu-devel] Disable image locking for snapshot drive?
> From: Eric Blake [mailto:ebl...@redhat.com] > Sent: Tuesday, 18 July 2017 8:07 > On 07/17/2017 07:33 PM, John Snow wrote: > > On 07/17/2017 07:30 PM, Andrew Baumann via Qemu-devel wrote: > >> I'm running a recent Linux build of qemu on Windows Subsystem for Linux > (WSL) which doesn't appear to implement file locking: > >> > >> $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 -device > virtio-blk-pci,drive=hd0 > >> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to > unlock byte 100 > > Does WSL implement fcntl(F_SETLK) but not fcntl(F_OFD_SETLK)? Yes, this appears to be the case (there's also one report that it's broken): https://github.com/Microsoft/BashOnWindows/issues/1927 > We > already have code in place for compiling when F_OFD_SETLK is not > supported (which makes lock=auto do nothing, and issues a warning that > F_SETLK locks may be ineffective when locks are explicitly requested), > do we need to just expand that code into a runtime test of whether > F_OFD_SETLK appears to be unsupported? That would be a nice fix, and it would avoid the need for yet another flag. On the other hand, WSL is aiming for ABI compatibility, so they should get around to implementing F_OFD_SETLK et al eventually. Even if this were fixed in QEMU or implemented in WSL, shouldn't there to be a way to turn snapshot file locking off on a per-drive basis? Andrew
Re: [Qemu-devel] Disable image locking for snapshot drive?
On 07/17/2017 07:33 PM, John Snow wrote: > > > On 07/17/2017 07:30 PM, Andrew Baumann via Qemu-devel wrote: >> Hi all, >> >> I'm running a recent Linux build of qemu on Windows Subsystem for Linux >> (WSL) which doesn't appear to implement file locking: >> >> $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 -device >> virtio-blk-pci,drive=hd0 >> qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock >> byte 100 Does WSL implement fcntl(F_SETLK) but not fcntl(F_OFD_SETLK)? We already have code in place for compiling when F_OFD_SETLK is not supported (which makes lock=auto do nothing, and issues a warning that F_SETLK locks may be ineffective when locks are explicitly requested), do we need to just expand that code into a runtime test of whether F_OFD_SETLK appears to be unsupported? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] Disable image locking for snapshot drive?
> From: John Snow [mailto:js...@redhat.com] > Sent: Monday, 17 July 2017 17:34 > On 07/17/2017 07:30 PM, Andrew Baumann via Qemu-devel wrote: > > Hi all, > > > > I'm running a recent Linux build of qemu on Windows Subsystem for Linux > (WSL) which doesn't appear to implement file locking: > > > > $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 -device > virtio-blk-pci,drive=hd0 > > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to > unlock byte 100 > > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to > unlock byte 100 > > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to lock > byte 100 > > > > That's no big deal; I can switch it off: > > > > $ qemu-system-aarch64 ... -drive > file=test.vhdx,if=none,file.locking=off,id=hd0 ... > > (all good) > > > > But how can I do the same for a snapshot drive? > > > > $ qemu-system-aarch64 ... -drive > file=test.vhdx,if=none,file.locking=off,id=hd0 -snapshot ... > > qemu-system-aarch64: -drive > file=test.vhdx,if=none,file.locking=off,id=hd0: Failed to unlock byte 100 > > qemu-system-aarch64: -drive > file=test.vhdx,if=none,file.locking=off,id=hd0: Failed to unlock byte 100 > > qemu-system-aarch64: -drive > file=test.vhdx,if=none,file.locking=off,id=hd0: Could not create temporary > overlay '/var/tmp/vl.o83dxn': Failed to lock byte 100 > > > > (I also tried the snapshot=on drive option with similar results.) > > > > Thanks, > > Andrew > > > > Looks like the shorthand "-snapshot" doesn't let you specify any further > options, which is a bummer. > > You may need to do something a little more manual, and create your own > temporary overlay, and launch QEMU pointing to that overlay instead. Can you give me some more clues what this might look like? > That sounds like a bit of a hassle. > > Can we compile locking support out of QEMU instead for this platform? Or > is there a runtime option for disabling it globally? The compile target is just Linux, so at best it would need to be a runtime choice based on platform detection, and I doubt that is a good idea (WSL may well get around to implementing this syscall in the future). I agree a runtime command-line flag to disable it globally would be ideal, but from a quick look at the code it doesn't seem to exist at present. Thanks, Andrew
Re: [Qemu-devel] Disable image locking for snapshot drive?
On 07/17/2017 07:30 PM, Andrew Baumann via Qemu-devel wrote: > Hi all, > > I'm running a recent Linux build of qemu on Windows Subsystem for Linux (WSL) > which doesn't appear to implement file locking: > > $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 -device > virtio-blk-pci,drive=hd0 > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock > byte 100 > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock > byte 100 > qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to lock > byte 100 > > That's no big deal; I can switch it off: > > $ qemu-system-aarch64 ... -drive > file=test.vhdx,if=none,file.locking=off,id=hd0 ... > (all good) > > But how can I do the same for a snapshot drive? > > $ qemu-system-aarch64 ... -drive > file=test.vhdx,if=none,file.locking=off,id=hd0 -snapshot ... > qemu-system-aarch64: -drive file=test.vhdx,if=none,file.locking=off,id=hd0: > Failed to unlock byte 100 > qemu-system-aarch64: -drive file=test.vhdx,if=none,file.locking=off,id=hd0: > Failed to unlock byte 100 > qemu-system-aarch64: -drive file=test.vhdx,if=none,file.locking=off,id=hd0: > Could not create temporary overlay '/var/tmp/vl.o83dxn': Failed to lock byte > 100 > > (I also tried the snapshot=on drive option with similar results.) > > Thanks, > Andrew > Looks like the shorthand "-snapshot" doesn't let you specify any further options, which is a bummer. You may need to do something a little more manual, and create your own temporary overlay, and launch QEMU pointing to that overlay instead. That sounds like a bit of a hassle. Can we compile locking support out of QEMU instead for this platform? Or is there a runtime option for disabling it globally?
