[Virtio-fs] Few queries about virtiofsd read implementation

[Vivek Goyal]

On Mon, May 10, 2021 at 05:40:05PM -0400, Vivek Goyal wrote:
> On Mon, May 10, 2021 at 06:23:48PM +0100, Dr. David Alan Gilbert wrote:
> > * Vivek Goyal (vgoyal at redhat.com) wrote:
> > > Hi David/Stefan,
> > > 
> > > I am browsing through the code of read requests (FUSE_READ) in virtiofsd
> > > (and in virtiofs) and I have few questions. You folks probably know the
> > > answers.
> > > 
> > > 1. virtio_send_data_iov(), reads the data from file into the scatter list.
> > >   Some of the code looks strange.
> > > 
> > >   We seem to be retrying read if we read less number of bytes than what
> > >   client asked for. I am wondering shoudl this really be our
> > >   responsibility or client should deal with it. I am assuming that client
> > >   should be ready to deal with less number of bytes read.
> > > 
> > >   So what was the thought process behind retrying.
> > > 
> > >           if (ret < len && ret) {
> > >             fuse_log(FUSE_LOG_DEBUG, "%s: ret < len\n", __func__);
> > >             /* Skip over this much next time around */
> > >             skip_size = ret;
> > >             buf->buf[0].pos += ret;
> > >             len -= ret;
> > > 
> > >             /* Lets do another read */
> > >             continue;
> > >         }
> > > 
> > > - After this we have code where if number of bytes read are not same
> > >   as we expect to, then we return EIO.
> > > 
> > >           if (ret != len) {
> > >             fuse_log(FUSE_LOG_DEBUG, "%s: ret!=len\n", __func__);
> > >             ret = EIO;
> > >             free(in_sg_cpy);
> > >             goto err;
> > >         }
> > > 
> > >   When do we hit this. IIUC, preadv() will return.
> > > 
> > >   A. Either number of bytes we expected (no issues)
> > >   B. 0 in case of EOF (We break out of loop and just return to client with
> > >   		      number of bytes we have read so far).
> > >   C. <0 (This is error case and we return error to client)
> > >   D. X bytes which is less than len. 
> > > 
> > > To handle D we have code to retry. So when do we hit the above if
> > > condition where "ret !=len). Is this a dead code. Or I missed something.
> > 
> > I think you're right, that's dead.
> > And oyu're probably also right that we could just take it easy and
> > return less data to the client if preadv just gives us part of it.
> 
> Also, looks like we never return error to client.  virtio_send_data_iov()
> only sends reply back if preadv() reads requested bytes or reads less due
> to EOF. If preadv() returnes error, then we return to caller with error.
> And I don't see anybody propagating that error back to client.
> 
> lo_read()
>   fuse_reply_data()
>     fuse_send_data_iov()
>       fuse_send_data_iov_fallback()
>         virtio_send_data_iov()
> 
> fuse_reply_data() returns error only if ret > 0 and that's not the case
> here. In fact that seems to be another piece of dead code for virtiofs.

Actually I might have misread the code. In case of error
virtio_send_data_iov() returns errno, which probably is positive and
fuse_reply_data() returns error back to client in that case.

    res = fuse_send_data_iov(req->se, req->ch, iov, 1, bufv);
    if (res <= 0) {
        fuse_free_req(req);
        return res;
    } else {
        return fuse_reply_err(req, res);
    }

This is confusing...

Vivek