[Virtio-fs] [PATCH v4 2/2] virtiofsd: use fuse_buf_writev to replace fuse_buf_write for better performance

[Stefan Hajnoczi]

On Fri, Aug 16, 2019 at 08:48:59AM +0800, piaojun wrote:
> On 2019/8/15 22:50, Stefan Hajnoczi wrote:
> > On Thu, Aug 15, 2019 at 09:11:49AM +0800, piaojun wrote:
> >> Hi Stefan,
> >>
> >> On 2019/8/14 20:52, Stefan Hajnoczi wrote:
> >>> On Fri, Aug 09, 2019 at 08:53:20AM +0800, piaojun wrote:
> >>>> diff --git a/contrib/virtiofsd/buffer.c b/contrib/virtiofsd/buffer.c
> >>>> index cec762f..d127c45 100644
> >>>> --- a/contrib/virtiofsd/buffer.c
> >>>> +++ b/contrib/virtiofsd/buffer.c
> >>>> @@ -318,11 +318,22 @@ static int fuse_bufvec_advance(struct fuse_bufvec *bufv, size_t len)
> >>>>  ssize_t fuse_buf_copy(fuse_req_t req, struct fuse_bufvec *dstv, struct fuse_bufvec *srcv,
> >>>>  		      enum fuse_buf_copy_flags flags)
> >>>>  {
> >>>> +	size_t i;
> >>>>  	size_t copied = 0;
> >>>>
> >>>>  	if (dstv == srcv)
> >>>>  		return fuse_buf_size(dstv);
> >>>>
> >>>> +	/* use writev to improve bandwidth when all the
> >>>> +	 * src buffers already mapped by the daemon
> >>>> +	 * process */
> >>>
> >>> Some more checks to make this generic:
> >>>
> >>>> +	for (i = 0; i < srcv->count; i++) {
> >>>> +		if (srcv->buf[i].flags & FUSE_BUF_PHYS_ADDR)
> >>>> +			break;
> >>>
> >>>                 if (srcv->buf[i].flags & FUSE_BUF_IS_FD)
> >>> 		        break;
> >>>
> >>>> +	}
> >>>> +	if (i == srcv->count)
> >>>
> >>> Please verify the preconditions for the destination buffer too:
> >>>
> >>>   && dstv->count == 1 && dstv->idx == 0 && dstv->off == 0 &&
> >>>   (dstv->buf[0].flags & FUSE_BUF_IS_FD)
> >>
> >> Thanks for your comments, and I prefer adding checker just for
> >> FUSE_BUF_IS_FD as fuse_buf_writev() does not care much about the other
> >> parts. And I personally think so many preconditions will make
> >> fuse_buf_writev() so specific.
> > 
> > I think these preconditions are necessary.  This function won't do the
> > right thing if dstv has multiple elements or has been previously
> > modified (idx/off).
> > 
> > Can you explain why these cases will never happen or why the code is
> > correct under these cases?
> > 
> > Stefan
> > 
> 
> We have already reached an agreement with the src buf and need discuss
> with the dst buf. So I spent some time looking into libfuse code, and
> found your suggestion really make sense. My previous thought was that
> idx/off only works when dst is buf. So I will make these preconditions
> more generic like this:
> 
> In fuse_buf_copy():
> if (i == srcv->count) {
> dstv->buf[0].pos += dstv->off; // add dst offset even if it's always 0
> fuse_buf_writev(req, &dstv->buf[0], srcv);
> }

The caller doesn't expect dstv->buf[0].pos to change, so it is safer to
use a local variable for the file position inside fuse_buf_writev()
instead of doing this in fuse_buf_copy().

Hope this makes sense,
Stefan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virtio-fs/attachments/20190819/a12e90d6/attachment.sig>