Hi,
On 2/29/2024 11:01 PM, Brian Foster wrote:
> On Wed, Feb 28, 2024 at 10:41:24PM +0800, Hou Tao wrote:
>> From: Hou Tao <houtao1@xxxxxxxxxx>
>>
>> When reading a file kept in virtiofs from kernel (e.g., insmod a kernel
>> module), if the cache of virtiofs is disabled, the read buffer will be
>> passed to virtiofs through out_args[0].value instead of pages. Because
>> virtiofs can't get the pages for the read buffer, virtio_fs_argbuf_new()
>> will create a bounce buffer for the read buffer by using kmalloc() and
>> copy the read buffer into bounce buffer. If the read buffer is large
>> (e.g., 1MB), the allocation will incur significant stress on the memory
>> subsystem.
>>
>> So instead of allocating bounce buffer by using kmalloc(), allocate a
>> bounce buffer which is backed by scattered pages. The original idea is
>> to use vmap(), but the use of GFP_ATOMIC is no possible for vmap(). To
>> simplify the copy operations in the bounce buffer, use a bio_vec flex
>> array to represent the argbuf. Also add an is_flat field in struct
>> virtio_fs_argbuf to distinguish between kmalloc-ed and scattered bounce
>> buffer.
>>
>> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
>> ---
>> fs/fuse/virtio_fs.c | 163 ++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 149 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
>> index f10fff7f23a0f..ffea684bd100d 100644
>> --- a/fs/fuse/virtio_fs.c
>> +++ b/fs/fuse/virtio_fs.c
> ...
>> @@ -408,42 +425,143 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
>> }
>> }
>>
> ...
>> static void virtio_fs_argbuf_copy_from_in_arg(struct virtio_fs_argbuf *argbuf,
>> unsigned int offset,
>> const void *src, unsigned int len)
>> {
>> - memcpy(argbuf->buf + offset, src, len);
>> + struct iov_iter iter;
>> + unsigned int copied;
>> +
>> + if (argbuf->is_flat) {
>> + memcpy(argbuf->f.buf + offset, src, len);
>> + return;
>> + }
>> +
>> + iov_iter_bvec(&iter, ITER_DEST, argbuf->s.bvec,
>> + argbuf->s.nr, argbuf->s.size);
>> + iov_iter_advance(&iter, offset);
> Hi Hou,
>
> Just a random comment, but it seems a little inefficient to reinit and
> readvance the iter like this on every copy/call. It looks like offset is
> already incremented in the callers of the argbuf copy helpers. Perhaps
> iov_iter could be lifted into the callers and passed down, or even just
> include it in the argbuf structure and init it at alloc time?
Sorry for the late reply. Being busy with off-site workshop these days.
I have tried a similar idea before in which iov_iter was saved directly
in argbuf struct, but it didn't work out well. The reason is that for
copy both in_args and out_args, an iov_iter is needed, but the direction
is different. Currently the bi-directional io_vec is not supported, so
the code have to initialize the iov_iter twice: one for copy from
in_args and another one for copy to out_args.
For dio read initiated from kernel, both of its in_numargs and
out_numargs is 1, so there will be only one iov_iter_advance() in
virtio_fs_argbuf_copy_to_out_arg() and the offset is 64, so I think the
overhead will be fine. For dio write initiated from kernel, its
in_numargs is 2 and out_numargs is 1, so there will be two invocations
of iov_iter_advance(). The first one with offset=64, and the another one
with offset=round_up_page_size(64 + write_size), so the later one may
introduce extra overhead. But compared with the overhead of data copy, I
still think the overhead of calling iov_iter_advance() is fine.
> Brian
>
>> +
>> + copied = _copy_to_iter(src, len, &iter);
>> + WARN_ON_ONCE(copied != len);
>> }
>>
>> static unsigned int
>> @@ -451,15 +569,32 @@ virtio_fs_argbuf_out_args_offset(struct virtio_fs_argbuf *argbuf,
>> const struct fuse_args *args)
>> {
>> unsigned int num_in = args->in_numargs - args->in_pages;
>> + unsigned int offset = fuse_len_args(num_in,
>> + (struct fuse_arg *)args->in_args);
>>
>> - return fuse_len_args(num_in, (struct fuse_arg *)args->in_args);
>> + if (argbuf->is_flat)
>> + return offset;
>> + return round_up(offset, PAGE_SIZE);
>> }
>>
>> static void virtio_fs_argbuf_copy_to_out_arg(struct virtio_fs_argbuf *argbuf,
>> unsigned int offset, void *dst,
>> unsigned int len)
>> {
>> - memcpy(dst, argbuf->buf + offset, len);
>> + struct iov_iter iter;
>> + unsigned int copied;
>> +
>> + if (argbuf->is_flat) {
>> + memcpy(dst, argbuf->f.buf + offset, len);
>> + return;
>> + }
>> +
>> + iov_iter_bvec(&iter, ITER_SOURCE, argbuf->s.bvec,
>> + argbuf->s.nr, argbuf->s.size);
>> + iov_iter_advance(&iter, offset);
>> +
>> + copied = _copy_from_iter(dst, len, &iter);
>> + WARN_ON_ONCE(copied != len);
>> }
>>
>> /*
>> @@ -1154,7 +1289,7 @@ static unsigned int sg_init_fuse_args(struct scatterlist *sg,
>> len = fuse_len_args(numargs - argpages, args);
>> if (len)
>> total_sgs += virtio_fs_argbuf_setup_sg(req->argbuf, *len_used,
>> - len, &sg[total_sgs]);
>> + &len, &sg[total_sgs]);
>>
>> if (argpages)
>> total_sgs += sg_init_fuse_pages(&sg[total_sgs],
>> @@ -1199,7 +1334,7 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
>> }
>>
>> /* Use a bounce buffer since stack args cannot be mapped */
>> - req->argbuf = virtio_fs_argbuf_new(args, GFP_ATOMIC);
>> + req->argbuf = virtio_fs_argbuf_new(args, GFP_ATOMIC, true);
>> if (!req->argbuf) {
>> ret = -ENOMEM;
>> goto out;
>> --
>> 2.29.2
>>
>>
