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