* Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE
2018-04-13 14:31 [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE Vladimir Sementsov-Ogievskiy
@ 2018-04-13 14:43 ` Vladimir Sementsov-Ogievskiy
2018-04-18 16:37 ` Eric Blake
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-04-13 14:43 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, den, pbonzini, eblake
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE
2018-04-13 14:31 [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE Vladimir Sementsov-Ogievskiy
2018-04-13 14:43 ` Vladimir Sementsov-Ogievskiy
@ 2018-04-18 16:37 ` Eric Blake
2018-04-19 8:36 ` Vladimir Sementsov-Ogievskiy
2018-05-17 9:52 ` Vladimir Sementsov-Ogievskiy
2018-06-21 14:41 ` Eric Blake
3 siblings, 1 reply; 10+ messages in thread
From: Eric Blake @ 2018-04-18 16:37 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Cc: mreitz, kwolf, den, pbonzini
[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE
2018-04-18 16:37 ` Eric Blake
@ 2018-04-19 8:36 ` Vladimir Sementsov-Ogievskiy
2018-04-19 15:59 ` Eric Blake
0 siblings, 1 reply; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-04-19 8:36 UTC (permalink / raw)
To: Eric Blake, qemu-devel, qemu-block; +Cc: mreitz, kwolf, den, pbonzini
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE
2018-04-19 8:36 ` Vladimir Sementsov-Ogievskiy
@ 2018-04-19 15:59 ` Eric Blake
0 siblings, 0 replies; 10+ messages in thread
From: Eric Blake @ 2018-04-19 15:59 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Cc: mreitz, kwolf, den, pbonzini
[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE
2018-04-13 14:31 [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE Vladimir Sementsov-Ogievskiy
2018-04-13 14:43 ` Vladimir Sementsov-Ogievskiy
2018-04-18 16:37 ` Eric Blake
@ 2018-05-17 9:52 ` Vladimir Sementsov-Ogievskiy
2018-05-17 13:34 ` Eric Blake
2018-06-21 14:41 ` Eric Blake
3 siblings, 1 reply; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-05-17 9:52 UTC (permalink / raw)
To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, den, pbonzini, eblake
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE
2018-05-17 9:52 ` Vladimir Sementsov-Ogievskiy
@ 2018-05-17 13:34 ` Eric Blake
2018-05-29 15:27 ` Vladimir Sementsov-Ogievskiy
2018-06-21 13:13 ` Vladimir Sementsov-Ogievskiy
0 siblings, 2 replies; 10+ messages in thread
From: Eric Blake @ 2018-05-17 13:34 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Cc: mreitz, kwolf, den, pbonzini
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE
2018-05-17 13:34 ` Eric Blake
@ 2018-05-29 15:27 ` Vladimir Sementsov-Ogievskiy
2018-06-21 13:13 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-05-29 15:27 UTC (permalink / raw)
To: Eric Blake, qemu-devel, qemu-block; +Cc: mreitz, kwolf, den, pbonzini
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE
2018-05-17 13:34 ` Eric Blake
2018-05-29 15:27 ` Vladimir Sementsov-Ogievskiy
@ 2018-06-21 13:13 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 10+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-06-21 13:13 UTC (permalink / raw)
To: Eric Blake, qemu-devel, qemu-block; +Cc: mreitz, kwolf, den, pbonzini
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE
2018-04-13 14:31 [Qemu-devel] [PATCH] nbd/server: introduce NBD_CMD_CACHE Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2018-05-17 9:52 ` Vladimir Sementsov-Ogievskiy
@ 2018-06-21 14:41 ` Eric Blake
3 siblings, 0 replies; 10+ messages in thread
From: Eric Blake @ 2018-06-21 14:41 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Cc: mreitz, kwolf, den, pbonzini
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
^ permalink raw reply related [flat|nested] 10+ messages in thread