On Tue, Oct 06, 2020 at 03:09:49PM -0400, Vivek Goyal wrote: > On Tue, Oct 06, 2020 at 04:39:33PM +0100, Stefan Hajnoczi wrote: > > On Mon, Oct 05, 2020 at 01:45:31PM -0400, Vivek Goyal wrote: > > > virtiofs currently maps various buffers in scatter gather list and it looks > > > at number of pages (ap->pages) and assumes that same number of pages will > > > be used both for input and output (sg_count_fuse_req()), and calculates > > > total number of scatterlist elements accordingly. > > > > > > But looks like this assumption is not valid in all the cases. For example, > > > Cai Qian reported that trinity, triggers warning with virtiofs sometimes. > > > A closer look revealed that if one calls ioctl(fd, 0x5a004000, buf), it > > > will trigger following warning. > > > > > > WARN_ON(out_sgs + in_sgs != total_sgs) > > > > > > In this case, total_sgs = 8, out_sgs=4, in_sgs=3. Number of pages is 2 > > > (ap->pages), but out_sgs are using both the pages but in_sgs are using > > > only one page. (fuse_do_ioctl() sets out_size to one page). > > > > > > So existing WARN_ON() seems to be wrong. Instead of total_sgs, it should > > > be max_sgs and make sure out_sgs and in_sgs don't cross max_sgs. This > > > will allow input and output pages numbers to be different. > > > > > > Reported-by: Qian Cai <cai@xxxxxxxxxx> > > > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx> > > > Link: https://lore.kernel.org/linux-fsdevel/5ea77e9f6cb8c2db43b09fbd4158ab2d8c066a0a.camel@xxxxxxxxxx/ > > > --- > > > fs/fuse/virtio_fs.c | 14 +++++++------- > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c > > > index da3ede268604..3f4f2fa0bb96 100644 > > > --- a/fs/fuse/virtio_fs.c > > > +++ b/fs/fuse/virtio_fs.c > > > @@ -1110,17 +1110,17 @@ static int virtio_fs_enqueue_req(struct virtio_fs_vq *fsvq, > > > unsigned int argbuf_used = 0; > > > unsigned int out_sgs = 0; > > > unsigned int in_sgs = 0; > > > - unsigned int total_sgs; > > > + unsigned int max_sgs; > > > unsigned int i; > > > int ret; > > > bool notify; > > > struct fuse_pqueue *fpq; > > > > > > /* Does the sglist fit on the stack? */ > > > - total_sgs = sg_count_fuse_req(req); > > > > sg_count_fuse_req() should be exact. It's risky to treat it as a maximum > > unless all cases where in_sgs + out_sgs < total_sgs are understood. Even > > then, it's still possible that new bugs introduced to the code will go > > undetected due to the weaker WARN_ON() condition. > > > > Do you have the values of the relevant fuse_req and fuse_args_pages > > fields so we can understand exactly what happened? I think the issue is > > that sg_count_fuse_req() doesn't use the fuse_page_desc size field. > > Hi Stefan, > > I revised the patch. How about following. This calculates number of > sgs accurately by going through ap->descs and size fields. > > Thanks > Vivek > > From 24b590ebc2ffc8ed02c013b11818af89d0b135ba Mon Sep 17 00:00:00 2001 > From: Vivek Goyal <vgoyal@xxxxxxxxxx> > Date: Tue, 6 Oct 2020 14:53:06 -0400 > Subject: [PATCH 1/1] virtiofs: Calculate number of scatter-gather elements > accurately > > virtiofs currently maps various buffers in scatter gather list and it looks > at number of pages (ap->pages) and assumes that same number of pages will > be used both for input and output (sg_count_fuse_req()), and calculates > total number of scatterlist elements accordingly. > > But looks like this assumption is not valid in all the cases. For example, > Cai Qian reported that trinity, triggers warning with virtiofs sometimes. > A closer look revealed that if one calls ioctl(fd, 0x5a004000, buf), it > will trigger following warning. > > WARN_ON(out_sgs + in_sgs != total_sgs) > > In this case, total_sgs = 8, out_sgs=4, in_sgs=3. Number of pages is 2 > (ap->pages), but out_sgs are using both the pages but in_sgs are using > only one page. In this case, fuse_do_ioctl() sets different size values > for input and output. > > args->in_args[args->in_numargs - 1].size == 6656 > args->out_args[args->out_numargs - 1].size == 4096 > > So current method of calculating how many scatter-gather list elements > will be used is not accurate. Make calculations more precise by parsing > size and ap->descs. > > Reported-by: Qian Cai <cai@xxxxxxxxxx> > Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx> > Link: https://lore.kernel.org/linux-fsdevel/5ea77e9f6cb8c2db43b09fbd4158ab2d8c066a0a.camel@xxxxxxxxxx/ > --- > fs/fuse/virtio_fs.c | 30 ++++++++++++++++++++++++++---- > 1 file changed, 26 insertions(+), 4 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
Attachment:
signature.asc
Description: PGP signature
