[Qemu-devel] [PATCH 0/3] block: blk_co_pcache

[Qemu-devel] [PATCH 0/3] block: blk_co_pcache

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is small new io API: blk_co_pcache, which does copy-on-read without
extra buffer for read data. This means that only parts that needs COR
will be actually read and only corresponding buffers allocated, no more.

This allows to improve a bit block-stream and NBD_CMD_CACHE

Vladimir Sementsov-Ogievskiy (3):
  block: implement blk_co_pcache
  block/stream: use blk_co_pcache
  nbd: improve CMD_CACHE: use blk_co_pcache

 include/block/block.h          |  8 ++++++-
 include/sysemu/block-backend.h |  7 ++++++
 block/io.c                     | 18 +++++++++-----
 block/stream.c                 | 19 +++++----------
 nbd/server.c                   | 43 +++++++++++++++++++++++++++-------
 5 files changed, 67 insertions(+), 28 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH 1/3] block: implement blk_co_pcache

[Vladimir Sementsov-Ogievskiy]

Do effective copy-on-read request when we don't need data actually. It
will be used for block-stream and NBD_CMD_CACHE.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block.h          |  8 +++++++-
 include/sysemu/block-backend.h |  7 +++++++
 block/io.c                     | 18 ++++++++++++------
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index f9415ed740..4662f25513 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -88,8 +88,14 @@ typedef enum {
      * fallback. */
     BDRV_REQ_NO_FALLBACK        = 0x100,
 
+    /*
+     * BDRV_REQ_CACHE may be used only together with BDRV_REQ_COPY_ON_READ
+     * on read request and means that caller don't really need data to be
+     * written to qiov parameter which may be NULL.
+     */
+    BDRV_REQ_CACHE  = 0x200,
     /* Mask of valid flags */
-    BDRV_REQ_MASK               = 0x1ff,
+    BDRV_REQ_MASK               = 0x3ff,
 } BdrvRequestFlags;
 
 typedef struct BlockSizes {
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 733c4957eb..2bcecd1190 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -135,6 +135,13 @@ static inline int coroutine_fn blk_co_pread(BlockBackend *blk, int64_t offset,
     return blk_co_preadv(blk, offset, bytes, &qiov, flags);
 }
 
+static inline int coroutine_fn blk_co_pcache(BlockBackend *blk, int64_t offset,
+                                             unsigned int bytes)
+{
+    return blk_co_preadv(blk, offset, bytes, NULL,
+                         BDRV_REQ_COPY_ON_READ | BDRV_REQ_CACHE);
+}
+
 static inline int coroutine_fn blk_co_pwrite(BlockBackend *blk, int64_t offset,
                                              unsigned int bytes, void *buf,
                                              BdrvRequestFlags flags)
diff --git a/block/io.c b/block/io.c
index 9ba1bada36..9daf7332bd 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1104,7 +1104,8 @@ bdrv_driver_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
 }
 
 static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
-        int64_t offset, unsigned int bytes, QEMUIOVector *qiov)
+        int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
+        int flags)
 {
     BlockDriverState *bs = child->bs;
 
@@ -1215,9 +1216,11 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
                 goto err;
             }
 
-            qemu_iovec_from_buf(qiov, progress, bounce_buffer + skip_bytes,
-                                pnum - skip_bytes);
-        } else {
+            if (!(flags & BDRV_REQ_CACHE)) {
+                qemu_iovec_from_buf(qiov, progress, bounce_buffer + skip_bytes,
+                                    pnum - skip_bytes);
+            }
+        } else if (!(flags & BDRV_REQ_CACHE)) {
             /* Read directly into the destination */
             qemu_iovec_init(&local_qiov, qiov->niov);
             qemu_iovec_concat(&local_qiov, qiov, progress, pnum - skip_bytes);
@@ -1268,7 +1271,8 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
      * potential fallback support, if we ever implement any read flags
      * to pass through to drivers.  For now, there aren't any
      * passthrough flags.  */
-    assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ)));
+    assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ |
+                       BDRV_REQ_CACHE)));
 
     /* Handle Copy on Read and associated serialisation */
     if (flags & BDRV_REQ_COPY_ON_READ) {
@@ -1296,7 +1300,9 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
         }
 
         if (!ret || pnum != bytes) {
-            ret = bdrv_co_do_copy_on_readv(child, offset, bytes, qiov);
+            ret = bdrv_co_do_copy_on_readv(child, offset, bytes, qiov, flags);
+            goto out;
+        } else if (flags & BDRV_REQ_CACHE) {
             goto out;
         }
     }
-- 
2.18.0

[Qemu-devel] [PATCH 2/3] block/stream: use blk_co_pcache

[Vladimir Sementsov-Ogievskiy]

This helps to avoid extra io, allocations and memory copying.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/stream.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 1a906fd860..8478aa4a50 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -22,11 +22,11 @@
 
 enum {
     /*
-     * Size of data buffer for populating the image file.  This should be large
+     * Maximum chunk size to feed it to copy-on-read.  This should be large
      * enough to process multiple clusters in a single call, so that populating
      * contiguous regions of the image is efficient.
      */
-    STREAM_BUFFER_SIZE = 512 * 1024, /* in bytes */
+    STREAM_CHUNK = 512 * 1024, /* in bytes */
 };
 
 typedef struct StreamBlockJob {
@@ -39,13 +39,11 @@ typedef struct StreamBlockJob {
 } StreamBlockJob;
 
 static int coroutine_fn stream_populate(BlockBackend *blk,
-                                        int64_t offset, uint64_t bytes,
-                                        void *buf)
+                                        int64_t offset, uint64_t bytes)
 {
     assert(bytes < SIZE_MAX);
 
-    /* Copy-on-read the unallocated clusters */
-    return blk_co_pread(blk, offset, bytes, buf, BDRV_REQ_COPY_ON_READ);
+    return blk_co_pcache(blk, offset, bytes);
 }
 
 static void stream_abort(Job *job)
@@ -117,7 +115,6 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
     int error = 0;
     int ret = 0;
     int64_t n = 0; /* bytes */
-    void *buf;
 
     if (!bs->backing) {
         goto out;
@@ -130,8 +127,6 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
     }
     job_progress_set_remaining(&s->common.job, len);
 
-    buf = qemu_blockalign(bs, STREAM_BUFFER_SIZE);
-
     /* Turn on copy-on-read for the whole block device so that guest read
      * requests help us make progress.  Only do this when copying the entire
      * backing chain since the copy-on-read operation does not take base into
@@ -154,7 +149,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
 
         copy = false;
 
-        ret = bdrv_is_allocated(bs, offset, STREAM_BUFFER_SIZE, &n);
+        ret = bdrv_is_allocated(bs, offset, STREAM_CHUNK, &n);
         if (ret == 1) {
             /* Allocated in the top, no need to copy.  */
         } else if (ret >= 0) {
@@ -172,7 +167,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
         }
         trace_stream_one_iteration(s, offset, n, ret);
         if (copy) {
-            ret = stream_populate(blk, offset, n, buf);
+            ret = stream_populate(blk, offset, n);
         }
         if (ret < 0) {
             BlockErrorAction action =
@@ -206,8 +201,6 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
     /* Do not remove the backing file if an error was there but ignored.  */
     ret = error;
 
-    qemu_vfree(buf);
-
 out:
     /* Modify backing chain and close BDSes in main loop */
     return ret;
-- 
2.18.0

[Qemu-devel] [PATCH 3/3] nbd: improve CMD_CACHE: use blk_co_pcache

[Vladimir Sementsov-Ogievskiy]

This helps to avoid extra io, allocations and memory copying.
We assume here that CMD_CACHE is always used with copy-on-read, as
otherwise it's a noop.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index aeca3893fe..a7e8dbaccd 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2101,12 +2101,15 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
             return -EINVAL;
         }
 
-        req->data = blk_try_blockalign(client->exp->blk, request->len);
-        if (req->data == NULL) {
-            error_setg(errp, "No memory");
-            return -ENOMEM;
+        if (request->type != NBD_CMD_CACHE) {
+            req->data = blk_try_blockalign(client->exp->blk, request->len);
+            if (req->data == NULL) {
+                error_setg(errp, "No memory");
+                return -ENOMEM;
+            }
         }
     }
+
     if (request->type == NBD_CMD_WRITE) {
         if (nbd_read(client->ioc, req->data, request->len, "CMD_WRITE data",
                      errp) < 0)
@@ -2191,7 +2194,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request,
     int ret;
     NBDExport *exp = client->exp;
 
-    assert(request->type == NBD_CMD_READ || request->type == NBD_CMD_CACHE);
+    assert(request->type == NBD_CMD_READ);
 
     /* XXX: NBD Protocol only documents use of FUA with WRITE */
     if (request->flags & NBD_CMD_FLAG_FUA) {
@@ -2203,7 +2206,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request,
     }
 
     if (client->structured_reply && !(request->flags & NBD_CMD_FLAG_DF) &&
-        request->len && request->type != NBD_CMD_CACHE)
+        request->len)
     {
         return nbd_co_send_sparse_read(client, request->handle, request->from,
                                        data, request->len, errp);
@@ -2211,7 +2214,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request,
 
     ret = blk_pread(exp->blk, request->from + exp->dev_offset, data,
                     request->len);
-    if (ret < 0 || request->type == NBD_CMD_CACHE) {
+    if (ret < 0) {
         return nbd_send_generic_reply(client, request->handle, ret,
                                       "reading from file failed", errp);
     }
@@ -2230,6 +2233,28 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request,
     }
 }
 
+/*
+ * nbd_do_cmd_cache
+ *
+ * Handle NBD_CMD_CACHE request.
+ * Return -errno if sending fails. Other errors are reported directly to the
+ * client as an error reply.
+ */
+static coroutine_fn int nbd_do_cmd_cache(NBDClient *client, NBDRequest *request,
+                                         Error **errp)
+{
+    int ret;
+    NBDExport *exp = client->exp;
+
+    assert(request->type == NBD_CMD_CACHE);
+
+    ret = blk_co_pcache(exp->blk, request->from + exp->dev_offset,
+                        request->len);
+
+    return nbd_send_generic_reply(client, request->handle, ret,
+                                  "caching data failed", errp);
+}
+
 /* Handle NBD request.
  * Return -errno if sending fails. Other errors are reported directly to the
  * client as an error reply. */
@@ -2243,8 +2268,10 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
     char *msg;
 
     switch (request->type) {
-    case NBD_CMD_READ:
     case NBD_CMD_CACHE:
+        return nbd_do_cmd_cache(client, request, errp);
+
+    case NBD_CMD_READ:
         return nbd_do_cmd_read(client, request, data, errp);
 
     case NBD_CMD_WRITE:
-- 
2.18.0

Re: [Qemu-devel] [PATCH 0/3] block: blk_co_pcache

[Eric Blake]

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

On 6/6/19 8:48 AM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here is small new io API: blk_co_pcache, which does copy-on-read without
> extra buffer for read data. This means that only parts that needs COR
> will be actually read and only corresponding buffers allocated, no more.
> 
> This allows to improve a bit block-stream and NBD_CMD_CACHE

I'd really like to see qemu-io gain access to calling this command, so
that we can add iotests coverage of this new feature. Note that the
in-development libnbd
(https://github.com/libguestfs/libnbd/commits/master) is also usable as
an NBD client that can drive NBD_CMD_CACHE, although it's still new
enough that we probably don't want to rely on it being available yet.

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


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

Re: [Qemu-devel] [PATCH 0/3] block: blk_co_pcache

[Vladimir Sementsov-Ogievskiy]

06.06.2019 16:55, Eric Blake wrote:
> On 6/6/19 8:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is small new io API: blk_co_pcache, which does copy-on-read without
>> extra buffer for read data. This means that only parts that needs COR
>> will be actually read and only corresponding buffers allocated, no more.
>>
>> This allows to improve a bit block-stream and NBD_CMD_CACHE
> 
> I'd really like to see qemu-io gain access to calling this command, so
> that we can add iotests coverage of this new feature. Note that the
> in-development libnbd
> (https://github.com/libguestfs/libnbd/commits/master) is also usable as
> an NBD client that can drive NBD_CMD_CACHE, although it's still new
> enough that we probably don't want to rely on it being available yet.
> 

Hmm, don't you think that blk_co_pcache sends NBD_CMD_CACHE if called on nbd driver?
I didn't implement it. But may be I should..

May aim was only to avoid extra allocation and unnecessary reads. But if we implement
full-featured io request, what should it do?

On qcow2 with backing it should pull data from backing to top, like in copy-on-read.
And for nbd it will send NBD_CMD_CACHE?
These semantics seems different, but why not?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/3] block: blk_co_pcache

[Vladimir Sementsov-Ogievskiy]

06.06.2019 17:07, Vladimir Sementsov-Ogievskiy wrote:
> 06.06.2019 16:55, Eric Blake wrote:
>> On 6/6/19 8:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here is small new io API: blk_co_pcache, which does copy-on-read without
>>> extra buffer for read data. This means that only parts that needs COR
>>> will be actually read and only corresponding buffers allocated, no more.
>>>
>>> This allows to improve a bit block-stream and NBD_CMD_CACHE
>>
>> I'd really like to see qemu-io gain access to calling this command, so
>> that we can add iotests coverage of this new feature. Note that the
>> in-development libnbd
>> (https://github.com/libguestfs/libnbd/commits/master) is also usable as
>> an NBD client that can drive NBD_CMD_CACHE, although it's still new
>> enough that we probably don't want to rely on it being available yet.
>>
> 
> Hmm, don't you think that blk_co_pcache sends NBD_CMD_CACHE if called on nbd driver?
> I didn't implement it. But may be I should..
> 
> May aim was only to avoid extra allocation and unnecessary reads. But if we implement
> full-featured io request, what should it do?
> 
> On qcow2 with backing it should pull data from backing to top, like in copy-on-read.
> And for nbd it will send NBD_CMD_CACHE?
> These semantics seems different, but why not?
> 

Any opinions here? Should I resend or could we use it as a first step, not touching
external API but improving things a little bit?


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/3] block: blk_co_pcache

[Kevin Wolf]

Am 17.06.2019 um 13:20 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 06.06.2019 17:07, Vladimir Sementsov-Ogievskiy wrote:
> > 06.06.2019 16:55, Eric Blake wrote:
> >> On 6/6/19 8:48 AM, Vladimir Sementsov-Ogievskiy wrote:
> >>> Hi all!
> >>>
> >>> Here is small new io API: blk_co_pcache, which does copy-on-read without
> >>> extra buffer for read data. This means that only parts that needs COR
> >>> will be actually read and only corresponding buffers allocated, no more.
> >>>
> >>> This allows to improve a bit block-stream and NBD_CMD_CACHE
> >>
> >> I'd really like to see qemu-io gain access to calling this command, so
> >> that we can add iotests coverage of this new feature. Note that the
> >> in-development libnbd
> >> (https://github.com/libguestfs/libnbd/commits/master) is also usable as
> >> an NBD client that can drive NBD_CMD_CACHE, although it's still new
> >> enough that we probably don't want to rely on it being available yet.
> >>
> > 
> > Hmm, don't you think that blk_co_pcache sends NBD_CMD_CACHE if called on nbd driver?
> > I didn't implement it. But may be I should..
> > 
> > May aim was only to avoid extra allocation and unnecessary reads. But if we implement
> > full-featured io request, what should it do?
> > 
> > On qcow2 with backing it should pull data from backing to top, like in copy-on-read.
> > And for nbd it will send NBD_CMD_CACHE?
> > These semantics seems different, but why not?
> > 
> 
> Any opinions here? Should I resend or could we use it as a first step,
> not touching external API but improving things a little bit?

I'm not opposed to making only a first step now. The interface seems to
make sense to me; the only thing that I don't really like is the naming
both of the operation (blk_co_pcache) and of the flag (BDRV_REQ_CACHE)
because to me, "cache" doesn't mean "read, but ignore the result".

The operation only results in something being cached if the block graph
is configured to cache things. And indeed, the most importatn use case
for the flag is not populating a cache, but triggering copy-on-read. So
I think calling this operation "cache" is misleading.

Now, I don't have very good ideas for better names either. I guess
BDRV_REQ_NO_DATA could work, though it's not perfect. (Also, not sure if
a blk_co_preadv_no_read wrapper is even needed when you can just call
blk_co_preadv with the right flag.)

I'm open for good naming ideas.

Kevin

Re: [Qemu-devel] [PATCH 0/3] block: blk_co_pcache

[Vladimir Sementsov-Ogievskiy]

17.06.2019 15:09, Kevin Wolf wrote:
> Am 17.06.2019 um 13:20 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 06.06.2019 17:07, Vladimir Sementsov-Ogievskiy wrote:
>>> 06.06.2019 16:55, Eric Blake wrote:
>>>> On 6/6/19 8:48 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Hi all!
>>>>>
>>>>> Here is small new io API: blk_co_pcache, which does copy-on-read without
>>>>> extra buffer for read data. This means that only parts that needs COR
>>>>> will be actually read and only corresponding buffers allocated, no more.
>>>>>
>>>>> This allows to improve a bit block-stream and NBD_CMD_CACHE
>>>>
>>>> I'd really like to see qemu-io gain access to calling this command, so
>>>> that we can add iotests coverage of this new feature. Note that the
>>>> in-development libnbd
>>>> (https://github.com/libguestfs/libnbd/commits/master) is also usable as
>>>> an NBD client that can drive NBD_CMD_CACHE, although it's still new
>>>> enough that we probably don't want to rely on it being available yet.
>>>>
>>>
>>> Hmm, don't you think that blk_co_pcache sends NBD_CMD_CACHE if called on nbd driver?
>>> I didn't implement it. But may be I should..
>>>
>>> May aim was only to avoid extra allocation and unnecessary reads. But if we implement
>>> full-featured io request, what should it do?
>>>
>>> On qcow2 with backing it should pull data from backing to top, like in copy-on-read.
>>> And for nbd it will send NBD_CMD_CACHE?
>>> These semantics seems different, but why not?
>>>
>>
>> Any opinions here? Should I resend or could we use it as a first step,
>> not touching external API but improving things a little bit?
> 
> I'm not opposed to making only a first step now. The interface seems to
> make sense to me; the only thing that I don't really like is the naming
> both of the operation (blk_co_pcache) and of the flag (BDRV_REQ_CACHE)
> because to me, "cache" doesn't mean "read, but ignore the result".
> 
> The operation only results in something being cached if the block graph
> is configured to cache things. And indeed, the most importatn use case
> for the flag is not populating a cache, but triggering copy-on-read. So
> I think calling this operation "cache" is misleading.
> 
> Now, I don't have very good ideas for better names either. I guess
> BDRV_REQ_NO_DATA could work, though it's not perfect. (Also, not sure if
> a blk_co_preadv_no_read wrapper is even needed when you can just call
> blk_co_preadv with the right flag.)
> 
> I'm open for good naming ideas.
> 

My first try (not published) was BDRV_REQ_FAKE_READ, passed as flag to blk_co_preadv,
without separate io request function.

I decided to make it to be Cache request inspired by NBD_CMD_CACHE, which was created
to do exactly copy-on-read operation. So if we call it cache it will correspond to
NBD protocol.

_NO_DATA also works for me, not a problem to resend with this flag and without additional
wrapper, as a first step.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/3] block: blk_co_pcache

[Eric Blake]

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

On 6/17/19 7:09 AM, Kevin Wolf wrote:

>>>
>>> Hmm, don't you think that blk_co_pcache sends NBD_CMD_CACHE if called on nbd driver?
>>> I didn't implement it. But may be I should..
>>>
>>> May aim was only to avoid extra allocation and unnecessary reads. But if we implement
>>> full-featured io request, what should it do?
>>>
>>> On qcow2 with backing it should pull data from backing to top, like in copy-on-read.
>>> And for nbd it will send NBD_CMD_CACHE?
>>> These semantics seems different, but why not?
>>>
>>
>> Any opinions here? Should I resend or could we use it as a first step,
>> not touching external API but improving things a little bit?
> 
> I'm not opposed to making only a first step now. The interface seems to
> make sense to me; the only thing that I don't really like is the naming
> both of the operation (blk_co_pcache) and of the flag (BDRV_REQ_CACHE)
> because to me, "cache" doesn't mean "read, but ignore the result".
> 
> The operation only results in something being cached if the block graph
> is configured to cache things. And indeed, the most importatn use case
> for the flag is not populating a cache, but triggering copy-on-read. So
> I think calling this operation "cache" is misleading.
> 
> Now, I don't have very good ideas for better names either. I guess
> BDRV_REQ_NO_DATA could work, though it's not perfect. (Also, not sure if
> a blk_co_preadv_no_read wrapper is even needed when you can just call
> blk_co_preadv with the right flag.)
> 
> I'm open for good naming ideas.

Would 'prefetch' be a more useful name? The name NBD_CMD_CACHE was
chosen in the NBD project, but we are not stuck to that name if we think
something better makes more sense.

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


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

Re: [Qemu-devel] [PATCH 0/3] block: blk_co_pcache

[Kevin Wolf]

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

Am 17.06.2019 um 15:09 hat Eric Blake geschrieben:
> On 6/17/19 7:09 AM, Kevin Wolf wrote:
> 
> >>>
> >>> Hmm, don't you think that blk_co_pcache sends NBD_CMD_CACHE if called on nbd driver?
> >>> I didn't implement it. But may be I should..
> >>>
> >>> May aim was only to avoid extra allocation and unnecessary reads. But if we implement
> >>> full-featured io request, what should it do?
> >>>
> >>> On qcow2 with backing it should pull data from backing to top, like in copy-on-read.
> >>> And for nbd it will send NBD_CMD_CACHE?
> >>> These semantics seems different, but why not?
> >>>
> >>
> >> Any opinions here? Should I resend or could we use it as a first step,
> >> not touching external API but improving things a little bit?
> > 
> > I'm not opposed to making only a first step now. The interface seems to
> > make sense to me; the only thing that I don't really like is the naming
> > both of the operation (blk_co_pcache) and of the flag (BDRV_REQ_CACHE)
> > because to me, "cache" doesn't mean "read, but ignore the result".
> > 
> > The operation only results in something being cached if the block graph
> > is configured to cache things. And indeed, the most importatn use case
> > for the flag is not populating a cache, but triggering copy-on-read. So
> > I think calling this operation "cache" is misleading.
> > 
> > Now, I don't have very good ideas for better names either. I guess
> > BDRV_REQ_NO_DATA could work, though it's not perfect. (Also, not sure if
> > a blk_co_preadv_no_read wrapper is even needed when you can just call
> > blk_co_preadv with the right flag.)
> > 
> > I'm open for good naming ideas.
> 
> Would 'prefetch' be a more useful name? The name NBD_CMD_CACHE was
> chosen in the NBD project, but we are not stuck to that name if we think
> something better makes more sense.

Whether 'prefetch' is entirely accurate really depends on the graph
configuration, too. But at least it gives me the right idea of "read
something, but don't return it yet", so yes, I think that would work for
me.

Kevin

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

Re: [Qemu-devel] [PATCH 0/3] block: blk_co_pcache

[Vladimir Sementsov-Ogievskiy]

17.06.2019 16:20, Kevin Wolf wrote:
> Am 17.06.2019 um 15:09 hat Eric Blake geschrieben:
>> On 6/17/19 7:09 AM, Kevin Wolf wrote:
>>
>>>>>
>>>>> Hmm, don't you think that blk_co_pcache sends NBD_CMD_CACHE if called on nbd driver?
>>>>> I didn't implement it. But may be I should..
>>>>>
>>>>> May aim was only to avoid extra allocation and unnecessary reads. But if we implement
>>>>> full-featured io request, what should it do?
>>>>>
>>>>> On qcow2 with backing it should pull data from backing to top, like in copy-on-read.
>>>>> And for nbd it will send NBD_CMD_CACHE?
>>>>> These semantics seems different, but why not?
>>>>>
>>>>
>>>> Any opinions here? Should I resend or could we use it as a first step,
>>>> not touching external API but improving things a little bit?
>>>
>>> I'm not opposed to making only a first step now. The interface seems to
>>> make sense to me; the only thing that I don't really like is the naming
>>> both of the operation (blk_co_pcache) and of the flag (BDRV_REQ_CACHE)
>>> because to me, "cache" doesn't mean "read, but ignore the result".
>>>
>>> The operation only results in something being cached if the block graph
>>> is configured to cache things. And indeed, the most importatn use case
>>> for the flag is not populating a cache, but triggering copy-on-read. So
>>> I think calling this operation "cache" is misleading.
>>>
>>> Now, I don't have very good ideas for better names either. I guess
>>> BDRV_REQ_NO_DATA could work, though it's not perfect. (Also, not sure if
>>> a blk_co_preadv_no_read wrapper is even needed when you can just call
>>> blk_co_preadv with the right flag.)
>>>
>>> I'm open for good naming ideas.
>>
>> Would 'prefetch' be a more useful name? The name NBD_CMD_CACHE was
>> chosen in the NBD project, but we are not stuck to that name if we think
>> something better makes more sense.
> 
> Whether 'prefetch' is entirely accurate really depends on the graph
> configuration, too. But at least it gives me the right idea of "read
> something, but don't return it yet", so yes, I think that would work for
> me.
> 

Do you mean BDRV_REQ_PREFETCH + blk_co_pprefetch, or only the flag? Hmm, doubled 'p'
in blk_co_pprefetch looks strange, bit it should be here for consistency with other
requests..


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/3] block: blk_co_pcache

[Kevin Wolf]

Am 18.06.2019 um 09:38 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 17.06.2019 16:20, Kevin Wolf wrote:
> > Am 17.06.2019 um 15:09 hat Eric Blake geschrieben:
> >> On 6/17/19 7:09 AM, Kevin Wolf wrote:
> >>
> >>>>>
> >>>>> Hmm, don't you think that blk_co_pcache sends NBD_CMD_CACHE if called on nbd driver?
> >>>>> I didn't implement it. But may be I should..
> >>>>>
> >>>>> May aim was only to avoid extra allocation and unnecessary reads. But if we implement
> >>>>> full-featured io request, what should it do?
> >>>>>
> >>>>> On qcow2 with backing it should pull data from backing to top, like in copy-on-read.
> >>>>> And for nbd it will send NBD_CMD_CACHE?
> >>>>> These semantics seems different, but why not?
> >>>>>
> >>>>
> >>>> Any opinions here? Should I resend or could we use it as a first step,
> >>>> not touching external API but improving things a little bit?
> >>>
> >>> I'm not opposed to making only a first step now. The interface seems to
> >>> make sense to me; the only thing that I don't really like is the naming
> >>> both of the operation (blk_co_pcache) and of the flag (BDRV_REQ_CACHE)
> >>> because to me, "cache" doesn't mean "read, but ignore the result".
> >>>
> >>> The operation only results in something being cached if the block graph
> >>> is configured to cache things. And indeed, the most importatn use case
> >>> for the flag is not populating a cache, but triggering copy-on-read. So
> >>> I think calling this operation "cache" is misleading.
> >>>
> >>> Now, I don't have very good ideas for better names either. I guess
> >>> BDRV_REQ_NO_DATA could work, though it's not perfect. (Also, not sure if
> >>> a blk_co_preadv_no_read wrapper is even needed when you can just call
> >>> blk_co_preadv with the right flag.)
> >>>
> >>> I'm open for good naming ideas.
> >>
> >> Would 'prefetch' be a more useful name? The name NBD_CMD_CACHE was
> >> chosen in the NBD project, but we are not stuck to that name if we think
> >> something better makes more sense.
> > 
> > Whether 'prefetch' is entirely accurate really depends on the graph
> > configuration, too. But at least it gives me the right idea of "read
> > something, but don't return it yet", so yes, I think that would work for
> > me.
> 
> Do you mean BDRV_REQ_PREFETCH + blk_co_pprefetch, or only the flag?
> Hmm, doubled 'p' in blk_co_pprefetch looks strange, bit it should be
> here for consistency with other requests..

I think I would only do the flag because the wrapper is so trivial, but
this is a matter of taste. The kind of thing that is decided by whoever
writes the patch.

Kevin