[PATCH] net: 9p: free what was emitted when read count is 0

[PATCH] net: 9p: free what was emitted when read count is 0

[Jisheng Zhang]

I met below warning when cating a small size(about 80bytes) txt file
on 9pfs(msize=2097152 is passed to 9p mount option), the reason is we
miss iov_iter_advance() if the read count is 0, so we didn't truncate
the pipe, then iov_iter_pipe() thinks the pipe is full. Fix it by
calling iov_iter_advance() on the iov_iter "to" even if read count is 0

[    8.279568] WARNING: CPU: 0 PID: 39 at lib/iov_iter.c:1203 iov_iter_pipe+0x31/0x40
[    8.280028] Modules linked in:
[    8.280561] CPU: 0 PID: 39 Comm: cat Not tainted 5.11.0+ #6
[    8.281260] RIP: 0010:iov_iter_pipe+0x31/0x40
[    8.281974] Code: 2b 42 54 39 42 5c 76 22 c7 07 20 00 00 00 48 89 57 18 8b 42 50 48 c7 47 08 b
[    8.283169] RSP: 0018:ffff888000cbbd80 EFLAGS: 00000246
[    8.283512] RAX: 0000000000000010 RBX: ffff888000117d00 RCX: 0000000000000000
[    8.283876] RDX: ffff88800031d600 RSI: 0000000000000000 RDI: ffff888000cbbd90
[    8.284244] RBP: ffff888000cbbe38 R08: 0000000000000000 R09: ffff8880008d2058
[    8.284605] R10: 0000000000000002 R11: ffff888000375510 R12: 0000000000000050
[    8.284964] R13: ffff888000cbbe80 R14: 0000000000000050 R15: ffff88800031d600
[    8.285439] FS:  00007f24fd8af600(0000) GS:ffff88803ec00000(0000) knlGS:0000000000000000
[    8.285844] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    8.286150] CR2: 00007f24fd7d7b90 CR3: 0000000000c97000 CR4: 00000000000406b0
[    8.286710] Call Trace:
[    8.288279]  generic_file_splice_read+0x31/0x1a0
[    8.289273]  ? do_splice_to+0x2f/0x90
[    8.289511]  splice_direct_to_actor+0xcc/0x220
[    8.289788]  ? pipe_to_sendpage+0xa0/0xa0
[    8.290052]  do_splice_direct+0x8b/0xd0
[    8.290314]  do_sendfile+0x1ad/0x470
[    8.290576]  do_syscall_64+0x2d/0x40
[    8.290818]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[    8.291409] RIP: 0033:0x7f24fd7dca0a
[    8.292511] Code: c3 0f 1f 80 00 00 00 00 4c 89 d2 4c 89 c6 e9 bd fd ff ff 0f 1f 44 00 00 31 8
[    8.293360] RSP: 002b:00007ffc20932818 EFLAGS: 00000206 ORIG_RAX: 0000000000000028
[    8.293800] RAX: ffffffffffffffda RBX: 0000000001000000 RCX: 00007f24fd7dca0a
[    8.294153] RDX: 0000000000000000 RSI: 0000000000000003 RDI: 0000000000000001
[    8.294504] RBP: 0000000000000003 R08: 0000000000000000 R09: 0000000000000000
[    8.294867] R10: 0000000001000000 R11: 0000000000000206 R12: 0000000000000003
[    8.295217] R13: 0000000000000001 R14: 0000000000000001 R15: 0000000000000000
[    8.295782] ---[ end trace 63317af81b3ca24b ]---

Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
---
 net/9p/client.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/9p/client.c b/net/9p/client.c
index 4f62f299da0c..6dc01008cd3b 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -1624,6 +1624,7 @@ p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to,
 
 	p9_debug(P9_DEBUG_9P, "<<< RREAD count %d\n", count);
 	if (!count) {
+		iov_iter_advance(to, count);
 		p9_tag_remove(clnt, req);
 		return 0;
 	}
-- 
2.30.1

Re: [PATCH] net: 9p: free what was emitted when read count is 0

[Dominique Martinet]

Jisheng Zhang wrote on Mon, Mar 01, 2021 at 10:33:36AM +0800:
> I met below warning when cating a small size(about 80bytes) txt file
> on 9pfs(msize=2097152 is passed to 9p mount option), the reason is we
> miss iov_iter_advance() if the read count is 0, so we didn't truncate
> the pipe, then iov_iter_pipe() thinks the pipe is full. Fix it by
> calling iov_iter_advance() on the iov_iter "to" even if read count is 0

Hm, there are plenty of other error cases that don't call
iov_iter_advance() and shouldn't trigger this warning ; I'm not sure
just adding one particular call to this is a good solution.


How reproducible is this? From the description it should happen
everytime you cat a small file? (I'm surprised cat uses sendfile, what
cat version? coreutils' doesn't seem to do that on their git)

What kernel version do you get this on? Bonus points if you can confirm
this didn't use to happen, and full points for a bisect.


(cat on a small file is something I do all the time in my tests, I'd
like to be able to reproduce to understand the issue better as I'm not
familiar with that part of the code)

Thanks,
-- 
Dominique

Re: [PATCH] net: 9p: free what was emitted when read count is 0

[Jisheng Zhang]

On Mon, 1 Mar 2021 11:51:24 +0900 Dominique Martinet wrote:


> 
> 
> Jisheng Zhang wrote on Mon, Mar 01, 2021 at 10:33:36AM +0800:
> > I met below warning when cating a small size(about 80bytes) txt file
> > on 9pfs(msize=2097152 is passed to 9p mount option), the reason is we
> > miss iov_iter_advance() if the read count is 0, so we didn't truncate
> > the pipe, then iov_iter_pipe() thinks the pipe is full. Fix it by
> > calling iov_iter_advance() on the iov_iter "to" even if read count is 0  
> 
> Hm, there are plenty of other error cases that don't call
> iov_iter_advance() and shouldn't trigger this warning ; I'm not sure
> just adding one particular call to this is a good solution.

Per my understanding of iov_iter, we need to call iov_iter_advance()
even when the read out count is 0. I believe we can see this common style
in other fs.

> 
> 
> How reproducible is this? From the description it should happen

100%

> everytime you cat a small file? (I'm surprised cat uses sendfile, what

it happened every time when catting a small file.

> cat version? coreutils' doesn't seem to do that on their git)

busybox cat

> 
> What kernel version do you get this on? Bonus points if you can confirm

5.11 and the latest linus tree

> this didn't use to happen, and full points for a bisect.
> 
> 
> (cat on a small file is something I do all the time in my tests, I'd
> like to be able to reproduce to understand the issue better as I'm not
> familiar with that part of the code)

Per my check, it can be 100% reproduced with busybox cat + "msize=2097152"
mount option. NOTE: msize=2097152 isn't a magic number it can be other
numbers which can ensure zerocopy code path is executed: p9_client_read_once
->p9_client_zc_rpc()

Thanks

Re: [PATCH] net: 9p: free what was emitted when read count is 0

[Dominique Martinet]

Jisheng Zhang wrote on Mon, Mar 01, 2021 at 11:01:57AM +0800:
> Per my understanding of iov_iter, we need to call iov_iter_advance()
> even when the read out count is 0. I believe we can see this common style
> in other fs.

I'm not sure where you see this style, but I don't see exceptions for
0-sized read not advancing the iov in general, and I guess this makes
sense.


Rather than make an exception for 0, how about just removing the if as
follow ?

I've checked that the non_zc case (copy_to_iter with 0 size) also works
to the same effect, so I'm not sure why the check got added in the
first place... But then again this is old code so maybe the semantics
changed since 2015.


----
diff --git a/net/9p/client.c b/net/9p/client.c
index 4f62f299da0c..0a0039255c5b 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -1623,11 +1623,6 @@ p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to,
        }

        p9_debug(P9_DEBUG_9P, "<<< RREAD count %d\n", count);
-       if (!count) {
-               p9_tag_remove(clnt, req);
-               return 0;
-       }
-
        if (non_zc) {
                int n = copy_to_iter(dataptr, count, to);


----

If you're ok with that, would you mind resending that way?

I'd also want the commit message to be reworded a bit, at least the
first line (summary) doesn't make sense right now: I have no idea
what you mean by "free what was emitted".
Just "9p: advance iov on empty read" or something similar would do.


> > cat version? coreutils' doesn't seem to do that on their git)
> 
> busybox cat

Ok, could reproduce with busybox cat, thanks.
As expected I can't reproduce with older kernels so will run a bisect
for the sake of it as time allows

-- 
Dominique

Re: [PATCH] net: 9p: free what was emitted when read count is 0

[Jisheng Zhang]

On Tue, 2 Mar 2021 13:38:08 +0900 Dominique Martinet wrote:

> 
> 
> Jisheng Zhang wrote on Mon, Mar 01, 2021 at 11:01:57AM +0800:
> > Per my understanding of iov_iter, we need to call iov_iter_advance()
> > even when the read out count is 0. I believe we can see this common style
> > in other fs.  
> 
> I'm not sure where you see this style, but I don't see exceptions for
> 0-sized read not advancing the iov in general, and I guess this makes
> sense.

for example, function dio_refill_pages() in fs/direct-io.c, and below code piece
from net/core/datagram.c:

                copied = iov_iter_get_pages(from, pages, length,
                                            MAX_SKB_FRAGS - frag, &start);
                if (copied < 0)
                        return -EFAULT;

                iov_iter_advance(from, copied);

As can be seen, for "copied >=0" case, we call iov_iter_advance()

> 
> 
> Rather than make an exception for 0, how about just removing the if as
> follow ?

IMHO, we may need to keep the "if" in current logic. When count
reaches zero, we need to break the "while(iov_iter_count(to))" loop, so removing
the "if" modifying the logic.

> 
> I've checked that the non_zc case (copy_to_iter with 0 size) also works
> to the same effect, so I'm not sure why the check got added in the
> first place... But then again this is old code so maybe the semantics
> changed since 2015.
> 
> 
> ----
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 4f62f299da0c..0a0039255c5b 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -1623,11 +1623,6 @@ p9_client_read_once(struct p9_fid *fid, u64 offset, struct iov_iter *to,
>         }
> 
>         p9_debug(P9_DEBUG_9P, "<<< RREAD count %d\n", count);
> -       if (!count) {
> -               p9_tag_remove(clnt, req);
> -               return 0;
> -       }
> -
>         if (non_zc) {
>                 int n = copy_to_iter(dataptr, count, to);
> 
> 
> ----
> 
> If you're ok with that, would you mind resending that way?
> 
> I'd also want the commit message to be reworded a bit, at least the
> first line (summary) doesn't make sense right now: I have no idea
> what you mean by "free what was emitted".
> Just "9p: advance iov on empty read" or something similar would do.

Thanks for the suggestion. I will send a v2 to update the commit msg but
keep the patch as is if you agree with above keeping "if" logic.
> 
> 
> > > cat version? coreutils' doesn't seem to do that on their git)  
> >
> > busybox cat  
> 
> Ok, could reproduce with busybox cat, thanks.
> As expected I can't reproduce with older kernels so will run a bisect
> for the sake of it as time allows
> 

Thanks

Re: [PATCH] net: 9p: free what was emitted when read count is 0

[Dominique Martinet]

Jisheng Zhang wrote on Tue, Mar 02, 2021 at 03:39:40PM +0800:
> > Rather than make an exception for 0, how about just removing the if as
> > follow ?
> 
> IMHO, we may need to keep the "if" in current logic. When count
> reaches zero, we need to break the "while(iov_iter_count(to))" loop, so removing
> the "if" modifying the logic.

We're not looking at the same loop, the break will happen properly
without the if because it's the return value of p9_client_read_once()
now.

In the old code I remember what you're saying and it makes sense, I
guess that was the reason for the special case.
It's not longer required, let's remove it.

-- 
Dominique

Re: [PATCH] net: 9p: free what was emitted when read count is 0

[Jisheng Zhang]

On Tue, 2 Mar 2021 17:08:13 +0900 Dominique Martinet wrote:


> 
> 
> Jisheng Zhang wrote on Tue, Mar 02, 2021 at 03:39:40PM +0800:
> > > Rather than make an exception for 0, how about just removing the if as
> > > follow ?  
> >
> > IMHO, we may need to keep the "if" in current logic. When count
> > reaches zero, we need to break the "while(iov_iter_count(to))" loop, so removing
> > the "if" modifying the logic.  
> 
> We're not looking at the same loop, the break will happen properly

I was reading the old code because I switched to linux-5.4 longterm tree
for other development ;)

> without the if because it's the return value of p9_client_read_once()
> now.
> 
> In the old code I remember what you're saying and it makes sense, I
> guess that was the reason for the special case.
> It's not longer required, let's remove it.

Thank you. patch v2 is sent out.