On Tue, 05/24 18:28, Max Reitz wrote:
> 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 <f...@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.
You are right, this is driver specific. raw-posix should "update" the lock.
Will update this patch.
>
> Also: Should bdrv_reopen_prepare() check that the locking flags are not
> changed?
Read only flag also matters in fcntl locks, so practically we almost always
need some change on the lock.
Fam
