[Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Fam Zheng]

Hi all,

I wanted to post something before the long holiday as promised, but I couldn't
refine or test enough due to limited time.  Please take this as an RFC and do a
high level review. Thanks.

v8: Move user interface option from block device to qdev. [Kevin]
    Add "exclusive" back. [Kevin]

    Note: limited by the qdev interface, until blk_lock_image() is called, the
    images are not locked by block layer at open, because before device tells
    us what to do, block layer cannot figure out the correct lock mode any
    more.

    TODO 1: Lock explicitly in utils (qemu-img, qemu-io, etc) after opening
    image.

    TODO 2: The image locking test case 153 patch is not updated thus broken
    because of the moved option.

    TODO 3: Are the open flags unnecessary because we already have
    ImageLockMode? If so, how to converge them?

Fam Zheng (36):
  block: Add flag bits for image locking
  qapi: Add ImageLockMode
  block: Introduce image file locking
  osdep: Add qemu_lock_fd and qemu_unlock_fd
  raw-posix: Add image locking support
  qemu-io: Add "-L" option for BDRV_O_NO_LOCK
  qemu-img: Add "-L" option to sub commands
  qemu-img: Update documentation of "-L" option
  qemu-nbd: Add "--no-lock/-L" option
  block: Don't lock drive-backup target image in none mode
  block: Add blk_lock_image
  virtio-blk: Apply lock-mode when realize
  scsi-disk: Apply lock-mode when realize
  scsi-generic: Apply lock-mode when realize
  qdev: Add "lock-mode" to block device options
  ide: Apply lock-mode when initialize
  nvme: Apply lock-mode when initialize
  usb-storage: Apply lock-mode when realize
  pflash: Add "lock-mode" property
  qemu-iotests: 046: Move version detection out from verify_io
  qemu-iotests: 091: Prepare for image lock
  qemu-iotests: 030: Disable image locking when checking test image
  iotests: 087: Disable image locking in cases where file is shared
  iotests: 087: Disable image locking in cases where file is shared
  iotests: 130: Check image info locklessly
  iotests: Disable image locking in 085
  tests: Use null-co:// instead of /dev/null
  qemu-iotests: Add test case 153 for image locking
  ahci: Use shared lock for shared storage migration
  tests/postcopy: Use shared lock for images
  fdc: Add lock-mode qdev properties
  m25p80: Add 'lock-mode' property
  nand: Add 'lock-mode' property
  onenand: Add 'lock-mode' property
  spapr_nvram: Add 'lock-mode' property
  sd: Add 'lock-mode' property

 block.c                        |  52 +++++
 block/block-backend.c          |  18 ++
 block/raw-posix.c              | 318 +++++++++++++++++++++++++++++-
 blockdev.c                     |   5 +
 hw/block/fdc.c                 |  19 +-
 hw/block/m25p80.c              |   8 +
 hw/block/nand.c                |   8 +
 hw/block/nvme.c                |   6 +-
 hw/block/onenand.c             |   7 +
 hw/block/pflash_cfi01.c        |  10 +
 hw/block/pflash_cfi02.c        |   9 +
 hw/block/virtio-blk.c          |   5 +
 hw/core/qdev-properties.c      |  10 +
 hw/ide/core.c                  |  10 +-
 hw/ide/qdev.c                  |   2 +-
 hw/nvram/spapr_nvram.c         |   8 +
 hw/scsi/scsi-disk.c            |   6 +
 hw/scsi/scsi-generic.c         |   7 +
 hw/sd/sd.c                     |   8 +
 hw/usb/dev-storage.c           |   5 +
 include/block/block.h          |   8 +-
 include/block/block_int.h      |   5 +
 include/hw/block/block.h       |   3 +
 include/hw/ide/internal.h      |   3 +-
 include/hw/qdev-properties.h   |   3 +
 include/qemu/osdep.h           |   2 +
 include/sysemu/block-backend.h |   2 +
 qapi/block-core.json           |  18 ++
 qemu-img-cmds.hx               |  44 ++---
 qemu-img.c                     |  92 +++++++--
 qemu-img.texi                  |   3 +
 qemu-io.c                      |  24 ++-
 qemu-nbd.c                     |   7 +-
 qemu-nbd.texi                  |   2 +
 tests/ahci-test.c              |  27 ++-
 tests/drive_del-test.c         |   2 +-
 tests/nvme-test.c              |   2 +-
 tests/postcopy-test.c          |   9 +-
 tests/qemu-iotests/030         |   2 +-
 tests/qemu-iotests/046         |  22 ++-
 tests/qemu-iotests/085         |   6 +-
 tests/qemu-iotests/091         |   9 +-
 tests/qemu-iotests/091.out     |   1 +
 tests/qemu-iotests/130         |   4 +-
 tests/qemu-iotests/130.out     |   4 +-
 tests/qemu-iotests/153         | 197 +++++++++++++++++++
 tests/qemu-iotests/153.out     | 426 +++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group       |   1 +
 tests/usb-hcd-uhci-test.c      |   2 +-
 tests/usb-hcd-xhci-test.c      |   2 +-
 tests/virtio-blk-test.c        |   2 +-
 tests/virtio-scsi-test.c       |   4 +-
 util/osdep.c                   |  29 +++
 53 files changed, 1398 insertions(+), 90 deletions(-)
 create mode 100755 tests/qemu-iotests/153
 create mode 100644 tests/qemu-iotests/153.out

-- 
2.7.4

[Qemu-devel] [PATCH v8 01/36] block: Add flag bits for image locking

[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_SHARED_LOCK or BDRV_O_NO_LOCK to advise an appropriate
locking mode.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index e18233a..cecab7a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -98,7 +98,11 @@ 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_SHARED_LOCK 0x40000 /* lock the image file in shared mode */
+#define BDRV_O_EXCLUSIVE_LOCK 0x80000 /* lock the image file in exclusive mode */
 
+#define BDRV_O_LOCK_MASK   (BDRV_O_NO_LOCK | BDRV_O_SHARED_LOCK | BDRV_O_EXCLUSIVE_LOCK)
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
 
 
-- 
2.7.4

[Qemu-devel] [PATCH v8 02/36] qapi: Add ImageLockMode

[Fam Zheng]

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

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 92193ab..22e8d04 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2754,3 +2754,21 @@
   'data' : { 'parent': 'str',
              '*child': 'str',
              '*node': 'str' } }
+
+##
+# @ImageLockMode:
+#
+# @auto: defer to the block driver to use the least strict mode, based on
+#        the nature of format and read-only flag, and the supported locking
+#        operations of the protocol.
+#
+# @exclusive: always exclusively lock the image.
+#
+# @shared: use a shared lock mode.
+#
+# @nolock: don't lock the image.
+#
+# Since: 2.8
+##
+{ 'enum': 'ImageLockMode',
+  'data': ['auto', 'exclusive', 'shared', 'nolock'] }
-- 
2.7.4

[Qemu-devel] [PATCH v8 03/36] block: Introduce image file locking

[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                   | 52 +++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  4 +++-
 include/block/block_int.h |  5 +++++
 include/hw/block/block.h  |  2 ++
 4 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 493ecf3..9d600df 100644
--- a/block.c
+++ b/block.c
@@ -235,6 +235,7 @@ BlockDriverState *bdrv_new(void)
     notifier_with_return_list_init(&bs->before_write_notifiers);
     bs->refcnt = 1;
     bs->aio_context = qemu_get_aio_context();
+    bs->cur_lock = IMAGE_LOCK_MODE__MAX;
 
     qemu_co_queue_init(&bs->flush_queue);
 
@@ -925,6 +926,48 @@ out:
     g_free(gen_node_name);
 }
 
+ImageLockMode bdrv_lock_mode_from_flags(int flags)
+{
+    if (flags & BDRV_O_NO_LOCK) {
+        return IMAGE_LOCK_MODE_NOLOCK;
+    } else if (flags & BDRV_O_SHARED_LOCK) {
+        return IMAGE_LOCK_MODE_SHARED;
+    } else if (flags & BDRV_O_EXCLUSIVE_LOCK) {
+        return IMAGE_LOCK_MODE_EXCLUSIVE;
+    } else {
+        return IMAGE_LOCK_MODE_AUTO;
+    }
+}
+
+ImageLockMode bdrv_get_lock_mode(BlockDriverState *bs)
+{
+    return bs->cur_lock;
+}
+
+int bdrv_set_lock_mode(BlockDriverState *bs, ImageLockMode mode)
+{
+    int ret;
+
+    if (bs->cur_lock == mode) {
+        return 0;
+    } else if (!bs->drv) {
+        return -ENOMEDIUM;
+    } else if (!bs->drv->bdrv_lockf) {
+        if (bs->file) {
+            return bdrv_set_lock_mode(bs->file->bs, mode);
+        }
+        return 0;
+    }
+    ret = bs->drv->bdrv_lockf(bs, mode);
+    if (ret == -ENOTSUP) {
+        /* Handle it the same way as !bs->drv->bdrv_lockf */
+        ret = 0;
+    } else if (ret == 0) {
+        bs->cur_lock = mode;
+    }
+    return ret;
+}
+
 static QemuOptsList bdrv_runtime_opts = {
     .name = "bdrv_common",
     .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
@@ -1076,6 +1119,10 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
         goto free_and_fail;
     }
 
+    if (open_flags & BDRV_O_INACTIVE) {
+        open_flags = (open_flags & ~BDRV_O_LOCK_MASK) & BDRV_O_NO_LOCK;
+    }
+
     ret = refresh_total_sectors(bs, bs->total_sectors);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not refresh total sector count");
@@ -2273,6 +2320,7 @@ static void bdrv_close(BlockDriverState *bs)
     if (bs->drv) {
         BdrvChild *child, *next;
 
+        bdrv_set_lock_mode(bs, IMAGE_LOCK_MODE_NOLOCK);
         bs->drv->bdrv_close(bs);
         bs->drv = NULL;
 
@@ -3188,6 +3236,9 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
         error_setg_errno(errp, -ret, "Could not refresh total sector count");
         return;
     }
+    if (bs->cur_lock != IMAGE_LOCK_MODE__MAX) {
+        bdrv_set_lock_mode(bs, bs->cur_lock);
+    }
 }
 
 void bdrv_invalidate_cache_all(Error **errp)
@@ -3230,6 +3281,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
     }
 
     if (setting_flag) {
+        ret = bdrv_set_lock_mode(bs, IMAGE_LOCK_MODE_NOLOCK);
         bs->open_flags |= BDRV_O_INACTIVE;
     }
     return 0;
diff --git a/include/block/block.h b/include/block/block.h
index cecab7a..828ab14 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -272,7 +272,8 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
 BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
                                     BlockDriverState *bs);
 BlockDriverState *bdrv_find_base(BlockDriverState *bs);
-
+ImageLockMode bdrv_lock_mode_from_flags(int flags);
+ImageLockMode bdrv_get_lock_mode(BlockDriverState *bs);
 
 typedef struct BdrvCheckResult {
     int corruptions;
@@ -529,5 +530,6 @@ void bdrv_drained_end(BlockDriverState *bs);
 void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
                     Error **errp);
 void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
+int bdrv_set_lock_mode(BlockDriverState *bs, ImageLockMode mode);
 
 #endif
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3e79228..486e1ea 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -319,6 +319,10 @@ struct BlockDriver {
                            Error **errp);
     void (*bdrv_del_child)(BlockDriverState *parent, BdrvChild *child,
                            Error **errp);
+    /**
+     * Lock/unlock the image.
+     */
+    int (*bdrv_lockf)(BlockDriverState *bs, ImageLockMode mode);
 
     QLIST_ENTRY(BlockDriver) list;
 };
@@ -528,6 +532,7 @@ struct BlockDriverState {
     unsigned io_plug_disabled;
 
     int quiesce_counter;
+    ImageLockMode cur_lock;
 };
 
 struct BlockBackendRootState {
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index df9d207..8c04469 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -12,11 +12,13 @@
 #define HW_BLOCK_H
 
 #include "qemu-common.h"
+#include "block/block.h"
 
 /* Configuration */
 
 typedef struct BlockConf {
     BlockBackend *blk;
+    ImageLockMode lock_mode;
     uint16_t physical_block_size;
     uint16_t logical_block_size;
     uint16_t min_io_size;
-- 
2.7.4

[Qemu-devel] [PATCH v8 04/36] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Fam Zheng]

They are wrappers of POSIX fcntl "file private locking".

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/qemu/osdep.h |  2 ++
 util/osdep.c         | 29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 9e9fa61..f773f49 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -286,6 +286,8 @@ int qemu_close(int fd);
 #ifndef _WIN32
 int qemu_dup(int fd);
 #endif
+int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
+int qemu_unlock_fd(int fd, int64_t start, int64_t len);
 
 #if defined(__HAIKU__) && defined(__i386__)
 #define FMT_pid "%ld"
diff --git a/util/osdep.c b/util/osdep.c
index 06fb1cf..b85a490 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -140,6 +140,35 @@ static int qemu_parse_fdset(const char *param)
 {
     return qemu_parse_fd(param);
 }
+
+static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
+{
+#ifdef F_OFD_SETLK
+    int ret;
+    struct flock fl = {
+        .l_whence = SEEK_SET,
+        .l_start  = start,
+        .l_len    = len,
+        .l_type   = fl_type,
+    };
+    do {
+        ret = fcntl(fd, F_OFD_SETLK, &fl);
+    } while (ret == -1 && errno == EINTR);
+    return ret == -1 ? -errno : 0;
+#else
+    return -ENOTSUP;
+#endif
+}
+
+int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive)
+{
+    return qemu_lock_fcntl(fd, start, len, exclusive ? F_WRLCK : F_RDLCK);
+}
+
+int qemu_unlock_fd(int fd, int64_t start, int64_t len)
+{
+    return qemu_lock_fcntl(fd, start, len, F_UNLCK);
+}
 #endif
 
 /*
-- 
2.7.4

[Qemu-devel] [PATCH v8 05/36] raw-posix: Add image locking support

[Fam Zheng]

virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
the intervene.

Both file and host device protocols are covered.

The complication is with reopen. We have three different locking states,
namely "unlocked", "shared locked" and "exclusively locked".

When we reopen, the new fd may need a new locking mode. Moving away to or from
exclusive is a bit tricky because we cannot do it atomically. This patch solves
it by dup() s->fd to s->lock_fd and avoid close(), so that there isn't a racy
window where we drop the lock on one fd before acquiring the exclusive lock on
the other.

To make the logic easier to manage, and allow better reuse, the code is
internally organized by state transition table (old_lock -> new_lock).

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

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6ed7547..22de242 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -133,6 +133,7 @@ do { \
 
 typedef struct BDRVRawState {
     int fd;
+    int lock_fd;
     int type;
     int open_flags;
     size_t buf_align;
@@ -149,6 +150,7 @@ typedef struct BDRVRawState {
 
 typedef struct BDRVRawReopenState {
     int fd;
+    int lock_fd;
     int open_flags;
 } BDRVRawReopenState;
 
@@ -367,6 +369,43 @@ static void raw_parse_flags(int bdrv_flags, int *open_flags)
     }
 }
 
+static int raw_lock_fd(int fd, ImageLockMode mode)
+{
+    assert(fd >= 0);
+    /* Locking byte 1 avoids interfereing with virtlockd. */
+    switch (mode) {
+    case IMAGE_LOCK_MODE_EXCLUSIVE:
+        return qemu_lock_fd(fd, 1, 1, true);
+    case IMAGE_LOCK_MODE_SHARED:
+        return qemu_lock_fd(fd, 1, 1, false);
+    case IMAGE_LOCK_MODE_NOLOCK:
+        return qemu_unlock_fd(fd, 1, 1);
+    default:
+        abort();
+    }
+}
+
+static int raw_lockf(BlockDriverState *bs, ImageLockMode mode)
+{
+    BDRVRawState *s = bs->opaque;
+
+    if (s->lock_fd < 0) {
+        if (mode == IMAGE_LOCK_MODE_NOLOCK) {
+            return 0;
+        }
+        s->lock_fd = qemu_dup(s->fd);
+        if (s->lock_fd < 0) {
+            return s->lock_fd;
+        }
+    }
+    if (mode == IMAGE_LOCK_MODE_AUTO) {
+        mode = bdrv_get_flags(bs) & BDRV_O_RDWR ?
+               IMAGE_LOCK_MODE_EXCLUSIVE :
+               IMAGE_LOCK_MODE_SHARED;
+    }
+    return raw_lock_fd(s->lock_fd, mode);
+}
+
 #ifdef CONFIG_LINUX_AIO
 static bool raw_use_aio(int bdrv_flags)
 {
@@ -433,6 +472,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     raw_parse_flags(bdrv_flags, &s->open_flags);
 
     s->fd = -1;
+    s->lock_fd = -1;
     fd = qemu_open(filename, s->open_flags, 0644);
     if (fd < 0) {
         ret = -errno;
@@ -529,6 +569,268 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     return raw_open_common(bs, options, flags, 0, errp);
 }
 
+typedef enum {
+    RAW_REOPEN_PREPARE,
+    RAW_REOPEN_COMMIT,
+    RAW_REOPEN_ABORT
+} RawReopenOperation;
+
+typedef int (*RawReopenFunc)(BDRVReopenState *state,
+                             RawReopenOperation op,
+                             ImageLockMode old_lock,
+                             ImageLockMode new_lock,
+                             Error **errp);
+
+static int
+raw_reopen_identical(BDRVReopenState *state,
+                     RawReopenOperation op,
+                     ImageLockMode old_lock,
+                     ImageLockMode new_lock,
+                     Error **errp)
+{
+    assert(old_lock == new_lock);
+    return 0;
+}
+
+static int
+raw_reopen_from_unlock(BDRVReopenState *state,
+                       RawReopenOperation op,
+                       ImageLockMode old_lock,
+                       ImageLockMode new_lock,
+                       Error **errp)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+    int ret = 0;
+
+    assert(old_lock != new_lock);
+    assert(old_lock == IMAGE_LOCK_MODE_NOLOCK);
+    switch (op) {
+    case RAW_REOPEN_PREPARE:
+        ret = raw_lock_fd(raw_s->lock_fd, new_lock);
+        if (ret) {
+            error_setg_errno(errp, -ret, "Failed to lock new fd %d", raw_s->lock_fd);
+        }
+        break;
+    case RAW_REOPEN_COMMIT:
+    case RAW_REOPEN_ABORT:
+        break;
+    }
+
+    return ret;
+}
+
+static int
+raw_reopen_to_unlock(BDRVReopenState *state,
+                     RawReopenOperation op,
+                     ImageLockMode old_lock,
+                     ImageLockMode new_lock,
+                     Error **errp)
+{
+    BDRVRawState *s = state->bs->opaque;
+    int ret = 0;
+
+    assert(old_lock != new_lock);
+    assert(new_lock == IMAGE_LOCK_MODE_NOLOCK);
+    switch (op) {
+    case RAW_REOPEN_PREPARE:
+        break;
+    case RAW_REOPEN_COMMIT:
+        if (s->lock_fd >= 0) {
+            qemu_close(s->lock_fd);
+            s->lock_fd = -1;
+        }
+        break;
+    case RAW_REOPEN_ABORT:
+        break;
+    }
+
+    return ret;
+}
+
+static int
+raw_reopen_upgrade(BDRVReopenState *state,
+                   RawReopenOperation op,
+                   ImageLockMode old_lock,
+                   ImageLockMode new_lock,
+                   Error **errp)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+    BDRVRawState *s = state->bs->opaque;
+    int ret = 0, ret2;
+
+    assert(old_lock == IMAGE_LOCK_MODE_SHARED);
+    assert(new_lock == IMAGE_LOCK_MODE_EXCLUSIVE);
+    switch (op) {
+    case RAW_REOPEN_PREPARE:
+        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
+        if (ret) {
+            error_setg_errno(errp, -ret, "Failed to lock new fd (shared)");
+            break;
+        }
+        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_NOLOCK);
+        if (ret) {
+            error_setg_errno(errp, -ret, "Failed to unlock old fd");
+            goto restore;
+        }
+        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_EXCLUSIVE);
+        if (ret) {
+            error_setg_errno(errp, -ret, "Failed to lock new fd (exclusive)");
+            goto restore;
+        }
+        break;
+    case RAW_REOPEN_COMMIT:
+        break;
+    case RAW_REOPEN_ABORT:
+        raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
+        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);
+        if (ret) {
+            error_report("Failed to restore lock on old fd");
+        }
+        break;
+    }
+
+    return ret;
+restore:
+    ret2 = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);
+    if (ret2) {
+        error_report("Failed to restore old lock");
+    }
+    return ret;
+}
+
+static int
+raw_reopen_downgrade(BDRVReopenState *state,
+                     RawReopenOperation op,
+                     ImageLockMode old_lock,
+                     ImageLockMode new_lock,
+                     Error **errp)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+    BDRVRawState *s = state->bs->opaque;
+    int ret = 0;
+
+    assert(old_lock == IMAGE_LOCK_MODE_EXCLUSIVE);
+    assert(new_lock == IMAGE_LOCK_MODE_SHARED);
+    switch (op) {
+    case RAW_REOPEN_PREPARE:
+        break;
+    case RAW_REOPEN_COMMIT:
+        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);
+        if (ret) {
+            error_report("Failed to downgrade old lock");
+            break;
+        }
+        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
+        if (ret) {
+            error_report("Failed to lock new fd (shared)");
+            break;
+        }
+        break;
+    case RAW_REOPEN_ABORT:
+        break;
+    }
+
+    return ret;
+}
+
+/**
+ * Transactionally moving between three possible locking states is tricky and
+ * must be done carefully. That is mostly because downgrading an exclusive lock
+ * to shared or unlocked is not guaranteed to be revertable. As a result, in
+ * such cases we have to defer the downgraing to "commit", given that no revert
+ * will happen after that point, and that downgrading a lock should never fail.
+ *
+ * On the other hand, upgrading a lock (e.g. from unlocked or shared to
+ * exclusive lock) must happen in "prepare" because it may fail.
+ *
+ * Manage the operation matrix with this state transition table to make
+ * fulfulling above conditions easier.
+ */
+static const struct RawReopenFuncRecord {
+    ImageLockMode old_lock;
+    ImageLockMode new_lock;
+    RawReopenFunc func;
+    bool need_lock_fd;
+} reopen_functions[] = {
+    {IMAGE_LOCK_MODE_NOLOCK, IMAGE_LOCK_MODE_NOLOCK, raw_reopen_identical, false},
+    {IMAGE_LOCK_MODE_NOLOCK, IMAGE_LOCK_MODE_SHARED, raw_reopen_from_unlock, true},
+    {IMAGE_LOCK_MODE_NOLOCK, IMAGE_LOCK_MODE_EXCLUSIVE, raw_reopen_from_unlock, true},
+    {IMAGE_LOCK_MODE_SHARED, IMAGE_LOCK_MODE_NOLOCK, raw_reopen_to_unlock, false},
+    {IMAGE_LOCK_MODE_SHARED, IMAGE_LOCK_MODE_SHARED, raw_reopen_identical, false},
+    {IMAGE_LOCK_MODE_SHARED, IMAGE_LOCK_MODE_EXCLUSIVE, raw_reopen_upgrade, true},
+    {IMAGE_LOCK_MODE_EXCLUSIVE, IMAGE_LOCK_MODE_NOLOCK, raw_reopen_to_unlock, false},
+    {IMAGE_LOCK_MODE_EXCLUSIVE, IMAGE_LOCK_MODE_SHARED, raw_reopen_downgrade, true},
+    {IMAGE_LOCK_MODE_EXCLUSIVE, IMAGE_LOCK_MODE_EXCLUSIVE, raw_reopen_identical, false},
+};
+
+static int raw_reopen_handle_lock(BDRVReopenState *state,
+                                  RawReopenOperation op,
+                                  Error **errp)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+    BDRVRawState *s = state->bs->opaque;
+    ImageLockMode old_lock, new_lock;
+    const struct RawReopenFuncRecord *rec;
+    int ret;
+
+    old_lock = bdrv_get_lock_mode(state->bs);
+    new_lock = bdrv_lock_mode_from_flags(state->flags);
+
+    if (old_lock == IMAGE_LOCK_MODE__MAX) {
+        /* bs was not locked, leave it unlocked. */
+        old_lock = new_lock = IMAGE_LOCK_MODE_NOLOCK;
+    }
+
+    if (old_lock == IMAGE_LOCK_MODE_AUTO) {
+        old_lock = bdrv_get_flags(state->bs) & BDRV_O_RDWR ?
+                   IMAGE_LOCK_MODE_EXCLUSIVE : IMAGE_LOCK_MODE_SHARED;
+    }
+
+    if (new_lock == IMAGE_LOCK_MODE_AUTO) {
+        new_lock = state->flags & BDRV_O_RDWR ?
+                   IMAGE_LOCK_MODE_EXCLUSIVE : IMAGE_LOCK_MODE_SHARED;
+    }
+
+    for (rec = &reopen_functions[0];
+         rec < &reopen_functions[ARRAY_SIZE(reopen_functions)];
+         rec++) {
+        if (rec->old_lock == old_lock && rec->new_lock == new_lock) {
+            break;
+        }
+    }
+    assert(rec != &reopen_functions[ARRAY_SIZE(reopen_functions)]);
+
+    switch (op) {
+    case RAW_REOPEN_PREPARE:
+        if (rec->need_lock_fd) {
+            ret = qemu_dup(raw_s->fd);
+            if (ret < 0) {
+                error_setg_errno(errp, -ret, "Failed to dup new fd");
+                return ret;
+            }
+            raw_s->lock_fd = ret;
+        }
+        return rec->func(state, op, old_lock, new_lock, errp);
+    case RAW_REOPEN_COMMIT:
+        rec->func(state, op, old_lock, new_lock, errp);
+        if (rec->need_lock_fd) {
+            if (s->lock_fd >= 0) {
+                qemu_close(s->lock_fd);
+            }
+            s->lock_fd = raw_s->lock_fd;
+        }
+        break;
+    case RAW_REOPEN_ABORT:
+        rec->func(state, op, old_lock, new_lock, errp);
+        if (rec->need_lock_fd && raw_s->lock_fd >= 0) {
+            qemu_close(raw_s->lock_fd);
+            raw_s->lock_fd = -1;
+        }
+        break;
+    }
+    return 0;
+}
+
 static int raw_reopen_prepare(BDRVReopenState *state,
                               BlockReopenQueue *queue, Error **errp)
 {
@@ -607,6 +909,10 @@ static int raw_reopen_prepare(BDRVReopenState *state,
         }
     }
 
+    if (!ret) {
+        ret = raw_reopen_handle_lock(state, RAW_REOPEN_PREPARE, errp);
+    }
+
     return ret;
 }
 
@@ -617,6 +923,8 @@ static void raw_reopen_commit(BDRVReopenState *state)
 
     s->open_flags = raw_s->open_flags;
 
+    raw_reopen_handle_lock(state, RAW_REOPEN_COMMIT, NULL);
+
     qemu_close(s->fd);
     s->fd = raw_s->fd;
 
@@ -634,6 +942,8 @@ static void raw_reopen_abort(BDRVReopenState *state)
         return;
     }
 
+    raw_reopen_handle_lock(state, RAW_REOPEN_ABORT, NULL);
+
     if (raw_s->fd >= 0) {
         qemu_close(raw_s->fd);
         raw_s->fd = -1;
@@ -1321,6 +1631,10 @@ static void raw_close(BlockDriverState *bs)
         qemu_close(s->fd);
         s->fd = -1;
     }
+    if (s->lock_fd >= 0) {
+        qemu_close(s->lock_fd);
+        s->lock_fd = -1;
+    }
 }
 
 static int raw_truncate(BlockDriverState *bs, int64_t offset)
@@ -1874,7 +2188,7 @@ BlockDriver bdrv_file = {
     .bdrv_get_info = raw_get_info,
     .bdrv_get_allocated_file_size
                         = raw_get_allocated_file_size,
-
+    .bdrv_lockf = raw_lockf,
     .create_opts = &raw_create_opts,
 };
 
@@ -2324,6 +2638,8 @@ static BlockDriver bdrv_host_device = {
 #ifdef __linux__
     .bdrv_aio_ioctl     = hdev_aio_ioctl,
 #endif
+
+    .bdrv_lockf = raw_lockf,
 };
 
 #if defined(__linux__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
-- 
2.7.4

[Qemu-devel] [PATCH v8 06/36] qemu-io: Add "-L" option for BDRV_O_NO_LOCK

[Fam Zheng]

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

diff --git a/qemu-io.c b/qemu-io.c
index db129ea..5c14eba 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -108,6 +108,7 @@ static void open_help(void)
 " -r, -- open file read-only\n"
 " -s, -- use snapshot file\n"
 " -n, -- disable host cache, short for -t none\n"
+" -L, -- disable image locking\n"
 " -k, -- use kernel AIO implementation (on Linux only)\n"
 " -t, -- use the given cache mode for the image\n"
 " -d, -- use the given discard mode for the image\n"
@@ -124,7 +125,7 @@ static const cmdinfo_t open_cmd = {
     .argmin     = 1,
     .argmax     = -1,
     .flags      = CMD_NOFILE_OK,
-    .args       = "[-rsnk] [-t cache] [-d discard] [-o options] [path]",
+    .args       = "[-rsnLk] [-t cache] [-d discard] [-o options] [path]",
     .oneline    = "open the file specified by path",
     .help       = open_help,
 };
@@ -143,12 +144,13 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
 {
     int flags = BDRV_O_UNMAP;
     int readonly = 0;
+    bool nolock = false;
     bool writethrough = true;
     int c;
     QemuOpts *qopts;
     QDict *opts;
 
-    while ((c = getopt(argc, argv, "snro:kt:d:")) != -1) {
+    while ((c = getopt(argc, argv, "snrLo:kt:d:")) != -1) {
         switch (c) {
         case 's':
             flags |= BDRV_O_SNAPSHOT;
@@ -177,6 +179,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
                 return 0;
             }
             break;
+        case 'L':
+            nolock = true;
+            break;
         case 'o':
             if (imageOpts) {
                 printf("--image-opts and 'open -o' are mutually exclusive\n");
@@ -198,6 +203,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);
@@ -436,13 +445,15 @@ static QemuOptsList file_opts = {
 int main(int argc, char **argv)
 {
     int readonly = 0;
-    const char *sopt = "hVc:d:f:rsnmkt:T:";
+    const char *sopt = "hVc:d:f:rLsnmkt:T:";
+    bool nolock = false;
     const struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
         { "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' },
@@ -501,6 +512,9 @@ int main(int argc, char **argv)
         case 'r':
             readonly = 1;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case 'm':
             qemuio_misalign = true;
             break;
@@ -586,6 +600,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.7.4

[Qemu-devel] [PATCH v8 07/36] qemu-img: Add "-L" option to sub commands

[Fam Zheng]

If specified, BDRV_O_NO_LOCK flag will be set when opening the image.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 72 insertions(+), 19 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index ceffefe..e8d0e78 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -616,6 +616,7 @@ static int img_check(int argc, char **argv)
     ImageCheck *check;
     bool quiet = false;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     output = NULL;
@@ -632,7 +633,7 @@ static int img_check(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:r:T:q",
+        c = getopt_long(argc, argv, "hf:r:T:qL",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -666,6 +667,9 @@ static int img_check(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -699,6 +703,7 @@ static int img_check(int argc, char **argv)
         return 1;
     }
 
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", cache);
@@ -819,6 +824,7 @@ static int img_commit(int argc, char **argv)
     Error *local_err = NULL;
     CommonBlockJobCBInfo cbi;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     cache = BDRV_DEFAULT_CACHE;
@@ -830,7 +836,7 @@ static int img_commit(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:ht:b:dpq",
+        c = getopt_long(argc, argv, "f:ht:b:dpqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -860,6 +866,9 @@ static int img_commit(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -891,6 +900,7 @@ static int img_commit(int argc, char **argv)
     }
 
     flags = BDRV_O_RDWR | BDRV_O_UNMAP;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -1149,6 +1159,7 @@ static int img_compare(int argc, char **argv)
     int c, pnum;
     uint64_t progress_base;
     bool image_opts = false;
+    bool nolock = false;
 
     cache = BDRV_DEFAULT_CACHE;
     for (;;) {
@@ -1158,7 +1169,7 @@ static int img_compare(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:F:T:pqs",
+        c = getopt_long(argc, argv, "hf:F:T:pqsL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -1186,6 +1197,9 @@ static int img_compare(int argc, char **argv)
         case 's':
             strict = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -1223,7 +1237,7 @@ static int img_compare(int argc, char **argv)
     /* Initialize before goto out */
     qemu_progress_init(progress, 2.0);
 
-    flags = 0;
+    flags = nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", cache);
@@ -1774,6 +1788,7 @@ static int img_convert(int argc, char **argv)
     QemuOpts *sn_opts = NULL;
     ImgConvertState state;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     out_fmt = "raw";
@@ -1789,7 +1804,7 @@ static int img_convert(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qn",
+        c = getopt_long(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qnL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -1878,6 +1893,9 @@ static int img_convert(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case 'n':
             skip_create = 1;
             break;
@@ -1926,7 +1944,7 @@ static int img_convert(int argc, char **argv)
         goto out;
     }
 
-    src_flags = 0;
+    src_flags = nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", src_cache);
@@ -2076,6 +2094,7 @@ static int img_convert(int argc, char **argv)
     }
 
     flags = min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -2256,12 +2275,14 @@ static gboolean str_equal_func(gconstpointer a, gconstpointer b)
 static ImageInfoList *collect_image_info_list(bool image_opts,
                                               const char *filename,
                                               const char *fmt,
-                                              bool chain)
+                                              bool chain,
+                                              bool nolock)
 {
     ImageInfoList *head = NULL;
     ImageInfoList **last = &head;
     GHashTable *filenames;
     Error *err = NULL;
+    int flags;
 
     filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
 
@@ -2278,8 +2299,9 @@ static ImageInfoList *collect_image_info_list(bool image_opts,
         }
         g_hash_table_insert(filenames, (gpointer)filename, NULL);
 
-        blk = img_open(image_opts, filename, fmt,
-                       BDRV_O_NO_BACKING | BDRV_O_NO_IO, false, false);
+        flags = BDRV_O_NO_BACKING | BDRV_O_NO_IO;
+        flags |= nolock ? BDRV_O_NO_LOCK : 0;
+        blk = img_open(image_opts, filename, fmt, flags, false, false);
         if (!blk) {
             goto err;
         }
@@ -2331,6 +2353,7 @@ static int img_info(int argc, char **argv)
     const char *filename, *fmt, *output;
     ImageInfoList *list;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     output = NULL;
@@ -2345,7 +2368,7 @@ static int img_info(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:h",
+        c = getopt_long(argc, argv, "f:hL",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -2358,6 +2381,9 @@ static int img_info(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OUTPUT:
             output = optarg;
             break;
@@ -2397,7 +2423,7 @@ static int img_info(int argc, char **argv)
         return 1;
     }
 
-    list = collect_image_info_list(image_opts, filename, fmt, chain);
+    list = collect_image_info_list(image_opts, filename, fmt, chain, nolock);
     if (!list) {
         return 1;
     }
@@ -2543,6 +2569,8 @@ static int img_map(int argc, char **argv)
     MapEntry curr = { .length = 0 }, next;
     int ret = 0;
     bool image_opts = false;
+    bool nolock = false;
+    int flags;
 
     fmt = NULL;
     output = NULL;
@@ -2556,7 +2584,7 @@ static int img_map(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:h",
+        c = getopt_long(argc, argv, "f:hL",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -2569,6 +2597,9 @@ static int img_map(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OUTPUT:
             output = optarg;
             break;
@@ -2605,7 +2636,8 @@ static int img_map(int argc, char **argv)
         return 1;
     }
 
-    blk = img_open(image_opts, filename, fmt, 0, false, false);
+    flags = nolock ? BDRV_O_NO_LOCK : 0;
+    blk = img_open(image_opts, filename, fmt, flags, false, false);
     if (!blk) {
         return 1;
     }
@@ -2668,6 +2700,7 @@ static int img_snapshot(int argc, char **argv)
     bool quiet = false;
     Error *err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     bdrv_oflags = BDRV_O_RDWR;
     /* Parse commandline parameters */
@@ -2678,7 +2711,7 @@ static int img_snapshot(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "la:c:d:hq",
+        c = getopt_long(argc, argv, "la:c:d:hqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -2723,6 +2756,9 @@ static int img_snapshot(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -2748,6 +2784,7 @@ static int img_snapshot(int argc, char **argv)
         return 1;
     }
 
+    bdrv_oflags |= nolock ? BDRV_O_NO_LOCK : 0;
     /* Open the image */
     blk = img_open(image_opts, filename, NULL, bdrv_oflags, false, quiet);
     if (!blk) {
@@ -2817,6 +2854,7 @@ static int img_rebase(int argc, char **argv)
     bool quiet = false;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     /* Parse commandline parameters */
     fmt = NULL;
@@ -2831,7 +2869,7 @@ static int img_rebase(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:F:b:upt:T:q",
+        c = getopt_long(argc, argv, "hf:F:b:upt:T:qL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -2862,6 +2900,9 @@ static int img_rebase(int argc, char **argv)
         case 'T':
             src_cache = optarg;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case 'q':
             quiet = true;
             break;
@@ -2901,6 +2942,7 @@ static int img_rebase(int argc, char **argv)
     qemu_progress_print(0, 100);
 
     flags = BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0);
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -3162,6 +3204,8 @@ static int img_resize(int argc, char **argv)
     bool quiet = false;
     BlockBackend *blk = NULL;
     QemuOpts *param;
+    int flags;
+    bool nolock = false;
 
     static QemuOptsList resize_options = {
         .name = "resize_options",
@@ -3196,7 +3240,7 @@ static int img_resize(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:hq",
+        c = getopt_long(argc, argv, "f:hqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -3212,6 +3256,9 @@ static int img_resize(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -3263,8 +3310,9 @@ static int img_resize(int argc, char **argv)
     n = qemu_opt_get_size(param, BLOCK_OPT_SIZE, 0);
     qemu_opts_del(param);
 
-    blk = img_open(image_opts, filename, fmt,
-                   BDRV_O_RDWR, false, quiet);
+    flags = BDRV_O_RDWR;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
+    blk = img_open(image_opts, filename, fmt, flags, false, quiet);
     if (!blk) {
         ret = -1;
         goto out;
@@ -3325,6 +3373,7 @@ static int img_amend(int argc, char **argv)
     BlockBackend *blk = NULL;
     BlockDriverState *bs = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     cache = BDRV_DEFAULT_CACHE;
     for (;;) {
@@ -3334,7 +3383,7 @@ static int img_amend(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "ho:f:t:pq",
+        c = getopt_long(argc, argv, "ho:f:t:pqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -3371,6 +3420,9 @@ static int img_amend(int argc, char **argv)
             case 'q':
                 quiet = true;
                 break;
+            case 'L':
+                nolock = true;
+                break;
             case OPTION_OBJECT:
                 opts = qemu_opts_parse_noisily(&qemu_object_opts,
                                                optarg, true);
@@ -3416,6 +3468,7 @@ static int img_amend(int argc, char **argv)
     }
 
     flags = BDRV_O_RDWR;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
-- 
2.7.4

[Qemu-devel] [PATCH v8 08/36] qemu-img: Update documentation of "-L" option

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img-cmds.hx | 44 ++++++++++++++++++++++----------------------
 qemu-img.c       |  1 +
 qemu-img.texi    |  3 +++
 3 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index f054599..eaca454 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -16,33 +16,33 @@ STEXI
 ETEXI
 
 DEF("check", img_check,
-    "check [-q] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
+    "check [-q] [-L] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
 STEXI
-@item check [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
+@item check [--object @var{objectdef}] [--image-opts] [-q] [-L] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
 ETEXI
 
 DEF("create", img_create,
-    "create [-q] [--object objectdef] [--image-opts] [-f fmt] [-o options] filename [size]")
+    "create [-q] [-L] [--object objectdef] [--image-opts] [-f fmt] [-o options] filename [size]")
 STEXI
-@item create [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
+@item create [--object @var{objectdef}] [--image-opts] [-q] [-L] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
 ETEXI
 
 DEF("commit", img_commit,
-    "commit [-q] [--object objectdef] [--image-opts] [-f fmt] [-t cache] [-b base] [-d] [-p] filename")
+    "commit [-q] [-L] [--object objectdef] [--image-opts] [-f fmt] [-t cache] [-b base] [-d] [-p] filename")
 STEXI
-@item commit [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-t @var{cache}] [-b @var{base}] [-d] [-p] @var{filename}
+@item commit [--object @var{objectdef}] [--image-opts] [-q] [-L] [-f @var{fmt}] [-t @var{cache}] [-b @var{base}] [-d] [-p] @var{filename}
 ETEXI
 
 DEF("compare", img_compare,
-    "compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-s] filename1 filename2")
+    "compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-L] [-s] filename1 filename2")
 STEXI
-@item compare [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-F @var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] @var{filename1} @var{filename2}
+@item compare [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-F @var{fmt}] [-T @var{src_cache}] [-p] [-q] [-L] [-s] @var{filename1} @var{filename2}
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
+    "convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-L] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
 STEXI
-@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-L] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("dd", img_dd,
@@ -52,38 +52,38 @@ STEXI
 ETEXI
 
 DEF("info", img_info,
-    "info [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [--backing-chain] filename")
+    "info [--object objectdef] [--image-opts] [-f fmt] [-L] [--output=ofmt] [--backing-chain] filename")
 STEXI
-@item info [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
+@item info [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-L] [--output=@var{ofmt}] [--backing-chain] @var{filename}
 ETEXI
 
 DEF("map", img_map,
-    "map [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] filename")
+    "map [--object objectdef] [--image-opts] [-f fmt] [-L] [--output=ofmt] filename")
 STEXI
-@item map [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] @var{filename}
+@item map [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-L] [--output=@var{ofmt}] @var{filename}
 ETEXI
 
 DEF("snapshot", img_snapshot,
-    "snapshot [--object objectdef] [--image-opts] [-q] [-l | -a snapshot | -c snapshot | -d snapshot] filename")
+    "snapshot [--object objectdef] [--image-opts] [-q] [-L] [-l | -a snapshot | -c snapshot | -d snapshot] filename")
 STEXI
-@item snapshot [--object @var{objectdef}] [--image-opts] [-q] [-l | -a @var{snapshot} | -c @var{snapshot} | -d @var{snapshot}] @var{filename}
+@item snapshot [--object @var{objectdef}] [--image-opts] [-q] [-L] [-l | -a @var{snapshot} | -c @var{snapshot} | -d @var{snapshot}] @var{filename}
 ETEXI
 
 DEF("rebase", img_rebase,
-    "rebase [--object objectdef] [--image-opts] [-q] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
+    "rebase [--object objectdef] [--image-opts] [-q] [-L] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
 STEXI
-@item rebase [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
+@item rebase [--object @var{objectdef}] [--image-opts] [-q] [-L] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
 ETEXI
 
 DEF("resize", img_resize,
-    "resize [--object objectdef] [--image-opts] [-q] filename [+ | -]size")
+    "resize [--object objectdef] [--image-opts] [-q] [-L] filename [+ | -]size")
 STEXI
-@item resize [--object @var{objectdef}] [--image-opts] [-q] @var{filename} [+ | -]@var{size}
+@item resize [--object @var{objectdef}] [--image-opts] [-q] [-L] @var{filename} [+ | -]@var{size}
 ETEXI
 
 DEF("amend", img_amend,
-    "amend [--object objectdef] [--image-opts] [-p] [-q] [-f fmt] [-t cache] -o options filename")
+    "amend [--object objectdef] [--image-opts] [-p] [-q] [-L] [-f fmt] [-t cache] -o options filename")
 STEXI
-@item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
+@item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-L] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
 @end table
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index e8d0e78..36b0af5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -141,6 +141,7 @@ static void QEMU_NORETURN help(void)
            "  '-h' with or without a command shows this help and lists the supported formats\n"
            "  '-p' show progress of command (only certain commands)\n"
            "  '-q' use Quiet mode - do not print any output (except errors)\n"
+           "  '-L' don't lock the image\n"
            "  '-S' indicates the consecutive number of bytes (defaults to 4k) that must\n"
            "       contain only zeros for qemu-img to create a sparse image during\n"
            "       conversion. If the number of bytes is 0, the source will not be scanned for\n"
diff --git a/qemu-img.texi b/qemu-img.texi
index 174aae3..300ccf8 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -88,6 +88,9 @@ progress is reported when the process receives a @code{SIGUSR1} signal.
 @item -q
 Quiet mode - do not print any output (except errors). There's no progress bar
 in case both @var{-q} and @var{-p} options are used.
+@item -L
+disables image locking. The image will not be locked, other processes can
+access and lock this image while we are using it.
 @item -S @var{size}
 indicates the consecutive number of bytes that must contain only zeros
 for qemu-img to create a sparse image during conversion. This value is rounded
-- 
2.7.4

[Qemu-devel] [PATCH v8 09/36] qemu-nbd: Add "--no-lock/-L" option

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 qemu-nbd.c    | 7 ++++++-
 qemu-nbd.texi | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 99297a5..6585b2c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -102,6 +102,7 @@ static void usage(const char *name)
 "Block device options:\n"
 "  -f, --format=FORMAT       set image format (raw, qcow2, ...)\n"
 "  -r, --read-only           export read-only\n"
+"  -L, --no-lock             disable image locking\n"
 "  -s, --snapshot            use FILE as an external snapshot, create a temporary\n"
 "                            file with backing_file=FILE, redirect the write to\n"
 "                            the temporary one\n"
@@ -474,7 +475,7 @@ int main(int argc, char **argv)
     off_t fd_size;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:";
+    const char *sopt = "hVb:o:p:rsnLP:c:dvk:e:f:tl:x:T:";
     struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -483,6 +484,7 @@ int main(int argc, char **argv)
         { "socket", required_argument, NULL, 'k' },
         { "offset", required_argument, NULL, 'o' },
         { "read-only", no_argument, NULL, 'r' },
+        { "no-lock", no_argument, NULL, 'L' },
         { "partition", required_argument, NULL, 'P' },
         { "connect", required_argument, NULL, 'c' },
         { "disconnect", no_argument, NULL, 'd' },
@@ -638,6 +640,9 @@ int main(int argc, char **argv)
             nbdflags |= NBD_FLAG_READ_ONLY;
             flags &= ~BDRV_O_RDWR;
             break;
+        case 'L':
+            flags |= BDRV_O_NO_LOCK;
+            break;
         case 'P':
             partition = strtol(optarg, &end, 0);
             if (*end) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 91ebf04..5936b37 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -43,6 +43,8 @@ Force the use of the block driver for format @var{fmt} instead of
 auto-detecting
 @item -r, --read-only
 Export the disk as read-only
+@item -L, --no-lock
+Disable image locking
 @item -P, --partition=@var{num}
 Only expose partition @var{num}
 @item -s, --snapshot
-- 
2.7.4

[Qemu-devel] [PATCH v8 10/36] block: Don't lock drive-backup target image in none mode

[Fam Zheng]

As a very special case, in sync=none mode, the source is the backing image of
the target, which will be RO opened again. This won't work with image locking
because the first open could be exclusive.

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

diff --git a/blockdev.c b/blockdev.c
index 29c6561..a9a7dee 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3211,6 +3211,11 @@ static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, Error **errp)
         }
     }
     if (backup->sync == MIRROR_SYNC_MODE_NONE) {
+        /* XXX: bs will be open second time as the backing file of target,
+         * disable image locking. Once block layer allows sharing backing BDS,
+         * change below to BDRV_O_NO_BACKING and assign it after bdrv_open().
+         **/
+        flags |= BDRV_O_NO_LOCK;
         source = bs;
     }
 
-- 
2.7.4

[Qemu-devel] [PATCH v8 11/36] block: Add blk_lock_image

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/block-backend.c          | 18 ++++++++++++++++++
 include/sysemu/block-backend.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 0bd19ab..dc1ad36 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -61,6 +61,7 @@ struct BlockBackend {
     bool allow_write_beyond_eof;
 
     NotifierList remove_bs_notifiers, insert_bs_notifiers;
+    ImageLockMode lock_mode;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -1720,3 +1721,20 @@ static void blk_root_drained_end(BdrvChild *child)
     assert(blk->public.io_limits_disabled);
     --blk->public.io_limits_disabled;
 }
+
+void blk_lock_image(BlockBackend *blk, ImageLockMode mode, Error **errp)
+{
+    int r;
+    BlockDriverState *bs = blk_bs(blk);
+
+    if (!bs) {
+        blk->lock_mode = mode;
+        return;
+    }
+    r = bdrv_set_lock_mode(bs, mode);
+    if (r) {
+        error_setg(errp, "Failed to lock image");
+    } else {
+        blk->lock_mode = mode;
+    }
+}
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 3b29317..78f9da5 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -224,4 +224,6 @@ void blk_io_limits_disable(BlockBackend *blk);
 void blk_io_limits_enable(BlockBackend *blk, const char *group);
 void blk_io_limits_update_group(BlockBackend *blk, const char *group);
 
+void blk_lock_image(BlockBackend *blk, ImageLockMode mode, Error **errp);
+
 #endif
-- 
2.7.4

[Qemu-devel] [PATCH v8 12/36] virtio-blk: Apply lock-mode when realize

[Fam Zheng]

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

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 3a6112f..ce65615 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -896,6 +896,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "num-queues property must be larger than 0");
         return;
     }
+    blk_lock_image(conf->conf.blk, conf->conf.lock_mode, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
 
     blkconf_serial(&conf->conf, &conf->serial);
     blkconf_apply_backend_options(&conf->conf);
-- 
2.7.4

[Qemu-devel] [PATCH v8 13/36] scsi-disk: Apply lock-mode when realize

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/scsi-disk.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 88beaf4..fb406a0 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2288,6 +2288,12 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
         return;
     }
 
+    blk_lock_image(s->qdev.conf.blk, s->qdev.conf.lock_mode, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
     if (!(s->features & (1 << SCSI_DISK_F_REMOVABLE)) &&
         !blk_is_inserted(s->qdev.conf.blk)) {
         error_setg(errp, "Device needs media, but drive is empty");
-- 
2.7.4

[Qemu-devel] [PATCH v8 14/36] scsi-generic: Apply lock-mode when realize

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/scsi-generic.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index 7a588a7..4f131e8 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -502,12 +502,19 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
     int rc;
     int sg_version;
     struct sg_scsi_id scsiid;
+    Error *err = NULL;
 
     if (!s->conf.blk) {
         error_setg(errp, "drive property not set");
         return;
     }
 
+    blk_lock_image(s->conf.blk, s->conf.lock_mode, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
     if (blk_get_on_error(s->conf.blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
         error_setg(errp, "Device doesn't support drive option werror");
         return;
-- 
2.7.4

[Qemu-devel] [PATCH v8 15/36] qdev: Add "lock-mode" to block device options

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/core/qdev-properties.c    | 10 ++++++++++
 include/hw/block/block.h     |  1 +
 include/hw/qdev-properties.h |  3 +++
 3 files changed, 14 insertions(+)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 311af6d..829b143 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -552,6 +552,16 @@ PropertyInfo qdev_prop_blockdev_on_error = {
     .set = set_enum,
 };
 
+/* --- Block image lock mode --- */
+
+PropertyInfo qdev_prop_lock_mode = {
+    .name  = "ImageLockMode",
+    .description = "Image lock mode for drive (auto/shared/exclusive/nolock)",
+    .enum_table = ImageLockMode_lookup,
+    .get = get_enum,
+    .set = set_enum,
+};
+
 /* --- BIOS CHS translation */
 
 QEMU_BUILD_BUG_ON(sizeof(BiosAtaTranslation) != sizeof(int));
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 8c04469..520d142 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -47,6 +47,7 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
 
 #define DEFINE_BLOCK_PROPERTIES(_state, _conf)                          \
     DEFINE_PROP_DRIVE("drive", _state, _conf.blk),                      \
+    DEFINE_PROP_LOCK_MODE("lock-mode", _state, _conf.lock_mode),        \
     DEFINE_PROP_BLOCKSIZE("logical_block_size", _state,                 \
                           _conf.logical_block_size),                    \
     DEFINE_PROP_BLOCKSIZE("physical_block_size", _state,                \
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 2a9d2f9..1bce8ee 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -30,6 +30,7 @@ extern PropertyInfo qdev_prop_pci_devfn;
 extern PropertyInfo qdev_prop_blocksize;
 extern PropertyInfo qdev_prop_pci_host_devaddr;
 extern PropertyInfo qdev_prop_arraylen;
+extern PropertyInfo qdev_prop_lock_mode;
 
 #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
         .name      = (_name),                                    \
@@ -155,6 +156,8 @@ extern PropertyInfo qdev_prop_arraylen;
     DEFINE_PROP(_n, _s, _f, qdev_prop_vlan, NICPeers)
 #define DEFINE_PROP_DRIVE(_n, _s, _f) \
     DEFINE_PROP(_n, _s, _f, qdev_prop_drive, BlockBackend *)
+#define DEFINE_PROP_LOCK_MODE(_n, _s, _f) \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_lock_mode, ImageLockMode)
 #define DEFINE_PROP_MACADDR(_n, _s, _f)         \
     DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr)
 #define DEFINE_PROP_ON_OFF_AUTO(_n, _s, _f, _d) \
-- 
2.7.4

[Qemu-devel] [PATCH v8 16/36] ide: Apply lock-mode when initialize

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/ide/core.c             | 10 +++++++++-
 hw/ide/qdev.c             |  2 +-
 include/hw/ide/internal.h |  3 ++-
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index b0e42a6..a426baf 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -28,6 +28,7 @@
 #include "hw/pci/pci.h"
 #include "hw/isa/isa.h"
 #include "qemu/error-report.h"
+#include "qapi/error.h"
 #include "qemu/timer.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/dma.h"
@@ -2394,17 +2395,24 @@ static const BlockDevOps ide_hd_block_ops = {
     .resize_cb = ide_resize_cb,
 };
 
-int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
+int ide_init_drive(IDEState *s, BlockBackend *blk, ImageLockMode lock_mode,
+                   IDEDriveKind kind,
                    const char *version, const char *serial, const char *model,
                    uint64_t wwn,
                    uint32_t cylinders, uint32_t heads, uint32_t secs,
                    int chs_trans)
 {
+    Error *local_err = NULL;
     uint64_t nb_sectors;
 
     s->blk = blk;
     s->drive_kind = kind;
 
+    blk_lock_image(blk, lock_mode, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+        return -1;
+    }
     blk_get_geometry(blk, &nb_sectors);
     s->cylinders = cylinders;
     s->heads = heads;
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 2eb055a..c5b88fb 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -188,7 +188,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
     }
     blkconf_apply_backend_options(&dev->conf);
 
-    if (ide_init_drive(s, dev->conf.blk, kind,
+    if (ide_init_drive(s, dev->conf.blk, dev->conf.lock_mode, kind,
                        dev->version, dev->serial, dev->model, dev->wwn,
                        dev->conf.cyls, dev->conf.heads, dev->conf.secs,
                        dev->chs_trans) < 0) {
diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
index a6dd2c3..828f88c 100644
--- a/include/hw/ide/internal.h
+++ b/include/hw/ide/internal.h
@@ -600,7 +600,8 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
-int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
+int ide_init_drive(IDEState *s, BlockBackend *blk, ImageLockMode lock_mode,
+                   IDEDriveKind kind,
                    const char *version, const char *serial, const char *model,
                    uint64_t wwn,
                    uint32_t cylinders, uint32_t heads, uint32_t secs,
-- 
2.7.4

[Qemu-devel] [PATCH v8 17/36] nvme: Apply lock-mode when initialize

[Fam Zheng]

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

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index cef3bb4..318dc94 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -829,7 +829,7 @@ static int nvme_init(PCIDevice *pci_dev)
 {
     NvmeCtrl *n = NVME(pci_dev);
     NvmeIdCtrl *id = &n->id_ctrl;
-
+    Error *local_err = NULL;
     int i;
     int64_t bs_size;
     uint8_t *pci_conf;
@@ -837,6 +837,10 @@ static int nvme_init(PCIDevice *pci_dev)
     if (!n->conf.blk) {
         return -1;
     }
+    blk_lock_image(n->conf.blk, n->conf.lock_mode, &local_err);
+    if (local_err) {
+        return -1;
+    }
 
     bs_size = blk_getlength(n->conf.blk);
     if (bs_size < 0) {
-- 
2.7.4

[Qemu-devel] [PATCH v8 18/36] usb-storage: Apply lock-mode when realize

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/usb/dev-storage.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index c607f76..6d0c00e 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -600,6 +600,11 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
         error_setg(errp, "drive property not set");
         return;
     }
+    blk_lock_image(blk, s->conf.lock_mode, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
 
     blkconf_serial(&s->conf, &dev->serial);
     blkconf_blocksizes(&s->conf);
-- 
2.7.4

[Qemu-devel] [PATCH v8 19/36] pflash: Add "lock-mode" property

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/pflash_cfi01.c | 10 ++++++++++
 hw/block/pflash_cfi02.c |  9 +++++++++
 2 files changed, 19 insertions(+)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 62d7a56..df82471 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -76,6 +76,7 @@ struct pflash_t {
     /*< public >*/
 
     BlockBackend *blk;
+    ImageLockMode lock_mode;
     uint32_t nb_blocs;
     uint64_t sector_len;
     uint8_t bank_width;
@@ -738,6 +739,14 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
 
     if (pfl->blk) {
+        Error *local_err = NULL;
+
+        blk_lock_image(pfl->blk, pfl->lock_mode, &local_err);
+        if (local_err) {
+            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
+            error_propagate(errp, local_err);
+            return;
+        }
         /* read the initial flash content */
         ret = blk_pread(pfl->blk, 0, pfl->storage, total_len);
 
@@ -852,6 +861,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
 
 static Property pflash_cfi01_properties[] = {
     DEFINE_PROP_DRIVE("drive", struct pflash_t, blk),
+    DEFINE_PROP_LOCK_MODE("lock-mode", struct pflash_t, lock_mode),
     /* num-blocks is the number of blocks actually visible to the guest,
      * ie the total size of the device divided by the sector length.
      * If we're emulating flash devices wired in parallel the actual
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 4f6105c..ff6a9fb 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -620,6 +620,14 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
     pfl->storage = memory_region_get_ram_ptr(&pfl->orig_mem);
     pfl->chip_len = chip_len;
     if (pfl->blk) {
+        Error *local_err = NULL;
+
+        blk_lock_image(pfl->blk, pfl->lock_mode, &local_err);
+        if (local_err) {
+            vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
+            error_propagate(errp, local_err);
+            return;
+        }
         /* read the initial flash content */
         ret = blk_pread(pfl->blk, 0, pfl->storage, chip_len);
         if (ret < 0) {
@@ -724,6 +732,7 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
 
 static Property pflash_cfi02_properties[] = {
     DEFINE_PROP_DRIVE("drive", struct pflash_t, blk),
+    DEFINE_PROP_LOCK_MODE("lock-mode", struct pflash_t, lock_mode),
     DEFINE_PROP_UINT32("num-blocks", struct pflash_t, nb_blocs, 0),
     DEFINE_PROP_UINT32("sector-length", struct pflash_t, sector_len, 0),
     DEFINE_PROP_UINT8("width", struct pflash_t, width, 0),
-- 
2.7.4

[Qemu-devel] [PATCH v8 20/36] 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 e528b67..f15ccbf 100755
--- a/tests/qemu-iotests/046
+++ b/tests/qemu-iotests/046
@@ -192,15 +192,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
 
@@ -261,7 +253,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 "$TEST_IMG" | _filter_qemu_io
 
 _check_test_img
 
-- 
2.7.4

[Qemu-devel] [PATCH v8 21/36] qemu-iotests: 091: Prepare for image lock

[Fam Zheng]

We should wait for the QEMU process to terminate and close the image
before we check the data.

Also use shared lock in migration source and target.

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

diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
index 32bbd56..d54ab73 100755
--- a/tests/qemu-iotests/091
+++ b/tests/qemu-iotests/091
@@ -61,13 +61,15 @@ 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,if=none \
+             -device virtio-blk,drive=disk,lock-mode=shared
 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,if=none \
+             -device virtio-blk,drive=disk,lock-mode=shared \
              -incoming "exec: cat '${MIG_FIFO}'"
 h2=$QEMU_HANDLE
 
@@ -95,6 +97,9 @@ echo "vm2: qemu process running successfully"
 echo "vm2: flush io, and quit"
 _send_qemu_cmd $h2 'qemu-io disk flush' "(qemu)"
 _send_qemu_cmd $h2 'quit' ""
+echo "vm1: quit"
+_send_qemu_cmd $h1 'quit' ""
+wait
 
 echo "Check image pattern"
 ${QEMU_IO} -c "read -P 0x22 0 4M" "${TEST_IMG}" | _filter_testdir | _filter_qemu_io
diff --git a/tests/qemu-iotests/091.out b/tests/qemu-iotests/091.out
index 5017f8c..6658ca8 100644
--- a/tests/qemu-iotests/091.out
+++ b/tests/qemu-iotests/091.out
@@ -18,6 +18,7 @@ vm1: live migration completed
 vm2: qemu-io disk write complete
 vm2: qemu process running successfully
 vm2: flush io, and quit
+vm1: quit
 Check image pattern
 read 4194304/4194304 bytes at offset 0
 4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-- 
2.7.4

[Qemu-devel] [PATCH v8 22/36] qemu-iotests: 030: Disable image locking 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>
Reviewed-by: Max Reitz <mreitz@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 107049b..acae67e 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.7.4

[Qemu-devel] [PATCH v8 23/36] iotests: 087: Disable image locking in cases where file is shared

[Fam Zheng]

Otherwise the error handling we are expecting will be masked by the
preceding image locking check, and is going to be indistinguishable
because the error messages are all the same.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/087 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 5c04577..91e4c71 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -84,6 +84,7 @@ run_qemu -drive driver=$IMGFMT,id=disk,node-name=test-node,file="$TEST_IMG" <<EO
       "options": {
         "driver": "$IMGFMT",
         "node-name": "disk",
+        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
@@ -96,6 +97,7 @@ run_qemu -drive driver=$IMGFMT,id=disk,node-name=test-node,file="$TEST_IMG" <<EO
       "options": {
         "driver": "$IMGFMT",
         "node-name": "test-node",
+        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
-- 
2.7.4

[Qemu-devel] [PATCH v8 24/36] iotests: 087: Disable image locking in cases where file is shared

[Fam Zheng]

Otherwise the error handling we are expecting will be masked by the
preceding image locking check, and is going to be indistinguishable
because the error messages are all the same.

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

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 91e4c71..5c04577 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -84,7 +84,6 @@ run_qemu -drive driver=$IMGFMT,id=disk,node-name=test-node,file="$TEST_IMG" <<EO
       "options": {
         "driver": "$IMGFMT",
         "node-name": "disk",
-        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
@@ -97,7 +96,6 @@ run_qemu -drive driver=$IMGFMT,id=disk,node-name=test-node,file="$TEST_IMG" <<EO
       "options": {
         "driver": "$IMGFMT",
         "node-name": "test-node",
-        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
-- 
2.7.4

[Qemu-devel] [PATCH v8 25/36] iotests: 130: Check image info locklessly

[Fam Zheng]

By the time _img_info is run, QEMU process's resources may still be on
its way being cleaned up, asynchronously, even though the process itself
is already gone after the "kill -KILL" and "wait" commands in
_cleanup_qemu.

Change the last HMP command to 'q' to ensure the locks are released.

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

diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130
index ecc8a5b..f14a04f 100755
--- a/tests/qemu-iotests/130
+++ b/tests/qemu-iotests/130
@@ -60,7 +60,7 @@ echo
 # Test that a backing file isn't written
 _launch_qemu -drive id=testdisk,file="$TEST_IMG",backing.file.filename="$TEST_IMG.base"
 _send_qemu_cmd $QEMU_HANDLE "commit testdisk" "(qemu)"
-_send_qemu_cmd $QEMU_HANDLE '' '(qemu)'
+_send_qemu_cmd $QEMU_HANDLE 'q' '(qemu)'
 _cleanup_qemu
 _img_info | _filter_img_info
 
@@ -69,7 +69,7 @@ _img_info | _filter_img_info
 _make_test_img -F raw -b "$TEST_IMG.orig" 64M
 _launch_qemu -drive id=testdisk,file="$TEST_IMG",backing.file.filename="$TEST_IMG.base",backing.driver=$IMGFMT
 _send_qemu_cmd $QEMU_HANDLE "commit testdisk" "(qemu)"
-_send_qemu_cmd $QEMU_HANDLE '' '(qemu)'
+_send_qemu_cmd $QEMU_HANDLE 'q' '(qemu)'
 _cleanup_qemu
 _img_info | _filter_img_info
 
diff --git a/tests/qemu-iotests/130.out b/tests/qemu-iotests/130.out
index ae95b50..2422329 100644
--- a/tests/qemu-iotests/130.out
+++ b/tests/qemu-iotests/130.out
@@ -10,14 +10,14 @@ virtual size: 64M (67108864 bytes)
 
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) c^[[K^[[Dco^[[K^[[D^[[Dcom^[[K^[[D^[[D^[[Dcomm^[[K^[[D^[[D^[[D^[[Dcommi^[[K^[[D^[[D^[[D^[[D^[[Dcommit^[[K^[[D^[[D^[[D^[[D^[[D^[[Dcommit ^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit t^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit te^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit tes^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit test^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testd^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testdi^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testdis^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testdisk^[[K
-(qemu) 
+(qemu) q^[[K
 image: TEST_DIR/t.IMGFMT
 file format: IMGFMT
 virtual size: 64M (67108864 bytes)
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.orig backing_fmt=raw
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) c^[[K^[[Dco^[[K^[[D^[[Dcom^[[K^[[D^[[D^[[Dcomm^[[K^[[D^[[D^[[D^[[Dcommi^[[K^[[D^[[D^[[D^[[D^[[Dcommit^[[K^[[D^[[D^[[D^[[D^[[D^[[Dcommit ^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit t^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit te^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit tes^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit test^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testd^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testdi^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testdis^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testdisk^[[K
-(qemu) 
+(qemu) q^[[K
 image: TEST_DIR/t.IMGFMT
 file format: IMGFMT
 virtual size: 64M (67108864 bytes)
-- 
2.7.4

[Qemu-devel] [PATCH v8 26/36] iotests: Disable image locking in 085

[Fam Zheng]

The cases is about live snapshot features. Disable image locking because
otherwise a few tests are going to fail because we reuse the same images
at blockdev-add.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/085 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index aa77eca..59bfd0e 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -102,6 +102,7 @@ function add_snapshot_image()
     cmd="{ 'execute': 'blockdev-add', 'arguments':
            { 'options':
              { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
+               'read-only': true,
                'file':
                { 'driver': 'file', 'filename': '${snapshot_file}',
                  'node-name': 'file_${1}' } } } }"
@@ -130,7 +131,10 @@ echo === Running QEMU ===
 echo
 
 qemu_comm_method="qmp"
-_launch_qemu -drive file="${TEST_IMG}.1",if=virtio -drive file="${TEST_IMG}.2",if=virtio
+_launch_qemu -drive file="${TEST_IMG}.1",if=none,id=virtio0 \
+             -device virtio-blk,drive=virtio0,lock-mode=nolock \
+             -drive file="${TEST_IMG}.2",if=none,id=virtio1 \
+             -device virtio-blk,drive=virtio1,lock-mode=nolock
 h=$QEMU_HANDLE
 
 echo
-- 
2.7.4

[Qemu-devel] [PATCH v8 27/36] tests: Use null-co:// instead of /dev/null

[Fam Zheng]

With image locking, opening /dev/null can fail when multiple tests run
in parallel (make -j2, for example). Use null-co:// as the null protocol
doesn't do image locking.

While it's arguable we could special-case /dev/null, /dev/zero,
/dev/urandom etc in raw-posix driver, it is not really necessary because
user can always specify lock-mode=off when it is appropriate. So let's
write sensible testing code too.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 tests/drive_del-test.c    | 2 +-
 tests/nvme-test.c         | 2 +-
 tests/usb-hcd-uhci-test.c | 2 +-
 tests/usb-hcd-xhci-test.c | 2 +-
 tests/virtio-blk-test.c   | 2 +-
 tests/virtio-scsi-test.c  | 4 ++--
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 121b9c9..2175139 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -92,7 +92,7 @@ static void test_after_failed_device_add(void)
 static void test_drive_del_device_del(void)
 {
     /* Start with a drive used by a device that unplugs instantaneously */
-    qtest_start("-drive if=none,id=drive0,file=/dev/null,format=raw"
+    qtest_start("-drive if=none,id=drive0,file=null-co://,format=raw"
                 " -device virtio-scsi-pci"
                 " -device scsi-hd,drive=drive0,id=dev0");
 
diff --git a/tests/nvme-test.c b/tests/nvme-test.c
index c8bece4..7674a44 100644
--- a/tests/nvme-test.c
+++ b/tests/nvme-test.c
@@ -22,7 +22,7 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
     qtest_add_func("/nvme/nop", nop);
 
-    qtest_start("-drive id=drv0,if=none,file=/dev/null,format=raw "
+    qtest_start("-drive id=drv0,if=none,file=null-co://,format=raw "
                 "-device nvme,drive=drv0,serial=foo");
     ret = g_test_run();
 
diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
index 5cd59ad..3684503 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -85,7 +85,7 @@ int main(int argc, char **argv)
     qtest_add_func("/uhci/pci/hotplug/usb-storage", test_usb_storage_hotplug);
 
     qtest_start("-device piix3-usb-uhci,id=uhci,addr=1d.0"
-                " -drive id=drive0,if=none,file=/dev/null,format=raw"
+                " -drive id=drive0,if=none,file=null-co://,format=raw"
                 " -device usb-tablet,bus=uhci.0,port=1");
     ret = g_test_run();
     qtest_end();
diff --git a/tests/usb-hcd-xhci-test.c b/tests/usb-hcd-xhci-test.c
index 22513e9..031764d 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -89,7 +89,7 @@ int main(int argc, char **argv)
     qtest_add_func("/xhci/pci/hotplug/usb-uas", test_usb_uas_hotplug);
 
     qtest_start("-device nec-usb-xhci,id=xhci"
-                " -drive id=drive0,if=none,file=/dev/null,format=raw");
+                " -drive id=drive0,if=none,file=null-co://,format=raw");
     ret = g_test_run();
     qtest_end();
 
diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 811cf75..5dba567 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -66,7 +66,7 @@ static QPCIBus *pci_test_start(void)
     tmp_path = drive_create();
 
     cmdline = g_strdup_printf("-drive if=none,id=drive0,file=%s,format=raw "
-                        "-drive if=none,id=drive1,file=/dev/null,format=raw "
+                        "-drive if=none,id=drive1,file=null-co://,format=raw "
                         "-device virtio-blk-pci,id=drv0,drive=drive0,"
                         "addr=%x.%x",
                         tmp_path, PCI_SLOT, PCI_FN);
diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
index f1489e6..3c2f5df 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -40,7 +40,7 @@ static void qvirtio_scsi_start(const char *extra_opts)
     char *cmdline;
 
     cmdline = g_strdup_printf(
-                "-drive id=drv0,if=none,file=/dev/null,format=raw "
+                "-drive id=drv0,if=none,file=null-co://,format=raw "
                 "-device virtio-scsi-pci,id=vs0 "
                 "-device scsi-hd,bus=vs0.0,drive=drv0 %s",
                 extra_opts ? : "");
@@ -192,7 +192,7 @@ static void hotplug(void)
 {
     QDict *response;
 
-    qvirtio_scsi_start("-drive id=drv1,if=none,file=/dev/null,format=raw");
+    qvirtio_scsi_start("-drive id=drv1,if=none,file=null-co://,format=raw");
     response = qmp("{\"execute\": \"device_add\","
                    " \"arguments\": {"
                    "   \"driver\": \"scsi-hd\","
-- 
2.7.4

[Qemu-devel] [PATCH v8 28/36] qemu-iotests: Add test case 153 for image locking

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/153     | 197 +++++++++++++++++++++
 tests/qemu-iotests/153.out | 426 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 624 insertions(+)
 create mode 100755 tests/qemu-iotests/153
 create mode 100644 tests/qemu-iotests/153.out

diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
new file mode 100755
index 0000000..68770ec
--- /dev/null
+++ b/tests/qemu-iotests/153
@@ -0,0 +1,197 @@
+#!/bin/bash
+#
+# Test image locking
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=famz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm -f "${TEST_IMG}.base"
+    rm -f "${TEST_IMG}.convert"
+    rm -f "${TEST_IMG}.a"
+    rm -f "${TEST_IMG}.b"
+    rm -f "${TEST_IMG}.lnk"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+size=32M
+
+_run_cmd()
+{
+    echo
+    (echo "$@"; "$@" 2>&1 1>/dev/null) | _filter_testdir
+}
+
+function _do_run_qemu()
+{
+    (
+        if ! test -t 0; then
+            while read cmd; do
+                echo $cmd
+            done
+        fi
+        echo quit
+    ) | $QEMU -nographic -monitor stdio -serial none "$@" 1>/dev/null
+}
+
+function _run_qemu_with_images()
+{
+    _do_run_qemu \
+        $(for i in $@; do echo "-drive if=none,file=$i"; done) 2>&1 \
+        | _filter_testdir | _filter_qemu
+}
+
+for opts1 in "" "readonly=on" "lock-mode=off" "lock-mode=shared"; do
+    echo
+    echo "== Creating base image =="
+    TEST_IMG="${TEST_IMG}.base" _make_test_img $size
+
+    echo
+    echo "== Creating test image =="
+    $QEMU_IMG create -f $IMGFMT "${TEST_IMG}" -b ${TEST_IMG}.base | _filter_img_create
+
+    echo
+    echo "== Launching QEMU $opts1 =="
+    _launch_qemu -drive file="${TEST_IMG}",if=none,$opts1
+    h=$QEMU_HANDLE
+
+    for opts2 in "" "lock-mode=auto" "lock-mode=shared" \
+                 "lock-mode=off" "lock-mode=auto,readonly=on"; do
+        echo
+        echo "== Launching another QEMU $opts2 =="
+        echo "quit" | \
+            $QEMU -nographic -monitor stdio \
+            -drive file="${TEST_IMG}",if=none,$opts2 2>&1 1>/dev/null | \
+            _filter_testdir | _filter_qemu
+    done
+
+    for L in "" "-L"; do
+
+        echo
+        echo "== Running utility commands $(test -n "$L" && echo "(lock disabled)") =="
+        _run_cmd $QEMU_IO $L -c "read 0 512" "${TEST_IMG}"
+        _run_cmd $QEMU_IO $L -r -c "read 0 512" "${TEST_IMG}"
+        _run_cmd $QEMU_IMG info        $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG check       $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG compare     $L "${TEST_IMG}" "${TEST_IMG}"
+        _run_cmd $QEMU_IMG map         $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG amend -o "" $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG commit      $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG resize      $L "${TEST_IMG}" $size
+        _run_cmd $QEMU_IMG rebase      $L "${TEST_IMG}" -b "${TEST_IMG}.base"
+        _run_cmd $QEMU_IMG snapshot -l $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG convert     $L "${TEST_IMG}" "${TEST_IMG}.convert"
+    done
+    _send_qemu_cmd $h "{ 'execute': 'quit', }" ""
+    echo
+    echo "Round done"
+    _cleanup_qemu
+done
+
+function _enum_opts()
+{
+    echo lock-mode={shared,auto,off},readonly={on,off}
+}
+
+for opt1 in $(_enum_opts); do
+    for opt2 in $(_enum_opts); do
+        echo
+        echo "== Two devices with the same image ($opt1 - $opt2) =="
+        _run_qemu_with_images "${TEST_IMG},$opt1" "${TEST_IMG},$opt2"
+    done
+done
+
+(
+    $QEMU_IMG create -f qcow2 "${TEST_IMG}.a" -b "${TEST_IMG}"
+    $QEMU_IMG create -f qcow2 "${TEST_IMG}.b" -b "${TEST_IMG}"
+    $QEMU_IMG create -f qcow2 "${TEST_IMG}.c" -b "${TEST_IMG}.b"
+) | _filter_img_create
+
+echo
+echo "== Two devices sharing the same file in backing chain =="
+_run_qemu_with_images "${TEST_IMG}.a" "${TEST_IMG}.b"
+_run_qemu_with_images "${TEST_IMG}.a" "${TEST_IMG}.c"
+
+echo
+echo "== Backing image also as an active device =="
+_run_qemu_with_images "${TEST_IMG}.a" "${TEST_IMG}"
+
+echo
+echo "== Backing image also as an active device (ro) =="
+_run_qemu_with_images "${TEST_IMG}.a" "${TEST_IMG},readonly=on"
+
+echo
+echo "== Backing image also as an active device (shared) =="
+_run_qemu_with_images "${TEST_IMG}.a" "${TEST_IMG},lock-mode=shared"
+
+echo
+echo "== Symbolic link =="
+rm -f "${TEST_IMG}.lnk" &>/dev/null
+ln -s ${TEST_IMG} "${TEST_IMG}.lnk" || echo "Failed to create link"
+_run_qemu_with_images "${TEST_IMG}.lnk" "${TEST_IMG}"
+
+echo
+echo "== Test that close one BDS doesn't unlock others =="
+_launch_qemu -drive file="${TEST_IMG}",if=none,id=d0
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'qmp_capabilities' }" \
+    'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line': 'drive_add 0 if=none,id=d1,file=${TEST_IMG}' } }" \
+    "Failed to lock image"
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line': 'drive_add 0 if=none,id=d1,file=${TEST_IMG},lock-mode=off' } }" \
+    "OK"
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line': 'drive_del d1' } }" \
+    ""
+
+_run_cmd $QEMU_IMG info "${TEST_IMG}"
+
+_cleanup_qemu
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out
new file mode 100644
index 0000000..377d9e2
--- /dev/null
+++ b/tests/qemu-iotests/153.out
@@ -0,0 +1,426 @@
+QA output created by 153
+
+== Creating base image ==
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=33554432
+
+== Creating test image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT.base
+
+== Launching QEMU  ==
+
+== Launching another QEMU  ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,: Failed to lock image
+
+== Launching another QEMU lock-mode=auto ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=auto: Failed to lock image
+
+== Launching another QEMU lock-mode=shared ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=shared: Failed to lock image
+
+== Launching another QEMU lock-mode=off ==
+
+== Launching another QEMU lock-mode=auto,readonly=on ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=auto,readonly=on: Failed to lock image
+
+== Running utility commands  ==
+
+_qemu_io_wrapper -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: Failed to lock image
+no file open, try 'help open'
+
+_qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: Failed to lock image
+no file open, try 'help open'
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper check TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper compare TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper map TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper amend -o  TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper commit TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper resize TEST_DIR/t.qcow2 32M
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper rebase TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper snapshot -l TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper convert TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+== Running utility commands (lock disabled) ==
+
+_qemu_io_wrapper -L -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -L -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper commit -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize -L TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase -L TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+Round done
+
+== Creating base image ==
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=33554432
+
+== Creating test image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT.base
+
+== Launching QEMU readonly=on ==
+
+== Launching another QEMU  ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,: Failed to lock image
+
+== Launching another QEMU lock-mode=auto ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=auto: Failed to lock image
+
+== Launching another QEMU lock-mode=shared ==
+
+== Launching another QEMU lock-mode=off ==
+
+== Launching another QEMU lock-mode=auto,readonly=on ==
+
+== Running utility commands  ==
+
+_qemu_io_wrapper -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: Failed to lock image
+no file open, try 'help open'
+
+_qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper commit TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper resize TEST_DIR/t.qcow2 32M
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper rebase TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper snapshot -l TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+== Running utility commands (lock disabled) ==
+
+_qemu_io_wrapper -L -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -L -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper commit -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize -L TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase -L TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+Round done
+
+== Creating base image ==
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=33554432
+
+== Creating test image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT.base
+
+== Launching QEMU lock-mode=off ==
+
+== Launching another QEMU  ==
+
+== Launching another QEMU lock-mode=auto ==
+
+== Launching another QEMU lock-mode=shared ==
+
+== Launching another QEMU lock-mode=off ==
+
+== Launching another QEMU lock-mode=auto,readonly=on ==
+
+== Running utility commands  ==
+
+_qemu_io_wrapper -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  TEST_DIR/t.qcow2
+
+_qemu_img_wrapper commit TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+== Running utility commands (lock disabled) ==
+
+_qemu_io_wrapper -L -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -L -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper commit -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize -L TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase -L TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+Round done
+
+== Creating base image ==
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=33554432
+
+== Creating test image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT.base
+
+== Launching QEMU lock-mode=shared ==
+
+== Launching another QEMU  ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,: Failed to lock image
+
+== Launching another QEMU lock-mode=auto ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=auto: Failed to lock image
+
+== Launching another QEMU lock-mode=shared ==
+
+== Launching another QEMU lock-mode=off ==
+
+== Launching another QEMU lock-mode=auto,readonly=on ==
+
+== Running utility commands  ==
+
+_qemu_io_wrapper -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: Failed to lock image
+no file open, try 'help open'
+
+_qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper commit TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper resize TEST_DIR/t.qcow2 32M
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper rebase TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper snapshot -l TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+== Running utility commands (lock disabled) ==
+
+_qemu_io_wrapper -L -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -L -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -o  -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper commit -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize -L TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase -L TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+Round done
+
+== Two devices with the same image (lock-mode=shared,readonly=on - lock-mode=shared,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=on - lock-mode=shared,readonly=off) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=on - lock-mode=auto,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=on - lock-mode=auto,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=auto,readonly=off: Failed to lock image
+
+== Two devices with the same image (lock-mode=shared,readonly=on - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=on - lock-mode=off,readonly=off) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=off - lock-mode=shared,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=off - lock-mode=shared,readonly=off) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=off - lock-mode=auto,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=off - lock-mode=auto,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=auto,readonly=off: Failed to lock image
+
+== Two devices with the same image (lock-mode=shared,readonly=off - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=off - lock-mode=off,readonly=off) ==
+
+== Two devices with the same image (lock-mode=auto,readonly=on - lock-mode=shared,readonly=on) ==
+
+== Two devices with the same image (lock-mode=auto,readonly=on - lock-mode=shared,readonly=off) ==
+
+== Two devices with the same image (lock-mode=auto,readonly=on - lock-mode=auto,readonly=on) ==
+
+== Two devices with the same image (lock-mode=auto,readonly=on - lock-mode=auto,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=auto,readonly=off: Failed to lock image
+
+== Two devices with the same image (lock-mode=auto,readonly=on - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=auto,readonly=on - lock-mode=off,readonly=off) ==
+
+== Two devices with the same image (lock-mode=auto,readonly=off - lock-mode=shared,readonly=on) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=shared,readonly=on: Failed to lock image
+
+== Two devices with the same image (lock-mode=auto,readonly=off - lock-mode=shared,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=shared,readonly=off: Failed to lock image
+
+== Two devices with the same image (lock-mode=auto,readonly=off - lock-mode=auto,readonly=on) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=auto,readonly=on: Failed to lock image
+
+== Two devices with the same image (lock-mode=auto,readonly=off - lock-mode=auto,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=auto,readonly=off: Failed to lock image
+
+== Two devices with the same image (lock-mode=auto,readonly=off - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=auto,readonly=off - lock-mode=off,readonly=off) ==
+
+== Two devices with the same image (lock-mode=off,readonly=on - lock-mode=shared,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=on - lock-mode=shared,readonly=off) ==
+
+== Two devices with the same image (lock-mode=off,readonly=on - lock-mode=auto,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=on - lock-mode=auto,readonly=off) ==
+
+== Two devices with the same image (lock-mode=off,readonly=on - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=on - lock-mode=off,readonly=off) ==
+
+== Two devices with the same image (lock-mode=off,readonly=off - lock-mode=shared,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=off - lock-mode=shared,readonly=off) ==
+
+== Two devices with the same image (lock-mode=off,readonly=off - lock-mode=auto,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=off - lock-mode=auto,readonly=off) ==
+
+== Two devices with the same image (lock-mode=off,readonly=off - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=off - lock-mode=off,readonly=off) ==
+Formatting 'TEST_DIR/t.IMGFMT.a', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT.b', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT
+Formatting 'TEST_DIR/t.IMGFMT.c', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT.b
+
+== Two devices sharing the same file in backing chain ==
+
+== Backing image also as an active device ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2: Failed to lock image
+
+== Backing image also as an active device (ro) ==
+
+== Backing image also as an active device (shared) ==
+
+== Symbolic link ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2: Failed to lock image
+
+== Test that close one BDS doesn't unlock others ==
+{"return": {}}
+{"return": "Failed to lock imagern"}
+{"return": "OKrn"}
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 7eb1770..e2054e2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -153,6 +153,7 @@
 149 rw auto sudo
 150 rw auto quick
 152 rw auto quick
+153 rw auto quick
 154 rw auto backing quick
 155 rw auto
 156 rw auto quick
-- 
2.7.4

[Qemu-devel] [PATCH v8 29/36] ahci: Use shared lock for shared storage migration

[Fam Zheng]

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

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 9c0adce..838ff45 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -1132,10 +1132,14 @@ 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=none,id=drive0,file=%s,format=%s "
+                    "-device ide-hd,drive=drive0,lock-mode=shared ",
+                    tmp_path, imgfmt);
     dst = ahci_boot("-m 1024 -M q35 "
-                    "-drive if=ide,file=%s,format=%s "
-                    "-incoming %s", tmp_path, imgfmt, uri);
+                    "-drive if=none,id=drive0,file=%s,format=%s "
+                    "-device ide-hd,drive=drive0,lock-mode=shared "
+                    "-incoming %s",
+                    tmp_path, imgfmt, uri);
 
     ahci_migrate(src, dst, uri);
 
@@ -1157,11 +1161,14 @@ 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=none,format=%s,file=%s,id=drive0 "
+                               "-device ide-hd,drive=drive0,lock-mode=shared ",
                                imgfmt, tmp_path);
     dst = ahci_boot("-m 1024 -M q35 "
-                    "-drive if=ide,format=%s,file=%s "
-                    "-incoming %s", imgfmt, tmp_path, uri);
+                    "-drive if=none,format=%s,file=%s,id=drive0 "
+                    "-device ide-hd,drive=drive0,lock-mode=shared "
+                    "-incoming %s",
+                    imgfmt, tmp_path, uri);
 
     set_context(src->parent);
 
@@ -1286,7 +1293,7 @@ static void ahci_migrate_halted_io(uint8_t cmd_read, uint8_t cmd_write)
                                "format=%s,cache=writeback,"
                                "rerror=stop,werror=stop "
                                "-M q35 "
-                               "-device ide-hd,drive=drive0 ",
+                               "-device ide-hd,drive=drive0,lock-mode=shared ",
                                debug_path,
                                tmp_path, imgfmt);
 
@@ -1294,7 +1301,7 @@ static void ahci_migrate_halted_io(uint8_t cmd_read, uint8_t cmd_write)
                     "format=%s,cache=writeback,"
                     "rerror=stop,werror=stop "
                     "-M q35 "
-                    "-device ide-hd,drive=drive0 "
+                    "-device ide-hd,drive=drive0,lock-mode=shared "
                     "-incoming %s",
                     tmp_path, imgfmt, uri);
 
@@ -1358,13 +1365,13 @@ static void test_flush_migrate(void)
                                "cache=writeback,rerror=stop,werror=stop,"
                                "format=%s "
                                "-M q35 "
-                               "-device ide-hd,drive=drive0 ",
+                               "-device ide-hd,drive=drive0,lock-mode=shared ",
                                debug_path, tmp_path, imgfmt);
     dst = ahci_boot("-drive file=%s,if=none,id=drive0,"
                     "cache=writeback,rerror=stop,werror=stop,"
                     "format=%s "
                     "-M q35 "
-                    "-device ide-hd,drive=drive0 "
+                    "-device ide-hd,drive=drive0,lock-mode=shared "
                     "-incoming %s", tmp_path, imgfmt, uri);
 
     set_context(src->parent);
-- 
2.7.4

[Qemu-devel] [PATCH v8 30/36] tests/postcopy: Use shared lock for images

[Fam Zheng]

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

diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index 41ed1a9..3ec4d01 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -371,12 +371,14 @@ static void test_migrate(void)
         cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
                                   " -name pcsource,debug-threads=on"
                                   " -serial file:%s/src_serial"
-                                  " -drive file=%s,format=raw",
+                                  " -drive file=%s,format=raw,if=none,id=drive1"
+                                  " -device ide-hd,drive=drive1,lock-mode=shared",
                                   tmpfs, bootpath);
         cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
                                   " -name pcdest,debug-threads=on"
                                   " -serial file:%s/dest_serial"
-                                  " -drive file=%s,format=raw"
+                                  " -drive file=%s,format=raw,if=none,id=drive2"
+                                  " -device ide-hd,drive=drive2,lock-mode=shared"
                                   " -incoming %s",
                                   tmpfs, bootpath, uri);
     } else if (strcmp(arch, "ppc64") == 0) {
@@ -384,7 +386,8 @@ static void test_migrate(void)
         cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
                                   " -name pcsource,debug-threads=on"
                                   " -serial file:%s/src_serial"
-                                  " -drive file=%s,if=pflash,format=raw",
+                                  " -drive file=%s,if=none,format=raw,id=drive3"
+                                  " -device pflash,drive=drive3,lock-mode=shared",
                                   tmpfs, bootpath);
         cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
                                   " -name pcdest,debug-threads=on"
-- 
2.7.4

[Qemu-devel] [PATCH v8 31/36] fdc: Add lock-mode qdev properties

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/fdc.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index b79873a..20684b8 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -158,6 +158,7 @@ typedef enum FDiskFlags {
 typedef struct FDrive {
     FDCtrl *fdctrl;
     BlockBackend *blk;
+    ImageLockMode lock_mode;
     /* Drive status */
     FloppyDriveType drive;    /* CMOS drive type        */
     uint8_t perpendicular;    /* 2.88 MB access mode    */
@@ -2446,11 +2447,23 @@ static void fdctrl_realize_common(FDCtrl *fdctrl, Error **errp)
 {
     int i, j;
     static int command_tables_inited = 0;
+    Error *local_err = NULL;
 
     if (fdctrl->fallback == FLOPPY_DRIVE_TYPE_AUTO) {
         error_setg(errp, "Cannot choose a fallback FDrive type of 'auto'");
     }
 
+    for (i = 0; i < 2; ++i) {
+        if (fdctrl->drives[i].blk) {
+            blk_lock_image(fdctrl->drives[i].blk, fdctrl->drives[i].lock_mode,
+                           &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        }
+    }
+
     /* Fill 'command_to_handler' lookup table */
     if (!command_tables_inited) {
         command_tables_inited = 1;
@@ -2495,7 +2508,6 @@ static void isabus_fdc_realize(DeviceState *dev, Error **errp)
     FDCtrlISABus *isa = ISA_FDC(dev);
     FDCtrl *fdctrl = &isa->state;
     Error *err = NULL;
-
     isa_register_portio_list(isadev, &fdctrl->portio_list,
                              isa->iobase, fdc_portio_list, fdctrl,
                              "fdc");
@@ -2608,6 +2620,8 @@ static Property isa_fdc_properties[] = {
     DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
     DEFINE_PROP_DRIVE("driveA", FDCtrlISABus, state.drives[0].blk),
     DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.drives[1].blk),
+    DEFINE_PROP_LOCK_MODE("lock-modeA", FDCtrlISABus, state.drives[0].lock_mode),
+    DEFINE_PROP_LOCK_MODE("lock-modeB", FDCtrlISABus, state.drives[1].lock_mode),
     DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus, state.check_media_rate,
                     0, true),
     DEFINE_PROP_DEFAULT("fdtypeA", FDCtrlISABus, state.drives[0].drive,
@@ -2667,6 +2681,8 @@ static const VMStateDescription vmstate_sysbus_fdc ={
 static Property sysbus_fdc_properties[] = {
     DEFINE_PROP_DRIVE("driveA", FDCtrlSysBus, state.drives[0].blk),
     DEFINE_PROP_DRIVE("driveB", FDCtrlSysBus, state.drives[1].blk),
+    DEFINE_PROP_LOCK_MODE("lock-modeA", FDCtrlISABus, state.drives[0].lock_mode),
+    DEFINE_PROP_LOCK_MODE("lock-modeB", FDCtrlISABus, state.drives[1].lock_mode),
     DEFINE_PROP_DEFAULT("fdtypeA", FDCtrlSysBus, state.drives[0].drive,
                         FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
                         FloppyDriveType),
@@ -2696,6 +2712,7 @@ static const TypeInfo sysbus_fdc_info = {
 
 static Property sun4m_fdc_properties[] = {
     DEFINE_PROP_DRIVE("drive", FDCtrlSysBus, state.drives[0].blk),
+    DEFINE_PROP_LOCK_MODE("lock-mode", FDCtrlISABus, state.drives[0].lock_mode),
     DEFINE_PROP_DEFAULT("fdtype", FDCtrlSysBus, state.drives[0].drive,
                         FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
                         FloppyDriveType),
-- 
2.7.4

[Qemu-devel] [PATCH v8 32/36] m25p80: Add 'lock-mode' property

[Fam Zheng]

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

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index d29ff4c..3c1765a 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -380,6 +380,7 @@ typedef struct Flash {
     SSISlave parent_obj;
 
     BlockBackend *blk;
+    ImageLockMode lock_mode;
 
     uint8_t *storage;
     uint32_t size;
@@ -1144,6 +1145,7 @@ static void m25p80_realize(SSISlave *ss, Error **errp)
 {
     Flash *s = M25P80(ss);
     M25P80Class *mc = M25P80_GET_CLASS(s);
+    Error *local_err = NULL;
 
     s->pi = mc->pi;
 
@@ -1151,6 +1153,11 @@ static void m25p80_realize(SSISlave *ss, Error **errp)
     s->dirty_page = -1;
 
     if (s->blk) {
+        blk_lock_image(s->blk, s->lock_mode, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
         DB_PRINT_L(0, "Binding to IF_MTD drive\n");
         s->storage = blk_blockalign(s->blk, s->size);
 
@@ -1185,6 +1192,7 @@ static Property m25p80_properties[] = {
     DEFINE_PROP_UINT8("spansion-cr3nv", Flash, spansion_cr3nv, 0x2),
     DEFINE_PROP_UINT8("spansion-cr4nv", Flash, spansion_cr4nv, 0x10),
     DEFINE_PROP_DRIVE("drive", Flash, blk),
+    DEFINE_PROP_LOCK_MODE("lock-mode", Flash, lock_mode),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.7.4

[Qemu-devel] [PATCH v8 33/36] nand: Add 'lock-mode' property

[Fam Zheng]

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

diff --git a/hw/block/nand.c b/hw/block/nand.c
index c69e675..75b5a68 100644
--- a/hw/block/nand.c
+++ b/hw/block/nand.c
@@ -64,6 +64,7 @@ struct NANDFlashState {
     int page_shift, oob_shift, erase_shift, addr_shift;
     uint8_t *storage;
     BlockBackend *blk;
+    ImageLockMode lock_mode;
     int mem_oob;
 
     uint8_t cle, ale, ce, wp, gnd;
@@ -373,6 +374,7 @@ static void nand_realize(DeviceState *dev, Error **errp)
 {
     int pagesize;
     NANDFlashState *s = NAND(dev);
+    Error *local_err = NULL;
 
     s->buswidth = nand_flash_ids[s->chip_id].width >> 3;
     s->size = nand_flash_ids[s->chip_id].size << 20;
@@ -407,6 +409,11 @@ static void nand_realize(DeviceState *dev, Error **errp)
             error_setg(errp, "Can't use a read-only drive");
             return;
         }
+        blk_lock_image(s->blk, s->lock_mode, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
         if (blk_getlength(s->blk) >=
                 (s->pages << s->page_shift) + (s->pages << s->oob_shift)) {
             pagesize = 0;
@@ -427,6 +434,7 @@ static Property nand_properties[] = {
     DEFINE_PROP_UINT8("manufacturer_id", NANDFlashState, manf_id, 0),
     DEFINE_PROP_UINT8("chip_id", NANDFlashState, chip_id, 0),
     DEFINE_PROP_DRIVE("drive", NANDFlashState, blk),
+    DEFINE_PROP_LOCK_MODE("lock-mode", NANDFlashState, lock_mode),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.7.4

[Qemu-devel] [PATCH v8 34/36] onenand: Add 'lock-mode' property

[Fam Zheng]

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

diff --git a/hw/block/onenand.c b/hw/block/onenand.c
index 8d84227..9b058e8 100644
--- a/hw/block/onenand.c
+++ b/hw/block/onenand.c
@@ -778,6 +778,7 @@ static int onenand_initfn(SysBusDevice *sbd)
     OneNANDState *s = ONE_NAND(dev);
     uint32_t size = 1 << (24 + ((s->id.dev >> 4) & 7));
     void *ram;
+    Error *local_err = NULL;
 
     s->base = (hwaddr)-1;
     s->rdy = NULL;
@@ -796,6 +797,11 @@ static int onenand_initfn(SysBusDevice *sbd)
             error_report("Can't use a read-only drive");
             return -1;
         }
+        blk_lock_image(s->blk, s->lock_mode, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            return -1;
+        }
         s->blk_cur = s->blk;
     }
     s->otp = memset(g_malloc((64 + 2) << PAGE_SHIFT),
@@ -828,6 +834,7 @@ static Property onenand_properties[] = {
     DEFINE_PROP_UINT16("version_id", OneNANDState, id.ver, 0),
     DEFINE_PROP_INT32("shift", OneNANDState, shift, 0),
     DEFINE_PROP_DRIVE("drive", OneNANDState, blk),
+    DEFINE_PROP_LOCK_MODE("lock-mode", OneNANDState, lock_mode),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.7.4

[Qemu-devel] [PATCH v8 35/36] spapr_nvram: Add 'lock-mode' property

[Fam Zheng]

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

diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
index 4de5f70..b679e0b 100644
--- a/hw/nvram/spapr_nvram.c
+++ b/hw/nvram/spapr_nvram.c
@@ -39,6 +39,7 @@ typedef struct sPAPRNVRAM {
     uint32_t size;
     uint8_t *buf;
     BlockBackend *blk;
+    ImageLockMode lock_mode;
     VMChangeStateEntry *vmstate;
 } sPAPRNVRAM;
 
@@ -140,8 +141,14 @@ static void rtas_nvram_store(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 static void spapr_nvram_realize(VIOsPAPRDevice *dev, Error **errp)
 {
     sPAPRNVRAM *nvram = VIO_SPAPR_NVRAM(dev);
+    Error *local_err = NULL;
 
     if (nvram->blk) {
+        blk_lock_image(nvram->blk, nvram->lock_mode, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
         nvram->size = blk_getlength(nvram->blk);
     } else {
         nvram->size = DEFAULT_NVRAM_SIZE;
@@ -226,6 +233,7 @@ static const VMStateDescription vmstate_spapr_nvram = {
 static Property spapr_nvram_properties[] = {
     DEFINE_SPAPR_PROPERTIES(sPAPRNVRAM, sdev),
     DEFINE_PROP_DRIVE("drive", sPAPRNVRAM, blk),
+    DEFINE_PROP_LOCK_MODE("lock-mode", sPAPRNVRAM, lock_mode),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.7.4

[Qemu-devel] [PATCH v8 36/36] sd: Add 'lock-mode' property

[Fam Zheng]

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 8e88e83..6c18fb8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -124,6 +124,7 @@ struct SDState {
     qemu_irq readonly_cb;
     qemu_irq inserted_cb;
     BlockBackend *blk;
+    ImageLockMode lock_mode;
 
     bool enable;
 };
@@ -1886,6 +1887,7 @@ static void sd_instance_finalize(Object *obj)
 
 static void sd_realize(DeviceState *dev, Error **errp)
 {
+    Error *local_err = NULL;
     SDState *sd = SD_CARD(dev);
 
     if (sd->blk && blk_is_read_only(sd->blk)) {
@@ -1894,12 +1896,18 @@ static void sd_realize(DeviceState *dev, Error **errp)
     }
 
     if (sd->blk) {
+        blk_lock_image(sd->blk, sd->lock_mode, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
         blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
     }
 }
 
 static Property sd_properties[] = {
     DEFINE_PROP_DRIVE("drive", SDState, blk),
+    DEFINE_PROP_LOCK_MODE("lock-mode", SDState, lock_mode),
     /* We do not model the chip select pin, so allow the board to select
      * whether card should be in SSI or MMC/SD mode.  It is also up to the
      * board to ensure that ssi transfers only occur when the chip select
-- 
2.7.4

Re: [Qemu-devel] [PATCH v8 02/36] qapi: Add ImageLockMode

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1412 bytes --]

On 30.09.2016 14:09, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qapi/block-core.json | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 92193ab..22e8d04 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2754,3 +2754,21 @@
>    'data' : { 'parent': 'str',
>               '*child': 'str',
>               '*node': 'str' } }
> +
> +##
> +# @ImageLockMode:
> +#
> +# @auto: defer to the block driver to use the least strict mode, based on
> +#        the nature of format and read-only flag, and the supported locking
> +#        operations of the protocol.

I have some difficulty understanding this description. I'd intuitively
assume no locking to be the "least strict mode"; however, since it
should be always possible not to lock an image, this would mean that
auto=nolock. Which is hopefully isn't.

If it's not easy to come up with a thorough explanation, perhaps it
would be best to give some examples which help to understand the concept
behind "auto" intuitively.

Max

> +#
> +# @exclusive: always exclusively lock the image.
> +#
> +# @shared: use a shared lock mode.
> +#
> +# @nolock: don't lock the image.
> +#
> +# Since: 2.8
> +##
> +{ 'enum': 'ImageLockMode',
> +  'data': ['auto', 'exclusive', 'shared', 'nolock'] }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 03/36] block: Introduce image file locking

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4635 bytes --]

On 30.09.2016 14:09, Fam Zheng wrote:
> 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                   | 52 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h     |  4 +++-
>  include/block/block_int.h |  5 +++++
>  include/hw/block/block.h  |  2 ++
>  4 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 493ecf3..9d600df 100644
> --- a/block.c
> +++ b/block.c
> @@ -235,6 +235,7 @@ BlockDriverState *bdrv_new(void)
>      notifier_with_return_list_init(&bs->before_write_notifiers);
>      bs->refcnt = 1;
>      bs->aio_context = qemu_get_aio_context();
> +    bs->cur_lock = IMAGE_LOCK_MODE__MAX;

(Yes, I know, I'm supposed to write a high-level review, but...)

I don't really like using values for enums that are not actually
supposed to be part of the enum. Maybe nolock would be a reasonable choice?

>      qemu_co_queue_init(&bs->flush_queue);
>  
> @@ -925,6 +926,48 @@ out:
>      g_free(gen_node_name);
>  }
>  
> +ImageLockMode bdrv_lock_mode_from_flags(int flags)
> +{
> +    if (flags & BDRV_O_NO_LOCK) {
> +        return IMAGE_LOCK_MODE_NOLOCK;
> +    } else if (flags & BDRV_O_SHARED_LOCK) {
> +        return IMAGE_LOCK_MODE_SHARED;
> +    } else if (flags & BDRV_O_EXCLUSIVE_LOCK) {
> +        return IMAGE_LOCK_MODE_EXCLUSIVE;
> +    } else {
> +        return IMAGE_LOCK_MODE_AUTO;
> +    }
> +}

I don't know if there's been any discussion about the order of the flags
here, but I personally would order them exactly the other way around:
Asking for exclusive locking should override nolock, in my opinion.

> +
> +ImageLockMode bdrv_get_lock_mode(BlockDriverState *bs)
> +{
> +    return bs->cur_lock;
> +}
> +
> +int bdrv_set_lock_mode(BlockDriverState *bs, ImageLockMode mode)
> +{
> +    int ret;
> +
> +    if (bs->cur_lock == mode) {
> +        return 0;
> +    } else if (!bs->drv) {
> +        return -ENOMEDIUM;
> +    } else if (!bs->drv->bdrv_lockf) {
> +        if (bs->file) {
> +            return bdrv_set_lock_mode(bs->file->bs, mode);
> +        }
> +        return 0;
> +    }
> +    ret = bs->drv->bdrv_lockf(bs, mode);
> +    if (ret == -ENOTSUP) {
> +        /* Handle it the same way as !bs->drv->bdrv_lockf */
> +        ret = 0;

Yes, well, why do you handle both as success? Wouldn't returning
-ENOTSUP make more sense?

I guess the caller can find out itself by checking whether bs->cur_lock
has changed, but...

> +    } else if (ret == 0) {
> +        bs->cur_lock = mode;
> +    }
> +    return ret;
> +}
> +
>  static QemuOptsList bdrv_runtime_opts = {
>      .name = "bdrv_common",
>      .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
> @@ -1076,6 +1119,10 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
>          goto free_and_fail;
>      }
>  
> +    if (open_flags & BDRV_O_INACTIVE) {
> +        open_flags = (open_flags & ~BDRV_O_LOCK_MASK) & BDRV_O_NO_LOCK;

I suppose the second & is supposed to be a |?

> +    }
> +
>      ret = refresh_total_sectors(bs, bs->total_sectors);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Could not refresh total sector count");
> @@ -2273,6 +2320,7 @@ static void bdrv_close(BlockDriverState *bs)
>      if (bs->drv) {
>          BdrvChild *child, *next;
>  
> +        bdrv_set_lock_mode(bs, IMAGE_LOCK_MODE_NOLOCK);
>          bs->drv->bdrv_close(bs);
>          bs->drv = NULL;
>  
> @@ -3188,6 +3236,9 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)

This function's name is pretty weird... Maybe it would be better to
rename it to "bdrv_complete_incoming" or something. (Unrelated to this
series, of course.)

>          error_setg_errno(errp, -ret, "Could not refresh total sector count");
>          return;
>      }
> +    if (bs->cur_lock != IMAGE_LOCK_MODE__MAX) {
> +        bdrv_set_lock_mode(bs, bs->cur_lock);
> +    }
>  }
>  
>  void bdrv_invalidate_cache_all(Error **errp)
> @@ -3230,6 +3281,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
>      }
>  
>      if (setting_flag) {
> +        ret = bdrv_set_lock_mode(bs, IMAGE_LOCK_MODE_NOLOCK);

Maybe it would make sense to do something with the return value...? :-)

At least you should probably check whether bdrv_get_lock_mode(bs) ==
IMAGE_LOCK_MODE_NOLOCK.

Max

>          bs->open_flags |= BDRV_O_INACTIVE;
>      }
>      return 0;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 04/36] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]

On 30.09.2016 14:09, Fam Zheng wrote:
> They are wrappers of POSIX fcntl "file private locking".
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  include/qemu/osdep.h |  2 ++
>  util/osdep.c         | 29 +++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 9e9fa61..f773f49 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -286,6 +286,8 @@ int qemu_close(int fd);
>  #ifndef _WIN32
>  int qemu_dup(int fd);
>  #endif
> +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> +int qemu_unlock_fd(int fd, int64_t start, int64_t len);
>  
>  #if defined(__HAIKU__) && defined(__i386__)
>  #define FMT_pid "%ld"
> diff --git a/util/osdep.c b/util/osdep.c
> index 06fb1cf..b85a490 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -140,6 +140,35 @@ static int qemu_parse_fdset(const char *param)
>  {
>      return qemu_parse_fd(param);
>  }
> +
> +static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
> +{
> +#ifdef F_OFD_SETLK
> +    int ret;
> +    struct flock fl = {
> +        .l_whence = SEEK_SET,
> +        .l_start  = start,
> +        .l_len    = len,
> +        .l_type   = fl_type,
> +    };
> +    do {
> +        ret = fcntl(fd, F_OFD_SETLK, &fl);
> +    } while (ret == -1 && errno == EINTR);

Can EINTR happen? My fcntl() man page claims it's only possible with
F(_OFD)_SETLKW.

Max

> +    return ret == -1 ? -errno : 0;
> +#else
> +    return -ENOTSUP;
> +#endif
> +}
> +
> +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive)
> +{
> +    return qemu_lock_fcntl(fd, start, len, exclusive ? F_WRLCK : F_RDLCK);
> +}
> +
> +int qemu_unlock_fd(int fd, int64_t start, int64_t len)
> +{
> +    return qemu_lock_fcntl(fd, start, len, F_UNLCK);
> +}
>  #endif
>  
>  /*
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 05/36] raw-posix: Add image locking support

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 16241 bytes --]

On 30.09.2016 14:09, Fam Zheng wrote:
> virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> the intervene.

s/the intervene/a conflict/?

> 
> Both file and host device protocols are covered.
> 
> The complication is with reopen. We have three different locking states,
> namely "unlocked", "shared locked" and "exclusively locked".
> 
> When we reopen, the new fd may need a new locking mode. Moving away to or from
> exclusive is a bit tricky because we cannot do it atomically. This patch solves
> it by dup() s->fd to s->lock_fd and avoid close(), so that there isn't a racy
> window where we drop the lock on one fd before acquiring the exclusive lock on
> the other.
> 
> To make the logic easier to manage, and allow better reuse, the code is
> internally organized by state transition table (old_lock -> new_lock).
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/raw-posix.c | 318 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 317 insertions(+), 1 deletion(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 6ed7547..22de242 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -133,6 +133,7 @@ do { \
>  
>  typedef struct BDRVRawState {
>      int fd;
> +    int lock_fd;

I think it would be a good idea to put a comment about the semantics of
this lock_fd here.

>      int type;
>      int open_flags;
>      size_t buf_align;
> @@ -149,6 +150,7 @@ typedef struct BDRVRawState {
>  
>  typedef struct BDRVRawReopenState {
>      int fd;
> +    int lock_fd;
>      int open_flags;
>  } BDRVRawReopenState;
>  
> @@ -367,6 +369,43 @@ static void raw_parse_flags(int bdrv_flags, int *open_flags)
>      }
>  }
>  
> +static int raw_lock_fd(int fd, ImageLockMode mode)
> +{
> +    assert(fd >= 0);
> +    /* Locking byte 1 avoids interfereing with virtlockd. */
> +    switch (mode) {
> +    case IMAGE_LOCK_MODE_EXCLUSIVE:
> +        return qemu_lock_fd(fd, 1, 1, true);
> +    case IMAGE_LOCK_MODE_SHARED:
> +        return qemu_lock_fd(fd, 1, 1, false);
> +    case IMAGE_LOCK_MODE_NOLOCK:
> +        return qemu_unlock_fd(fd, 1, 1);
> +    default:
> +        abort();
> +    }
> +}
> +
> +static int raw_lockf(BlockDriverState *bs, ImageLockMode mode)
> +{
> +    BDRVRawState *s = bs->opaque;
> +
> +    if (s->lock_fd < 0) {
> +        if (mode == IMAGE_LOCK_MODE_NOLOCK) {
> +            return 0;
> +        }
> +        s->lock_fd = qemu_dup(s->fd);
> +        if (s->lock_fd < 0) {
> +            return s->lock_fd;

You should probably return -errno instead (qemu_dup apparently doesn't
return that but just -1).

(I'm getting further and further from a high-level review, am I not?)

> +        }
> +    }
> +    if (mode == IMAGE_LOCK_MODE_AUTO) {
> +        mode = bdrv_get_flags(bs) & BDRV_O_RDWR ?
> +               IMAGE_LOCK_MODE_EXCLUSIVE :
> +               IMAGE_LOCK_MODE_SHARED;
> +    }
> +    return raw_lock_fd(s->lock_fd, mode);

Without a comment for how lock_fd is supposed to work, this (for
example) looks a bit weird. After all, the user is trying to lock the
fd, and I don't find it immediately clear that lock_fd always points to
the same file as fd, and you're using it only for locking (for reasons
the comment should explain as well).

> +}
> +
>  #ifdef CONFIG_LINUX_AIO
>  static bool raw_use_aio(int bdrv_flags)
>  {
> @@ -433,6 +472,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>      raw_parse_flags(bdrv_flags, &s->open_flags);
>  
>      s->fd = -1;
> +    s->lock_fd = -1;
>      fd = qemu_open(filename, s->open_flags, 0644);
>      if (fd < 0) {
>          ret = -errno;
> @@ -529,6 +569,268 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
>      return raw_open_common(bs, options, flags, 0, errp);
>  }
>  
> +typedef enum {
> +    RAW_REOPEN_PREPARE,
> +    RAW_REOPEN_COMMIT,
> +    RAW_REOPEN_ABORT
> +} RawReopenOperation;
> +
> +typedef int (*RawReopenFunc)(BDRVReopenState *state,
> +                             RawReopenOperation op,
> +                             ImageLockMode old_lock,
> +                             ImageLockMode new_lock,
> +                             Error **errp);
> +
> +static int
> +raw_reopen_identical(BDRVReopenState *state,
> +                     RawReopenOperation op,
> +                     ImageLockMode old_lock,
> +                     ImageLockMode new_lock,
> +                     Error **errp)
> +{
> +    assert(old_lock == new_lock);
> +    return 0;
> +}
> +
> +static int
> +raw_reopen_from_unlock(BDRVReopenState *state,
> +                       RawReopenOperation op,
> +                       ImageLockMode old_lock,
> +                       ImageLockMode new_lock,
> +                       Error **errp)
> +{
> +    BDRVRawReopenState *raw_s = state->opaque;
> +    int ret = 0;
> +
> +    assert(old_lock != new_lock);
> +    assert(old_lock == IMAGE_LOCK_MODE_NOLOCK);
> +    switch (op) {
> +    case RAW_REOPEN_PREPARE:
> +        ret = raw_lock_fd(raw_s->lock_fd, new_lock);
> +        if (ret) {
> +            error_setg_errno(errp, -ret, "Failed to lock new fd %d", raw_s->lock_fd);
> +        }
> +        break;
> +    case RAW_REOPEN_COMMIT:
> +    case RAW_REOPEN_ABORT:
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static int
> +raw_reopen_to_unlock(BDRVReopenState *state,
> +                     RawReopenOperation op,
> +                     ImageLockMode old_lock,
> +                     ImageLockMode new_lock,
> +                     Error **errp)
> +{
> +    BDRVRawState *s = state->bs->opaque;
> +    int ret = 0;
> +
> +    assert(old_lock != new_lock);
> +    assert(new_lock == IMAGE_LOCK_MODE_NOLOCK);
> +    switch (op) {
> +    case RAW_REOPEN_PREPARE:
> +        break;
> +    case RAW_REOPEN_COMMIT:
> +        if (s->lock_fd >= 0) {
> +            qemu_close(s->lock_fd);
> +            s->lock_fd = -1;
> +        }
> +        break;
> +    case RAW_REOPEN_ABORT:
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static int
> +raw_reopen_upgrade(BDRVReopenState *state,
> +                   RawReopenOperation op,
> +                   ImageLockMode old_lock,
> +                   ImageLockMode new_lock,
> +                   Error **errp)
> +{
> +    BDRVRawReopenState *raw_s = state->opaque;
> +    BDRVRawState *s = state->bs->opaque;
> +    int ret = 0, ret2;
> +
> +    assert(old_lock == IMAGE_LOCK_MODE_SHARED);
> +    assert(new_lock == IMAGE_LOCK_MODE_EXCLUSIVE);
> +    switch (op) {
> +    case RAW_REOPEN_PREPARE:
> +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> +        if (ret) {
> +            error_setg_errno(errp, -ret, "Failed to lock new fd (shared)");
> +            break;
> +        }
> +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_NOLOCK);
> +        if (ret) {
> +            error_setg_errno(errp, -ret, "Failed to unlock old fd");
> +            goto restore;
> +        }
> +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_EXCLUSIVE);
> +        if (ret) {
> +            error_setg_errno(errp, -ret, "Failed to lock new fd (exclusive)");
> +            goto restore;
> +        }
> +        break;
> +    case RAW_REOPEN_COMMIT:
> +        break;
> +    case RAW_REOPEN_ABORT:
> +        raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> +        if (ret) {
> +            error_report("Failed to restore lock on old fd");

If we get here, s->lock_fd is still locked exclusively. The following is
a very personal opinion, but anyway: I think it would be be better for
it to be unlocked. If it's locked too strictly, this can really break
something; but if it's just not locked (while it should be locked in
shared mode), everything's going to be fine unless the user makes a
mistake. I think the latter is less bad.

And we can always achieve unlocking the FD by closing s->lock_fd, so I
think that is what we should do here.

> +        }
> +        break;
> +    }
> +
> +    return ret;
> +restore:
> +    ret2 = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> +    if (ret2) {
> +        error_report("Failed to restore old lock");
> +    }
> +    return ret;
> +}
> +
> +static int
> +raw_reopen_downgrade(BDRVReopenState *state,
> +                     RawReopenOperation op,
> +                     ImageLockMode old_lock,
> +                     ImageLockMode new_lock,
> +                     Error **errp)
> +{
> +    BDRVRawReopenState *raw_s = state->opaque;
> +    BDRVRawState *s = state->bs->opaque;
> +    int ret = 0;
> +
> +    assert(old_lock == IMAGE_LOCK_MODE_EXCLUSIVE);
> +    assert(new_lock == IMAGE_LOCK_MODE_SHARED);
> +    switch (op) {
> +    case RAW_REOPEN_PREPARE:
> +        break;
> +    case RAW_REOPEN_COMMIT:
> +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> +        if (ret) {
> +            error_report("Failed to downgrade old lock");
> +            break;
> +        }
> +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> +        if (ret) {
> +            error_report("Failed to lock new fd (shared)");
> +            break;
> +        }
> +        break;

It's really unfortunate that we cannot do anything in prepare and have
to hope for the best in commit. But I can't come up with anything better.

And if something does fail, the FD will be unlocked after reopening.
That's good enough for me.

> +    case RAW_REOPEN_ABORT:
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +/**
> + * Transactionally moving between three possible locking states is tricky and
> + * must be done carefully. That is mostly because downgrading an exclusive lock
> + * to shared or unlocked is not guaranteed to be revertable. As a result, in
> + * such cases we have to defer the downgraing to "commit", given that no revert
> + * will happen after that point, and that downgrading a lock should never fail.
> + *
> + * On the other hand, upgrading a lock (e.g. from unlocked or shared to
> + * exclusive lock) must happen in "prepare" because it may fail.
> + *
> + * Manage the operation matrix with this state transition table to make
> + * fulfulling above conditions easier.
> + */
> +static const struct RawReopenFuncRecord {
> +    ImageLockMode old_lock;
> +    ImageLockMode new_lock;
> +    RawReopenFunc func;
> +    bool need_lock_fd;
> +} reopen_functions[] = {
> +    {IMAGE_LOCK_MODE_NOLOCK, IMAGE_LOCK_MODE_NOLOCK, raw_reopen_identical, false},
> +    {IMAGE_LOCK_MODE_NOLOCK, IMAGE_LOCK_MODE_SHARED, raw_reopen_from_unlock, true},
> +    {IMAGE_LOCK_MODE_NOLOCK, IMAGE_LOCK_MODE_EXCLUSIVE, raw_reopen_from_unlock, true},
> +    {IMAGE_LOCK_MODE_SHARED, IMAGE_LOCK_MODE_NOLOCK, raw_reopen_to_unlock, false},
> +    {IMAGE_LOCK_MODE_SHARED, IMAGE_LOCK_MODE_SHARED, raw_reopen_identical, false},
> +    {IMAGE_LOCK_MODE_SHARED, IMAGE_LOCK_MODE_EXCLUSIVE, raw_reopen_upgrade, true},
> +    {IMAGE_LOCK_MODE_EXCLUSIVE, IMAGE_LOCK_MODE_NOLOCK, raw_reopen_to_unlock, false},
> +    {IMAGE_LOCK_MODE_EXCLUSIVE, IMAGE_LOCK_MODE_SHARED, raw_reopen_downgrade, true},
> +    {IMAGE_LOCK_MODE_EXCLUSIVE, IMAGE_LOCK_MODE_EXCLUSIVE, raw_reopen_identical, false},
> +};
> +
> +static int raw_reopen_handle_lock(BDRVReopenState *state,
> +                                  RawReopenOperation op,
> +                                  Error **errp)
> +{
> +    BDRVRawReopenState *raw_s = state->opaque;

Please choose another name, it's hard not to confuse this with the
BDRVRawState all the time. (e.g. raw_rs or just rs would be enough.)

(Same in the places above, I got confused more than once...)

> +    BDRVRawState *s = state->bs->opaque;
> +    ImageLockMode old_lock, new_lock;
> +    const struct RawReopenFuncRecord *rec;
> +    int ret;
> +
> +    old_lock = bdrv_get_lock_mode(state->bs);
> +    new_lock = bdrv_lock_mode_from_flags(state->flags);
> +
> +    if (old_lock == IMAGE_LOCK_MODE__MAX) {
> +        /* bs was not locked, leave it unlocked. */
> +        old_lock = new_lock = IMAGE_LOCK_MODE_NOLOCK;
> +    }
> +
> +    if (old_lock == IMAGE_LOCK_MODE_AUTO) {
> +        old_lock = bdrv_get_flags(state->bs) & BDRV_O_RDWR ?
> +                   IMAGE_LOCK_MODE_EXCLUSIVE : IMAGE_LOCK_MODE_SHARED;
> +    }
> +
> +    if (new_lock == IMAGE_LOCK_MODE_AUTO) {
> +        new_lock = state->flags & BDRV_O_RDWR ?
> +                   IMAGE_LOCK_MODE_EXCLUSIVE : IMAGE_LOCK_MODE_SHARED;
> +    }
> +
> +    for (rec = &reopen_functions[0];
> +         rec < &reopen_functions[ARRAY_SIZE(reopen_functions)];
> +         rec++) {
> +        if (rec->old_lock == old_lock && rec->new_lock == new_lock) {
> +            break;
> +        }
> +    }
> +    assert(rec != &reopen_functions[ARRAY_SIZE(reopen_functions)]);
> +
> +    switch (op) {
> +    case RAW_REOPEN_PREPARE:
> +        if (rec->need_lock_fd) {
> +            ret = qemu_dup(raw_s->fd);
> +            if (ret < 0) {
> +                error_setg_errno(errp, -ret, "Failed to dup new fd");
> +                return ret;
> +            }
> +            raw_s->lock_fd = ret;
> +        }
> +        return rec->func(state, op, old_lock, new_lock, errp);
> +    case RAW_REOPEN_COMMIT:
> +        rec->func(state, op, old_lock, new_lock, errp);
> +        if (rec->need_lock_fd) {
> +            if (s->lock_fd >= 0) {
> +                qemu_close(s->lock_fd);
> +            }
> +            s->lock_fd = raw_s->lock_fd;
> +        }
> +        break;
> +    case RAW_REOPEN_ABORT:
> +        rec->func(state, op, old_lock, new_lock, errp);
> +        if (rec->need_lock_fd && raw_s->lock_fd >= 0) {
> +            qemu_close(raw_s->lock_fd);
> +            raw_s->lock_fd = -1;
> +        }
> +        break;
> +    }
> +    return 0;
> +}
> +
>  static int raw_reopen_prepare(BDRVReopenState *state,
>                                BlockReopenQueue *queue, Error **errp)
>  {
> @@ -607,6 +909,10 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>          }
>      }
>  
> +    if (!ret) {
> +        ret = raw_reopen_handle_lock(state, RAW_REOPEN_PREPARE, errp);
> +    }
> +
>      return ret;
>  }
>  
> @@ -617,6 +923,8 @@ static void raw_reopen_commit(BDRVReopenState *state)
>  
>      s->open_flags = raw_s->open_flags;
>  
> +    raw_reopen_handle_lock(state, RAW_REOPEN_COMMIT, NULL);

I'd prefer &error_abort instead of NULL.

> +
>      qemu_close(s->fd);
>      s->fd = raw_s->fd;
>  
> @@ -634,6 +942,8 @@ static void raw_reopen_abort(BDRVReopenState *state)
>          return;
>      }
>  
> +    raw_reopen_handle_lock(state, RAW_REOPEN_ABORT, NULL);

Same here.

> +
>      if (raw_s->fd >= 0) {
>          qemu_close(raw_s->fd);
>          raw_s->fd = -1;
> @@ -1321,6 +1631,10 @@ static void raw_close(BlockDriverState *bs)
>          qemu_close(s->fd);
>          s->fd = -1;
>      }
> +    if (s->lock_fd >= 0) {
> +        qemu_close(s->lock_fd);
> +        s->lock_fd = -1;
> +    }
>  }
>  
>  static int raw_truncate(BlockDriverState *bs, int64_t offset)
> @@ -1874,7 +2188,7 @@ BlockDriver bdrv_file = {
>      .bdrv_get_info = raw_get_info,
>      .bdrv_get_allocated_file_size
>                          = raw_get_allocated_file_size,
> -
> +    .bdrv_lockf = raw_lockf,
>      .create_opts = &raw_create_opts,
>  };
>  
> @@ -2324,6 +2638,8 @@ static BlockDriver bdrv_host_device = {
>  #ifdef __linux__
>      .bdrv_aio_ioctl     = hdev_aio_ioctl,
>  #endif
> +
> +    .bdrv_lockf = raw_lockf,
>  };
>  
>  #if defined(__linux__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> 

Overall design looks good to me. Unfortunately we can't strictly follow
the transaction principle for downgrades and upgrades, but I guess it'll
work most of the time (fingers crossed).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 12/36] virtio-blk: Apply lock-mode when realize

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]

On 30.09.2016 14:09, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/block/virtio-blk.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 3a6112f..ce65615 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -896,6 +896,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>          error_setg(errp, "num-queues property must be larger than 0");
>          return;
>      }
> +    blk_lock_image(conf->conf.blk, conf->conf.lock_mode, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
>  
>      blkconf_serial(&conf->conf, &conf->serial);
>      blkconf_apply_backend_options(&conf->conf);
> 

Hmmm... Patch 3 introduced the conf.lock_mode field, but didn't do
anything with it. That is a bit weird. Now you're applying it here but
can't set it anywhere. That's a bit weird, too. Well, behavior won't
change as this probably just means that now everything explicitly
unlocked instead of implicitly "because we don't have locking yet".

Maybe it would make sense to move the introduction of conf.lock_mode to
an own patch and explain in the commit message that this field will be
implicitly set to 0, so until the user is able to control the field,
every BB (and thus BDS tree) will not be locked (thus not changing
behavior).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 15/36] qdev: Add "lock-mode" to block device options

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On 30.09.2016 14:09, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/core/qdev-properties.c    | 10 ++++++++++
>  include/hw/block/block.h     |  1 +
>  include/hw/qdev-properties.h |  3 +++
>  3 files changed, 14 insertions(+)

Why don't you add _conf.lock_mode in this very patch (instead of patch
3) and pull it in front of patch 12? Setting the mode won't do anything
until it's implemented for the various block devices, but I don't think
that's a bad thing.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 12/36] virtio-blk: Apply lock-mode when realize

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1742 bytes --]

On 22.10.2016 02:08, Max Reitz wrote:
> On 30.09.2016 14:09, Fam Zheng wrote:
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>  hw/block/virtio-blk.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index 3a6112f..ce65615 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -896,6 +896,11 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>>          error_setg(errp, "num-queues property must be larger than 0");
>>          return;
>>      }
>> +    blk_lock_image(conf->conf.blk, conf->conf.lock_mode, &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>>  
>>      blkconf_serial(&conf->conf, &conf->serial);
>>      blkconf_apply_backend_options(&conf->conf);
>>
> 
> Hmmm... Patch 3 introduced the conf.lock_mode field, but didn't do
> anything with it. That is a bit weird. Now you're applying it here but
> can't set it anywhere. That's a bit weird, too. Well, behavior won't
> change as this probably just means that now everything explicitly
> unlocked instead of implicitly "because we don't have locking yet".
> 
> Maybe it would make sense to move the introduction of conf.lock_mode to
> an own patch and explain in the commit message that this field will be
> implicitly set to 0, so until the user is able to control the field,
> every BB (and thus BDS tree) will not be locked (thus not changing
> behavior).

Oops, just noticed that it won't be "unlocked" but "auto-locked" (all
the more reason to mention the implicit behavior somewhere). But maybe
my comment for patch 15 has obsoleted this comment altogether.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5105 bytes --]

On 30.09.2016 14:09, Fam Zheng wrote:
> Hi all,
> 
> I wanted to post something before the long holiday as promised, but I couldn't
> refine or test enough due to limited time.  Please take this as an RFC and do a
> high level review. Thanks.

I did that, looked at some patches in more and at some in less detail.
All patches I didn't reply to looked roughly fine to me (although that
doesn't have to mean much, as I really didn't take a close look at many
patches :-)).

> 
> v8: Move user interface option from block device to qdev. [Kevin]
>     Add "exclusive" back. [Kevin]
> 
>     Note: limited by the qdev interface, until blk_lock_image() is called, the
>     images are not locked by block layer at open, because before device tells
>     us what to do, block layer cannot figure out the correct lock mode any
>     more.
> 
>     TODO 1: Lock explicitly in utils (qemu-img, qemu-io, etc) after opening
>     image.
> 
>     TODO 2: The image locking test case 153 patch is not updated thus broken
>     because of the moved option.
> 
>     TODO 3: Are the open flags unnecessary because we already have
>     ImageLockMode? If so, how to converge them?

Well, yes, what do the flags do now? :-)

BDRV_O_NO_LOCK is still used, but it could probably be replaced by a
call to blk_lock_image(). BDRV_O_SHARED_LOCK and BDRV_O_EXCLUSIVE_LOCK
are no longer used at all, though.

<parenthesis>

I personally still don't like making locking a qdev property very much
because it doesn't make sense to me*. But I remember Kevin had his
reasons (even though I can no longer remember them) for asking for it,
and I don't have any besides "It doesn't make sense to me". After having
though a bit about it (= having written three paragraphs and deleted
them again), I guess I can make some sense of it, though it seems to be
a rather esoteric use case still; it appears to me that a guest could
use it to signal that it's fine with some block device being shared;
then we could use a shared lock or none at all or I don't know.
Otherwise, we should get an exclusive lock for write access and a shared
lock for read access.

(Although, fun question (that's the reason for the "I don't know"): What
if guest A supports such a volatile block device, but guest B doesn't?
Then imagine we run A with write access and B with read-only access. A
will claim no lock or a shared lock, while B will probably claim a
shared lock, too, expecting writing users to try to grab an exclusive
lock. So suddenly both can open the image file, but B isn't actually
prepared for that. Fun!)

So as far as I understand it, those qdev properties should eventually be
changeable by the guest. And if that is so, the user probably shouldn't
touch them at all because the guest OS really knows whether it can cope
with volatile block devices or not, and it will tell qemu if that is so.

That may be the reason why the qdev property doesn't make sense to me at
the first glance: It does make sense for the guest OS to set this
property at that level, but it doesn't make sense for the user to do so.
Or maybe it does, but the user is really asking for things breaking then
("YES, I AM SURE THAT MY GUEST WILL BE COMPLETELY FINE WITH FLOPPY DISKS
JUST CHANGING RANDOMLY" -- but I guess we've all been at that point
ourselves...).

So after having convinced myself twice now, having the qdev property is
probably correct.

</parenthesis>

But that's where the flags come in again: The guest may be fine with a
shared lock or none at all. But what about the block layer? Say you're
using a qcow2 image; that will not like sharing at all, independently of
what the guest things. So the block layer needs to have internal
mechanisms to elevate the locks proposed by the guest devices to e.g.
exclusive ones. I don't think that is something addressed by this series
in this version. Maybe you'll still need the flags for that.


Final thing: Say you have a user who's crazy. Maybe they want to debug
something. Anyway, they have some qcow2 image attached to some guest
device, and they want to access that image simultaneously from some
other process; as I said, crazy, but there may be legitimate uses for
that so we should allow it.

Now first that user of course has to set the device's lock_mode to
shared or none, thus promising qemu that the guest will be fine with
volatile data. But of course qcow2 will override that lock mode, because
qcow2 doesn't care about the guest: It primarily cares about its own
metadata integrity. So at this point the user will need some way to
override qcow2's choice, too. Therefore, I think we also need a per-node
flag to override the locking mode.

qcow2 would probably make this flag default to exclusive for its
underlying node, and the user would then have to override that flag for
exactly that underlying node.

Therefore I think we still need a block layer property for the lock
mode. While I now agree that we do need a qdev property, I think that
alone won't be sufficient.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 7007 bytes --]

Am 22.10.2016 um 03:00 hat Max Reitz geschrieben:
> <parenthesis>
> 
> I personally still don't like making locking a qdev property very much
> because it doesn't make sense to me*. But I remember Kevin had his
> reasons (even though I can no longer remember them) for asking for it,
> and I don't have any besides "It doesn't make sense to me". After having
> though a bit about it (= having written three paragraphs and deleted
> them again), I guess I can make some sense of it, though it seems to be
> a rather esoteric use case still; it appears to me that a guest could
> use it to signal that it's fine with some block device being shared;
> then we could use a shared lock or none at all or I don't know.
> Otherwise, we should get an exclusive lock for write access and a shared
> lock for read access.

The reason is pretty simple if you think about this question: Why do we
need user input in the first place? It's just because we don't know the
requiremens of the guest, and unfortunately the guest won't tell us
(because things like shared IDE disks simply don't exist on real
hardware). So we need ask the user, and we should do this in a layer as
close to the guest (which is the origin of the requirement) as possible.
This point is the device emulation.

Everything that isn't a guest device is under the control of qemu, so
we don't need user input there.

> (Although, fun question (that's the reason for the "I don't know"): What
> if guest A supports such a volatile block device, but guest B doesn't?
> Then imagine we run A with write access and B with read-only access. A
> will claim no lock or a shared lock, while B will probably claim a
> shared lock, too, expecting writing users to try to grab an exclusive
> lock. So suddenly both can open the image file, but B isn't actually
> prepared for that. Fun!)

Hmm, that's a good point actually.

And in fact, it reminds me how I suggested that the problem at hand is
really similar to what the new op blockers are supposed to do. And the
system I proposed for those already handles this situation just fine.

The relevant part here is how the proposal had two bit masks per user,
one describing which operatings the user needs to be able to perform
itself, and the other one for operations that other users are still
allowed perform. This means that for every operation you have four
possible states: Exclusively used; shared use; unused, but blocking
other users; unused, but allowed for other users.

If we interpret the locking flag as the operation "writes to the image",
in your example, A would have the mode "shared use" and B would have
"unused, but blocking". These two modes are in conflict.

Now, the big question is how to translate this into file locking. This
could become a little tricky. I had a few thoughts involving another
lock on byte 2, but none of them actually worked out so far, because
what we want is essentially a lock that can be shared by readers, that
can also be shared by writers, but not by readers and writers at the
same time.

> So as far as I understand it, those qdev properties should eventually be
> changeable by the guest. And if that is so, the user probably shouldn't
> touch them at all because the guest OS really knows whether it can cope
> with volatile block devices or not, and it will tell qemu if that is so.

This is an interesting thought for PV devices, but it hasn't been part
of the plan so far. I'm not sure if the guest OS block device drivers
even have this information generally, because that's more a matter of
whether a cluster file system is used on the block device or not.

> That may be the reason why the qdev property doesn't make sense to me at
> the first glance: It does make sense for the guest OS to set this
> property at that level, but it doesn't make sense for the user to do so.
> Or maybe it does, but the user is really asking for things breaking then
> ("YES, I AM SURE THAT MY GUEST WILL BE COMPLETELY FINE WITH FLOPPY DISKS
> JUST CHANGING RANDOMLY" -- but I guess we've all been at that point
> ourselves...).
> 
> So after having convinced myself twice now, having the qdev property is
> probably correct.

Essentially the whole reason is that we need the information and the
guest doesn't tell us, so we need to ask someone who supposedly knows
enough about the guest - which is the user.

> </parenthesis>
> 
> But that's where the flags come in again: The guest may be fine with a
> shared lock or none at all. But what about the block layer? Say you're
> using a qcow2 image; that will not like sharing at all, independently of
> what the guest things. So the block layer needs to have internal
> mechanisms to elevate the locks proposed by the guest devices to e.g.
> exclusive ones. I don't think that is something addressed by this series
> in this version. Maybe you'll still need the flags for that.

I'm not completely sure about the mechanism and whether it should
involve flags, but yes, you need something to propagate the required
locking mode down recursively (just like op blockers will need to be
done - maybe we should really use the same infrastructure and only
do file locking as its implementation of the lowest layer).

What you don't need, however, is user input. We already know that qcow2
doesn't like concurrent writes.

> Final thing: Say you have a user who's crazy. Maybe they want to debug
> something. Anyway, they have some qcow2 image attached to some guest
> device, and they want to access that image simultaneously from some
> other process; as I said, crazy, but there may be legitimate uses for
> that so we should allow it.
> 
> Now first that user of course has to set the device's lock_mode to
> shared or none, thus promising qemu that the guest will be fine with
> volatile data. But of course qcow2 will override that lock mode, because
> qcow2 doesn't care about the guest: It primarily cares about its own
> metadata integrity. So at this point the user will need some way to
> override qcow2's choice, too. Therefore, I think we also need a per-node
> flag to override the locking mode.
> 
> qcow2 would probably make this flag default to exclusive for its
> underlying node, and the user would then have to override that flag for
> exactly that underlying node.
> 
> Therefore I think we still need a block layer property for the lock
> mode. While I now agree that we do need a qdev property, I think that
> alone won't be sufficient.

Attaching a qcow2 image to two processes simply doesn't work unless both
are read-only, so I disagree that we should allow it. Or can you really
come up with a specific reasonable use case that requires this?

Users crazy enough to want something like this patch qemu. Both of us
have patched qemu for debugging guests before and we didn't do things as
crazy as writing to the same qcow2 image from multiple processes.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5756 bytes --]

On 24.10.2016 12:11, Kevin Wolf wrote:

[...]

> Now, the big question is how to translate this into file locking. This
> could become a little tricky. I had a few thoughts involving another
> lock on byte 2, but none of them actually worked out so far, because
> what we want is essentially a lock that can be shared by readers, that
> can also be shared by writers, but not by readers and writers at the
> same time.

You can also share it between readers and writers, as long as everyone
can cope with volatile data.

I agree that it's very similar to the proposed op blocker style, but I
can't really come up with a meaningful translation either.

Maybe something like this (?): All readers who do not want the file to
be modified grab a shared lock on byte 1. All writers who can deal with
volatile data grab a shared lock on byte 2. Exclusive writers grab an
exclusive lock on byte 1 and 2. Readers who can cope with volatile data
get no lock at all.

When opening, the first and second group would always have to test
whether there is a lock on the other byte, respectively. E.g. sharing
writers would first grab an exclusive lock on byte 1, then the shared
lock on byte 2 and then release the exclusive lock again.

Would that work?

>> So as far as I understand it, those qdev properties should eventually be
>> changeable by the guest. And if that is so, the user probably shouldn't
>> touch them at all because the guest OS really knows whether it can cope
>> with volatile block devices or not, and it will tell qemu if that is so.
> 
> This is an interesting thought for PV devices, but it hasn't been part
> of the plan so far. I'm not sure if the guest OS block device drivers
> even have this information generally, because that's more a matter of
> whether a cluster file system is used on the block device or not.
> 
>> That may be the reason why the qdev property doesn't make sense to me at
>> the first glance: It does make sense for the guest OS to set this
>> property at that level, but it doesn't make sense for the user to do so.
>> Or maybe it does, but the user is really asking for things breaking then
>> ("YES, I AM SURE THAT MY GUEST WILL BE COMPLETELY FINE WITH FLOPPY DISKS
>> JUST CHANGING RANDOMLY" -- but I guess we've all been at that point
>> ourselves...).
>>
>> So after having convinced myself twice now, having the qdev property is
>> probably correct.
> 
> Essentially the whole reason is that we need the information and the
> guest doesn't tell us, so we need to ask someone who supposedly knows
> enough about the guest - which is the user.

Hm, OK, fair enough.

>> </parenthesis>
>>
>> But that's where the flags come in again: The guest may be fine with a
>> shared lock or none at all. But what about the block layer? Say you're
>> using a qcow2 image; that will not like sharing at all, independently of
>> what the guest things. So the block layer needs to have internal
>> mechanisms to elevate the locks proposed by the guest devices to e.g.
>> exclusive ones. I don't think that is something addressed by this series
>> in this version. Maybe you'll still need the flags for that.
> 
> I'm not completely sure about the mechanism and whether it should
> involve flags, but yes, you need something to propagate the required
> locking mode down recursively (just like op blockers will need to be
> done - maybe we should really use the same infrastructure and only
> do file locking as its implementation of the lowest layer).

Sounds good to me, but that implies that we have the new op blockers
rather soon. ;-)

> What you don't need, however, is user input. We already know that qcow2
> doesn't like concurrent writes.
> 
>> Final thing: Say you have a user who's crazy. Maybe they want to debug
>> something. Anyway, they have some qcow2 image attached to some guest
>> device, and they want to access that image simultaneously from some
>> other process; as I said, crazy, but there may be legitimate uses for
>> that so we should allow it.
>>
>> Now first that user of course has to set the device's lock_mode to
>> shared or none, thus promising qemu that the guest will be fine with
>> volatile data. But of course qcow2 will override that lock mode, because
>> qcow2 doesn't care about the guest: It primarily cares about its own
>> metadata integrity. So at this point the user will need some way to
>> override qcow2's choice, too. Therefore, I think we also need a per-node
>> flag to override the locking mode.
>>
>> qcow2 would probably make this flag default to exclusive for its
>> underlying node, and the user would then have to override that flag for
>> exactly that underlying node.
>>
>> Therefore I think we still need a block layer property for the lock
>> mode. While I now agree that we do need a qdev property, I think that
>> alone won't be sufficient.
> 
> Attaching a qcow2 image to two processes simply doesn't work unless both
> are read-only, so I disagree that we should allow it. Or can you really
> come up with a specific reasonable use case that requires this?

Yes, it's more fun if you know that all of your data can suddenly be
broken beyond repair. It gives you this great kick of excitement.

> Users crazy enough to want something like this patch qemu. Both of us
> have patched qemu for debugging guests before and we didn't do things as
> crazy as writing to the same qcow2 image from multiple processes.

Now that you mention it, indeed I have not. I wonder why. I should give
it a try some day.

OK, agreed that we don't need a block layer flag. If someone later finds
out that we do for some weird reason, we can still add it.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 02/36] qapi: Add ImageLockMode

[Fam Zheng]

On Fri, 10/21 22:45, Max Reitz wrote:
> On 30.09.2016 14:09, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  qapi/block-core.json | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 92193ab..22e8d04 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -2754,3 +2754,21 @@
> >    'data' : { 'parent': 'str',
> >               '*child': 'str',
> >               '*node': 'str' } }
> > +
> > +##
> > +# @ImageLockMode:
> > +#
> > +# @auto: defer to the block driver to use the least strict mode, based on
> > +#        the nature of format and read-only flag, and the supported locking
> > +#        operations of the protocol.
> 
> I have some difficulty understanding this description. I'd intuitively
> assume no locking to be the "least strict mode"; however, since it
> should be always possible not to lock an image, this would mean that
> auto=nolock. Which is hopefully isn't.
> 
> If it's not easy to come up with a thorough explanation, perhaps it
> would be best to give some examples which help to understand the concept
> behind "auto" intuitively.

It could have beeen more specific, it's my bad being too terse here. Maybe
something like this:

    @auto: defer to the block layer to use an appropriate lock mode, based on
           the driver used and read-only option: for read-only images, shared
           lock mode, or otherwise exclusive lock mode, will be attempted; if
           the driver doesn't support this mode (or sharing is particularly
           desired by its design), nolock will be used.

?

Fam

Re: [Qemu-devel] [PATCH v8 03/36] block: Introduce image file locking

[Fam Zheng]

On Fri, 10/21 23:04, Max Reitz wrote:
> > +ImageLockMode bdrv_lock_mode_from_flags(int flags)
> > +{
> > +    if (flags & BDRV_O_NO_LOCK) {
> > +        return IMAGE_LOCK_MODE_NOLOCK;
> > +    } else if (flags & BDRV_O_SHARED_LOCK) {
> > +        return IMAGE_LOCK_MODE_SHARED;
> > +    } else if (flags & BDRV_O_EXCLUSIVE_LOCK) {
> > +        return IMAGE_LOCK_MODE_EXCLUSIVE;
> > +    } else {
> > +        return IMAGE_LOCK_MODE_AUTO;
> > +    }
> > +}
> 
> I don't know if there's been any discussion about the order of the flags
> here, but I personally would order them exactly the other way around:
> Asking for exclusive locking should override nolock, in my opinion.

The idea was to assert no two bits are set at the same time. But I seem to have
forgotten to actually add the assertion.

> 
> > +
> > +ImageLockMode bdrv_get_lock_mode(BlockDriverState *bs)
> > +{
> > +    return bs->cur_lock;
> > +}
> > +
> > +int bdrv_set_lock_mode(BlockDriverState *bs, ImageLockMode mode)
> > +{
> > +    int ret;
> > +
> > +    if (bs->cur_lock == mode) {
> > +        return 0;
> > +    } else if (!bs->drv) {
> > +        return -ENOMEDIUM;
> > +    } else if (!bs->drv->bdrv_lockf) {
> > +        if (bs->file) {
> > +            return bdrv_set_lock_mode(bs->file->bs, mode);
> > +        }
> > +        return 0;
> > +    }
> > +    ret = bs->drv->bdrv_lockf(bs, mode);
> > +    if (ret == -ENOTSUP) {
> > +        /* Handle it the same way as !bs->drv->bdrv_lockf */
> > +        ret = 0;
> 
> Yes, well, why do you handle both as success? Wouldn't returning
> -ENOTSUP make more sense?
> 
> I guess the caller can find out itself by checking whether bs->cur_lock
> has changed, but...

I can't think of a reason for any caller to do something different for -ENOTSUP
from success, hence the check here.

> 
> > +    } else if (ret == 0) {
> > +        bs->cur_lock = mode;
> > +    }
> > +    return ret;
> > +}
> > +
> >  static QemuOptsList bdrv_runtime_opts = {
> >      .name = "bdrv_common",
> >      .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
> > @@ -1076,6 +1119,10 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
> >          goto free_and_fail;
> >      }
> >  
> > +    if (open_flags & BDRV_O_INACTIVE) {
> > +        open_flags = (open_flags & ~BDRV_O_LOCK_MASK) & BDRV_O_NO_LOCK;
> 
> I suppose the second & is supposed to be a |?

Yes. Thanks for catching it.

> 
> > +    }
> > +
> >      ret = refresh_total_sectors(bs, bs->total_sectors);
> >      if (ret < 0) {
> >          error_setg_errno(errp, -ret, "Could not refresh total sector count");
> > @@ -2273,6 +2320,7 @@ static void bdrv_close(BlockDriverState *bs)
> >      if (bs->drv) {
> >          BdrvChild *child, *next;
> >  
> > +        bdrv_set_lock_mode(bs, IMAGE_LOCK_MODE_NOLOCK);
> >          bs->drv->bdrv_close(bs);
> >          bs->drv = NULL;
> >  
> > @@ -3188,6 +3236,9 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
> 
> This function's name is pretty weird... Maybe it would be better to
> rename it to "bdrv_complete_incoming" or something. (Unrelated to this
> series, of course.)
> 
> >          error_setg_errno(errp, -ret, "Could not refresh total sector count");
> >          return;
> >      }
> > +    if (bs->cur_lock != IMAGE_LOCK_MODE__MAX) {
> > +        bdrv_set_lock_mode(bs, bs->cur_lock);
> > +    }
> >  }
> >  
> >  void bdrv_invalidate_cache_all(Error **errp)
> > @@ -3230,6 +3281,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
> >      }
> >  
> >      if (setting_flag) {
> > +        ret = bdrv_set_lock_mode(bs, IMAGE_LOCK_MODE_NOLOCK);
> 
> Maybe it would make sense to do something with the return value...? :-)

Yes, sounds good.

Fam

Re: [Qemu-devel] [PATCH v8 15/36] qdev: Add "lock-mode" to block device options

[Fam Zheng]

On Sat, 10/22 02:11, Max Reitz wrote:
> On 30.09.2016 14:09, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  hw/core/qdev-properties.c    | 10 ++++++++++
> >  include/hw/block/block.h     |  1 +
> >  include/hw/qdev-properties.h |  3 +++
> >  3 files changed, 14 insertions(+)
> 
> Why don't you add _conf.lock_mode in this very patch (instead of patch
> 3) and pull it in front of patch 12? Setting the mode won't do anything
> until it's implemented for the various block devices, but I don't think
> that's a bad thing.

Sounds good!

Fam

Re: [Qemu-devel] [PATCH v8 05/36] raw-posix: Add image locking support

[Fam Zheng]

On Sat, 10/22 01:40, Max Reitz wrote:
> On 30.09.2016 14:09, Fam Zheng wrote:
> > virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> > the intervene.
> 
> s/the intervene/a conflict/?

OK.

> 
> > 
> > Both file and host device protocols are covered.
> > 
> > The complication is with reopen. We have three different locking states,
> > namely "unlocked", "shared locked" and "exclusively locked".
> > 
> > When we reopen, the new fd may need a new locking mode. Moving away to or from
> > exclusive is a bit tricky because we cannot do it atomically. This patch solves
> > it by dup() s->fd to s->lock_fd and avoid close(), so that there isn't a racy
> > window where we drop the lock on one fd before acquiring the exclusive lock on
> > the other.
> > 
> > To make the logic easier to manage, and allow better reuse, the code is
> > internally organized by state transition table (old_lock -> new_lock).
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/raw-posix.c | 318 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 317 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > index 6ed7547..22de242 100644
> > --- a/block/raw-posix.c
> > +++ b/block/raw-posix.c
> > @@ -133,6 +133,7 @@ do { \
> >  
> >  typedef struct BDRVRawState {
> >      int fd;
> > +    int lock_fd;
> 
> I think it would be a good idea to put a comment about the semantics of
> this lock_fd here.

Will do.

> 
> >      int type;
> >      int open_flags;
> >      size_t buf_align;
> > @@ -149,6 +150,7 @@ typedef struct BDRVRawState {
> >  
> >  typedef struct BDRVRawReopenState {
> >      int fd;
> > +    int lock_fd;
> >      int open_flags;
> >  } BDRVRawReopenState;
> >  
> > @@ -367,6 +369,43 @@ static void raw_parse_flags(int bdrv_flags, int *open_flags)
> >      }
> >  }
> >  
> > +static int raw_lock_fd(int fd, ImageLockMode mode)
> > +{
> > +    assert(fd >= 0);
> > +    /* Locking byte 1 avoids interfereing with virtlockd. */
> > +    switch (mode) {
> > +    case IMAGE_LOCK_MODE_EXCLUSIVE:
> > +        return qemu_lock_fd(fd, 1, 1, true);
> > +    case IMAGE_LOCK_MODE_SHARED:
> > +        return qemu_lock_fd(fd, 1, 1, false);
> > +    case IMAGE_LOCK_MODE_NOLOCK:
> > +        return qemu_unlock_fd(fd, 1, 1);
> > +    default:
> > +        abort();
> > +    }
> > +}
> > +
> > +static int raw_lockf(BlockDriverState *bs, ImageLockMode mode)
> > +{
> > +    BDRVRawState *s = bs->opaque;
> > +
> > +    if (s->lock_fd < 0) {
> > +        if (mode == IMAGE_LOCK_MODE_NOLOCK) {
> > +            return 0;
> > +        }
> > +        s->lock_fd = qemu_dup(s->fd);
> > +        if (s->lock_fd < 0) {
> > +            return s->lock_fd;
> 
> You should probably return -errno instead (qemu_dup apparently doesn't
> return that but just -1).

Yes.

> 
> (I'm getting further and further from a high-level review, am I not?)
> 
> > +        }
> > +    }
> > +    if (mode == IMAGE_LOCK_MODE_AUTO) {
> > +        mode = bdrv_get_flags(bs) & BDRV_O_RDWR ?
> > +               IMAGE_LOCK_MODE_EXCLUSIVE :
> > +               IMAGE_LOCK_MODE_SHARED;
> > +    }
> > +    return raw_lock_fd(s->lock_fd, mode);
> 
> Without a comment for how lock_fd is supposed to work, this (for
> example) looks a bit weird. After all, the user is trying to lock the
> fd, and I don't find it immediately clear that lock_fd always points to
> the same file as fd, and you're using it only for locking (for reasons
> the comment should explain as well).
> 
> > +}
> > +
> >  #ifdef CONFIG_LINUX_AIO
> >  static bool raw_use_aio(int bdrv_flags)
> >  {
> > @@ -433,6 +472,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> >      raw_parse_flags(bdrv_flags, &s->open_flags);
> >  
> >      s->fd = -1;
> > +    s->lock_fd = -1;
> >      fd = qemu_open(filename, s->open_flags, 0644);
> >      if (fd < 0) {
> >          ret = -errno;
> > @@ -529,6 +569,268 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
> >      return raw_open_common(bs, options, flags, 0, errp);
> >  }
> >  
> > +typedef enum {
> > +    RAW_REOPEN_PREPARE,
> > +    RAW_REOPEN_COMMIT,
> > +    RAW_REOPEN_ABORT
> > +} RawReopenOperation;
> > +
> > +typedef int (*RawReopenFunc)(BDRVReopenState *state,
> > +                             RawReopenOperation op,
> > +                             ImageLockMode old_lock,
> > +                             ImageLockMode new_lock,
> > +                             Error **errp);
> > +
> > +static int
> > +raw_reopen_identical(BDRVReopenState *state,
> > +                     RawReopenOperation op,
> > +                     ImageLockMode old_lock,
> > +                     ImageLockMode new_lock,
> > +                     Error **errp)
> > +{
> > +    assert(old_lock == new_lock);
> > +    return 0;
> > +}
> > +
> > +static int
> > +raw_reopen_from_unlock(BDRVReopenState *state,
> > +                       RawReopenOperation op,
> > +                       ImageLockMode old_lock,
> > +                       ImageLockMode new_lock,
> > +                       Error **errp)
> > +{
> > +    BDRVRawReopenState *raw_s = state->opaque;
> > +    int ret = 0;
> > +
> > +    assert(old_lock != new_lock);
> > +    assert(old_lock == IMAGE_LOCK_MODE_NOLOCK);
> > +    switch (op) {
> > +    case RAW_REOPEN_PREPARE:
> > +        ret = raw_lock_fd(raw_s->lock_fd, new_lock);
> > +        if (ret) {
> > +            error_setg_errno(errp, -ret, "Failed to lock new fd %d", raw_s->lock_fd);
> > +        }
> > +        break;
> > +    case RAW_REOPEN_COMMIT:
> > +    case RAW_REOPEN_ABORT:
> > +        break;
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static int
> > +raw_reopen_to_unlock(BDRVReopenState *state,
> > +                     RawReopenOperation op,
> > +                     ImageLockMode old_lock,
> > +                     ImageLockMode new_lock,
> > +                     Error **errp)
> > +{
> > +    BDRVRawState *s = state->bs->opaque;
> > +    int ret = 0;
> > +
> > +    assert(old_lock != new_lock);
> > +    assert(new_lock == IMAGE_LOCK_MODE_NOLOCK);
> > +    switch (op) {
> > +    case RAW_REOPEN_PREPARE:
> > +        break;
> > +    case RAW_REOPEN_COMMIT:
> > +        if (s->lock_fd >= 0) {
> > +            qemu_close(s->lock_fd);
> > +            s->lock_fd = -1;
> > +        }
> > +        break;
> > +    case RAW_REOPEN_ABORT:
> > +        break;
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static int
> > +raw_reopen_upgrade(BDRVReopenState *state,
> > +                   RawReopenOperation op,
> > +                   ImageLockMode old_lock,
> > +                   ImageLockMode new_lock,
> > +                   Error **errp)
> > +{
> > +    BDRVRawReopenState *raw_s = state->opaque;
> > +    BDRVRawState *s = state->bs->opaque;
> > +    int ret = 0, ret2;
> > +
> > +    assert(old_lock == IMAGE_LOCK_MODE_SHARED);
> > +    assert(new_lock == IMAGE_LOCK_MODE_EXCLUSIVE);
> > +    switch (op) {
> > +    case RAW_REOPEN_PREPARE:
> > +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);

[1]

> > +        if (ret) {
> > +            error_setg_errno(errp, -ret, "Failed to lock new fd (shared)");
> > +            break;
> > +        }
> > +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_NOLOCK);

[2]

> > +        if (ret) {
> > +            error_setg_errno(errp, -ret, "Failed to unlock old fd");
> > +            goto restore;
> > +        }
> > +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_EXCLUSIVE);

[3]

> > +        if (ret) {
> > +            error_setg_errno(errp, -ret, "Failed to lock new fd (exclusive)");
> > +            goto restore;
> > +        }
> > +        break;
> > +    case RAW_REOPEN_COMMIT:
> > +        break;
> > +    case RAW_REOPEN_ABORT:
> > +        raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> > +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);

[4]

> > +        if (ret) {
> > +            error_report("Failed to restore lock on old fd");
> 
> If we get here, s->lock_fd is still locked exclusively. The following is
> a very personal opinion, but anyway: I think it would be be better for
> it to be unlocked. If it's locked too strictly, this can really break
> something; but if it's just not locked (while it should be locked in
> shared mode), everything's going to be fine unless the user makes a
> mistake. I think the latter is less bad.

There are four lock states when we land on this "abort" branch:

  a) Lock "prepare" was not run, some other error happened earlier, so the lock
     aren't changed compared to before the transaction starts: raw_s->lock_fd is
     unlocked, s->lock_fd is shared locked. In this case raw_lock_fd [4] cannot
     fail, and even if it does, s->lock_fd is in the correct state.

  b) The raw_lock_fd [1] failed. This is similar to 1), s->lock_fd is intact, so
     we are good.

  c) The raw_lock_fd [2] failed. Again, similar to above.

  d) The raw_lock_fd [3] failed. Here raw_s->lock_fd is shared locked, and
     s->lock_fd is unlocked. The correct "abort" sequence is downgrade
     raw_s->lock_fd and "shared lock" s->lock_fd again. If the "abort" sequence
     failed, s->lock_fd is unlocked.

> 
> And we can always achieve unlocking the FD by closing s->lock_fd, so I
> think that is what we should do here.

So I think this already does what you want.

> 
> > +        }
> > +        break;
> > +    }
> > +
> > +    return ret;
> > +restore:
> > +    ret2 = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> > +    if (ret2) {
> > +        error_report("Failed to restore old lock");
> > +    }
> > +    return ret;
> > +}
> > +
> > +static int
> > +raw_reopen_downgrade(BDRVReopenState *state,
> > +                     RawReopenOperation op,
> > +                     ImageLockMode old_lock,
> > +                     ImageLockMode new_lock,
> > +                     Error **errp)
> > +{
> > +    BDRVRawReopenState *raw_s = state->opaque;
> > +    BDRVRawState *s = state->bs->opaque;
> > +    int ret = 0;
> > +
> > +    assert(old_lock == IMAGE_LOCK_MODE_EXCLUSIVE);
> > +    assert(new_lock == IMAGE_LOCK_MODE_SHARED);
> > +    switch (op) {
> > +    case RAW_REOPEN_PREPARE:
> > +        break;
> > +    case RAW_REOPEN_COMMIT:
> > +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> > +        if (ret) {
> > +            error_report("Failed to downgrade old lock");
> > +            break;
> > +        }
> > +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> > +        if (ret) {
> > +            error_report("Failed to lock new fd (shared)");
> > +            break;
> > +        }
> > +        break;
> 
> It's really unfortunate that we cannot do anything in prepare and have
> to hope for the best in commit. But I can't come up with anything better.
> 
> And if something does fail, the FD will be unlocked after reopening.
> That's good enough for me.
> 
> > +    case RAW_REOPEN_ABORT:
> > +        break;
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +/**
> > + * Transactionally moving between three possible locking states is tricky and
> > + * must be done carefully. That is mostly because downgrading an exclusive lock
> > + * to shared or unlocked is not guaranteed to be revertable. As a result, in
> > + * such cases we have to defer the downgraing to "commit", given that no revert
> > + * will happen after that point, and that downgrading a lock should never fail.
> > + *
> > + * On the other hand, upgrading a lock (e.g. from unlocked or shared to
> > + * exclusive lock) must happen in "prepare" because it may fail.
> > + *
> > + * Manage the operation matrix with this state transition table to make
> > + * fulfulling above conditions easier.
> > + */
> > +static const struct RawReopenFuncRecord {
> > +    ImageLockMode old_lock;
> > +    ImageLockMode new_lock;
> > +    RawReopenFunc func;
> > +    bool need_lock_fd;
> > +} reopen_functions[] = {
> > +    {IMAGE_LOCK_MODE_NOLOCK, IMAGE_LOCK_MODE_NOLOCK, raw_reopen_identical, false},
> > +    {IMAGE_LOCK_MODE_NOLOCK, IMAGE_LOCK_MODE_SHARED, raw_reopen_from_unlock, true},
> > +    {IMAGE_LOCK_MODE_NOLOCK, IMAGE_LOCK_MODE_EXCLUSIVE, raw_reopen_from_unlock, true},
> > +    {IMAGE_LOCK_MODE_SHARED, IMAGE_LOCK_MODE_NOLOCK, raw_reopen_to_unlock, false},
> > +    {IMAGE_LOCK_MODE_SHARED, IMAGE_LOCK_MODE_SHARED, raw_reopen_identical, false},
> > +    {IMAGE_LOCK_MODE_SHARED, IMAGE_LOCK_MODE_EXCLUSIVE, raw_reopen_upgrade, true},
> > +    {IMAGE_LOCK_MODE_EXCLUSIVE, IMAGE_LOCK_MODE_NOLOCK, raw_reopen_to_unlock, false},
> > +    {IMAGE_LOCK_MODE_EXCLUSIVE, IMAGE_LOCK_MODE_SHARED, raw_reopen_downgrade, true},
> > +    {IMAGE_LOCK_MODE_EXCLUSIVE, IMAGE_LOCK_MODE_EXCLUSIVE, raw_reopen_identical, false},
> > +};
> > +
> > +static int raw_reopen_handle_lock(BDRVReopenState *state,
> > +                                  RawReopenOperation op,
> > +                                  Error **errp)
> > +{
> > +    BDRVRawReopenState *raw_s = state->opaque;
> 
> Please choose another name, it's hard not to confuse this with the
> BDRVRawState all the time. (e.g. raw_rs or just rs would be enough.)

Sorry I can't change the name in this patch, it will cause more inconsistency
because raw_reopen_* already use the name broadly. If you really insist it's a
bad name, I can append or prepend a patch in the next version.

> 
> (Same in the places above, I got confused more than once...)
> 
> > +    BDRVRawState *s = state->bs->opaque;
> > +    ImageLockMode old_lock, new_lock;
> > +    const struct RawReopenFuncRecord *rec;
> > +    int ret;
> > +
> > +    old_lock = bdrv_get_lock_mode(state->bs);
> > +    new_lock = bdrv_lock_mode_from_flags(state->flags);
> > +
> > +    if (old_lock == IMAGE_LOCK_MODE__MAX) {
> > +        /* bs was not locked, leave it unlocked. */
> > +        old_lock = new_lock = IMAGE_LOCK_MODE_NOLOCK;
> > +    }
> > +
> > +    if (old_lock == IMAGE_LOCK_MODE_AUTO) {
> > +        old_lock = bdrv_get_flags(state->bs) & BDRV_O_RDWR ?
> > +                   IMAGE_LOCK_MODE_EXCLUSIVE : IMAGE_LOCK_MODE_SHARED;
> > +    }
> > +
> > +    if (new_lock == IMAGE_LOCK_MODE_AUTO) {
> > +        new_lock = state->flags & BDRV_O_RDWR ?
> > +                   IMAGE_LOCK_MODE_EXCLUSIVE : IMAGE_LOCK_MODE_SHARED;
> > +    }
> > +
> > +    for (rec = &reopen_functions[0];
> > +         rec < &reopen_functions[ARRAY_SIZE(reopen_functions)];
> > +         rec++) {
> > +        if (rec->old_lock == old_lock && rec->new_lock == new_lock) {
> > +            break;
> > +        }
> > +    }
> > +    assert(rec != &reopen_functions[ARRAY_SIZE(reopen_functions)]);
> > +
> > +    switch (op) {
> > +    case RAW_REOPEN_PREPARE:
> > +        if (rec->need_lock_fd) {
> > +            ret = qemu_dup(raw_s->fd);
> > +            if (ret < 0) {
> > +                error_setg_errno(errp, -ret, "Failed to dup new fd");
> > +                return ret;
> > +            }
> > +            raw_s->lock_fd = ret;
> > +        }
> > +        return rec->func(state, op, old_lock, new_lock, errp);
> > +    case RAW_REOPEN_COMMIT:
> > +        rec->func(state, op, old_lock, new_lock, errp);
> > +        if (rec->need_lock_fd) {
> > +            if (s->lock_fd >= 0) {
> > +                qemu_close(s->lock_fd);
> > +            }
> > +            s->lock_fd = raw_s->lock_fd;
> > +        }
> > +        break;
> > +    case RAW_REOPEN_ABORT:
> > +        rec->func(state, op, old_lock, new_lock, errp);
> > +        if (rec->need_lock_fd && raw_s->lock_fd >= 0) {
> > +            qemu_close(raw_s->lock_fd);
> > +            raw_s->lock_fd = -1;
> > +        }
> > +        break;
> > +    }
> > +    return 0;
> > +}
> > +
> >  static int raw_reopen_prepare(BDRVReopenState *state,
> >                                BlockReopenQueue *queue, Error **errp)
> >  {
> > @@ -607,6 +909,10 @@ static int raw_reopen_prepare(BDRVReopenState *state,
> >          }
> >      }
> >  
> > +    if (!ret) {
> > +        ret = raw_reopen_handle_lock(state, RAW_REOPEN_PREPARE, errp);
> > +    }
> > +
> >      return ret;
> >  }
> >  
> > @@ -617,6 +923,8 @@ static void raw_reopen_commit(BDRVReopenState *state)
> >  
> >      s->open_flags = raw_s->open_flags;
> >  
> > +    raw_reopen_handle_lock(state, RAW_REOPEN_COMMIT, NULL);
> 
> I'd prefer &error_abort instead of NULL.

OK.

> 
> > +
> >      qemu_close(s->fd);
> >      s->fd = raw_s->fd;
> >  
> > @@ -634,6 +942,8 @@ static void raw_reopen_abort(BDRVReopenState *state)
> >          return;
> >      }
> >  
> > +    raw_reopen_handle_lock(state, RAW_REOPEN_ABORT, NULL);
> 
> Same here.

OK.

> 
> > +
> >      if (raw_s->fd >= 0) {
> >          qemu_close(raw_s->fd);
> >          raw_s->fd = -1;
> > @@ -1321,6 +1631,10 @@ static void raw_close(BlockDriverState *bs)
> >          qemu_close(s->fd);
> >          s->fd = -1;
> >      }
> > +    if (s->lock_fd >= 0) {
> > +        qemu_close(s->lock_fd);
> > +        s->lock_fd = -1;
> > +    }
> >  }
> >  
> >  static int raw_truncate(BlockDriverState *bs, int64_t offset)
> > @@ -1874,7 +2188,7 @@ BlockDriver bdrv_file = {
> >      .bdrv_get_info = raw_get_info,
> >      .bdrv_get_allocated_file_size
> >                          = raw_get_allocated_file_size,
> > -
> > +    .bdrv_lockf = raw_lockf,
> >      .create_opts = &raw_create_opts,
> >  };
> >  
> > @@ -2324,6 +2638,8 @@ static BlockDriver bdrv_host_device = {
> >  #ifdef __linux__
> >      .bdrv_aio_ioctl     = hdev_aio_ioctl,
> >  #endif
> > +
> > +    .bdrv_lockf = raw_lockf,
> >  };
> >  
> >  #if defined(__linux__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
> > 
> 
> Overall design looks good to me. Unfortunately we can't strictly follow
> the transaction principle for downgrades and upgrades, but I guess it'll
> work most of the time (fingers crossed).

Thanks for not yelling at this scary patch.

Fam

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Fam Zheng]

On Mon, 10/24 12:11, Kevin Wolf wrote:
> Am 22.10.2016 um 03:00 hat Max Reitz geschrieben:
> > <parenthesis>
> > 
> > I personally still don't like making locking a qdev property very much
> > because it doesn't make sense to me*. But I remember Kevin had his
> > reasons (even though I can no longer remember them) for asking for it,
> > and I don't have any besides "It doesn't make sense to me". After having
> > though a bit about it (= having written three paragraphs and deleted
> > them again), I guess I can make some sense of it, though it seems to be
> > a rather esoteric use case still; it appears to me that a guest could
> > use it to signal that it's fine with some block device being shared;
> > then we could use a shared lock or none at all or I don't know.
> > Otherwise, we should get an exclusive lock for write access and a shared
> > lock for read access.
> 
> The reason is pretty simple if you think about this question: Why do we
> need user input in the first place? 

I think the reason why we have an option at all is rather because of the special
case of libguestfs [1], otherwise locks should just be acquired sensibly as the
"auto" mode does.

Other than that, I don't think we have a concrete use case of non-auto lock
mode.  Do we?

[1]: https://lists.nongnu.org/archive/html/qemu-block/2016-04/msg00414.html

Fam

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Richard W.M. Jones]

On Tue, Oct 25, 2016 at 03:09:51PM +0800, Fam Zheng wrote:
> On Mon, 10/24 12:11, Kevin Wolf wrote:
> > Am 22.10.2016 um 03:00 hat Max Reitz geschrieben:
> > > <parenthesis>
> > > 
> > > I personally still don't like making locking a qdev property very much
> > > because it doesn't make sense to me*. But I remember Kevin had his
> > > reasons (even though I can no longer remember them) for asking for it,
> > > and I don't have any besides "It doesn't make sense to me". After having
> > > though a bit about it (= having written three paragraphs and deleted
> > > them again), I guess I can make some sense of it, though it seems to be
> > > a rather esoteric use case still; it appears to me that a guest could
> > > use it to signal that it's fine with some block device being shared;
> > > then we could use a shared lock or none at all or I don't know.
> > > Otherwise, we should get an exclusive lock for write access and a shared
> > > lock for read access.
> > 
> > The reason is pretty simple if you think about this question: Why do we
> > need user input in the first place? 
> 
> I think the reason why we have an option at all is rather because of the special
> case of libguestfs [1], otherwise locks should just be acquired sensibly as the
> "auto" mode does.

It's not just that.

Qemu doesn't have enough information to make the correct decision
about locking automatically -- for example, Qemu doesn't know if the
guest is using cluster filesystems or not (or for some other reason
the guest wants a shared writable block device, eg. for running Disk
Paxos).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1955 bytes --]

Am 24.10.2016 um 20:03 hat Max Reitz geschrieben:
> On 24.10.2016 12:11, Kevin Wolf wrote:
> 
> [...]
> 
> > Now, the big question is how to translate this into file locking. This
> > could become a little tricky. I had a few thoughts involving another
> > lock on byte 2, but none of them actually worked out so far, because
> > what we want is essentially a lock that can be shared by readers, that
> > can also be shared by writers, but not by readers and writers at the
> > same time.
> 
> You can also share it between readers and writers, as long as everyone
> can cope with volatile data.

Sorry, that was ambiguous. I meant a file-level lock rather than the
high-level one. If we had a lock that can be shared by one or the other,
but not both, then two locks would be enough to build what we really
want.

> I agree that it's very similar to the proposed op blocker style, but I
> can't really come up with a meaningful translation either.
> 
> Maybe something like this (?): All readers who do not want the file to
> be modified grab a shared lock on byte 1. All writers who can deal with
> volatile data grab a shared lock on byte 2. Exclusive writers grab an
> exclusive lock on byte 1 and 2. Readers who can cope with volatile data
> get no lock at all.
> 
> When opening, the first and second group would always have to test
> whether there is a lock on the other byte, respectively. E.g. sharing
> writers would first grab an exclusive lock on byte 1, then the shared
> lock on byte 2 and then release the exclusive lock again.
> 
> Would that work?

I'm afraid it wouldn't. If you start the sharing writer first and then
the writer-blocking reader, the writer doesn't hold a lock on byte 1 any
more, so the reader can start even though someone is writing to the
image. On the other hand, the writer can't keep an exclusive lock
because it would block other users that can share the image.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Fam Zheng]

On Tue, 10/25 09:06, Richard W.M. Jones wrote:
> On Tue, Oct 25, 2016 at 03:09:51PM +0800, Fam Zheng wrote:
> > On Mon, 10/24 12:11, Kevin Wolf wrote:
> > > Am 22.10.2016 um 03:00 hat Max Reitz geschrieben:
> > > > <parenthesis>
> > > > 
> > > > I personally still don't like making locking a qdev property very much
> > > > because it doesn't make sense to me*. But I remember Kevin had his
> > > > reasons (even though I can no longer remember them) for asking for it,
> > > > and I don't have any besides "It doesn't make sense to me". After having
> > > > though a bit about it (= having written three paragraphs and deleted
> > > > them again), I guess I can make some sense of it, though it seems to be
> > > > a rather esoteric use case still; it appears to me that a guest could
> > > > use it to signal that it's fine with some block device being shared;
> > > > then we could use a shared lock or none at all or I don't know.
> > > > Otherwise, we should get an exclusive lock for write access and a shared
> > > > lock for read access.
> > > 
> > > The reason is pretty simple if you think about this question: Why do we
> > > need user input in the first place? 
> > 
> > I think the reason why we have an option at all is rather because of the special
> > case of libguestfs [1], otherwise locks should just be acquired sensibly as the
> > "auto" mode does.
> 
> It's not just that.
> 
> Qemu doesn't have enough information to make the correct decision
> about locking automatically -- for example, Qemu doesn't know if the
> guest is using cluster filesystems or not (or for some other reason
> the guest wants a shared writable block device, eg. for running Disk
> Paxos).

Yeah, okay, but I agree with Max that this doesn't make any sense on many
emulated device types that inherently aren't sharable in real world (like IDE).
It feels we are trying to solve two problems at the same time which makes a
questionable (qdev) interface.

Fam

Re: [Qemu-devel] [PATCH v8 02/36] qapi: Add ImageLockMode

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]

On 25.10.2016 07:36, Fam Zheng wrote:
> On Fri, 10/21 22:45, Max Reitz wrote:
>> On 30.09.2016 14:09, Fam Zheng wrote:
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>  qapi/block-core.json | 18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 92193ab..22e8d04 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -2754,3 +2754,21 @@
>>>    'data' : { 'parent': 'str',
>>>               '*child': 'str',
>>>               '*node': 'str' } }
>>> +
>>> +##
>>> +# @ImageLockMode:
>>> +#
>>> +# @auto: defer to the block driver to use the least strict mode, based on
>>> +#        the nature of format and read-only flag, and the supported locking
>>> +#        operations of the protocol.
>>
>> I have some difficulty understanding this description. I'd intuitively
>> assume no locking to be the "least strict mode"; however, since it
>> should be always possible not to lock an image, this would mean that
>> auto=nolock. Which is hopefully isn't.
>>
>> If it's not easy to come up with a thorough explanation, perhaps it
>> would be best to give some examples which help to understand the concept
>> behind "auto" intuitively.
> 
> It could have beeen more specific, it's my bad being too terse here. Maybe
> something like this:
> 
>     @auto: defer to the block layer to use an appropriate lock mode, based on
>            the driver used and read-only option: for read-only images, shared
>            lock mode, or otherwise exclusive lock mode, will be attempted; if
>            the driver doesn't support this mode (or sharing is particularly
>            desired by its design), nolock will be used.
> 
> ?

Sounds good to me, if that's how it's supposed to be.

Do we actually want to use nolock "if sharing is particularly desired by
its design"? I mean, I think one of the drivers that would apply to is
NBD, but is the fact that multiple parties can freely access (and write
to!) an NBD disk at the same time really what we want or just a design
limitation?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 03/36] block: Introduce image file locking

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]

On 25.10.2016 07:48, Fam Zheng wrote:
> On Fri, 10/21 23:04, Max Reitz wrote:

[...]

>>> +int bdrv_set_lock_mode(BlockDriverState *bs, ImageLockMode mode)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (bs->cur_lock == mode) {
>>> +        return 0;
>>> +    } else if (!bs->drv) {
>>> +        return -ENOMEDIUM;
>>> +    } else if (!bs->drv->bdrv_lockf) {
>>> +        if (bs->file) {
>>> +            return bdrv_set_lock_mode(bs->file->bs, mode);
>>> +        }
>>> +        return 0;
>>> +    }
>>> +    ret = bs->drv->bdrv_lockf(bs, mode);
>>> +    if (ret == -ENOTSUP) {
>>> +        /* Handle it the same way as !bs->drv->bdrv_lockf */
>>> +        ret = 0;
>>
>> Yes, well, why do you handle both as success? Wouldn't returning
>> -ENOTSUP make more sense?
>>
>> I guess the caller can find out itself by checking whether bs->cur_lock
>> has changed, but...
> 
> I can't think of a reason for any caller to do something different for -ENOTSUP
> from success, hence the check here.

OK, that's fine, then.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 05/36] raw-posix: Add image locking support

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4100 bytes --]

On 25.10.2016 08:31, Fam Zheng wrote:
> On Sat, 10/22 01:40, Max Reitz wrote:
>> On 30.09.2016 14:09, Fam Zheng wrote:

[...]

>>> +static int
>>> +raw_reopen_upgrade(BDRVReopenState *state,
>>> +                   RawReopenOperation op,
>>> +                   ImageLockMode old_lock,
>>> +                   ImageLockMode new_lock,
>>> +                   Error **errp)
>>> +{
>>> +    BDRVRawReopenState *raw_s = state->opaque;
>>> +    BDRVRawState *s = state->bs->opaque;
>>> +    int ret = 0, ret2;
>>> +
>>> +    assert(old_lock == IMAGE_LOCK_MODE_SHARED);
>>> +    assert(new_lock == IMAGE_LOCK_MODE_EXCLUSIVE);
>>> +    switch (op) {
>>> +    case RAW_REOPEN_PREPARE:
>>> +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> 
> [1]
> 
>>> +        if (ret) {
>>> +            error_setg_errno(errp, -ret, "Failed to lock new fd (shared)");
>>> +            break;
>>> +        }
>>> +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_NOLOCK);
> 
> [2]
> 
>>> +        if (ret) {
>>> +            error_setg_errno(errp, -ret, "Failed to unlock old fd");
>>> +            goto restore;
>>> +        }
>>> +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_EXCLUSIVE);
> 
> [3]
> 
>>> +        if (ret) {
>>> +            error_setg_errno(errp, -ret, "Failed to lock new fd (exclusive)");
>>> +            goto restore;
>>> +        }
>>> +        break;
>>> +    case RAW_REOPEN_COMMIT:
>>> +        break;
>>> +    case RAW_REOPEN_ABORT:
>>> +        raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
>>> +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> 
> [4]
> 
>>> +        if (ret) {
>>> +            error_report("Failed to restore lock on old fd");
>>
>> If we get here, s->lock_fd is still locked exclusively. The following is
>> a very personal opinion, but anyway: I think it would be be better for
>> it to be unlocked. If it's locked too strictly, this can really break
>> something; but if it's just not locked (while it should be locked in
>> shared mode), everything's going to be fine unless the user makes a
>> mistake. I think the latter is less bad.
> 
> There are four lock states when we land on this "abort" branch:
> 
>   a) Lock "prepare" was not run, some other error happened earlier, so the lock
>      aren't changed compared to before the transaction starts: raw_s->lock_fd is
>      unlocked, s->lock_fd is shared locked. In this case raw_lock_fd [4] cannot
>      fail, and even if it does, s->lock_fd is in the correct state.
> 
>   b) The raw_lock_fd [1] failed. This is similar to 1), s->lock_fd is intact, so
>      we are good.
> 
>   c) The raw_lock_fd [2] failed. Again, similar to above.
> 
>   d) The raw_lock_fd [3] failed. Here raw_s->lock_fd is shared locked, and
>      s->lock_fd is unlocked. The correct "abort" sequence is downgrade
>      raw_s->lock_fd and "shared lock" s->lock_fd again. If the "abort" sequence
>      failed, s->lock_fd is unlocked.

OK, you're right, I somehow forgot about the cases where our prepare
sequence was either not run at all or failed. But I was thinking about
the case where our preparation was successful, but some later
preparation in the reopen transaction failed. Then, s->lock_fd should be
locked exclusively, no?

[...]

>>> +static int raw_reopen_handle_lock(BDRVReopenState *state,
>>> +                                  RawReopenOperation op,
>>> +                                  Error **errp)
>>> +{
>>> +    BDRVRawReopenState *raw_s = state->opaque;
>>
>> Please choose another name, it's hard not to confuse this with the
>> BDRVRawState all the time. (e.g. raw_rs or just rs would be enough.)
> 
> Sorry I can't change the name in this patch, it will cause more inconsistency
> because raw_reopen_* already use the name broadly. If you really insist it's a
> bad name, I can append or prepend a patch in the next version.

Well, I don't insist, but it really confused me a couple of times, even
after I had realized my mistake once.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2226 bytes --]

On 25.10.2016 10:24, Kevin Wolf wrote:
> Am 24.10.2016 um 20:03 hat Max Reitz geschrieben:
>> On 24.10.2016 12:11, Kevin Wolf wrote:
>>
>> [...]
>>
>>> Now, the big question is how to translate this into file locking. This
>>> could become a little tricky. I had a few thoughts involving another
>>> lock on byte 2, but none of them actually worked out so far, because
>>> what we want is essentially a lock that can be shared by readers, that
>>> can also be shared by writers, but not by readers and writers at the
>>> same time.
>>
>> You can also share it between readers and writers, as long as everyone
>> can cope with volatile data.
> 
> Sorry, that was ambiguous. I meant a file-level lock rather than the
> high-level one. If we had a lock that can be shared by one or the other,
> but not both, then two locks would be enough to build what we really
> want.
> 
>> I agree that it's very similar to the proposed op blocker style, but I
>> can't really come up with a meaningful translation either.
>>
>> Maybe something like this (?): All readers who do not want the file to
>> be modified grab a shared lock on byte 1. All writers who can deal with
>> volatile data grab a shared lock on byte 2. Exclusive writers grab an
>> exclusive lock on byte 1 and 2. Readers who can cope with volatile data
>> get no lock at all.
>>
>> When opening, the first and second group would always have to test
>> whether there is a lock on the other byte, respectively. E.g. sharing
>> writers would first grab an exclusive lock on byte 1, then the shared
>> lock on byte 2 and then release the exclusive lock again.
>>
>> Would that work?
> 
> I'm afraid it wouldn't. If you start the sharing writer first and then
> the writer-blocking reader, the writer doesn't hold a lock on byte 1 any
> more,

But it holds a lock on byte 2.

>       so the reader can start even though someone is writing to the
> image.

It can't because it would try to grab an exclusive lock on byte 2 before
grabbing the shared lock on byte 1.

Max

>        On the other hand, the writer can't keep an exclusive lock
> because it would block other users that can share the image.
> 
> Kevin
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 02/36] qapi: Add ImageLockMode

[Fam Zheng]

On Tue, 10/25 15:20, Max Reitz wrote:
> On 25.10.2016 07:36, Fam Zheng wrote:
> > On Fri, 10/21 22:45, Max Reitz wrote:
> >> On 30.09.2016 14:09, Fam Zheng wrote:
> >>> Signed-off-by: Fam Zheng <famz@redhat.com>
> >>> ---
> >>>  qapi/block-core.json | 18 ++++++++++++++++++
> >>>  1 file changed, 18 insertions(+)
> >>>
> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >>> index 92193ab..22e8d04 100644
> >>> --- a/qapi/block-core.json
> >>> +++ b/qapi/block-core.json
> >>> @@ -2754,3 +2754,21 @@
> >>>    'data' : { 'parent': 'str',
> >>>               '*child': 'str',
> >>>               '*node': 'str' } }
> >>> +
> >>> +##
> >>> +# @ImageLockMode:
> >>> +#
> >>> +# @auto: defer to the block driver to use the least strict mode, based on
> >>> +#        the nature of format and read-only flag, and the supported locking
> >>> +#        operations of the protocol.
> >>
> >> I have some difficulty understanding this description. I'd intuitively
> >> assume no locking to be the "least strict mode"; however, since it
> >> should be always possible not to lock an image, this would mean that
> >> auto=nolock. Which is hopefully isn't.
> >>
> >> If it's not easy to come up with a thorough explanation, perhaps it
> >> would be best to give some examples which help to understand the concept
> >> behind "auto" intuitively.
> > 
> > It could have beeen more specific, it's my bad being too terse here. Maybe
> > something like this:
> > 
> >     @auto: defer to the block layer to use an appropriate lock mode, based on
> >            the driver used and read-only option: for read-only images, shared
> >            lock mode, or otherwise exclusive lock mode, will be attempted; if
> >            the driver doesn't support this mode (or sharing is particularly
> >            desired by its design), nolock will be used.
> > 
> > ?
> 
> Sounds good to me, if that's how it's supposed to be.
> 
> Do we actually want to use nolock "if sharing is particularly desired by
> its design"? I mean, I think one of the drivers that would apply to is
> NBD, but is the fact that multiple parties can freely access (and write
> to!) an NBD disk at the same time really what we want or just a design
> limitation?

The guest can always corrupt data themselves so I think my wording is inaccurate
here. Actually the more reasonable case of this is for example when server side
locking is applied automatically and transparently:

https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg03576.html

(I don't know how the mentioned feature above ended up at RBD side, but that
makes an interesting consideration point).

Fam

Re: [Qemu-devel] [PATCH v8 05/36] raw-posix: Add image locking support

[Fam Zheng]

On Tue, 10/25 15:28, Max Reitz wrote:
> On 25.10.2016 08:31, Fam Zheng wrote:
> > On Sat, 10/22 01:40, Max Reitz wrote:
> >> On 30.09.2016 14:09, Fam Zheng wrote:
> 
> [...]
> 
> >>> +static int
> >>> +raw_reopen_upgrade(BDRVReopenState *state,
> >>> +                   RawReopenOperation op,
> >>> +                   ImageLockMode old_lock,
> >>> +                   ImageLockMode new_lock,
> >>> +                   Error **errp)
> >>> +{
> >>> +    BDRVRawReopenState *raw_s = state->opaque;
> >>> +    BDRVRawState *s = state->bs->opaque;
> >>> +    int ret = 0, ret2;
> >>> +
> >>> +    assert(old_lock == IMAGE_LOCK_MODE_SHARED);
> >>> +    assert(new_lock == IMAGE_LOCK_MODE_EXCLUSIVE);
> >>> +    switch (op) {
> >>> +    case RAW_REOPEN_PREPARE:
> >>> +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> > 
> > [1]
> > 
> >>> +        if (ret) {
> >>> +            error_setg_errno(errp, -ret, "Failed to lock new fd (shared)");
> >>> +            break;
> >>> +        }
> >>> +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_NOLOCK);
> > 
> > [2]
> > 
> >>> +        if (ret) {
> >>> +            error_setg_errno(errp, -ret, "Failed to unlock old fd");
> >>> +            goto restore;
> >>> +        }
> >>> +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_EXCLUSIVE);
> > 
> > [3]
> > 
> >>> +        if (ret) {
> >>> +            error_setg_errno(errp, -ret, "Failed to lock new fd (exclusive)");
> >>> +            goto restore;
> >>> +        }
> >>> +        break;
> >>> +    case RAW_REOPEN_COMMIT:
> >>> +        break;
> >>> +    case RAW_REOPEN_ABORT:
> >>> +        raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> >>> +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);
> > 
> > [4]
> > 
> >>> +        if (ret) {
> >>> +            error_report("Failed to restore lock on old fd");
> >>
> >> If we get here, s->lock_fd is still locked exclusively. The following is
> >> a very personal opinion, but anyway: I think it would be be better for
> >> it to be unlocked. If it's locked too strictly, this can really break
> >> something; but if it's just not locked (while it should be locked in
> >> shared mode), everything's going to be fine unless the user makes a
> >> mistake. I think the latter is less bad.
> > 
> > There are four lock states when we land on this "abort" branch:
> > 
> >   a) Lock "prepare" was not run, some other error happened earlier, so the lock
> >      aren't changed compared to before the transaction starts: raw_s->lock_fd is
> >      unlocked, s->lock_fd is shared locked. In this case raw_lock_fd [4] cannot
> >      fail, and even if it does, s->lock_fd is in the correct state.
> > 
> >   b) The raw_lock_fd [1] failed. This is similar to 1), s->lock_fd is intact, so
> >      we are good.
> > 
> >   c) The raw_lock_fd [2] failed. Again, similar to above.
> > 
> >   d) The raw_lock_fd [3] failed. Here raw_s->lock_fd is shared locked, and
> >      s->lock_fd is unlocked. The correct "abort" sequence is downgrade
> >      raw_s->lock_fd and "shared lock" s->lock_fd again. If the "abort" sequence
> >      failed, s->lock_fd is unlocked.
> 
> OK, you're right, I somehow forgot about the cases where our prepare
> sequence was either not run at all or failed. But I was thinking about
> the case where our preparation was successful, but some later
> preparation in the reopen transaction failed. Then, s->lock_fd should be
> locked exclusively, no?

Oh I missed to reason about that branch. Here we go:

If our preparation succeeded, raw_s->lock_fd is exclusively locked, s->lock_fd
is unlocked. In abort we should try to return to the old state (raw_s->lock_fd
is _not_ exclusively locked and s->lock_fd is shared locked), hence the two
raw_lock_fd() calls at [4]. In this case, if the second raw_lock_fd() at [4]
doesn't work, s->lock_fd is unlocked, and raw_s->lock_fd will be closed anyway,
which again matches what you suggested.

Fam

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 3234 bytes --]

Am 25.10.2016 um 15:30 hat Max Reitz geschrieben:
> On 25.10.2016 10:24, Kevin Wolf wrote:
> > Am 24.10.2016 um 20:03 hat Max Reitz geschrieben:
> >> On 24.10.2016 12:11, Kevin Wolf wrote:
> >>
> >> [...]
> >>
> >>> Now, the big question is how to translate this into file locking. This
> >>> could become a little tricky. I had a few thoughts involving another
> >>> lock on byte 2, but none of them actually worked out so far, because
> >>> what we want is essentially a lock that can be shared by readers, that
> >>> can also be shared by writers, but not by readers and writers at the
> >>> same time.
> >>
> >> You can also share it between readers and writers, as long as everyone
> >> can cope with volatile data.
> > 
> > Sorry, that was ambiguous. I meant a file-level lock rather than the
> > high-level one. If we had a lock that can be shared by one or the other,
> > but not both, then two locks would be enough to build what we really
> > want.
> > 
> >> I agree that it's very similar to the proposed op blocker style, but I
> >> can't really come up with a meaningful translation either.
> >>
> >> Maybe something like this (?): All readers who do not want the file to
> >> be modified grab a shared lock on byte 1. All writers who can deal with
> >> volatile data grab a shared lock on byte 2. Exclusive writers grab an
> >> exclusive lock on byte 1 and 2. Readers who can cope with volatile data
> >> get no lock at all.
> >>
> >> When opening, the first and second group would always have to test
> >> whether there is a lock on the other byte, respectively. E.g. sharing
> >> writers would first grab an exclusive lock on byte 1, then the shared
> >> lock on byte 2 and then release the exclusive lock again.
> >>
> >> Would that work?
> > 
> > I'm afraid it wouldn't. If you start the sharing writer first and then
> > the writer-blocking reader, the writer doesn't hold a lock on byte 1 any
> > more,
> 
> But it holds a lock on byte 2.
> 
> >       so the reader can start even though someone is writing to the
> > image.
> 
> It can't because it would try to grab an exclusive lock on byte 2 before
> grabbing the shared lock on byte 1.

Apparently I failed to understand the most important part of the
proposal. :-)

So we have two locks. Both are only held for a longer time in shared
mode. Exclusive mode is only used for testing whether the lock is being
held and is immediately given up again.

The meaning of holding a shared lock is:

    byte 1: I can't allow other processes to write to the image
    byte 2: I am writing to the image

The four cases that we have involve:

* shared writer: Take shared lock on byte 2. Test whether byte 1 is
  locked using an exclusive lock, and fail if so.

* exclusive writer: Take shared lock on byte 2. Test whether byte 1 is
  locked using an exclusive lock, and fail if so. Then take shared lock
  on byte 1. I suppose this is racy, but we can probably tolerate that.

* reader that can tolerate writers: Don't do anything

* reader that can't tolerate writers: Take shared lock on byte 1. Test
  whether byte 2 is locked, and fail if so.

Seems to work if I got that right.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Fam Zheng]

On Tue, 10/25 16:57, Kevin Wolf wrote:
> Am 25.10.2016 um 15:30 hat Max Reitz geschrieben:
> > On 25.10.2016 10:24, Kevin Wolf wrote:
> > > Am 24.10.2016 um 20:03 hat Max Reitz geschrieben:
> > >> On 24.10.2016 12:11, Kevin Wolf wrote:
> > >>
> > >> [...]
> > >>
> > >>> Now, the big question is how to translate this into file locking. This
> > >>> could become a little tricky. I had a few thoughts involving another
> > >>> lock on byte 2, but none of them actually worked out so far, because
> > >>> what we want is essentially a lock that can be shared by readers, that
> > >>> can also be shared by writers, but not by readers and writers at the
> > >>> same time.
> > >>
> > >> You can also share it between readers and writers, as long as everyone
> > >> can cope with volatile data.
> > > 
> > > Sorry, that was ambiguous. I meant a file-level lock rather than the
> > > high-level one. If we had a lock that can be shared by one or the other,
> > > but not both, then two locks would be enough to build what we really
> > > want.
> > > 
> > >> I agree that it's very similar to the proposed op blocker style, but I
> > >> can't really come up with a meaningful translation either.
> > >>
> > >> Maybe something like this (?): All readers who do not want the file to
> > >> be modified grab a shared lock on byte 1. All writers who can deal with
> > >> volatile data grab a shared lock on byte 2. Exclusive writers grab an
> > >> exclusive lock on byte 1 and 2. Readers who can cope with volatile data
> > >> get no lock at all.
> > >>
> > >> When opening, the first and second group would always have to test
> > >> whether there is a lock on the other byte, respectively. E.g. sharing
> > >> writers would first grab an exclusive lock on byte 1, then the shared
> > >> lock on byte 2 and then release the exclusive lock again.
> > >>
> > >> Would that work?
> > > 
> > > I'm afraid it wouldn't. If you start the sharing writer first and then
> > > the writer-blocking reader, the writer doesn't hold a lock on byte 1 any
> > > more,
> > 
> > But it holds a lock on byte 2.
> > 
> > >       so the reader can start even though someone is writing to the
> > > image.
> > 
> > It can't because it would try to grab an exclusive lock on byte 2 before
> > grabbing the shared lock on byte 1.
> 
> Apparently I failed to understand the most important part of the
> proposal. :-)
> 
> So we have two locks. Both are only held for a longer time in shared
> mode. Exclusive mode is only used for testing whether the lock is being
> held and is immediately given up again.
> 
> The meaning of holding a shared lock is:
> 
>     byte 1: I can't allow other processes to write to the image
>     byte 2: I am writing to the image
> 
> The four cases that we have involve:
> 
> * shared writer: Take shared lock on byte 2. Test whether byte 1 is
>   locked using an exclusive lock, and fail if so.
> 
> * exclusive writer: Take shared lock on byte 2. Test whether byte 1 is
>   locked using an exclusive lock, and fail if so. Then take shared lock
>   on byte 1. I suppose this is racy, but we can probably tolerate that.
> 
> * reader that can tolerate writers: Don't do anything
> 
> * reader that can't tolerate writers: Take shared lock on byte 1. Test
>   whether byte 2 is locked, and fail if so.
> 
> Seems to work if I got that right.

Does this mean I should change ImageLockMode to:

 * exclusive
 * shared-write
 * shared-read
 * nolock
 * auto

Where "auto" maps to exclusive for O_RDWR and shared-read for O_RDONLY?

Fam

Re: [Qemu-devel] [PATCH v8 05/36] raw-posix: Add image locking support

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4340 bytes --]

On 25.10.2016 15:43, Fam Zheng wrote:
> On Tue, 10/25 15:28, Max Reitz wrote:
>> On 25.10.2016 08:31, Fam Zheng wrote:
>>> On Sat, 10/22 01:40, Max Reitz wrote:
>>>> On 30.09.2016 14:09, Fam Zheng wrote:
>>
>> [...]
>>
>>>>> +static int
>>>>> +raw_reopen_upgrade(BDRVReopenState *state,
>>>>> +                   RawReopenOperation op,
>>>>> +                   ImageLockMode old_lock,
>>>>> +                   ImageLockMode new_lock,
>>>>> +                   Error **errp)
>>>>> +{
>>>>> +    BDRVRawReopenState *raw_s = state->opaque;
>>>>> +    BDRVRawState *s = state->bs->opaque;
>>>>> +    int ret = 0, ret2;
>>>>> +
>>>>> +    assert(old_lock == IMAGE_LOCK_MODE_SHARED);
>>>>> +    assert(new_lock == IMAGE_LOCK_MODE_EXCLUSIVE);
>>>>> +    switch (op) {
>>>>> +    case RAW_REOPEN_PREPARE:
>>>>> +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
>>>
>>> [1]
>>>
>>>>> +        if (ret) {
>>>>> +            error_setg_errno(errp, -ret, "Failed to lock new fd (shared)");
>>>>> +            break;
>>>>> +        }
>>>>> +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_NOLOCK);
>>>
>>> [2]
>>>
>>>>> +        if (ret) {
>>>>> +            error_setg_errno(errp, -ret, "Failed to unlock old fd");
>>>>> +            goto restore;
>>>>> +        }
>>>>> +        ret = raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_EXCLUSIVE);
>>>
>>> [3]
>>>
>>>>> +        if (ret) {
>>>>> +            error_setg_errno(errp, -ret, "Failed to lock new fd (exclusive)");
>>>>> +            goto restore;
>>>>> +        }
>>>>> +        break;
>>>>> +    case RAW_REOPEN_COMMIT:
>>>>> +        break;
>>>>> +    case RAW_REOPEN_ABORT:
>>>>> +        raw_lock_fd(raw_s->lock_fd, IMAGE_LOCK_MODE_SHARED);
>>>>> +        ret = raw_lock_fd(s->lock_fd, IMAGE_LOCK_MODE_SHARED);
>>>
>>> [4]
>>>
>>>>> +        if (ret) {
>>>>> +            error_report("Failed to restore lock on old fd");
>>>>
>>>> If we get here, s->lock_fd is still locked exclusively. The following is
>>>> a very personal opinion, but anyway: I think it would be be better for
>>>> it to be unlocked. If it's locked too strictly, this can really break
>>>> something; but if it's just not locked (while it should be locked in
>>>> shared mode), everything's going to be fine unless the user makes a
>>>> mistake. I think the latter is less bad.
>>>
>>> There are four lock states when we land on this "abort" branch:
>>>
>>>   a) Lock "prepare" was not run, some other error happened earlier, so the lock
>>>      aren't changed compared to before the transaction starts: raw_s->lock_fd is
>>>      unlocked, s->lock_fd is shared locked. In this case raw_lock_fd [4] cannot
>>>      fail, and even if it does, s->lock_fd is in the correct state.
>>>
>>>   b) The raw_lock_fd [1] failed. This is similar to 1), s->lock_fd is intact, so
>>>      we are good.
>>>
>>>   c) The raw_lock_fd [2] failed. Again, similar to above.
>>>
>>>   d) The raw_lock_fd [3] failed. Here raw_s->lock_fd is shared locked, and
>>>      s->lock_fd is unlocked. The correct "abort" sequence is downgrade
>>>      raw_s->lock_fd and "shared lock" s->lock_fd again. If the "abort" sequence
>>>      failed, s->lock_fd is unlocked.
>>
>> OK, you're right, I somehow forgot about the cases where our prepare
>> sequence was either not run at all or failed. But I was thinking about
>> the case where our preparation was successful, but some later
>> preparation in the reopen transaction failed. Then, s->lock_fd should be
>> locked exclusively, no?
> 
> Oh I missed to reason about that branch. Here we go:
> 
> If our preparation succeeded, raw_s->lock_fd is exclusively locked, s->lock_fd
> is unlocked. In abort we should try to return to the old state (raw_s->lock_fd
> is _not_ exclusively locked and s->lock_fd is shared locked), hence the two
> raw_lock_fd() calls at [4]. In this case, if the second raw_lock_fd() at [4]
> doesn't work, s->lock_fd is unlocked, and raw_s->lock_fd will be closed anyway,
> which again matches what you suggested.

Oh, right, it's raw_s->lock_fd that's exclusively locked, not s->lock_fd...

See? I really can't tell the difference between raw_s and s. :-/

So you're right, but that makes me just all the more skeptical about
raw_s...

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4046 bytes --]

On 25.10.2016 16:57, Kevin Wolf wrote:
> Am 25.10.2016 um 15:30 hat Max Reitz geschrieben:
>> On 25.10.2016 10:24, Kevin Wolf wrote:
>>> Am 24.10.2016 um 20:03 hat Max Reitz geschrieben:
>>>> On 24.10.2016 12:11, Kevin Wolf wrote:
>>>>
>>>> [...]
>>>>
>>>>> Now, the big question is how to translate this into file locking. This
>>>>> could become a little tricky. I had a few thoughts involving another
>>>>> lock on byte 2, but none of them actually worked out so far, because
>>>>> what we want is essentially a lock that can be shared by readers, that
>>>>> can also be shared by writers, but not by readers and writers at the
>>>>> same time.
>>>>
>>>> You can also share it between readers and writers, as long as everyone
>>>> can cope with volatile data.
>>>
>>> Sorry, that was ambiguous. I meant a file-level lock rather than the
>>> high-level one. If we had a lock that can be shared by one or the other,
>>> but not both, then two locks would be enough to build what we really
>>> want.
>>>
>>>> I agree that it's very similar to the proposed op blocker style, but I
>>>> can't really come up with a meaningful translation either.
>>>>
>>>> Maybe something like this (?): All readers who do not want the file to
>>>> be modified grab a shared lock on byte 1. All writers who can deal with
>>>> volatile data grab a shared lock on byte 2. Exclusive writers grab an
>>>> exclusive lock on byte 1 and 2. Readers who can cope with volatile data
>>>> get no lock at all.
>>>>
>>>> When opening, the first and second group would always have to test
>>>> whether there is a lock on the other byte, respectively. E.g. sharing
>>>> writers would first grab an exclusive lock on byte 1, then the shared
>>>> lock on byte 2 and then release the exclusive lock again.
>>>>
>>>> Would that work?
>>>
>>> I'm afraid it wouldn't. If you start the sharing writer first and then
>>> the writer-blocking reader, the writer doesn't hold a lock on byte 1 any
>>> more,
>>
>> But it holds a lock on byte 2.
>>
>>>       so the reader can start even though someone is writing to the
>>> image.
>>
>> It can't because it would try to grab an exclusive lock on byte 2 before
>> grabbing the shared lock on byte 1.
> 
> Apparently I failed to understand the most important part of the
> proposal. :-)
> 
> So we have two locks. Both are only held for a longer time in shared
> mode. Exclusive mode is only used for testing whether the lock is being
> held and is immediately given up again.
> 
> The meaning of holding a shared lock is:
> 
>     byte 1: I can't allow other processes to write to the image
>     byte 2: I am writing to the image
> 
> The four cases that we have involve:
> 
> * shared writer: Take shared lock on byte 2. Test whether byte 1 is
>   locked using an exclusive lock, and fail if so.

To not be racy, I'd first take the exclusive lock, then the shared lock,
and then release the exclusive lock.

> * exclusive writer: Take shared lock on byte 2. Test whether byte 1 is
>   locked using an exclusive lock, and fail if so. Then take shared lock
>   on byte 1. I suppose this is racy, but we can probably tolerate that.

Well, if you want to have the explicit meanings you gave to two bytes...
Otherwise, you'd just take an exclusive lock on both bytes.

Or you'd start out as a reader that can't tolerate writers (i.e. take
exclusive lock on byte 2, take shared lock on byte 1, release exclusive
lock on byte 2) and then try to upgrade it to be an exclusive writer by
taking an exclusive lock on byte 2 and then downgrading it to a shared lock.

Of course, you can optimize this sequence to: Take exclusive lock on
byte 2, take shared lock on byte 1, downgrade exclusive lock to shared lock.

Max

> * reader that can tolerate writers: Don't do anything
> 
> * reader that can't tolerate writers: Take shared lock on byte 1. Test
>   whether byte 2 is locked, and fail if so.
> 
> Seems to work if I got that right.
> 
> Kevin
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4265 bytes --]

On 26.10.2016 13:01, Fam Zheng wrote:
> On Tue, 10/25 16:57, Kevin Wolf wrote:
>> Am 25.10.2016 um 15:30 hat Max Reitz geschrieben:
>>> On 25.10.2016 10:24, Kevin Wolf wrote:
>>>> Am 24.10.2016 um 20:03 hat Max Reitz geschrieben:
>>>>> On 24.10.2016 12:11, Kevin Wolf wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>> Now, the big question is how to translate this into file locking. This
>>>>>> could become a little tricky. I had a few thoughts involving another
>>>>>> lock on byte 2, but none of them actually worked out so far, because
>>>>>> what we want is essentially a lock that can be shared by readers, that
>>>>>> can also be shared by writers, but not by readers and writers at the
>>>>>> same time.
>>>>>
>>>>> You can also share it between readers and writers, as long as everyone
>>>>> can cope with volatile data.
>>>>
>>>> Sorry, that was ambiguous. I meant a file-level lock rather than the
>>>> high-level one. If we had a lock that can be shared by one or the other,
>>>> but not both, then two locks would be enough to build what we really
>>>> want.
>>>>
>>>>> I agree that it's very similar to the proposed op blocker style, but I
>>>>> can't really come up with a meaningful translation either.
>>>>>
>>>>> Maybe something like this (?): All readers who do not want the file to
>>>>> be modified grab a shared lock on byte 1. All writers who can deal with
>>>>> volatile data grab a shared lock on byte 2. Exclusive writers grab an
>>>>> exclusive lock on byte 1 and 2. Readers who can cope with volatile data
>>>>> get no lock at all.
>>>>>
>>>>> When opening, the first and second group would always have to test
>>>>> whether there is a lock on the other byte, respectively. E.g. sharing
>>>>> writers would first grab an exclusive lock on byte 1, then the shared
>>>>> lock on byte 2 and then release the exclusive lock again.
>>>>>
>>>>> Would that work?
>>>>
>>>> I'm afraid it wouldn't. If you start the sharing writer first and then
>>>> the writer-blocking reader, the writer doesn't hold a lock on byte 1 any
>>>> more,
>>>
>>> But it holds a lock on byte 2.
>>>
>>>>       so the reader can start even though someone is writing to the
>>>> image.
>>>
>>> It can't because it would try to grab an exclusive lock on byte 2 before
>>> grabbing the shared lock on byte 1.
>>
>> Apparently I failed to understand the most important part of the
>> proposal. :-)
>>
>> So we have two locks. Both are only held for a longer time in shared
>> mode. Exclusive mode is only used for testing whether the lock is being
>> held and is immediately given up again.
>>
>> The meaning of holding a shared lock is:
>>
>>     byte 1: I can't allow other processes to write to the image
>>     byte 2: I am writing to the image
>>
>> The four cases that we have involve:
>>
>> * shared writer: Take shared lock on byte 2. Test whether byte 1 is
>>   locked using an exclusive lock, and fail if so.
>>
>> * exclusive writer: Take shared lock on byte 2. Test whether byte 1 is
>>   locked using an exclusive lock, and fail if so. Then take shared lock
>>   on byte 1. I suppose this is racy, but we can probably tolerate that.
>>
>> * reader that can tolerate writers: Don't do anything
>>
>> * reader that can't tolerate writers: Take shared lock on byte 1. Test
>>   whether byte 2 is locked, and fail if so.
>>
>> Seems to work if I got that right.
> 
> Does this mean I should change ImageLockMode to:
> 
>  * exclusive
>  * shared-write
>  * shared-read

Hm, those don't sound quite right, since it sounds as if you could mix
shared-read and shared-write. But you shouldn't be able to open an image
in shared-read lock mode when someone has opened it in shared-write lock
mode already.

It's difficult to come up with a clear but short name for shared-read
("exclusive", "shared-write", and "nolock" sound good to me). Maybe
"non-volatile" or "constant"? Or maybe "shared-only-read" would be clear
enough?

>  * nolock
>  * auto
> 
> Where "auto" maps to exclusive for O_RDWR and shared-read for O_RDONLY?

Yep, that would be the correct mapping. Maybe later we can introduce an
auto-shared mode that maps to shared-write or nolock, respectively.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 4906 bytes --]

Am 26.10.2016 um 17:12 hat Max Reitz geschrieben:
> On 26.10.2016 13:01, Fam Zheng wrote:
> > On Tue, 10/25 16:57, Kevin Wolf wrote:
> >> Am 25.10.2016 um 15:30 hat Max Reitz geschrieben:
> >>> On 25.10.2016 10:24, Kevin Wolf wrote:
> >>>> Am 24.10.2016 um 20:03 hat Max Reitz geschrieben:
> >>>>> On 24.10.2016 12:11, Kevin Wolf wrote:
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>>> Now, the big question is how to translate this into file locking. This
> >>>>>> could become a little tricky. I had a few thoughts involving another
> >>>>>> lock on byte 2, but none of them actually worked out so far, because
> >>>>>> what we want is essentially a lock that can be shared by readers, that
> >>>>>> can also be shared by writers, but not by readers and writers at the
> >>>>>> same time.
> >>>>>
> >>>>> You can also share it between readers and writers, as long as everyone
> >>>>> can cope with volatile data.
> >>>>
> >>>> Sorry, that was ambiguous. I meant a file-level lock rather than the
> >>>> high-level one. If we had a lock that can be shared by one or the other,
> >>>> but not both, then two locks would be enough to build what we really
> >>>> want.
> >>>>
> >>>>> I agree that it's very similar to the proposed op blocker style, but I
> >>>>> can't really come up with a meaningful translation either.
> >>>>>
> >>>>> Maybe something like this (?): All readers who do not want the file to
> >>>>> be modified grab a shared lock on byte 1. All writers who can deal with
> >>>>> volatile data grab a shared lock on byte 2. Exclusive writers grab an
> >>>>> exclusive lock on byte 1 and 2. Readers who can cope with volatile data
> >>>>> get no lock at all.
> >>>>>
> >>>>> When opening, the first and second group would always have to test
> >>>>> whether there is a lock on the other byte, respectively. E.g. sharing
> >>>>> writers would first grab an exclusive lock on byte 1, then the shared
> >>>>> lock on byte 2 and then release the exclusive lock again.
> >>>>>
> >>>>> Would that work?
> >>>>
> >>>> I'm afraid it wouldn't. If you start the sharing writer first and then
> >>>> the writer-blocking reader, the writer doesn't hold a lock on byte 1 any
> >>>> more,
> >>>
> >>> But it holds a lock on byte 2.
> >>>
> >>>>       so the reader can start even though someone is writing to the
> >>>> image.
> >>>
> >>> It can't because it would try to grab an exclusive lock on byte 2 before
> >>> grabbing the shared lock on byte 1.
> >>
> >> Apparently I failed to understand the most important part of the
> >> proposal. :-)
> >>
> >> So we have two locks. Both are only held for a longer time in shared
> >> mode. Exclusive mode is only used for testing whether the lock is being
> >> held and is immediately given up again.
> >>
> >> The meaning of holding a shared lock is:
> >>
> >>     byte 1: I can't allow other processes to write to the image
> >>     byte 2: I am writing to the image
> >>
> >> The four cases that we have involve:
> >>
> >> * shared writer: Take shared lock on byte 2. Test whether byte 1 is
> >>   locked using an exclusive lock, and fail if so.
> >>
> >> * exclusive writer: Take shared lock on byte 2. Test whether byte 1 is
> >>   locked using an exclusive lock, and fail if so. Then take shared lock
> >>   on byte 1. I suppose this is racy, but we can probably tolerate that.
> >>
> >> * reader that can tolerate writers: Don't do anything
> >>
> >> * reader that can't tolerate writers: Take shared lock on byte 1. Test
> >>   whether byte 2 is locked, and fail if so.
> >>
> >> Seems to work if I got that right.
> > 
> > Does this mean I should change ImageLockMode to:
> > 
> >  * exclusive
> >  * shared-write
> >  * shared-read
> 
> Hm, those don't sound quite right, since it sounds as if you could mix
> shared-read and shared-write. But you shouldn't be able to open an image
> in shared-read lock mode when someone has opened it in shared-write lock
> mode already.
> 
> It's difficult to come up with a clear but short name for shared-read
> ("exclusive", "shared-write", and "nolock" sound good to me). Maybe
> "non-volatile" or "constant"? Or maybe "shared-only-read" would be clear
> enough?

As we concluded above, this is really a matrix a two bools rather than a
single property. We need a new option for "I can't allow other processes
to write to the image", but we don't need one for "I am writing to the
image" because that's the read-only property that we already have.

> >  * nolock
> >  * auto
> > 
> > Where "auto" maps to exclusive for O_RDWR and shared-read for O_RDONLY?
> 
> Yep, that would be the correct mapping. Maybe later we can introduce an
> auto-shared mode that maps to shared-write or nolock, respectively.

No auto needed any more, the default is simply false (i.e. don't share
with other writers).

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5006 bytes --]

On 26.10.2016 17:33, Kevin Wolf wrote:
> Am 26.10.2016 um 17:12 hat Max Reitz geschrieben:
>> On 26.10.2016 13:01, Fam Zheng wrote:
>>> On Tue, 10/25 16:57, Kevin Wolf wrote:
>>>> Am 25.10.2016 um 15:30 hat Max Reitz geschrieben:
>>>>> On 25.10.2016 10:24, Kevin Wolf wrote:
>>>>>> Am 24.10.2016 um 20:03 hat Max Reitz geschrieben:
>>>>>>> On 24.10.2016 12:11, Kevin Wolf wrote:
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>> Now, the big question is how to translate this into file locking. This
>>>>>>>> could become a little tricky. I had a few thoughts involving another
>>>>>>>> lock on byte 2, but none of them actually worked out so far, because
>>>>>>>> what we want is essentially a lock that can be shared by readers, that
>>>>>>>> can also be shared by writers, but not by readers and writers at the
>>>>>>>> same time.
>>>>>>>
>>>>>>> You can also share it between readers and writers, as long as everyone
>>>>>>> can cope with volatile data.
>>>>>>
>>>>>> Sorry, that was ambiguous. I meant a file-level lock rather than the
>>>>>> high-level one. If we had a lock that can be shared by one or the other,
>>>>>> but not both, then two locks would be enough to build what we really
>>>>>> want.
>>>>>>
>>>>>>> I agree that it's very similar to the proposed op blocker style, but I
>>>>>>> can't really come up with a meaningful translation either.
>>>>>>>
>>>>>>> Maybe something like this (?): All readers who do not want the file to
>>>>>>> be modified grab a shared lock on byte 1. All writers who can deal with
>>>>>>> volatile data grab a shared lock on byte 2. Exclusive writers grab an
>>>>>>> exclusive lock on byte 1 and 2. Readers who can cope with volatile data
>>>>>>> get no lock at all.
>>>>>>>
>>>>>>> When opening, the first and second group would always have to test
>>>>>>> whether there is a lock on the other byte, respectively. E.g. sharing
>>>>>>> writers would first grab an exclusive lock on byte 1, then the shared
>>>>>>> lock on byte 2 and then release the exclusive lock again.
>>>>>>>
>>>>>>> Would that work?
>>>>>>
>>>>>> I'm afraid it wouldn't. If you start the sharing writer first and then
>>>>>> the writer-blocking reader, the writer doesn't hold a lock on byte 1 any
>>>>>> more,
>>>>>
>>>>> But it holds a lock on byte 2.
>>>>>
>>>>>>       so the reader can start even though someone is writing to the
>>>>>> image.
>>>>>
>>>>> It can't because it would try to grab an exclusive lock on byte 2 before
>>>>> grabbing the shared lock on byte 1.
>>>>
>>>> Apparently I failed to understand the most important part of the
>>>> proposal. :-)
>>>>
>>>> So we have two locks. Both are only held for a longer time in shared
>>>> mode. Exclusive mode is only used for testing whether the lock is being
>>>> held and is immediately given up again.
>>>>
>>>> The meaning of holding a shared lock is:
>>>>
>>>>     byte 1: I can't allow other processes to write to the image
>>>>     byte 2: I am writing to the image
>>>>
>>>> The four cases that we have involve:
>>>>
>>>> * shared writer: Take shared lock on byte 2. Test whether byte 1 is
>>>>   locked using an exclusive lock, and fail if so.
>>>>
>>>> * exclusive writer: Take shared lock on byte 2. Test whether byte 1 is
>>>>   locked using an exclusive lock, and fail if so. Then take shared lock
>>>>   on byte 1. I suppose this is racy, but we can probably tolerate that.
>>>>
>>>> * reader that can tolerate writers: Don't do anything
>>>>
>>>> * reader that can't tolerate writers: Take shared lock on byte 1. Test
>>>>   whether byte 2 is locked, and fail if so.
>>>>
>>>> Seems to work if I got that right.
>>>
>>> Does this mean I should change ImageLockMode to:
>>>
>>>  * exclusive
>>>  * shared-write
>>>  * shared-read
>>
>> Hm, those don't sound quite right, since it sounds as if you could mix
>> shared-read and shared-write. But you shouldn't be able to open an image
>> in shared-read lock mode when someone has opened it in shared-write lock
>> mode already.
>>
>> It's difficult to come up with a clear but short name for shared-read
>> ("exclusive", "shared-write", and "nolock" sound good to me). Maybe
>> "non-volatile" or "constant"? Or maybe "shared-only-read" would be clear
>> enough?
> 
> As we concluded above, this is really a matrix a two bools rather than a
> single property. We need a new option for "I can't allow other processes
> to write to the image", but we don't need one for "I am writing to the
> image" because that's the read-only property that we already have.
> 
>>>  * nolock
>>>  * auto
>>>
>>> Where "auto" maps to exclusive for O_RDWR and shared-read for O_RDONLY?
>>
>> Yep, that would be the correct mapping. Maybe later we can introduce an
>> auto-shared mode that maps to shared-write or nolock, respectively.
> 
> No auto needed any more, the default is simply false (i.e. don't share
> with other writers).

Well, that was too easy. :-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]

Re: [Qemu-devel] [PATCH v8 00/36] block: Image locking series

[Fam Zheng]

On Wed, 10/26 17:33, Kevin Wolf wrote:
> 
> As we concluded above, this is really a matrix a two bools rather than a
> single property. We need a new option for "I can't allow other processes
> to write to the image", but we don't need one for "I am writing to the
> image" because that's the read-only property that we already have.

Sounds good! Thanks.

Fam