[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 07/27] block: Handle image locking during reo
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v5 07/27] block: Handle image locking during reopen |
Date: |
Tue, 24 May 2016 18:28:55 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 |
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 <address@hidden>
> ---
> 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;
>
> /*
>
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v5 00/27] block: Lock images when opening, Fam Zheng, 2016/05/17
- [Qemu-devel] [PATCH v5 01/27] block: Add flag bits for image locking, Fam Zheng, 2016/05/17
- [Qemu-devel] [PATCH v5 03/27] blockdev: Add and parse "lock-mode" option for image locking, Fam Zheng, 2016/05/17
- [Qemu-devel] [PATCH v5 02/27] qapi: Add lock-mode in blockdev-add options, Fam Zheng, 2016/05/17
- [Qemu-devel] [PATCH v5 04/27] block: Introduce image file locking, Fam Zheng, 2016/05/17
- [Qemu-devel] [PATCH v5 07/27] block: Handle image locking during reopen, Fam Zheng, 2016/05/17
- Re: [Qemu-devel] [PATCH v5 07/27] block: Handle image locking during reopen,
Max Reitz <=
[Qemu-devel] [PATCH v5 05/27] block: Add bdrv_image_locked, Fam Zheng, 2016/05/17
[Qemu-devel] [PATCH v5 06/27] block: Make bdrv_reopen_{commit, abort} private functions, Fam Zheng, 2016/05/17
[Qemu-devel] [PATCH v5 08/27] osdep: Add qemu_lock_fd and qemu_unlock_fd, Fam Zheng, 2016/05/17