Re: [Qemu-devel] [PATCH v5 07/27] block: Handle image locking during reopen

[Max Reitz <mreitz@redhat.com>]

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

On 17.05.2016 09:35, Fam Zheng wrote:
> Stash the locking state into BDRVReopenState. If it was locked, unlock
> in prepare, and lock it again when commit or abort.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c               | 11 +++++++++++
>  include/block/block.h |  1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/block.c b/block.c
> index ad3663c..2be42bb 100644
> --- a/block.c
> +++ b/block.c
> @@ -2112,6 +2112,11 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>          } while ((entry = qdict_next(reopen_state->options, entry)));
>      }
>  
> +    reopen_state->was_locked = reopen_state->bs->image_locked;
> +    if (reopen_state->was_locked) {
> +        bdrv_unlock_image(reopen_state->bs);
> +    }
> +
>      ret = 0;
>  
>  error:
> @@ -2136,6 +2141,9 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
>      if (drv->bdrv_reopen_commit) {
>          drv->bdrv_reopen_commit(reopen_state);
>      }
> +    if (reopen_state->was_locked) {
> +        bdrv_lock_image(reopen_state->bs);
> +    }
>  
>      /* set BDS specific flags now */
>      QDECREF(reopen_state->bs->explicit_options);
> @@ -2162,6 +2170,9 @@ static void bdrv_reopen_abort(BDRVReopenState *reopen_state)
>      if (drv->bdrv_reopen_abort) {
>          drv->bdrv_reopen_abort(reopen_state);
>      }
> +    if (reopen_state->was_locked) {
> +        bdrv_lock_image(reopen_state->bs);

I'm not sure I'm quite happy with this, because this may fail;
therefore, it may happen that the operation done in _prepare() cannot be
undone.

Of course, the same applies to _commit(): bdrv_lock_image() can just
fail. It's probably even worse there. I don't see a good reason why just
relocking the image in _abort() should fail; but it's very much
conceivable that locking the reopened image in _commit() fails.

I think the correct way would be to rely on the drivers which implement
locking to handle reopening correctly, that is, try lock the reopened
image in _prepare() and unlock the old one before discarding it in
_commit().

However, I'm not sure myself whether it's worth the effort. It's very
simple to do it like you did here -- but then again, it doesn't seem
quite correct.

Also: Should bdrv_reopen_prepare() check that the locking flags are not
changed?

Max

> +    }
>  
>      QDECREF(reopen_state->explicit_options);
>  }
> diff --git a/include/block/block.h b/include/block/block.h
> index 7740d3f..28b8ae9 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -158,6 +158,7 @@ typedef struct BDRVReopenState {
>      QDict *options;
>      QDict *explicit_options;
>      void *opaque;
> +    bool was_locked;
>  } BDRVReopenState;
>  
>  /*
> 



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