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

[piaojun <piaojun@huawei.com>]

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);
}

In fuse_buf_writev():

// modify the preconditions
if (i == srcv->count && dstv->count == 1 &&
dstv->idx == 0 && (dstv->buf[0].flags & FUSE_BUF_IS_FD))

Jun