[Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE

Vladimir Sementsov-Ogievskiy posted 1 patch 23 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180413143156.11409-1-vsementsov@virtuozzo.com
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test s390x passed
include/block/nbd.h |  3 ++-
nbd/server.c        | 10 ++++++----
2 files changed, 8 insertions(+), 5 deletions(-)

[Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE

Posted by Vladimir Sementsov-Ogievskiy 23 weeks ago
Handle nbd CACHE command. Just do read, without sending read data back.
Cache mechanism should be done by exported node driver chain.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/nbd.h |  3 ++-
 nbd/server.c        | 10 ++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index fcdcd54502..b4793d0a29 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -135,6 +135,7 @@ typedef struct NBDExtent {
 #define NBD_FLAG_SEND_TRIM         (1 << 5) /* Send TRIM (discard) */
 #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6) /* Send WRITE_ZEROES */
 #define NBD_FLAG_SEND_DF           (1 << 7) /* Send DF (Do not Fragment) */
+#define NBD_FLAG_SEND_CACHE        (1 << 8) /* Send CACHE (prefetch) */
 
 /* New-style handshake (global) flags, sent from server to client, and
    control what will happen during handshake phase. */
@@ -195,7 +196,7 @@ enum {
     NBD_CMD_DISC = 2,
     NBD_CMD_FLUSH = 3,
     NBD_CMD_TRIM = 4,
-    /* 5 reserved for failed experiment NBD_CMD_CACHE */
+    NBD_CMD_CACHE = 5,
     NBD_CMD_WRITE_ZEROES = 6,
     NBD_CMD_BLOCK_STATUS = 7,
 };
diff --git a/nbd/server.c b/nbd/server.c
index 9e1f227178..30d7d3f444 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1134,7 +1134,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
     int ret;
     const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
                               NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
-                              NBD_FLAG_SEND_WRITE_ZEROES);
+                              NBD_FLAG_SEND_WRITE_ZEROES | NBD_FLAG_SEND_CACHE);
     bool oldStyle;
 
     /* Old style negotiation header, no room for options
@@ -1826,7 +1826,9 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
         return -EIO;
     }
 
-    if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE) {
+    if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE ||
+        request->type == NBD_CMD_CACHE)
+    {
         if (request->len > NBD_MAX_BUFFER_SIZE) {
             error_setg(errp, "len (%" PRIu32" ) is larger than max len (%u)",
                        request->len, NBD_MAX_BUFFER_SIZE);
@@ -1911,7 +1913,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);
+    assert(request->type == NBD_CMD_READ || request->type == NBD_CMD_CACHE);
 
     /* XXX: NBD Protocol only documents use of FUA with WRITE */
     if (request->flags & NBD_CMD_FLAG_FUA) {
@@ -1930,7 +1932,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) {
+    if (ret < 0 || request->type == NBD_CMD_CACHE) {
         return nbd_send_generic_reply(client, request->handle, ret,
                                       "reading from file failed", errp);
     }
-- 
2.11.1


Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE

Posted by Vladimir Sementsov-Ogievskiy 23 weeks ago
13.04.2018 17:31, Vladimir Sementsov-Ogievskiy wrote:
> Handle nbd CACHE command. Just do read, without sending read data back.
> Cache mechanism should be done by exported node driver chain.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/nbd.h |  3 ++-
>   nbd/server.c        | 10 ++++++----
>   2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index fcdcd54502..b4793d0a29 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -135,6 +135,7 @@ typedef struct NBDExtent {
>   #define NBD_FLAG_SEND_TRIM         (1 << 5) /* Send TRIM (discard) */
>   #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6) /* Send WRITE_ZEROES */
>   #define NBD_FLAG_SEND_DF           (1 << 7) /* Send DF (Do not Fragment) */
> +#define NBD_FLAG_SEND_CACHE        (1 << 8) /* Send CACHE (prefetch) */
>   
>   /* New-style handshake (global) flags, sent from server to client, and
>      control what will happen during handshake phase. */
> @@ -195,7 +196,7 @@ enum {
>       NBD_CMD_DISC = 2,
>       NBD_CMD_FLUSH = 3,
>       NBD_CMD_TRIM = 4,
> -    /* 5 reserved for failed experiment NBD_CMD_CACHE */
> +    NBD_CMD_CACHE = 5,
>       NBD_CMD_WRITE_ZEROES = 6,
>       NBD_CMD_BLOCK_STATUS = 7,
>   };
> diff --git a/nbd/server.c b/nbd/server.c
> index 9e1f227178..30d7d3f444 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1134,7 +1134,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
>       int ret;
>       const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
>                                 NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
> -                              NBD_FLAG_SEND_WRITE_ZEROES);
> +                              NBD_FLAG_SEND_WRITE_ZEROES | NBD_FLAG_SEND_CACHE);
>       bool oldStyle;
>   
>       /* Old style negotiation header, no room for options
> @@ -1826,7 +1826,9 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
>           return -EIO;
>       }
>   
> -    if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE) {
> +    if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE ||
> +        request->type == NBD_CMD_CACHE)
> +    {
>           if (request->len > NBD_MAX_BUFFER_SIZE) {
>               error_setg(errp, "len (%" PRIu32" ) is larger than max len (%u)",
>                          request->len, NBD_MAX_BUFFER_SIZE);
> @@ -1911,7 +1913,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);
> +    assert(request->type == NBD_CMD_READ || request->type == NBD_CMD_CACHE);
>   
>       /* XXX: NBD Protocol only documents use of FUA with WRITE */
>       if (request->flags & NBD_CMD_FLAG_FUA) {
> @@ -1930,7 +1932,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) {
> +    if (ret < 0 || request->type == NBD_CMD_CACHE) {
>           return nbd_send_generic_reply(client, request->handle, ret,
>                                         "reading from file failed", errp);
>       }

ohh, forget the main thing:

diff --git a/nbd/server.c b/nbd/server.c
index 30d7d3f444..d6a161c8d5 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1965,6 +1965,7 @@ static coroutine_fn int 
nbd_handle_request(NBDClient *client,

      switch (request->type) {
      case NBD_CMD_READ:
+    case NBD_CMD_CACHE:
          return nbd_do_cmd_read(client, request, data, errp);

      case NBD_CMD_WRITE:



-- 
Best regards,
Vladimir


Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE

Posted by Eric Blake 22 weeks ago
On 04/13/2018 09:31 AM, Vladimir Sementsov-Ogievskiy wrote:
> Handle nbd CACHE command. Just do read, without sending read data back.
> Cache mechanism should be done by exported node driver chain.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/nbd.h |  3 ++-
>  nbd/server.c        | 10 ++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index fcdcd54502..b4793d0a29 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -135,6 +135,7 @@ typedef struct NBDExtent {
>  #define NBD_FLAG_SEND_TRIM         (1 << 5) /* Send TRIM (discard) */
>  #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6) /* Send WRITE_ZEROES */
>  #define NBD_FLAG_SEND_DF           (1 << 7) /* Send DF (Do not Fragment) */
> +#define NBD_FLAG_SEND_CACHE        (1 << 8) /* Send CACHE (prefetch) */

Hmm, this flag is not documented in the upstream NBD protocol yet; are
we sure it matches the xNBD implementation?  We'll want at least a
documentation patch proposed for the NBD list before taking this.

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

Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE

Posted by Vladimir Sementsov-Ogievskiy 22 weeks ago
18.04.2018 19:37, Eric Blake wrote:
> On 04/13/2018 09:31 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Handle nbd CACHE command. Just do read, without sending read data back.
>> Cache mechanism should be done by exported node driver chain.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/nbd.h |  3 ++-
>>   nbd/server.c        | 10 ++++++----
>>   2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/block/nbd.h b/include/block/nbd.h
>> index fcdcd54502..b4793d0a29 100644
>> --- a/include/block/nbd.h
>> +++ b/include/block/nbd.h
>> @@ -135,6 +135,7 @@ typedef struct NBDExtent {
>>   #define NBD_FLAG_SEND_TRIM         (1 << 5) /* Send TRIM (discard) */
>>   #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6) /* Send WRITE_ZEROES */
>>   #define NBD_FLAG_SEND_DF           (1 << 7) /* Send DF (Do not Fragment) */
>> +#define NBD_FLAG_SEND_CACHE        (1 << 8) /* Send CACHE (prefetch) */
> Hmm, this flag is not documented in the upstream NBD protocol yet; are
> we sure it matches the xNBD implementation?  We'll want at least a
> documentation patch proposed for the NBD list before taking this.
>

The discussion already on list:
https://lists.debian.org/nbd/2018/03/msg00040.html

-- 
Best regards,
Vladimir


Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE

Posted by Eric Blake 22 weeks ago
On 04/19/2018 03:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> 18.04.2018 19:37, Eric Blake wrote:
>> On 04/13/2018 09:31 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Handle nbd CACHE command. Just do read, without sending read data back.
>>> Cache mechanism should be done by exported node driver chain.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   include/block/nbd.h |  3 ++-
>>>   nbd/server.c        | 10 ++++++----
>>>   2 files changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/block/nbd.h b/include/block/nbd.h
>>> index fcdcd54502..b4793d0a29 100644
>>> --- a/include/block/nbd.h
>>> +++ b/include/block/nbd.h
>>> @@ -135,6 +135,7 @@ typedef struct NBDExtent {
>>>   #define NBD_FLAG_SEND_TRIM         (1 << 5) /* Send TRIM (discard) */
>>>   #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6) /* Send WRITE_ZEROES */
>>>   #define NBD_FLAG_SEND_DF           (1 << 7) /* Send DF (Do not
>>> Fragment) */
>>> +#define NBD_FLAG_SEND_CACHE        (1 << 8) /* Send CACHE (prefetch) */
>> Hmm, this flag is not documented in the upstream NBD protocol yet; are
>> we sure it matches the xNBD implementation?  We'll want at least a
>> documentation patch proposed for the NBD list before taking this.
>>
> 
> The discussion already on list:
> https://lists.debian.org/nbd/2018/03/msg00040.html

Indeed, and that made me realize that we may want to add NBD_CMD_FLAG_*
values corresponding to other useful posix_fadvise() modes before
finalizing the NBD protocol addition of this feature, in which case we'd
want to make sure the qemu implementation is in line with that.

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

Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE

Posted by Vladimir Sementsov-Ogievskiy 18 weeks ago
Finally, what about this?

13.04.2018 17:31, Vladimir Sementsov-Ogievskiy wrote:
> Handle nbd CACHE command. Just do read, without sending read data back.
> Cache mechanism should be done by exported node driver chain.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/nbd.h |  3 ++-
>   nbd/server.c        | 10 ++++++----
>   2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index fcdcd54502..b4793d0a29 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -135,6 +135,7 @@ typedef struct NBDExtent {
>   #define NBD_FLAG_SEND_TRIM         (1 << 5) /* Send TRIM (discard) */
>   #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6) /* Send WRITE_ZEROES */
>   #define NBD_FLAG_SEND_DF           (1 << 7) /* Send DF (Do not Fragment) */
> +#define NBD_FLAG_SEND_CACHE        (1 << 8) /* Send CACHE (prefetch) */
>   
>   /* New-style handshake (global) flags, sent from server to client, and
>      control what will happen during handshake phase. */
> @@ -195,7 +196,7 @@ enum {
>       NBD_CMD_DISC = 2,
>       NBD_CMD_FLUSH = 3,
>       NBD_CMD_TRIM = 4,
> -    /* 5 reserved for failed experiment NBD_CMD_CACHE */
> +    NBD_CMD_CACHE = 5,
>       NBD_CMD_WRITE_ZEROES = 6,
>       NBD_CMD_BLOCK_STATUS = 7,
>   };
> diff --git a/nbd/server.c b/nbd/server.c
> index 9e1f227178..30d7d3f444 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1134,7 +1134,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
>       int ret;
>       const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
>                                 NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
> -                              NBD_FLAG_SEND_WRITE_ZEROES);
> +                              NBD_FLAG_SEND_WRITE_ZEROES | NBD_FLAG_SEND_CACHE);
>       bool oldStyle;
>   
>       /* Old style negotiation header, no room for options
> @@ -1826,7 +1826,9 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
>           return -EIO;
>       }
>   
> -    if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE) {
> +    if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE ||
> +        request->type == NBD_CMD_CACHE)
> +    {
>           if (request->len > NBD_MAX_BUFFER_SIZE) {
>               error_setg(errp, "len (%" PRIu32" ) is larger than max len (%u)",
>                          request->len, NBD_MAX_BUFFER_SIZE);
> @@ -1911,7 +1913,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);
> +    assert(request->type == NBD_CMD_READ || request->type == NBD_CMD_CACHE);
>   
>       /* XXX: NBD Protocol only documents use of FUA with WRITE */
>       if (request->flags & NBD_CMD_FLAG_FUA) {
> @@ -1930,7 +1932,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) {
> +    if (ret < 0 || request->type == NBD_CMD_CACHE) {
>           return nbd_send_generic_reply(client, request->handle, ret,
>                                         "reading from file failed", errp);
>       }


-- 
Best regards,
Vladimir


Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE

Posted by Eric Blake 18 weeks ago
On 05/17/2018 04:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> Finally, what about this?
> 
> 13.04.2018 17:31, Vladimir Sementsov-Ogievskiy wrote:
>> Handle nbd CACHE command. Just do read, without sending read data back.
>> Cache mechanism should be done by exported node driver chain.

Still waiting on the NBD spec review, which I've pinged on the NBD list. 
But as mentioned there, I'll probably go ahead and accept this (possibly 
with slight tweaks) on Monday, after giving one more weekend for any 
last-minute review comments.

>> @@ -1826,7 +1826,9 @@ static int nbd_co_receive_request(NBDRequestData 
>> *req, NBDRequest *request,
>>           return -EIO;
>>       }
>> -    if (request->type == NBD_CMD_READ || request->type == 
>> NBD_CMD_WRITE) {
>> +    if (request->type == NBD_CMD_READ || request->type == 
>> NBD_CMD_WRITE ||
>> +        request->type == NBD_CMD_CACHE)
>> +    {
>>           if (request->len > NBD_MAX_BUFFER_SIZE) {
>>               error_setg(errp, "len (%" PRIu32" ) is larger than max 
>> len (%u)",

I'm not sure I agree with this one. Since we aren't passing the cached 
data over the wire, we can reject the command with EINVAL instead of 
killing the connection entirely.

(As it is, I wonder if we can be nicer about rejecting a read request > 
32M. For a write request, we have to disconnect; but for a read request, 
we can keep the connection alive by just returning EINVAL for a 
too-large read, instead of our current behavior of disconnecting)

>>                          request->len, NBD_MAX_BUFFER_SIZE);
>> @@ -1911,7 +1913,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);
>> +    assert(request->type == NBD_CMD_READ || request->type == 
>> NBD_CMD_CACHE);
>>       /* XXX: NBD Protocol only documents use of FUA with WRITE */
>>       if (request->flags & NBD_CMD_FLAG_FUA) {
>> @@ -1930,7 +1932,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) {
>> +    if (ret < 0 || request->type == NBD_CMD_CACHE) {
>>           return nbd_send_generic_reply(client, request->handle, ret,
>>                                         "reading from file failed", 
>> errp);

As for the implementation on the server side, yes, this looks 
reasonable, given the proposed spec wording being considered on the NBD 
list.

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

Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE

Posted by Vladimir Sementsov-Ogievskiy 17 weeks ago
17.05.2018 16:34, Eric Blake wrote:
> On 05/17/2018 04:52 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Finally, what about this?
>>
>> 13.04.2018 17:31, Vladimir Sementsov-Ogievskiy wrote:
>>> Handle nbd CACHE command. Just do read, without sending read data back.
>>> Cache mechanism should be done by exported node driver chain.
>
> Still waiting on the NBD spec review, which I've pinged on the NBD 
> list. But as mentioned there, I'll probably go ahead and accept this 
> (possibly with slight tweaks) on Monday, after giving one more weekend 
> for any last-minute review comments.
>
>>> @@ -1826,7 +1826,9 @@ static int 
>>> nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
>>>           return -EIO;
>>>       }
>>> -    if (request->type == NBD_CMD_READ || request->type == 
>>> NBD_CMD_WRITE) {
>>> +    if (request->type == NBD_CMD_READ || request->type == 
>>> NBD_CMD_WRITE ||
>>> +        request->type == NBD_CMD_CACHE)
>>> +    {
>>>           if (request->len > NBD_MAX_BUFFER_SIZE) {
>>>               error_setg(errp, "len (%" PRIu32" ) is larger than max 
>>> len (%u)",
>
> I'm not sure I agree with this one. Since we aren't passing the cached 
> data over the wire, we can reject the command with EINVAL instead of 
> killing the connection entirely.

this chunk do entirely this: return EINVAL :)


>
> (As it is, I wonder if we can be nicer about rejecting a read request 
> > 32M. For a write request, we have to disconnect; but for a read 
> request, we can keep the connection alive by just returning EINVAL for 
> a too-large read, instead of our current behavior of disconnecting)
>
>>> request->len, NBD_MAX_BUFFER_SIZE);
>>> @@ -1911,7 +1913,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);
>>> +    assert(request->type == NBD_CMD_READ || request->type == 
>>> NBD_CMD_CACHE);
>>>       /* XXX: NBD Protocol only documents use of FUA with WRITE */
>>>       if (request->flags & NBD_CMD_FLAG_FUA) {
>>> @@ -1930,7 +1932,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) {
>>> +    if (ret < 0 || request->type == NBD_CMD_CACHE) {
>>>           return nbd_send_generic_reply(client, request->handle, ret,
>>>                                         "reading from file failed", 
>>> errp);
>
> As for the implementation on the server side, yes, this looks 
> reasonable, given the proposed spec wording being considered on the 
> NBD list.
>


-- 
Best regards,
Vladimir


Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE

Posted by Vladimir Sementsov-Ogievskiy 13 weeks ago
17.05.2018 16:34, Eric Blake wrote:
> On 05/17/2018 04:52 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Finally, what about this?
>>
>> 13.04.2018 17:31, Vladimir Sementsov-Ogievskiy wrote:
>>> Handle nbd CACHE command. Just do read, without sending read data back.
>>> Cache mechanism should be done by exported node driver chain.
>
> Still waiting on the NBD spec review, which I've pinged on the NBD 
> list. But as mentioned there, I'll probably go ahead and accept this 
> (possibly with slight tweaks) on Monday, after giving one more weekend 
> for any last-minute review comments.

Hmm, Eric?

>
>>> @@ -1826,7 +1826,9 @@ static int 
>>> nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
>>>           return -EIO;
>>>       }
>>> -    if (request->type == NBD_CMD_READ || request->type == 
>>> NBD_CMD_WRITE) {
>>> +    if (request->type == NBD_CMD_READ || request->type == 
>>> NBD_CMD_WRITE ||
>>> +        request->type == NBD_CMD_CACHE)
>>> +    {
>>>           if (request->len > NBD_MAX_BUFFER_SIZE) {
>>>               error_setg(errp, "len (%" PRIu32" ) is larger than max 
>>> len (%u)",
>
> I'm not sure I agree with this one. Since we aren't passing the cached 
> data over the wire, we can reject the command with EINVAL instead of 
> killing the connection entirely.
>
> (As it is, I wonder if we can be nicer about rejecting a read request 
> > 32M. For a write request, we have to disconnect; but for a read 
> request, we can keep the connection alive by just returning EINVAL for 
> a too-large read, instead of our current behavior of disconnecting)
>
>>> request->len, NBD_MAX_BUFFER_SIZE);
>>> @@ -1911,7 +1913,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);
>>> +    assert(request->type == NBD_CMD_READ || request->type == 
>>> NBD_CMD_CACHE);
>>>       /* XXX: NBD Protocol only documents use of FUA with WRITE */
>>>       if (request->flags & NBD_CMD_FLAG_FUA) {
>>> @@ -1930,7 +1932,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) {
>>> +    if (ret < 0 || request->type == NBD_CMD_CACHE) {
>>>           return nbd_send_generic_reply(client, request->handle, ret,
>>>                                         "reading from file failed", 
>>> errp);
>
> As for the implementation on the server side, yes, this looks 
> reasonable, given the proposed spec wording being considered on the 
> NBD list.
>


-- 
Best regards,
Vladimir


Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE

Posted by Eric Blake 13 weeks ago
On 04/13/2018 09:31 AM, Vladimir Sementsov-Ogievskiy wrote:
> Handle nbd CACHE command. Just do read, without sending read data back.
> Cache mechanism should be done by exported node driver chain.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/nbd.h |  3 ++-
>   nbd/server.c        | 10 ++++++----
>   2 files changed, 8 insertions(+), 5 deletions(-)

Missing a change to nbd/common.c nbd_cmd_lookup().

> @@ -1911,7 +1913,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);
> +    assert(request->type == NBD_CMD_READ || request->type == NBD_CMD_CACHE);

Also, missing a call into this function for NBD_CMD_CACHE.

Squashing the following, then adding to my NBD queue, with:

Reviewed-by: Eric Blake <eblake@redhat.com>

diff --git i/nbd/common.c w/nbd/common.c
index 8c95c1d606e..41f5ed8d9fa 100644
--- i/nbd/common.c
+++ w/nbd/common.c
@@ -148,6 +148,8 @@ const char *nbd_cmd_lookup(uint16_t cmd)
          return "flush";
      case NBD_CMD_TRIM:
          return "trim";
+    case NBD_CMD_CACHE:
+        return "cache";
      case NBD_CMD_WRITE_ZEROES:
          return "write zeroes";
      case NBD_CMD_BLOCK_STATUS:
diff --git i/nbd/server.c w/nbd/server.c
index 323c6d84004..274604609f4 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -2173,6 +2173,7 @@ static coroutine_fn int 
nbd_handle_request(NBDClient *client,

      switch (request->type) {
      case NBD_CMD_READ:
+    case NBD_CMD_CACHE:
          return nbd_do_cmd_read(client, request, data, errp);

      case NBD_CMD_WRITE:


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