[Qemu-devel] [PATCH v5 0/3] file-posix: Simplifications on image locking

[Qemu-devel] [PATCH v5 0/3] file-posix: Simplifications on image locking

[Fam Zheng]

v5: Address Max's comments (Thanks for reviewing):
    - Clean up after test done.
    - Add rev-by to patch 1 and 2.

v4: Fix test on systems without OFD. [Patchew]

The first patch reduces chances of QEMU crash in unusual (but not unlikely)
cases especially when used by Libvirt (see commit message).

The second patch halves fd for images.

The third adds some more test for patch one (would have caught the regression
caused by v2).

Fam Zheng (3):
  file-posix: Skip effectiveless OFD lock operations
  file-posix: Drop s->lock_fd
  tests: Add unit tests for image locking

 block/file-posix.c         |  83 +++++++++++++-------
 tests/Makefile.include     |   2 +
 tests/test-image-locking.c | 157 +++++++++++++++++++++++++++++++++++++
 3 files changed, 212 insertions(+), 30 deletions(-)
 create mode 100644 tests/test-image-locking.c

-- 
2.17.1

[Qemu-devel] [PATCH v5 1/3] file-posix: Skip effectiveless OFD lock operations

[Fam Zheng]

If we know we've already locked the bytes, don't do it again; similarly
don't unlock a byte if we haven't locked it. This doesn't change the
behavior, but fixes a corner case explained below.

Libvirt had an error handling bug that an image can get its (ownership,
file mode, SELinux) permissions changed (RHBZ 1584982) by mistake behind
QEMU. Specifically, an image in use by Libvirt VM has:

    $ ls -lhZ b.img
    -rw-r--r--. qemu qemu system_u:object_r:svirt_image_t:s0:c600,c690 b.img

Trying to attach it a second time won't work because of image locking.
And after the error, it becomes:

    $ ls -lhZ b.img
    -rw-r--r--. root root system_u:object_r:virt_image_t:s0 b.img

Then, we won't be able to do OFD lock operations with the existing fd.
In other words, the code such as in blk_detach_dev:

    blk_set_perm(blk, 0, BLK_PERM_ALL, &error_abort);

can abort() QEMU, out of environmental changes.

This patch is an easy fix to this and the change is regardlessly
reasonable, so do it.

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

diff --git a/block/file-posix.c b/block/file-posix.c
index 2da3a76355..cf5eb98caa 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -152,6 +152,11 @@ typedef struct BDRVRawState {
     uint64_t perm;
     uint64_t shared_perm;
 
+    /* The perms bits whose corresponding bytes are already locked in
+     * s->lock_fd. */
+    uint64_t locked_perm;
+    uint64_t locked_shared_perm;
+
 #ifdef CONFIG_XFS
     bool is_xfs:1;
 #endif
@@ -680,43 +685,72 @@ typedef enum {
  * file; if @unlock == true, also unlock the unneeded bytes.
  * @shared_perm_lock_bits is the mask of all permissions that are NOT shared.
  */
-static int raw_apply_lock_bytes(int fd,
+static int raw_apply_lock_bytes(BDRVRawState *s, int fd,
                                 uint64_t perm_lock_bits,
                                 uint64_t shared_perm_lock_bits,
                                 bool unlock, Error **errp)
 {
     int ret;
     int i;
+    uint64_t locked_perm, locked_shared_perm;
+
+    if (s) {
+        locked_perm = s->locked_perm;
+        locked_shared_perm = s->locked_shared_perm;
+    } else {
+        /*
+         * We don't have the previous bits, just lock/unlock for each of the
+         * requested bits.
+         */
+        if (unlock) {
+            locked_perm = BLK_PERM_ALL;
+            locked_shared_perm = BLK_PERM_ALL;
+        } else {
+            locked_perm = 0;
+            locked_shared_perm = 0;
+        }
+    }
 
     PERM_FOREACH(i) {
         int off = RAW_LOCK_PERM_BASE + i;
-        if (perm_lock_bits & (1ULL << i)) {
+        uint64_t bit = (1ULL << i);
+        if ((perm_lock_bits & bit) && !(locked_perm & bit)) {
             ret = qemu_lock_fd(fd, off, 1, false);
             if (ret) {
                 error_setg(errp, "Failed to lock byte %d", off);
                 return ret;
+            } else if (s) {
+                s->locked_perm |= bit;
             }
-        } else if (unlock) {
+        } else if (unlock && (locked_perm & bit) && !(perm_lock_bits & bit)) {
             ret = qemu_unlock_fd(fd, off, 1);
             if (ret) {
                 error_setg(errp, "Failed to unlock byte %d", off);
                 return ret;
+            } else if (s) {
+                s->locked_perm &= ~bit;
             }
         }
     }
     PERM_FOREACH(i) {
         int off = RAW_LOCK_SHARED_BASE + i;
-        if (shared_perm_lock_bits & (1ULL << i)) {
+        uint64_t bit = (1ULL << i);
+        if ((shared_perm_lock_bits & bit) && !(locked_shared_perm & bit)) {
             ret = qemu_lock_fd(fd, off, 1, false);
             if (ret) {
                 error_setg(errp, "Failed to lock byte %d", off);
                 return ret;
+            } else if (s) {
+                s->locked_shared_perm |= bit;
             }
-        } else if (unlock) {
+        } else if (unlock && (locked_shared_perm & bit) &&
+                   !(shared_perm_lock_bits & bit)) {
             ret = qemu_unlock_fd(fd, off, 1);
             if (ret) {
                 error_setg(errp, "Failed to unlock byte %d", off);
                 return ret;
+            } else if (s) {
+                s->locked_shared_perm &= ~bit;
             }
         }
     }
@@ -784,7 +818,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
 
     switch (op) {
     case RAW_PL_PREPARE:
-        ret = raw_apply_lock_bytes(s->lock_fd, s->perm | new_perm,
+        ret = raw_apply_lock_bytes(s, s->lock_fd, s->perm | new_perm,
                                    ~s->shared_perm | ~new_shared,
                                    false, errp);
         if (!ret) {
@@ -799,7 +833,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
         op = RAW_PL_ABORT;
         /* fall through to unlock bytes. */
     case RAW_PL_ABORT:
-        raw_apply_lock_bytes(s->lock_fd, s->perm, ~s->shared_perm,
+        raw_apply_lock_bytes(s, s->lock_fd, s->perm, ~s->shared_perm,
                              true, &local_err);
         if (local_err) {
             /* Theoretically the above call only unlocks bytes and it cannot
@@ -809,7 +843,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
         }
         break;
     case RAW_PL_COMMIT:
-        raw_apply_lock_bytes(s->lock_fd, new_perm, ~new_shared,
+        raw_apply_lock_bytes(s, s->lock_fd, new_perm, ~new_shared,
                              true, &local_err);
         if (local_err) {
             /* Theoretically the above call only unlocks bytes and it cannot
@@ -2213,7 +2247,7 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp)
     shared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
 
     /* Step one: Take locks */
-    result = raw_apply_lock_bytes(fd, perm, ~shared, false, errp);
+    result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp);
     if (result < 0) {
         goto out_close;
     }
@@ -2257,7 +2291,7 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp)
     }
 
 out_unlock:
-    raw_apply_lock_bytes(fd, 0, 0, true, &local_err);
+    raw_apply_lock_bytes(NULL, fd, 0, 0, true, &local_err);
     if (local_err) {
         /* The above call should not fail, and if it does, that does
          * not mean the whole creation operation has failed.  So
-- 
2.17.1

[Qemu-devel] [PATCH v5 2/3] file-posix: Drop s->lock_fd

[Fam Zheng]

The lock_fd field is not strictly necessary because transferring locked
bytes from old fd to the new one shouldn't fail anyway. This spares the
user one fd per image.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/file-posix.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index cf5eb98caa..d493357b89 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -142,7 +142,6 @@ do { \
 
 typedef struct BDRVRawState {
     int fd;
-    int lock_fd;
     bool use_lock;
     int type;
     int open_flags;
@@ -153,7 +152,7 @@ typedef struct BDRVRawState {
     uint64_t shared_perm;
 
     /* The perms bits whose corresponding bytes are already locked in
-     * s->lock_fd. */
+     * s->fd. */
     uint64_t locked_perm;
     uint64_t locked_shared_perm;
 
@@ -542,18 +541,6 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     }
     s->fd = fd;
 
-    s->lock_fd = -1;
-    if (s->use_lock) {
-        fd = qemu_open(filename, s->open_flags);
-        if (fd < 0) {
-            ret = -errno;
-            error_setg_errno(errp, errno, "Could not open '%s' for locking",
-                             filename);
-            qemu_close(s->fd);
-            goto fail;
-        }
-        s->lock_fd = fd;
-    }
     s->perm = 0;
     s->shared_perm = BLK_PERM_ALL;
 
@@ -814,15 +801,13 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
         return 0;
     }
 
-    assert(s->lock_fd > 0);
-
     switch (op) {
     case RAW_PL_PREPARE:
-        ret = raw_apply_lock_bytes(s, s->lock_fd, s->perm | new_perm,
+        ret = raw_apply_lock_bytes(s, s->fd, s->perm | new_perm,
                                    ~s->shared_perm | ~new_shared,
                                    false, errp);
         if (!ret) {
-            ret = raw_check_lock_bytes(s->lock_fd, new_perm, new_shared, errp);
+            ret = raw_check_lock_bytes(s->fd, new_perm, new_shared, errp);
             if (!ret) {
                 return 0;
             }
@@ -833,7 +818,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
         op = RAW_PL_ABORT;
         /* fall through to unlock bytes. */
     case RAW_PL_ABORT:
-        raw_apply_lock_bytes(s, s->lock_fd, s->perm, ~s->shared_perm,
+        raw_apply_lock_bytes(s, s->fd, s->perm, ~s->shared_perm,
                              true, &local_err);
         if (local_err) {
             /* Theoretically the above call only unlocks bytes and it cannot
@@ -843,7 +828,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
         }
         break;
     case RAW_PL_COMMIT:
-        raw_apply_lock_bytes(s, s->lock_fd, new_perm, ~new_shared,
+        raw_apply_lock_bytes(s, s->fd, new_perm, ~new_shared,
                              true, &local_err);
         if (local_err) {
             /* Theoretically the above call only unlocks bytes and it cannot
@@ -960,10 +945,18 @@ static void raw_reopen_commit(BDRVReopenState *state)
 {
     BDRVRawReopenState *rs = state->opaque;
     BDRVRawState *s = state->bs->opaque;
+    Error *local_err = NULL;
 
     s->check_cache_dropped = rs->check_cache_dropped;
     s->open_flags = rs->open_flags;
 
+    /* Copy locks to the new fd before closing the old one. */
+    raw_apply_lock_bytes(NULL, rs->fd, s->locked_perm,
+                         ~s->locked_shared_perm, false, &local_err);
+    if (local_err) {
+        /* shouldn't fail in a sane host, but report it just in case. */
+        error_report_err(local_err);
+    }
     qemu_close(s->fd);
     s->fd = rs->fd;
 
@@ -1956,10 +1949,6 @@ 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;
-    }
 }
 
 /**
-- 
2.17.1

[Qemu-devel] [PATCH v5 3/3] tests: Add unit tests for image locking

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/Makefile.include     |   2 +
 tests/test-image-locking.c | 157 +++++++++++++++++++++++++++++++++++++
 2 files changed, 159 insertions(+)
 create mode 100644 tests/test-image-locking.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 7a3059bf6c..995c84f38a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -95,6 +95,7 @@ check-unit-y += tests/test-bdrv-drain$(EXESUF)
 check-unit-y += tests/test-blockjob$(EXESUF)
 check-unit-y += tests/test-blockjob-txn$(EXESUF)
 check-unit-y += tests/test-block-backend$(EXESUF)
+check-unit-y += tests/test-image-locking$(EXESUF)
 check-unit-y += tests/test-x86-cpuid$(EXESUF)
 # all code tested by test-x86-cpuid is inside topology.h
 gcov-files-test-x86-cpuid-y =
@@ -651,6 +652,7 @@ tests/test-bdrv-drain$(EXESUF): tests/test-bdrv-drain.o $(test-block-obj-y) $(te
 tests/test-blockjob$(EXESUF): tests/test-blockjob.o $(test-block-obj-y) $(test-util-obj-y)
 tests/test-blockjob-txn$(EXESUF): tests/test-blockjob-txn.o $(test-block-obj-y) $(test-util-obj-y)
 tests/test-block-backend$(EXESUF): tests/test-block-backend.o $(test-block-obj-y) $(test-util-obj-y)
+tests/test-image-locking$(EXESUF): tests/test-image-locking.o $(test-block-obj-y) $(test-util-obj-y)
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(test-block-obj-y)
 tests/test-iov$(EXESUF): tests/test-iov.o $(test-util-obj-y)
 tests/test-hbitmap$(EXESUF): tests/test-hbitmap.o $(test-util-obj-y) $(test-crypto-obj-y)
diff --git a/tests/test-image-locking.c b/tests/test-image-locking.c
new file mode 100644
index 0000000000..7614cbf90c
--- /dev/null
+++ b/tests/test-image-locking.c
@@ -0,0 +1,157 @@
+/*
+ * Image locking tests
+ *
+ * Copyright (c) 2018 Red Hat Inc.
+ *
+ * Author: Fam Zheng <famz@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "block/block.h"
+#include "sysemu/block-backend.h"
+#include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+
+static BlockBackend *open_image(const char *path,
+                                uint64_t perm, uint64_t shared_perm,
+                                Error **errp)
+{
+    Error *local_err = NULL;
+    BlockBackend *blk;
+    QDict *options = qdict_new();
+
+    qdict_put_str(options, "driver", "raw");
+    blk = blk_new_open(path, NULL, options, BDRV_O_RDWR, &local_err);
+    if (blk) {
+        g_assert_null(local_err);
+        if (blk_set_perm(blk, perm, shared_perm, errp)) {
+            blk_unref(blk);
+            blk = NULL;
+        }
+    } else {
+        error_propagate(errp, local_err);
+    }
+    return blk;
+}
+
+static void check_locked_bytes(int fd, uint64_t perm_locks,
+                               uint64_t shared_perm_locks)
+{
+    int i;
+
+    if (!perm_locks && !shared_perm_locks) {
+        g_assert(!qemu_lock_fd_test(fd, 0, 0, true));
+        return;
+    }
+    for (i = 0; (1ULL << i) <= BLK_PERM_ALL; i++) {
+        uint64_t bit = (1ULL << i);
+        bool perm_expected = !!(bit & perm_locks);
+        bool shared_perm_expected = !!(bit & shared_perm_locks);
+        g_assert_cmpint(perm_expected, ==,
+                        !!qemu_lock_fd_test(fd, 100 + i, 1, true));
+        g_assert_cmpint(shared_perm_expected, ==,
+                        !!qemu_lock_fd_test(fd, 200 + i, 1, true));
+    }
+}
+
+static void test_image_locking_basic(void)
+{
+    BlockBackend *blk1, *blk2, *blk3;
+    char img_path[] = "/tmp/qtest.XXXXXX";
+    uint64_t perm, shared_perm;
+
+    int fd = mkstemp(img_path);
+    assert(fd >= 0);
+
+    perm = BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ;
+    shared_perm = BLK_PERM_ALL;
+    blk1 = open_image(img_path, perm, shared_perm, &error_abort);
+    g_assert(blk1);
+
+    check_locked_bytes(fd, perm, ~shared_perm);
+
+    /* compatible perm between blk1 and blk2 */
+    blk2 = open_image(img_path, perm | BLK_PERM_RESIZE, shared_perm, NULL);
+    g_assert(blk2);
+    check_locked_bytes(fd, perm | BLK_PERM_RESIZE, ~shared_perm);
+
+    /* incompatible perm with already open blk1 and blk2 */
+    blk3 = open_image(img_path, perm, BLK_PERM_WRITE_UNCHANGED, NULL);
+    g_assert_null(blk3);
+
+    blk_unref(blk2);
+
+    /* Check that extra bytes in blk2 are correctly unlocked */
+    check_locked_bytes(fd, perm, ~shared_perm);
+
+    blk_unref(blk1);
+
+    /* Image is unused, no lock there */
+    check_locked_bytes(fd, 0, 0);
+    blk3 = open_image(img_path, perm, BLK_PERM_WRITE_UNCHANGED, &error_abort);
+    g_assert(blk3);
+    blk_unref(blk3);
+    close(fd);
+    unlink(img_path);
+}
+
+static void test_set_perm_abort(void)
+{
+    BlockBackend *blk1, *blk2;
+    char img_path[] = "/tmp/qtest.XXXXXX";
+    uint64_t perm, shared_perm;
+    int r;
+    int fd = mkstemp(img_path);
+    assert(fd >= 0);
+
+    perm = BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ;
+    shared_perm = BLK_PERM_ALL;
+    blk1 = open_image(img_path, perm, shared_perm, &error_abort);
+    g_assert(blk1);
+
+    blk2 = open_image(img_path, perm, shared_perm, &error_abort);
+    g_assert(blk2);
+
+    check_locked_bytes(fd, perm, ~shared_perm);
+
+    /* A failed blk_set_perm mustn't change perm status (locked bytes) */
+    r = blk_set_perm(blk2, perm | BLK_PERM_RESIZE, BLK_PERM_WRITE_UNCHANGED,
+                     NULL);
+    g_assert_cmpint(r, !=, 0);
+    check_locked_bytes(fd, perm, ~shared_perm);
+    blk_unref(blk1);
+    blk_unref(blk2);
+}
+
+int main(int argc, char **argv)
+{
+    bdrv_init();
+    qemu_init_main_loop(&error_abort);
+
+    g_test_init(&argc, &argv, NULL);
+
+    if (qemu_has_ofd_lock()) {
+        g_test_add_func("/image-locking/basic", test_image_locking_basic);
+        g_test_add_func("/image-locking/set-perm-abort", test_set_perm_abort);
+    }
+
+    return g_test_run();
+}
-- 
2.17.1

Re: [Qemu-devel] [PATCH v5 0/3] file-posix: Simplifications on image locking

[Fam Zheng]

On Thu, 10/11 15:21, Fam Zheng wrote:
> v5: Address Max's comments (Thanks for reviewing):
>     - Clean up after test done.
>     - Add rev-by to patch 1 and 2.

Ping?

Fam

Re: [Qemu-devel] [PATCH v5 0/3] file-posix: Simplifications on image locking

[Kevin Wolf]

Am 11.10.2018 um 09:21 hat Fam Zheng geschrieben:
> v5: Address Max's comments (Thanks for reviewing):
>     - Clean up after test done.
>     - Add rev-by to patch 1 and 2.
> 
> v4: Fix test on systems without OFD. [Patchew]
> 
> The first patch reduces chances of QEMU crash in unusual (but not unlikely)
> cases especially when used by Libvirt (see commit message).
> 
> The second patch halves fd for images.
> 
> The third adds some more test for patch one (would have caught the regression
> caused by v2).

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v5 2/3] file-posix: Drop s->lock_fd

[Alberto Garcia]

On Thu 11 Oct 2018 09:21:34 AM CEST, Fam Zheng wrote:
> The lock_fd field is not strictly necessary because transferring locked
> bytes from old fd to the new one shouldn't fail anyway. This spares the
> user one fd per image.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

One of my tests (not published yet) starts to fail after this
patch. Here's how you can reproduce the error:

$ qemu-img create -f qcow2 hd.qcow2 4G
$ qemu-system-x86_64 -qmp stdio

{ "execute": "qmp_capabilities" }
{ "execute": "blockdev-add", "arguments": {"driver": "qcow2", "node-name": "hd0", "file": {"driver": "file", "filename": "hd.qcow2", "locking": "on" }}}
{ "execute": "human-monitor-command", "arguments": {"command-line": "qemu-io hd0 \"reopen -o file.locking=on\""}}
{ "execute": "human-monitor-command", "arguments": {"command-line": "qemu-io hd0 \"reopen -o file.locking=off\""}}
{ "execute": "blockdev-del", "arguments": {"node-name": "hd0"}}
{ "execute": "blockdev-add", "arguments": {"driver": "qcow2", "node-name": "hd0", "file": {"driver": "file", "filename": "hd.qcow2"}}}

{"error": {"class": "GenericError", "desc": "Failed to get \"consistent read\" lock"}}

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH v5 2/3] file-posix: Drop s->lock_fd

[Max Reitz]

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

On 14.11.18 14:54, Alberto Garcia wrote:
> On Thu 11 Oct 2018 09:21:34 AM CEST, Fam Zheng wrote:
>> The lock_fd field is not strictly necessary because transferring locked
>> bytes from old fd to the new one shouldn't fail anyway. This spares the
>> user one fd per image.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> One of my tests (not published yet) starts to fail after this
> patch. Here's how you can reproduce the error:
> 
> $ qemu-img create -f qcow2 hd.qcow2 4G
> $ qemu-system-x86_64 -qmp stdio
> 
> { "execute": "qmp_capabilities" }
> { "execute": "blockdev-add", "arguments": {"driver": "qcow2", "node-name": "hd0", "file": {"driver": "file", "filename": "hd.qcow2", "locking": "on" }}}
> { "execute": "human-monitor-command", "arguments": {"command-line": "qemu-io hd0 \"reopen -o file.locking=on\""}}
> { "execute": "human-monitor-command", "arguments": {"command-line": "qemu-io hd0 \"reopen -o file.locking=off\""}}
> { "execute": "blockdev-del", "arguments": {"node-name": "hd0"}}
> { "execute": "blockdev-add", "arguments": {"driver": "qcow2", "node-name": "hd0", "file": {"driver": "file", "filename": "hd.qcow2"}}}
> 
> {"error": {"class": "GenericError", "desc": "Failed to get \"consistent read\" lock"}}

To me that looks like a problem in the general reopen code.
raw_reopen_prepare() is called and succeeds.  Then bdrv_reopen_prepare()
notices the option wasn't handled and therefore fails.
bdrv_reopen_multiple() thus doesn't set bs_entry->prepared to true,
which means raw_reopen_abort() won't be called.

We should always call either BlockDriver.bdrv_reopen_commit() or
BlockDriver.bdrv_reopen_abort() when BlockDriver.bdrv_reopen_prepare()
succeeded.

Max


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

Re: [Qemu-devel] [Qemu-block] [PATCH v5 2/3] file-posix: Drop s->lock_fd

[Alberto Garcia]

On Fri 16 Nov 2018 02:34:25 PM CET, Max Reitz wrote:
> To me that looks like a problem in the general reopen code.
> raw_reopen_prepare() is called and succeeds.  Then
> bdrv_reopen_prepare() notices the option wasn't handled and therefore
> fails.  bdrv_reopen_multiple() thus doesn't set bs_entry->prepared to
> true, which means raw_reopen_abort() won't be called.
>
> We should always call either BlockDriver.bdrv_reopen_commit() or
> BlockDriver.bdrv_reopen_abort() when BlockDriver.bdrv_reopen_prepare()
> succeeded.

So you mean getting rid of BlockReopenQueueEntry.prepared altogether?

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH v5 2/3] file-posix: Drop s->lock_fd

[Max Reitz]

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

On 16.11.18 14:58, Alberto Garcia wrote:
> On Fri 16 Nov 2018 02:34:25 PM CET, Max Reitz wrote:
>> To me that looks like a problem in the general reopen code.
>> raw_reopen_prepare() is called and succeeds.  Then
>> bdrv_reopen_prepare() notices the option wasn't handled and therefore
>> fails.  bdrv_reopen_multiple() thus doesn't set bs_entry->prepared to
>> true, which means raw_reopen_abort() won't be called.
>>
>> We should always call either BlockDriver.bdrv_reopen_commit() or
>> BlockDriver.bdrv_reopen_abort() when BlockDriver.bdrv_reopen_prepare()
>> succeeded.
> 
> So you mean getting rid of BlockReopenQueueEntry.prepared altogether?

I mean just calling .bdrv_reopen_abort() in bdrv_reopen_prepare() if
there was an error after .bdrv_reopen_prepare() succeeded.

I have a patch, I'm just trying to think of a useful test...

Max


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

Re: [Qemu-devel] [Qemu-block] [PATCH v5 2/3] file-posix: Drop s->lock_fd

[Max Reitz]

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

On 16.11.18 15:02, Max Reitz wrote:
> On 16.11.18 14:58, Alberto Garcia wrote:
>> On Fri 16 Nov 2018 02:34:25 PM CET, Max Reitz wrote:
>>> To me that looks like a problem in the general reopen code.
>>> raw_reopen_prepare() is called and succeeds.  Then
>>> bdrv_reopen_prepare() notices the option wasn't handled and therefore
>>> fails.  bdrv_reopen_multiple() thus doesn't set bs_entry->prepared to
>>> true, which means raw_reopen_abort() won't be called.
>>>
>>> We should always call either BlockDriver.bdrv_reopen_commit() or
>>> BlockDriver.bdrv_reopen_abort() when BlockDriver.bdrv_reopen_prepare()
>>> succeeded.
>>
>> So you mean getting rid of BlockReopenQueueEntry.prepared altogether?
> 
> I mean just calling .bdrv_reopen_abort() in bdrv_reopen_prepare() if
> there was an error after .bdrv_reopen_prepare() succeeded.
> 
> I have a patch, I'm just trying to think of a useful test...

To elaborate, I didn't want to use your test for two reasons.  First, it
seemed generally too dependant on what file-posix's implementation does.
 Second, I couldn't get it to work in any shorter form (which seemed
weird) and why would it even print something about consistent_read?
That seemed weird, too.

I would've thought that just any case where bdrv_reopen_prepare() fails
after a successful .bdrv_reopen_prepare() could trigger the issue, but
that's not the case.

This is because the file locks are not applied to the new FD just by
duping it, they are only applied in raw_reopen_commit().  So if you
prepare without abort without any commit before, the problem disappears.

So there had to be something wrong in raw_reopen_commit(), too.  It
turns out that this is indeed in the above commit, it should pass
"s->locked_shared_perm" instead of "~s->locked_shared_perm" to
raw_apply_lock_bytes().

Max


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