Re: [PATCH] net: 9p: free what was emitted when read count is 0
From: Jisheng Zhang <hidden>
Date: 2021-03-02 08:39:35
Also in:
lkml
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 ↗ jump to 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. ----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.
quoted
quoted
cat version? coreutils' doesn't seem to do that on their git)busybox catOk, 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