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

[Eric Blake <eblake@redhat.com>]

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