| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACGkMEvgPAJcZW0EUuOK7byo2djOavT8Cai83=tH_dFJaVSY8Q@mail.gmail.com>
Date: Thu, 23 Jan 2025 09:54:01 +0800
From: Jason Wang <jasowang@...hat.com>
To: Eugenio Pérez <eperezma@...hat.com>
Cc: linux-kernel@...r.kernel.org, mst@...hat.com,
Hanna Reitz <hreitz@...hat.com>, Stefano Garzarella <sgarzare@...hat.com>,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Vivek Goyal <vgoyal@...hat.com>,
Miklos Szeredi <miklos@...redi.hu>, German Maglione <gmaglione@...hat.com>,
Stefan Hajnoczi <stefanha@...hat.com>, virtualization@...ts.linux.dev
Subject: Re: [RFC 3/5] virtiofs: Move stack sg to fuse_req
On Thu, Jan 23, 2025 at 12:32 AM Eugenio Pérez <eperezma@...hat.com> wrote:
>
> We need to move the map and unmap out of virtiofs queue lock. We need to
> store the unmap sgs to call virtqueue_unmap_sgs, so use the request
> itself.
>
> TODO: We need to store the forget sgs too. In my tests it is not called
> so we're safe. Find a way to force-call it.
>
> Signed-off-by: Eugenio Pérez <eperezma@...hat.com>
> ---
> fs/fuse/fuse_i.h | 7 +++++++
> fs/fuse/virtio_fs.c | 45 +++++++++++++++++++++------------------------
> 2 files changed, 28 insertions(+), 24 deletions(-)
>
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 74744c6f2860..e57664daa761 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -18,6 +18,7 @@
> #include <linux/mount.h>
> #include <linux/wait.h>
> #include <linux/list.h>
> +#include <linux/scatterlist.h>
> #include <linux/spinlock.h>
> #include <linux/mm.h>
> #include <linux/backing-dev.h>
> @@ -434,6 +435,12 @@ struct fuse_req {
> #if IS_ENABLED(CONFIG_VIRTIO_FS)
> /** virtio-fs's physically contiguous buffer for in and out args */
> void *argbuf;
> +
> + /** virtio-fs's pre-mapped stuff */
> + struct scatterlist sg_inline_data[6]; /* optimization for short requests */
> + struct scatterlist *sg;
> + unsigned int out_sgs;
> + unsigned int in_sgs;
> #endif
As this touches FUSE, I think we need to do a careful benchmark just
for this patch to make sure no regression is introduced.
A dumb question (know little about FUSE), if this is only used by
virtio-fs, could we find a way to embed fuse_req in a virtio-fs
dedicated req structure?
>
> /** fuse_mount this request belongs to */
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 82afe78ec542..1344c5782a7c 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -1377,14 +1377,10 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
> {
> /* requests need at least 4 elements */
> struct scatterlist *stack_sgs[6];
> - struct scatterlist stack_sg[ARRAY_SIZE(stack_sgs)];
> struct scatterlist **sgs = stack_sgs;
> - struct scatterlist *sg = stack_sg;
> struct virtqueue *vq;
> struct fuse_args *args = req->args;
> unsigned int argbuf_used = 0;
> - unsigned int out_sgs = 0;
> - unsigned int in_sgs = 0;
> unsigned int total_sgs;
> unsigned int i;
> int ret;
> @@ -1392,11 +1388,13 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
> struct fuse_pqueue *fpq;
>
> /* Does the sglist fit on the stack? */
> + /* TODO replace magic 6 by a macro */
> + req->sg = req->sg_inline_data;
> total_sgs = sg_count_fuse_req(req);
> - if (total_sgs > ARRAY_SIZE(stack_sgs)) {
> + if (total_sgs > 6) {
> sgs = kmalloc_array(total_sgs, sizeof(sgs[0]), gfp);
> - sg = kmalloc_array(total_sgs, sizeof(sg[0]), gfp);
> - if (!sgs || !sg) {
> + req->sg = kmalloc_array(total_sgs, sizeof(req->sg[0]), gfp);
> + if (!sgs || !req->sg) {
> ret = -ENOMEM;
> goto out;
> }
> @@ -1408,26 +1406,25 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
> goto out;
>
> /* Request elements */
> - sg_init_one(&sg[out_sgs++], &req->in.h, sizeof(req->in.h));
> - out_sgs += sg_init_fuse_args(&sg[out_sgs], req,
> - (struct fuse_arg *)args->in_args,
> - args->in_numargs, args->in_pages,
> - req->argbuf, &argbuf_used);
> + sg_init_one(&req->sg[req->out_sgs++], &req->in.h, sizeof(req->in.h));
> + req->out_sgs += sg_init_fuse_args(&req->sg[req->out_sgs], req,
> + (struct fuse_arg *)args->in_args,
> + args->in_numargs, args->in_pages,
> + req->argbuf, &argbuf_used);
>
> /* Reply elements */
> if (test_bit(FR_ISREPLY, &req->flags)) {
> - sg_init_one(&sg[out_sgs + in_sgs++],
> + sg_init_one(&req->sg[req->out_sgs + req->in_sgs++],
> &req->out.h, sizeof(req->out.h));
> - in_sgs += sg_init_fuse_args(&sg[out_sgs + in_sgs], req,
> - args->out_args, args->out_numargs,
> - args->out_pages,
> - req->argbuf + argbuf_used, NULL);
> + req->in_sgs += sg_init_fuse_args(&req->sg[req->out_sgs + req->in_sgs], req,
> + args->out_args, args->out_numargs,
> + args->out_pages,
> + req->argbuf + argbuf_used, NULL);
> }
>
> - WARN_ON(out_sgs + in_sgs != total_sgs);
> -
> for (i = 0; i < total_sgs; i++)
> - sgs[i] = &sg[i];
> + sgs[i] = &req->sg[i];
> + WARN_ON(req->out_sgs + req->in_sgs != total_sgs);
>
> spin_lock(&fsvq->lock);
>
> @@ -1438,7 +1435,7 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
> }
>
> vq = fsvq->vq;
> - ret = virtqueue_add_sgs(vq, sgs, out_sgs, in_sgs, req, GFP_ATOMIC);
> + ret = virtqueue_add_sgs(vq, sgs, req->out_sgs, req->in_sgs, req, GFP_ATOMIC);
> if (ret < 0) {
> spin_unlock(&fsvq->lock);
> goto out;
> @@ -1467,10 +1464,10 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq,
> kfree(req->argbuf);
> req->argbuf = NULL;
> }
> - if (sgs != stack_sgs) {
> + if (ret < 0 && req->sg != req->sg_inline_data)
> + kfree(req->sg);
> + if (sgs != stack_sgs)
> kfree(sgs);
> - kfree(sg);
> - }
>
> return ret;
> }
> --
> 2.48.1
>
Thanks
Powered by blists - more mailing lists