Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 2 Mar 2021 13:38:08 +0900
From:   Dominique Martinet <asmadeus@codewreck.org>
To:     Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Cc:     Eric Van Hensbergen <ericvh@gmail.com>,
        Latchesar Ionkov <lucho@ionkov.net>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        v9fs-developer@lists.sourceforge.net, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: 9p: free what was emitted when read count is 0
Message-ID: <YD3BMLuZXIcETtzp@codewreck.org>
References: <20210301103336.2e29da13@xhacker.debian>
 <YDxWrB8AoxJOmScE@odin>
 <20210301110157.19d9ad4e@xhacker.debian>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20210301110157.19d9ad4e@xhacker.debian>
Precedence: bulk

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