[Qemu-devel] [PATCH v0 0/2] enable/disable copy-on-read via qmp

[Qemu-devel] [PATCH v0 0/2] enable/disable copy-on-read via qmp

[Denis Plotnikov]

In some cases there is a need to perform a special access pattern to a disk
backend. This may be done by providing disk reading access to external readers.
In this case there should be the ability to enable/disable the copy-on-read mode
for the disk during VM runtime. 
By the moment, there is no such ability. The patch set adds qmp command to 
enable/disable the copy-on-read mode.

Denis Plotnikov (2):
  block: check for read-only on copy-on-read setting
  qmp: add block-set-copy-on-read command

 block.c               |  4 +---
 block/io.c            | 10 ++++++++--
 block/stream.c        |  5 +++--
 blockdev.c            | 38 ++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  2 +-
 qapi/block-core.json  | 20 ++++++++++++++++++++
 6 files changed, 71 insertions(+), 8 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH v0 1/2] block: check for read-only on copy-on-read setting

[Denis Plotnikov]

We should always check whether block device is in the
read-only state before enabling copy-on-read.
The patch embeds the check in the copy-on-read setter.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 block.c               |  4 +---
 block/io.c            | 10 ++++++++--
 block/stream.c        |  5 +++--
 include/block/block.h |  2 +-
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 50887087f3..e73fccd397 100644
--- a/block.c
+++ b/block.c
@@ -1370,9 +1370,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
     assert(atomic_read(&bs->copy_on_read) == 0);
 
     if (bs->open_flags & BDRV_O_COPY_ON_READ) {
-        if (!bs->read_only) {
-            bdrv_enable_copy_on_read(bs);
-        } else {
+        if (!bdrv_enable_copy_on_read(bs)) {
             error_setg(errp, "Can't use copy-on-read on read-only device");
             ret = -EINVAL;
             goto fail_opts;
diff --git a/block/io.c b/block/io.c
index b7beaeeb9f..6ec008dbdc 100644
--- a/block/io.c
+++ b/block/io.c
@@ -131,9 +131,15 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
  * use the feature without worrying about clobbering its previous state.
  * Copy-on-read stays enabled until all users have called to disable it.
  */
-void bdrv_enable_copy_on_read(BlockDriverState *bs)
+bool bdrv_enable_copy_on_read(BlockDriverState *bs)
 {
-    atomic_inc(&bs->copy_on_read);
+    if (bs->read_only) {
+        return false;
+    } else {
+        atomic_inc(&bs->copy_on_read);
+    }
+
+    return true;
 }
 
 void bdrv_disable_copy_on_read(BlockDriverState *bs)
diff --git a/block/stream.c b/block/stream.c
index 9264b68a1e..033e24f22c 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -130,8 +130,9 @@ static void coroutine_fn stream_run(void *opaque)
      * backing chain since the copy-on-read operation does not take base into
      * account.
      */
-    if (!base) {
-        bdrv_enable_copy_on_read(bs);
+    if (!base && !bdrv_enable_copy_on_read(bs)) {
+        ret = -EPERM;
+        goto out;
     }
 
     for ( ; offset < len; offset += n) {
diff --git a/include/block/block.h b/include/block/block.h
index e677080c4e..dddfd032f9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -490,7 +490,7 @@ void *qemu_try_blockalign(BlockDriverState *bs, size_t size);
 void *qemu_try_blockalign0(BlockDriverState *bs, size_t size);
 bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
 
-void bdrv_enable_copy_on_read(BlockDriverState *bs);
+bool bdrv_enable_copy_on_read(BlockDriverState *bs);
 void bdrv_disable_copy_on_read(BlockDriverState *bs);
 
 void bdrv_ref(BlockDriverState *bs);
-- 
2.17.0

[Qemu-devel] [PATCH v0 2/2] qmp: add block-set-copy-on-read command

[Denis Plotnikov]

The command enables/disables copy-on-read mode for VM's disk while
VM is running.

This is needed when using external disk readers to shape access pattern
to the disk backend.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 blockdev.c           | 38 ++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 20 ++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 4862323012..4a297dabef 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4276,6 +4276,44 @@ out:
     aio_context_release(aio_context);
 }
 
+void qmp_block_set_copy_on_read(const char *device, bool enable, Error **errp)
+{
+    Error *local_err = NULL;
+    AioContext *aio_context;
+    BlockDriverState *bs = bdrv_lookup_bs(device, device, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(aio_context);
+
+    if (enable) {
+        if (bs->open_flags & BDRV_O_COPY_ON_READ) {
+            error_setg(errp, "Can't enable copy-on-read for the device. "
+                             "Copy-on-read is already enabled.");
+        } else {
+            if (bdrv_enable_copy_on_read(bs)) {
+                bs->open_flags |= BDRV_O_COPY_ON_READ;
+            } else {
+                error_setg(errp, "Can't enable copy-on-read. "
+                                 "The device is read-only.");
+            }
+        }
+    } else {
+        if (bs->open_flags & BDRV_O_COPY_ON_READ) {
+            bs->open_flags &= ~BDRV_O_COPY_ON_READ;
+            bdrv_disable_copy_on_read(bs);
+        } else {
+            error_setg(errp, "Can't disable copy-on-read for the device. "
+                             "Copy-on-read is already disabled.");
+        }
+    }
+
+    aio_context_release(aio_context);
+}
+
 static BdrvChild *bdrv_find_child(BlockDriverState *parent_bs,
                                   const char *child_name)
 {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index fff23fc82b..7369a13009 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4701,6 +4701,26 @@
 { 'command': 'block-set-write-threshold',
   'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
 
+##
+# @block-set-copy-on-read:
+#
+# Enables and disables the copy-on-read property of a block device.
+#
+# @device: device or graph node name on which copy-on-read must be set.
+#
+# Since: 2.12
+#
+# Example:
+#
+# -> { "execute": "block-set-copy-on-read",
+#      "arguments": { "device": "scsi0-0-0-0",
+#                     "enable": true } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'block-set-copy-on-read',
+  'data': { 'device': 'str', 'enable': 'bool' } }
+
 ##
 # @x-blockdev-change:
 #
-- 
2.17.0

Re: [Qemu-devel] [PATCH v0 2/2] qmp: add block-set-copy-on-read command

[Eric Blake]

On 06/13/2018 10:47 AM, Denis Plotnikov wrote:
> The command enables/disables copy-on-read mode for VM's disk while
> VM is running.
> 
> This is needed when using external disk readers to shape access pattern
> to the disk backend.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---

Deferring thoughts on the actual design for later; this is just a 
cursory implementation review for now.


> +++ b/qapi/block-core.json
> @@ -4701,6 +4701,26 @@
>   { 'command': 'block-set-write-threshold',
>     'data': { 'node-name': 'str', 'write-threshold': 'uint64' } }
>   
> +##
> +# @block-set-copy-on-read:
> +#
> +# Enables and disables the copy-on-read property of a block device.
> +#
> +# @device: device or graph node name on which copy-on-read must be set.
> +#
> +# Since: 2.12

We've missed the 2.12 release.  This should be since 3.0.

> +#
> +# Example:
> +#
> +# -> { "execute": "block-set-copy-on-read",
> +#      "arguments": { "device": "scsi0-0-0-0",
> +#                     "enable": true } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'block-set-copy-on-read',
> +  'data': { 'device': 'str', 'enable': 'bool' } }

Missing documentation of @enable.

And just checking: is the current copy-on-read setting visible through 
an existing query-* command, or have you just introduced a write-only 
toggle?  If the state can be changed at runtime, then it needs to be 
easy to learn what the current state is.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v0 2/2] qmp: add block-set-copy-on-read command

[Max Reitz]

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

On 2018-06-13 18:02, Eric Blake wrote:
> On 06/13/2018 10:47 AM, Denis Plotnikov wrote:
>> The command enables/disables copy-on-read mode for VM's disk while
>> VM is running.
>>
>> This is needed when using external disk readers to shape access pattern
>> to the disk backend.
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> ---
> 
> Deferring thoughts on the actual design for later;

But why? ;-)

This patch would definitely be superseded by a block reconfiguration
command (i.e. presumably one that makes reopen accessible over QMP).
With such a command, you could insert or remove a copy-on-read filter
node at any point in time.

Since we definitely want block graph configuration, I don't think we
want to add special commands now.

Max


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

Re: [Qemu-devel] [PATCH v0 2/2] qmp: add block-set-copy-on-read command

[Kevin Wolf]

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

Am 13.06.2018 um 18:41 hat Max Reitz geschrieben:
> On 2018-06-13 18:02, Eric Blake wrote:
> > On 06/13/2018 10:47 AM, Denis Plotnikov wrote:
> >> The command enables/disables copy-on-read mode for VM's disk while
> >> VM is running.
> >>
> >> This is needed when using external disk readers to shape access pattern
> >> to the disk backend.
> >>
> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> >> ---
> > 
> > Deferring thoughts on the actual design for later;
> 
> But why? ;-)
> 
> This patch would definitely be superseded by a block reconfiguration
> command (i.e. presumably one that makes reopen accessible over QMP).
> With such a command, you could insert or remove a copy-on-read filter
> node at any point in time.
> 
> Since we definitely want block graph configuration, I don't think we
> want to add special commands now.

I agree, and it seems that we get more and more use cases for a block
reconfiguration commands. Only yesterday we talked about the "true"
blockdev way for libvirt to implement I/O throttling. The result was
that without reopen, we still need to use the old QMP command to set
BlockBackend-based I/O throttling instead of using a filter node.

So I'm eagerly awaiting Berto's promised patches for it.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH v0 2/2] qmp: add block-set-copy-on-read command

[Alberto Garcia]

On Thu 14 Jun 2018 11:00:55 AM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 13.06.2018 um 18:41 hat Max Reitz geschrieben:
>> On 2018-06-13 18:02, Eric Blake wrote:
>> > On 06/13/2018 10:47 AM, Denis Plotnikov wrote:
>> >> The command enables/disables copy-on-read mode for VM's disk while
>> >> VM is running.
>> >>
>> >> This is needed when using external disk readers to shape access pattern
>> >> to the disk backend.
>> >>
>> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> >> ---
>> > 
>> > Deferring thoughts on the actual design for later;
>> 
>> But why? ;-)
>> 
>> This patch would definitely be superseded by a block reconfiguration
>> command (i.e. presumably one that makes reopen accessible over QMP).
>> With such a command, you could insert or remove a copy-on-read filter
>> node at any point in time.
>> 
>> Since we definitely want block graph configuration, I don't think we
>> want to add special commands now.
>
> I agree, and it seems that we get more and more use cases for a block
> reconfiguration commands. Only yesterday we talked about the "true"
> blockdev way for libvirt to implement I/O throttling. The result was
> that without reopen, we still need to use the old QMP command to set
> BlockBackend-based I/O throttling instead of using a filter node.
>
> So I'm eagerly awaiting Berto's promised patches for it.

They're coming, it's just a bit tricker than I initially though. I'll
probably send the first series as an RFC if I don't manage to fix all
problems, so we can discuss them a bit.

Berto

Re: [Qemu-devel] [PATCH v0 2/2] qmp: add block-set-copy-on-read command

[Kevin Wolf]

Am 14.06.2018 um 11:03 hat Alberto Garcia geschrieben:
> On Thu 14 Jun 2018 11:00:55 AM CEST, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 13.06.2018 um 18:41 hat Max Reitz geschrieben:
> >> On 2018-06-13 18:02, Eric Blake wrote:
> >> > On 06/13/2018 10:47 AM, Denis Plotnikov wrote:
> >> >> The command enables/disables copy-on-read mode for VM's disk while
> >> >> VM is running.
> >> >>
> >> >> This is needed when using external disk readers to shape access pattern
> >> >> to the disk backend.
> >> >>
> >> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> >> >> ---
> >> > 
> >> > Deferring thoughts on the actual design for later;
> >> 
> >> But why? ;-)
> >> 
> >> This patch would definitely be superseded by a block reconfiguration
> >> command (i.e. presumably one that makes reopen accessible over QMP).
> >> With such a command, you could insert or remove a copy-on-read filter
> >> node at any point in time.
> >> 
> >> Since we definitely want block graph configuration, I don't think we
> >> want to add special commands now.
> >
> > I agree, and it seems that we get more and more use cases for a block
> > reconfiguration commands. Only yesterday we talked about the "true"
> > blockdev way for libvirt to implement I/O throttling. The result was
> > that without reopen, we still need to use the old QMP command to set
> > BlockBackend-based I/O throttling instead of using a filter node.
> >
> > So I'm eagerly awaiting Berto's promised patches for it.
> 
> They're coming, it's just a bit tricker than I initially though. I'll
> probably send the first series as an RFC if I don't manage to fix all
> problems, so we can discuss them a bit.

Sounds good to me. It's not completely surprising that there are some
tricky spots there, we've been postponing this for a reason...

Kevin