[Qemu-devel] [PATCH for-2.7 v2 00/17] block: Lock images when opening

[Qemu-devel] [PATCH for-2.7 v2 00/17] block: Lock images when opening

[Fam Zheng]

v2: Lock byte 1 in the image itself, no lock file. [Daniel]
    Fix migration (image are not locked in bdrv_open_common if
    BDRV_O_INACTIVE). [Denis]
    Simplify test case fixes because of the above.
    Add lock for RBD.
    Add "-L" option in "qemu-img" and "qemu-nbd" too. [Denis]
    Add test case for image locking.

Too many troubles have been caused by two processes writing to the same image
unexpectedly. This series introduces automatical image locking into QEMU to
avoid such tragedy. With this, the user won't be able to open the image from
two processes (e.g. using qemu-img when the image is attached to the guest).

Underneath is the fcntl syscall that locks the local file, similar to what is
already used in libvirt virtlockd. The difference is that libvirt locks byte 0,
we lock byte 1.  Read only openings are mapped to shared locks.

The alternative locking API, flock(2), cannot protect host NFS mount points, so
it's not used.

Gluster locking is also implemented wrapping glfs_posix_lock in patch 6. It's
only lightly tested.

All other drivers that don't implement .bdrv_lockf are always permissive and
does no checking.

In the future, the intention is that image format drivers that introduce
locking mechanisms could also fit into this API.

Fam Zheng (17):
  block: Add BDRV_O_NO_LOCK
  qapi: Add lock-image in blockdev-add options
  blockdev: Add and parse "lock-image" option for block devices
  block: Introduce image file locking
  raw-posix: Implement .bdrv_lockf
  gluster: Implement .bdrv_lockf
  rbd: Implement image locking
  qemu-io: Add "-L" option for BDRV_O_NO_LOCK
  qemu-img: Add "-L" option to sub commands
  qemu-img: Update documentation of "-L" option
  qemu-nbd: Add "--no-lock/-L" option
  qemu-iotests: 140: Disable image lock for qemu-io access
  qemu-iotests: 046: Move version detection out from verify_io
  qemu-iotests: Wait for QEMU processes before checking image in 091
  qemu-iotests: Disable image lock when checking test image
  block: Turn on image locking by default
  qemu-iotests: Add test case 152 for image locking

 block.c                    |  42 ++++++++
 block/gluster.c            |  30 ++++++
 block/raw-posix.c          |  35 +++++++
 block/rbd.c                |  25 +++++
 blockdev.c                 |   8 ++
 include/block/block.h      |   1 +
 include/block/block_int.h  |  12 +++
 qapi/block-core.json       |   6 +-
 qemu-img-cmds.hx           |  44 ++++-----
 qemu-img.c                 |  90 +++++++++++++----
 qemu-img.texi              |   3 +
 qemu-io.c                  |  22 ++++-
 qemu-nbd.c                 |   6 +-
 qemu-nbd.texi              |   2 +
 tests/qemu-iotests/030     |   2 +-
 tests/qemu-iotests/046     |  22 +++--
 tests/qemu-iotests/091     |   3 +
 tests/qemu-iotests/091.out |   1 +
 tests/qemu-iotests/140     |   2 +-
 tests/qemu-iotests/152     | 106 ++++++++++++++++++++
 tests/qemu-iotests/152.out | 237 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 22 files changed, 645 insertions(+), 55 deletions(-)
 create mode 100755 tests/qemu-iotests/152
 create mode 100644 tests/qemu-iotests/152.out

-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 v2 01/17] block: Add BDRV_O_NO_LOCK

[Fam Zheng]

Later the block layer will automatically lock the images to avoid unexpected
concurrent accesses to the same image, which will easily corrupt the metadata
or user data, unless in some very special cases, like migration.

The exceptional cases like shared storage migration and testing should set
BDRV_O_NO_LOCK the flag indicating that the block layer should skip the
automatic locking of the image, like the old behavior.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/block/block.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/block/block.h b/include/block/block.h
index 3a73137..b803597 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -94,6 +94,7 @@ typedef struct HDGeometry {
                                       select an appropriate protocol driver,
                                       ignoring the format layer */
 #define BDRV_O_NO_IO       0x10000 /* don't initialize for I/O */
+#define BDRV_O_NO_LOCK     0x20000 /* don't lock image file */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
 
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.7 v2 01/17] block: Add BDRV_O_NO_LOCK

[Denis V. Lunev]

On 04/15/2016 06:27 AM, Fam Zheng wrote:
> Later the block layer will automatically lock the images to avoid unexpected
> concurrent accesses to the same image, which will easily corrupt the metadata
> or user data, unless in some very special cases, like migration.
>
> The exceptional cases like shared storage migration and testing should set
> BDRV_O_NO_LOCK the flag indicating that the block layer should skip the
> automatic locking of the image, like the old behavior.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   include/block/block.h | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 3a73137..b803597 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -94,6 +94,7 @@ typedef struct HDGeometry {
>                                         select an appropriate protocol driver,
>                                         ignoring the format layer */
>   #define BDRV_O_NO_IO       0x10000 /* don't initialize for I/O */
> +#define BDRV_O_NO_LOCK     0x20000 /* don't lock image file */
>   
>   #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_NO_FLUSH)
>   
Reviewed-by: Denis V. Lunev <den@openvz.org>

[Qemu-devel] [PATCH for-2.7 v2 02/17] qapi: Add lock-image in blockdev-add options

[Fam Zheng]

To allow overriding the default locking behavior when opening the image.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qapi/block-core.json | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1d09079..2913f3e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2065,6 +2065,9 @@
 # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
 #                 (default: off)
 #
+# @lock-image: #optional whether to lock the image (default: true)
+#              (Since 2.7)
+#
 # Remaining options are determined by the block driver.
 #
 # Since: 1.7
@@ -2082,7 +2085,8 @@
             '*stats-account-invalid': 'bool',
             '*stats-account-failed': 'bool',
             '*stats-intervals': ['int'],
-            '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
+            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
+            '*lock-image': 'bool' },
   'discriminator': 'driver',
   'data': {
       'archipelago':'BlockdevOptionsArchipelago',
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.7 v2 02/17] qapi: Add lock-image in blockdev-add options

[Denis V. Lunev]

On 04/15/2016 06:27 AM, Fam Zheng wrote:
> To allow overriding the default locking behavior when opening the image.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   qapi/block-core.json | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 1d09079..2913f3e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2065,6 +2065,9 @@
>   # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
>   #                 (default: off)
>   #
> +# @lock-image: #optional whether to lock the image (default: true)
> +#              (Since 2.7)
> +#
>   # Remaining options are determined by the block driver.
>   #
>   # Since: 1.7
> @@ -2082,7 +2085,8 @@
>               '*stats-account-invalid': 'bool',
>               '*stats-account-failed': 'bool',
>               '*stats-intervals': ['int'],
> -            '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
> +            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
> +            '*lock-image': 'bool' },
>     'discriminator': 'driver',
>     'data': {
>         'archipelago':'BlockdevOptionsArchipelago',
should we touch qmp-command.hx?

Re: [Qemu-devel] [PATCH for-2.7 v2 02/17] qapi: Add lock-image in blockdev-add options

[Fam Zheng]

On Sat, 04/16 13:48, Denis V. Lunev wrote:
> On 04/15/2016 06:27 AM, Fam Zheng wrote:
> > To allow overriding the default locking behavior when opening the image.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >   qapi/block-core.json | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 1d09079..2913f3e 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -2065,6 +2065,9 @@
> >   # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
> >   #                 (default: off)
> >   #
> > +# @lock-image: #optional whether to lock the image (default: true)
> > +#              (Since 2.7)
> > +#
> >   # Remaining options are determined by the block driver.
> >   #
> >   # Since: 1.7
> > @@ -2082,7 +2085,8 @@
> >               '*stats-account-invalid': 'bool',
> >               '*stats-account-failed': 'bool',
> >               '*stats-intervals': ['int'],
> > -            '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
> > +            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
> > +            '*lock-image': 'bool' },
> >     'discriminator': 'driver',
> >     'data': {
> >         'archipelago':'BlockdevOptionsArchipelago',
> should we touch qmp-command.hx?

These fields have no documentation there.

Fam

[Qemu-devel] [PATCH for-2.7 v2 03/17] blockdev: Add and parse "lock-image" option for block devices

[Fam Zheng]

Honor the locking switch specified in CLI or QMP, and set the open flags for
the image accordingly.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index f1f520a..93bd43e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -382,6 +382,10 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
                return;
             }
         }
+
+        if (!qemu_opt_get_bool(opts, "lock-image", false)) {
+            *bdrv_flags |= BDRV_O_NO_LOCK;
+        }
     }
 
     /* disk I/O throttling */
@@ -4249,6 +4253,10 @@ QemuOptsList qemu_common_drive_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "whether to account for failed I/O operations "
                     "in the statistics",
+        },{
+            .name = "lock-image",
+            .type = QEMU_OPT_BOOL,
+            .help = "whether to lock the image (default: on)",
         },
         { /* end of list */ }
     },
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.7 v2 03/17] blockdev: Add and parse "lock-image" option for block devices

[Denis V. Lunev]

On 04/15/2016 06:27 AM, Fam Zheng wrote:
> Honor the locking switch specified in CLI or QMP, and set the open flags for
> the image accordingly.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   blockdev.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/blockdev.c b/blockdev.c
> index f1f520a..93bd43e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -382,6 +382,10 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
>                  return;
>               }
>           }
> +
> +        if (!qemu_opt_get_bool(opts, "lock-image", false)) {
> +            *bdrv_flags |= BDRV_O_NO_LOCK;
> +        }
>       }
>   
>       /* disk I/O throttling */
> @@ -4249,6 +4253,10 @@ QemuOptsList qemu_common_drive_opts = {
>               .type = QEMU_OPT_BOOL,
>               .help = "whether to account for failed I/O operations "
>                       "in the statistics",
> +        },{
> +            .name = "lock-image",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "whether to lock the image (default: on)",
>           },
>           { /* end of list */ }
>       },
should you add proper clause into qmp-commands.hx?

Re: [Qemu-devel] [PATCH for-2.7 v2 03/17] blockdev: Add and parse "lock-image" option for block devices

[Fam Zheng]

On Sat, 04/16 16:15, Denis V. Lunev wrote:
> On 04/15/2016 06:27 AM, Fam Zheng wrote:
> >Honor the locking switch specified in CLI or QMP, and set the open flags for
> >the image accordingly.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  blockdev.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> >diff --git a/blockdev.c b/blockdev.c
> >index f1f520a..93bd43e 100644
> >--- a/blockdev.c
> >+++ b/blockdev.c
> >@@ -382,6 +382,10 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
> >                 return;
> >              }
> >          }
> >+
> >+        if (!qemu_opt_get_bool(opts, "lock-image", false)) {
> >+            *bdrv_flags |= BDRV_O_NO_LOCK;
> >+        }
> >      }
> >      /* disk I/O throttling */
> >@@ -4249,6 +4253,10 @@ QemuOptsList qemu_common_drive_opts = {
> >              .type = QEMU_OPT_BOOL,
> >              .help = "whether to account for failed I/O operations "
> >                      "in the statistics",
> >+        },{
> >+            .name = "lock-image",
> >+            .type = QEMU_OPT_BOOL,
> >+            .help = "whether to lock the image (default: on)",
> >          },
> >          { /* end of list */ }
> >      },
> should you add proper clause into qmp-commands.hx?

Yes, I'll update this patch.

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

[Fam Zheng]

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);
+}
+
+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);
     return 0;
 }
 
@@ -3981,3 +4022,4 @@ void bdrv_refresh_filename(BlockDriverState *bs)
         QDECREF(json);
     }
 }
+
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 10d8759..ffa30b0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -85,6 +85,12 @@ typedef struct BdrvTrackedRequest {
     struct BdrvTrackedRequest *waiting_for;
 } BdrvTrackedRequest;
 
+typedef enum {
+    BDRV_LOCKF_RWLOCK,
+    BDRV_LOCKF_ROLOCK,
+    BDRV_LOCKF_UNLOCK,
+} BdrvLockfCmd;
+
 struct BlockDriver {
     const char *format_name;
     int instance_size;
@@ -317,6 +323,11 @@ struct BlockDriver {
      */
     void (*bdrv_drain)(BlockDriverState *bs);
 
+    /**
+     * Lock/unlock the image.
+     */
+    int (*bdrv_lockf)(BlockDriverState *bs, BdrvLockfCmd cmd);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
@@ -485,6 +496,7 @@ struct BlockDriverState {
     NotifierWithReturn write_threshold_notifier;
 
     int quiesce_counter;
+    bool image_locked;
 };
 
 struct BlockBackendRootState {
-- 
2.8.0

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

[Denis V. Lunev]

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

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


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

> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 10d8759..ffa30b0 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -85,6 +85,12 @@ typedef struct BdrvTrackedRequest {
>       struct BdrvTrackedRequest *waiting_for;
>   } BdrvTrackedRequest;
>   
> +typedef enum {
> +    BDRV_LOCKF_RWLOCK,
> +    BDRV_LOCKF_ROLOCK,
> +    BDRV_LOCKF_UNLOCK,
> +} BdrvLockfCmd;
> +
>   struct BlockDriver {
>       const char *format_name;
>       int instance_size;
> @@ -317,6 +323,11 @@ struct BlockDriver {
>        */
>       void (*bdrv_drain)(BlockDriverState *bs);
>   
> +    /**
> +     * Lock/unlock the image.
> +     */
> +    int (*bdrv_lockf)(BlockDriverState *bs, BdrvLockfCmd cmd);
> +
>       QLIST_ENTRY(BlockDriver) list;
>   };
>   
> @@ -485,6 +496,7 @@ struct BlockDriverState {
>       NotifierWithReturn write_threshold_notifier;
>   
>       int quiesce_counter;
> +    bool image_locked;
>   };
>   
>   struct BlockBackendRootState {

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

[Fam Zheng]

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

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 4036 bytes --]

On 15.04.2016 05:27, 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(+)

I'm prepared for everyone hating this idea, but I'm just bringing it up
so I can always say I did bring it up.

Heads up: This will be about qcow2 locking again.

Relax, though, it won't be about how much better qcow2 locking is better
than protocol locking.

Now that you know this feel free to drop out.

This patch implements locking by just trying to lock every single BDS
that is being opened. While it may fulfill its purpose, I don't think
that is what we actually want.

What we want is the following: qemu has a BDS graph. It is basically a
forest of trees. It may be a bit more complicated (DAGs instead of
trees), but let's just assume it is.

What we want to protect are leaves in this tree. Every leaf basically
corresponds to a physical resource such as a file or an NBD connection.
Every leaf is driven by a protocol block driver. We want to protect
these physical resources from concurrent access.

Ideally, we can just protect the physical resource itself. This works
for raw-posix, this works for gluster, this works for raw-win32, and
probably some other protocols, too. But I guess it won't work for all
protocols, and even if it does, it would need to be implemented.

But we can protect leaves in the BDS forest by locking non-leaves also:
If you lock a qcow2 node, all of its "file" subtree will be protected;
normally, that's just a single leaf.

Therefore, I think the ideal approach would be for each BDS tree that is
to be created we try to lock all of its leaves, and if that does not
work for some, we walk up the tree and try to lock inner nodes (e.g.
format BDSs which then use format locking) so that the leaves are still
protected even if their protocol does not support that.

This could be implemented like this: Whenever a leaf BDS is created, try
to lock it. If we can't, leave some information to the parent node that
its child could not be locked. Then, the parent will evaluate this
information and try to act upon it. This then recurses up the tree. Or,
well, down the tree, considering that in most natural trees the root is
at the bottom.


We could just implement qcow2 locking on top of this series as it is,
but this would result in qcow2 files being locked even if their files'
protocol nodes have been successfully locked. That would be superfluous
and we'd have all the issues with force-unlocking qcow2 files we have
discussed before.


So what am I saying? I think that it makes sense to consider format
locking as a backup alternative to protocol locking in case the latter
is not possible. I think it is possible to implement both using the same
framework.

I don't think we need to worry about the actual implementation of format
locking now. But I do think having a framework which supports both
format and protocol locking is possible and would be nice to have.

Such a framework would require more effort, however, than the basically
brute-force "just lock everything" method presented in this patch. Don't
get me wrong, this method here works for what it's supposed to do (I
haven't reviewed it yet, though), and it's very reasonable if protocol
locking is all we intend to have. I'm just suggesting that maybe we do
want to have more than that.


All in all, I won't object if the locking framework introduced by this
series is not supposed to and does not work with format locking. It can
always be added later if I really like it so much, and I can definitely
understand if it appears to be too much effort for basically no gain
right now.

As I said above, I just brought this up so I brought it up. :-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Fam Zheng]

On Sun, 04/17 01:29, Max Reitz wrote:
> On 15.04.2016 05:27, 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(+)
> 
> I'm prepared for everyone hating this idea, but I'm just bringing it up
> so I can always say I did bring it up.
> 
> Heads up: This will be about qcow2 locking again.
> 
> Relax, though, it won't be about how much better qcow2 locking is better
> than protocol locking.
> 
> Now that you know this feel free to drop out.
> 
> This patch implements locking by just trying to lock every single BDS
> that is being opened. While it may fulfill its purpose, I don't think
> that is what we actually want.
> 
> What we want is the following: qemu has a BDS graph. It is basically a
> forest of trees. It may be a bit more complicated (DAGs instead of
> trees), but let's just assume it is.
> 
> What we want to protect are leaves in this tree. Every leaf basically
> corresponds to a physical resource such as a file or an NBD connection.
> Every leaf is driven by a protocol block driver. We want to protect
> these physical resources from concurrent access.
> 
> Ideally, we can just protect the physical resource itself. This works
> for raw-posix, this works for gluster, this works for raw-win32, and
> probably some other protocols, too. But I guess it won't work for all
> protocols, and even if it does, it would need to be implemented.
> 
> But we can protect leaves in the BDS forest by locking non-leaves also:
> If you lock a qcow2 node, all of its "file" subtree will be protected;
> normally, that's just a single leaf.
> 
> Therefore, I think the ideal approach would be for each BDS tree that is
> to be created we try to lock all of its leaves, and if that does not
> work for some, we walk up the tree and try to lock inner nodes (e.g.
> format BDSs which then use format locking) so that the leaves are still
> protected even if their protocol does not support that.
> 
> This could be implemented like this: Whenever a leaf BDS is created, try
> to lock it. If we can't, leave some information to the parent node that
> its child could not be locked. Then, the parent will evaluate this
> information and try to act upon it. This then recurses up the tree. Or,
> well, down the tree, considering that in most natural trees the root is
> at the bottom.
> 
> 
> We could just implement qcow2 locking on top of this series as it is,
> but this would result in qcow2 files being locked even if their files'
> protocol nodes have been successfully locked. That would be superfluous
> and we'd have all the issues with force-unlocking qcow2 files we have
> discussed before.
> 
> 
> So what am I saying? I think that it makes sense to consider format
> locking as a backup alternative to protocol locking in case the latter
> is not possible. I think it is possible to implement both using the same
> framework.
> 
> I don't think we need to worry about the actual implementation of format
> locking now. But I do think having a framework which supports both
> format and protocol locking is possible and would be nice to have.
> 
> Such a framework would require more effort, however, than the basically
> brute-force "just lock everything" method presented in this patch. Don't
> get me wrong, this method here works for what it's supposed to do (I
> haven't reviewed it yet, though), and it's very reasonable if protocol
> locking is all we intend to have. I'm just suggesting that maybe we do
> want to have more than that.
> 
> 
> All in all, I won't object if the locking framework introduced by this
> series is not supposed to and does not work with format locking. It can
> always be added later if I really like it so much, and I can definitely
> understand if it appears to be too much effort for basically no gain
> right now.
> 
> As I said above, I just brought this up so I brought it up. :-)

I don't hate this idea, but it is not necessarily much more effort.  We can
always check the underlying file in qcow2's locking implementation, can't we?

    int qcow2_lockf(BlockDriverState *bs, int cmd)
    {
        if ((cmd != BDRV_LOCKF_UNLOCK) && !bdrv_is_locked(bs->file)) {
            return 0;
        }
        ...
    }

The problem with doing this generically in block layer is the chicken-and-egg
problem: it's not safe to just have format probling code or qcow2 driver to
read the image or even writing to the header field for opening it, another
process could be writing to the image already. A challenge with format
locking is the lack of file level atomic operations (cmpxchg on the image
header).

Fam

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

[Denis V. Lunev]

On 04/18/2016 04:33 AM, Fam Zheng wrote:
> On Sun, 04/17 01:29, Max Reitz wrote:
>> On 15.04.2016 05:27, 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(+)
>> I'm prepared for everyone hating this idea, but I'm just bringing it up
>> so I can always say I did bring it up.
>>
>> Heads up: This will be about qcow2 locking again.
>>
>> Relax, though, it won't be about how much better qcow2 locking is better
>> than protocol locking.
>>
>> Now that you know this feel free to drop out.
>>
>> This patch implements locking by just trying to lock every single BDS
>> that is being opened. While it may fulfill its purpose, I don't think
>> that is what we actually want.
>>
>> What we want is the following: qemu has a BDS graph. It is basically a
>> forest of trees. It may be a bit more complicated (DAGs instead of
>> trees), but let's just assume it is.
>>
>> What we want to protect are leaves in this tree. Every leaf basically
>> corresponds to a physical resource such as a file or an NBD connection.
>> Every leaf is driven by a protocol block driver. We want to protect
>> these physical resources from concurrent access.
>>
>> Ideally, we can just protect the physical resource itself. This works
>> for raw-posix, this works for gluster, this works for raw-win32, and
>> probably some other protocols, too. But I guess it won't work for all
>> protocols, and even if it does, it would need to be implemented.
>>
>> But we can protect leaves in the BDS forest by locking non-leaves also:
>> If you lock a qcow2 node, all of its "file" subtree will be protected;
>> normally, that's just a single leaf.
>>
>> Therefore, I think the ideal approach would be for each BDS tree that is
>> to be created we try to lock all of its leaves, and if that does not
>> work for some, we walk up the tree and try to lock inner nodes (e.g.
>> format BDSs which then use format locking) so that the leaves are still
>> protected even if their protocol does not support that.
>>
>> This could be implemented like this: Whenever a leaf BDS is created, try
>> to lock it. If we can't, leave some information to the parent node that
>> its child could not be locked. Then, the parent will evaluate this
>> information and try to act upon it. This then recurses up the tree. Or,
>> well, down the tree, considering that in most natural trees the root is
>> at the bottom.
>>
>>
>> We could just implement qcow2 locking on top of this series as it is,
>> but this would result in qcow2 files being locked even if their files'
>> protocol nodes have been successfully locked. That would be superfluous
>> and we'd have all the issues with force-unlocking qcow2 files we have
>> discussed before.
>>
>>
>> So what am I saying? I think that it makes sense to consider format
>> locking as a backup alternative to protocol locking in case the latter
>> is not possible. I think it is possible to implement both using the same
>> framework.
>>
>> I don't think we need to worry about the actual implementation of format
>> locking now. But I do think having a framework which supports both
>> format and protocol locking is possible and would be nice to have.
>>
>> Such a framework would require more effort, however, than the basically
>> brute-force "just lock everything" method presented in this patch. Don't
>> get me wrong, this method here works for what it's supposed to do (I
>> haven't reviewed it yet, though), and it's very reasonable if protocol
>> locking is all we intend to have. I'm just suggesting that maybe we do
>> want to have more than that.
>>
>>
>> All in all, I won't object if the locking framework introduced by this
>> series is not supposed to and does not work with format locking. It can
>> always be added later if I really like it so much, and I can definitely
>> understand if it appears to be too much effort for basically no gain
>> right now.
>>
>> As I said above, I just brought this up so I brought it up. :-)
> I don't hate this idea, but it is not necessarily much more effort.  We can
> always check the underlying file in qcow2's locking implementation, can't we?
>
>      int qcow2_lockf(BlockDriverState *bs, int cmd)
>      {
>          if ((cmd != BDRV_LOCKF_UNLOCK) && !bdrv_is_locked(bs->file)) {
>              return 0;
>          }
>          ...
>      }
>
> The problem with doing this generically in block layer is the chicken-and-egg
> problem: it's not safe to just have format probling code or qcow2 driver to
> read the image or even writing to the header field for opening it, another
> process could be writing to the image already. A challenge with format
> locking is the lack of file level atomic operations (cmpxchg on the image
> header).
>
> Fam
We should not touch images!

If QEMU will die, f.e. on assert or by power off, we will suffer
a REAL pain to guess whether the lock is taken or not.
flock() and friends is a perfect mechanics for the purpose
as the kernel will drop corresponding locks by magic.

Header changes are good for detecting of unclean stops and
running consistency checks after that, f.e. if we have lazy
ref-counters on.

Pls do not do this inside the image. We have eaten that
stuff in our older products and this is really BAD way to
follow.

Den

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 6159 bytes --]

On 18.04.2016 07:34, Denis V. Lunev wrote:
> On 04/18/2016 04:33 AM, Fam Zheng wrote:
>> On Sun, 04/17 01:29, Max Reitz wrote:
>>> On 15.04.2016 05:27, 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(+)
>>> I'm prepared for everyone hating this idea, but I'm just bringing it up
>>> so I can always say I did bring it up.
>>>
>>> Heads up: This will be about qcow2 locking again.
>>>
>>> Relax, though, it won't be about how much better qcow2 locking is better
>>> than protocol locking.
>>>
>>> Now that you know this feel free to drop out.
>>>
>>> This patch implements locking by just trying to lock every single BDS
>>> that is being opened. While it may fulfill its purpose, I don't think
>>> that is what we actually want.
>>>
>>> What we want is the following: qemu has a BDS graph. It is basically a
>>> forest of trees. It may be a bit more complicated (DAGs instead of
>>> trees), but let's just assume it is.
>>>
>>> What we want to protect are leaves in this tree. Every leaf basically
>>> corresponds to a physical resource such as a file or an NBD connection.
>>> Every leaf is driven by a protocol block driver. We want to protect
>>> these physical resources from concurrent access.
>>>
>>> Ideally, we can just protect the physical resource itself. This works
>>> for raw-posix, this works for gluster, this works for raw-win32, and
>>> probably some other protocols, too. But I guess it won't work for all
>>> protocols, and even if it does, it would need to be implemented.
>>>
>>> But we can protect leaves in the BDS forest by locking non-leaves also:
>>> If you lock a qcow2 node, all of its "file" subtree will be protected;
>>> normally, that's just a single leaf.
>>>
>>> Therefore, I think the ideal approach would be for each BDS tree that is
>>> to be created we try to lock all of its leaves, and if that does not
>>> work for some, we walk up the tree and try to lock inner nodes (e.g.
>>> format BDSs which then use format locking) so that the leaves are still
>>> protected even if their protocol does not support that.
>>>
>>> This could be implemented like this: Whenever a leaf BDS is created, try
>>> to lock it. If we can't, leave some information to the parent node that
>>> its child could not be locked. Then, the parent will evaluate this
>>> information and try to act upon it. This then recurses up the tree. Or,
>>> well, down the tree, considering that in most natural trees the root is
>>> at the bottom.
>>>
>>>
>>> We could just implement qcow2 locking on top of this series as it is,
>>> but this would result in qcow2 files being locked even if their files'
>>> protocol nodes have been successfully locked. That would be superfluous
>>> and we'd have all the issues with force-unlocking qcow2 files we have
>>> discussed before.
>>>
>>>
>>> So what am I saying? I think that it makes sense to consider format
>>> locking as a backup alternative to protocol locking in case the latter
>>> is not possible. I think it is possible to implement both using the same
>>> framework.
>>>
>>> I don't think we need to worry about the actual implementation of format
>>> locking now. But I do think having a framework which supports both
>>> format and protocol locking is possible and would be nice to have.
>>>
>>> Such a framework would require more effort, however, than the basically
>>> brute-force "just lock everything" method presented in this patch. Don't
>>> get me wrong, this method here works for what it's supposed to do (I
>>> haven't reviewed it yet, though), and it's very reasonable if protocol
>>> locking is all we intend to have. I'm just suggesting that maybe we do
>>> want to have more than that.
>>>
>>>
>>> All in all, I won't object if the locking framework introduced by this
>>> series is not supposed to and does not work with format locking. It can
>>> always be added later if I really like it so much, and I can definitely
>>> understand if it appears to be too much effort for basically no gain
>>> right now.
>>>
>>> As I said above, I just brought this up so I brought it up. :-)
>> I don't hate this idea, but it is not necessarily much more effort. 
>> We can
>> always check the underlying file in qcow2's locking implementation,
>> can't we?
>>
>>      int qcow2_lockf(BlockDriverState *bs, int cmd)
>>      {
>>          if ((cmd != BDRV_LOCKF_UNLOCK) && !bdrv_is_locked(bs->file)) {
>>              return 0;
>>          }
>>          ...
>>      }
>>
>> The problem with doing this generically in block layer is the
>> chicken-and-egg
>> problem: it's not safe to just have format probling code or qcow2
>> driver to
>> read the image or even writing to the header field for opening it,
>> another
>> process could be writing to the image already. A challenge with format
>> locking is the lack of file level atomic operations (cmpxchg on the image
>> header).
>>
>> Fam
> We should not touch images!
> 
> If QEMU will die, f.e. on assert or by power off, we will suffer
> a REAL pain to guess whether the lock is taken or not.
> flock() and friends is a perfect mechanics for the purpose
> as the kernel will drop corresponding locks by magic.
> 
> Header changes are good for detecting of unclean stops and
> running consistency checks after that, f.e. if we have lazy
> ref-counters on.
> 
> Pls do not do this inside the image. We have eaten that
> stuff in our older products and this is really BAD way to
> follow.

My suggestion was to use format locking only when protocol locking is
not available. And you can always switch it off manually.

Also, the bulk of my argument was not to implement format locking right
now, but to keep in mind that maybe we do want to implement it later.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Denis V. Lunev]

On 04/19/2016 10:14 PM, Max Reitz wrote:
> On 18.04.2016 07:34, Denis V. Lunev wrote:
>> On 04/18/2016 04:33 AM, Fam Zheng wrote:
>>> On Sun, 04/17 01:29, Max Reitz wrote:
>>>> On 15.04.2016 05:27, 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(+)
>>>> I'm prepared for everyone hating this idea, but I'm just bringing it up
>>>> so I can always say I did bring it up.
>>>>
>>>> Heads up: This will be about qcow2 locking again.
>>>>
>>>> Relax, though, it won't be about how much better qcow2 locking is better
>>>> than protocol locking.
>>>>
>>>> Now that you know this feel free to drop out.
>>>>
>>>> This patch implements locking by just trying to lock every single BDS
>>>> that is being opened. While it may fulfill its purpose, I don't think
>>>> that is what we actually want.
>>>>
>>>> What we want is the following: qemu has a BDS graph. It is basically a
>>>> forest of trees. It may be a bit more complicated (DAGs instead of
>>>> trees), but let's just assume it is.
>>>>
>>>> What we want to protect are leaves in this tree. Every leaf basically
>>>> corresponds to a physical resource such as a file or an NBD connection.
>>>> Every leaf is driven by a protocol block driver. We want to protect
>>>> these physical resources from concurrent access.
>>>>
>>>> Ideally, we can just protect the physical resource itself. This works
>>>> for raw-posix, this works for gluster, this works for raw-win32, and
>>>> probably some other protocols, too. But I guess it won't work for all
>>>> protocols, and even if it does, it would need to be implemented.
>>>>
>>>> But we can protect leaves in the BDS forest by locking non-leaves also:
>>>> If you lock a qcow2 node, all of its "file" subtree will be protected;
>>>> normally, that's just a single leaf.
>>>>
>>>> Therefore, I think the ideal approach would be for each BDS tree that is
>>>> to be created we try to lock all of its leaves, and if that does not
>>>> work for some, we walk up the tree and try to lock inner nodes (e.g.
>>>> format BDSs which then use format locking) so that the leaves are still
>>>> protected even if their protocol does not support that.
>>>>
>>>> This could be implemented like this: Whenever a leaf BDS is created, try
>>>> to lock it. If we can't, leave some information to the parent node that
>>>> its child could not be locked. Then, the parent will evaluate this
>>>> information and try to act upon it. This then recurses up the tree. Or,
>>>> well, down the tree, considering that in most natural trees the root is
>>>> at the bottom.
>>>>
>>>>
>>>> We could just implement qcow2 locking on top of this series as it is,
>>>> but this would result in qcow2 files being locked even if their files'
>>>> protocol nodes have been successfully locked. That would be superfluous
>>>> and we'd have all the issues with force-unlocking qcow2 files we have
>>>> discussed before.
>>>>
>>>>
>>>> So what am I saying? I think that it makes sense to consider format
>>>> locking as a backup alternative to protocol locking in case the latter
>>>> is not possible. I think it is possible to implement both using the same
>>>> framework.
>>>>
>>>> I don't think we need to worry about the actual implementation of format
>>>> locking now. But I do think having a framework which supports both
>>>> format and protocol locking is possible and would be nice to have.
>>>>
>>>> Such a framework would require more effort, however, than the basically
>>>> brute-force "just lock everything" method presented in this patch. Don't
>>>> get me wrong, this method here works for what it's supposed to do (I
>>>> haven't reviewed it yet, though), and it's very reasonable if protocol
>>>> locking is all we intend to have. I'm just suggesting that maybe we do
>>>> want to have more than that.
>>>>
>>>>
>>>> All in all, I won't object if the locking framework introduced by this
>>>> series is not supposed to and does not work with format locking. It can
>>>> always be added later if I really like it so much, and I can definitely
>>>> understand if it appears to be too much effort for basically no gain
>>>> right now.
>>>>
>>>> As I said above, I just brought this up so I brought it up. :-)
>>> I don't hate this idea, but it is not necessarily much more effort.
>>> We can
>>> always check the underlying file in qcow2's locking implementation,
>>> can't we?
>>>
>>>       int qcow2_lockf(BlockDriverState *bs, int cmd)
>>>       {
>>>           if ((cmd != BDRV_LOCKF_UNLOCK) && !bdrv_is_locked(bs->file)) {
>>>               return 0;
>>>           }
>>>           ...
>>>       }
>>>
>>> The problem with doing this generically in block layer is the
>>> chicken-and-egg
>>> problem: it's not safe to just have format probling code or qcow2
>>> driver to
>>> read the image or even writing to the header field for opening it,
>>> another
>>> process could be writing to the image already. A challenge with format
>>> locking is the lack of file level atomic operations (cmpxchg on the image
>>> header).
>>>
>>> Fam
>> We should not touch images!
>>
>> If QEMU will die, f.e. on assert or by power off, we will suffer
>> a REAL pain to guess whether the lock is taken or not.
>> flock() and friends is a perfect mechanics for the purpose
>> as the kernel will drop corresponding locks by magic.
>>
>> Header changes are good for detecting of unclean stops and
>> running consistency checks after that, f.e. if we have lazy
>> ref-counters on.
>>
>> Pls do not do this inside the image. We have eaten that
>> stuff in our older products and this is really BAD way to
>> follow.
> My suggestion was to use format locking only when protocol locking is
> not available. And you can always switch it off manually.
>
> Also, the bulk of my argument was not to implement format locking right
> now, but to keep in mind that maybe we do want to implement it later.
>
> Max
>
OK. this sounds sane and fair if we could start with the
current approach and extend it later on with format
locking.

Den

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 5284 bytes --]

On 18.04.2016 03:33, Fam Zheng wrote:
> On Sun, 04/17 01:29, Max Reitz wrote:
>> On 15.04.2016 05:27, 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(+)
>>
>> I'm prepared for everyone hating this idea, but I'm just bringing it up
>> so I can always say I did bring it up.
>>
>> Heads up: This will be about qcow2 locking again.
>>
>> Relax, though, it won't be about how much better qcow2 locking is better
>> than protocol locking.
>>
>> Now that you know this feel free to drop out.
>>
>> This patch implements locking by just trying to lock every single BDS
>> that is being opened. While it may fulfill its purpose, I don't think
>> that is what we actually want.
>>
>> What we want is the following: qemu has a BDS graph. It is basically a
>> forest of trees. It may be a bit more complicated (DAGs instead of
>> trees), but let's just assume it is.
>>
>> What we want to protect are leaves in this tree. Every leaf basically
>> corresponds to a physical resource such as a file or an NBD connection.
>> Every leaf is driven by a protocol block driver. We want to protect
>> these physical resources from concurrent access.
>>
>> Ideally, we can just protect the physical resource itself. This works
>> for raw-posix, this works for gluster, this works for raw-win32, and
>> probably some other protocols, too. But I guess it won't work for all
>> protocols, and even if it does, it would need to be implemented.
>>
>> But we can protect leaves in the BDS forest by locking non-leaves also:
>> If you lock a qcow2 node, all of its "file" subtree will be protected;
>> normally, that's just a single leaf.
>>
>> Therefore, I think the ideal approach would be for each BDS tree that is
>> to be created we try to lock all of its leaves, and if that does not
>> work for some, we walk up the tree and try to lock inner nodes (e.g.
>> format BDSs which then use format locking) so that the leaves are still
>> protected even if their protocol does not support that.
>>
>> This could be implemented like this: Whenever a leaf BDS is created, try
>> to lock it. If we can't, leave some information to the parent node that
>> its child could not be locked. Then, the parent will evaluate this
>> information and try to act upon it. This then recurses up the tree. Or,
>> well, down the tree, considering that in most natural trees the root is
>> at the bottom.
>>
>>
>> We could just implement qcow2 locking on top of this series as it is,
>> but this would result in qcow2 files being locked even if their files'
>> protocol nodes have been successfully locked. That would be superfluous
>> and we'd have all the issues with force-unlocking qcow2 files we have
>> discussed before.
>>
>>
>> So what am I saying? I think that it makes sense to consider format
>> locking as a backup alternative to protocol locking in case the latter
>> is not possible. I think it is possible to implement both using the same
>> framework.
>>
>> I don't think we need to worry about the actual implementation of format
>> locking now. But I do think having a framework which supports both
>> format and protocol locking is possible and would be nice to have.
>>
>> Such a framework would require more effort, however, than the basically
>> brute-force "just lock everything" method presented in this patch. Don't
>> get me wrong, this method here works for what it's supposed to do (I
>> haven't reviewed it yet, though), and it's very reasonable if protocol
>> locking is all we intend to have. I'm just suggesting that maybe we do
>> want to have more than that.
>>
>>
>> All in all, I won't object if the locking framework introduced by this
>> series is not supposed to and does not work with format locking. It can
>> always be added later if I really like it so much, and I can definitely
>> understand if it appears to be too much effort for basically no gain
>> right now.
>>
>> As I said above, I just brought this up so I brought it up. :-)
> 
> I don't hate this idea, but it is not necessarily much more effort.  We can
> always check the underlying file in qcow2's locking implementation, can't we?
> 
>     int qcow2_lockf(BlockDriverState *bs, int cmd)
>     {
>         if ((cmd != BDRV_LOCKF_UNLOCK) && !bdrv_is_locked(bs->file)) {
>             return 0;
>         }
>         ...
>     }

Good point. I like that.

> The problem with doing this generically in block layer is the chicken-and-egg
> problem: it's not safe to just have format probling code or qcow2 driver to
> read the image or even writing to the header field for opening it, another
> process could be writing to the image already. A challenge with format
> locking is the lack of file level atomic operations (cmpxchg on the image
> header).

Well, I'm not sure whether we'd need to format-lock an image for
probing, but with the above, we'd circumvent the whole issue anyway.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Laszlo Ersek]

On 04/15/16 05:27, 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);
> +}
> +
> +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);
>      return 0;
>  }
>  
> @@ -3981,3 +4022,4 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>          QDECREF(json);
>      }
>  }
> +
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 10d8759..ffa30b0 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -85,6 +85,12 @@ typedef struct BdrvTrackedRequest {
>      struct BdrvTrackedRequest *waiting_for;
>  } BdrvTrackedRequest;
>  
> +typedef enum {
> +    BDRV_LOCKF_RWLOCK,
> +    BDRV_LOCKF_ROLOCK,
> +    BDRV_LOCKF_UNLOCK,
> +} BdrvLockfCmd;
> +
>  struct BlockDriver {
>      const char *format_name;
>      int instance_size;
> @@ -317,6 +323,11 @@ struct BlockDriver {
>       */
>      void (*bdrv_drain)(BlockDriverState *bs);
>  
> +    /**
> +     * Lock/unlock the image.
> +     */
> +    int (*bdrv_lockf)(BlockDriverState *bs, BdrvLockfCmd cmd);
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> @@ -485,6 +496,7 @@ struct BlockDriverState {
>      NotifierWithReturn write_threshold_notifier;
>  
>      int quiesce_counter;
> +    bool image_locked;
>  };
>  
>  struct BlockBackendRootState {
> 

I'd like to raise one point which I think may not have been, yet (after
briefly skimming the v1 / v2 comments). Sorry if this has been discussed
already.

IIUC, the idea is that "protocols" (in the block layer sense) implement
the lockf method, and then bdrv_open_common() automatically locks image
files, if the lockf method is available, and if various settings
(cmdline options etc) don't request otherwise.

I tried to see if this series modifies -- for example --
raw_reopen_commit() and raw_reopen_abort(), in "block/raw-posix.c". Or,
if it modifies bdrv_reopen_multiple(), in "block.c". It doesn't seem to.

Those functions are relevant for the following reason. Given the
following chain of references:

  file descriptor --> file description --> file

an fcntl() lock is associated with the file. However, the fcntl() lock
held by the process on the file is dropped if the process closes *any*
file descriptor that points (through the same or another file
description) to the file. From
<http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html>:

    All locks associated with a file for a given process shall be
    removed when a file descriptor for that file is closed by that
    process [...]

>From <http://pubs.opengroup.org/onlinepubs/9699919799/functions/close.html>:

    All outstanding record locks owned by the process on the file
    associated with the file descriptor shall be removed (that is,
    unlocked).

>From <http://man7.org/linux/man-pages/man2/fcntl.2.html>:

    If a process closes any file descriptor referring to a file, then
    all of the process's locks on that file are released, regardless of
    the file descriptor(s) on which the locks were obtained.

The bdrv_reopen_multiple() function reopens a bunch of image files.
Backed by the raw-posix protocol driver, this seems to boil down to a
series of (i) fcntl(F_DUPFD_CLOEXEC), and/or (ii) dup(), and/or (iii)
qemu_open() calls, in raw_reopen_prepare(). The result is stored in
"raw_s->fd" every time.

(In the first two cases, the file description will be shared, in the
third case, the file will be shared, between "s->fd" and "raw_s->fd".)

Assume that one of the raw_reopen_prepare() calls fails. Then
bdrv_reopen_multiple() will roll back the work done thus far, calling
raw_reopen_abort() on the initial subset of image files. This results in
"raw_s->fd" being passed to close(), which is when the lock
(conceptually held for "s->fd") is dropped for good.

If all of the raw_reopen_prepare() calls succeed, then a series of
raw_reopen_commit() calls will occur. That has the same effect: "s->fd"
is passed to close(), which drops the lock for "raw_s->fd" too (which is
supposed to be used for accessing the file, going forward).

Sorry if this is already handled in the series, I couldn't find it.

Thanks
Laszlo

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

[Fam Zheng]

On Tue, 04/26 01:55, Laszlo Ersek wrote:
> On 04/15/16 05:27, 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);
> > +}
> > +
> > +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);
> >      return 0;
> >  }
> >  
> > @@ -3981,3 +4022,4 @@ void bdrv_refresh_filename(BlockDriverState *bs)
> >          QDECREF(json);
> >      }
> >  }
> > +
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 10d8759..ffa30b0 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -85,6 +85,12 @@ typedef struct BdrvTrackedRequest {
> >      struct BdrvTrackedRequest *waiting_for;
> >  } BdrvTrackedRequest;
> >  
> > +typedef enum {
> > +    BDRV_LOCKF_RWLOCK,
> > +    BDRV_LOCKF_ROLOCK,
> > +    BDRV_LOCKF_UNLOCK,
> > +} BdrvLockfCmd;
> > +
> >  struct BlockDriver {
> >      const char *format_name;
> >      int instance_size;
> > @@ -317,6 +323,11 @@ struct BlockDriver {
> >       */
> >      void (*bdrv_drain)(BlockDriverState *bs);
> >  
> > +    /**
> > +     * Lock/unlock the image.
> > +     */
> > +    int (*bdrv_lockf)(BlockDriverState *bs, BdrvLockfCmd cmd);
> > +
> >      QLIST_ENTRY(BlockDriver) list;
> >  };
> >  
> > @@ -485,6 +496,7 @@ struct BlockDriverState {
> >      NotifierWithReturn write_threshold_notifier;
> >  
> >      int quiesce_counter;
> > +    bool image_locked;
> >  };
> >  
> >  struct BlockBackendRootState {
> > 
> 
> I'd like to raise one point which I think may not have been, yet (after
> briefly skimming the v1 / v2 comments). Sorry if this has been discussed
> already.
> 
> IIUC, the idea is that "protocols" (in the block layer sense) implement
> the lockf method, and then bdrv_open_common() automatically locks image
> files, if the lockf method is available, and if various settings
> (cmdline options etc) don't request otherwise.
> 
> I tried to see if this series modifies -- for example --
> raw_reopen_commit() and raw_reopen_abort(), in "block/raw-posix.c". Or,
> if it modifies bdrv_reopen_multiple(), in "block.c". It doesn't seem to.
> 
> Those functions are relevant for the following reason. Given the
> following chain of references:
> 
>   file descriptor --> file description --> file
> 
> an fcntl() lock is associated with the file. However, the fcntl() lock
> held by the process on the file is dropped if the process closes *any*
> file descriptor that points (through the same or another file
> description) to the file. From
> <http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html>:
> 
>     All locks associated with a file for a given process shall be
>     removed when a file descriptor for that file is closed by that
>     process [...]
> 
> From <http://pubs.opengroup.org/onlinepubs/9699919799/functions/close.html>:
> 
>     All outstanding record locks owned by the process on the file
>     associated with the file descriptor shall be removed (that is,
>     unlocked).
> 
> From <http://man7.org/linux/man-pages/man2/fcntl.2.html>:
> 
>     If a process closes any file descriptor referring to a file, then
>     all of the process's locks on that file are released, regardless of
>     the file descriptor(s) on which the locks were obtained.
> 
> The bdrv_reopen_multiple() function reopens a bunch of image files.
> Backed by the raw-posix protocol driver, this seems to boil down to a
> series of (i) fcntl(F_DUPFD_CLOEXEC), and/or (ii) dup(), and/or (iii)
> qemu_open() calls, in raw_reopen_prepare(). The result is stored in
> "raw_s->fd" every time.
> 
> (In the first two cases, the file description will be shared, in the
> third case, the file will be shared, between "s->fd" and "raw_s->fd".)
> 
> Assume that one of the raw_reopen_prepare() calls fails. Then
> bdrv_reopen_multiple() will roll back the work done thus far, calling
> raw_reopen_abort() on the initial subset of image files. This results in
> "raw_s->fd" being passed to close(), which is when the lock
> (conceptually held for "s->fd") is dropped for good.
> 
> If all of the raw_reopen_prepare() calls succeed, then a series of
> raw_reopen_commit() calls will occur. That has the same effect: "s->fd"
> is passed to close(), which drops the lock for "raw_s->fd" too (which is
> supposed to be used for accessing the file, going forward).
> 

Yes, this is a good catch! I'll take care of it in next version, and add some
tests.

Thanks!

Fam

[Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Fam Zheng]

virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
the intervene.

Suggested-by: "Daniel P. Berrange" <berrange@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/raw-posix.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 906d5c9..3a2c17f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -35,6 +35,7 @@
 #include "raw-aio.h"
 #include "qapi/util.h"
 #include "qapi/qmp/qstring.h"
+#include "glib.h"
 
 #if defined(__APPLE__) && (__MACH__)
 #include <paths.h>
@@ -397,6 +398,38 @@ static void raw_attach_aio_context(BlockDriverState *bs,
 #endif
 }
 
+static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
+{
+
+    BDRVRawState *s = bs->opaque;
+    int ret;
+    struct flock fl = (struct flock) {
+        .l_whence  = SEEK_SET,
+        /* Locking byte 1 avoids interfereing with virtlockd. */
+        .l_start = 1,
+        .l_len = 1,
+    };
+
+    switch (cmd) {
+    case BDRV_LOCKF_RWLOCK:
+        fl.l_type = F_WRLCK;
+        break;
+    case BDRV_LOCKF_ROLOCK:
+        fl.l_type = F_RDLCK;
+        break;
+    case BDRV_LOCKF_UNLOCK:
+        fl.l_type = F_UNLCK;
+        break;
+    default:
+        abort();
+    }
+    ret = fcntl(s->fd, F_SETLK, &fl);
+    if (ret) {
+        ret = -errno;
+    }
+    return ret;
+}
+
 #ifdef CONFIG_LINUX_AIO
 static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags)
 {
@@ -1960,6 +1993,8 @@ BlockDriver bdrv_file = {
     .bdrv_detach_aio_context = raw_detach_aio_context,
     .bdrv_attach_aio_context = raw_attach_aio_context,
 
+    .bdrv_lockf = raw_lockf,
+
     .create_opts = &raw_create_opts,
 };
 
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Denis V. Lunev]

On 04/15/2016 06:27 AM, Fam Zheng wrote:
> virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> the intervene.
>
> Suggested-by: "Daniel P. Berrange" <berrange@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/raw-posix.c | 35 +++++++++++++++++++++++++++++++++++
>   1 file changed, 35 insertions(+)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 906d5c9..3a2c17f 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -35,6 +35,7 @@
>   #include "raw-aio.h"
>   #include "qapi/util.h"
>   #include "qapi/qmp/qstring.h"
> +#include "glib.h"
>   
>   #if defined(__APPLE__) && (__MACH__)
>   #include <paths.h>
> @@ -397,6 +398,38 @@ static void raw_attach_aio_context(BlockDriverState *bs,
>   #endif
>   }
>   
> +static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> +{
> +
> +    BDRVRawState *s = bs->opaque;
> +    int ret;
> +    struct flock fl = (struct flock) {
> +        .l_whence  = SEEK_SET,
> +        /* Locking byte 1 avoids interfereing with virtlockd. */
> +        .l_start = 1,
> +        .l_len = 1,
> +    };
> +
> +    switch (cmd) {
> +    case BDRV_LOCKF_RWLOCK:
> +        fl.l_type = F_WRLCK;
> +        break;
> +    case BDRV_LOCKF_ROLOCK:
> +        fl.l_type = F_RDLCK;
> +        break;
> +    case BDRV_LOCKF_UNLOCK:
> +        fl.l_type = F_UNLCK;
> +        break;
> +    default:
> +        abort();
> +    }
> +    ret = fcntl(s->fd, F_SETLK, &fl);
> +    if (ret) {
> +        ret = -errno;
> +    }
> +    return ret;
> +}
> +
>   #ifdef CONFIG_LINUX_AIO
>   static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags)
>   {
> @@ -1960,6 +1993,8 @@ BlockDriver bdrv_file = {
>       .bdrv_detach_aio_context = raw_detach_aio_context,
>       .bdrv_attach_aio_context = raw_attach_aio_context,
>   
> +    .bdrv_lockf = raw_lockf,
> +
>       .create_opts = &raw_create_opts,
>   };
>   
would it be better to use

        int flock(int fd, int operation);

DESCRIPTION
        Apply or remove an advisory lock on the open file specified by 
fd.  The
        argument operation is one of the following:

            LOCK_SH  Place a shared lock.  More than one  process may  
hold  a
                     shared lock for a given file at a given time.

            LOCK_EX  Place  an  exclusive  lock.   Only one process may 
hold an
                     exclusive lock for a given file at a given time.

            LOCK_UN  Remove an existing lock held by this process.

for this purpose?

Sorry that missed this point in the initial review...
We will not intersect with libvirt for the case.

Den

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Fam Zheng]

On Sat, 04/16 16:29, Denis V. Lunev wrote:
> On 04/15/2016 06:27 AM, Fam Zheng wrote:
> >virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> >the intervene.
> >
> >Suggested-by: "Daniel P. Berrange" <berrange@redhat.com>
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  block/raw-posix.c | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> >diff --git a/block/raw-posix.c b/block/raw-posix.c
> >index 906d5c9..3a2c17f 100644
> >--- a/block/raw-posix.c
> >+++ b/block/raw-posix.c
> >@@ -35,6 +35,7 @@
> >  #include "raw-aio.h"
> >  #include "qapi/util.h"
> >  #include "qapi/qmp/qstring.h"
> >+#include "glib.h"
> >  #if defined(__APPLE__) && (__MACH__)
> >  #include <paths.h>
> >@@ -397,6 +398,38 @@ static void raw_attach_aio_context(BlockDriverState *bs,
> >  #endif
> >  }
> >+static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> >+{
> >+
> >+    BDRVRawState *s = bs->opaque;
> >+    int ret;
> >+    struct flock fl = (struct flock) {
> >+        .l_whence  = SEEK_SET,
> >+        /* Locking byte 1 avoids interfereing with virtlockd. */
> >+        .l_start = 1,
> >+        .l_len = 1,
> >+    };
> >+
> >+    switch (cmd) {
> >+    case BDRV_LOCKF_RWLOCK:
> >+        fl.l_type = F_WRLCK;
> >+        break;
> >+    case BDRV_LOCKF_ROLOCK:
> >+        fl.l_type = F_RDLCK;
> >+        break;
> >+    case BDRV_LOCKF_UNLOCK:
> >+        fl.l_type = F_UNLCK;
> >+        break;
> >+    default:
> >+        abort();
> >+    }
> >+    ret = fcntl(s->fd, F_SETLK, &fl);
> >+    if (ret) {
> >+        ret = -errno;
> >+    }
> >+    return ret;
> >+}
> >+
> >  #ifdef CONFIG_LINUX_AIO
> >  static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags)
> >  {
> >@@ -1960,6 +1993,8 @@ BlockDriver bdrv_file = {
> >      .bdrv_detach_aio_context = raw_detach_aio_context,
> >      .bdrv_attach_aio_context = raw_attach_aio_context,
> >+    .bdrv_lockf = raw_lockf,
> >+
> >      .create_opts = &raw_create_opts,
> >  };
> would it be better to use
> 
>        int flock(int fd, int operation);
> 
> DESCRIPTION
>        Apply or remove an advisory lock on the open file specified by fd.
> The
>        argument operation is one of the following:
> 
>            LOCK_SH  Place a shared lock.  More than one  process may  hold
> a
>                     shared lock for a given file at a given time.
> 
>            LOCK_EX  Place  an  exclusive  lock.   Only one process may hold
> an
>                     exclusive lock for a given file at a given time.
> 
>            LOCK_UN  Remove an existing lock held by this process.
> 
> for this purpose?
> 
> Sorry that missed this point in the initial review...
> We will not intersect with libvirt for the case.
> 

As noted in the cover letter, flock() is nop on NFS mount points on Linux, so
fcntl is safer.

Fam

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Denis V. Lunev]

On 04/18/2016 04:12 AM, Fam Zheng wrote:
> On Sat, 04/16 16:29, Denis V. Lunev wrote:
>> On 04/15/2016 06:27 AM, Fam Zheng wrote:
>>> virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
>>> the intervene.
>>>
>>> Suggested-by: "Daniel P. Berrange" <berrange@redhat.com>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>   block/raw-posix.c | 35 +++++++++++++++++++++++++++++++++++
>>>   1 file changed, 35 insertions(+)
>>>
>>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>>> index 906d5c9..3a2c17f 100644
>>> --- a/block/raw-posix.c
>>> +++ b/block/raw-posix.c
>>> @@ -35,6 +35,7 @@
>>>   #include "raw-aio.h"
>>>   #include "qapi/util.h"
>>>   #include "qapi/qmp/qstring.h"
>>> +#include "glib.h"
>>>   #if defined(__APPLE__) && (__MACH__)
>>>   #include <paths.h>
>>> @@ -397,6 +398,38 @@ static void raw_attach_aio_context(BlockDriverState *bs,
>>>   #endif
>>>   }
>>> +static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
>>> +{
>>> +
>>> +    BDRVRawState *s = bs->opaque;
>>> +    int ret;
>>> +    struct flock fl = (struct flock) {
>>> +        .l_whence  = SEEK_SET,
>>> +        /* Locking byte 1 avoids interfereing with virtlockd. */
>>> +        .l_start = 1,
>>> +        .l_len = 1,
>>> +    };
>>> +
>>> +    switch (cmd) {
>>> +    case BDRV_LOCKF_RWLOCK:
>>> +        fl.l_type = F_WRLCK;
>>> +        break;
>>> +    case BDRV_LOCKF_ROLOCK:
>>> +        fl.l_type = F_RDLCK;
>>> +        break;
>>> +    case BDRV_LOCKF_UNLOCK:
>>> +        fl.l_type = F_UNLCK;
>>> +        break;
>>> +    default:
>>> +        abort();
>>> +    }
>>> +    ret = fcntl(s->fd, F_SETLK, &fl);
>>> +    if (ret) {
>>> +        ret = -errno;
>>> +    }
>>> +    return ret;
>>> +}
>>> +
>>>   #ifdef CONFIG_LINUX_AIO
>>>   static int raw_set_aio(void **aio_ctx, int *use_aio, int bdrv_flags)
>>>   {
>>> @@ -1960,6 +1993,8 @@ BlockDriver bdrv_file = {
>>>       .bdrv_detach_aio_context = raw_detach_aio_context,
>>>       .bdrv_attach_aio_context = raw_attach_aio_context,
>>> +    .bdrv_lockf = raw_lockf,
>>> +
>>>       .create_opts = &raw_create_opts,
>>>   };
>> would it be better to use
>>
>>         int flock(int fd, int operation);
>>
>> DESCRIPTION
>>         Apply or remove an advisory lock on the open file specified by fd.
>> The
>>         argument operation is one of the following:
>>
>>             LOCK_SH  Place a shared lock.  More than one  process may  hold
>> a
>>                      shared lock for a given file at a given time.
>>
>>             LOCK_EX  Place  an  exclusive  lock.   Only one process may hold
>> an
>>                      exclusive lock for a given file at a given time.
>>
>>             LOCK_UN  Remove an existing lock held by this process.
>>
>> for this purpose?
>>
>> Sorry that missed this point in the initial review...
>> We will not intersect with libvirt for the case.
>>
> As noted in the cover letter, flock() is nop on NFS mount points on Linux, so
> fcntl is safer.
>
> Fam
>
that seems STRANGE to me. For the time being flock() was
working for Linux NFS server. Here is the implementation.

int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
{
         struct inode *inode = filp->f_mapping->host;
         int is_local = 0;

         dprintk("NFS: flock(%pD2, t=%x, fl=%x)\n",
                         filp, fl->fl_type, fl->fl_flags);

         if (!(fl->fl_flags & FL_FLOCK))
                 return -ENOLCK;

         /*
          * The NFSv4 protocol doesn't support LOCK_MAND, which is not 
part of
          * any standard. In principle we might be able to support LOCK_MAND
          * on NFSv2/3 since NLMv3/4 support DOS share modes, but for 
now the
          * NFS code is not set up for it.
          */
         if (fl->fl_type & LOCK_MAND)
                 return -EINVAL;

         if (NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FLOCK)
                 is_local = 1;

         /* We're simulating flock() locks using posix locks on the 
server */
         if (fl->fl_type == F_UNLCK)
                 return do_unlk(filp, cmd, fl, is_local);
         return do_setlk(filp, cmd, fl, is_local);
}

Den

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Daniel P. Berrange]

On Mon, Apr 18, 2016 at 09:12:44AM +0800, Fam Zheng wrote:
> On Sat, 04/16 16:29, Denis V. Lunev wrote:
> > On 04/15/2016 06:27 AM, Fam Zheng wrote:
> > >virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> > >the intervene.
> > >
> > >Suggested-by: "Daniel P. Berrange" <berrange@redhat.com>
> > >Signed-off-by: Fam Zheng <famz@redhat.com>
> > >---
> > >  block/raw-posix.c | 35 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 35 insertions(+)


> > >@@ -1960,6 +1993,8 @@ BlockDriver bdrv_file = {
> > >      .bdrv_detach_aio_context = raw_detach_aio_context,
> > >      .bdrv_attach_aio_context = raw_attach_aio_context,
> > >+    .bdrv_lockf = raw_lockf,
> > >+
> > >      .create_opts = &raw_create_opts,
> > >  };
> > would it be better to use
> > 
> >        int flock(int fd, int operation);
> > 
> > DESCRIPTION
> >        Apply or remove an advisory lock on the open file specified by fd.
> > The
> >        argument operation is one of the following:
> > 
> >            LOCK_SH  Place a shared lock.  More than one  process may  hold
> > a
> >                     shared lock for a given file at a given time.
> > 
> >            LOCK_EX  Place  an  exclusive  lock.   Only one process may hold
> > an
> >                     exclusive lock for a given file at a given time.
> > 
> >            LOCK_UN  Remove an existing lock held by this process.
> > 
> > for this purpose?
> > 
> > Sorry that missed this point in the initial review...
> > We will not intersect with libvirt for the case.
> 
> As noted in the cover letter, flock() is nop on NFS mount points on Linux, so
> fcntl is safer.

Actually on Modern linux flock is implemented in terms of fcntl
on Linux. Flock does not do byte-range locking so by using
flock on NFS, IIUC you'll get a fcntl lock for the entire file
range on the server, which will clash with the fcntl lock that
virtlogd has acquired. On older linux flock is a no-op on NFS.
Other UNIX have varying degrees of usable for flock on NFS. So
in general fcntl is a better bet for NFS compatibility and will
not clash with libvirt.

The only minor issue is that some versions of OCFS do not support
fcntl locks at all, only flock.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Denis V. Lunev]

On 04/18/2016 12:34 PM, Daniel P. Berrange wrote:
> On Mon, Apr 18, 2016 at 09:12:44AM +0800, Fam Zheng wrote:
>> On Sat, 04/16 16:29, Denis V. Lunev wrote:
>>> On 04/15/2016 06:27 AM, Fam Zheng wrote:
>>>> virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
>>>> the intervene.
>>>>
>>>> Suggested-by: "Daniel P. Berrange" <berrange@redhat.com>
>>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>>> ---
>>>>   block/raw-posix.c | 35 +++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 35 insertions(+)
>
>>>> @@ -1960,6 +1993,8 @@ BlockDriver bdrv_file = {
>>>>       .bdrv_detach_aio_context = raw_detach_aio_context,
>>>>       .bdrv_attach_aio_context = raw_attach_aio_context,
>>>> +    .bdrv_lockf = raw_lockf,
>>>> +
>>>>       .create_opts = &raw_create_opts,
>>>>   };
>>> would it be better to use
>>>
>>>         int flock(int fd, int operation);
>>>
>>> DESCRIPTION
>>>         Apply or remove an advisory lock on the open file specified by fd.
>>> The
>>>         argument operation is one of the following:
>>>
>>>             LOCK_SH  Place a shared lock.  More than one  process may  hold
>>> a
>>>                      shared lock for a given file at a given time.
>>>
>>>             LOCK_EX  Place  an  exclusive  lock.   Only one process may hold
>>> an
>>>                      exclusive lock for a given file at a given time.
>>>
>>>             LOCK_UN  Remove an existing lock held by this process.
>>>
>>> for this purpose?
>>>
>>> Sorry that missed this point in the initial review...
>>> We will not intersect with libvirt for the case.
>> As noted in the cover letter, flock() is nop on NFS mount points on Linux, so
>> fcntl is safer.
> Actually on Modern linux flock is implemented in terms of fcntl
> on Linux. Flock does not do byte-range locking so by using
> flock on NFS, IIUC you'll get a fcntl lock for the entire file
> range on the server, which will clash with the fcntl lock that
> virtlogd has acquired. On older linux flock is a no-op on NFS.
> Other UNIX have varying degrees of usable for flock on NFS. So
> in general fcntl is a better bet for NFS compatibility and will
> not clash with libvirt.
>
> The only minor issue is that some versions of OCFS do not support
> fcntl locks at all, only flock.
>
> Regards,
> Daniel
thank you for clarification

this is not a bid deal at all :) This approach would work fine.

Den

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Richard W.M. Jones]

On Fri, Apr 15, 2016 at 11:27:55AM +0800, Fam Zheng wrote:
> virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> the intervene.
> +static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> +{
> +
> +    BDRVRawState *s = bs->opaque;
> +    int ret;
> +    struct flock fl = (struct flock) {
> +        .l_whence  = SEEK_SET,
> +        /* Locking byte 1 avoids interfereing with virtlockd. */
> +        .l_start = 1,
> +        .l_len = 1,
> +    };
> +
> +    switch (cmd) {
> +    case BDRV_LOCKF_RWLOCK:
> +        fl.l_type = F_WRLCK;
> +        break;
> +    case BDRV_LOCKF_ROLOCK:
> +        fl.l_type = F_RDLCK;
> +        break;
> +    case BDRV_LOCKF_UNLOCK:
> +        fl.l_type = F_UNLCK;
> +        break;

My understanding is this prevents libguestfs from working on live disk
images -- we want to be able to read live disk images (using a
writable overlay and the real disk image as a read-only backing file).

So please don't do this; or suggest how we can ignore the lock for
backing files.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Fam Zheng]

On Sun, 04/17 20:27, Richard W.M. Jones wrote:
> On Fri, Apr 15, 2016 at 11:27:55AM +0800, Fam Zheng wrote:
> > virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> > the intervene.
> > +static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> > +{
> > +
> > +    BDRVRawState *s = bs->opaque;
> > +    int ret;
> > +    struct flock fl = (struct flock) {
> > +        .l_whence  = SEEK_SET,
> > +        /* Locking byte 1 avoids interfereing with virtlockd. */
> > +        .l_start = 1,
> > +        .l_len = 1,
> > +    };
> > +
> > +    switch (cmd) {
> > +    case BDRV_LOCKF_RWLOCK:
> > +        fl.l_type = F_WRLCK;
> > +        break;
> > +    case BDRV_LOCKF_ROLOCK:
> > +        fl.l_type = F_RDLCK;
> > +        break;
> > +    case BDRV_LOCKF_UNLOCK:
> > +        fl.l_type = F_UNLCK;
> > +        break;
> 
> My understanding is this prevents libguestfs from working on live disk
> images -- we want to be able to read live disk images (using a
> writable overlay and the real disk image as a read-only backing file).

Do you lock the live image or the backing file? If not, you can just read/write
as before, as what the -L option does in this series. Otherwise, you should use
an RO lock on the backing image, which still works.

Fam

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Richard W.M. Jones]

On Mon, Apr 18, 2016 at 09:10:36AM +0800, Fam Zheng wrote:
> On Sun, 04/17 20:27, Richard W.M. Jones wrote:
> > On Fri, Apr 15, 2016 at 11:27:55AM +0800, Fam Zheng wrote:
> > > virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> > > the intervene.
> > > +static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> > > +{
> > > +
> > > +    BDRVRawState *s = bs->opaque;
> > > +    int ret;
> > > +    struct flock fl = (struct flock) {
> > > +        .l_whence  = SEEK_SET,
> > > +        /* Locking byte 1 avoids interfereing with virtlockd. */
> > > +        .l_start = 1,
> > > +        .l_len = 1,
> > > +    };
> > > +
> > > +    switch (cmd) {
> > > +    case BDRV_LOCKF_RWLOCK:
> > > +        fl.l_type = F_WRLCK;
> > > +        break;
> > > +    case BDRV_LOCKF_ROLOCK:
> > > +        fl.l_type = F_RDLCK;
> > > +        break;
> > > +    case BDRV_LOCKF_UNLOCK:
> > > +        fl.l_type = F_UNLCK;
> > > +        break;
> > 
> > My understanding is this prevents libguestfs from working on live disk
> > images -- we want to be able to read live disk images (using a
> > writable overlay and the real disk image as a read-only backing file).
> 
> Do you lock the live image or the backing file?

At the moment we don't need to do anything, but we do have the problem
that if someone uses libguestfs in write mode on a live image, then
obviously they end up with a corrupted guest.

However in this email I'm talking about using libguestfs in
"read-only" mode on a live guest, which is a completely different
thing, and should not be prevented.

My assumption [with this patch applied] is the live image (being live)
is locked by some other qemu.

Now libguestfs creates a temporary overlay with the live image as
backing, using a command similar to:

  qemu-img create -f qcow2 -b live.img tmp-overlay.img

and opens 'tmp-overlay.img'.  But since the live image has an
exclusive lock, the open will *fail*, and that is a problem.

> If not, you can just read/write as before, as what the -L option
> does in this series. Otherwise, you should use an RO lock on the
> backing image, which still works.

fcntl-locks don't allow a shared lock to share with an exclusive lock.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Fam Zheng]

On Mon, 04/18 09:04, Richard W.M. Jones wrote:
> On Mon, Apr 18, 2016 at 09:10:36AM +0800, Fam Zheng wrote:
> > On Sun, 04/17 20:27, Richard W.M. Jones wrote:
> > > On Fri, Apr 15, 2016 at 11:27:55AM +0800, Fam Zheng wrote:
> > > > virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> > > > the intervene.
> > > > +static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> > > > +{
> > > > +
> > > > +    BDRVRawState *s = bs->opaque;
> > > > +    int ret;
> > > > +    struct flock fl = (struct flock) {
> > > > +        .l_whence  = SEEK_SET,
> > > > +        /* Locking byte 1 avoids interfereing with virtlockd. */
> > > > +        .l_start = 1,
> > > > +        .l_len = 1,
> > > > +    };
> > > > +
> > > > +    switch (cmd) {
> > > > +    case BDRV_LOCKF_RWLOCK:
> > > > +        fl.l_type = F_WRLCK;
> > > > +        break;
> > > > +    case BDRV_LOCKF_ROLOCK:
> > > > +        fl.l_type = F_RDLCK;
> > > > +        break;
> > > > +    case BDRV_LOCKF_UNLOCK:
> > > > +        fl.l_type = F_UNLCK;
> > > > +        break;
> > > 
> > > My understanding is this prevents libguestfs from working on live disk
> > > images -- we want to be able to read live disk images (using a
> > > writable overlay and the real disk image as a read-only backing file).
> > 
> > Do you lock the live image or the backing file?
> 
> At the moment we don't need to do anything, but we do have the problem
> that if someone uses libguestfs in write mode on a live image, then
> obviously they end up with a corrupted guest.
> 
> However in this email I'm talking about using libguestfs in
> "read-only" mode on a live guest, which is a completely different
> thing, and should not be prevented.
> 
> My assumption [with this patch applied] is the live image (being live)
> is locked by some other qemu.
> 
> Now libguestfs creates a temporary overlay with the live image as
> backing, using a command similar to:
> 
>   qemu-img create -f qcow2 -b live.img tmp-overlay.img
> 
> and opens 'tmp-overlay.img'.  But since the live image has an
> exclusive lock, the open will *fail*, and that is a problem.

I'm not sure that is what we want to support. The image is being read-write
open by the VM, any other accessing is a bad idea.

Fam

> 
> > If not, you can just read/write as before, as what the -L option
> > does in this series. Otherwise, you should use an RO lock on the
> > backing image, which still works.
> 
> fcntl-locks don't allow a shared lock to share with an exclusive lock.
> 
> Rich.
> 
> -- 
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-df lists disk usage of guests without needing to install any
> software inside the virtual machine.  Supports Linux and Windows.
> http://people.redhat.com/~rjones/virt-df/

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Richard W.M. Jones]

On Tue, Apr 19, 2016 at 08:37:14PM +0800, Fam Zheng wrote:
> On Mon, 04/18 09:04, Richard W.M. Jones wrote:
> > On Mon, Apr 18, 2016 at 09:10:36AM +0800, Fam Zheng wrote:
> > > On Sun, 04/17 20:27, Richard W.M. Jones wrote:
> > > > On Fri, Apr 15, 2016 at 11:27:55AM +0800, Fam Zheng wrote:
> > > > > virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> > > > > the intervene.
> > > > > +static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> > > > > +{
> > > > > +
> > > > > +    BDRVRawState *s = bs->opaque;
> > > > > +    int ret;
> > > > > +    struct flock fl = (struct flock) {
> > > > > +        .l_whence  = SEEK_SET,
> > > > > +        /* Locking byte 1 avoids interfereing with virtlockd. */
> > > > > +        .l_start = 1,
> > > > > +        .l_len = 1,
> > > > > +    };
> > > > > +
> > > > > +    switch (cmd) {
> > > > > +    case BDRV_LOCKF_RWLOCK:
> > > > > +        fl.l_type = F_WRLCK;
> > > > > +        break;
> > > > > +    case BDRV_LOCKF_ROLOCK:
> > > > > +        fl.l_type = F_RDLCK;
> > > > > +        break;
> > > > > +    case BDRV_LOCKF_UNLOCK:
> > > > > +        fl.l_type = F_UNLCK;
> > > > > +        break;
> > > > 
> > > > My understanding is this prevents libguestfs from working on live disk
> > > > images -- we want to be able to read live disk images (using a
> > > > writable overlay and the real disk image as a read-only backing file).
> > > 
> > > Do you lock the live image or the backing file?
> > 
> > At the moment we don't need to do anything, but we do have the problem
> > that if someone uses libguestfs in write mode on a live image, then
> > obviously they end up with a corrupted guest.
> > 
> > However in this email I'm talking about using libguestfs in
> > "read-only" mode on a live guest, which is a completely different
> > thing, and should not be prevented.
> > 
> > My assumption [with this patch applied] is the live image (being live)
> > is locked by some other qemu.
> > 
> > Now libguestfs creates a temporary overlay with the live image as
> > backing, using a command similar to:
> > 
> >   qemu-img create -f qcow2 -b live.img tmp-overlay.img
> > 
> > and opens 'tmp-overlay.img'.  But since the live image has an
> > exclusive lock, the open will *fail*, and that is a problem.
> 
> I'm not sure that is what we want to support. The image is being read-write
> open by the VM, any other accessing is a bad idea.

We've done this successfully for years, for people monitoring their
VMs using virt-df, pulling out files using guestfish and so on.  We
allow you to do it while the guest is live and running, with the
proviso that a consistent view cannot always be guaranteed (although
it works so reliably that it's never really a problem), or users can
briefly pause VMs if they need a guaranteed consistent view.

I'm afraid the onus is on you to explain why this existing practice is
a bad idea.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Fam Zheng]

On Tue, 04/19 14:07, Richard W.M. Jones wrote:
> On Tue, Apr 19, 2016 at 08:37:14PM +0800, Fam Zheng wrote:
> > On Mon, 04/18 09:04, Richard W.M. Jones wrote:
> > > On Mon, Apr 18, 2016 at 09:10:36AM +0800, Fam Zheng wrote:
> > > > On Sun, 04/17 20:27, Richard W.M. Jones wrote:
> > > > > On Fri, Apr 15, 2016 at 11:27:55AM +0800, Fam Zheng wrote:
> > > > > > virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> > > > > > the intervene.
> > > > > > +static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> > > > > > +{
> > > > > > +
> > > > > > +    BDRVRawState *s = bs->opaque;
> > > > > > +    int ret;
> > > > > > +    struct flock fl = (struct flock) {
> > > > > > +        .l_whence  = SEEK_SET,
> > > > > > +        /* Locking byte 1 avoids interfereing with virtlockd. */
> > > > > > +        .l_start = 1,
> > > > > > +        .l_len = 1,
> > > > > > +    };
> > > > > > +
> > > > > > +    switch (cmd) {
> > > > > > +    case BDRV_LOCKF_RWLOCK:
> > > > > > +        fl.l_type = F_WRLCK;
> > > > > > +        break;
> > > > > > +    case BDRV_LOCKF_ROLOCK:
> > > > > > +        fl.l_type = F_RDLCK;
> > > > > > +        break;
> > > > > > +    case BDRV_LOCKF_UNLOCK:
> > > > > > +        fl.l_type = F_UNLCK;
> > > > > > +        break;
> > > > > 
> > > > > My understanding is this prevents libguestfs from working on live disk
> > > > > images -- we want to be able to read live disk images (using a
> > > > > writable overlay and the real disk image as a read-only backing file).
> > > > 
> > > > Do you lock the live image or the backing file?
> > > 
> > > At the moment we don't need to do anything, but we do have the problem
> > > that if someone uses libguestfs in write mode on a live image, then
> > > obviously they end up with a corrupted guest.
> > > 
> > > However in this email I'm talking about using libguestfs in
> > > "read-only" mode on a live guest, which is a completely different
> > > thing, and should not be prevented.
> > > 
> > > My assumption [with this patch applied] is the live image (being live)
> > > is locked by some other qemu.
> > > 
> > > Now libguestfs creates a temporary overlay with the live image as
> > > backing, using a command similar to:
> > > 
> > >   qemu-img create -f qcow2 -b live.img tmp-overlay.img
> > > 
> > > and opens 'tmp-overlay.img'.  But since the live image has an
> > > exclusive lock, the open will *fail*, and that is a problem.
> > 
> > I'm not sure that is what we want to support. The image is being read-write
> > open by the VM, any other accessing is a bad idea.
> 
> We've done this successfully for years, for people monitoring their
> VMs using virt-df, pulling out files using guestfish and so on.  We
> allow you to do it while the guest is live and running, with the
> proviso that a consistent view cannot always be guaranteed (although
> it works so reliably that it's never really a problem), or users can
> briefly pause VMs if they need a guaranteed consistent view.
> 
> I'm afraid the onus is on you to explain why this existing practice is
> a bad idea.

It is bad idea because it can produce erroneous data. Perhaps it's not
critical and is rare enough to be practically useful.

As a tradeoff, I guess, we can skip the shared lock in this series. Does that
work for you?

Fam

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Richard W.M. Jones]

On Tue, Apr 19, 2016 at 09:19:02PM +0800, Fam Zheng wrote:
> On Tue, 04/19 14:07, Richard W.M. Jones wrote:
> > We've done this successfully for years, for people monitoring their
> > VMs using virt-df, pulling out files using guestfish and so on.  We
> > allow you to do it while the guest is live and running, with the
> > proviso that a consistent view cannot always be guaranteed (although
> > it works so reliably that it's never really a problem), or users can
> > briefly pause VMs if they need a guaranteed consistent view.
> > 
> > I'm afraid the onus is on you to explain why this existing practice is
> > a bad idea.
> 
> It is bad idea because it can produce erroneous data. Perhaps it's not
> critical and is rare enough to be practically useful.

As explained above, we deal with the inconsistent case (by detecting
and ignoring it, or retrying), and then there's the case where we
pause the guest to get consistent data.

> As a tradeoff, I guess, we can skip the shared lock in this series. Does that
> work for you?

I'd prefer some kind of no lock / ignore lock.  There is a legitimate
case where you want to have the shared lock behaviour, but also a
legitimate one for turning it off.  I'm not opposed to the idea --
there are very real cases where your patch saves people from
themselves.

Off topic: How does this patch deal with genuinely shared (writable)
disks, as used in cluster filesystems?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Daniel P. Berrange]

On Tue, Apr 19, 2016 at 02:36:05PM +0100, Richard W.M. Jones wrote:
> 
> I'd prefer some kind of no lock / ignore lock.  There is a legitimate
> case where you want to have the shared lock behaviour, but also a
> legitimate one for turning it off.  I'm not opposed to the idea --
> there are very real cases where your patch saves people from
> themselves.

[snip]

> Off topic: How does this patch deal with genuinely shared (writable)
> disks, as used in cluster filesystems?

This series does add support for a lock_image=no parameter for disks
which turns off all locking. IIUC, this was intended for use when
the disk image must be shared between guests

IOW, this series is doing the following:

 - read-only, exclusive -> F_RDLOCK
 - writable, shared     -> no locking
 - writable, exclusive  -> F_WRLOCK

This stuff leaves open possibility of mistakes if one VM is
given a disk in write+exclusive, while other VM gets the
same disk in write+shared mode.

In virtlockd we took a different approach to this:

 - read-only, exclusive -> no locking
 - writable, shared     -> F_RDLOCK
 - writable, exclusive  -> F_WRLOCK

because this lets the kernel prevent case where the disk image is presented
in write+shared mode to one VM and write+exlcusive mode to another VM.
Not doing any locking at all in readonly case allowed for the scenario
needed with libguestfs where they create an overlay for disks in use.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Daniel P. Berrange]

On Mon, Apr 18, 2016 at 09:04:19AM +0100, Richard W.M. Jones wrote:
> On Mon, Apr 18, 2016 at 09:10:36AM +0800, Fam Zheng wrote:
> > On Sun, 04/17 20:27, Richard W.M. Jones wrote:
> > > On Fri, Apr 15, 2016 at 11:27:55AM +0800, Fam Zheng wrote:
> > > > virtlockd in libvirt locks the first byte, we lock byte 1 to avoid
> > > > the intervene.
> > > > +static int raw_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> > > > +{
> > > > +
> > > > +    BDRVRawState *s = bs->opaque;
> > > > +    int ret;
> > > > +    struct flock fl = (struct flock) {
> > > > +        .l_whence  = SEEK_SET,
> > > > +        /* Locking byte 1 avoids interfereing with virtlockd. */
> > > > +        .l_start = 1,
> > > > +        .l_len = 1,
> > > > +    };
> > > > +
> > > > +    switch (cmd) {
> > > > +    case BDRV_LOCKF_RWLOCK:
> > > > +        fl.l_type = F_WRLCK;
> > > > +        break;
> > > > +    case BDRV_LOCKF_ROLOCK:
> > > > +        fl.l_type = F_RDLCK;
> > > > +        break;
> > > > +    case BDRV_LOCKF_UNLOCK:
> > > > +        fl.l_type = F_UNLCK;
> > > > +        break;
> > > 
> > > My understanding is this prevents libguestfs from working on live disk
> > > images -- we want to be able to read live disk images (using a
> > > writable overlay and the real disk image as a read-only backing file).
> > 
> > Do you lock the live image or the backing file?
> 
> At the moment we don't need to do anything, but we do have the problem
> that if someone uses libguestfs in write mode on a live image, then
> obviously they end up with a corrupted guest.
> 
> However in this email I'm talking about using libguestfs in
> "read-only" mode on a live guest, which is a completely different
> thing, and should not be prevented.
> 
> My assumption [with this patch applied] is the live image (being live)
> is locked by some other qemu.
> 
> Now libguestfs creates a temporary overlay with the live image as
> backing, using a command similar to:
> 
>   qemu-img create -f qcow2 -b live.img tmp-overlay.img
> 
> and opens 'tmp-overlay.img'.  But since the live image has an
> exclusive lock, the open will *fail*, and that is a problem.

Have you ever considered integration with the QEMU NBD server. We
don't have APIs for enabling it explicitly in libvirt, but it strikes
me that it could be ideally suited for your needs.

eg a hypothetical libvirt command to export a disk via NBD:

   virsh dom-export-disk myguest --readonly vda1  localhost 9000
   qemu-img create -f qcow2 -b nbd:localhost:9000 tmp-overlay.img
   ...do stuff...
   virsh dom-unexport-disk myguest vda1

Or to make cleanup easier, perhaps there's a way to tell QEMU
to close its NBD server after it has had 1 client connection.

With this approach, you wouldn't need to take any lock on the
underlying real image.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH for-2.7 v2 05/17] raw-posix: Implement .bdrv_lockf

[Richard W.M. Jones]

On Tue, Apr 19, 2016 at 02:34:30PM +0100, Daniel P. Berrange wrote:
> Have you ever considered integration with the QEMU NBD server. We
> don't have APIs for enabling it explicitly in libvirt, but it strikes
> me that it could be ideally suited for your needs.
> 
> eg a hypothetical libvirt command to export a disk via NBD:
> 
>    virsh dom-export-disk myguest --readonly vda1  localhost 9000
>    qemu-img create -f qcow2 -b nbd:localhost:9000 tmp-overlay.img
>    ...do stuff...
>    virsh dom-unexport-disk myguest vda1
> 
> Or to make cleanup easier, perhaps there's a way to tell QEMU
> to close its NBD server after it has had 1 client connection.
> 
> With this approach, you wouldn't need to take any lock on the
> underlying real image.

It's been on my todo list for a long time, but this does require
libvirt to be involved, and libvirt is not the default for upstream
libguestfs.  Even with the libvirt backend, there are still uses cases
like:

  virt-df -a /mnt/vms/disk.img -h
  virt-inspector -a /mnt/vms/disk.img | grep some_insecure_software
  for f in /mnt/vms/*; do virt-alignment-scan -a $f; done
and so on.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

[Qemu-devel] [PATCH for-2.7 v2 06/17] gluster: Implement .bdrv_lockf

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/gluster.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/block/gluster.c b/block/gluster.c
index 51e154c..c23e944 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -672,6 +672,32 @@ static void qemu_gluster_close(BlockDriverState *bs)
     glfs_fini(s->glfs);
 }
 
+static int qemu_gluster_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
+{
+    BDRVGlusterState *s = bs->opaque;
+    int ret;
+    struct flock fl = (struct flock) {
+        .l_start = 0,
+        .l_whence  = SEEK_SET,
+        .l_len = 0,
+    };
+    switch (cmd) {
+    case BDRV_LOCKF_RWLOCK:
+        fl.l_type = F_WRLCK;
+        break;
+    case BDRV_LOCKF_ROLOCK:
+        fl.l_type = F_RDLCK;
+        break;
+    case BDRV_LOCKF_UNLOCK:
+        fl.l_type = F_UNLCK;
+        break;
+    default:
+        abort();
+    }
+    ret = glfs_posix_lock(s->fd, F_SETLK, &fl);
+    return ret == -1 ? -errno : 0;
+}
+
 static int qemu_gluster_has_zero_init(BlockDriverState *bs)
 {
     /* GlusterFS volume could be backed by a block device */
@@ -713,6 +739,7 @@ static BlockDriver bdrv_gluster = {
     .bdrv_co_readv                = qemu_gluster_co_readv,
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
+    .bdrv_lockf                   = qemu_gluster_lockf,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
@@ -740,6 +767,7 @@ static BlockDriver bdrv_gluster_tcp = {
     .bdrv_co_readv                = qemu_gluster_co_readv,
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
+    .bdrv_lockf                   = qemu_gluster_lockf,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
@@ -767,6 +795,7 @@ static BlockDriver bdrv_gluster_unix = {
     .bdrv_co_readv                = qemu_gluster_co_readv,
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
+    .bdrv_lockf                   = qemu_gluster_lockf,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
@@ -794,6 +823,7 @@ static BlockDriver bdrv_gluster_rdma = {
     .bdrv_co_readv                = qemu_gluster_co_readv,
     .bdrv_co_writev               = qemu_gluster_co_writev,
     .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
+    .bdrv_lockf                   = qemu_gluster_lockf,
     .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
 #ifdef CONFIG_GLUSTERFS_DISCARD
     .bdrv_co_discard              = qemu_gluster_co_discard,
-- 
2.8.0

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.7 v2 06/17] gluster: Implement .bdrv_lockf

[Niels de Vos]

On Fri, Apr 15, 2016 at 11:27:56AM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/gluster.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)

The Gluster changes look good to me.

Reviewed-by: Niels de Vos <ndevos@redhat.com>

> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 51e154c..c23e944 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -672,6 +672,32 @@ static void qemu_gluster_close(BlockDriverState *bs)
>      glfs_fini(s->glfs);
>  }
>  
> +static int qemu_gluster_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> +{
> +    BDRVGlusterState *s = bs->opaque;
> +    int ret;
> +    struct flock fl = (struct flock) {
> +        .l_start = 0,
> +        .l_whence  = SEEK_SET,
> +        .l_len = 0,
> +    };
> +    switch (cmd) {
> +    case BDRV_LOCKF_RWLOCK:
> +        fl.l_type = F_WRLCK;
> +        break;
> +    case BDRV_LOCKF_ROLOCK:
> +        fl.l_type = F_RDLCK;
> +        break;
> +    case BDRV_LOCKF_UNLOCK:
> +        fl.l_type = F_UNLCK;
> +        break;
> +    default:
> +        abort();
> +    }
> +    ret = glfs_posix_lock(s->fd, F_SETLK, &fl);
> +    return ret == -1 ? -errno : 0;
> +}
> +
>  static int qemu_gluster_has_zero_init(BlockDriverState *bs)
>  {
>      /* GlusterFS volume could be backed by a block device */
> @@ -713,6 +739,7 @@ static BlockDriver bdrv_gluster = {
>      .bdrv_co_readv                = qemu_gluster_co_readv,
>      .bdrv_co_writev               = qemu_gluster_co_writev,
>      .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
> +    .bdrv_lockf                   = qemu_gluster_lockf,
>      .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
>  #ifdef CONFIG_GLUSTERFS_DISCARD
>      .bdrv_co_discard              = qemu_gluster_co_discard,
> @@ -740,6 +767,7 @@ static BlockDriver bdrv_gluster_tcp = {
>      .bdrv_co_readv                = qemu_gluster_co_readv,
>      .bdrv_co_writev               = qemu_gluster_co_writev,
>      .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
> +    .bdrv_lockf                   = qemu_gluster_lockf,
>      .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
>  #ifdef CONFIG_GLUSTERFS_DISCARD
>      .bdrv_co_discard              = qemu_gluster_co_discard,
> @@ -767,6 +795,7 @@ static BlockDriver bdrv_gluster_unix = {
>      .bdrv_co_readv                = qemu_gluster_co_readv,
>      .bdrv_co_writev               = qemu_gluster_co_writev,
>      .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
> +    .bdrv_lockf                   = qemu_gluster_lockf,
>      .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
>  #ifdef CONFIG_GLUSTERFS_DISCARD
>      .bdrv_co_discard              = qemu_gluster_co_discard,
> @@ -794,6 +823,7 @@ static BlockDriver bdrv_gluster_rdma = {
>      .bdrv_co_readv                = qemu_gluster_co_readv,
>      .bdrv_co_writev               = qemu_gluster_co_writev,
>      .bdrv_co_flush_to_disk        = qemu_gluster_co_flush_to_disk,
> +    .bdrv_lockf                   = qemu_gluster_lockf,
>      .bdrv_has_zero_init           = qemu_gluster_has_zero_init,
>  #ifdef CONFIG_GLUSTERFS_DISCARD
>      .bdrv_co_discard              = qemu_gluster_co_discard,
> -- 
> 2.8.0
> 
> 

[Qemu-devel] [PATCH for-2.7 v2 07/17] rbd: Implement image locking

[Fam Zheng]

librbd has the locking API that can be used to implement .bdrv_lockf.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/rbd.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/block/rbd.c b/block/rbd.c
index 5bc5b32..a495083 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -810,6 +810,30 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
     return 0;
 }
 
+static int qemu_rbd_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
+{
+    int ret;
+    BDRVRBDState *s = bs->opaque;
+
+    /* XXX: RBD locks are not released automatically when program exits, which
+     * means if QEMU dies it cannot open the image next time until manually
+     * unlocked. */
+    switch (cmd) {
+    case BDRV_LOCKF_RWLOCK:
+        ret = rbd_lock_exclusive(s->image, NULL);
+        break;
+    case BDRV_LOCKF_ROLOCK:
+        ret = rbd_lock_shared(s->image, NULL, NULL);
+        break;
+    case BDRV_LOCKF_UNLOCK:
+        ret = rbd_unlock(s->image, NULL);
+        break;
+    default:
+        abort();
+    }
+    return ret;
+}
+
 static int qemu_rbd_snap_create(BlockDriverState *bs,
                                 QEMUSnapshotInfo *sn_info)
 {
@@ -998,6 +1022,7 @@ static BlockDriver bdrv_rbd = {
     .bdrv_aio_discard       = qemu_rbd_aio_discard,
 #endif
 
+    .bdrv_lockf             = qemu_rbd_lockf,
     .bdrv_snapshot_create   = qemu_rbd_snap_create,
     .bdrv_snapshot_delete   = qemu_rbd_snap_remove,
     .bdrv_snapshot_list     = qemu_rbd_snap_list,
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.7 v2 07/17] rbd: Implement image locking

[Jason Dillaman]

[-- Attachment #1: Type: text/plain, Size: 2619 bytes --]

Since this cannot automatically recover from a crashed QEMU client with an
RBD image, perhaps this RBD locking should not default to enabled.
Additionally, this will conflict with the "exclusive-lock" feature
available since the Ceph Hammer-release since both utilize the same locking
construct.

As a quick background, the optional exclusive-lock feature can be
enabled/disabled on image and safely/automatically handles the case of
recovery from a crashed client.  Under normal conditions, the RBD
exclusive-lock feature automatically acquires the lock upon the first
attempt to write to the image and transparently transitions ownership of
the lock between two or more clients -- used for QEMU live-migration.

I'd be happy to add a new librbd API method to explicitly expose acquiring
and releasing the RBD exclusive lock since it certainly solves a couple
compromises in our current QEMU integration.

On Thu, Apr 14, 2016 at 11:27 PM, Fam Zheng <famz@redhat.com> wrote:

> librbd has the locking API that can be used to implement .bdrv_lockf.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/rbd.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 5bc5b32..a495083 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -810,6 +810,30 @@ static int qemu_rbd_truncate(BlockDriverState *bs,
> int64_t offset)
>      return 0;
>  }
>
> +static int qemu_rbd_lockf(BlockDriverState *bs, BdrvLockfCmd cmd)
> +{
> +    int ret;
> +    BDRVRBDState *s = bs->opaque;
> +
> +    /* XXX: RBD locks are not released automatically when program exits,
> which
> +     * means if QEMU dies it cannot open the image next time until
> manually
> +     * unlocked. */
> +    switch (cmd) {
> +    case BDRV_LOCKF_RWLOCK:
> +        ret = rbd_lock_exclusive(s->image, NULL);
> +        break;
> +    case BDRV_LOCKF_ROLOCK:
> +        ret = rbd_lock_shared(s->image, NULL, NULL);
> +        break;
> +    case BDRV_LOCKF_UNLOCK:
> +        ret = rbd_unlock(s->image, NULL);
> +        break;
> +    default:
> +        abort();
> +    }
> +    return ret;
> +}
> +
>  static int qemu_rbd_snap_create(BlockDriverState *bs,
>                                  QEMUSnapshotInfo *sn_info)
>  {
> @@ -998,6 +1022,7 @@ static BlockDriver bdrv_rbd = {
>      .bdrv_aio_discard       = qemu_rbd_aio_discard,
>  #endif
>
> +    .bdrv_lockf             = qemu_rbd_lockf,
>      .bdrv_snapshot_create   = qemu_rbd_snap_create,
>      .bdrv_snapshot_delete   = qemu_rbd_snap_remove,
>      .bdrv_snapshot_list     = qemu_rbd_snap_list,
> --
> 2.8.0
>
>
>

-- 

Jason

[-- Attachment #2: Type: text/html, Size: 3518 bytes --]

Re: [Qemu-devel] [PATCH for-2.7 v2 07/17] rbd: Implement image locking

[Fam Zheng]

On Fri, 04/22 21:57, Jason Dillaman wrote:
> Since this cannot automatically recover from a crashed QEMU client with an
> RBD image, perhaps this RBD locking should not default to enabled.
> Additionally, this will conflict with the "exclusive-lock" feature
> available since the Ceph Hammer-release since both utilize the same locking
> construct.
> 
> As a quick background, the optional exclusive-lock feature can be
> enabled/disabled on image and safely/automatically handles the case of
> recovery from a crashed client.  Under normal conditions, the RBD
> exclusive-lock feature automatically acquires the lock upon the first
> attempt to write to the image and transparently transitions ownership of
> the lock between two or more clients -- used for QEMU live-migration.

Is it enabled by default?

> 
> I'd be happy to add a new librbd API method to explicitly expose acquiring
> and releasing the RBD exclusive lock since it certainly solves a couple
> compromises in our current QEMU integration.

Does the API do enable/disable or acquire/relase? Could you explain the
differences between it and rbd_lock_exclusive?

Fam

Re: [Qemu-devel] [PATCH for-2.7 v2 07/17] rbd: Implement image locking

[Jason Dillaman]

On Sun, Apr 24, 2016 at 7:42 PM, Fam Zheng <famz@redhat.com> wrote:
> On Fri, 04/22 21:57, Jason Dillaman wrote:
>> Since this cannot automatically recover from a crashed QEMU client with an
>> RBD image, perhaps this RBD locking should not default to enabled.
>> Additionally, this will conflict with the "exclusive-lock" feature
>> available since the Ceph Hammer-release since both utilize the same locking
>> construct.
>>
>> As a quick background, the optional exclusive-lock feature can be
>> enabled/disabled on image and safely/automatically handles the case of
>> recovery from a crashed client.  Under normal conditions, the RBD
>> exclusive-lock feature automatically acquires the lock upon the first
>> attempt to write to the image and transparently transitions ownership of
>> the lock between two or more clients -- used for QEMU live-migration.
>
> Is it enabled by default?
>

Starting with the Jewel release of Ceph it is enabled by default.

>>
>> I'd be happy to add a new librbd API method to explicitly expose acquiring
>> and releasing the RBD exclusive lock since it certainly solves a couple
>> compromises in our current QEMU integration.
>
> Does the API do enable/disable or acquire/relase? Could you explain the
> differences between it and rbd_lock_exclusive?

There is already an API for enabling/disabling the exclusive-lock
feature (and associated CLI tooling).  This would be a new API method
to explicitly acquire / release the exclusive-lock (instead of
implicitly acquiring the lock upon first write request as it currently
does in order to support QEMU live-migration).

The rbd_lock_exclusive/rbd_lock_shared are essentially advisory locks.
Nothing stops a client from accessing the image if it doesn't attempt
to acquire the lock (even if another client already has the image
locked exclusively).  It also doesn't support automatic recovery from
failed clients.

-- 
Jason

Re: [Qemu-devel] [PATCH for-2.7 v2 07/17] rbd: Implement image locking

[Fam Zheng]

On Tue, 04/26 10:42, Jason Dillaman wrote:
> On Sun, Apr 24, 2016 at 7:42 PM, Fam Zheng <famz@redhat.com> wrote:
> > On Fri, 04/22 21:57, Jason Dillaman wrote:
> >> Since this cannot automatically recover from a crashed QEMU client with an
> >> RBD image, perhaps this RBD locking should not default to enabled.
> >> Additionally, this will conflict with the "exclusive-lock" feature
> >> available since the Ceph Hammer-release since both utilize the same locking
> >> construct.
> >>
> >> As a quick background, the optional exclusive-lock feature can be
> >> enabled/disabled on image and safely/automatically handles the case of
> >> recovery from a crashed client.  Under normal conditions, the RBD
> >> exclusive-lock feature automatically acquires the lock upon the first
> >> attempt to write to the image and transparently transitions ownership of
> >> the lock between two or more clients -- used for QEMU live-migration.
> >
> > Is it enabled by default?
> >
> 
> Starting with the Jewel release of Ceph it is enabled by default.

OK, then I'll leave rbd in this QEMU series for now.

> 
> >>
> >> I'd be happy to add a new librbd API method to explicitly expose acquiring
> >> and releasing the RBD exclusive lock since it certainly solves a couple
> >> compromises in our current QEMU integration.
> >
> > Does the API do enable/disable or acquire/relase? Could you explain the
> > differences between it and rbd_lock_exclusive?
> 
> There is already an API for enabling/disabling the exclusive-lock
> feature (and associated CLI tooling).  This would be a new API method
> to explicitly acquire / release the exclusive-lock (instead of
> implicitly acquiring the lock upon first write request as it currently
> does in order to support QEMU live-migration).
> 
> The rbd_lock_exclusive/rbd_lock_shared are essentially advisory locks.
> Nothing stops a client from accessing the image if it doesn't attempt
> to acquire the lock (even if another client already has the image
> locked exclusively).  It also doesn't support automatic recovery from
> failed clients.

I see, thanks for the explanation!

Fam

Re: [Qemu-devel] [PATCH for-2.7 v2 07/17] rbd: Implement image locking

[Jason Dillaman]

On Tue, Apr 26, 2016 at 7:20 PM, Fam Zheng <famz@redhat.com> wrote:
> On Tue, 04/26 10:42, Jason Dillaman wrote:
>> On Sun, Apr 24, 2016 at 7:42 PM, Fam Zheng <famz@redhat.com> wrote:
>> > On Fri, 04/22 21:57, Jason Dillaman wrote:
>> >> Since this cannot automatically recover from a crashed QEMU client with an
>> >> RBD image, perhaps this RBD locking should not default to enabled.
>> >> Additionally, this will conflict with the "exclusive-lock" feature
>> >> available since the Ceph Hammer-release since both utilize the same locking
>> >> construct.
>> >>
>> >> As a quick background, the optional exclusive-lock feature can be
>> >> enabled/disabled on image and safely/automatically handles the case of
>> >> recovery from a crashed client.  Under normal conditions, the RBD
>> >> exclusive-lock feature automatically acquires the lock upon the first
>> >> attempt to write to the image and transparently transitions ownership of
>> >> the lock between two or more clients -- used for QEMU live-migration.
>> >
>> > Is it enabled by default?
>> >
>>
>> Starting with the Jewel release of Ceph it is enabled by default.
>
> OK, then I'll leave rbd in this QEMU series for now.

Without exposing some new API methods, this patch will, unfortunately,
directly conflict with the new Jewel rbd defaults and will actually
result in the image becoming read-only since librbd won't be able to
acquire the exclusive lock when QEMU owns the advisory lock.

We can probably get the new API methods upstream within a week or two
[1].  If that's too long of a delay, I'd recommend dropping rbd
locking from the series for now.

[1] http://tracker.ceph.com/issues/15632

-- 
Jason

Re: [Qemu-devel] [PATCH for-2.7 v2 07/17] rbd: Implement image locking

[Fam Zheng]

On Wed, 04/27 13:18, Jason Dillaman wrote:
> On Tue, Apr 26, 2016 at 7:20 PM, Fam Zheng <famz@redhat.com> wrote:
> > On Tue, 04/26 10:42, Jason Dillaman wrote:
> >> On Sun, Apr 24, 2016 at 7:42 PM, Fam Zheng <famz@redhat.com> wrote:
> >> > On Fri, 04/22 21:57, Jason Dillaman wrote:
> >> >> Since this cannot automatically recover from a crashed QEMU client with an
> >> >> RBD image, perhaps this RBD locking should not default to enabled.
> >> >> Additionally, this will conflict with the "exclusive-lock" feature
> >> >> available since the Ceph Hammer-release since both utilize the same locking
> >> >> construct.
> >> >>
> >> >> As a quick background, the optional exclusive-lock feature can be
> >> >> enabled/disabled on image and safely/automatically handles the case of
> >> >> recovery from a crashed client.  Under normal conditions, the RBD
> >> >> exclusive-lock feature automatically acquires the lock upon the first
> >> >> attempt to write to the image and transparently transitions ownership of
> >> >> the lock between two or more clients -- used for QEMU live-migration.
> >> >
> >> > Is it enabled by default?
> >> >
> >>
> >> Starting with the Jewel release of Ceph it is enabled by default.
> >
> > OK, then I'll leave rbd in this QEMU series for now.
> 
> Without exposing some new API methods, this patch will, unfortunately,
> directly conflict with the new Jewel rbd defaults and will actually
> result in the image becoming read-only since librbd won't be able to
> acquire the exclusive lock when QEMU owns the advisory lock.
> 
> We can probably get the new API methods upstream within a week or two
> [1].  If that's too long of a delay, I'd recommend dropping rbd
> locking from the series for now.

Yes you are right, I tried to mean "drop" with "leave" :)

Thanks,
Fam

> 
> [1] http://tracker.ceph.com/issues/15632
> 
> -- 
> Jason

[Qemu-devel] [PATCH for-2.7 v2 08/17] qemu-io: Add "-L" option for BDRV_O_NO_LOCK

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-io.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index 288bba8..6bb6232 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -107,6 +107,7 @@ static void open_help(void)
 " -r, -- open file read-only\n"
 " -s, -- use snapshot file\n"
 " -n, -- disable host cache\n"
+" -L, -- disable image lock\n"
 " -o, -- options to be given to the block driver"
 "\n");
 }
@@ -139,12 +140,13 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
 {
     int flags = 0;
     int readonly = 0;
+    int nolock = 0;
     bool writethrough = true;
     int c;
     QemuOpts *qopts;
     QDict *opts;
 
-    while ((c = getopt(argc, argv, "snrgo:")) != -1) {
+    while ((c = getopt(argc, argv, "snrgLo:")) != -1) {
         switch (c) {
         case 's':
             flags |= BDRV_O_SNAPSHOT;
@@ -156,6 +158,9 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
         case 'r':
             readonly = 1;
             break;
+        case 'L':
+            nolock = 1;
+            break;
         case 'o':
             if (imageOpts) {
                 printf("--image-opts and 'open -o' are mutually exclusive\n");
@@ -176,6 +181,10 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
         flags |= BDRV_O_RDWR;
     }
 
+    if (nolock) {
+        flags |= BDRV_O_NO_LOCK;
+    }
+
     if (imageOpts && (optind == argc - 1)) {
         if (!qemu_opts_parse_noisily(&empty_opts, argv[optind], false)) {
             qemu_opts_reset(&empty_opts);
@@ -410,7 +419,8 @@ static QemuOptsList file_opts = {
 int main(int argc, char **argv)
 {
     int readonly = 0;
-    const char *sopt = "hVc:d:f:rsnmgkt:T:";
+    int nolock = 0;
+    const char *sopt = "hVc:d:f:rLsnmgkt:T:";
     const struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -418,6 +428,7 @@ int main(int argc, char **argv)
         { "cmd", required_argument, NULL, 'c' },
         { "format", required_argument, NULL, 'f' },
         { "read-only", no_argument, NULL, 'r' },
+        { "no-lock", no_argument, NULL, 'L' },
         { "snapshot", no_argument, NULL, 's' },
         { "nocache", no_argument, NULL, 'n' },
         { "misalign", no_argument, NULL, 'm' },
@@ -477,6 +488,9 @@ int main(int argc, char **argv)
         case 'r':
             readonly = 1;
             break;
+        case 'L':
+            nolock = 1;
+            break;
         case 'm':
             qemuio_misalign = true;
             break;
@@ -558,6 +572,10 @@ int main(int argc, char **argv)
         flags |= BDRV_O_RDWR;
     }
 
+    if (nolock) {
+        flags |= BDRV_O_NO_LOCK;
+    }
+
     if ((argc - optind) == 1) {
         if (imageOpts) {
             QemuOpts *qopts = NULL;
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.7 v2 08/17] qemu-io: Add "-L" option for BDRV_O_NO_LOCK

[Denis V. Lunev]

On 04/15/2016 06:27 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   qemu-io.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/qemu-io.c b/qemu-io.c
> index 288bba8..6bb6232 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -107,6 +107,7 @@ static void open_help(void)
>   " -r, -- open file read-only\n"
>   " -s, -- use snapshot file\n"
>   " -n, -- disable host cache\n"
> +" -L, -- disable image lock\n"
>   " -o, -- options to be given to the block driver"
>   "\n");
>   }
pls add L to .args below

static const cmdinfo_t open_cmd = {
     .name       = "open",
     .altname    = "o",
     .cfunc      = open_f,
     .argmin     = 1,
     .argmax     = -1,
     .flags      = CMD_NOFILE_OK,
     .args       = "[-Crsn] [-o options] [path]",
     .oneline    = "open the file specified by path",
     .help       = open_help,
};

Re: [Qemu-devel] [PATCH for-2.7 v2 08/17] qemu-io: Add "-L" option for BDRV_O_NO_LOCK

[Fam Zheng]

On Sat, 04/16 16:46, Denis V. Lunev wrote:
> On 04/15/2016 06:27 AM, Fam Zheng wrote:
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  qemu-io.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> >
> >diff --git a/qemu-io.c b/qemu-io.c
> >index 288bba8..6bb6232 100644
> >--- a/qemu-io.c
> >+++ b/qemu-io.c
> >@@ -107,6 +107,7 @@ static void open_help(void)
> >  " -r, -- open file read-only\n"
> >  " -s, -- use snapshot file\n"
> >  " -n, -- disable host cache\n"
> >+" -L, -- disable image lock\n"
> >  " -o, -- options to be given to the block driver"
> >  "\n");
> >  }
> pls add L to .args below
> 
> static const cmdinfo_t open_cmd = {
>     .name       = "open",
>     .altname    = "o",
>     .cfunc      = open_f,
>     .argmin     = 1,
>     .argmax     = -1,
>     .flags      = CMD_NOFILE_OK,
>     .args       = "[-Crsn] [-o options] [path]",
>     .oneline    = "open the file specified by path",
>     .help       = open_help,
> };

Will fix it.

Fam

[Qemu-devel] [PATCH for-2.7 v2 09/17] qemu-img: Add "-L" option to sub commands

[Fam Zheng]

If specified, BDRV_O_NO_LOCK flag will be set when opening the image.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 72 insertions(+), 17 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 1697762..327be44 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -600,6 +600,7 @@ static int img_check(int argc, char **argv)
     bool quiet = false;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     output = NULL;
@@ -616,7 +617,7 @@ static int img_check(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:r:T:q",
+        c = getopt_long(argc, argv, "hf:r:T:qL",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -650,6 +651,9 @@ static int img_check(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -684,6 +688,7 @@ static int img_check(int argc, char **argv)
         return 1;
     }
 
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", cache);
@@ -804,6 +809,7 @@ static int img_commit(int argc, char **argv)
     Error *local_err = NULL;
     CommonBlockJobCBInfo cbi;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     cache = BDRV_DEFAULT_CACHE;
@@ -815,7 +821,7 @@ static int img_commit(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:ht:b:dpq",
+        c = getopt_long(argc, argv, "f:ht:b:dpqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -845,6 +851,9 @@ static int img_commit(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -877,6 +886,7 @@ static int img_commit(int argc, char **argv)
     }
 
     flags = BDRV_O_RDWR | BDRV_O_UNMAP;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -1135,6 +1145,7 @@ static int img_compare(int argc, char **argv)
     uint64_t progress_base;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     cache = BDRV_DEFAULT_CACHE;
     for (;;) {
@@ -1144,7 +1155,7 @@ static int img_compare(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:F:T:pqs",
+        c = getopt_long(argc, argv, "hf:F:T:pqsL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -1172,6 +1183,9 @@ static int img_compare(int argc, char **argv)
         case 's':
             strict = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -1211,6 +1225,7 @@ static int img_compare(int argc, char **argv)
     qemu_progress_init(progress, 2.0);
 
     flags = 0;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", cache);
@@ -1742,6 +1757,7 @@ static int img_convert(int argc, char **argv)
     QemuOpts *sn_opts = NULL;
     ImgConvertState state;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     out_fmt = "raw";
@@ -1757,7 +1773,7 @@ static int img_convert(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qn",
+        c = getopt_long(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qnL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -1846,6 +1862,9 @@ static int img_convert(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case 'n':
             skip_create = 1;
             break;
@@ -1896,6 +1915,7 @@ static int img_convert(int argc, char **argv)
     }
 
     src_flags = 0;
+    src_flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
     if (ret < 0) {
         error_report("Invalid source cache option: %s", src_cache);
@@ -2045,6 +2065,7 @@ static int img_convert(int argc, char **argv)
     }
 
     flags = min_sparse ? (BDRV_O_RDWR | BDRV_O_UNMAP) : BDRV_O_RDWR;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -2225,12 +2246,14 @@ static gboolean str_equal_func(gconstpointer a, gconstpointer b)
 static ImageInfoList *collect_image_info_list(bool image_opts,
                                               const char *filename,
                                               const char *fmt,
-                                              bool chain)
+                                              bool chain,
+                                              bool nolock)
 {
     ImageInfoList *head = NULL;
     ImageInfoList **last = &head;
     GHashTable *filenames;
     Error *err = NULL;
+    int flags;
 
     filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
 
@@ -2247,8 +2270,9 @@ static ImageInfoList *collect_image_info_list(bool image_opts,
         }
         g_hash_table_insert(filenames, (gpointer)filename, NULL);
 
-        blk = img_open(image_opts, filename, fmt,
-                       BDRV_O_NO_BACKING | BDRV_O_NO_IO, false, false);
+        flags = BDRV_O_NO_BACKING | BDRV_O_NO_IO;
+        flags |= nolock ? BDRV_O_NO_LOCK : 0;
+        blk = img_open(image_opts, filename, fmt, flags, false, false);
         if (!blk) {
             goto err;
         }
@@ -2301,6 +2325,7 @@ static int img_info(int argc, char **argv)
     ImageInfoList *list;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     fmt = NULL;
     output = NULL;
@@ -2315,7 +2340,7 @@ static int img_info(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:h",
+        c = getopt_long(argc, argv, "f:hL",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -2328,6 +2353,9 @@ static int img_info(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OUTPUT:
             output = optarg;
             break;
@@ -2368,7 +2396,7 @@ static int img_info(int argc, char **argv)
         return 1;
     }
 
-    list = collect_image_info_list(image_opts, filename, fmt, chain);
+    list = collect_image_info_list(image_opts, filename, fmt, chain, nolock);
     if (!list) {
         return 1;
     }
@@ -2515,6 +2543,8 @@ static int img_map(int argc, char **argv)
     int ret = 0;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool nolock = false;
+    int flags;
 
     fmt = NULL;
     output = NULL;
@@ -2528,7 +2558,7 @@ static int img_map(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:h",
+        c = getopt_long(argc, argv, "f:hL",
                         long_options, &option_index);
         if (c == -1) {
             break;
@@ -2541,6 +2571,9 @@ static int img_map(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OUTPUT:
             output = optarg;
             break;
@@ -2578,7 +2611,8 @@ static int img_map(int argc, char **argv)
         return 1;
     }
 
-    blk = img_open(image_opts, filename, fmt, 0, false, false);
+    flags = nolock ? BDRV_O_NO_LOCK : 0;
+    blk = img_open(image_opts, filename, fmt, flags, false, false);
     if (!blk) {
         return 1;
     }
@@ -2641,6 +2675,7 @@ static int img_snapshot(int argc, char **argv)
     bool quiet = false;
     Error *err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     bdrv_oflags = BDRV_O_RDWR;
     /* Parse commandline parameters */
@@ -2651,7 +2686,7 @@ static int img_snapshot(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "la:c:d:hq",
+        c = getopt_long(argc, argv, "la:c:d:hqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -2696,6 +2731,9 @@ static int img_snapshot(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -2722,6 +2760,7 @@ static int img_snapshot(int argc, char **argv)
         return 1;
     }
 
+    bdrv_oflags |= nolock ? BDRV_O_NO_LOCK : 0;
     /* Open the image */
     blk = img_open(image_opts, filename, NULL, bdrv_oflags, false, quiet);
     if (!blk) {
@@ -2791,6 +2830,7 @@ static int img_rebase(int argc, char **argv)
     bool quiet = false;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     /* Parse commandline parameters */
     fmt = NULL;
@@ -2805,7 +2845,7 @@ static int img_rebase(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "hf:F:b:upt:T:q",
+        c = getopt_long(argc, argv, "hf:F:b:upt:T:qL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -2836,6 +2876,9 @@ static int img_rebase(int argc, char **argv)
         case 'T':
             src_cache = optarg;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case 'q':
             quiet = true;
             break;
@@ -2876,6 +2919,7 @@ static int img_rebase(int argc, char **argv)
     qemu_progress_print(0, 100);
 
     flags = BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0);
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
@@ -3134,6 +3178,8 @@ static int img_resize(int argc, char **argv)
     BlockBackend *blk = NULL;
     QemuOpts *param;
     Error *local_err = NULL;
+    int flags;
+    bool nolock = false;
 
     static QemuOptsList resize_options = {
         .name = "resize_options",
@@ -3168,7 +3214,7 @@ static int img_resize(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "f:hq",
+        c = getopt_long(argc, argv, "f:hqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -3184,6 +3230,9 @@ static int img_resize(int argc, char **argv)
         case 'q':
             quiet = true;
             break;
+        case 'L':
+            nolock = true;
+            break;
         case OPTION_OBJECT: {
             QemuOpts *opts;
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
@@ -3236,8 +3285,9 @@ static int img_resize(int argc, char **argv)
     n = qemu_opt_get_size(param, BLOCK_OPT_SIZE, 0);
     qemu_opts_del(param);
 
-    blk = img_open(image_opts, filename, fmt,
-                   BDRV_O_RDWR, false, quiet);
+    flags = BDRV_O_RDWR;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
+    blk = img_open(image_opts, filename, fmt, flags, false, quiet);
     if (!blk) {
         ret = -1;
         goto out;
@@ -3299,6 +3349,7 @@ static int img_amend(int argc, char **argv)
     BlockDriverState *bs = NULL;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool nolock = false;
 
     cache = BDRV_DEFAULT_CACHE;
     for (;;) {
@@ -3308,7 +3359,7 @@ static int img_amend(int argc, char **argv)
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
-        c = getopt_long(argc, argv, "ho:f:t:pq",
+        c = getopt_long(argc, argv, "ho:f:t:pqL",
                         long_options, NULL);
         if (c == -1) {
             break;
@@ -3345,6 +3396,9 @@ static int img_amend(int argc, char **argv)
             case 'q':
                 quiet = true;
                 break;
+            case 'L':
+                nolock = true;
+                break;
             case OPTION_OBJECT:
                 opts = qemu_opts_parse_noisily(&qemu_object_opts,
                                                optarg, true);
@@ -3391,6 +3445,7 @@ static int img_amend(int argc, char **argv)
     }
 
     flags = BDRV_O_RDWR;
+    flags |= nolock ? BDRV_O_NO_LOCK : 0;
     ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
     if (ret < 0) {
         error_report("Invalid cache option: %s", cache);
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.7 v2 09/17] qemu-img: Add "-L" option to sub commands

[Denis V. Lunev]

On 04/15/2016 06:27 AM, Fam Zheng wrote:
> If specified, BDRV_O_NO_LOCK flag will be set when opening the image.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   qemu-img.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 72 insertions(+), 17 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 1697762..327be44 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
pls fix help message near

static void QEMU_NORETURN help(void)
{
     const char *help_msg =
            QEMU_IMG_VERSION
            "usage: qemu-img command [command options]\n"
            "QEMU disk image utility\n"
            "\n"
            "Command syntax:\n"
#define DEF(option, callback, arg_string)        \
            "  " arg_string "\n"
#include "qemu-img-cmds.h"
#undef DEF
#undef GEN_DOCS


IMHO img_create should also take lock if the image exists already
to validate that there is no process on top of it.


> @@ -600,6 +600,7 @@ static int img_check(int argc, char **argv)
>       bool quiet = false;
>       Error *local_err = NULL;
>       bool image_opts = false;
> +    bool nolock = false;
>   
>       fmt = NULL;
>       output = NULL;
> @@ -616,7 +617,7 @@ static int img_check(int argc, char **argv)
>               {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
>               {0, 0, 0, 0}
>           };
> -        c = getopt_long(argc, argv, "hf:r:T:q",
> +        c = getopt_long(argc, argv, "hf:r:T:qL",
>                           long_options, &option_index);
>           if (c == -1) {
>               break;
> @@ -650,6 +651,9 @@ static int img_check(int argc, char **argv)
>           case 'q':
>               quiet = true;
>               break;
> +        case 'L':
> +            nolock = true;
> +            break;
I think that you could fix flags just here as done for 'r', i.e.
                              flags |= BDRV_O_NO_LOCK

It would be better to switch all other places to this style. Some
tweaks to old code would be necessary.

Though this is personal and does not block the review.

Re: [Qemu-devel] [PATCH for-2.7 v2 09/17] qemu-img: Add "-L" option to sub commands

[Denis V. Lunev]

On 04/16/2016 05:29 PM, Denis V. Lunev wrote:
> On 04/15/2016 06:27 AM, Fam Zheng wrote:
>> If specified, BDRV_O_NO_LOCK flag will be set when opening the image.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>   qemu-img.c | 89 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++------------
>>   1 file changed, 72 insertions(+), 17 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 1697762..327be44 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
> pls fix help message near
>
> static void QEMU_NORETURN help(void)
> {
>     const char *help_msg =
>            QEMU_IMG_VERSION
>            "usage: qemu-img command [command options]\n"
>            "QEMU disk image utility\n"
>            "\n"
>            "Command syntax:\n"
> #define DEF(option, callback, arg_string)        \
>            "  " arg_string "\n"
> #include "qemu-img-cmds.h"
> #undef DEF
> #undef GEN_DOCS
>
ah, I see this in the next patch. Though the question about img_create
is still actual.



>
> IMHO img_create should also take lock if the image exists already
> to validate that there is no process on top of it.
>
>
>> @@ -600,6 +600,7 @@ static int img_check(int argc, char **argv)
>>       bool quiet = false;
>>       Error *local_err = NULL;
>>       bool image_opts = false;
>> +    bool nolock = false;
>>         fmt = NULL;
>>       output = NULL;
>> @@ -616,7 +617,7 @@ static int img_check(int argc, char **argv)
>>               {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
>>               {0, 0, 0, 0}
>>           };
>> -        c = getopt_long(argc, argv, "hf:r:T:q",
>> +        c = getopt_long(argc, argv, "hf:r:T:qL",
>>                           long_options, &option_index);
>>           if (c == -1) {
>>               break;
>> @@ -650,6 +651,9 @@ static int img_check(int argc, char **argv)
>>           case 'q':
>>               quiet = true;
>>               break;
>> +        case 'L':
>> +            nolock = true;
>> +            break;
> I think that you could fix flags just here as done for 'r', i.e.
>                              flags |= BDRV_O_NO_LOCK
>
> It would be better to switch all other places to this style. Some
> tweaks to old code would be necessary.
>
> Though this is personal and does not block the review.

Re: [Qemu-devel] [PATCH for-2.7 v2 09/17] qemu-img: Add "-L" option to sub commands

[Fam Zheng]

On Sat, 04/16 17:29, Denis V. Lunev wrote:
> On 04/15/2016 06:27 AM, Fam Zheng wrote:
> >If specified, BDRV_O_NO_LOCK flag will be set when opening the image.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  qemu-img.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 72 insertions(+), 17 deletions(-)
> >
> >diff --git a/qemu-img.c b/qemu-img.c
> >index 1697762..327be44 100644
> >--- a/qemu-img.c
> >+++ b/qemu-img.c
> pls fix help message near
> 
> static void QEMU_NORETURN help(void)
> {
>     const char *help_msg =
>            QEMU_IMG_VERSION
>            "usage: qemu-img command [command options]\n"
>            "QEMU disk image utility\n"
>            "\n"
>            "Command syntax:\n"
> #define DEF(option, callback, arg_string)        \
>            "  " arg_string "\n"
> #include "qemu-img-cmds.h"
> #undef DEF
> #undef GEN_DOCS
> 
> 
> IMHO img_create should also take lock if the image exists already
> to validate that there is no process on top of it.

Yes, good point.

> 
> 
> >@@ -600,6 +600,7 @@ static int img_check(int argc, char **argv)
> >      bool quiet = false;
> >      Error *local_err = NULL;
> >      bool image_opts = false;
> >+    bool nolock = false;
> >      fmt = NULL;
> >      output = NULL;
> >@@ -616,7 +617,7 @@ static int img_check(int argc, char **argv)
> >              {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
> >              {0, 0, 0, 0}
> >          };
> >-        c = getopt_long(argc, argv, "hf:r:T:q",
> >+        c = getopt_long(argc, argv, "hf:r:T:qL",
> >                          long_options, &option_index);
> >          if (c == -1) {
> >              break;
> >@@ -650,6 +651,9 @@ static int img_check(int argc, char **argv)
> >          case 'q':
> >              quiet = true;
> >              break;
> >+        case 'L':
> >+            nolock = true;
> >+            break;
> I think that you could fix flags just here as done for 'r', i.e.
>                              flags |= BDRV_O_NO_LOCK
> 
> It would be better to switch all other places to this style. Some
> tweaks to old code would be necessary.
> 
> Though this is personal and does not block the review.

Yes, I can look into that.

Fam

[Qemu-devel] [PATCH for-2.7 v2 10/17] qemu-img: Update documentation of "-L" option

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img-cmds.hx | 44 ++++++++++++++++++++++----------------------
 qemu-img.c       |  1 +
 qemu-img.texi    |  3 +++
 3 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index e7cded6..fa87942 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -10,68 +10,68 @@ STEXI
 ETEXI
 
 DEF("check", img_check,
-    "check [-q] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
+    "check [-q] [-L] [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [-r [leaks | all]] [-T src_cache] filename")
 STEXI
-@item check [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
+@item check [--object @var{objectdef}] [--image-opts] [-q] [-L] [-f @var{fmt}] [--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] @var{filename}
 ETEXI
 
 DEF("create", img_create,
-    "create [-q] [--object objectdef] [--image-opts] [-f fmt] [-o options] filename [size]")
+    "create [-q] [-L] [--object objectdef] [--image-opts] [-f fmt] [-o options] filename [size]")
 STEXI
-@item create [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
+@item create [--object @var{objectdef}] [--image-opts] [-q] [-L] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
 ETEXI
 
 DEF("commit", img_commit,
-    "commit [-q] [--object objectdef] [--image-opts] [-f fmt] [-t cache] [-b base] [-d] [-p] filename")
+    "commit [-q] [-L] [--object objectdef] [--image-opts] [-f fmt] [-t cache] [-b base] [-d] [-p] filename")
 STEXI
-@item commit [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-t @var{cache}] [-b @var{base}] [-d] [-p] @var{filename}
+@item commit [--object @var{objectdef}] [--image-opts] [-q] [-L] [-f @var{fmt}] [-t @var{cache}] [-b @var{base}] [-d] [-p] @var{filename}
 ETEXI
 
 DEF("compare", img_compare,
-    "compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-s] filename1 filename2")
+    "compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-L] [-s] filename1 filename2")
 STEXI
-@item compare [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-F @var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] @var{filename1} @var{filename2}
+@item compare [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-F @var{fmt}] [-T @var{src_cache}] [-p] [-q] [-L] [-s] @var{filename1} @var{filename2}
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
+    "convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-L] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] filename [filename2 [...]] output_filename")
 STEXI
-@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-L] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("info", img_info,
-    "info [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] [--backing-chain] filename")
+    "info [--object objectdef] [--image-opts] [-f fmt] [-L] [--output=ofmt] [--backing-chain] filename")
 STEXI
-@item info [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
+@item info [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-L] [--output=@var{ofmt}] [--backing-chain] @var{filename}
 ETEXI
 
 DEF("map", img_map,
-    "map [--object objectdef] [--image-opts] [-f fmt] [--output=ofmt] filename")
+    "map [--object objectdef] [--image-opts] [-f fmt] [-L] [--output=ofmt] filename")
 STEXI
-@item map [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [--output=@var{ofmt}] @var{filename}
+@item map [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-L] [--output=@var{ofmt}] @var{filename}
 ETEXI
 
 DEF("snapshot", img_snapshot,
-    "snapshot [--object objectdef] [--image-opts] [-q] [-l | -a snapshot | -c snapshot | -d snapshot] filename")
+    "snapshot [--object objectdef] [--image-opts] [-q] [-L] [-l | -a snapshot | -c snapshot | -d snapshot] filename")
 STEXI
-@item snapshot [--object @var{objectdef}] [--image-opts] [-q] [-l | -a @var{snapshot} | -c @var{snapshot} | -d @var{snapshot}] @var{filename}
+@item snapshot [--object @var{objectdef}] [--image-opts] [-q] [-L] [-l | -a @var{snapshot} | -c @var{snapshot} | -d @var{snapshot}] @var{filename}
 ETEXI
 
 DEF("rebase", img_rebase,
-    "rebase [--object objectdef] [--image-opts] [-q] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
+    "rebase [--object objectdef] [--image-opts] [-q] [-L] [-f fmt] [-t cache] [-T src_cache] [-p] [-u] -b backing_file [-F backing_fmt] filename")
 STEXI
-@item rebase [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
+@item rebase [--object @var{objectdef}] [--image-opts] [-q] [-L] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-p] [-u] -b @var{backing_file} [-F @var{backing_fmt}] @var{filename}
 ETEXI
 
 DEF("resize", img_resize,
-    "resize [--object objectdef] [--image-opts] [-q] filename [+ | -]size")
+    "resize [--object objectdef] [--image-opts] [-q] [-L] filename [+ | -]size")
 STEXI
-@item resize [--object @var{objectdef}] [--image-opts] [-q] @var{filename} [+ | -]@var{size}
+@item resize [--object @var{objectdef}] [--image-opts] [-q] [-L] @var{filename} [+ | -]@var{size}
 ETEXI
 
 DEF("amend", img_amend,
-    "amend [--object objectdef] [--image-opts] [-p] [-q] [-f fmt] [-t cache] -o options filename")
+    "amend [--object objectdef] [--image-opts] [-p] [-q] [-L] [-f fmt] [-t cache] -o options filename")
 STEXI
-@item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
+@item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-L] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
 @end table
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index 327be44..d737934 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -130,6 +130,7 @@ static void QEMU_NORETURN help(void)
            "  '-h' with or without a command shows this help and lists the supported formats\n"
            "  '-p' show progress of command (only certain commands)\n"
            "  '-q' use Quiet mode - do not print any output (except errors)\n"
+           "  '-L' disables image locking\n"
            "  '-S' indicates the consecutive number of bytes (defaults to 4k) that must\n"
            "       contain only zeros for qemu-img to create a sparse image during\n"
            "       conversion. If the number of bytes is 0, the source will not be scanned for\n"
diff --git a/qemu-img.texi b/qemu-img.texi
index afaebdd..b91f2e1 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -77,6 +77,9 @@ progress is reported when the process receives a @code{SIGUSR1} signal.
 @item -q
 Quiet mode - do not print any output (except errors). There's no progress bar
 in case both @var{-q} and @var{-p} options are used.
+@item -L
+disables image locking. The image will not be locked, other processes can
+access and lock this image while we are using it.
 @item -S @var{size}
 indicates the consecutive number of bytes that must contain only zeros
 for qemu-img to create a sparse image during conversion. This value is rounded
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 v2 11/17] qemu-nbd: Add "--no-lock/-L" option

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-nbd.c    | 6 +++++-
 qemu-nbd.texi | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index b5751f8..37da7a9 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -464,7 +464,7 @@ int main(int argc, char **argv)
     off_t fd_size;
     QemuOpts *sn_opts = NULL;
     const char *sn_id_or_name = NULL;
-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:";
+    const char *sopt = "hVb:o:p:rsnLP:c:dvk:e:f:tl:x:";
     struct option lopt[] = {
         { "help", no_argument, NULL, 'h' },
         { "version", no_argument, NULL, 'V' },
@@ -473,6 +473,7 @@ int main(int argc, char **argv)
         { "socket", required_argument, NULL, 'k' },
         { "offset", required_argument, NULL, 'o' },
         { "read-only", no_argument, NULL, 'r' },
+        { "no-lock", no_argument, NULL, 'L' },
         { "partition", required_argument, NULL, 'P' },
         { "connect", required_argument, NULL, 'c' },
         { "disconnect", no_argument, NULL, 'd' },
@@ -628,6 +629,9 @@ int main(int argc, char **argv)
             nbdflags |= NBD_FLAG_READ_ONLY;
             flags &= ~BDRV_O_RDWR;
             break;
+        case 'L':
+            flags |= BDRV_O_NO_LOCK;
+            break;
         case 'P':
             partition = strtol(optarg, &end, 0);
             if (*end) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index 9f23343..6b7b1d1 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -43,6 +43,8 @@ Force the use of the block driver for format @var{fmt} instead of
 auto-detecting
 @item -r, --read-only
 Export the disk as read-only
+@item -L, --no-lock
+Disable image locking
 @item -P, --partition=@var{num}
 Only expose partition @var{num}
 @item -s, --snapshot
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.7 v2 11/17] qemu-nbd: Add "--no-lock/-L" option

[Denis V. Lunev]

On 04/15/2016 06:28 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   qemu-nbd.c    | 6 +++++-
>   qemu-nbd.texi | 2 ++
>   2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index b5751f8..37da7a9 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -464,7 +464,7 @@ int main(int argc, char **argv)
>       off_t fd_size;
>       QemuOpts *sn_opts = NULL;
>       const char *sn_id_or_name = NULL;
> -    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:";
> +    const char *sopt = "hVb:o:p:rsnLP:c:dvk:e:f:tl:x:";
>       struct option lopt[] = {
>           { "help", no_argument, NULL, 'h' },
>           { "version", no_argument, NULL, 'V' },
> @@ -473,6 +473,7 @@ int main(int argc, char **argv)
>           { "socket", required_argument, NULL, 'k' },
>           { "offset", required_argument, NULL, 'o' },
>           { "read-only", no_argument, NULL, 'r' },
> +        { "no-lock", no_argument, NULL, 'L' },
>           { "partition", required_argument, NULL, 'P' },
>           { "connect", required_argument, NULL, 'c' },
>           { "disconnect", no_argument, NULL, 'd' },
> @@ -628,6 +629,9 @@ int main(int argc, char **argv)
>               nbdflags |= NBD_FLAG_READ_ONLY;
>               flags &= ~BDRV_O_RDWR;
>               break;
> +        case 'L':
> +            flags |= BDRV_O_NO_LOCK;
> +            break;
>           case 'P':
>               partition = strtol(optarg, &end, 0);
>               if (*end) {
> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index 9f23343..6b7b1d1 100644
> --- a/qemu-nbd.texi
> +++ b/qemu-nbd.texi
> @@ -43,6 +43,8 @@ Force the use of the block driver for format @var{fmt} instead of
>   auto-detecting
>   @item -r, --read-only
>   Export the disk as read-only
> +@item -L, --no-lock
> +Disable image locking
>   @item -P, --partition=@var{num}
>   Only expose partition @var{num}
>   @item -s, --snapshot
pls fix

static void usage(const char *name)
{
     (printf) (
"Usage: %s [OPTIONS] FILE\n"

Re: [Qemu-devel] [PATCH for-2.7 v2 11/17] qemu-nbd: Add "--no-lock/-L" option

[Fam Zheng]

On Sat, 04/16 17:32, Denis V. Lunev wrote:
> On 04/15/2016 06:28 AM, Fam Zheng wrote:
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  qemu-nbd.c    | 6 +++++-
> >  qemu-nbd.texi | 2 ++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> >
> >diff --git a/qemu-nbd.c b/qemu-nbd.c
> >index b5751f8..37da7a9 100644
> >--- a/qemu-nbd.c
> >+++ b/qemu-nbd.c
> >@@ -464,7 +464,7 @@ int main(int argc, char **argv)
> >      off_t fd_size;
> >      QemuOpts *sn_opts = NULL;
> >      const char *sn_id_or_name = NULL;
> >-    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:";
> >+    const char *sopt = "hVb:o:p:rsnLP:c:dvk:e:f:tl:x:";
> >      struct option lopt[] = {
> >          { "help", no_argument, NULL, 'h' },
> >          { "version", no_argument, NULL, 'V' },
> >@@ -473,6 +473,7 @@ int main(int argc, char **argv)
> >          { "socket", required_argument, NULL, 'k' },
> >          { "offset", required_argument, NULL, 'o' },
> >          { "read-only", no_argument, NULL, 'r' },
> >+        { "no-lock", no_argument, NULL, 'L' },
> >          { "partition", required_argument, NULL, 'P' },
> >          { "connect", required_argument, NULL, 'c' },
> >          { "disconnect", no_argument, NULL, 'd' },
> >@@ -628,6 +629,9 @@ int main(int argc, char **argv)
> >              nbdflags |= NBD_FLAG_READ_ONLY;
> >              flags &= ~BDRV_O_RDWR;
> >              break;
> >+        case 'L':
> >+            flags |= BDRV_O_NO_LOCK;
> >+            break;
> >          case 'P':
> >              partition = strtol(optarg, &end, 0);
> >              if (*end) {
> >diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> >index 9f23343..6b7b1d1 100644
> >--- a/qemu-nbd.texi
> >+++ b/qemu-nbd.texi
> >@@ -43,6 +43,8 @@ Force the use of the block driver for format @var{fmt} instead of
> >  auto-detecting
> >  @item -r, --read-only
> >  Export the disk as read-only
> >+@item -L, --no-lock
> >+Disable image locking
> >  @item -P, --partition=@var{num}
> >  Only expose partition @var{num}
> >  @item -s, --snapshot
> pls fix
> 
> static void usage(const char *name)
> {
>     (printf) (
> "Usage: %s [OPTIONS] FILE\n"
> 

Will do.

Fam

[Qemu-devel] [PATCH for-2.7 v2 12/17] qemu-iotests: 140: Disable image lock for qemu-io access

[Fam Zheng]

The VM is still on, the image locking check would complain.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/140 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140
index 05e4506..412b26f 100755
--- a/tests/qemu-iotests/140
+++ b/tests/qemu-iotests/140
@@ -71,7 +71,7 @@ _send_qemu_cmd $QEMU_HANDLE \
        'arguments': { 'device': 'drv' }}" \
     'return'
 
-$QEMU_IO_PROG -f raw -c 'read -P 42 0 64k' \
+$QEMU_IO_PROG -f raw -L -c 'read -P 42 0 64k' \
     "nbd+unix:///drv?socket=$TEST_DIR/nbd" 2>&1 \
     | _filter_qemu_io | _filter_nbd
 
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 v2 13/17] qemu-iotests: 046: Move version detection out from verify_io

[Fam Zheng]

So the image lock won't complain.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/046 | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
index e0be46c..40c4bc0 100755
--- a/tests/qemu-iotests/046
+++ b/tests/qemu-iotests/046
@@ -193,15 +193,7 @@ echo "== Verify image content =="
 
 function verify_io()
 {
-    if ($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > /dev/null); then
-        # For v2 images, discarded clusters are read from the backing file
-        # Keep the variable empty so that the backing file value can be used as
-        # the default below
-        discarded=
-    else
-        # Discarded clusters are zeroed for v3 or later
-        discarded=0
-    fi
+    discarded=$1
 
     echo read -P 0 0 0x10000
 
@@ -262,7 +254,17 @@ function verify_io()
     echo read -P 17  0x11c000 0x4000
 }
 
-verify_io | $QEMU_IO "$TEST_IMG" | _filter_qemu_io
+if ($QEMU_IMG info -f "$IMGFMT" "$TEST_IMG" | grep "compat: 0.10" > /dev/null); then
+    # For v2 images, discarded clusters are read from the backing file
+    # Keep the variable empty so that the backing file value can be used as
+    # the default below
+    discarded=
+else
+    # Discarded clusters are zeroed for v3 or later
+    discarded=0
+fi
+
+verify_io $discarded | $QEMU_IO -L "$TEST_IMG" | _filter_qemu_io
 
 _check_test_img
 
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 v2 14/17] qemu-iotests: Wait for QEMU processes before checking image in 091

[Fam Zheng]

We should wait for the QEMU process to terminate and close the image
before we check the data.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/091     | 3 +++
 tests/qemu-iotests/091.out | 1 +
 2 files changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
index 32bbd56..32aa5c3 100755
--- a/tests/qemu-iotests/091
+++ b/tests/qemu-iotests/091
@@ -95,6 +95,9 @@ echo "vm2: qemu process running successfully"
 echo "vm2: flush io, and quit"
 _send_qemu_cmd $h2 'qemu-io disk flush' "(qemu)"
 _send_qemu_cmd $h2 'quit' ""
+echo "vm1: quit"
+_send_qemu_cmd $h1 'quit' ""
+wait
 
 echo "Check image pattern"
 ${QEMU_IO} -c "read -P 0x22 0 4M" "${TEST_IMG}" | _filter_testdir | _filter_qemu_io
diff --git a/tests/qemu-iotests/091.out b/tests/qemu-iotests/091.out
index 5017f8c..6658ca8 100644
--- a/tests/qemu-iotests/091.out
+++ b/tests/qemu-iotests/091.out
@@ -18,6 +18,7 @@ vm1: live migration completed
 vm2: qemu-io disk write complete
 vm2: qemu process running successfully
 vm2: flush io, and quit
+vm1: quit
 Check image pattern
 read 4194304/4194304 bytes at offset 0
 4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 v2 15/17] qemu-iotests: Disable image lock when checking test image

[Fam Zheng]

The VM is running, qemu-io would fail the lock acquisition.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/030 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 3ac2443..fa996ef 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -95,7 +95,7 @@ class TestSingleDrive(iotests.QMPTestCase):
         self.assert_no_active_block_jobs()
 
         # The image map is empty before the operation
-        empty_map = qemu_io('-f', iotests.imgfmt, '-c', 'map', test_img)
+        empty_map = qemu_io('-L', '-f', iotests.imgfmt, '-c', 'map', test_img)
 
         # This is a no-op: no data should ever be copied from the base image
         result = self.vm.qmp('block-stream', device='drive0', base=mid_img)
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 v2 16/17] block: Turn on image locking by default

[Fam Zheng]

Now that test cases are covered, we can turn it on.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 93bd43e..87d22c3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -383,7 +383,7 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
             }
         }
 
-        if (!qemu_opt_get_bool(opts, "lock-image", false)) {
+        if (!qemu_opt_get_bool(opts, "lock-image", true)) {
             *bdrv_flags |= BDRV_O_NO_LOCK;
         }
     }
-- 
2.8.0

[Qemu-devel] [PATCH for-2.7 v2 17/17] qemu-iotests: Add test case 152 for image locking

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/152     | 106 ++++++++++++++++++++
 tests/qemu-iotests/152.out | 237 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 344 insertions(+)
 create mode 100755 tests/qemu-iotests/152
 create mode 100644 tests/qemu-iotests/152.out

diff --git a/tests/qemu-iotests/152 b/tests/qemu-iotests/152
new file mode 100755
index 0000000..552a0f0
--- /dev/null
+++ b/tests/qemu-iotests/152
@@ -0,0 +1,106 @@
+#!/bin/bash
+#
+# Test image locking
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=famz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+    rm -f "${TEST_IMG}.base"
+    rm -f "${TEST_IMG}.convert"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+size=32M
+
+_run_cmd()
+{
+    echo
+    (echo "$@"; "$@" 2>&1 1>/dev/null) | _filter_testdir
+}
+
+for opts1 in "" "readonly=on" "lock-image=off"; do
+    echo
+    echo "== Creating base image =="
+    TEST_IMG="${TEST_IMG}.base" _make_test_img $size
+
+    echo
+    echo "== Creating test image =="
+    $QEMU_IMG create -f $IMGFMT "${TEST_IMG}" -b ${TEST_IMG}.base | _filter_img_create
+
+    echo
+    echo "== Launching QEMU $opts1 =="
+    _launch_qemu -drive file="${TEST_IMG}",if=none,$opts1
+    h=$QEMU_HANDLE
+
+    for opts2 in "" "lock-image=on" "lock-image=off" "lock-image=on,readonly=on"; do
+        echo
+        echo "== Launching another QEMU $opts2 =="
+        echo "quit" | \
+            $QEMU -nographic -monitor stdio \
+            -drive file="${TEST_IMG}",if=none,$opts2 2>&1 1>/dev/null | \
+            _filter_testdir | _filter_qemu
+    done
+
+    for L in "" "-L"; do
+
+        echo
+        echo "== Running utility commands $(test -n "$L" && echo "(lock disabled)") =="
+        _run_cmd $QEMU_IO $L -c "read 0 512" "${TEST_IMG}"
+        _run_cmd $QEMU_IO $L -r -c "read 0 512" "${TEST_IMG}"
+        _run_cmd $QEMU_IMG info        $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG check       $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG compare     $L "${TEST_IMG}" "${TEST_IMG}"
+        _run_cmd $QEMU_IMG map         $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG amend       $L "${TEST_IMG}" -o ""
+        _run_cmd $QEMU_IMG commit      $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG resize      $L "${TEST_IMG}" $size
+        _run_cmd $QEMU_IMG rebase      $L "${TEST_IMG}" -b "${TEST_IMG}.base"
+        _run_cmd $QEMU_IMG snapshot -l $L "${TEST_IMG}"
+        _run_cmd $QEMU_IMG convert     $L "${TEST_IMG}" "${TEST_IMG}.convert"
+    done
+    _send_qemu_cmd $h 'quit' ""
+    echo
+    echo "Round done"
+    _cleanup_qemu
+done
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/152.out b/tests/qemu-iotests/152.out
new file mode 100644
index 0000000..fa4f86e
--- /dev/null
+++ b/tests/qemu-iotests/152.out
@@ -0,0 +1,237 @@
+QA output created by 152
+
+== Creating base image ==
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=33554432
+
+== Creating test image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT.base
+
+== Launching QEMU  ==
+
+== Launching another QEMU  ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,: Failed to lock image
+
+== Launching another QEMU lock-image=on ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-image=on: Failed to lock image
+
+== Launching another QEMU lock-image=off ==
+
+== Launching another QEMU lock-image=on,readonly=on ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-image=on,readonly=on: Failed to lock image
+
+== Running utility commands  ==
+
+_qemu_io_wrapper -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: Failed to lock image
+no file open, try 'help open'
+
+_qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: Failed to lock image
+no file open, try 'help open'
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper check TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper compare TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper map TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper amend TEST_DIR/t.qcow2 -o 
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper commit TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper resize TEST_DIR/t.qcow2 32M
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper rebase TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper snapshot -l TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper convert TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+== Running utility commands (lock disabled) ==
+
+_qemu_io_wrapper -L -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -L -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -L TEST_DIR/t.qcow2 -o 
+
+_qemu_img_wrapper commit -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize -L TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase -L TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+Round done
+
+== Creating base image ==
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=33554432
+
+== Creating test image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT.base
+
+== Launching QEMU readonly=on ==
+
+== Launching another QEMU  ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,: Failed to lock image
+
+== Launching another QEMU lock-image=on ==
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,lock-image=on: Failed to lock image
+
+== Launching another QEMU lock-image=off ==
+
+== Launching another QEMU lock-image=on,readonly=on ==
+
+== Running utility commands  ==
+
+_qemu_io_wrapper -c read 0 512 TEST_DIR/t.qcow2
+can't open device TEST_DIR/t.qcow2: Failed to lock image
+no file open, try 'help open'
+
+_qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend TEST_DIR/t.qcow2 -o 
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper commit TEST_DIR/t.qcow2
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper resize TEST_DIR/t.qcow2 32M
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper rebase TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to lock image
+
+_qemu_img_wrapper snapshot -l TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+== Running utility commands (lock disabled) ==
+
+_qemu_io_wrapper -L -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -L -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -L TEST_DIR/t.qcow2 -o 
+
+_qemu_img_wrapper commit -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize -L TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase -L TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+Round done
+
+== Creating base image ==
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=33554432
+
+== Creating test image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=33554432 backing_file=TEST_DIR/t.IMGFMT.base
+
+== Launching QEMU lock-image=off ==
+
+== Launching another QEMU  ==
+
+== Launching another QEMU lock-image=on ==
+
+== Launching another QEMU lock-image=off ==
+
+== Launching another QEMU lock-image=on,readonly=on ==
+
+== Running utility commands  ==
+
+_qemu_io_wrapper -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend TEST_DIR/t.qcow2 -o 
+
+_qemu_img_wrapper commit TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+== Running utility commands (lock disabled) ==
+
+_qemu_io_wrapper -L -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_io_wrapper -L -r -c read 0 512 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper info -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper check -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper compare -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2
+
+_qemu_img_wrapper map -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper amend -L TEST_DIR/t.qcow2 -o 
+
+_qemu_img_wrapper commit -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper resize -L TEST_DIR/t.qcow2 32M
+
+_qemu_img_wrapper rebase -L TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+
+_qemu_img_wrapper snapshot -l -L TEST_DIR/t.qcow2
+
+_qemu_img_wrapper convert -L TEST_DIR/t.qcow2 TEST_DIR/t.qcow2.convert
+
+Round done
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 2952b9d..822953b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -152,3 +152,4 @@
 148 rw auto quick
 149 rw auto sudo
 150 rw auto quick
+152 rw auto quick
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.7 v2 00/17] block: Lock images when opening

[Denis V. Lunev]

On 04/15/2016 06:27 AM, Fam Zheng wrote:
> v2: Lock byte 1 in the image itself, no lock file. [Daniel]
>      Fix migration (image are not locked in bdrv_open_common if
>      BDRV_O_INACTIVE). [Denis]
>      Simplify test case fixes because of the above.
>      Add lock for RBD.
>      Add "-L" option in "qemu-img" and "qemu-nbd" too. [Denis]
>      Add test case for image locking.
>
> Too many troubles have been caused by two processes writing to the same image
> unexpectedly. This series introduces automatical image locking into QEMU to
> avoid such tragedy. With this, the user won't be able to open the image from
> two processes (e.g. using qemu-img when the image is attached to the guest).
>
> Underneath is the fcntl syscall that locks the local file, similar to what is
> already used in libvirt virtlockd. The difference is that libvirt locks byte 0,
> we lock byte 1.  Read only openings are mapped to shared locks.
>
> The alternative locking API, flock(2), cannot protect host NFS mount points, so
> it's not used.
>
> Gluster locking is also implemented wrapping glfs_posix_lock in patch 6. It's
> only lightly tested.
>
> All other drivers that don't implement .bdrv_lockf are always permissive and
> does no checking.
>
> In the future, the intention is that image format drivers that introduce
> locking mechanisms could also fit into this API.
>
> Fam Zheng (17):
>    block: Add BDRV_O_NO_LOCK
>    qapi: Add lock-image in blockdev-add options
>    blockdev: Add and parse "lock-image" option for block devices
>    block: Introduce image file locking
>    raw-posix: Implement .bdrv_lockf
>    gluster: Implement .bdrv_lockf
>    rbd: Implement image locking
>    qemu-io: Add "-L" option for BDRV_O_NO_LOCK
>    qemu-img: Add "-L" option to sub commands
>    qemu-img: Update documentation of "-L" option
>    qemu-nbd: Add "--no-lock/-L" option
>    qemu-iotests: 140: Disable image lock for qemu-io access
>    qemu-iotests: 046: Move version detection out from verify_io
>    qemu-iotests: Wait for QEMU processes before checking image in 091
>    qemu-iotests: Disable image lock when checking test image
>    block: Turn on image locking by default
>    qemu-iotests: Add test case 152 for image locking
>
>   block.c                    |  42 ++++++++
>   block/gluster.c            |  30 ++++++
>   block/raw-posix.c          |  35 +++++++
>   block/rbd.c                |  25 +++++
>   blockdev.c                 |   8 ++
>   include/block/block.h      |   1 +
>   include/block/block_int.h  |  12 +++
>   qapi/block-core.json       |   6 +-
>   qemu-img-cmds.hx           |  44 ++++-----
>   qemu-img.c                 |  90 +++++++++++++----
>   qemu-img.texi              |   3 +
>   qemu-io.c                  |  22 ++++-
>   qemu-nbd.c                 |   6 +-
>   qemu-nbd.texi              |   2 +
>   tests/qemu-iotests/030     |   2 +-
>   tests/qemu-iotests/046     |  22 +++--
>   tests/qemu-iotests/091     |   3 +
>   tests/qemu-iotests/091.out |   1 +
>   tests/qemu-iotests/140     |   2 +-
>   tests/qemu-iotests/152     | 106 ++++++++++++++++++++
>   tests/qemu-iotests/152.out | 237 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group   |   1 +
>   22 files changed, 645 insertions(+), 55 deletions(-)
>   create mode 100755 tests/qemu-iotests/152
>   create mode 100644 tests/qemu-iotests/152.out
>
done for now...

Re: [Qemu-devel] [PATCH for-2.7 v2 00/17] block: Lock images when opening

[Daniel P. Berrange]

On Fri, Apr 15, 2016 at 11:27:50AM +0800, Fam Zheng wrote:
> v2: Lock byte 1 in the image itself, no lock file. [Daniel]
>     Fix migration (image are not locked in bdrv_open_common if
>     BDRV_O_INACTIVE). [Denis]
>     Simplify test case fixes because of the above.
>     Add lock for RBD.
>     Add "-L" option in "qemu-img" and "qemu-nbd" too. [Denis]
>     Add test case for image locking.
> 
> Too many troubles have been caused by two processes writing to the same image
> unexpectedly. This series introduces automatical image locking into QEMU to
> avoid such tragedy. With this, the user won't be able to open the image from
> two processes (e.g. using qemu-img when the image is attached to the guest).
> 
> Underneath is the fcntl syscall that locks the local file, similar to what is
> already used in libvirt virtlockd. The difference is that libvirt locks byte 0,
> we lock byte 1.  Read only openings are mapped to shared locks.

I'm afraid the way you are using fcntl is not actually safe, because
fcntl does not correctly deal with concurrent usage within the same
VM. I have tested your series with the following sequence of steps.

Consider I have a pair of disk images:

 $ qemu-img create master.img 10G
 Formatting 'master.img', fmt=raw size=10737418240
 $ qemu-img create -f qcow2 -obacking_file=master.img overlay.img
 Formatting 'overlay.img', fmt=qcow2 size=10737418240 backing_file='master.img' encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16


Now I launch QEMU pointing it to overlay.img:

 $ qemu-system-x86_64 -drive file=/home/berrange/VirtualMachines/overlay.img  -monitor stdio
 QEMU 2.5.91 monitor - type 'help' for more information

Looking at the locks held, everything is correct:

 $lslocks  | grep qemu
 qemu-system-x86 28723  POSIX 192.5K WRITE 0          1          1 /home/berrange/VirtualMachines/overlay.img
 qemu-system-x86 28723  POSIX    10G READ  0          1          1 /home/berrange/VirtualMachines/master.img


Now see what happens when I hotplug a second disk image pointing
to master.img in write mode - this should be denied since writing
to master.img will invalidate the backing store used by the currently
open overlay.img:

(qemu) drive_add 0:1:1 file=/home/berrange/VirtualMachines/master.img,if=none
WARNING: Image format was not specified for '/home/berrange/VirtualMachines/master.img' and probing guessed raw.
         Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
         Specify the 'raw' format explicitly to remove the restrictions.
OK

It was mistakenly allowed. We should not allow writing to a disk
image that is used as backing store for an overlay. Now look at
the locks held:

$ lslocks  | grep qemu
qemu-system-x86 28723  POSIX 192.5K WRITE 0          1          1 /home/berrange/VirtualMachines/overlay.img
qemu-system-x86 28723  POSIX    10G WRITE 0          1          1 /home/berrange/VirtualMachines/master.img

The read lock on master.img has simply been "upgraded" to a write
lock. This is totally incorrect behaviour from POV of protecting
the disk images.

Now lets unplug the drive we just added:

(qemu) drive_del none0


And look at the locks held again:

$ lslocks  | grep qemu
qemu-system-x86 28723  POSIX 192.5K WRITE 0          1          1 /home/berrange/VirtualMachines/overlay.img

So we've now lost the lock on msater.img, despite the fact that
it is still open for reading as a backing store of overlay.img.


This is all caused by the problems I've mentioned in previous
discussions whereby  locks are scoped to the process, not the
file handle. eg opening a second file handle wil happily upgrde
the lock held by the first file handle from read to write.
Closing the second file handle will happily release the lock,
even though the first file handle is still open.


If you want todo locks inside of QEMU, you really can't rely
on delegating handling to each individual block driver instance.
You need to have a process global registry of which files you have
already locked, and check against that registry before even
attempting to open any file that might correspond to a disk image.
This registry needs to canonicalize the file path too, to allow
for possibility of QEMU being given differen paths to the same
file.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH for-2.7 v2 00/17] block: Lock images when opening

[Fam Zheng]

On Mon, 04/18 10:53, Daniel P. Berrange wrote:
> If you want todo locks inside of QEMU, you really can't rely
> on delegating handling to each individual block driver instance.
> You need to have a process global registry of which files you have
> already locked, and check against that registry before even
> attempting to open any file that might correspond to a disk image.
> This registry needs to canonicalize the file path too, to allow
> for possibility of QEMU being given differen paths to the same
> file.

Yes, you're completely right. Thank you for bringing up it again and so
clearly. I'll revise the code.

Fam