[Qemu-devel] [PATCH v6 00/22] block: Lock images when opening

[Qemu-devel] [PATCH v6 00/22] block: Lock images when opening

[Fam Zheng]

v6: Rebase, and address comments from Max and Rich:

    Dropped the gluster implementation because the previous reviewed version
    doesn't handle reopen correctly. Left for another day.

    Added Max's reviewed-by in patches 1, 3, 7, 12, 13, 16, 17 and 20 - 22.

    Updated "qemu -help" text. [Rich]

    Per patch changelog:

    [02/22] qapi: Add lock-mode in blockdev-add options
            Fix the copy-pasto in qapi comment. [Max]

    [04/22] block: Introduce image file locking
            Partly reworked, main difference being the new
            "bdrv_get_locking_cmd".

    [05/22] osdep: Add qemu_lock_fd and qemu_unlock_fd
            Comment fix, whitespace and empty line cleaning up, and error code.
            [Max]
            Put the new functions under "#ifndef _WIN32" so windows build
            doesn't break.

    [06/22] osdep: Introduce qemu_dup
            Commit message grammar. [Max]
            Wrap with "#ifndef _WIN32". [Max]

    [07/22] raw-posix: Use qemu_dup

    [08/22] raw-posix: Add image locking support
            Basically rewritten to handle reopen more reliably.

    [09/22] qemu-io: Add "-L" option for BDRV_O_NO_LOCK
            "image lock" -> "image locking" for consistency. [Max]
            "int nolock" -> "bool nolock". [Max]

    [11/22] qemu-img: Update documentation of "-L" option
            Drop dead assignment to flags and src_flags. [Max]

    [15/22] qemu-iotests: 046: Move version detection out from verify_io
            Drop unnecessary "-L". [Max]

    [18/22] iotests: 087: Disable image locking in cases where file is shared
            Spell fix in commit message. [Max]
            Move '"lock-mode": "off"' after "node-name" for consistency. [Max]

    [19/22] iotests: Disable image locking in 085
            Use "'read-only': true". [Max]

v5: - Change "lock-image=on/off" to "lock-mode=exclusive/shared/off".
      Default is "lock-mode=exclusive" to exclusively lock RW images and shared
      lock RO images; with lock-mode="shared", RW images are shared locked too;
      lock-mode=off turns off image locking completely.
    - Use F_OFD_SETLK fcntl so that close/dup on different fds are not a
      problem.
    - Update test cases.


Fam Zheng (22):
  block: Add flag bits for image locking
  qapi: Add lock-mode in blockdev-add options
  blockdev: Add and parse "lock-mode" option for image locking
  block: Introduce image file locking
  osdep: Add qemu_lock_fd and qemu_unlock_fd
  osdep: Introduce qemu_dup
  raw-posix: Use qemu_dup
  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
  mirror: Disable image locking on target backing chain
  qemu-iotests: 046: Move version detection out from verify_io
  qemu-iotests: Wait for QEMU processes before checking image in 091
  qemu-iotests: 030: Disable image locking when checking test image
  iotests: 087: Disable image locking in cases where file is shared
  iotests: Disable image locking in 085
  tests: Use null-co:// instead of /dev/null
  block: Turn on image locking by default
  qemu-iotests: Add test case 153 for image locking

 block.c                    |  57 ++++++
 block/raw-posix.c          | 295 ++++++++++++++++++++++++++++++-
 blockdev.c                 |  38 +++-
 include/block/block.h      |  13 +-
 include/block/block_int.h  |   5 +
 include/qemu/osdep.h       |   5 +
 qapi/block-core.json       |  19 +-
 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 +
 qemu-options.hx            |   1 +
 tests/drive_del-test.c     |   2 +-
 tests/nvme-test.c          |   2 +-
 tests/qemu-iotests/030     |   2 +-
 tests/qemu-iotests/046     |  22 +--
 tests/qemu-iotests/085     |   3 +-
 tests/qemu-iotests/087     |   6 +
 tests/qemu-iotests/091     |   3 +
 tests/qemu-iotests/091.out |   1 +
 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               |  38 ++++
 30 files changed, 1241 insertions(+), 77 deletions(-)
 create mode 100755 tests/qemu-iotests/153
 create mode 100644 tests/qemu-iotests/153.out

-- 
2.8.2

[Qemu-devel] [PATCH v6 01/22] 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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index 70ea299..02b598b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -94,6 +94,8 @@ 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_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
 
-- 
2.8.2

[Qemu-devel] [PATCH v6 02/22] qapi: Add lock-mode in blockdev-add options

[Fam Zheng]

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

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

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 98a20d2..23ec31d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2032,6 +2032,20 @@
             '*read-pattern': 'QuorumReadPattern' } }
 
 ##
+# @BlockdevLockMode
+#
+# Describes how QEMU should lock the image.
+#
+# @off:       Disabled
+# @shared:    Use shared lock for both RO and RW images.
+# @exclusive: Use exclusive lock for RW images, and shared lock for RO images.
+#
+# Since: 2.7
+##
+{ 'enum': 'BlockdevLockMode',
+  'data': [ 'off', 'shared', 'exclusive' ] }
+
+##
 # @BlockdevOptions
 #
 # Options for creating a block device.  Many options are available for all
@@ -2065,6 +2079,8 @@
 # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
 #                 (default: off)
 #
+# @lock-mode: #optional how to lock the image. (default: exclusive) (Since 2.7)
+#
 # Remaining options are determined by the block driver.
 #
 # Since: 1.7
@@ -2082,7 +2098,8 @@
             '*stats-account-invalid': 'bool',
             '*stats-account-failed': 'bool',
             '*stats-intervals': ['int'],
-            '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
+            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
+            '*lock-mode': 'BlockdevLockMode' },
   'discriminator': 'driver',
   'data': {
       'archipelago':'BlockdevOptionsArchipelago',
-- 
2.8.2

[Qemu-devel] [PATCH v6 03/22] blockdev: Add and parse "lock-mode" option for image locking

[Fam Zheng]

Respect the locking mode from CLI or QMP, and set the open flags
accordingly.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c      | 23 +++++++++++++++++++++++
 qemu-options.hx |  1 +
 2 files changed, 24 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 717785e..5acb286 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -356,6 +356,7 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
     const char *discard;
     Error *local_error = NULL;
     const char *aio;
+    const char *lock_mode;
 
     if (bdrv_flags) {
         if (!qemu_opt_get_bool(opts, "read-only", false)) {
@@ -382,6 +383,18 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
                return;
             }
         }
+
+        lock_mode = qemu_opt_get(opts, "lock-mode") ? : "off";
+        if (!strcmp(lock_mode, "exclusive")) {
+            /* Default */
+        } else if (!strcmp(lock_mode, "shared")) {
+            *bdrv_flags |= BDRV_O_SHARED_LOCK;
+        } else if (!strcmp(lock_mode, "off")) {
+            *bdrv_flags |= BDRV_O_NO_LOCK;
+        } else {
+           error_setg(errp, "invalid lock mode");
+           return;
+        }
     }
 
     /* disk I/O throttling */
@@ -4296,6 +4309,11 @@ QemuOptsList qemu_common_drive_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "whether to account for failed I/O operations "
                     "in the statistics",
+        },{
+            .name = "lock-mode",
+            .type = QEMU_OPT_STRING,
+            .help = "how to lock the image (exclusive, shared, off. "
+                    "default: exclusive)",
         },
         { /* end of list */ }
     },
@@ -4325,6 +4343,11 @@ static QemuOptsList qemu_root_bds_opts = {
             .name = "detect-zeroes",
             .type = QEMU_OPT_STRING,
             .help = "try to optimize zero writes (off, on, unmap)",
+        },{
+            .name = "lock-mode",
+            .type = QEMU_OPT_STRING,
+            .help = "how to lock the image (exclusive, shared, off. "
+                    "default: exclusive)",
         },
         { /* end of list */ }
     },
diff --git a/qemu-options.hx b/qemu-options.hx
index 6106520..731cdda 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -524,6 +524,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "       [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
     "       [,readonly=on|off][,copy-on-read=on|off]\n"
     "       [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
+    "       [,lock-mode=exclusive|shared|off]\n"
     "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
     "       [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n"
     "       [[,bps_max=bm]|[[,bps_rd_max=rm][,bps_wr_max=wm]]]\n"
-- 
2.8.2

[Qemu-devel] [PATCH v6 04/22] 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                   | 57 +++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h     | 11 ++++++++-
 include/block/block_int.h |  5 +++++
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 736432f..4c2a3ff 100644
--- a/block.c
+++ b/block.c
@@ -854,6 +854,50 @@ out:
     g_free(gen_node_name);
 }
 
+BdrvLockfCmd bdrv_get_locking_cmd(int flags)
+{
+    if (flags & BDRV_O_NO_LOCK) {
+        return BDRV_LOCKF_UNLOCK;
+    } else if (flags & BDRV_O_SHARED_LOCK) {
+        return BDRV_LOCKF_SHARED;
+    } else if (flags & BDRV_O_RDWR) {
+        return BDRV_LOCKF_EXCLUSIVE;
+    } else {
+        return BDRV_LOCKF_SHARED;
+    }
+}
+
+static int bdrv_lock_unlock_image_do(BlockDriverState *bs, BdrvLockfCmd cmd)
+{
+    int ret;
+
+    if (bs->cur_lock == cmd) {
+        return 0;
+    } else if (!bs->drv) {
+        return -ENOMEDIUM;
+    } else if (!bs->drv->bdrv_lockf) {
+        return 0;
+    }
+    ret = bs->drv->bdrv_lockf(bs, cmd);
+    if (ret == -ENOTSUP) {
+        /* Handle it the same way as !bs->drv->bdrv_lockf */
+        ret = 0;
+    } else if (ret == 0) {
+        bs->cur_lock = cmd;
+    }
+    return ret;
+}
+
+static int bdrv_lock_image(BlockDriverState *bs)
+{
+    return bdrv_lock_unlock_image_do(bs, bdrv_get_locking_cmd(bs->open_flags));
+}
+
+static int bdrv_unlock_image(BlockDriverState *bs)
+{
+    return bdrv_lock_unlock_image_do(bs, BDRV_LOCKF_UNLOCK);
+}
+
 static QemuOptsList bdrv_runtime_opts = {
     .name = "bdrv_common",
     .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
@@ -1003,6 +1047,14 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
         goto free_and_fail;
     }
 
+    if (!(open_flags & (BDRV_O_NO_LOCK | BDRV_O_INACTIVE))) {
+        ret = bdrv_lock_image(bs);
+        if (ret) {
+            error_setg(errp, "Failed to lock image");
+            goto free_and_fail;
+        }
+    }
+
     ret = refresh_total_sectors(bs, bs->total_sectors);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not refresh total sector count");
@@ -2164,6 +2216,7 @@ static void bdrv_close(BlockDriverState *bs)
     if (bs->drv) {
         BdrvChild *child, *next;
 
+        bdrv_unlock_image(bs);
         bs->drv->bdrv_close(bs);
         bs->drv = NULL;
 
@@ -3187,6 +3240,9 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
         error_setg_errno(errp, -ret, "Could not refresh total sector count");
         return;
     }
+    if (!(bs->open_flags & BDRV_O_NO_LOCK)) {
+        bdrv_lock_image(bs);
+    }
 }
 
 void bdrv_invalidate_cache_all(Error **errp)
@@ -3229,6 +3285,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
     }
 
     if (setting_flag) {
+        ret = bdrv_unlock_image(bs);
         bs->open_flags |= BDRV_O_INACTIVE;
     }
     return 0;
diff --git a/include/block/block.h b/include/block/block.h
index 02b598b..782ef2f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -183,6 +183,15 @@ typedef enum BlockOpType {
     BLOCK_OP_TYPE_MAX,
 } BlockOpType;
 
+typedef enum {
+    /* The values are ordered so that lower number implies higher restriction.
+     * Starting from 1 to make 0 an invalid value.
+     * */
+    BDRV_LOCKF_EXCLUSIVE = 1,
+    BDRV_LOCKF_SHARED,
+    BDRV_LOCKF_UNLOCK,
+} BdrvLockfCmd;
+
 void bdrv_info_print(Monitor *mon, const QObject *data);
 void bdrv_info(Monitor *mon, QObject **ret_data);
 void bdrv_stats_print(Monitor *mon, const QObject *data);
@@ -274,7 +283,7 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
 BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
                                     BlockDriverState *bs);
 BlockDriverState *bdrv_find_base(BlockDriverState *bs);
-
+BdrvLockfCmd bdrv_get_locking_cmd(int flags);
 
 typedef struct BdrvCheckResult {
     int corruptions;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 30a9717..29cc632 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -317,6 +317,10 @@ struct BlockDriver {
                            Error **errp);
     void (*bdrv_del_child)(BlockDriverState *parent, BdrvChild *child,
                            Error **errp);
+    /**
+     * Lock/unlock the image.
+     */
+    int (*bdrv_lockf)(BlockDriverState *bs, BdrvLockfCmd cmd);
 
     QLIST_ENTRY(BlockDriver) list;
 };
@@ -500,6 +504,7 @@ struct BlockDriverState {
     unsigned io_plug_disabled;
 
     int quiesce_counter;
+    BdrvLockfCmd cur_lock;
 };
 
 struct BlockBackendRootState {
-- 
2.8.2

[Qemu-devel] [PATCH v6 05/22] 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 6937694..749214a 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -280,6 +280,8 @@ int qemu_madvise(void *addr, size_t len, int advice);
 
 int qemu_open(const char *name, int flags, ...);
 int qemu_close(int fd);
+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 9a7a439..085ed52 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -137,6 +137,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.8.2

[Qemu-devel] [PATCH v6 06/22] osdep: Introduce qemu_dup

[Fam Zheng]

This takes care of both the CLOEXEC flag and fd-path mapping for image
locking.

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

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 749214a..89c63c7 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -281,6 +281,9 @@ int qemu_madvise(void *addr, size_t len, int advice);
 int qemu_open(const char *name, int flags, ...);
 int qemu_close(int fd);
 int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
+#ifndef _WIN32
+int qemu_dup(int fd);
+#endif
 int qemu_unlock_fd(int fd, int64_t start, int64_t len);
 
 #if defined(__HAIKU__) && defined(__i386__)
diff --git a/util/osdep.c b/util/osdep.c
index 085ed52..1c87c1e 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -133,6 +133,15 @@ fail:
     return -1;
 }
 
+int qemu_dup(int fd)
+{
+    int r = qemu_dup_flags(fd, 0);
+    if (r == -1) {
+        return -errno;
+    }
+    return r;
+}
+
 static int qemu_parse_fdset(const char *param)
 {
     return qemu_parse_fd(param);
-- 
2.8.2

[Qemu-devel] [PATCH v6 07/22] raw-posix: Use qemu_dup

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/raw-posix.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index a4f5a1b..bb8669f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -645,15 +645,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
 
     if ((raw_s->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) {
         /* dup the original fd */
-        /* TODO: use qemu fcntl wrapper */
-#ifdef F_DUPFD_CLOEXEC
-        raw_s->fd = fcntl(s->fd, F_DUPFD_CLOEXEC, 0);
-#else
-        raw_s->fd = dup(s->fd);
-        if (raw_s->fd != -1) {
-            qemu_set_cloexec(raw_s->fd);
-        }
-#endif
+        raw_s->fd = qemu_dup(s->fd);
         if (raw_s->fd >= 0) {
             ret = fcntl_setfl(raw_s->fd, raw_s->open_flags);
             if (ret) {
-- 
2.8.2

[Qemu-devel] [PATCH v6 08/22] 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".

There have three different states, "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 | 285 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 285 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index bb8669f..6347350 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;
@@ -153,6 +154,7 @@ typedef struct BDRVRawState {
 
 typedef struct BDRVRawReopenState {
     int fd;
+    int lock_fd;
     int open_flags;
 #ifdef CONFIG_LINUX_AIO
     int use_aio;
@@ -397,6 +399,37 @@ static void raw_attach_aio_context(BlockDriverState *bs,
 #endif
 }
 
+static int raw_lockf_fd(int fd, BdrvLockfCmd cmd)
+{
+    assert(fd >= 0);
+    /* Locking byte 1 avoids interfereing with virtlockd. */
+    switch (cmd) {
+    case BDRV_LOCKF_EXCLUSIVE:
+        return qemu_lock_fd(fd, 1, 1, true);
+    case BDRV_LOCKF_SHARED:
+        return qemu_lock_fd(fd, 1, 1, false);
+    case BDRV_LOCKF_UNLOCK:
+        return qemu_unlock_fd(fd, 1, 1);
+    default:
+        abort();
+    }
+}
+
+static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
+{
+
+    BDRVRawState *s = bs->opaque;
+
+    if (s->lock_fd < 0) {
+        s->lock_fd = qemu_dup(s->fd);
+        if (s->lock_fd < 0) {
+            return s->lock_fd;
+        }
+    }
+
+    return raw_lockf_fd(s->lock_fd, cmd);
+}
+
 #ifdef CONFIG_LINUX_AIO
 static int raw_set_aio(LinuxAioState **aio_ctx, int *use_aio, int bdrv_flags)
 {
@@ -483,6 +516,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;
@@ -593,6 +627,241 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     return ret;
 }
 
+typedef enum {
+    RAW_REOPEN_PREPARE,
+    RAW_REOPEN_COMMIT,
+    RAW_REOPEN_ABORT
+} RawReopenOperation;
+
+typedef int (*RawReopenFunc)(BDRVReopenState *state,
+                             RawReopenOperation op,
+                             BdrvLockfCmd old_lock,
+                             BdrvLockfCmd new_lock,
+                             Error **errp);
+
+static
+int raw_reopen_identical(BDRVReopenState *state,
+                         RawReopenOperation op,
+                         BdrvLockfCmd old_lock,
+                         BdrvLockfCmd new_lock,
+                         Error **errp)
+{
+    assert(old_lock == new_lock);
+    return 0;
+}
+
+static
+int raw_reopen_from_unlock(BDRVReopenState *state,
+                           RawReopenOperation op,
+                           BdrvLockfCmd old_lock,
+                           BdrvLockfCmd new_lock,
+                           Error **errp)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+    int ret = 0;
+
+    assert(old_lock != new_lock);
+    assert(old_lock == BDRV_LOCKF_UNLOCK);
+    switch (op) {
+    case RAW_REOPEN_PREPARE:
+        ret = raw_lockf_fd(raw_s->lock_fd, new_lock);
+        if (ret) {
+            error_setg_errno(errp, -ret, "Failed to lock new fd");
+        }
+        break;
+    case RAW_REOPEN_COMMIT:
+    case RAW_REOPEN_ABORT:
+        break;
+    }
+
+    return ret;
+}
+
+static
+int raw_reopen_to_unlock(BDRVReopenState *state,
+                         RawReopenOperation op,
+                         BdrvLockfCmd old_lock,
+                         BdrvLockfCmd new_lock,
+                         Error **errp)
+{
+    BDRVRawState *s = state->bs->opaque;
+    int ret = 0;
+
+    assert(old_lock != new_lock);
+    assert(old_lock == BDRV_LOCKF_UNLOCK);
+    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,
+                       BdrvLockfCmd old_lock,
+                       BdrvLockfCmd new_lock,
+                       Error **errp)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+    BDRVRawState *s = state->bs->opaque;
+    int ret = 0, ret2;
+
+    assert(old_lock == BDRV_LOCKF_SHARED);
+    assert(new_lock == BDRV_LOCKF_EXCLUSIVE);
+    switch (op) {
+    case RAW_REOPEN_PREPARE:
+        ret = raw_lockf_fd(raw_s->lock_fd, BDRV_LOCKF_SHARED);
+        if (ret) {
+            error_setg_errno(errp, -ret, "Failed to lock new fd (shared)");
+            break;
+        }
+        ret = raw_lockf_fd(s->lock_fd, BDRV_LOCKF_UNLOCK);
+        if (ret) {
+            error_setg_errno(errp, -ret, "Failed to unlock old fd");
+            goto restore;
+        }
+        ret = raw_lockf_fd(raw_s->lock_fd, BDRV_LOCKF_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_lockf_fd(raw_s->lock_fd, BDRV_LOCKF_SHARED);
+        ret = raw_lockf_fd(s->lock_fd, BDRV_LOCKF_SHARED);
+        if (ret) {
+            error_report("Failed to restore lock on old fd");
+        }
+        break;
+    }
+
+    return ret;
+restore:
+    ret2 = raw_lockf_fd(s->lock_fd, BDRV_LOCKF_SHARED);
+    if (ret2) {
+        error_report("Failed to restore old lock");
+    }
+    return ret;
+
+}
+
+static
+int raw_reopen_downgrade(BDRVReopenState *state,
+                         RawReopenOperation op,
+                         BdrvLockfCmd old_lock,
+                         BdrvLockfCmd new_lock,
+                         Error **errp)
+{
+    BDRVRawReopenState *raw_s = state->opaque;
+    BDRVRawState *s = state->bs->opaque;
+    int ret;
+
+    assert(old_lock == BDRV_LOCKF_EXCLUSIVE);
+    assert(new_lock == BDRV_LOCKF_SHARED);
+    switch (op) {
+    case RAW_REOPEN_PREPARE:
+        break;
+    case RAW_REOPEN_COMMIT:
+        ret = raw_lockf_fd(s->lock_fd, BDRV_LOCKF_SHARED);
+        if (ret) {
+            error_report("Failed to downgrade old lock");
+            break;
+        }
+        ret = raw_lockf_fd(raw_s->lock_fd, BDRV_LOCKF_SHARED);
+        if (ret) {
+            error_report("Failed to lock new fd");
+            break;
+        }
+        break;
+    case RAW_REOPEN_ABORT:
+        break;
+    }
+
+    return ret;
+}
+
+static const struct RawReopenFuncRecord {
+    BdrvLockfCmd old_lock;
+    BdrvLockfCmd new_lock;
+    RawReopenFunc func;
+    bool need_lock_fd;
+} reopen_functions[] = {
+    {BDRV_LOCKF_UNLOCK, BDRV_LOCKF_UNLOCK, raw_reopen_identical, false},
+    {BDRV_LOCKF_UNLOCK, BDRV_LOCKF_SHARED, raw_reopen_from_unlock, true},
+    {BDRV_LOCKF_UNLOCK, BDRV_LOCKF_EXCLUSIVE, raw_reopen_from_unlock, true},
+    {BDRV_LOCKF_SHARED, BDRV_LOCKF_UNLOCK, raw_reopen_to_unlock, false},
+    {BDRV_LOCKF_SHARED, BDRV_LOCKF_SHARED, raw_reopen_identical, false},
+    {BDRV_LOCKF_SHARED, BDRV_LOCKF_EXCLUSIVE, raw_reopen_upgrade, true},
+    {BDRV_LOCKF_EXCLUSIVE, BDRV_LOCKF_UNLOCK, raw_reopen_to_unlock, false},
+    {BDRV_LOCKF_EXCLUSIVE, BDRV_LOCKF_SHARED, raw_reopen_downgrade, true},
+    {BDRV_LOCKF_EXCLUSIVE, BDRV_LOCKF_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;
+    BdrvLockfCmd old_lock, new_lock;
+    const struct RawReopenFuncRecord *rec;
+    int ret;
+
+    old_lock = bdrv_get_locking_cmd(bdrv_get_flags(state->bs));
+    new_lock = bdrv_get_locking_cmd(state->flags);
+
+    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)
 {
@@ -683,6 +952,10 @@ static int raw_reopen_prepare(BDRVReopenState *state,
         }
     }
 
+    if (!ret) {
+        ret = raw_reopen_handle_lock(state, RAW_REOPEN_PREPARE, errp);
+    }
+
     return ret;
 }
 
@@ -693,6 +966,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;
 #ifdef CONFIG_LINUX_AIO
@@ -713,6 +988,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;
@@ -1385,6 +1662,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)
@@ -1942,6 +2223,8 @@ BlockDriver bdrv_file = {
     .bdrv_detach_aio_context = raw_detach_aio_context,
     .bdrv_attach_aio_context = raw_attach_aio_context,
 
+    .bdrv_lockf = raw_lockf,
+
     .create_opts = &raw_create_opts,
 };
 
@@ -2396,6 +2679,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.8.2

[Qemu-devel] [PATCH v6 09/22] 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 d977a6e..7a5e824 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -107,6 +107,7 @@ static void open_help(void)
 " -r, -- open file read-only\n"
 " -s, -- use snapshot file\n"
 " -n, -- disable host cache, 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"
@@ -123,7 +124,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,
 };
@@ -142,12 +143,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;
@@ -176,6 +178,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");
@@ -197,6 +202,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);
@@ -433,13 +442,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' },
@@ -496,6 +507,9 @@ int main(int argc, char **argv)
         case 'r':
             readonly = 1;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case 'm':
             qemuio_misalign = true;
             break;
@@ -576,6 +590,10 @@ int main(int argc, char **argv)
         flags |= BDRV_O_RDWR;
     }
 
+    if (nolock) {
+        flags |= BDRV_O_NO_LOCK;
+    }
+
     if ((argc - optind) == 1) {
         if (imageOpts) {
             QemuOpts *qopts = NULL;
-- 
2.8.2

[Qemu-devel] [PATCH v6 10/22] 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 4b56ad3..88a12b5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -598,6 +598,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;
@@ -614,7 +615,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;
@@ -648,6 +649,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,
@@ -681,6 +685,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);
@@ -801,6 +806,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;
@@ -812,7 +818,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;
@@ -842,6 +848,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,
@@ -873,6 +882,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);
@@ -1131,6 +1141,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 (;;) {
@@ -1140,7 +1151,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;
@@ -1168,6 +1179,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,
@@ -1205,7 +1219,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);
@@ -1754,6 +1768,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";
@@ -1769,7 +1784,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;
@@ -1858,6 +1873,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;
@@ -1906,7 +1924,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);
@@ -2056,6 +2074,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);
@@ -2236,12 +2255,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);
 
@@ -2258,8 +2279,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;
         }
@@ -2311,6 +2333,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;
@@ -2325,7 +2348,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;
@@ -2338,6 +2361,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;
@@ -2377,7 +2403,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;
     }
@@ -2523,6 +2549,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;
@@ -2536,7 +2564,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;
@@ -2549,6 +2577,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;
@@ -2585,7 +2616,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;
     }
@@ -2648,6 +2680,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 */
@@ -2658,7 +2691,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;
@@ -2703,6 +2736,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,
@@ -2728,6 +2764,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) {
@@ -2797,6 +2834,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;
@@ -2811,7 +2849,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;
@@ -2842,6 +2880,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;
@@ -2881,6 +2922,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);
@@ -3142,6 +3184,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",
@@ -3176,7 +3220,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;
@@ -3192,6 +3236,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,
@@ -3243,8 +3290,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;
@@ -3305,6 +3353,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 (;;) {
@@ -3314,7 +3363,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;
@@ -3351,6 +3400,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);
@@ -3396,6 +3448,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.8.2

[Qemu-devel] [PATCH v6 11/22] 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 e7cded6..fa87942 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -10,68 +10,68 @@ 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("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 88a12b5..76dd694 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -130,6 +130,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 afaebdd..b91f2e1 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -77,6 +77,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.8.2

[Qemu-devel] [PATCH v6 12/22] 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 6554f0a..6adfc46 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -98,6 +98,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"
@@ -470,7 +471,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:";
+    const char *sopt = "hVb:o:p:rsnLP:c:dvk:e:f:tl:x:";
     struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -479,6 +480,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' },
@@ -631,6 +633,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 9f23343..6b7b1d1 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.8.2

[Qemu-devel] [PATCH v6 13/22] 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 5acb286..fa8b50c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3256,6 +3256,11 @@ static void do_drive_backup(const char *device, const char *target,
         }
     }
     if (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.8.2

[Qemu-devel] [PATCH v6 14/22] mirror: Disable image locking on target backing chain

[Fam Zheng]

In sync=none the backing image of s->target is s->common.bs, which could
be exclusively locked, the image locking wouldn't work here.

Later we can update completion code to lock it after the replaced node
has dropped its lock.

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

diff --git a/blockdev.c b/blockdev.c
index fa8b50c..4a9fa7c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3620,8 +3620,14 @@ void qmp_drive_mirror(const char *device, const char *target,
     /* Mirroring takes care of copy-on-write using the source's backing
      * file.
      */
-    target_bs = bdrv_open(target, NULL, options, flags | BDRV_O_NO_BACKING,
-                          errp);
+    flags |= BDRV_O_NO_BACKING;
+    if (sync == MIRROR_SYNC_MODE_NONE) {
+        /* TODO: in mirror complete, after target_bs is switched to and the
+         * original BDS's lock is dropped, we should enable the lock on
+         * target_bs.  */
+        flags |= BDRV_O_NO_LOCK;
+    }
+    target_bs = bdrv_open(target, NULL, options, flags, errp);
     if (!target_bs) {
         goto out;
     }
-- 
2.8.2

[Qemu-devel] [PATCH v6 15/22] 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.8.2

[Qemu-devel] [PATCH v6 16/22] qemu-iotests: Wait for QEMU processes before checking image in 091

[Fam Zheng]

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

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

diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
index 32bbd56..32aa5c3 100755
--- a/tests/qemu-iotests/091
+++ b/tests/qemu-iotests/091
@@ -95,6 +95,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.8.2

[Qemu-devel] [PATCH v6 17/22] 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 3ac2443..fa996ef 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -95,7 +95,7 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         # The image map is empty before the operation
-        empty_map = qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img)
+        empty_map = qemu_io('-L', '-f', iotests.imgfmt, '-c', 'map', test_img)
 
         # This is a no-op: no data should ever be copied from the base image
         result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
-- 
2.8.2

[Qemu-devel] [PATCH v6 18/22] 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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index e7bca37..b640cf6 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -85,6 +85,7 @@ run_qemu <<EOF
         "driver": "$IMGFMT",
         "id": "disk",
         "node-name": "test-node",
+        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
@@ -97,6 +98,7 @@ run_qemu <<EOF
       "options": {
         "driver": "$IMGFMT",
         "id": "disk",
+        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
@@ -109,6 +111,7 @@ run_qemu <<EOF
       "options": {
         "driver": "$IMGFMT",
         "id": "test-node",
+        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
@@ -122,6 +125,7 @@ run_qemu <<EOF
         "driver": "$IMGFMT",
         "id": "disk2",
         "node-name": "disk",
+        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
@@ -135,6 +139,7 @@ run_qemu <<EOF
         "driver": "$IMGFMT",
         "id": "disk2",
         "node-name": "test-node",
+        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
@@ -148,6 +153,7 @@ run_qemu <<EOF
         "driver": "$IMGFMT",
         "id": "disk3",
         "node-name": "disk3",
+        "lock-mode": "off",
         "file": {
             "driver": "file",
             "filename": "$TEST_IMG"
-- 
2.8.2

[Qemu-devel] [PATCH v6 19/22] 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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index aa77eca..6109981 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,7 @@ 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=virtio,lock-mode=off -drive file="${TEST_IMG}.2",if=virtio,lock-mode=off
 h=$QEMU_HANDLE
 
 echo
-- 
2.8.2

[Qemu-devel] [PATCH v6 20/22] 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 fe03236..b9a6036 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -93,7 +93,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 ec06893..60734e8 100644
--- a/tests/nvme-test.c
+++ b/tests/nvme-test.c
@@ -23,7 +23,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 71ff2ea..4a2f3ad 100644
--- a/tests/usb-hcd-uhci-test.c
+++ b/tests/usb-hcd-uhci-test.c
@@ -86,7 +86,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 7e2e212..5e96bec 100644
--- a/tests/usb-hcd-xhci-test.c
+++ b/tests/usb-hcd-xhci-test.c
@@ -90,7 +90,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 3a66630..7c21938 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -81,7 +81,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 d78747a..74f85a8 100644
--- a/tests/virtio-scsi-test.c
+++ b/tests/virtio-scsi-test.c
@@ -57,7 +57,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 ? : "");
@@ -208,7 +208,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.8.2

[Qemu-devel] [PATCH v6 21/22] block: Turn on image locking by default

[Fam Zheng]

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

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

diff --git a/blockdev.c b/blockdev.c
index 4a9fa7c..7e70e02 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -384,7 +384,7 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
             }
         }
 
-        lock_mode = qemu_opt_get(opts, "lock-mode") ? : "off";
+        lock_mode = qemu_opt_get(opts, "lock-mode") ? : "exclusive";
         if (!strcmp(lock_mode, "exclusive")) {
             /* Default */
         } else if (!strcmp(lock_mode, "shared")) {
-- 
2.8.2

[Qemu-devel] [PATCH v6 22/22] 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..cbeda8e
--- /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=exclusive" "lock-mode=shared" \
+                 "lock-mode=off" "lock-mode=exclusive,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,exclusive,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..991f84c
--- /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=exclusive ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=exclusive: 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=exclusive,readonly=on ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=exclusive,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=exclusive ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=exclusive: Failed to lock image
+
+== Launching another QEMU lock-mode=shared ==
+
+== Launching another QEMU lock-mode=off ==
+
+== Launching another QEMU lock-mode=exclusive,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=exclusive ==
+
+== Launching another QEMU lock-mode=shared ==
+
+== Launching another QEMU lock-mode=off ==
+
+== Launching another QEMU lock-mode=exclusive,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=exclusive ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-mode=exclusive: Failed to lock image
+
+== Launching another QEMU lock-mode=shared ==
+
+== Launching another QEMU lock-mode=off ==
+
+== Launching another QEMU lock-mode=exclusive,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=exclusive,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=on - lock-mode=exclusive,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=exclusive,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=exclusive,readonly=on) ==
+
+== Two devices with the same image (lock-mode=shared,readonly=off - lock-mode=exclusive,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=exclusive,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=exclusive,readonly=on - lock-mode=shared,readonly=on) ==
+
+== Two devices with the same image (lock-mode=exclusive,readonly=on - lock-mode=shared,readonly=off) ==
+
+== Two devices with the same image (lock-mode=exclusive,readonly=on - lock-mode=exclusive,readonly=on) ==
+
+== Two devices with the same image (lock-mode=exclusive,readonly=on - lock-mode=exclusive,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=exclusive,readonly=off: Failed to lock image
+
+== Two devices with the same image (lock-mode=exclusive,readonly=on - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=exclusive,readonly=on - lock-mode=off,readonly=off) ==
+
+== Two devices with the same image (lock-mode=exclusive,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=exclusive,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=exclusive,readonly=off - lock-mode=exclusive,readonly=on) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=exclusive,readonly=on: Failed to lock image
+
+== Two devices with the same image (lock-mode=exclusive,readonly=off - lock-mode=exclusive,readonly=off) ==
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,lock-mode=exclusive,readonly=off: Failed to lock image
+
+== Two devices with the same image (lock-mode=exclusive,readonly=off - lock-mode=off,readonly=on) ==
+
+== Two devices with the same image (lock-mode=exclusive,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=exclusive,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=on - lock-mode=exclusive,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=exclusive,readonly=on) ==
+
+== Two devices with the same image (lock-mode=off,readonly=off - lock-mode=exclusive,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 ab1d76e..2b35ce5 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -153,4 +153,5 @@
 149 rw auto sudo
 150 rw auto quick
 152 rw auto quick
+153 rw auto quick
 154 rw auto backing quick
-- 
2.8.2

Re: [Qemu-devel] [PATCH v6 08/22] raw-posix: Add image locking support

[Fam Zheng]

On Fri, 06/03 16:49, Fam Zheng wrote:
> +static
> +int raw_reopen_downgrade(BDRVReopenState *state,
> +                         RawReopenOperation op,
> +                         BdrvLockfCmd old_lock,
> +                         BdrvLockfCmd new_lock,
> +                         Error **errp)
> +{
> +    BDRVRawReopenState *raw_s = state->opaque;
> +    BDRVRawState *s = state->bs->opaque;
> +    int ret;

This should be initialized to 0 for the nop branches.

Fam

> +
> +    assert(old_lock == BDRV_LOCKF_EXCLUSIVE);
> +    assert(new_lock == BDRV_LOCKF_SHARED);
> +    switch (op) {
> +    case RAW_REOPEN_PREPARE:
> +        break;
> +    case RAW_REOPEN_COMMIT:
> +        ret = raw_lockf_fd(s->lock_fd, BDRV_LOCKF_SHARED);
> +        if (ret) {
> +            error_report("Failed to downgrade old lock");
> +            break;
> +        }
> +        ret = raw_lockf_fd(raw_s->lock_fd, BDRV_LOCKF_SHARED);
> +        if (ret) {
> +            error_report("Failed to lock new fd");
> +            break;
> +        }
> +        break;
> +    case RAW_REOPEN_ABORT:
> +        break;
> +    }
> +
> +    return ret;
> +}

Re: [Qemu-devel] [PATCH v6 14/22] mirror: Disable image locking on target backing chain

[Max Reitz]

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

On 03.06.2016 10:49, Fam Zheng wrote:
> In sync=none the backing image of s->target is s->common.bs, which could
> be exclusively locked, the image locking wouldn't work here.
> 
> Later we can update completion code to lock it after the replaced node
> has dropped its lock.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

Without having reviewed the patch in the new context, when basing your
series on v2 of my "block/mirror: Fix target backing BDS​", all iotests
pass even without this patch.

Without my series, test 041 fails.

So I have reason to hope that I was actually able to make this patch
superfluous.

Max


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

Re: [Qemu-devel] [PATCH v6 04/22] block: Introduce image file locking

[Jason Dillaman]

On Fri, Jun 3, 2016 at 4:48 AM, Fam Zheng <famz@redhat.com> wrote:
> +typedef enum {
> +    /* The values are ordered so that lower number implies higher restriction.
> +     * Starting from 1 to make 0 an invalid value.
> +     * */
> +    BDRV_LOCKF_EXCLUSIVE = 1,
> +    BDRV_LOCKF_SHARED,
> +    BDRV_LOCKF_UNLOCK,
> +} BdrvLockfCmd;
> +

We started to talk about new APIs in librbd to support this feature
where we don't need to worry about admin action should QEMU crash
while holding the lock.

Any chance for separating the UNLOCK enum into the exclusive vs shared
case? We could do some magic in the rbd block driver to guess how it
was locked but it seems like it would be cleaner (at least for us) to
explicitly call out what type of unlock you are requesting since it
will involve different API methods.

-- 
Jason

Re: [Qemu-devel] [PATCH v6 04/22] block: Introduce image file locking

[Fam Zheng]

On Tue, 06/07 21:51, Jason Dillaman wrote:
> On Fri, Jun 3, 2016 at 4:48 AM, Fam Zheng <famz@redhat.com> wrote:
> > +typedef enum {
> > +    /* The values are ordered so that lower number implies higher restriction.
> > +     * Starting from 1 to make 0 an invalid value.
> > +     * */
> > +    BDRV_LOCKF_EXCLUSIVE = 1,
> > +    BDRV_LOCKF_SHARED,
> > +    BDRV_LOCKF_UNLOCK,
> > +} BdrvLockfCmd;
> > +
> 
> We started to talk about new APIs in librbd to support this feature
> where we don't need to worry about admin action should QEMU crash
> while holding the lock.
> 
> Any chance for separating the UNLOCK enum into the exclusive vs shared
> case? We could do some magic in the rbd block driver to guess how it
> was locked but it seems like it would be cleaner (at least for us) to
> explicitly call out what type of unlock you are requesting since it
> will involve different API methods.

This should be possible but I'm not sure I fully understand the rationale
behind it. The server side who implements the lock and keeps track of states
should have the lock type information already, why is it necessary for the
client to be explicit? It doesn't sound necessary to me at all from an
interface point of view. Can you elaborate more on the API methods that need
this?

Fam

Re: [Qemu-devel] [PATCH v6 14/22] mirror: Disable image locking on target backing chain

[Fam Zheng]

On Mon, 06/06 17:03, Max Reitz wrote:
> On 03.06.2016 10:49, Fam Zheng wrote:
> > In sync=none the backing image of s->target is s->common.bs, which could
> > be exclusively locked, the image locking wouldn't work here.
> > 
> > Later we can update completion code to lock it after the replaced node
> > has dropped its lock.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  blockdev.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> Without having reviewed the patch in the new context, when basing your
> series on v2 of my "block/mirror: Fix target backing BDS​", all iotests
> pass even without this patch.
> 
> Without my series, test 041 fails.
> 
> So I have reason to hope that I was actually able to make this patch
> superfluous.

Great, then I can drop this patch from v7 later.

Fam

Re: [Qemu-devel] [PATCH v6 01/22] block: Add flag bits for image locking

[Kevin Wolf]

Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> 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>

Worth asserting in bdrv_open_common() that BDRV_O_NO_LOCK and
BDRV_O_SHARED_LOCK aren't passed at the same time?

Kevin

>  include/block/block.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 70ea299..02b598b 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -94,6 +94,8 @@ 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_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
>  
> -- 
> 2.8.2
> 

Re: [Qemu-devel] [PATCH v6 02/22] qapi: Add lock-mode in blockdev-add options

[Kevin Wolf]

Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> To allow overriding the default locking behavior when opening the image.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qapi/block-core.json | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 98a20d2..23ec31d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2032,6 +2032,20 @@
>              '*read-pattern': 'QuorumReadPattern' } }
>  
>  ##
> +# @BlockdevLockMode
> +#
> +# Describes how QEMU should lock the image.
> +#
> +# @off:       Disabled
> +# @shared:    Use shared lock for both RO and RW images.
> +# @exclusive: Use exclusive lock for RW images, and shared lock for RO images.

This feels odd. If I request 'exclusive', I want to have exclusive.
Reasons may include that I anticipate reopening the image r/w later for
a commit operation and don't want to have this blocked by other readers.

I see where you're coming from, though, because this might not be a good
default. Perhaps we need to have both then, an 'exclusive' option that
does what it promises and a 'default' option that infers the wanted
locking mode from the writability of the image.

Kevin

> +#
> +# Since: 2.7
> +##
> +{ 'enum': 'BlockdevLockMode',
> +  'data': [ 'off', 'shared', 'exclusive' ] }
> +
> +##
>  # @BlockdevOptions
>  #
>  # Options for creating a block device.  Many options are available for all
> @@ -2065,6 +2079,8 @@
>  # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
>  #                 (default: off)
>  #
> +# @lock-mode: #optional how to lock the image. (default: exclusive) (Since 2.7)
> +#
>  # Remaining options are determined by the block driver.
>  #
>  # Since: 1.7
> @@ -2082,7 +2098,8 @@
>              '*stats-account-invalid': 'bool',
>              '*stats-account-failed': 'bool',
>              '*stats-intervals': ['int'],
> -            '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
> +            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
> +            '*lock-mode': 'BlockdevLockMode' },
>    'discriminator': 'driver',
>    'data': {
>        'archipelago':'BlockdevOptionsArchipelago',
> -- 
> 2.8.2
> 

Re: [Qemu-devel] [PATCH v6 03/22] blockdev: Add and parse "lock-mode" option for image locking

[Kevin Wolf]

Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> Respect the locking mode from CLI or QMP, and set the open flags
> accordingly.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c      | 23 +++++++++++++++++++++++
>  qemu-options.hx |  1 +
>  2 files changed, 24 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 717785e..5acb286 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -356,6 +356,7 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
>      const char *discard;
>      Error *local_error = NULL;
>      const char *aio;
> +    const char *lock_mode;
>  
>      if (bdrv_flags) {
>          if (!qemu_opt_get_bool(opts, "read-only", false)) {
> @@ -382,6 +383,18 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
>                 return;
>              }
>          }
> +
> +        lock_mode = qemu_opt_get(opts, "lock-mode") ? : "off";
> +        if (!strcmp(lock_mode, "exclusive")) {
> +            /* Default */
> +        } else if (!strcmp(lock_mode, "shared")) {
> +            *bdrv_flags |= BDRV_O_SHARED_LOCK;
> +        } else if (!strcmp(lock_mode, "off")) {
> +            *bdrv_flags |= BDRV_O_NO_LOCK;
> +        } else {
> +           error_setg(errp, "invalid lock mode");
> +           return;
> +        }
>      }
>  
>      /* disk I/O throttling */
> @@ -4296,6 +4309,11 @@ QemuOptsList qemu_common_drive_opts = {
>              .type = QEMU_OPT_BOOL,
>              .help = "whether to account for failed I/O operations "
>                      "in the statistics",
> +        },{
> +            .name = "lock-mode",
> +            .type = QEMU_OPT_STRING,
> +            .help = "how to lock the image (exclusive, shared, off. "
> +                    "default: exclusive)",
>          },
>          { /* end of list */ }
>      },
> @@ -4325,6 +4343,11 @@ static QemuOptsList qemu_root_bds_opts = {
>              .name = "detect-zeroes",
>              .type = QEMU_OPT_STRING,
>              .help = "try to optimize zero writes (off, on, unmap)",
> +        },{
> +            .name = "lock-mode",
> +            .type = QEMU_OPT_STRING,
> +            .help = "how to lock the image (exclusive, shared, off. "
> +                    "default: exclusive)",
>          },
>          { /* end of list */ }
>      },

This is the wrong level to implement the feature. You would only be able
to configure the locking on the top level image this way (because what
we're doing here is BB, not BDS configuration). If you want to allow
configuration per node, you need to put the options into
bdrv_runtime_opts and interpret them in bdrv_open_common().

Otherwise we would have to specify in the blockdev-add documentation
that this works only on the top level, but I don't see a reason for
such a restriction.

Kevin

Re: [Qemu-devel] [PATCH v6 04/22] block: Introduce image file locking

[Kevin Wolf]

Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> 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                   | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h     | 11 ++++++++-
>  include/block/block_int.h |  5 +++++
>  3 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 736432f..4c2a3ff 100644
> --- a/block.c
> +++ b/block.c
> @@ -854,6 +854,50 @@ out:
>      g_free(gen_node_name);
>  }
>  
> +BdrvLockfCmd bdrv_get_locking_cmd(int flags)
> +{
> +    if (flags & BDRV_O_NO_LOCK) {
> +        return BDRV_LOCKF_UNLOCK;
> +    } else if (flags & BDRV_O_SHARED_LOCK) {
> +        return BDRV_LOCKF_SHARED;
> +    } else if (flags & BDRV_O_RDWR) {
> +        return BDRV_LOCKF_EXCLUSIVE;
> +    } else {
> +        return BDRV_LOCKF_SHARED;
> +    }
> +}

It feels a bit counterintuitive to use the very same operation for
"start of critical section, but don't actually lock" and "end of
critical section".

Is there a specific reason why you chose this instead of separate
.bdrv_lock/bdrv_unlock callbacks?

> +static int bdrv_lock_unlock_image_do(BlockDriverState *bs, BdrvLockfCmd cmd)
> +{
> +    int ret;
> +
> +    if (bs->cur_lock == cmd) {
> +        return 0;
> +    } else if (!bs->drv) {
> +        return -ENOMEDIUM;
> +    } else if (!bs->drv->bdrv_lockf) {
> +        return 0;
> +    }
> +    ret = bs->drv->bdrv_lockf(bs, cmd);
> +    if (ret == -ENOTSUP) {
> +        /* Handle it the same way as !bs->drv->bdrv_lockf */
> +        ret = 0;
> +    } else if (ret == 0) {
> +        bs->cur_lock = cmd;
> +    }
> +    return ret;
> +}

Okay, so the callback is supposed to support going from exclusive to
shared and vice versa? Noted for the next patches.

> +static int bdrv_lock_image(BlockDriverState *bs)
> +{
> +    return bdrv_lock_unlock_image_do(bs, bdrv_get_locking_cmd(bs->open_flags));
> +}
> +
> +static int bdrv_unlock_image(BlockDriverState *bs)
> +{
> +    return bdrv_lock_unlock_image_do(bs, BDRV_LOCKF_UNLOCK);
> +}
> +
>  static QemuOptsList bdrv_runtime_opts = {
>      .name = "bdrv_common",
>      .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
> @@ -1003,6 +1047,14 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
>          goto free_and_fail;
>      }
>  
> +    if (!(open_flags & (BDRV_O_NO_LOCK | BDRV_O_INACTIVE))) {
> +        ret = bdrv_lock_image(bs);
> +        if (ret) {
> +            error_setg(errp, "Failed to lock image");
> +            goto free_and_fail;
> +        }
> +    }
> +
>      ret = refresh_total_sectors(bs, bs->total_sectors);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Could not refresh total sector count");
> @@ -2164,6 +2216,7 @@ static void bdrv_close(BlockDriverState *bs)
>      if (bs->drv) {
>          BdrvChild *child, *next;
>  
> +        bdrv_unlock_image(bs);
>          bs->drv->bdrv_close(bs);
>          bs->drv = NULL;
>  
> @@ -3187,6 +3240,9 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
>          error_setg_errno(errp, -ret, "Could not refresh total sector count");
>          return;
>      }
> +    if (!(bs->open_flags & BDRV_O_NO_LOCK)) {
> +        bdrv_lock_image(bs);
> +    }
>  }

I think the if is unnecessary, bdrv_lock_image() already looks at
BDRV_O_NO_LOCK.

>  void bdrv_invalidate_cache_all(Error **errp)
> @@ -3229,6 +3285,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
>      }
>  
>      if (setting_flag) {
> +        ret = bdrv_unlock_image(bs);
>          bs->open_flags |= BDRV_O_INACTIVE;
>      }
>      return 0;

Kevin

Re: [Qemu-devel] [PATCH v6 05/22] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Kevin Wolf]

Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> 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 6937694..749214a 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -280,6 +280,8 @@ int qemu_madvise(void *addr, size_t len, int advice);
>  
>  int qemu_open(const char *name, int flags, ...);
>  int qemu_close(int fd);
> +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 9a7a439..085ed52 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -137,6 +137,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
> +}

This will return -ENOTSUP in the case that the function wasn't available
at build time, but -EINVAL if it was available at build time but the
kernel doesn't support it at runtime. Should we unify this?

Kevin

Re: [Qemu-devel] [PATCH v6 06/22] osdep: Introduce qemu_dup

[Kevin Wolf]

Am 03.06.2016 um 10:49 hat Fam Zheng geschrieben:
> This takes care of both the CLOEXEC flag and fd-path mapping for image
> locking.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  include/qemu/osdep.h | 3 +++
>  util/osdep.c         | 9 +++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 749214a..89c63c7 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -281,6 +281,9 @@ int qemu_madvise(void *addr, size_t len, int advice);
>  int qemu_open(const char *name, int flags, ...);
>  int qemu_close(int fd);
>  int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> +#ifndef _WIN32
> +int qemu_dup(int fd);
> +#endif
>  int qemu_unlock_fd(int fd, int64_t start, int64_t len);
>  
>  #if defined(__HAIKU__) && defined(__i386__)
> diff --git a/util/osdep.c b/util/osdep.c
> index 085ed52..1c87c1e 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -133,6 +133,15 @@ fail:
>      return -1;
>  }
>  
> +int qemu_dup(int fd)
> +{
> +    int r = qemu_dup_flags(fd, 0);

This clears all file status flags that might be set. I don't think we
use any of them (on Linux at least, raw-posix still seems to use it for
platform without O_DIRECT), but isn't this still surprising?

> +    if (r == -1) {
> +        return -errno;
> +    }
> +    return r;
> +}

Kevin

Re: [Qemu-devel] [PATCH v6 08/22] raw-posix: Add image locking support

[Kevin Wolf]

Am 03.06.2016 um 10:49 hat Fam Zheng geschrieben:
> 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".
> 
> There have three different states, "unlocked", "shared locked" and "exclusively
> locked".

This seems to be a corrupted copy of the previous sentence. :-)

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

I must admit that I don't fully understand yet why we can't change the
lock atomincally and how s->lock_fd helps. In any case, I think it
deserves comments in the code and not only in the commit message.

So I'm not giving a full review here, but I think I have one important
point to make at least.

>  block/raw-posix.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 285 insertions(+)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index bb8669f..6347350 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;
> @@ -153,6 +154,7 @@ typedef struct BDRVRawState {
>  
>  typedef struct BDRVRawReopenState {
>      int fd;
> +    int lock_fd;
>      int open_flags;
>  #ifdef CONFIG_LINUX_AIO
>      int use_aio;
> @@ -397,6 +399,37 @@ static void raw_attach_aio_context(BlockDriverState *bs,
>  #endif
>  }
>  
> +static int raw_lockf_fd(int fd, BdrvLockfCmd cmd)
> +{
> +    assert(fd >= 0);
> +    /* Locking byte 1 avoids interfereing with virtlockd. */
> +    switch (cmd) {
> +    case BDRV_LOCKF_EXCLUSIVE:
> +        return qemu_lock_fd(fd, 1, 1, true);
> +    case BDRV_LOCKF_SHARED:
> +        return qemu_lock_fd(fd, 1, 1, false);
> +    case BDRV_LOCKF_UNLOCK:
> +        return qemu_unlock_fd(fd, 1, 1);
> +    default:
> +        abort();
> +    }
> +}
> +
> +static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> +{
> +
> +    BDRVRawState *s = bs->opaque;
> +
> +    if (s->lock_fd < 0) {
> +        s->lock_fd = qemu_dup(s->fd);
> +        if (s->lock_fd < 0) {
> +            return s->lock_fd;
> +        }
> +    }
> +
> +    return raw_lockf_fd(s->lock_fd, cmd);
> +}
> +
>  #ifdef CONFIG_LINUX_AIO
>  static int raw_set_aio(LinuxAioState **aio_ctx, int *use_aio, int bdrv_flags)
>  {
> @@ -483,6 +516,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;
> @@ -593,6 +627,241 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
>      return ret;
>  }
>  
> +typedef enum {
> +    RAW_REOPEN_PREPARE,
> +    RAW_REOPEN_COMMIT,
> +    RAW_REOPEN_ABORT
> +} RawReopenOperation;
> +
> +typedef int (*RawReopenFunc)(BDRVReopenState *state,
> +                             RawReopenOperation op,
> +                             BdrvLockfCmd old_lock,
> +                             BdrvLockfCmd new_lock,
> +                             Error **errp);
> +
> +static
> +int raw_reopen_identical(BDRVReopenState *state,

This is unusual formatting. I'm used to having everything on a single
line or "static int" on its own line, but breaking between "static" and
"int" feels odd.

> +                         RawReopenOperation op,
> +                         BdrvLockfCmd old_lock,
> +                         BdrvLockfCmd new_lock,
> +                         Error **errp)
> +{
> +    assert(old_lock == new_lock);
> +    return 0;
> +}
> +
> +static
> +int raw_reopen_from_unlock(BDRVReopenState *state,
> +                           RawReopenOperation op,
> +                           BdrvLockfCmd old_lock,
> +                           BdrvLockfCmd new_lock,
> +                           Error **errp)
> +{
> +    BDRVRawReopenState *raw_s = state->opaque;
> +    int ret = 0;
> +
> +    assert(old_lock != new_lock);
> +    assert(old_lock == BDRV_LOCKF_UNLOCK);
> +    switch (op) {
> +    case RAW_REOPEN_PREPARE:
> +        ret = raw_lockf_fd(raw_s->lock_fd, new_lock);
> +        if (ret) {
> +            error_setg_errno(errp, -ret, "Failed to lock new fd");
> +        }
> +        break;
> +    case RAW_REOPEN_COMMIT:
> +    case RAW_REOPEN_ABORT:
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static
> +int raw_reopen_to_unlock(BDRVReopenState *state,
> +                         RawReopenOperation op,
> +                         BdrvLockfCmd old_lock,
> +                         BdrvLockfCmd new_lock,
> +                         Error **errp)
> +{
> +    BDRVRawState *s = state->bs->opaque;
> +    int ret = 0;
> +
> +    assert(old_lock != new_lock);
> +    assert(old_lock == BDRV_LOCKF_UNLOCK);
> +    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,
> +                       BdrvLockfCmd old_lock,
> +                       BdrvLockfCmd new_lock,
> +                       Error **errp)
> +{
> +    BDRVRawReopenState *raw_s = state->opaque;
> +    BDRVRawState *s = state->bs->opaque;
> +    int ret = 0, ret2;
> +
> +    assert(old_lock == BDRV_LOCKF_SHARED);
> +    assert(new_lock == BDRV_LOCKF_EXCLUSIVE);
> +    switch (op) {
> +    case RAW_REOPEN_PREPARE:
> +        ret = raw_lockf_fd(raw_s->lock_fd, BDRV_LOCKF_SHARED);
> +        if (ret) {
> +            error_setg_errno(errp, -ret, "Failed to lock new fd (shared)");
> +            break;
> +        }
> +        ret = raw_lockf_fd(s->lock_fd, BDRV_LOCKF_UNLOCK);
> +        if (ret) {
> +            error_setg_errno(errp, -ret, "Failed to unlock old fd");
> +            goto restore;
> +        }
> +        ret = raw_lockf_fd(raw_s->lock_fd, BDRV_LOCKF_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_lockf_fd(raw_s->lock_fd, BDRV_LOCKF_SHARED);
> +        ret = raw_lockf_fd(s->lock_fd, BDRV_LOCKF_SHARED);
> +        if (ret) {
> +            error_report("Failed to restore lock on old fd");
> +        }
> +        break;
> +    }
> +
> +    return ret;
> +restore:
> +    ret2 = raw_lockf_fd(s->lock_fd, BDRV_LOCKF_SHARED);
> +    if (ret2) {
> +        error_report("Failed to restore old lock");
> +    }
> +    return ret;
> +
> +}

That final empty line doesn't look intentional.

> +
> +static
> +int raw_reopen_downgrade(BDRVReopenState *state,
> +                         RawReopenOperation op,
> +                         BdrvLockfCmd old_lock,
> +                         BdrvLockfCmd new_lock,
> +                         Error **errp)
> +{
> +    BDRVRawReopenState *raw_s = state->opaque;
> +    BDRVRawState *s = state->bs->opaque;
> +    int ret;
> +
> +    assert(old_lock == BDRV_LOCKF_EXCLUSIVE);
> +    assert(new_lock == BDRV_LOCKF_SHARED);
> +    switch (op) {
> +    case RAW_REOPEN_PREPARE:
> +        break;
> +    case RAW_REOPEN_COMMIT:
> +        ret = raw_lockf_fd(s->lock_fd, BDRV_LOCKF_SHARED);
> +        if (ret) {
> +            error_report("Failed to downgrade old lock");
> +            break;
> +        }
> +        ret = raw_lockf_fd(raw_s->lock_fd, BDRV_LOCKF_SHARED);
> +        if (ret) {
> +            error_report("Failed to lock new fd");
> +            break;
> +        }
> +        break;
> +    case RAW_REOPEN_ABORT:
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +static const struct RawReopenFuncRecord {
> +    BdrvLockfCmd old_lock;
> +    BdrvLockfCmd new_lock;
> +    RawReopenFunc func;
> +    bool need_lock_fd;
> +} reopen_functions[] = {
> +    {BDRV_LOCKF_UNLOCK, BDRV_LOCKF_UNLOCK, raw_reopen_identical, false},
> +    {BDRV_LOCKF_UNLOCK, BDRV_LOCKF_SHARED, raw_reopen_from_unlock, true},
> +    {BDRV_LOCKF_UNLOCK, BDRV_LOCKF_EXCLUSIVE, raw_reopen_from_unlock, true},
> +    {BDRV_LOCKF_SHARED, BDRV_LOCKF_UNLOCK, raw_reopen_to_unlock, false},
> +    {BDRV_LOCKF_SHARED, BDRV_LOCKF_SHARED, raw_reopen_identical, false},
> +    {BDRV_LOCKF_SHARED, BDRV_LOCKF_EXCLUSIVE, raw_reopen_upgrade, true},
> +    {BDRV_LOCKF_EXCLUSIVE, BDRV_LOCKF_UNLOCK, raw_reopen_to_unlock, false},
> +    {BDRV_LOCKF_EXCLUSIVE, BDRV_LOCKF_SHARED, raw_reopen_downgrade, true},
> +    {BDRV_LOCKF_EXCLUSIVE, BDRV_LOCKF_EXCLUSIVE, raw_reopen_identical, false},
> +};
> +
> +static int raw_reopen_handle_lock(BDRVReopenState *state,
> +                                  RawReopenOperation op,
> +                                  Error **errp)

I think we have one big problem here: We don't know whether raw_s->fd is
already locked or not. If dup() and setting the new flags with fcntl()
succeeded, it is, but if we had to fall back on qemu_open(), it isn't.

This means that doing nothing in the raw_reopen_identical case isn't
right because reopening without intending to change anything about the
locking could end up unlocking the image.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v6 06/22] osdep: Introduce qemu_dup

[Kevin Wolf]

Am 17.06.2016 um 14:32 hat Kevin Wolf geschrieben:
> Am 03.06.2016 um 10:49 hat Fam Zheng geschrieben:
> > This takes care of both the CLOEXEC flag and fd-path mapping for image
> > locking.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  include/qemu/osdep.h | 3 +++
> >  util/osdep.c         | 9 +++++++++
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 749214a..89c63c7 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -281,6 +281,9 @@ int qemu_madvise(void *addr, size_t len, int advice);
> >  int qemu_open(const char *name, int flags, ...);
> >  int qemu_close(int fd);
> >  int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> > +#ifndef _WIN32
> > +int qemu_dup(int fd);
> > +#endif
> >  int qemu_unlock_fd(int fd, int64_t start, int64_t len);
> >  
> >  #if defined(__HAIKU__) && defined(__i386__)
> > diff --git a/util/osdep.c b/util/osdep.c
> > index 085ed52..1c87c1e 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -133,6 +133,15 @@ fail:
> >      return -1;
> >  }
> >  
> > +int qemu_dup(int fd)
> > +{
> > +    int r = qemu_dup_flags(fd, 0);
> 
> This clears all file status flags that might be set. I don't think we
> use any of them (on Linux at least, raw-posix still seems to use it for
> platform without O_DIRECT), but isn't this still surprising?

Maybe this means that qemu_dup_flags() should call qemu_dup() instead of
the other way round.

Kevin

Re: [Qemu-devel] [PATCH v6 02/22] qapi: Add lock-mode in blockdev-add options

[Fam Zheng]

On Fri, 06/17 11:17, Kevin Wolf wrote:
> Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > To allow overriding the default locking behavior when opening the image.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  qapi/block-core.json | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 98a20d2..23ec31d 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -2032,6 +2032,20 @@
> >              '*read-pattern': 'QuorumReadPattern' } }
> >  
> >  ##
> > +# @BlockdevLockMode
> > +#
> > +# Describes how QEMU should lock the image.
> > +#
> > +# @off:       Disabled
> > +# @shared:    Use shared lock for both RO and RW images.
> > +# @exclusive: Use exclusive lock for RW images, and shared lock for RO images.
> 
> This feels odd. If I request 'exclusive', I want to have exclusive.
> Reasons may include that I anticipate reopening the image r/w later for
> a commit operation and don't want to have this blocked by other readers.
> 
> I see where you're coming from, though, because this might not be a good
> default. Perhaps we need to have both then, an 'exclusive' option that
> does what it promises and a 'default' option that infers the wanted
> locking mode from the writability of the image.

Fair enough, though I'd call it "auto" instead of "default", what do you think?

Fam

Re: [Qemu-devel] [PATCH v6 02/22] qapi: Add lock-mode in blockdev-add options

[Kevin Wolf]

Am 18.06.2016 um 13:16 hat Fam Zheng geschrieben:
> On Fri, 06/17 11:17, Kevin Wolf wrote:
> > Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > > To allow overriding the default locking behavior when opening the image.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  qapi/block-core.json | 19 ++++++++++++++++++-
> > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > index 98a20d2..23ec31d 100644
> > > --- a/qapi/block-core.json
> > > +++ b/qapi/block-core.json
> > > @@ -2032,6 +2032,20 @@
> > >              '*read-pattern': 'QuorumReadPattern' } }
> > >  
> > >  ##
> > > +# @BlockdevLockMode
> > > +#
> > > +# Describes how QEMU should lock the image.
> > > +#
> > > +# @off:       Disabled
> > > +# @shared:    Use shared lock for both RO and RW images.
> > > +# @exclusive: Use exclusive lock for RW images, and shared lock for RO images.
> > 
> > This feels odd. If I request 'exclusive', I want to have exclusive.
> > Reasons may include that I anticipate reopening the image r/w later for
> > a commit operation and don't want to have this blocked by other readers.
> > 
> > I see where you're coming from, though, because this might not be a good
> > default. Perhaps we need to have both then, an 'exclusive' option that
> > does what it promises and a 'default' option that infers the wanted
> > locking mode from the writability of the image.
> 
> Fair enough, though I'd call it "auto" instead of "default", what do you think?

Agreed, that's a better name.

Kevin

Re: [Qemu-devel] [PATCH v6 04/22] block: Introduce image file locking

[Fam Zheng]

On Fri, 06/17 13:34, Kevin Wolf wrote:
> Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > 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                   | 57 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/block/block.h     | 11 ++++++++-
> >  include/block/block_int.h |  5 +++++
> >  3 files changed, 72 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block.c b/block.c
> > index 736432f..4c2a3ff 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -854,6 +854,50 @@ out:
> >      g_free(gen_node_name);
> >  }
> >  
> > +BdrvLockfCmd bdrv_get_locking_cmd(int flags)
> > +{
> > +    if (flags & BDRV_O_NO_LOCK) {
> > +        return BDRV_LOCKF_UNLOCK;
> > +    } else if (flags & BDRV_O_SHARED_LOCK) {
> > +        return BDRV_LOCKF_SHARED;
> > +    } else if (flags & BDRV_O_RDWR) {
> > +        return BDRV_LOCKF_EXCLUSIVE;
> > +    } else {
> > +        return BDRV_LOCKF_SHARED;
> > +    }
> > +}
> 
> It feels a bit counterintuitive to use the very same operation for
> "start of critical section, but don't actually lock" and "end of
> critical section".
> 
> Is there a specific reason why you chose this instead of separate
> .bdrv_lock/bdrv_unlock callbacks?

Because unlike open(2)/close(2), locking and unlocking are typically
implemented with one parameterized operation (fcntl(2)) underneath, I followed
that naturally.

> 
> > +static int bdrv_lock_unlock_image_do(BlockDriverState *bs, BdrvLockfCmd cmd)
> > +{
> > +    int ret;
> > +
> > +    if (bs->cur_lock == cmd) {
> > +        return 0;
> > +    } else if (!bs->drv) {
> > +        return -ENOMEDIUM;
> > +    } else if (!bs->drv->bdrv_lockf) {
> > +        return 0;
> > +    }
> > +    ret = bs->drv->bdrv_lockf(bs, cmd);
> > +    if (ret == -ENOTSUP) {
> > +        /* Handle it the same way as !bs->drv->bdrv_lockf */
> > +        ret = 0;
> > +    } else if (ret == 0) {
> > +        bs->cur_lock = cmd;
> > +    }
> > +    return ret;
> > +}
> 
> Okay, so the callback is supposed to support going from exclusive to
> shared and vice versa? Noted for the next patches.

Yes.

> 
> > +static int bdrv_lock_image(BlockDriverState *bs)
> > +{
> > +    return bdrv_lock_unlock_image_do(bs, bdrv_get_locking_cmd(bs->open_flags));
> > +}
> > +
> > +static int bdrv_unlock_image(BlockDriverState *bs)
> > +{
> > +    return bdrv_lock_unlock_image_do(bs, BDRV_LOCKF_UNLOCK);
> > +}
> > +
> >  static QemuOptsList bdrv_runtime_opts = {
> >      .name = "bdrv_common",
> >      .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
> > @@ -1003,6 +1047,14 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
> >          goto free_and_fail;
> >      }
> >  
> > +    if (!(open_flags & (BDRV_O_NO_LOCK | BDRV_O_INACTIVE))) {
> > +        ret = bdrv_lock_image(bs);
> > +        if (ret) {
> > +            error_setg(errp, "Failed to lock image");
> > +            goto free_and_fail;
> > +        }
> > +    }
> > +
> >      ret = refresh_total_sectors(bs, bs->total_sectors);
> >      if (ret < 0) {
> >          error_setg_errno(errp, -ret, "Could not refresh total sector count");
> > @@ -2164,6 +2216,7 @@ static void bdrv_close(BlockDriverState *bs)
> >      if (bs->drv) {
> >          BdrvChild *child, *next;
> >  
> > +        bdrv_unlock_image(bs);
> >          bs->drv->bdrv_close(bs);
> >          bs->drv = NULL;
> >  
> > @@ -3187,6 +3240,9 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
> >          error_setg_errno(errp, -ret, "Could not refresh total sector count");
> >          return;
> >      }
> > +    if (!(bs->open_flags & BDRV_O_NO_LOCK)) {
> > +        bdrv_lock_image(bs);
> > +    }
> >  }
> 
> I think the if is unnecessary, bdrv_lock_image() already looks at
> BDRV_O_NO_LOCK.

I intentionally made enum BDRV_O_LOCK_* start from non-zero, so bdrv_lock_image
will call drv->bdrv_lockf even for BDRV_O_NO_LOCK. In the case of
lock-mode=off, I think we should skip that.

> 
> >  void bdrv_invalidate_cache_all(Error **errp)
> > @@ -3229,6 +3285,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
> >      }
> >  
> >      if (setting_flag) {
> > +        ret = bdrv_unlock_image(bs);
> >          bs->open_flags |= BDRV_O_INACTIVE;
> >      }
> >      return 0;
> 
> Kevin

Fam

Re: [Qemu-devel] [PATCH v6 05/22] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Fam Zheng]

On Fri, 06/17 14:12, Kevin Wolf wrote:
> Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > 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 6937694..749214a 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -280,6 +280,8 @@ int qemu_madvise(void *addr, size_t len, int advice);
> >  
> >  int qemu_open(const char *name, int flags, ...);
> >  int qemu_close(int fd);
> > +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 9a7a439..085ed52 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -137,6 +137,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
> > +}
> 
> This will return -ENOTSUP in the case that the function wasn't available
> at build time, but -EINVAL if it was available at build time but the
> kernel doesn't support it at runtime. Should we unify this?

I'm not sure we can reliably distinguish "fcntl cmd not supported" and "fcntl
cmd supported but parameters have invalid value" out of -EINVAL.

Quoting the manual:

> https://www.gnu.org/software/libc/manual/html_node/Open-File-Description-Locks.html
> 
> Macro: int F_OFD_SETL
> 
>     EINVAL
> 
>     Either the lockp argument doesn’t specify valid lock information, the
>     operating system kernel doesn’t support open file description locks, or the
>     file associated with filedes doesn’t support locks.
> 

I'd expect the user to know what he's doing if he is using a kernel that is
much older than the one built QEMU, since this is relatively a very uncommon
thing to do.

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v6 06/22] osdep: Introduce qemu_dup

[Fam Zheng]

On Fri, 06/17 15:08, Kevin Wolf wrote:
> Am 17.06.2016 um 14:32 hat Kevin Wolf geschrieben:
> > Am 03.06.2016 um 10:49 hat Fam Zheng geschrieben:
> > > This takes care of both the CLOEXEC flag and fd-path mapping for image
> > > locking.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  include/qemu/osdep.h | 3 +++
> > >  util/osdep.c         | 9 +++++++++
> > >  2 files changed, 12 insertions(+)
> > > 
> > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > > index 749214a..89c63c7 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -281,6 +281,9 @@ int qemu_madvise(void *addr, size_t len, int advice);
> > >  int qemu_open(const char *name, int flags, ...);
> > >  int qemu_close(int fd);
> > >  int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> > > +#ifndef _WIN32
> > > +int qemu_dup(int fd);
> > > +#endif
> > >  int qemu_unlock_fd(int fd, int64_t start, int64_t len);
> > >  
> > >  #if defined(__HAIKU__) && defined(__i386__)
> > > diff --git a/util/osdep.c b/util/osdep.c
> > > index 085ed52..1c87c1e 100644
> > > --- a/util/osdep.c
> > > +++ b/util/osdep.c
> > > @@ -133,6 +133,15 @@ fail:
> > >      return -1;
> > >  }
> > >  
> > > +int qemu_dup(int fd)
> > > +{
> > > +    int r = qemu_dup_flags(fd, 0);
> > 
> > This clears all file status flags that might be set. I don't think we
> > use any of them (on Linux at least, raw-posix still seems to use it for
> > platform without O_DIRECT), but isn't this still surprising?
> 
> Maybe this means that qemu_dup_flags() should call qemu_dup() instead of
> the other way round.
> 

Yes, I think you are right.

Fam

Re: [Qemu-devel] [PATCH v6 08/22] raw-posix: Add image locking support

[Fam Zheng]

On Fri, 06/17 15:07, Kevin Wolf wrote:
> Am 03.06.2016 um 10:49 hat Fam Zheng geschrieben:
> > 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".
> > 
> > There have three different states, "unlocked", "shared locked" and "exclusively
> > locked".
> 
> This seems to be a corrupted copy of the previous sentence. :-)

Right, fixing.

> 
> > 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>
> 
> I must admit that I don't fully understand yet why we can't change the
> lock atomincally and how s->lock_fd helps. In any case, I think it
> deserves comments in the code and not only in the commit message.

I'll add comments in the code too.

> > +static const struct RawReopenFuncRecord {
> > +    BdrvLockfCmd old_lock;
> > +    BdrvLockfCmd new_lock;
> > +    RawReopenFunc func;
> > +    bool need_lock_fd;
> > +} reopen_functions[] = {
> > +    {BDRV_LOCKF_UNLOCK, BDRV_LOCKF_UNLOCK, raw_reopen_identical, false},
> > +    {BDRV_LOCKF_UNLOCK, BDRV_LOCKF_SHARED, raw_reopen_from_unlock, true},
> > +    {BDRV_LOCKF_UNLOCK, BDRV_LOCKF_EXCLUSIVE, raw_reopen_from_unlock, true},
> > +    {BDRV_LOCKF_SHARED, BDRV_LOCKF_UNLOCK, raw_reopen_to_unlock, false},
> > +    {BDRV_LOCKF_SHARED, BDRV_LOCKF_SHARED, raw_reopen_identical, false},
> > +    {BDRV_LOCKF_SHARED, BDRV_LOCKF_EXCLUSIVE, raw_reopen_upgrade, true},
> > +    {BDRV_LOCKF_EXCLUSIVE, BDRV_LOCKF_UNLOCK, raw_reopen_to_unlock, false},
> > +    {BDRV_LOCKF_EXCLUSIVE, BDRV_LOCKF_SHARED, raw_reopen_downgrade, true},
> > +    {BDRV_LOCKF_EXCLUSIVE, BDRV_LOCKF_EXCLUSIVE, raw_reopen_identical, false},
> > +};
> > +
> > +static int raw_reopen_handle_lock(BDRVReopenState *state,
> > +                                  RawReopenOperation op,
> > +                                  Error **errp)
> 
> I think we have one big problem here: We don't know whether raw_s->fd is
> already locked or not. If dup() and setting the new flags with fcntl()
> succeeded, it is, but if we had to fall back on qemu_open(), it isn't.
> 
> This means that doing nothing in the raw_reopen_identical case isn't
> right because reopening without intending to change anything about the
> locking could end up unlocking the image.
> 

Unless I'm missing something, we don't rely on that, becasue raw_s->fd is never
locked. Instead, raw_s->lock_fd, as a dup() of raw_s->fd, is what we actually
handle in raw_reopen_identical(), and it always has the correct state when
raw_reopen_handle_lock() is called.  (That is also an advantage of introducing
raw_s->lock_fd.)

Fam

Re: [Qemu-devel] [PATCH v6 04/22] block: Introduce image file locking

[Kevin Wolf]

Am 22.06.2016 um 09:23 hat Fam Zheng geschrieben:
> On Fri, 06/17 13:34, Kevin Wolf wrote:
> > Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > > 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                   | 57 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/block/block.h     | 11 ++++++++-
> > >  include/block/block_int.h |  5 +++++
> > >  3 files changed, 72 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/block.c b/block.c
> > > index 736432f..4c2a3ff 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > @@ -854,6 +854,50 @@ out:
> > >      g_free(gen_node_name);
> > >  }
> > >  
> > > +BdrvLockfCmd bdrv_get_locking_cmd(int flags)
> > > +{
> > > +    if (flags & BDRV_O_NO_LOCK) {
> > > +        return BDRV_LOCKF_UNLOCK;
> > > +    } else if (flags & BDRV_O_SHARED_LOCK) {
> > > +        return BDRV_LOCKF_SHARED;
> > > +    } else if (flags & BDRV_O_RDWR) {
> > > +        return BDRV_LOCKF_EXCLUSIVE;
> > > +    } else {
> > > +        return BDRV_LOCKF_SHARED;
> > > +    }
> > > +}
> > 
> > It feels a bit counterintuitive to use the very same operation for
> > "start of critical section, but don't actually lock" and "end of
> > critical section".
> > 
> > Is there a specific reason why you chose this instead of separate
> > .bdrv_lock/bdrv_unlock callbacks?
> 
> Because unlike open(2)/close(2), locking and unlocking are typically
> implemented with one parameterized operation (fcntl(2)) underneath, I followed
> that naturally.

Is it really "typically", or is this an idiosyncrasy of POSIX and we end
up modeling our interface for one particular block driver even though
for others a different interface might be preferable? Did you look at
more interfaces?

Apart from that, we generally make our internal interfaces so that they
are easy to use and code is easy to read rather than directly mapping
POSIX everywhere. For example, our I/O function never do short
reads/writes even though POSIX does that.

> > > @@ -3187,6 +3240,9 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
> > >          error_setg_errno(errp, -ret, "Could not refresh total sector count");
> > >          return;
> > >      }
> > > +    if (!(bs->open_flags & BDRV_O_NO_LOCK)) {
> > > +        bdrv_lock_image(bs);
> > > +    }
> > >  }
> > 
> > I think the if is unnecessary, bdrv_lock_image() already looks at
> > BDRV_O_NO_LOCK.
> 
> I intentionally made enum BDRV_O_LOCK_* start from non-zero, so bdrv_lock_image
> will call drv->bdrv_lockf even for BDRV_O_NO_LOCK. In the case of
> lock-mode=off, I think we should skip that.

Fair enough.

Kevin

Re: [Qemu-devel] [PATCH v6 05/22] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Kevin Wolf]

Am 22.06.2016 um 09:34 hat Fam Zheng geschrieben:
> On Fri, 06/17 14:12, Kevin Wolf wrote:
> > Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > > 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 6937694..749214a 100644
> > > --- a/include/qemu/osdep.h
> > > +++ b/include/qemu/osdep.h
> > > @@ -280,6 +280,8 @@ int qemu_madvise(void *addr, size_t len, int advice);
> > >  
> > >  int qemu_open(const char *name, int flags, ...);
> > >  int qemu_close(int fd);
> > > +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 9a7a439..085ed52 100644
> > > --- a/util/osdep.c
> > > +++ b/util/osdep.c
> > > @@ -137,6 +137,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
> > > +}
> > 
> > This will return -ENOTSUP in the case that the function wasn't available
> > at build time, but -EINVAL if it was available at build time but the
> > kernel doesn't support it at runtime. Should we unify this?
> 
> I'm not sure we can reliably distinguish "fcntl cmd not supported" and "fcntl
> cmd supported but parameters have invalid value" out of -EINVAL.

Well, the other option is returning -EINVAL instead of -ENOTSUP in the
#else branch.

> Quoting the manual:
> 
> > https://www.gnu.org/software/libc/manual/html_node/Open-File-Description-Locks.html
> > 
> > Macro: int F_OFD_SETL
> > 
> >     EINVAL
> > 
> >     Either the lockp argument doesn’t specify valid lock information, the
> >     operating system kernel doesn’t support open file description locks, or the
> >     file associated with filedes doesn’t support locks.
> > 
> 
> I'd expect the user to know what he's doing if he is using a kernel that is
> much older than the one built QEMU, since this is relatively a very uncommon
> thing to do.

I'm talking about possible bugs if a caller of this function is checking
for -ENOTSUP, it's easy to forget -EINVAL there. Fixing qemu isn't one
of the things that we should expect even from a "user who knows what
he's doing".

Kevin

Re: [Qemu-devel] [PATCH v6 04/22] block: Introduce image file locking

[Fam Zheng]

On Wed, 06/22 10:30, Kevin Wolf wrote:
> Am 22.06.2016 um 09:23 hat Fam Zheng geschrieben:
> > On Fri, 06/17 13:34, Kevin Wolf wrote:
> > > Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > > > 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                   | 57 +++++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/block/block.h     | 11 ++++++++-
> > > >  include/block/block_int.h |  5 +++++
> > > >  3 files changed, 72 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/block.c b/block.c
> > > > index 736432f..4c2a3ff 100644
> > > > --- a/block.c
> > > > +++ b/block.c
> > > > @@ -854,6 +854,50 @@ out:
> > > >      g_free(gen_node_name);
> > > >  }
> > > >  
> > > > +BdrvLockfCmd bdrv_get_locking_cmd(int flags)
> > > > +{
> > > > +    if (flags & BDRV_O_NO_LOCK) {
> > > > +        return BDRV_LOCKF_UNLOCK;
> > > > +    } else if (flags & BDRV_O_SHARED_LOCK) {
> > > > +        return BDRV_LOCKF_SHARED;
> > > > +    } else if (flags & BDRV_O_RDWR) {
> > > > +        return BDRV_LOCKF_EXCLUSIVE;
> > > > +    } else {
> > > > +        return BDRV_LOCKF_SHARED;
> > > > +    }
> > > > +}
> > > 
> > > It feels a bit counterintuitive to use the very same operation for
> > > "start of critical section, but don't actually lock" and "end of
> > > critical section".
> > > 
> > > Is there a specific reason why you chose this instead of separate
> > > .bdrv_lock/bdrv_unlock callbacks?
> > 
> > Because unlike open(2)/close(2), locking and unlocking are typically
> > implemented with one parameterized operation (fcntl(2)) underneath, I followed
> > that naturally.
> 
> Is it really "typically", or is this an idiosyncrasy of POSIX and we end
> up modeling our interface for one particular block driver even though
> for others a different interface might be preferable? Did you look at
> more interfaces?

Yes I did.

RBD has rbd_lock_exclusive, rbd_lock_shared and rbd_unlock; Windows have
LockFile and UnlockFile.

But I was also thinking about flock(2), lockf(3) and glfs_posix_lock(). None of
them decided to use a different interface from posix, and I really have had no
personal preference on this.

Either way, we'd have to adapt one style to another by the day when we support
locking in more drivers. At this point the one first implementation, raw-posix,
fits fine.

Back to your intuition concern, I think there is no "start critical section,
but don't actually lock" semantic in this series, the block layer won't call
the driver function unless locking is needed.

> 
> Apart from that, we generally make our internal interfaces so that they
> are easy to use and code is easy to read rather than directly mapping
> POSIX everywhere. For example, our I/O function never do short
> reads/writes even though POSIX does that.

This is kinda of personal preference but as I am not insisting on either side,
we can go the other way if you don't like it.

Fam

Re: [Qemu-devel] [PATCH v6 05/22] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Fam Zheng]

On Wed, 06/22 10:34, Kevin Wolf wrote:
> > > This will return -ENOTSUP in the case that the function wasn't available
> > > at build time, but -EINVAL if it was available at build time but the
> > > kernel doesn't support it at runtime. Should we unify this?
> > 
> > I'm not sure we can reliably distinguish "fcntl cmd not supported" and "fcntl
> > cmd supported but parameters have invalid value" out of -EINVAL.
> 
> Well, the other option is returning -EINVAL instead of -ENOTSUP in the
> #else branch.
> 
> > Quoting the manual:
> > 
> > > https://www.gnu.org/software/libc/manual/html_node/Open-File-Description-Locks.html
> > > 
> > > Macro: int F_OFD_SETL
> > > 
> > >     EINVAL
> > > 
> > >     Either the lockp argument doesn’t specify valid lock information, the
> > >     operating system kernel doesn’t support open file description locks, or the
> > >     file associated with filedes doesn’t support locks.
> > > 
> > 
> > I'd expect the user to know what he's doing if he is using a kernel that is
> > much older than the one built QEMU, since this is relatively a very uncommon
> > thing to do.
> 
> I'm talking about possible bugs if a caller of this function is checking
> for -ENOTSUP, it's easy to forget -EINVAL there. Fixing qemu isn't one
> of the things that we should expect even from a "user who knows what
> he's doing".

Calling function should interprete -ENOTSUP as "not available at build time",
and -EINVAL as any one of the three reasons reported by kernel.

If we return -EINVAL here in the #else branch, the "not available at build
time" is not obvious.  But we intentionally made locking a "nop" if the syscall
is not supported.  Why confuse that with "invalid locking parameter" case?

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v6 03/22] blockdev: Add and parse "lock-mode" option for image locking

[Kevin Wolf]

Am 17.06.2016 um 11:23 hat Kevin Wolf geschrieben:
> Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > Respect the locking mode from CLI or QMP, and set the open flags
> > accordingly.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Max Reitz <mreitz@redhat.com>

> This is the wrong level to implement the feature. You would only be able
> to configure the locking on the top level image this way (because what
> we're doing here is BB, not BDS configuration). If you want to allow
> configuration per node, you need to put the options into
> bdrv_runtime_opts and interpret them in bdrv_open_common().
> 
> Otherwise we would have to specify in the blockdev-add documentation
> that this works only on the top level, but I don't see a reason for
> such a restriction.

And actually, after some more thinking about block device configuration,
I'm not sure any more whether letting the user configure this on the
node level, at least as the primary interface.

A node usually knows by itself what locking mode it needs to request
from its children, depending on the locking mode that the parent node
requested for it. It could be passing down the locking mode (raw
format), it could require a stricter locking mode (non-raw formats never
work with r/w sharing) or it could even be less strict (backing files
are normally ro/ and can therefore be shared, even if the guest can't
share its image).

The real origin of the locking requirement is the top level. So maybe
the right interface for guest devices is adding a qdev option that tells
whether the guest can share the image. For NBD servers, we'd add a QMP
option that tells whether client can share the image. And starting from
these requirements, the locking mode would propagate through the graph,
with each node deciding what it needs to request from its children in
order to achieve the protection that its parent requested.

And at this point I start wondering... Doesn't this look an awful lot
like op blockers? (The new ones.) Should image locking be integrated
there?

I still see a (limited) use for the node-level configuration: The user
might want to request a stricter locking mode than is necessary because
they foresee an operation that will change the requirements (e.g. commit
to a backing file) and they don't want to risk failure then.

Any opinions?

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v6 03/22] blockdev: Add and parse "lock-mode" option for image locking

[Fam Zheng]

On Tue, 07/05 15:37, Kevin Wolf wrote:
> Am 17.06.2016 um 11:23 hat Kevin Wolf geschrieben:
> > Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > > Respect the locking mode from CLI or QMP, and set the open flags
> > > accordingly.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> > This is the wrong level to implement the feature. You would only be able
> > to configure the locking on the top level image this way (because what
> > we're doing here is BB, not BDS configuration). If you want to allow
> > configuration per node, you need to put the options into
> > bdrv_runtime_opts and interpret them in bdrv_open_common().
> > 
> > Otherwise we would have to specify in the blockdev-add documentation
> > that this works only on the top level, but I don't see a reason for
> > such a restriction.
> 
> And actually, after some more thinking about block device configuration,
> I'm not sure any more whether letting the user configure this on the
> node level, at least as the primary interface.
> 
> A node usually knows by itself what locking mode it needs to request
> from its children, depending on the locking mode that the parent node
> requested for it. It could be passing down the locking mode (raw
> format), it could require a stricter locking mode (non-raw formats never
> work with r/w sharing) or it could even be less strict (backing files
> are normally ro/ and can therefore be shared, even if the guest can't
> share its image).
> 
> The real origin of the locking requirement is the top level. So maybe
> the right interface for guest devices is adding a qdev option that tells
> whether the guest can share the image. For NBD servers, we'd add a QMP

I think most block devices are not designed to share the data, so in general
it's hard to imagine this as a device property.

> option that tells whether client can share the image. And starting from
> these requirements, the locking mode would propagate through the graph,
> with each node deciding what it needs to request from its children in
> order to achieve the protection that its parent requested.
> 
> And at this point I start wondering... Doesn't this look an awful lot
> like op blockers? (The new ones.) Should image locking be integrated
> there?
> 
> I still see a (limited) use for the node-level configuration: The user
> might want to request a stricter locking mode than is necessary because
> they foresee an operation that will change the requirements (e.g. commit
> to a backing file) and they don't want to risk failure then.
> 
> Any opinions?

Who is going to enable the default auto lock with an unattached (no BB or no
device) image, such as the qemu-img case?  Lock mode there needs to be
configurable too, but moving the option away from the BB/BDS makes this
trickier to do.

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v6 03/22] blockdev: Add and parse "lock-mode" option for image locking

[Kevin Wolf]

Am 08.07.2016 um 04:56 hat Fam Zheng geschrieben:
> On Tue, 07/05 15:37, Kevin Wolf wrote:
> > Am 17.06.2016 um 11:23 hat Kevin Wolf geschrieben:
> > > Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > > > Respect the locking mode from CLI or QMP, and set the open flags
> > > > accordingly.
> > > > 
> > > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > 
> > > This is the wrong level to implement the feature. You would only be able
> > > to configure the locking on the top level image this way (because what
> > > we're doing here is BB, not BDS configuration). If you want to allow
> > > configuration per node, you need to put the options into
> > > bdrv_runtime_opts and interpret them in bdrv_open_common().
> > > 
> > > Otherwise we would have to specify in the blockdev-add documentation
> > > that this works only on the top level, but I don't see a reason for
> > > such a restriction.
> > 
> > And actually, after some more thinking about block device configuration,
> > I'm not sure any more whether letting the user configure this on the
> > node level, at least as the primary interface.
> > 
> > A node usually knows by itself what locking mode it needs to request
> > from its children, depending on the locking mode that the parent node
> > requested for it. It could be passing down the locking mode (raw
> > format), it could require a stricter locking mode (non-raw formats never
> > work with r/w sharing) or it could even be less strict (backing files
> > are normally ro/ and can therefore be shared, even if the guest can't
> > share its image).
> > 
> > The real origin of the locking requirement is the top level. So maybe
> > the right interface for guest devices is adding a qdev option that tells
> > whether the guest can share the image. For NBD servers, we'd add a QMP
> 
> I think most block devices are not designed to share the data, so in general
> it's hard to imagine this as a device property.

Well, it's really a guest OS (or even guest application) property, but
obviously that doesn't exist. And the device is the qemu component that
is the closest to the guest. We generally have options about behaviour
that the guest expects at the device level.

> > option that tells whether client can share the image. And starting from
> > these requirements, the locking mode would propagate through the graph,
> > with each node deciding what it needs to request from its children in
> > order to achieve the protection that its parent requested.
> > 
> > And at this point I start wondering... Doesn't this look an awful lot
> > like op blockers? (The new ones.) Should image locking be integrated
> > there?
> > 
> > I still see a (limited) use for the node-level configuration: The user
> > might want to request a stricter locking mode than is necessary because
> > they foresee an operation that will change the requirements (e.g. commit
> > to a backing file) and they don't want to risk failure then.
> > 
> > Any opinions?
> 
> Who is going to enable the default auto lock with an unattached (no BB or no
> device) image, such as the qemu-img case?  Lock mode there needs to be
> configurable too, but moving the option away from the BB/BDS makes this
> trickier to do.

Unattached BDSes don't get I/O. qemu-img does use a BB.

Do you actually need to configure the locking mode for qemu-img or is
just a switch for ignoring locks enough? Your -L could be implemented
more or less the same way as it is now. You got a user option and you
pass it down to the lower layers. The exact way of how you pass it down
(flags vs. possibly some op blocker thing) could change, but it doesn't
really change much conceptually.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v6 03/22] blockdev: Add and parse "lock-mode" option for image locking

[Max Reitz]

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

On 08.07.2016 11:50, Kevin Wolf wrote:
> Am 08.07.2016 um 04:56 hat Fam Zheng geschrieben:
>> On Tue, 07/05 15:37, Kevin Wolf wrote:
>>> Am 17.06.2016 um 11:23 hat Kevin Wolf geschrieben:
>>>> Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
>>>>> Respect the locking mode from CLI or QMP, and set the open flags
>>>>> accordingly.
>>>>>
>>>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>
>>>> This is the wrong level to implement the feature. You would only be able
>>>> to configure the locking on the top level image this way (because what
>>>> we're doing here is BB, not BDS configuration). If you want to allow
>>>> configuration per node, you need to put the options into
>>>> bdrv_runtime_opts and interpret them in bdrv_open_common().
>>>>
>>>> Otherwise we would have to specify in the blockdev-add documentation
>>>> that this works only on the top level, but I don't see a reason for
>>>> such a restriction.
>>>
>>> And actually, after some more thinking about block device configuration,
>>> I'm not sure any more whether letting the user configure this on the
>>> node level, at least as the primary interface.
>>>
>>> A node usually knows by itself what locking mode it needs to request
>>> from its children, depending on the locking mode that the parent node
>>> requested for it. It could be passing down the locking mode (raw
>>> format), it could require a stricter locking mode (non-raw formats never
>>> work with r/w sharing) or it could even be less strict (backing files
>>> are normally ro/ and can therefore be shared, even if the guest can't
>>> share its image).
>>>
>>> The real origin of the locking requirement is the top level. So maybe
>>> the right interface for guest devices is adding a qdev option that tells
>>> whether the guest can share the image. For NBD servers, we'd add a QMP
>>
>> I think most block devices are not designed to share the data, so in general
>> it's hard to imagine this as a device property.
> 
> Well, it's really a guest OS (or even guest application) property, but
> obviously that doesn't exist. And the device is the qemu component that
> is the closest to the guest. We generally have options about behaviour
> that the guest expects at the device level.

Can the guest device really make a qualified decision here? If you're
using raw as the image format, sharing may be fine, if you're using
qcow2, it most likely is not.

I think whether to lock a BDS chain is a host-side property and has not
a lot to do with the guest, thus it should be a BDS property. I can
imagine that a guest may say that sharing should be disallowed under all
circumstances, but a guest is never able to decide to allow sharing.

Max


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

Re: [Qemu-devel] [Qemu-block] [PATCH v6 03/22] blockdev: Add and parse "lock-mode" option for image locking

[Kevin Wolf]

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

Am 08.07.2016 um 15:05 hat Max Reitz geschrieben:
> On 08.07.2016 11:50, Kevin Wolf wrote:
> > Am 08.07.2016 um 04:56 hat Fam Zheng geschrieben:
> >> On Tue, 07/05 15:37, Kevin Wolf wrote:
> >>> Am 17.06.2016 um 11:23 hat Kevin Wolf geschrieben:
> >>>> Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> >>>>> Respect the locking mode from CLI or QMP, and set the open flags
> >>>>> accordingly.
> >>>>>
> >>>>> Signed-off-by: Fam Zheng <famz@redhat.com>
> >>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
> >>>
> >>>> This is the wrong level to implement the feature. You would only be able
> >>>> to configure the locking on the top level image this way (because what
> >>>> we're doing here is BB, not BDS configuration). If you want to allow
> >>>> configuration per node, you need to put the options into
> >>>> bdrv_runtime_opts and interpret them in bdrv_open_common().
> >>>>
> >>>> Otherwise we would have to specify in the blockdev-add documentation
> >>>> that this works only on the top level, but I don't see a reason for
> >>>> such a restriction.
> >>>
> >>> And actually, after some more thinking about block device configuration,
> >>> I'm not sure any more whether letting the user configure this on the
> >>> node level, at least as the primary interface.
> >>>
> >>> A node usually knows by itself what locking mode it needs to request
> >>> from its children, depending on the locking mode that the parent node
> >>> requested for it. It could be passing down the locking mode (raw
> >>> format), it could require a stricter locking mode (non-raw formats never
> >>> work with r/w sharing) or it could even be less strict (backing files
> >>> are normally ro/ and can therefore be shared, even if the guest can't
> >>> share its image).
> >>>
> >>> The real origin of the locking requirement is the top level. So maybe
> >>> the right interface for guest devices is adding a qdev option that tells
> >>> whether the guest can share the image. For NBD servers, we'd add a QMP
> >>
> >> I think most block devices are not designed to share the data, so in general
> >> it's hard to imagine this as a device property.
> > 
> > Well, it's really a guest OS (or even guest application) property, but
> > obviously that doesn't exist. And the device is the qemu component that
> > is the closest to the guest. We generally have options about behaviour
> > that the guest expects at the device level.
> 
> Can the guest device really make a qualified decision here? If you're
> using raw as the image format, sharing may be fine, if you're using
> qcow2, it most likely is not.

Just noticed while looking at v7 that I never replied here...

Yes, you're right that the guest device can't make the decision for the
exact locking mode of the images. But the device is the point where the
user has to decide whether or not sharing is okay. For everything below
it, qemu knows by itself whether it can share the image.

This is essentially what I already described above:

> >>> A node usually knows by itself what locking mode it needs to request
> >>> from its children, depending on the locking mode that the parent node
> >>> requested for it. It could be passing down the locking mode (raw
> >>> format), it could require a stricter locking mode (non-raw formats never
> >>> work with r/w sharing) or it could even be less strict (backing files
> >>> are normally ro/ and can therefore be shared, even if the guest can't
> >>> share its image).

> I think whether to lock a BDS chain is a host-side property and has not
> a lot to do with the guest, thus it should be a BDS property. I can
> imagine that a guest may say that sharing should be disallowed under all
> circumstances, but a guest is never able to decide to allow sharing.

Well, yes and no. The decision which lock mode to use is at the node
level, no doubt. But it's not something that a user can configure.
Non-raw images simply can't be shared and the user can't do anything
about it. Why should they have an option to specify a lock mode when
there is only one correct setting?

The only possible exception where an option on the node level could make
sense (which I already mentioned earlier in this thread) is if the user
wants to be stricter than what qemu needs.

Kevin

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

Re: [Qemu-devel] [Qemu-block] [PATCH v6 03/22] blockdev: Add and parse "lock-mode" option for image locking

[Daniel P. Berrange]

On Tue, Sep 06, 2016 at 06:45:55PM +0200, Kevin Wolf wrote:
> Am 08.07.2016 um 15:05 hat Max Reitz geschrieben:
> > I think whether to lock a BDS chain is a host-side property and has not
> > a lot to do with the guest, thus it should be a BDS property. I can
> > imagine that a guest may say that sharing should be disallowed under all
> > circumstances, but a guest is never able to decide to allow sharing.
> 
> Well, yes and no. The decision which lock mode to use is at the node
> level, no doubt. But it's not something that a user can configure.
> Non-raw images simply can't be shared and the user can't do anything
> about it. Why should they have an option to specify a lock mode when
> there is only one correct setting?

I'm not sure that's true for all non-raw images. I understand things like
qcow2 won't work, because of cached metadata which can be updated by QEMU
during writes, but I think the luks format ought to be safe as the cached
metadata is never changed.

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

Re: [Qemu-devel] [Qemu-block] [PATCH v6 03/22] blockdev: Add and parse "lock-mode" option for image locking

[Kevin Wolf]

Am 06.09.2016 um 18:51 hat Daniel P. Berrange geschrieben:
> On Tue, Sep 06, 2016 at 06:45:55PM +0200, Kevin Wolf wrote:
> > Am 08.07.2016 um 15:05 hat Max Reitz geschrieben:
> > > I think whether to lock a BDS chain is a host-side property and has not
> > > a lot to do with the guest, thus it should be a BDS property. I can
> > > imagine that a guest may say that sharing should be disallowed under all
> > > circumstances, but a guest is never able to decide to allow sharing.
> > 
> > Well, yes and no. The decision which lock mode to use is at the node
> > level, no doubt. But it's not something that a user can configure.
> > Non-raw images simply can't be shared and the user can't do anything
> > about it. Why should they have an option to specify a lock mode when
> > there is only one correct setting?
> 
> I'm not sure that's true for all non-raw images. I understand things like
> qcow2 won't work, because of cached metadata which can be updated by QEMU
> during writes, but I think the luks format ought to be safe as the cached
> metadata is never changed.

Yes, I guess I've been overgeneralising. Doesn't really make a
difference for my point, though, the luks driver still knows what it
needs to request from the lower layer without user intervention.

Kevin