Re: [Qemu-devel] [PATCH for-2.7 v2 04/17] block: Introduce image file locking

[Fam Zheng <famz@redhat.com>]

On Sat, 04/16 16:22, Denis V. Lunev wrote:
> On 04/15/2016 06:27 AM, Fam Zheng wrote:
> >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                   | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  include/block/block_int.h | 12 ++++++++++++
> >  2 files changed, 54 insertions(+)
> >
> >diff --git a/block.c b/block.c
> >index 1c575e4..7971a25 100644
> >--- a/block.c
> >+++ b/block.c
> >@@ -846,6 +846,34 @@ out:
> >      g_free(gen_node_name);
> >  }
> >+static int bdrv_lock_unlock_image_do(BlockDriverState *bs, bool lock_image)
> >+{
> >+    int cmd = BDRV_LOCKF_UNLOCK;
> >+
> >+    if (bs->image_locked == lock_image) {
> >+        return 0;
> >+    } else if (!bs->drv) {
> >+        return -ENOMEDIUM;
> >+    } else if (!bs->drv->bdrv_lockf) {
> >+        return 0;
> >+    }
> >+    if (lock_image) {
> >+        cmd = bs->open_flags & BDRV_O_RDWR ? BDRV_LOCKF_RWLOCK :
> >+                                             BDRV_LOCKF_ROLOCK;
> >+    }
> >+    return bs->drv->bdrv_lockf(bs, cmd);
> should we handle ENOTSUP specially?
> f.e. this would fire with raw-posix.c on a filesystem which does not support
> locking.
> 
> from my POW this situations is equivalent to  the absence of
> bs->drv->bdrv_lockf

Yes, that's right. Will fix.

> 
> >+}
> >+
> >+static int bdrv_lock_image(BlockDriverState *bs)
> >+{
> >+    return bdrv_lock_unlock_image_do(bs, true);
> >+}
> >+
> >+static int bdrv_unlock_image(BlockDriverState *bs)
> >+{
> >+    return bdrv_lock_unlock_image_do(bs, false);
> >+}
> >+
> >  static QemuOptsList bdrv_runtime_opts = {
> >      .name = "bdrv_common",
> >      .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
> >@@ -995,6 +1023,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");
> >@@ -2144,6 +2180,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;
> >@@ -3230,6 +3267,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);
> >+    }
> >  }
> >  void bdrv_invalidate_cache_all(Error **errp)
> >@@ -3262,6 +3302,7 @@ static int bdrv_inactivate(BlockDriverState *bs)
> >      }
> >      bs->open_flags |= BDRV_O_INACTIVE;
> >+    ret = bdrv_unlock_image(bs);
> I'd better move unlock a line above.
> Though this is personal. This could be useful
> for debugging purposes.

OK, I can move it.

> 
> 
> >      return 0;
> >  }
> >@@ -3981,3 +4022,4 @@ void bdrv_refresh_filename(BlockDriverState *bs)
> >          QDECREF(json);
> >      }
> >  }
> >+
> this hunk is extra

Will remove.

Thanks,
Fam