Re: [Qemu-devel] [PATCH v6 04/22] block: Introduce image file locking

[Fam Zheng <famz@redhat.com>]

On Fri, 06/17 13:34, Kevin Wolf wrote:
> Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > Block drivers can implement this new operation .bdrv_lockf to actually lock the
> > image in the protocol specific way.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block.c                   | 57 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/block/block.h     | 11 ++++++++-
> >  include/block/block_int.h |  5 +++++
> >  3 files changed, 72 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block.c b/block.c
> > index 736432f..4c2a3ff 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -854,6 +854,50 @@ out:
> >      g_free(gen_node_name);
> >  }
> >  
> > +BdrvLockfCmd bdrv_get_locking_cmd(int flags)
> > +{
> > +    if (flags & BDRV_O_NO_LOCK) {
> > +        return BDRV_LOCKF_UNLOCK;
> > +    } else if (flags & BDRV_O_SHARED_LOCK) {
> > +        return BDRV_LOCKF_SHARED;
> > +    } else if (flags & BDRV_O_RDWR) {
> > +        return BDRV_LOCKF_EXCLUSIVE;
> > +    } else {
> > +        return BDRV_LOCKF_SHARED;
> > +    }
> > +}
> 
> It feels a bit counterintuitive to use the very same operation for
> "start of critical section, but don't actually lock" and "end of
> critical section".
> 
> Is there a specific reason why you chose this instead of separate
> .bdrv_lock/bdrv_unlock callbacks?

Because unlike open(2)/close(2), locking and unlocking are typically
implemented with one parameterized operation (fcntl(2)) underneath, I followed
that naturally.

> 
> > +static int bdrv_lock_unlock_image_do(BlockDriverState *bs, BdrvLockfCmd cmd)
> > +{
> > +    int ret;
> > +
> > +    if (bs->cur_lock == cmd) {
> > +        return 0;
> > +    } else if (!bs->drv) {
> > +        return -ENOMEDIUM;
> > +    } else if (!bs->drv->bdrv_lockf) {
> > +        return 0;
> > +    }
> > +    ret = bs->drv->bdrv_lockf(bs, cmd);
> > +    if (ret == -ENOTSUP) {
> > +        /* Handle it the same way as !bs->drv->bdrv_lockf */
> > +        ret = 0;
> > +    } else if (ret == 0) {
> > +        bs->cur_lock = cmd;
> > +    }
> > +    return ret;
> > +}
> 
> Okay, so the callback is supposed to support going from exclusive to
> shared and vice versa? Noted for the next patches.

Yes.

> 
> > +static int bdrv_lock_image(BlockDriverState *bs)
> > +{
> > +    return bdrv_lock_unlock_image_do(bs, bdrv_get_locking_cmd(bs->open_flags));
> > +}
> > +
> > +static int bdrv_unlock_image(BlockDriverState *bs)
> > +{
> > +    return bdrv_lock_unlock_image_do(bs, BDRV_LOCKF_UNLOCK);
> > +}
> > +
> >  static QemuOptsList bdrv_runtime_opts = {
> >      .name = "bdrv_common",
> >      .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
> > @@ -1003,6 +1047,14 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
> >          goto free_and_fail;
> >      }
> >  
> > +    if (!(open_flags & (BDRV_O_NO_LOCK | BDRV_O_INACTIVE))) {
> > +        ret = bdrv_lock_image(bs);
> > +        if (ret) {
> > +            error_setg(errp, "Failed to lock image");
> > +            goto free_and_fail;
> > +        }
> > +    }
> > +
> >      ret = refresh_total_sectors(bs, bs->total_sectors);
> >      if (ret < 0) {
> >          error_setg_errno(errp, -ret, "Could not refresh total sector count");
> > @@ -2164,6 +2216,7 @@ static void bdrv_close(BlockDriverState *bs)
> >      if (bs->drv) {
> >          BdrvChild *child, *next;
> >  
> > +        bdrv_unlock_image(bs);
> >          bs->drv->bdrv_close(bs);
> >          bs->drv = NULL;
> >  
> > @@ -3187,6 +3240,9 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
> >          error_setg_errno(errp, -ret, "Could not refresh total sector count");
> >          return;
> >      }
> > +    if (!(bs->open_flags & BDRV_O_NO_LOCK)) {
> > +        bdrv_lock_image(bs);
> > +    }
> >  }
> 
> I think the if is unnecessary, bdrv_lock_image() already looks at
> BDRV_O_NO_LOCK.

I intentionally made enum BDRV_O_LOCK_* start from non-zero, so bdrv_lock_image
will call drv->bdrv_lockf even for BDRV_O_NO_LOCK. In the case of
lock-mode=off, I think we should skip that.

> 
> >  void bdrv_invalidate_cache_all(Error **errp)
> > @@ -3229,6 +3285,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
> >      }
> >  
> >      if (setting_flag) {
> > +        ret = bdrv_unlock_image(bs);
> >          bs->open_flags |= BDRV_O_INACTIVE;
> >      }
> >      return 0;
> 
> Kevin

Fam