[Virtio-fs] [PATCH v4 2/2] virtiofsd: use fuse_buf_writev to replace fuse_buf_write for better performance
piaojun
piaojun at huawei.com
Fri Aug 16 00:48:59 UTC 2019
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
More information about the Virtio-fs
mailing list