diff mbox series

[v5,2/3] file-posix: Drop s->lock_fd

Message ID 20181011072135.588-3-famz@redhat.com
State New
Headers show
Series file-posix: Simplifications on image locking | expand

Commit Message

Fam Zheng Oct. 11, 2018, 7:21 a.m. UTC 
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(-)

Comments

Alberto Garcia Nov. 14, 2018, 1:54 p.m. UTC | #1 
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
Max Reitz Nov. 16, 2018, 1:34 p.m. UTC | #2 
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
Alberto Garcia Nov. 16, 2018, 1:58 p.m. UTC | #3 
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
Max Reitz Nov. 16, 2018, 2:02 p.m. UTC | #4 
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
Max Reitz Nov. 16, 2018, 2:16 p.m. UTC | #5 
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
diff mbox series

Patch

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;
-    }
 }
 
 /**