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

[Fam Zheng]

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 <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.

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