diff mbox

Disable image locking for snapshot drive?

Message ID 20170720065324.GH11032@lemon
State New
Headers show

Commit Message

Fam Zheng July 20, 2017, 6:53 a.m. UTC 
On Tue, 07/18 16:19, Andrew Baumann wrote:
> > From: Eric Blake [mailto:eblake@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.

---

Comments

Cameron Esfahani via July 20, 2017, 9:49 p.m. UTC | #1 
> From: Fam Zheng [mailto:famz@redhat.com]
> Sent: Wednesday, 19 July 2017 23:53
> 
> On Tue, 07/18 16:19, Andrew Baumann wrote:
> > > From: Eric Blake [mailto:eblake@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);
Fam Zheng July 21, 2017, 1:46 a.m. UTC | #2 
On Thu, 07/20 21:49, Andrew Baumann via Qemu-devel wrote:
> > From: Fam Zheng [mailto:famz@redhat.com]
> > Sent: Wednesday, 19 July 2017 23:53
> > 
> > On Tue, 07/18 16:19, Andrew Baumann wrote:
> > > > From: Eric Blake [mailto:eblake@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);
>
diff mbox

Patch

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);