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

[Vladimir Sementsov-Ogievskiy]

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