| Message ID | 20180613154711.12977-3-dplotnikov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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
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
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: #
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(+)