Messages in this thread Patch in this message | | | Date | Tue, 2 Mar 2021 15:39:40 +0800 | From | Jisheng Zhang <> | Subject | Re: [PATCH] net: 9p: free what was emitted when read count is 0 |
| |
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
| |