[v5,1/6] Stream block job involves copy-on-read filter driver

Commit Message

Andrey Shinkevich Dec. 30, 2018, 8:09 p.m. UTC

The copy-on-read filter is applied to block-stream operation.
It is necessary for further block discard option.

Signed-off-by: Andrey Shinkevich <[e-mail]>
---
 block/stream.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 88 insertions(+), 2 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Jan. 8, 2019, 1:45 p.m. UTC | #1

30.12.2018 23:09, Andrey Shinkevich wrote:
> The copy-on-read filter is applied to block-stream operation.
> It is necessary for further block discard option.

If we move to c-o-r filter in stream, I think we should:
1. get rid of COPY_ON_READ flag in stream code (so, this thing to be done in one place)
2. add base parameter to c-o-r filter, to not copy things that are below base.

Current behavior of stream is enabling copy-on-read on bs during stream if base == NULL.
So, if we just add c-o-r filter, we will copy extra blocks. On the other hand, if we
support base parameter in c-o-r filter we will involve guest reads to the stream
process for cases with base != NULL, so stream operation should become more efficient.

So for me the following sequence seems reasonable:

- prepare iotests for graph changes
- add base parameter to c-o-r filter
- use c-o-r in stream and drop COPY_ON_READ flag from stream code

then (may be in separate series) add discard functionality. And looks like again, it should
be property of c-o-r filter, not stream job itself, to involve guest reads to discarding too.

In general, Max, is that correspond to what you mean?

Any suggestions?

Max Reitz Jan. 9, 2019, 1:13 p.m. UTC | #2

On 08.01.19 14:45, Vladimir Sementsov-Ogievskiy wrote:
> 30.12.2018 23:09, Andrey Shinkevich wrote:
>> The copy-on-read filter is applied to block-stream operation.
>> It is necessary for further block discard option.
> 
> If we move to c-o-r filter in stream, I think we should:
> 1. get rid of COPY_ON_READ flag in stream code (so, this thing to be done in one place)
> 2. add base parameter to c-o-r filter, to not copy things that are below base.
> 
> Current behavior of stream is enabling copy-on-read on bs during stream if base == NULL.
> So, if we just add c-o-r filter, we will copy extra blocks. On the other hand, if we
> support base parameter in c-o-r filter we will involve guest reads to the stream
> process for cases with base != NULL, so stream operation should become more efficient.
> 
> So for me the following sequence seems reasonable:
> 
> - prepare iotests for graph changes
> - add base parameter to c-o-r filter
> - use c-o-r in stream and drop COPY_ON_READ flag from stream code
> 
> then (may be in separate series) add discard functionality. And looks like again, it should
> be property of c-o-r filter, not stream job itself, to involve guest reads to discarding too.
> 
> In general, Max, is that correspond to what you mean?

Yep, that sounds good to me.

Max

Alberto Garcia Feb. 8, 2019, 12:31 p.m. UTC | #3

On Sun 30 Dec 2018 09:09:12 PM CET, Andrey Shinkevich wrote:
> +static BlockDriverState *child_file_bs(BlockDriverState *bs)
> +{
> +    return bs->file ? bs->file->bs : NULL;
> +}

In addition to Vladimir's comments, I wonder if it's useful to put this
in block_int.h (together with backing_bs()). It seems that there's a
couple of more places where this can be used.

Berto

Patch

diff --git a/block/stream.c b/block/stream.c
index 7a49ac0..20e768e 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -16,6 +16,7 @@ 
 #include "block/block_int.h"
 #include "block/blockjob_int.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/ratelimit.h"
 #include "sysemu/block-backend.h"
@@ -35,8 +36,14 @@  typedef struct StreamBlockJob {
     BlockdevOnError on_error;
     char *backing_file_str;
     bool bs_read_only;
+    BlockDriverState *cor_filter_bs;
 } StreamBlockJob;
 
+static BlockDriverState *child_file_bs(BlockDriverState *bs)
+{
+    return bs->file ? bs->file->bs : NULL;
+}
+
 static int coroutine_fn stream_populate(BlockBackend *blk,
                                         int64_t offset, uint64_t bytes,
                                         void *buf)
@@ -54,7 +61,7 @@  static int coroutine_fn stream_populate(BlockBackend *blk,
     return blk_co_preadv(blk, offset, qiov.size, &qiov, BDRV_REQ_COPY_ON_READ);
 }
 
-static int stream_prepare(Job *job)
+static int stream_change_backing_file(Job *job)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockJob *bjob = &s->common;
@@ -82,6 +89,43 @@  static int stream_prepare(Job *job)
     return ret;
 }
 
+static void remove_filter(BlockDriverState *cor_filter_bs)
+{
+    BlockDriverState *bs = child_file_bs(cor_filter_bs);
+
+    /* Hold a guest back from writing until we remove the filter */
+    bdrv_drained_begin(bs);
+    bdrv_child_try_set_perm(cor_filter_bs->file, 0, BLK_PERM_ALL,
+                            &error_abort);
+    bdrv_replace_node(cor_filter_bs, bs, &error_abort);
+    bdrv_drained_end(bs);
+
+    bdrv_unref(cor_filter_bs);
+}
+
+static void stream_exit(Job *job)
+{
+    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
+    if (s->cor_filter_bs == NULL) {
+        return;
+    }
+    /* Remove the filter driver from the graph */
+    remove_filter(s->cor_filter_bs);
+    s->cor_filter_bs = NULL;
+}
+
+static int stream_prepare(Job *job)
+{
+    stream_exit(job);
+
+    return stream_change_backing_file(job);
+}
+
+static void stream_abort(Job *job)
+{
+    stream_exit(job);
+}
+
 static void stream_clean(Job *job)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
@@ -102,7 +146,7 @@  static int coroutine_fn stream_run(Job *job, Error **errp)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockBackend *blk = s->common.blk;
-    BlockDriverState *bs = blk_bs(blk);
+    BlockDriverState *bs = child_file_bs(s->cor_filter_bs);
     BlockDriverState *base = s->base;
     int64_t len;
     int64_t offset = 0;
@@ -206,6 +250,42 @@  out:
     return ret;
 }
 
+static BlockDriverState *create_filter_node(BlockDriverState *bs, Error **errp)
+{
+    QDict *opts = qdict_new();
+
+    qdict_put_str(opts, "driver", "copy-on-read");
+    qdict_put_str(opts, "file", bdrv_get_node_name(bs));
+
+    return bdrv_open(NULL, NULL, opts, BDRV_O_RDWR, errp);
+}
+
+static BlockDriverState *insert_filter(BlockDriverState *bs, Error **errp)
+{
+    BlockDriverState *cor_filter_bs;
+    Error *local_err = NULL;
+
+    cor_filter_bs = create_filter_node(bs, errp);
+    if (cor_filter_bs == NULL) {
+        error_prepend(errp, "Could not create filter node: ");
+        return NULL;
+    }
+
+    bdrv_set_aio_context(cor_filter_bs, bdrv_get_aio_context(bs));
+
+    bdrv_drained_begin(bs);
+    bdrv_replace_node(bs, cor_filter_bs, &local_err);
+    bdrv_drained_end(bs);
+
+    if (local_err) {
+        bdrv_unref(cor_filter_bs);
+        error_propagate(errp, local_err);
+        return NULL;
+    }
+
+    return cor_filter_bs;
+}
+
 static const BlockJobDriver stream_job_driver = {
     .job_driver = {
         .instance_size = sizeof(StreamBlockJob),
@@ -213,6 +293,7 @@  static const BlockJobDriver stream_job_driver = {
         .free          = block_job_free,
         .run           = stream_run,
         .prepare       = stream_prepare,
+        .abort         = stream_abort,
         .clean         = stream_clean,
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
@@ -259,6 +340,11 @@  void stream_start(const char *job_id, BlockDriverState *bs,
                            &error_abort);
     }
 
+    s->cor_filter_bs = insert_filter(bs, errp);
+    if (s->cor_filter_bs == NULL) {
+        goto fail;
+    }
+
     s->base = base;
     s->backing_file_str = g_strdup(backing_file_str);
     s->bs_read_only = bs_read_only;