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
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:
quoted
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
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.
----
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.
quoted
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
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:
quoted
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.
quoted hunk
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.
----
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.
quoted
quoted
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
Jisheng Zhang wrote on Tue, Mar 02, 2021 at 03:39:40PM +0800:
quoted
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
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:
quoted
quoted
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.