lkml.org 
[lkml]   [2021]   [Mar]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2021-03-02 16:22    [W:0.095 / U:0.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site