[Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

[Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

[Fam Zheng]

Too many troubles have been caused by two processes writing to the same image
unexpectedly. This series introduces automatical image locking into QEMU to
avoid such tragedy. With this, the user won't be able to open the image from
two processes (e.g. using qemu-img when the image is attached to the guest).

Underneath is the fcntl syscall that locks the local file, similar to what is
already used in libvirt virtlockd.  Also because of that, we cannot directly
apply fcntl lock on the image file itself, instead we open and lock
"/var/tmp/.qemu-$sha1.lock", where $sha1 is derived from the image's full path
as in realpath(3). This mechanism should be equally useful for the single host
case, and it doesn't conflict with virtlockd when managed by libvirt.

The alternative file locking API on Linux, flock(2), cannot protect host NFS
mount points, so it's not used.

Gluster locking is also implemented wrapping glfs_posix_lock in patch 6. It's
only lightly tested.

All other drivers that don't implement .bdrv_lockf are always permissive and
does no checking.

In the future, the intention is that image format drivers that introduce
locking mechanisms could also fit into this API.

The first 6 patches define the internal and external interfaces, and implement
the image locking.

Patch 7 adds an option in qemu-io to allow disabling the lock, for testing
purpose.

Patches 8 - 14 fixes the potential failures of test cases where multiple
processes may open the image concurrently.

Finally the default behavior is switched to on in patch 15.

Fam Zheng (15):
  block: Add BDRV_O_NO_LOCK
  qapi: Add lock-image in blockdev-add options
  blockdev: Add and parse "lock-image" option for block devices
  block: Introduce image file locking interface
  raw-posix: Implement .bdrv_lockf
  gluster: Implement .bdrv_lockf
  qemu-io: Add "-L" option for BDRV_O_NO_LOCK
  qemu-iotests: 140: Disable image lock for qemu-io access
  qemu-iotests: 046: Move version detection out from verify_io
  qemu-iotests: Fix lock-image for shared disk in test case 091
  qemu-iotests: Disable image lock when checking test image
  qemu-iotests: 051: Disable image lock in the command line
  ahci-test: Specify "lock-image=off" in CLI
  ide-test: Specify "lock-image=off" in command lines
  block: Turn on image locking by default

 block.c                       | 25 +++++++++++
 block/gluster.c               | 34 +++++++++++++++
 block/raw-posix.c             | 97 +++++++++++++++++++++++++++++++++++++++++++
 blockdev.c                    |  8 ++++
 include/block/block.h         |  9 ++++
 include/block/block_int.h     |  5 +++
 qapi/block-core.json          |  6 ++-
 qemu-io.c                     | 22 +++++++++-
 tests/ahci-test.c             | 16 +++++--
 tests/ide-test.c              |  5 ++-
 tests/qemu-iotests/030        |  2 +-
 tests/qemu-iotests/046        | 22 +++++-----
 tests/qemu-iotests/051        |  2 +-
 tests/qemu-iotests/051.out    | 10 ++---
 tests/qemu-iotests/051.pc.out | 10 ++---
 tests/qemu-iotests/091        |  4 +-
 tests/qemu-iotests/140        |  2 +-
 17 files changed, 245 insertions(+), 34 deletions(-)

-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 01/15] block: Add BDRV_O_NO_LOCK

[Fam Zheng]

Later the block layer will automatically lock the images to avoid unexpected
concurrent accesses to the same image, which will easily corrupt the metadata
or user data, unless in some very special cases, like migration.

The exceptional cases like shared storage migration and testing should set
BDRV_O_NO_LOCK the flag indicating that the block layer should skip the
automatic locking of the image, like the old behavior.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/block/block.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/block/block.h b/include/block/block.h
index 3a73137..b803597 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -94,6 +94,7 @@ typedef struct HDGeometry {
                                       select an appropriate protocol driver,
                                       ignoring the format layer */
 #define BDRV_O_NO_IO       0x10000 /* don't initialize for I/O */
+#define BDRV_O_NO_LOCK     0x20000 /* don't lock image file */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
 
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 02/15] qapi: Add lock-image in blockdev-add options

[Fam Zheng]

To allow overriding the default locking behavior when opening the image.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qapi/block-core.json | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1d09079..2913f3e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2065,6 +2065,9 @@
 # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
 #                 (default: off)
 #
+# @lock-image: #optional whether to lock the image (default: true)
+#              (Since 2.7)
+#
 # Remaining options are determined by the block driver.
 #
 # Since: 1.7
@@ -2082,7 +2085,8 @@
             '*stats-account-invalid': 'bool',
             '*stats-account-failed': 'bool',
             '*stats-intervals': ['int'],
-            '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
+            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
+            '*lock-image': 'bool' },
   'discriminator': 'driver',
   'data': {
       'archipelago':'BlockdevOptionsArchipelago',
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 03/15] blockdev: Add and parse "lock-image" option for block devices

[Fam Zheng]

Honor the locking switch specified in CLI or QMP, and set the open flags for
the image accordingly.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index f1f520a..93bd43e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -382,6 +382,10 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
                return;
             }
         }
+
+        if (!qemu_opt_get_bool(opts, "lock-image", false)) {
+            *bdrv_flags |= BDRV_O_NO_LOCK;
+        }
     }
 
     /* disk I/O throttling */
@@ -4249,6 +4253,10 @@ QemuOptsList qemu_common_drive_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "whether to account for failed I/O operations "
                     "in the statistics",
+        },{
+            .name = "lock-image",
+            .type = QEMU_OPT_BOOL,
+            .help = "whether to lock the image (default: on)",
         },
         { /* end of list */ }
     },
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 04/15] block: Introduce image file locking interface

[Fam Zheng]

Block drivers can implement this new operation .bdrv_lockf to actually lock the
image in the protocol specific way.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                   | 25 +++++++++++++++++++++++++
 include/block/block.h     |  8 ++++++++
 include/block/block_int.h |  5 +++++
 3 files changed, 38 insertions(+)

diff --git a/block.c b/block.c
index d4939b4..7f6e903 100644
--- a/block.c
+++ b/block.c
@@ -995,6 +995,15 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
         goto free_and_fail;
     }
 
+    if (!(open_flags & BDRV_O_NO_LOCK)) {
+        BdrvLockfCmd cmd = open_flags & BDRV_O_RDWR ? BDRV_LOCKF_RWLOCK :
+                                                      BDRV_LOCKF_ROLOCK;
+        ret = bdrv_lockf(bs, cmd, errp);
+        if (ret != 0 && ret != -ENOTSUP) {
+            goto free_and_fail;
+        }
+    }
+
     ret = refresh_total_sectors(bs, bs->total_sectors);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not refresh total sector count");
@@ -2141,6 +2150,10 @@ static void bdrv_close(BlockDriverState *bs)
         blk_dev_change_media_cb(bs->blk, false);
     }
 
+    if (bs->drv && !(bs->open_flags & BDRV_O_NO_LOCK)) {
+        bdrv_lockf(bs, BDRV_LOCKF_UNLOCK, &error_abort);
+    }
+
     if (bs->drv) {
         BdrvChild *child, *next;
 
@@ -3981,3 +3994,15 @@ void bdrv_refresh_filename(BlockDriverState *bs)
         QDECREF(json);
     }
 }
+
+int bdrv_lockf(BlockDriverState *bs, BdrvLockfCmd cmd, Error **errp)
+{
+    if (!bs->drv) {
+        error_setg(errp, "No medium to lock");
+        return -ENOMEDIUM;
+    } else if (!bs->drv->bdrv_lockf) {
+        return -ENOTSUP;
+    } else {
+        return bs->drv->bdrv_lockf(bs, cmd, errp);
+    }
+}
diff --git a/include/block/block.h b/include/block/block.h
index b803597..36e1271 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -542,4 +542,12 @@ void bdrv_drained_begin(BlockDriverState *bs);
  */
 void bdrv_drained_end(BlockDriverState *bs);
 
+typedef enum {
+    BDRV_LOCKF_RWLOCK,
+    BDRV_LOCKF_ROLOCK,
+    BDRV_LOCKF_UNLOCK,
+} BdrvLockfCmd;
+
+int bdrv_lockf(BlockDriverState *bs, BdrvLockfCmd cmd, Error **errp);
+
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 10d8759..0a79eba 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -317,6 +317,11 @@ struct BlockDriver {
      */
     void (*bdrv_drain)(BlockDriverState *bs);
 
+    /**
+     * Lock/unlock the image.
+     */
+    int (*bdrv_lockf)(BlockDriverState *bs, BdrvLockfCmd cmd, Error **errp);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 05/15] raw-posix: Implement .bdrv_lockf

[Fam Zheng]

Because virtlockd in libvirt already uses the fcntl lock on the image file, we
have to workaround this by locking a digest-mapped temporary file.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/raw-posix.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 906d5c9..277f20d 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -35,6 +35,7 @@
 #include "raw-aio.h"
 #include "qapi/util.h"
 #include "qapi/qmp/qstring.h"
+#include "glib.h"
 
 #if defined(__APPLE__) && (__MACH__)
 #include <paths.h>
@@ -149,6 +150,9 @@ typedef struct BDRVRawState {
     bool discard_zeroes:1;
     bool has_fallocate;
     bool needs_alignment;
+    bool image_locked;
+    int lock_file_fd;
+    char *lock_file_name;
 } BDRVRawState;
 
 typedef struct BDRVRawReopenState {
@@ -397,6 +401,87 @@ static void raw_attach_aio_context(BlockDriverState *bs,
 #endif
 }
 
+static int raw_do_lockf(int fd, BdrvLockfCmd cmd)
+{
+    int ret;
+    struct flock fl = (struct flock) {
+        .l_start = 0,
+        .l_whence  = SEEK_SET,
+        .l_len = 0,
+    };
+    switch (cmd) {
+    case BDRV_LOCKF_RWLOCK:
+        fl.l_type = F_WRLCK;
+        break;
+    case BDRV_LOCKF_ROLOCK:
+        fl.l_type = F_RDLCK;
+        break;
+    case BDRV_LOCKF_UNLOCK:
+        fl.l_type = F_UNLCK;
+        break;
+    default:
+        abort();
+    }
+    ret = fcntl(fd, F_SETLK, &fl);
+    return ret == -1 ? -errno : 0;
+}
+
+static char *raw_get_lock_file_name(BlockDriverState *bs)
+{
+    GChecksum *cksm;
+    const char *digest;
+    char *lock_file = NULL;
+    char *fullpath = g_malloc(PATH_MAX);
+
+    if (!realpath(bs->filename, fullpath)) {
+        pstrcpy(fullpath, PATH_MAX, bs->filename);
+    }
+    cksm = g_checksum_new(G_CHECKSUM_SHA1);
+    g_checksum_update(cksm, (const guchar *)fullpath, strlen(fullpath));
+    g_free(fullpath);
+    digest = g_checksum_get_string(cksm);
+    if (digest) {
+        lock_file = g_strdup_printf("/var/tmp/.qemu-%s.lock", digest);
+    }
+    g_checksum_free(cksm);
+    return lock_file;
+}
+
+static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd, Error **errp)
+{
+
+    BDRVRawState *s = bs->opaque;
+    int ret;
+
+    if ((cmd == BDRV_LOCKF_UNLOCK) == (!s->image_locked)) {
+        return 0;
+    }
+    if (s->lock_file_fd == -1) {
+        /* Create and open the lock file if necessary. */
+
+        assert(cmd != BDRV_LOCKF_UNLOCK);
+
+        s->lock_file_name = raw_get_lock_file_name(bs);
+
+        s->lock_file_fd = qemu_open(s->lock_file_name, O_RDWR | O_CREAT, 0644);
+        if (s->lock_file_fd == -1) {
+            error_setg_errno(errp, errno, "Failed to open file");
+            ret = -EIO;
+            goto out;
+        }
+    }
+
+    ret = raw_do_lockf(s->lock_file_fd, cmd);
+    if (ret) {
+        error_setg_errno(errp, errno,
+                         "Failed to lock file: %s", s->lock_file_name);
+    } else {
+        s->image_locked = cmd != BDRV_LOCKF_UNLOCK;
+    }
+out:
+    return ret;
+}
+
 #ifdef CONFIG_LINUX_AIO
 static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags)
 {
@@ -585,6 +670,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;
 
     s->type = FTYPE_FILE;
+    s->lock_file_fd = -1;
     ret = raw_open_common(bs, options, flags, 0, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -1393,6 +1479,15 @@ static void raw_close(BlockDriverState *bs)
 
     raw_detach_aio_context(bs);
 
+    if (s->lock_file_fd != -1) {
+        qemu_close(s->lock_file_fd);
+        s->lock_file_fd = -1;
+    }
+    if (s->lock_file_name) {
+        unlink(s->lock_file_name);
+        g_free(s->lock_file_name);
+        s->lock_file_name = NULL;
+   }
 #ifdef CONFIG_LINUX_AIO
     if (s->use_aio) {
         laio_cleanup(s->aio_ctx);
@@ -1960,6 +2055,8 @@ BlockDriver bdrv_file = {
     .bdrv_detach_aio_context = raw_detach_aio_context,
     .bdrv_attach_aio_context = raw_attach_aio_context,
 
+    .bdrv_lockf = raw_lockf,
+
     .create_opts = &raw_create_opts,
 };
 
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.7 05/15] raw-posix: Implement .bdrv_lockf

[Daniel P. Berrange]

On Wed, Apr 13, 2016 at 05:09:54PM +0800, Fam Zheng wrote:
> Because virtlockd in libvirt already uses the fcntl lock on the image file, we
> have to workaround this by locking a digest-mapped temporary file.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/raw-posix.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 906d5c9..277f20d 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -35,6 +35,7 @@
>  #include "raw-aio.h"
>  #include "qapi/util.h"
>  #include "qapi/qmp/qstring.h"
> +#include "glib.h"
>  
>  #if defined(__APPLE__) && (__MACH__)
>  #include <paths.h>
> @@ -149,6 +150,9 @@ typedef struct BDRVRawState {
>      bool discard_zeroes:1;
>      bool has_fallocate;
>      bool needs_alignment;
> +    bool image_locked;
> +    int lock_file_fd;
> +    char *lock_file_name;
>  } BDRVRawState;
>  
>  typedef struct BDRVRawReopenState {
> @@ -397,6 +401,87 @@ static void raw_attach_aio_context(BlockDriverState *bs,
>  #endif
>  }
>  
> +static int raw_do_lockf(int fd, BdrvLockfCmd cmd)
> +{
> +    int ret;
> +    struct flock fl = (struct flock) {
> +        .l_start = 0,
> +        .l_whence  = SEEK_SET,
> +        .l_len = 0,
> +    };

If you change this to

   .l_start = 1,
   .l_len = 1,

then you would be telling a selective lock at byte 1 which would
not interfere with anything virtlockd currently does, and also
leave the other bytes unlocked for future use by QEMU or libvirt
as needed.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH for-2.7 05/15] raw-posix: Implement .bdrv_lockf

[Fam Zheng]

On Wed, 04/13 10:21, Daniel P. Berrange wrote:
> On Wed, Apr 13, 2016 at 05:09:54PM +0800, Fam Zheng wrote:
> > Because virtlockd in libvirt already uses the fcntl lock on the image file, we
> > have to workaround this by locking a digest-mapped temporary file.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/raw-posix.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 97 insertions(+)
> > 
> > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > index 906d5c9..277f20d 100644
> > --- a/block/raw-posix.c
> > +++ b/block/raw-posix.c
> > @@ -35,6 +35,7 @@
> >  #include "raw-aio.h"
> >  #include "qapi/util.h"
> >  #include "qapi/qmp/qstring.h"
> > +#include "glib.h"
> >  
> >  #if defined(__APPLE__) && (__MACH__)
> >  #include <paths.h>
> > @@ -149,6 +150,9 @@ typedef struct BDRVRawState {
> >      bool discard_zeroes:1;
> >      bool has_fallocate;
> >      bool needs_alignment;
> > +    bool image_locked;
> > +    int lock_file_fd;
> > +    char *lock_file_name;
> >  } BDRVRawState;
> >  
> >  typedef struct BDRVRawReopenState {
> > @@ -397,6 +401,87 @@ static void raw_attach_aio_context(BlockDriverState *bs,
> >  #endif
> >  }
> >  
> > +static int raw_do_lockf(int fd, BdrvLockfCmd cmd)
> > +{
> > +    int ret;
> > +    struct flock fl = (struct flock) {
> > +        .l_start = 0,
> > +        .l_whence  = SEEK_SET,
> > +        .l_len = 0,
> > +    };
> 
> If you change this to
> 
>    .l_start = 1,
>    .l_len = 1,
> 
> then you would be telling a selective lock at byte 1 which would
> not interfere with anything virtlockd currently does, and also
> leave the other bytes unlocked for future use by QEMU or libvirt
> as needed.

That's brillent, thanks a lot!

Fam

[Qemu-devel] [PATCH for-2.7 06/15] gluster: Implement .bdrv_lockf

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/gluster.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/block/gluster.c b/block/gluster.c
index 51e154c..e76ec87 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -672,6 +672,36 @@ static void qemu_gluster_close(BlockDriverState *bs)
     glfs_fini(s->glfs);
 }
 
+static int qemu_gluster_lockf(BlockDriverState *bs, BdrvLockfCmd cmd,
+                              Error **errp)
+{
+    BDRVGlusterState *s = bs->opaque;
+    int ret;
+    struct flock fl = (struct flock) {
+        .l_start = 0,
+        .l_whence  = SEEK_SET,
+        .l_len = 0,
+    };
+    switch (cmd) {
+    case BDRV_LOCKF_RWLOCK:
+        fl.l_type = F_WRLCK;
+        break;
+    case BDRV_LOCKF_ROLOCK:
+        fl.l_type = F_RDLCK;
+        break;
+    case BDRV_LOCKF_UNLOCK:
+        fl.l_type = F_UNLCK;
+        break;
+    default:
+        abort();
+    }
+    ret = glfs_posix_lock(s->fd, F_SETLK, &fl);
+    if (ret == -1) {
+        error_setg_errno(errp, errno, "Failed to lock image");
+    }
+    return ret == -1 ? -errno : 0;
+}
+
 static int qemu_gluster_has_zero_init(BlockDriverState *bs)
 {
     /* GlusterFS volume could be backed by a block device */
@@ -713,6 +743,7 @@ static BlockDriver bdrv_gluster = {
     .bdrv_co_readv                = qemu_gluster_co_readv,
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
+    .bdrv_lockf                   = qemu_gluster_lockf,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
@@ -740,6 +771,7 @@ static BlockDriver bdrv_gluster_tcp = {
     .bdrv_co_readv                = qemu_gluster_co_readv,
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
+    .bdrv_lockf                   = qemu_gluster_lockf,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
@@ -767,6 +799,7 @@ static BlockDriver bdrv_gluster_unix = {
     .bdrv_co_readv                = qemu_gluster_co_readv,
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
+    .bdrv_lockf                   = qemu_gluster_lockf,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
@@ -794,6 +827,7 @@ static BlockDriver bdrv_gluster_rdma = {
     .bdrv_co_readv                = qemu_gluster_co_readv,
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
+    .bdrv_lockf                   = qemu_gluster_lockf,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 07/15] qemu-io: Add "-L" option for BDRV_O_NO_LOCK

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-io.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index 288bba8..6bb6232 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -107,6 +107,7 @@ static void open_help(void)
 " -r, -- open file read-only\n"
 " -s, -- use snapshot file\n"
 " -n, -- disable host cache\n"
+" -L, -- disable image lock\n"
 " -o, -- options to be given to the block driver"
 "\n");
 }
@@ -139,12 +140,13 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
 {
     int flags = 0;
     int readonly = 0;
+    int nolock = 0;
     bool writethrough = true;
     int c;
     QemuOpts *qopts;
     QDict *opts;
 
-    while ((c = getopt(argc, argv, "snrgo:")) != -1) {
+    while ((c = getopt(argc, argv, "snrgLo:")) != -1) {
         switch (c) {
         case 's':
             flags |= BDRV_O_SNAPSHOT;
@@ -156,6 +158,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
         case 'r':
             readonly = 1;
             break;
+        case 'L':
+            nolock = 1;
+            break;
         case 'o':
             if (imageOpts) {
                 printf("--image-opts and 'open -o' are mutually exclusive\n");
@@ -176,6 +181,10 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
         flags |= BDRV_O_RDWR;
     }
 
+    if (nolock) {
+        flags |= BDRV_O_NO_LOCK;
+    }
+
     if (imageOpts && (optind == argc - 1)) {
         if (!qemu_opts_parse_noisily(&empty_opts, argv[optind], false)) {
             qemu_opts_reset(&empty_opts);
@@ -410,7 +419,8 @@ static QemuOptsList file_opts = {
 int main(int argc, char **argv)
 {
     int readonly = 0;
-    const char *sopt = "hVc:d:f:rsnmgkt:T:";
+    int nolock = 0;
+    const char *sopt = "hVc:d:f:rLsnmgkt:T:";
     const struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -418,6 +428,7 @@ int main(int argc, char **argv)
         { "cmd", required_argument, NULL, 'c' },
         { "format", required_argument, NULL, 'f' },
         { "read-only", no_argument, NULL, 'r' },
+        { "no-lock", no_argument, NULL, 'L' },
         { "snapshot", no_argument, NULL, 's' },
         { "nocache", no_argument, NULL, 'n' },
         { "misalign", no_argument, NULL, 'm' },
@@ -477,6 +488,9 @@ int main(int argc, char **argv)
         case 'r':
             readonly = 1;
             break;
+        case 'L':
+            nolock = 1;
+            break;
         case 'm':
             qemuio_misalign = true;
             break;
@@ -558,6 +572,10 @@ int main(int argc, char **argv)
         flags |= BDRV_O_RDWR;
     }
 
+    if (nolock) {
+        flags |= BDRV_O_NO_LOCK;
+    }
+
     if ((argc - optind) == 1) {
         if (imageOpts) {
             QemuOpts *qopts = NULL;
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.7 07/15] qemu-io: Add "-L" option for BDRV_O_NO_LOCK

[Denis V. Lunev]

On 04/13/2016 12:09 PM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   qemu-io.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/qemu-io.c b/qemu-io.c
> index 288bba8..6bb6232 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -107,6 +107,7 @@ static void open_help(void)
>   " -r, -- open file read-only\n"
>   " -s, -- use snapshot file\n"
>   " -n, -- disable host cache\n"
> +" -L, -- disable image lock\n"
>   " -o, -- options to be given to the block driver"
>   "\n");
>   }
> @@ -139,12 +140,13 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
>   {
>       int flags = 0;
>       int readonly = 0;
> +    int nolock = 0;
>       bool writethrough = true;
>       int c;
>       QemuOpts *qopts;
>       QDict *opts;
>   
> -    while ((c = getopt(argc, argv, "snrgo:")) != -1) {
> +    while ((c = getopt(argc, argv, "snrgLo:")) != -1) {
>           switch (c) {
>           case 's':
>               flags |= BDRV_O_SNAPSHOT;
> @@ -156,6 +158,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
>           case 'r':
>               readonly = 1;
>               break;
> +        case 'L':
> +            nolock = 1;
> +            break;
>           case 'o':
>               if (imageOpts) {
>                   printf("--image-opts and 'open -o' are mutually exclusive\n");
> @@ -176,6 +181,10 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
>           flags |= BDRV_O_RDWR;
>       }
>   
> +    if (nolock) {
> +        flags |= BDRV_O_NO_LOCK;
> +    }
> +
>       if (imageOpts && (optind == argc - 1)) {
>           if (!qemu_opts_parse_noisily(&empty_opts, argv[optind], false)) {
>               qemu_opts_reset(&empty_opts);
> @@ -410,7 +419,8 @@ static QemuOptsList file_opts = {
>   int main(int argc, char **argv)
>   {
>       int readonly = 0;
> -    const char *sopt = "hVc:d:f:rsnmgkt:T:";
> +    int nolock = 0;
> +    const char *sopt = "hVc:d:f:rLsnmgkt:T:";
>       const struct option lopt[] = {
>           { "help", no_argument, NULL, 'h' },
>           { "version", no_argument, NULL, 'V' },
> @@ -418,6 +428,7 @@ int main(int argc, char **argv)
>           { "cmd", required_argument, NULL, 'c' },
>           { "format", required_argument, NULL, 'f' },
>           { "read-only", no_argument, NULL, 'r' },
> +        { "no-lock", no_argument, NULL, 'L' },
>           { "snapshot", no_argument, NULL, 's' },
>           { "nocache", no_argument, NULL, 'n' },
>           { "misalign", no_argument, NULL, 'm' },
> @@ -477,6 +488,9 @@ int main(int argc, char **argv)
>           case 'r':
>               readonly = 1;
>               break;
> +        case 'L':
> +            nolock = 1;
> +            break;
>           case 'm':
>               qemuio_misalign = true;
>               break;
> @@ -558,6 +572,10 @@ int main(int argc, char **argv)
>           flags |= BDRV_O_RDWR;
>       }
>   
> +    if (nolock) {
> +        flags |= BDRV_O_NO_LOCK;
> +    }
> +
>       if ((argc - optind) == 1) {
>           if (imageOpts) {
>               QemuOpts *qopts = NULL;
I think that we should do the same for qemu-img.

There are some operations like 'qemu-img info' which
are widely used to query f.e. block size of the image.

Den

Re: [Qemu-devel] [PATCH for-2.7 07/15] qemu-io: Add "-L" option for BDRV_O_NO_LOCK

[Fam Zheng]

On Thu, 04/14 10:06, Denis V. Lunev wrote:
> On 04/13/2016 12:09 PM, Fam Zheng wrote:
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  qemu-io.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> >diff --git a/qemu-io.c b/qemu-io.c
> >index 288bba8..6bb6232 100644
> >--- a/qemu-io.c
> >+++ b/qemu-io.c
> >@@ -107,6 +107,7 @@ static void open_help(void)
> >  " -r, -- open file read-only\n"
> >  " -s, -- use snapshot file\n"
> >  " -n, -- disable host cache\n"
> >+" -L, -- disable image lock\n"
> >  " -o, -- options to be given to the block driver"
> >  "\n");
> >  }
> >@@ -139,12 +140,13 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
> >  {
> >      int flags = 0;
> >      int readonly = 0;
> >+    int nolock = 0;
> >      bool writethrough = true;
> >      int c;
> >      QemuOpts *qopts;
> >      QDict *opts;
> >-    while ((c = getopt(argc, argv, "snrgo:")) != -1) {
> >+    while ((c = getopt(argc, argv, "snrgLo:")) != -1) {
> >          switch (c) {
> >          case 's':
> >              flags |= BDRV_O_SNAPSHOT;
> >@@ -156,6 +158,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
> >          case 'r':
> >              readonly = 1;
> >              break;
> >+        case 'L':
> >+            nolock = 1;
> >+            break;
> >          case 'o':
> >              if (imageOpts) {
> >                  printf("--image-opts and 'open -o' are mutually exclusive\n");
> >@@ -176,6 +181,10 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
> >          flags |= BDRV_O_RDWR;
> >      }
> >+    if (nolock) {
> >+        flags |= BDRV_O_NO_LOCK;
> >+    }
> >+
> >      if (imageOpts && (optind == argc - 1)) {
> >          if (!qemu_opts_parse_noisily(&empty_opts, argv[optind], false)) {
> >              qemu_opts_reset(&empty_opts);
> >@@ -410,7 +419,8 @@ static QemuOptsList file_opts = {
> >  int main(int argc, char **argv)
> >  {
> >      int readonly = 0;
> >-    const char *sopt = "hVc:d:f:rsnmgkt:T:";
> >+    int nolock = 0;
> >+    const char *sopt = "hVc:d:f:rLsnmgkt:T:";
> >      const struct option lopt[] = {
> >          { "help", no_argument, NULL, 'h' },
> >          { "version", no_argument, NULL, 'V' },
> >@@ -418,6 +428,7 @@ int main(int argc, char **argv)
> >          { "cmd", required_argument, NULL, 'c' },
> >          { "format", required_argument, NULL, 'f' },
> >          { "read-only", no_argument, NULL, 'r' },
> >+        { "no-lock", no_argument, NULL, 'L' },
> >          { "snapshot", no_argument, NULL, 's' },
> >          { "nocache", no_argument, NULL, 'n' },
> >          { "misalign", no_argument, NULL, 'm' },
> >@@ -477,6 +488,9 @@ int main(int argc, char **argv)
> >          case 'r':
> >              readonly = 1;
> >              break;
> >+        case 'L':
> >+            nolock = 1;
> >+            break;
> >          case 'm':
> >              qemuio_misalign = true;
> >              break;
> >@@ -558,6 +572,10 @@ int main(int argc, char **argv)
> >          flags |= BDRV_O_RDWR;
> >      }
> >+    if (nolock) {
> >+        flags |= BDRV_O_NO_LOCK;
> >+    }
> >+
> >      if ((argc - optind) == 1) {
> >          if (imageOpts) {
> >              QemuOpts *qopts = NULL;
> I think that we should do the same for qemu-img.
> 
> There are some operations like 'qemu-img info' which
> are widely used to query f.e. block size of the image.

Yes, I can extend this series to add "nolock" option to qemu-img sub commands.

Fam

[Qemu-devel] [PATCH for-2.7 08/15] qemu-iotests: 140: Disable image lock for qemu-io access

[Fam Zheng]

The VM is still on, the image locking check would complain.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/140 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
index 05e4506..412b26f 100755
--- a/tests/qemu-iotests/140
+++ b/tests/qemu-iotests/140
@@ -71,7 +71,7 @@ _send_qemu_cmd $QEMU_HANDLE \
        'arguments': { 'device': 'drv' }}" \
     'return'
 
-$QEMU_IO_PROG -f raw -c 'read -P 42 0 64k' \
+$QEMU_IO_PROG -f raw -L -c 'read -P 42 0 64k' \
     "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
     | _filter_qemu_io | _filter_nbd
 
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 09/15] qemu-iotests: 046: Move version detection out from verify_io

[Fam Zheng]

So the image lock won't complain.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/046 | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
index e0be46c..40c4bc0 100755
--- a/tests/qemu-iotests/046
+++ b/tests/qemu-iotests/046
@@ -193,15 +193,7 @@ echo "== Verify image content =="
 
 function verify_io()
 {
-    if ($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > /dev/null); then
-        # For v2 images, discarded clusters are read from the backing file
-        # Keep the variable empty so that the backing file value can be used as
-        # the default below
-        discarded=
-    else
-        # Discarded clusters are zeroed for v3 or later
-        discarded=0
-    fi
+    discarded=$1
 
     echo read -P 0 0 0x10000
 
@@ -262,7 +254,17 @@ function verify_io()
     echo read -P 17  0x11c000 0x4000
 }
 
-verify_io | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
+if ($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > /dev/null); then
+    # For v2 images, discarded clusters are read from the backing file
+    # Keep the variable empty so that the backing file value can be used as
+    # the default below
+    discarded=
+else
+    # Discarded clusters are zeroed for v3 or later
+    discarded=0
+fi
+
+verify_io $discarded | $QEMU_IO -L "$TEST_IMG" | _filter_qemu_io
 
 _check_test_img
 
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 10/15] qemu-iotests: Fix lock-image for shared disk in test case 091

[Fam Zheng]

Because the source and the destination QEMU instances both open the
image, we have to disable the lock.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/091 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
index 32bbd56..2a3c9bd 100755
--- a/tests/qemu-iotests/091
+++ b/tests/qemu-iotests/091
@@ -61,13 +61,13 @@ echo === Starting QEMU VM1 ===
 echo
 
 qemu_comm_method="monitor"
-_launch_qemu -drive file="${TEST_IMG}",cache=${CACHEMODE},id=disk
+_launch_qemu -drive file="${TEST_IMG}",cache=${CACHEMODE},id=disk,lock-image=off
 h1=$QEMU_HANDLE
 
 echo
 echo === Starting QEMU VM2 ===
 echo
-_launch_qemu -drive file="${TEST_IMG}",cache=${CACHEMODE},id=disk \
+_launch_qemu -drive file="${TEST_IMG}",cache=${CACHEMODE},id=disk,lock-image=off \
              -incoming "exec: cat '${MIG_FIFO}'"
 h2=$QEMU_HANDLE
 
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 11/15] qemu-iotests: Disable image lock when checking test image

[Fam Zheng]

The VM is running, qemu-io would fail the lock acquisition.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/030 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 3ac2443..fa996ef 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -95,7 +95,7 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         # The image map is empty before the operation
-        empty_map = qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img)
+        empty_map = qemu_io('-L', '-f', iotests.imgfmt, '-c', 'map', test_img)
 
         # This is a no-op: no data should ever be copied from the base image
         result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 12/15] qemu-iotests: 051: Disable image lock in the command line

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/051        |  2 +-
 tests/qemu-iotests/051.out    | 10 +++++-----
 tests/qemu-iotests/051.pc.out | 10 +++++-----
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 88b3d91..2fee7e8 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -218,7 +218,7 @@ run_qemu -drive driver=null-co,cache=invalid_value
 
 for cache in writeback writethrough unsafe invalid_value; do
     echo -e "info block\ninfo block file\ninfo block backing\ninfo block backing-file" | \
-    run_qemu -drive file="$TEST_IMG",cache=$cache,backing.file.filename="$TEST_IMG.base",backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=$device_id -nodefaults
+    run_qemu -drive file="$TEST_IMG",cache=$cache,backing.file.filename="$TEST_IMG.base",backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=$device_id,lock-image=off -nodefaults
 done
 
 echo
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 408d613..b886b1c 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -145,7 +145,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 Testing: -drive driver=null-co,cache=invalid_value
 QEMU_PROG: -drive driver=null-co,cache=invalid_value: invalid cache option
 
-Testing: -drive file=TEST_DIR/t.qcow2,cache=writeback,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
+Testing: -drive file=TEST_DIR/t.qcow2,cache=writeback,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0,lock-image=off -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i^[[K^[[Din^[[K^[[D^[[Dinf^[[K^[[D^[[D^[[Dinfo^[[K^[[D^[[D^[[D^[[Dinfo ^[[K^[[D^[[D^[[D^[[D^[[Dinfo b^[[K^[[D^[[D^[[D^[[D^[[D^[[Dinfo bl^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo blo^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo bloc^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block^[[K
 drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
@@ -165,7 +165,7 @@ backing-file: TEST_DIR/t.qcow2.base (file, read-only)
     Cache mode:       writeback, ignore flushes
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 
-Testing: -drive file=TEST_DIR/t.qcow2,cache=writethrough,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
+Testing: -drive file=TEST_DIR/t.qcow2,cache=writethrough,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0,lock-image=off -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i^[[K^[[Din^[[K^[[D^[[Dinf^[[K^[[D^[[D^[[Dinfo^[[K^[[D^[[D^[[D^[[Dinfo ^[[K^[[D^[[D^[[D^[[D^[[Dinfo b^[[K^[[D^[[D^[[D^[[D^[[D^[[Dinfo bl^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo blo^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo bloc^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block^[[K
 drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
@@ -185,7 +185,7 @@ backing-file: TEST_DIR/t.qcow2.base (file, read-only)
     Cache mode:       writeback, ignore flushes
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 
-Testing: -drive file=TEST_DIR/t.qcow2,cache=unsafe,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
+Testing: -drive file=TEST_DIR/t.qcow2,cache=unsafe,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0,lock-image=off -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i^[[K^[[Din^[[K^[[D^[[Dinf^[[K^[[D^[[D^[[Dinfo^[[K^[[D^[[D^[[D^[[Dinfo ^[[K^[[D^[[D^[[D^[[D^[[Dinfo b^[[K^[[D^[[D^[[D^[[D^[[D^[[Dinfo bl^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo blo^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo bloc^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block^[[K
 drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
@@ -205,8 +205,8 @@ backing-file: TEST_DIR/t.qcow2.base (file, read-only)
     Cache mode:       writeback, ignore flushes
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 
-Testing: -drive file=TEST_DIR/t.qcow2,cache=invalid_value,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,cache=invalid_value,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0: invalid cache option
+Testing: -drive file=TEST_DIR/t.qcow2,cache=invalid_value,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0,lock-image=off -nodefaults
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,cache=invalid_value,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0,lock-image=off: invalid cache option
 
 
 === Specifying the protocol layer ===
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index ec6d222..9aca65f 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -239,7 +239,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 Testing: -drive driver=null-co,cache=invalid_value
 QEMU_PROG: -drive driver=null-co,cache=invalid_value: invalid cache option
 
-Testing: -drive file=TEST_DIR/t.qcow2,cache=writeback,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
+Testing: -drive file=TEST_DIR/t.qcow2,cache=writeback,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0,lock-image=off -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i^[[K^[[Din^[[K^[[D^[[Dinf^[[K^[[D^[[D^[[Dinfo^[[K^[[D^[[D^[[D^[[Dinfo ^[[K^[[D^[[D^[[D^[[D^[[Dinfo b^[[K^[[D^[[D^[[D^[[D^[[D^[[Dinfo bl^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo blo^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo bloc^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block^[[K
 drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
@@ -259,7 +259,7 @@ backing-file: TEST_DIR/t.qcow2.base (file, read-only)
     Cache mode:       writeback, ignore flushes
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 
-Testing: -drive file=TEST_DIR/t.qcow2,cache=writethrough,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
+Testing: -drive file=TEST_DIR/t.qcow2,cache=writethrough,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0,lock-image=off -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i^[[K^[[Din^[[K^[[D^[[Dinf^[[K^[[D^[[D^[[Dinfo^[[K^[[D^[[D^[[D^[[Dinfo ^[[K^[[D^[[D^[[D^[[D^[[Dinfo b^[[K^[[D^[[D^[[D^[[D^[[D^[[Dinfo bl^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo blo^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo bloc^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block^[[K
 drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
@@ -279,7 +279,7 @@ backing-file: TEST_DIR/t.qcow2.base (file, read-only)
     Cache mode:       writeback, ignore flushes
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 
-Testing: -drive file=TEST_DIR/t.qcow2,cache=unsafe,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
+Testing: -drive file=TEST_DIR/t.qcow2,cache=unsafe,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0,lock-image=off -nodefaults
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) i^[[K^[[Din^[[K^[[D^[[Dinf^[[K^[[D^[[D^[[Dinfo^[[K^[[D^[[D^[[D^[[Dinfo ^[[K^[[D^[[D^[[D^[[D^[[Dinfo b^[[K^[[D^[[D^[[D^[[D^[[D^[[Dinfo bl^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo blo^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo bloc^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block^[[K
 drive0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
@@ -299,8 +299,8 @@ backing-file: TEST_DIR/t.qcow2.base (file, read-only)
     Cache mode:       writeback, ignore flushes
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 
-Testing: -drive file=TEST_DIR/t.qcow2,cache=invalid_value,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0 -nodefaults
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,cache=invalid_value,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0: invalid cache option
+Testing: -drive file=TEST_DIR/t.qcow2,cache=invalid_value,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0,lock-image=off -nodefaults
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,cache=invalid_value,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,if=none,id=drive0,lock-image=off: invalid cache option
 
 
 === Specifying the protocol layer ===
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 13/15] ahci-test: Specify "lock-image=off" in CLI

[Fam Zheng]

The case is the temporary image is sometimes used by more than one QEMU
processes, just use the nop lock to avoid image locking failures.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/ahci-test.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 6869f7f..fb3505f 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -179,6 +179,7 @@ static AHCIQState *ahci_boot(const char *cli, ...)
         va_end(ap);
     } else {
         cli = "-drive if=none,id=drive0,file=%s,cache=writeback,serial=%s"
+            ",lock-image=off"
             ",format=%s"
             " -M q35 "
             "-device ide-hd,drive=drive0 "
@@ -1082,6 +1083,7 @@ static void test_flush_retry(void)
     prepare_blkdebug_script(debug_path, "flush_to_disk");
     ahci = ahci_boot_and_enable("-drive file=blkdebug:%s:%s,if=none,id=drive0,"
                                 "format=%s,cache=writeback,"
+                                "lock-image=off,"
                                 "rerror=stop,werror=stop "
                                 "-M q35 "
                                 "-device ide-hd,drive=drive0 ",
@@ -1107,9 +1109,10 @@ static void test_migrate_sanity(void)
     char *uri = g_strdup_printf("unix:%s", mig_socket);
 
     src = ahci_boot("-m 1024 -M q35 "
-                    "-drive if=ide,file=%s,format=%s ", tmp_path, imgfmt);
+                    "-drive if=ide,file=%s,lock-image=off,format=%s ",
+                    tmp_path, imgfmt);
     dst = ahci_boot("-m 1024 -M q35 "
-                    "-drive if=ide,file=%s,format=%s "
+                    "-drive if=ide,file=%s,format=%s,lock-image=off "
                     "-incoming %s", tmp_path, imgfmt, uri);
 
     ahci_migrate(src, dst, uri);
@@ -1132,10 +1135,10 @@ static void ahci_migrate_simple(uint8_t cmd_read, uint8_t cmd_write)
     char *uri = g_strdup_printf("unix:%s", mig_socket);
 
     src = ahci_boot_and_enable("-m 1024 -M q35 "
-                               "-drive if=ide,format=%s,file=%s ",
+                               "-drive if=ide,format=%s,file=%s,lock-image=off ",
                                imgfmt, tmp_path);
     dst = ahci_boot("-m 1024 -M q35 "
-                    "-drive if=ide,format=%s,file=%s "
+                    "-drive if=ide,format=%s,file=%s,lock-image=off "
                     "-incoming %s", imgfmt, tmp_path, uri);
 
     set_context(src->parent);
@@ -1192,6 +1195,7 @@ static void ahci_halted_io_test(uint8_t cmd_read, uint8_t cmd_write)
     prepare_blkdebug_script(debug_path, "write_aio");
 
     ahci = ahci_boot_and_enable("-drive file=blkdebug:%s:%s,if=none,id=drive0,"
+                                "lock-image=off,"
                                 "format=%s,cache=writeback,"
                                 "rerror=stop,werror=stop "
                                 "-M q35 "
@@ -1258,6 +1262,7 @@ static void ahci_migrate_halted_io(uint8_t cmd_read, uint8_t cmd_write)
     prepare_blkdebug_script(debug_path, "write_aio");
 
     src = ahci_boot_and_enable("-drive file=blkdebug:%s:%s,if=none,id=drive0,"
+                                "lock-image=off,"
                                "format=%s,cache=writeback,"
                                "rerror=stop,werror=stop "
                                "-M q35 "
@@ -1266,6 +1271,7 @@ static void ahci_migrate_halted_io(uint8_t cmd_read, uint8_t cmd_write)
                                tmp_path, imgfmt);
 
     dst = ahci_boot("-drive file=%s,if=none,id=drive0,"
+                    "lock-image=off,"
                     "format=%s,cache=writeback,"
                     "rerror=stop,werror=stop "
                     "-M q35 "
@@ -1331,12 +1337,14 @@ static void test_flush_migrate(void)
 
     src = ahci_boot_and_enable("-drive file=blkdebug:%s:%s,if=none,id=drive0,"
                                "cache=writeback,rerror=stop,werror=stop,"
+                                "lock-image=off,"
                                "format=%s "
                                "-M q35 "
                                "-device ide-hd,drive=drive0 ",
                                debug_path, tmp_path, imgfmt);
     dst = ahci_boot("-drive file=%s,if=none,id=drive0,"
                     "cache=writeback,rerror=stop,werror=stop,"
+                    "lock-image=off,"
                     "format=%s "
                     "-M q35 "
                     "-device ide-hd,drive=drive0 "
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 14/15] ide-test: Specify "lock-image=off" in command lines

[Fam Zheng]

If a failure in a previous test case doesn't clean up the running qemu process
(it happens), the subsequent ones can fail because of a image locking failure.
That is not an authentic failure of the test case itself and could be sometimes
confusing.  Disable image locking to avoid that.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/ide-test.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 0d9ab4d..004ea4f 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -504,7 +504,8 @@ static void test_flush(void)
     uint8_t data;
 
     ide_test_start(
-        "-drive file=blkdebug::%s,if=ide,cache=writeback,format=raw",
+        "-drive file=blkdebug::%s,if=ide,lock-image=off,"
+        "cache=writeback,format=raw",
         tmp_path);
 
     /* Delay the completion of the flush request until we explicitly do it */
@@ -546,7 +547,7 @@ static void test_retry_flush(const char *machine)
     ide_test_start(
         "-vnc none "
         "-drive file=blkdebug:%s:%s,if=ide,cache=writeback,format=raw,"
-        "rerror=stop,werror=stop",
+        "rerror=stop,werror=stop,lock-image=off",
         debug_path, tmp_path);
 
     /* FLUSH CACHE command on device 0*/
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 15/15] block: Turn on image locking by default

[Fam Zheng]

Now that test cases are covered, we can turn it on.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 93bd43e..87d22c3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -383,7 +383,7 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
             }
         }
 
-        if (!qemu_opt_get_bool(opts, "lock-image", false)) {
+        if (!qemu_opt_get_bool(opts, "lock-image", true)) {
             *bdrv_flags |= BDRV_O_NO_LOCK;
         }
     }
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

[Daniel P. Berrange]

On Wed, Apr 13, 2016 at 05:09:49PM +0800, Fam Zheng wrote:
> Too many troubles have been caused by two processes writing to the same image
> unexpectedly. This series introduces automatical image locking into QEMU to
> avoid such tragedy. With this, the user won't be able to open the image from
> two processes (e.g. using qemu-img when the image is attached to the guest).
> 
> Underneath is the fcntl syscall that locks the local file, similar to what is
> already used in libvirt virtlockd.  Also because of that, we cannot directly
> apply fcntl lock on the image file itself, instead we open and lock
> "/var/tmp/.qemu-$sha1.lock", where $sha1 is derived from the image's full path
> as in realpath(3). This mechanism should be equally useful for the single host
> case, and it doesn't conflict with virtlockd when managed by libvirt.
> 
> The alternative file locking API on Linux, flock(2), cannot protect host NFS
> mount points, so it's not used.

Maybe I'm missing something, but since you are locking /var/tmp/.qemu-$sha1.lock
the question of NFS support is irrelevant. All your locks are only ever going
to apply within the local host, since /var/tmp is always a local filesystem,
regardless of whether the actual image is on NFS. IOW, even using fcntl() you
have no cross-host protection for NFS based images here.

BTW, looking again at the virtlockd code I notice that I had the good
idea to be very selective with our use of fcntl() - rather than locking
the entire file, we only lock a single byte at offset 0.

So it would be possible for QEMU to directly using fcntl() on the image
file, if it provided a non-zero offset to lock at. eg QEMU could lock
byte 1 and that shouldn't interact with virtlockd's lock at byte 0.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

[Fam Zheng]

On Wed, 04/13 10:19, Daniel P. Berrange wrote:
> On Wed, Apr 13, 2016 at 05:09:49PM +0800, Fam Zheng wrote:
> > Too many troubles have been caused by two processes writing to the same image
> > unexpectedly. This series introduces automatical image locking into QEMU to
> > avoid such tragedy. With this, the user won't be able to open the image from
> > two processes (e.g. using qemu-img when the image is attached to the guest).
> > 
> > Underneath is the fcntl syscall that locks the local file, similar to what is
> > already used in libvirt virtlockd.  Also because of that, we cannot directly
> > apply fcntl lock on the image file itself, instead we open and lock
> > "/var/tmp/.qemu-$sha1.lock", where $sha1 is derived from the image's full path
> > as in realpath(3). This mechanism should be equally useful for the single host
> > case, and it doesn't conflict with virtlockd when managed by libvirt.
> > 
> > The alternative file locking API on Linux, flock(2), cannot protect host NFS
> > mount points, so it's not used.
> 
> Maybe I'm missing something, but since you are locking /var/tmp/.qemu-$sha1.lock
> the question of NFS support is irrelevant. All your locks are only ever going
> to apply within the local host, since /var/tmp is always a local filesystem,
> regardless of whether the actual image is on NFS. IOW, even using fcntl() you
> have no cross-host protection for NFS based images here.

You are right. I had a wrong impression in mind when writing the cover letter,
because in a draft version, instead of "lock-image = on | off", I once
implemented "lock-mode = off | image | lockfile", where the "image" mode will
lock the image itself like libvirt does. Later I realized it is cleaner to
stick to one locking scheme, considering we could have format locking in the
future, also considering the glusterfs patch, where the "lockfile" mode doesn't
make much sense.

I'll update the code to lock byte 1 then this point will become valid.

Fam

Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

[Denis V. Lunev]

On 04/13/2016 12:09 PM, Fam Zheng wrote:
> Too many troubles have been caused by two processes writing to the same image
> unexpectedly. This series introduces automatical image locking into QEMU to
> avoid such tragedy. With this, the user won't be able to open the image from
> two processes (e.g. using qemu-img when the image is attached to the guest).
>
> Underneath is the fcntl syscall that locks the local file, similar to what is
> already used in libvirt virtlockd.  Also because of that, we cannot directly
> apply fcntl lock on the image file itself, instead we open and lock
> "/var/tmp/.qemu-$sha1.lock", where $sha1 is derived from the image's full path
> as in realpath(3). This mechanism should be equally useful for the single host
> case, and it doesn't conflict with virtlockd when managed by libvirt.
>
> The alternative file locking API on Linux, flock(2), cannot protect host NFS
> mount points, so it's not used.
>
> Gluster locking is also implemented wrapping glfs_posix_lock in patch 6. It's
> only lightly tested.
>
> All other drivers that don't implement .bdrv_lockf are always permissive and
> does no checking.
>
> In the future, the intention is that image format drivers that introduce
> locking mechanisms could also fit into this API.
>
> The first 6 patches define the internal and external interfaces, and implement
> the image locking.
>
> Patch 7 adds an option in qemu-io to allow disabling the lock, for testing
> purpose.
>
> Patches 8 - 14 fixes the potential failures of test cases where multiple
> processes may open the image concurrently.
>
> Finally the default behavior is switched to on in patch 15.
>
> Fam Zheng (15):
>    block: Add BDRV_O_NO_LOCK
>    qapi: Add lock-image in blockdev-add options
>    blockdev: Add and parse "lock-image" option for block devices
>    block: Introduce image file locking interface
>    raw-posix: Implement .bdrv_lockf
>    gluster: Implement .bdrv_lockf
>    qemu-io: Add "-L" option for BDRV_O_NO_LOCK
>    qemu-iotests: 140: Disable image lock for qemu-io access
>    qemu-iotests: 046: Move version detection out from verify_io
>    qemu-iotests: Fix lock-image for shared disk in test case 091
>    qemu-iotests: Disable image lock when checking test image
>    qemu-iotests: 051: Disable image lock in the command line
>    ahci-test: Specify "lock-image=off" in CLI
>    ide-test: Specify "lock-image=off" in command lines
>    block: Turn on image locking by default
>
>   block.c                       | 25 +++++++++++
>   block/gluster.c               | 34 +++++++++++++++
>   block/raw-posix.c             | 97 +++++++++++++++++++++++++++++++++++++++++++
>   blockdev.c                    |  8 ++++
>   include/block/block.h         |  9 ++++
>   include/block/block_int.h     |  5 +++
>   qapi/block-core.json          |  6 ++-
>   qemu-io.c                     | 22 +++++++++-
>   tests/ahci-test.c             | 16 +++++--
>   tests/ide-test.c              |  5 ++-
>   tests/qemu-iotests/030        |  2 +-
>   tests/qemu-iotests/046        | 22 +++++-----
>   tests/qemu-iotests/051        |  2 +-
>   tests/qemu-iotests/051.out    | 10 ++---
>   tests/qemu-iotests/051.pc.out | 10 ++---
>   tests/qemu-iotests/091        |  4 +-
>   tests/qemu-iotests/140        |  2 +-
>   17 files changed, 245 insertions(+), 34 deletions(-)
>
First of all, I like the approach as we have discussed :) Then,
in general, I support Daniel with the point that
the locking should be done on the image file
directly.

Also, it looks like this will break migration with the shared storage.
For me it seems that we will have lock the image from both ends

Den

Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

[Fam Zheng]

On Wed, 04/13 13:18, Denis V. Lunev wrote:
> On 04/13/2016 12:09 PM, Fam Zheng wrote:
> >Too many troubles have been caused by two processes writing to the same image
> >unexpectedly. This series introduces automatical image locking into QEMU to
> >avoid such tragedy. With this, the user won't be able to open the image from
> >two processes (e.g. using qemu-img when the image is attached to the guest).
> >
> >Underneath is the fcntl syscall that locks the local file, similar to what is
> >already used in libvirt virtlockd.  Also because of that, we cannot directly
> >apply fcntl lock on the image file itself, instead we open and lock
> >"/var/tmp/.qemu-$sha1.lock", where $sha1 is derived from the image's full path
> >as in realpath(3). This mechanism should be equally useful for the single host
> >case, and it doesn't conflict with virtlockd when managed by libvirt.
> >
> >The alternative file locking API on Linux, flock(2), cannot protect host NFS
> >mount points, so it's not used.
> >
> >Gluster locking is also implemented wrapping glfs_posix_lock in patch 6. It's
> >only lightly tested.
> >
> >All other drivers that don't implement .bdrv_lockf are always permissive and
> >does no checking.
> >
> >In the future, the intention is that image format drivers that introduce
> >locking mechanisms could also fit into this API.
> >
> >The first 6 patches define the internal and external interfaces, and implement
> >the image locking.
> >
> >Patch 7 adds an option in qemu-io to allow disabling the lock, for testing
> >purpose.
> >
> >Patches 8 - 14 fixes the potential failures of test cases where multiple
> >processes may open the image concurrently.
> >
> >Finally the default behavior is switched to on in patch 15.
> >
> >Fam Zheng (15):
> >   block: Add BDRV_O_NO_LOCK
> >   qapi: Add lock-image in blockdev-add options
> >   blockdev: Add and parse "lock-image" option for block devices
> >   block: Introduce image file locking interface
> >   raw-posix: Implement .bdrv_lockf
> >   gluster: Implement .bdrv_lockf
> >   qemu-io: Add "-L" option for BDRV_O_NO_LOCK
> >   qemu-iotests: 140: Disable image lock for qemu-io access
> >   qemu-iotests: 046: Move version detection out from verify_io
> >   qemu-iotests: Fix lock-image for shared disk in test case 091
> >   qemu-iotests: Disable image lock when checking test image
> >   qemu-iotests: 051: Disable image lock in the command line
> >   ahci-test: Specify "lock-image=off" in CLI
> >   ide-test: Specify "lock-image=off" in command lines
> >   block: Turn on image locking by default
> >
> >  block.c                       | 25 +++++++++++
> >  block/gluster.c               | 34 +++++++++++++++
> >  block/raw-posix.c             | 97 +++++++++++++++++++++++++++++++++++++++++++
> >  blockdev.c                    |  8 ++++
> >  include/block/block.h         |  9 ++++
> >  include/block/block_int.h     |  5 +++
> >  qapi/block-core.json          |  6 ++-
> >  qemu-io.c                     | 22 +++++++++-
> >  tests/ahci-test.c             | 16 +++++--
> >  tests/ide-test.c              |  5 ++-
> >  tests/qemu-iotests/030        |  2 +-
> >  tests/qemu-iotests/046        | 22 +++++-----
> >  tests/qemu-iotests/051        |  2 +-
> >  tests/qemu-iotests/051.out    | 10 ++---
> >  tests/qemu-iotests/051.pc.out | 10 ++---
> >  tests/qemu-iotests/091        |  4 +-
> >  tests/qemu-iotests/140        |  2 +-
> >  17 files changed, 245 insertions(+), 34 deletions(-)
> >
> First of all, I like the approach as we have discussed :) Then,
> in general, I support Daniel with the point that
> the locking should be done on the image file
> directly.
> 
> Also, it looks like this will break migration with the shared storage.
> For me it seems that we will have lock the image from both ends

There should already be state handling logic between source and destinition
ends, I'll take a look and see if we can add lock/unlock calls there to keep it
working.

Fam

Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

[Denis V. Lunev]

On 04/14/2016 05:36 AM, Fam Zheng wrote:
> On Wed, 04/13 13:18, Denis V. Lunev wrote:
>> On 04/13/2016 12:09 PM, Fam Zheng wrote:
>>> Too many troubles have been caused by two processes writing to the same image
>>> unexpectedly. This series introduces automatical image locking into QEMU to
>>> avoid such tragedy. With this, the user won't be able to open the image from
>>> two processes (e.g. using qemu-img when the image is attached to the guest).
>>>
>>> Underneath is the fcntl syscall that locks the local file, similar to what is
>>> already used in libvirt virtlockd.  Also because of that, we cannot directly
>>> apply fcntl lock on the image file itself, instead we open and lock
>>> "/var/tmp/.qemu-$sha1.lock", where $sha1 is derived from the image's full path
>>> as in realpath(3). This mechanism should be equally useful for the single host
>>> case, and it doesn't conflict with virtlockd when managed by libvirt.
>>>
>>> The alternative file locking API on Linux, flock(2), cannot protect host NFS
>>> mount points, so it's not used.
>>>
>>> Gluster locking is also implemented wrapping glfs_posix_lock in patch 6. It's
>>> only lightly tested.
>>>
>>> All other drivers that don't implement .bdrv_lockf are always permissive and
>>> does no checking.
>>>
>>> In the future, the intention is that image format drivers that introduce
>>> locking mechanisms could also fit into this API.
>>>
>>> The first 6 patches define the internal and external interfaces, and implement
>>> the image locking.
>>>
>>> Patch 7 adds an option in qemu-io to allow disabling the lock, for testing
>>> purpose.
>>>
>>> Patches 8 - 14 fixes the potential failures of test cases where multiple
>>> processes may open the image concurrently.
>>>
>>> Finally the default behavior is switched to on in patch 15.
>>>
>>> Fam Zheng (15):
>>>    block: Add BDRV_O_NO_LOCK
>>>    qapi: Add lock-image in blockdev-add options
>>>    blockdev: Add and parse "lock-image" option for block devices
>>>    block: Introduce image file locking interface
>>>    raw-posix: Implement .bdrv_lockf
>>>    gluster: Implement .bdrv_lockf
>>>    qemu-io: Add "-L" option for BDRV_O_NO_LOCK
>>>    qemu-iotests: 140: Disable image lock for qemu-io access
>>>    qemu-iotests: 046: Move version detection out from verify_io
>>>    qemu-iotests: Fix lock-image for shared disk in test case 091
>>>    qemu-iotests: Disable image lock when checking test image
>>>    qemu-iotests: 051: Disable image lock in the command line
>>>    ahci-test: Specify "lock-image=off" in CLI
>>>    ide-test: Specify "lock-image=off" in command lines
>>>    block: Turn on image locking by default
>>>
>>>   block.c                       | 25 +++++++++++
>>>   block/gluster.c               | 34 +++++++++++++++
>>>   block/raw-posix.c             | 97 +++++++++++++++++++++++++++++++++++++++++++
>>>   blockdev.c                    |  8 ++++
>>>   include/block/block.h         |  9 ++++
>>>   include/block/block_int.h     |  5 +++
>>>   qapi/block-core.json          |  6 ++-
>>>   qemu-io.c                     | 22 +++++++++-
>>>   tests/ahci-test.c             | 16 +++++--
>>>   tests/ide-test.c              |  5 ++-
>>>   tests/qemu-iotests/030        |  2 +-
>>>   tests/qemu-iotests/046        | 22 +++++-----
>>>   tests/qemu-iotests/051        |  2 +-
>>>   tests/qemu-iotests/051.out    | 10 ++---
>>>   tests/qemu-iotests/051.pc.out | 10 ++---
>>>   tests/qemu-iotests/091        |  4 +-
>>>   tests/qemu-iotests/140        |  2 +-
>>>   17 files changed, 245 insertions(+), 34 deletions(-)
>>>
>> First of all, I like the approach as we have discussed :) Then,
>> in general, I support Daniel with the point that
>> the locking should be done on the image file
>> directly.
>>
>> Also, it looks like this will break migration with the shared storage.
>> For me it seems that we will have lock the image from both ends
> There should already be state handling logic between source and destinition
> ends, I'll take a look and see if we can add lock/unlock calls there to keep it
> working.
>
> Fam
unfortunately no. If the lock will be on the image file,
we will have get it on the target node on QEMU start
and re-acquire it in bdrv_invalidate_cache.

 From my POW you should not get the lock if
BDRV_O_INACTIVE is set.

Den

Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

[Fam Zheng]

On Thu, 04/14 08:04, Denis V. Lunev wrote:
> unfortunately no. If the lock will be on the image file,
> we will have get it on the target node on QEMU start
> and re-acquire it in bdrv_invalidate_cache.
> 
> From my POW you should not get the lock if
> BDRV_O_INACTIVE is set.

That is what I meant. :)

At source: Images are unlocked in bdrv_inactivate().

At destinition: Images are not locked if opened with BDRV_O_INACTIVE, and
are locked in bdrv_invalidate_cache() (at the INMIGRATE -> RUNNING switch).

Fam

Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

[Denis V. Lunev]

On 04/14/2016 08:46 AM, Fam Zheng wrote:
> On Thu, 04/14 08:04, Denis V. Lunev wrote:
>> unfortunately no. If the lock will be on the image file,
>> we will have get it on the target node on QEMU start
>> and re-acquire it in bdrv_invalidate_cache.
>>
>>  From my POW you should not get the lock if
>> BDRV_O_INACTIVE is set.
> That is what I meant. :)
>
> At source: Images are unlocked in bdrv_inactivate().
>
> At destinition: Images are not locked if opened with BDRV_O_INACTIVE, and
> are locked in bdrv_invalidate_cache() (at the INMIGRATE -> RUNNING switch).
>
> Fam
it looks I have missed something. The above will work, but
I do not see clause where you set BDRV_O_NOLOCK when BDRV_O_INACTIVE
is set.

Do you mean that we will not reach bdrv_open_common with this flag
set?

Den

Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

[Fam Zheng]

On Thu, 04/14 09:14, Denis V. Lunev wrote:
> On 04/14/2016 08:46 AM, Fam Zheng wrote:
> >On Thu, 04/14 08:04, Denis V. Lunev wrote:
> >>unfortunately no. If the lock will be on the image file,
> >>we will have get it on the target node on QEMU start
> >>and re-acquire it in bdrv_invalidate_cache.
> >>
> >> From my POW you should not get the lock if
> >>BDRV_O_INACTIVE is set.
> >That is what I meant. :)
> >
> >At source: Images are unlocked in bdrv_inactivate().
> >
> >At destinition: Images are not locked if opened with BDRV_O_INACTIVE, and
> >are locked in bdrv_invalidate_cache() (at the INMIGRATE -> RUNNING switch).
> >
> >Fam
> it looks I have missed something. The above will work, but
> I do not see clause where you set BDRV_O_NOLOCK when BDRV_O_INACTIVE
> is set.
> 
> Do you mean that we will not reach bdrv_open_common with this flag
> set?

I'm updating the patch to also check BDRV_O_INACTIVE in bdrv_open_common in v2.

Fam

Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

[Denis V. Lunev]

On 04/14/2016 09:23 AM, Fam Zheng wrote:
> On Thu, 04/14 09:14, Denis V. Lunev wrote:
>> On 04/14/2016 08:46 AM, Fam Zheng wrote:
>>> On Thu, 04/14 08:04, Denis V. Lunev wrote:
>>>> unfortunately no. If the lock will be on the image file,
>>>> we will have get it on the target node on QEMU start
>>>> and re-acquire it in bdrv_invalidate_cache.
>>>>
>>>>  From my POW you should not get the lock if
>>>> BDRV_O_INACTIVE is set.
>>> That is what I meant. :)
>>>
>>> At source: Images are unlocked in bdrv_inactivate().
>>>
>>> At destinition: Images are not locked if opened with BDRV_O_INACTIVE, and
>>> are locked in bdrv_invalidate_cache() (at the INMIGRATE -> RUNNING switch).
>>>
>>> Fam
>> it looks I have missed something. The above will work, but
>> I do not see clause where you set BDRV_O_NOLOCK when BDRV_O_INACTIVE
>> is set.
>>
>> Do you mean that we will not reach bdrv_open_common with this flag
>> set?
> I'm updating the patch to also check BDRV_O_INACTIVE in bdrv_open_common in v2.
>
> Fam
thank you for clarification. This should work. I am eager to see the next
version :)

Den

Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

[Richard W.M. Jones]

On Wed, Apr 13, 2016 at 05:09:49PM +0800, Fam Zheng wrote:
> Underneath is the fcntl syscall that locks the local file, similar to what is
> already used in libvirt virtlockd.  Also because of that, we cannot directly
> apply fcntl lock on the image file itself, instead we open and lock
> "/var/tmp/.qemu-$sha1.lock", where $sha1 is derived

Any particular reason why not /tmp instead of /var/tmp?  I assume
these are zero-sized or otherwise small files.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW