| Message ID | 20200407191106.24045.88035.stgit@klimt.1015granger.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | NFS/RDMA server fixes for 5.7-rc | expand |
On Tue, Apr 07, 2020 at 03:11:06PM -0400, Chuck Lever wrote: > Utilize the xpo_release_rqst transport method to ensure that each > rqstp's svc_rdma_recv_ctxt object is released even when the server > cannot return a Reply for that rqstp. > > Without this fix, each RPC whose Reply cannot be sent leaks one > svc_rdma_recv_ctxt. This is a 2.5KB structure, a 4KB DMA-mapped > Receive buffer, and any pages that might be part of the Reply > message. > > The leak is infrequent unless the network fabric is unreliable or > Kerberos is in use, as GSS sequence window overruns, which result > in connection loss, are more common on fast transports. > > Fixes: 3a88092ee319 ("svcrdma: Preserve Receive buffer until ... ") Chuck, Can you please don't mangle the Fixes line? A lot of automatization is relying on the fact that this line is canonical, both in format and in the actual content. Thanks > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > include/linux/sunrpc/svc_rdma.h | 1 + > net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 22 ++++++++++++++++++++++ > net/sunrpc/xprtrdma/svc_rdma_sendto.c | 13 +++---------- > net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 ----- > 4 files changed, 26 insertions(+), 15 deletions(-) > > diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h > index 78fe2ac6dc6c..cbcfbd0521e3 100644 > --- a/include/linux/sunrpc/svc_rdma.h > +++ b/include/linux/sunrpc/svc_rdma.h > @@ -170,6 +170,7 @@ extern bool svc_rdma_post_recvs(struct svcxprt_rdma *rdma); > extern void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma, > struct svc_rdma_recv_ctxt *ctxt); > extern void svc_rdma_flush_recv_queues(struct svcxprt_rdma *rdma); > +extern void svc_rdma_release_rqst(struct svc_rqst *rqstp); > extern int svc_rdma_recvfrom(struct svc_rqst *); > > /* svc_rdma_rw.c */ > diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c > index 54469b72b25f..efa5fcb5793f 100644 > --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c > +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c > @@ -223,6 +223,26 @@ void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma, > svc_rdma_recv_ctxt_destroy(rdma, ctxt); > } > > +/** > + * svc_rdma_release_rqst - Release transport-specific per-rqst resources > + * @rqstp: svc_rqst being released > + * > + * Ensure that the recv_ctxt is released whether or not a Reply > + * was sent. For example, the client could close the connection, > + * or svc_process could drop an RPC, before the Reply is sent. > + */ > +void svc_rdma_release_rqst(struct svc_rqst *rqstp) > +{ > + struct svc_rdma_recv_ctxt *ctxt = rqstp->rq_xprt_ctxt; > + struct svc_xprt *xprt = rqstp->rq_xprt; > + struct svcxprt_rdma *rdma = > + container_of(xprt, struct svcxprt_rdma, sc_xprt); > + > + rqstp->rq_xprt_ctxt = NULL; > + if (ctxt) > + svc_rdma_recv_ctxt_put(rdma, ctxt); > +} > + > static int __svc_rdma_post_recv(struct svcxprt_rdma *rdma, > struct svc_rdma_recv_ctxt *ctxt) > { > @@ -820,6 +840,8 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp) > __be32 *p; > int ret; > > + rqstp->rq_xprt_ctxt = NULL; > + > spin_lock(&rdma_xprt->sc_rq_dto_lock); > ctxt = svc_rdma_next_recv_ctxt(&rdma_xprt->sc_read_complete_q); > if (ctxt) { > diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c > index 6a87a2379e91..b6c8643867f2 100644 > --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c > +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c > @@ -926,12 +926,7 @@ int svc_rdma_sendto(struct svc_rqst *rqstp) > ret = svc_rdma_send_reply_msg(rdma, sctxt, rctxt, rqstp); > if (ret < 0) > goto err1; > - ret = 0; > - > -out: > - rqstp->rq_xprt_ctxt = NULL; > - svc_rdma_recv_ctxt_put(rdma, rctxt); > - return ret; > + return 0; > > err2: > if (ret != -E2BIG && ret != -EINVAL) > @@ -940,16 +935,14 @@ int svc_rdma_sendto(struct svc_rqst *rqstp) > ret = svc_rdma_send_error_msg(rdma, sctxt, rqstp); > if (ret < 0) > goto err1; > - ret = 0; > - goto out; > + return 0; > > err1: > svc_rdma_send_ctxt_put(rdma, sctxt); > err0: > trace_svcrdma_send_failed(rqstp, ret); > set_bit(XPT_CLOSE, &xprt->xpt_flags); > - ret = -ENOTCONN; > - goto out; > + return -ENOTCONN; > } > > /** > diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c > index 8bb99980ae85..ea54785db4f8 100644 > --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c > +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c > @@ -71,7 +71,6 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv, > struct sockaddr *sa, int salen, > int flags); > static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt); > -static void svc_rdma_release_rqst(struct svc_rqst *); > static void svc_rdma_detach(struct svc_xprt *xprt); > static void svc_rdma_free(struct svc_xprt *xprt); > static int svc_rdma_has_wspace(struct svc_xprt *xprt); > @@ -552,10 +551,6 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt) > return NULL; > } > > -static void svc_rdma_release_rqst(struct svc_rqst *rqstp) > -{ > -} > - > /* > * When connected, an svc_xprt has at least two references: > * >
Hi Leon- > On Apr 8, 2020, at 2:02 AM, Leon Romanovsky <leon@kernel.org> wrote: > > On Tue, Apr 07, 2020 at 03:11:06PM -0400, Chuck Lever wrote: >> Utilize the xpo_release_rqst transport method to ensure that each >> rqstp's svc_rdma_recv_ctxt object is released even when the server >> cannot return a Reply for that rqstp. >> >> Without this fix, each RPC whose Reply cannot be sent leaks one >> svc_rdma_recv_ctxt. This is a 2.5KB structure, a 4KB DMA-mapped >> Receive buffer, and any pages that might be part of the Reply >> message. >> >> The leak is infrequent unless the network fabric is unreliable or >> Kerberos is in use, as GSS sequence window overruns, which result >> in connection loss, are more common on fast transports. >> >> Fixes: 3a88092ee319 ("svcrdma: Preserve Receive buffer until ... ") > > Chuck, > > Can you please don't mangle the Fixes line? I've read e-mail from others that advocate this form of mangling instead of using commit message lines that are too long. > A lot of automatization is relying on the fact that this line is canonical, > both in format and in the actual content. Understood, but checkpatch.pl does not complain about it. Perhaps, therefore, it is the automation that is not correct. The commit ID is what automation should key off of. The short description is only for human consumption. In fact, the short description can easily be reconstituted from the commit ID. The reverse cannot be done reliably. I'm not interested in bike-shedding this, however, so I will try to remember to use complete short descriptions when posting to linux-rdma. > Thanks > >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> include/linux/sunrpc/svc_rdma.h | 1 + >> net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 22 ++++++++++++++++++++++ >> net/sunrpc/xprtrdma/svc_rdma_sendto.c | 13 +++---------- >> net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 ----- >> 4 files changed, 26 insertions(+), 15 deletions(-) >> >> diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h >> index 78fe2ac6dc6c..cbcfbd0521e3 100644 >> --- a/include/linux/sunrpc/svc_rdma.h >> +++ b/include/linux/sunrpc/svc_rdma.h >> @@ -170,6 +170,7 @@ extern bool svc_rdma_post_recvs(struct svcxprt_rdma *rdma); >> extern void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma, >> struct svc_rdma_recv_ctxt *ctxt); >> extern void svc_rdma_flush_recv_queues(struct svcxprt_rdma *rdma); >> +extern void svc_rdma_release_rqst(struct svc_rqst *rqstp); >> extern int svc_rdma_recvfrom(struct svc_rqst *); >> >> /* svc_rdma_rw.c */ >> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c >> index 54469b72b25f..efa5fcb5793f 100644 >> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c >> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c >> @@ -223,6 +223,26 @@ void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma, >> svc_rdma_recv_ctxt_destroy(rdma, ctxt); >> } >> >> +/** >> + * svc_rdma_release_rqst - Release transport-specific per-rqst resources >> + * @rqstp: svc_rqst being released >> + * >> + * Ensure that the recv_ctxt is released whether or not a Reply >> + * was sent. For example, the client could close the connection, >> + * or svc_process could drop an RPC, before the Reply is sent. >> + */ >> +void svc_rdma_release_rqst(struct svc_rqst *rqstp) >> +{ >> + struct svc_rdma_recv_ctxt *ctxt = rqstp->rq_xprt_ctxt; >> + struct svc_xprt *xprt = rqstp->rq_xprt; >> + struct svcxprt_rdma *rdma = >> + container_of(xprt, struct svcxprt_rdma, sc_xprt); >> + >> + rqstp->rq_xprt_ctxt = NULL; >> + if (ctxt) >> + svc_rdma_recv_ctxt_put(rdma, ctxt); >> +} >> + >> static int __svc_rdma_post_recv(struct svcxprt_rdma *rdma, >> struct svc_rdma_recv_ctxt *ctxt) >> { >> @@ -820,6 +840,8 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp) >> __be32 *p; >> int ret; >> >> + rqstp->rq_xprt_ctxt = NULL; >> + >> spin_lock(&rdma_xprt->sc_rq_dto_lock); >> ctxt = svc_rdma_next_recv_ctxt(&rdma_xprt->sc_read_complete_q); >> if (ctxt) { >> diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c >> index 6a87a2379e91..b6c8643867f2 100644 >> --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c >> +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c >> @@ -926,12 +926,7 @@ int svc_rdma_sendto(struct svc_rqst *rqstp) >> ret = svc_rdma_send_reply_msg(rdma, sctxt, rctxt, rqstp); >> if (ret < 0) >> goto err1; >> - ret = 0; >> - >> -out: >> - rqstp->rq_xprt_ctxt = NULL; >> - svc_rdma_recv_ctxt_put(rdma, rctxt); >> - return ret; >> + return 0; >> >> err2: >> if (ret != -E2BIG && ret != -EINVAL) >> @@ -940,16 +935,14 @@ int svc_rdma_sendto(struct svc_rqst *rqstp) >> ret = svc_rdma_send_error_msg(rdma, sctxt, rqstp); >> if (ret < 0) >> goto err1; >> - ret = 0; >> - goto out; >> + return 0; >> >> err1: >> svc_rdma_send_ctxt_put(rdma, sctxt); >> err0: >> trace_svcrdma_send_failed(rqstp, ret); >> set_bit(XPT_CLOSE, &xprt->xpt_flags); >> - ret = -ENOTCONN; >> - goto out; >> + return -ENOTCONN; >> } >> >> /** >> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c >> index 8bb99980ae85..ea54785db4f8 100644 >> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c >> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c >> @@ -71,7 +71,6 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv, >> struct sockaddr *sa, int salen, >> int flags); >> static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt); >> -static void svc_rdma_release_rqst(struct svc_rqst *); >> static void svc_rdma_detach(struct svc_xprt *xprt); >> static void svc_rdma_free(struct svc_xprt *xprt); >> static int svc_rdma_has_wspace(struct svc_xprt *xprt); >> @@ -552,10 +551,6 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt) >> return NULL; >> } >> >> -static void svc_rdma_release_rqst(struct svc_rqst *rqstp) >> -{ >> -} >> - >> /* >> * When connected, an svc_xprt has at least two references: >> * -- Chuck Lever
On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote: > Hi Leon- > > > On Apr 8, 2020, at 2:02 AM, Leon Romanovsky <leon@kernel.org> wrote: > > > > On Tue, Apr 07, 2020 at 03:11:06PM -0400, Chuck Lever wrote: > >> Utilize the xpo_release_rqst transport method to ensure that each > >> rqstp's svc_rdma_recv_ctxt object is released even when the server > >> cannot return a Reply for that rqstp. > >> > >> Without this fix, each RPC whose Reply cannot be sent leaks one > >> svc_rdma_recv_ctxt. This is a 2.5KB structure, a 4KB DMA-mapped > >> Receive buffer, and any pages that might be part of the Reply > >> message. > >> > >> The leak is infrequent unless the network fabric is unreliable or > >> Kerberos is in use, as GSS sequence window overruns, which result > >> in connection loss, are more common on fast transports. > >> > >> Fixes: 3a88092ee319 ("svcrdma: Preserve Receive buffer until ... ") > > > > Chuck, > > > > Can you please don't mangle the Fixes line? > > I've read e-mail from others that advocate this form of mangling > instead of using commit message lines that are too long. Really? At least I won't accept Fixes lines that are not in the cannonical format, I routinely fix these things in all sorts of ways, but I've never seen someone shorten it with ... > > A lot of automatization is relying on the fact that this line is canonical, > > both in format and in the actual content. > > Understood, but checkpatch.pl does not complain about it. Perhaps, > therefore, it is the automation that is not correct. checkpatch.pl doesn't check Fixes lines for correctness, because it doesn't have access to the git or something. This was talked about too.. Stephen likes to check them as part of linux-next though. However, checkpatch.pl does not complain for long lines on Fixes: tags demanding they be shortened > The commit ID is what automation should key off of. The short > description is only for human consumption. Right, so if the actual commit message isn't included so humans can read it then what was the point of including anything? Jason
> On Apr 9, 2020, at 1:47 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote: >> Hi Leon- >> >>> On Apr 8, 2020, at 2:02 AM, Leon Romanovsky <leon@kernel.org> wrote: >>> >>> On Tue, Apr 07, 2020 at 03:11:06PM -0400, Chuck Lever wrote: >>>> Utilize the xpo_release_rqst transport method to ensure that each >>>> rqstp's svc_rdma_recv_ctxt object is released even when the server >>>> cannot return a Reply for that rqstp. >>>> >>>> Without this fix, each RPC whose Reply cannot be sent leaks one >>>> svc_rdma_recv_ctxt. This is a 2.5KB structure, a 4KB DMA-mapped >>>> Receive buffer, and any pages that might be part of the Reply >>>> message. >>>> >>>> The leak is infrequent unless the network fabric is unreliable or >>>> Kerberos is in use, as GSS sequence window overruns, which result >>>> in connection loss, are more common on fast transports. >>>> >>>> Fixes: 3a88092ee319 ("svcrdma: Preserve Receive buffer until ... ") >>> >>> Chuck, >>> >>> Can you please don't mangle the Fixes line? >> >> I've read e-mail from others that advocate this form of mangling >> instead of using commit message lines that are too long. > > Really? Yep. > At least I won't accept Fixes lines that are not in the cannonical > format, I routinely fix these things in all sorts of ways, but I've > never seen someone shorten it with ... It seems that is a Maintainers preference. >>> A lot of automatization is relying on the fact that this line is canonical, >>> both in format and in the actual content. >> >> Understood, but checkpatch.pl does not complain about it. Perhaps, >> therefore, it is the automation that is not correct. > > checkpatch.pl doesn't check Fixes lines for correctness, It certainly does check Fixes: lines for correctness. Lately, it's been warning about commit IDs that are not exactly 12 hexits long, and cases where the commit ID does not actually exist in the local repository. It used to complain about "..." as well, until someone said that is a frequently-used modification to keep the line length manageable. I think that might be why checkpatch.pl no longer checks the tag's short description. > because it > doesn't have access to the git or something. This was talked about > too.. Stephen likes to check them as part of linux-next though. > > However, checkpatch.pl does not complain for long lines on Fixes: > tags demanding they be shortened Maybe not, but I don't think that's always been the case. <shrug> >> The commit ID is what automation should key off of. The short >> description is only for human consumption. > > Right, so if the actual commit message isn't included so humans can > read it then what was the point of including anything? I didn't invent the Fixes: tag, and I didn't invent shortening it. That had to come from somewhere as well. So here we are. No matter, though. Now that I know this community's preference, I will stick to it. Will you take "yes" for an answer? ;-) -- Chuck Lever
On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote: > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote: > > The commit ID is what automation should key off of. The short > > description is only for human consumption. > > Right, so if the actual commit message isn't included so humans can > read it then what was the point of including anything? Personally as a human reading commits in a terminal window I prefer the abbreviated form. I haven't been doing the redundant parentheses and quotes either. Was that dreamt up by an Arlo Guthrie fan? ("KID, HAVE YOU REHABILITATED YOURSELF?") --b.
On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote: > On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote: > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote: > > > The commit ID is what automation should key off of. The short > > > description is only for human consumption. > > > > Right, so if the actual commit message isn't included so humans can > > read it then what was the point of including anything? > > Personally as a human reading commits in a terminal window I prefer the > abbreviated form. Frankly, I think they are useless, picking one of yours at random: Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... " And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so now we are just totally lost, with a bad commit ID and a mangled subject line. > I haven't been doing the redundant parentheses and quotes either. Was > that dreamt up by an Arlo Guthrie fan? ("KID, HAVE YOU REHABILITATED > YOURSELF?") Well it seems like you are just aren't following the standard style at all. :( Jason
On Tue, Apr 14, 2020 at 09:19:31AM -0300, Jason Gunthorpe wrote: > On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote: > > On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote: > > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote: > > > > The commit ID is what automation should key off of. The short > > > > description is only for human consumption. > > > > > > Right, so if the actual commit message isn't included so humans can > > > read it then what was the point of including anything? > > > > Personally as a human reading commits in a terminal window I prefer the > > abbreviated form. > > Frankly, I think they are useless, picking one of yours at random: > > Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... " > > And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so Ow, apologies. Looks like I rebased after writing that Fixes tag. I wonder if it's possible to make git warn.... Looks like a pre-rebase hook could check the branch being rebased for "Fixes:" lines referencing commits on the rebased branch. > now we are just totally lost, with a bad commit ID and a mangled > subject line. For what it's worth, that part of the subject line is enough to find the original commit (even to uniquely specify it). > > I haven't been doing the redundant parentheses and quotes either. Was > > that dreamt up by an Arlo Guthrie fan? ("KID, HAVE YOU REHABILITATED > > YOURSELF?") > > Well it seems like you are just aren't following the standard style > at all. :( Yeah, I don't like it. I'll admit I don't know why *this* exactly is what I'm choosing to feel stubborn about. --b.
On Tue, Apr 14, 2020 at 11:13:03AM -0400, J. Bruce Fields wrote: > On Tue, Apr 14, 2020 at 09:19:31AM -0300, Jason Gunthorpe wrote: > > On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote: > > > On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote: > > > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote: > > > > > The commit ID is what automation should key off of. The short > > > > > description is only for human consumption. > > > > > > > > Right, so if the actual commit message isn't included so humans can > > > > read it then what was the point of including anything? > > > > > > Personally as a human reading commits in a terminal window I prefer the > > > abbreviated form. > > > > Frankly, I think they are useless, picking one of yours at random: > > > > Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... " > > > > And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so > > Ow, apologies. Looks like I rebased after writing that Fixes tag. > > I wonder if it's possible to make git warn.... > > Looks like a pre-rebase hook could check the branch being rebased for > "Fixes:" lines referencing commits on the rebased branch. I have some silly stuff to check patches before pushing them and it includes checking the fixes lines because they are very often wrong, both with wrong commit IDs and wrong subjects! linux-next now automates complaining about them, but perhaps not following the standard format defeats that.. Use 'git merge-base --is-ancestor fixes_id linus/master' to check them. > > now we are just totally lost, with a bad commit ID and a mangled > > subject line. > > For what it's worth, that part of the subject line is enough to find the > original commit (even to uniquely specify it). Lucky, but I wouldn't count on this as the general rule.. The point of the full subject line is to be informative to the reader and serve as a backup key in case the hash got mangled, as happens surprisingly often.. Jason
On Tue, Apr 14, 2020 at 12:20:16PM -0300, Jason Gunthorpe wrote: > On Tue, Apr 14, 2020 at 11:13:03AM -0400, J. Bruce Fields wrote: > > On Tue, Apr 14, 2020 at 09:19:31AM -0300, Jason Gunthorpe wrote: > > > On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote: > > > > On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote: > > > > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote: > > > > > > The commit ID is what automation should key off of. The short > > > > > > description is only for human consumption. > > > > > > > > > > Right, so if the actual commit message isn't included so humans can > > > > > read it then what was the point of including anything? > > > > > > > > Personally as a human reading commits in a terminal window I prefer the > > > > abbreviated form. > > > > > > Frankly, I think they are useless, picking one of yours at random: > > > > > > Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... " > > > > > > And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so > > > > Ow, apologies. Looks like I rebased after writing that Fixes tag. > > > > I wonder if it's possible to make git warn.... > > > > Looks like a pre-rebase hook could check the branch being rebased for > > "Fixes:" lines referencing commits on the rebased branch. > > I have some silly stuff to check patches before pushing them and it > includes checking the fixes lines because they are very often > wrong, both with wrong commit IDs and wrong subjects! I'd be interested in seeing it. > linux-next now automates complaining about them, but perhaps not > following the standard format defeats that.. It's managed before: https://lore.kernel.org/r/20190704074048.65556740@canb.auug.org.au though admittedly I was breaking the rule in a different way. I can't even be consistently rebellious. > Use 'git merge-base --is-ancestor fixes_id linus/master' to check > them. Oh, yeah, that's better than what I was trying to do, thanks. --b.
On Tue, Apr 14, 2020 at 12:17:44PM -0400, J. Bruce Fields wrote: > On Tue, Apr 14, 2020 at 12:20:16PM -0300, Jason Gunthorpe wrote: > > On Tue, Apr 14, 2020 at 11:13:03AM -0400, J. Bruce Fields wrote: > > > On Tue, Apr 14, 2020 at 09:19:31AM -0300, Jason Gunthorpe wrote: > > > > On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote: > > > > > On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote: > > > > > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote: > > > > > > > The commit ID is what automation should key off of. The short > > > > > > > description is only for human consumption. > > > > > > > > > > > > Right, so if the actual commit message isn't included so humans can > > > > > > read it then what was the point of including anything? > > > > > > > > > > Personally as a human reading commits in a terminal window I prefer the > > > > > abbreviated form. > > > > > > > > Frankly, I think they are useless, picking one of yours at random: > > > > > > > > Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... " > > > > > > > > And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so > > > > > > Ow, apologies. Looks like I rebased after writing that Fixes tag. > > > > > > I wonder if it's possible to make git warn.... > > > > > > Looks like a pre-rebase hook could check the branch being rebased for > > > "Fixes:" lines referencing commits on the rebased branch. > > > > I have some silly stuff to check patches before pushing them and it > > includes checking the fixes lines because they are very often > > wrong, both with wrong commit IDs and wrong subjects! > > I'd be interested in seeing it. checkpatch.pl checks it or supposed to check. commit a8dd86bf746256fbf68f82bc13356244c5ad8efa Author: Matteo Croce <mcroce@redhat.com> Date: Wed Sep 25 16:46:38 2019 -0700 checkpatch.pl: warn on invalid commit id It can happen that a commit message refers to an invalid commit id, because the referenced hash changed following a rebase, or simply by mistake. Add a check in checkpatch.pl which checks that an hash referenced by a Fixes tag, or just cited in the commit message, is a valid commit id. $ scripts/checkpatch.pl <<'EOF' Subject: [PATCH] test commit Sample test commit to test checkpatch.pl Commit 1da177e4c3f4 ("Linux-2.6.12-rc2") really exists, commit 0bba044c4ce7 ("tree") is valid but not a commit, while commit b4cc0b1c0cca ("unknown") is invalid. Fixes: f0cacc14cade ("unknown") Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") EOF WARNING: Unknown commit id '0bba044c4ce7', maybe rebased or not pulled? #8: commit 0bba044c4ce7 ("tree") is valid but not a commit, WARNING: Unknown commit id 'b4cc0b1c0cca', maybe rebased or not pulled? #9: while commit b4cc0b1c0cca ("unknown") is invalid. WARNING: Unknown commit id 'f0cacc14cade', maybe rebased or not pulled? #11: Fixes: f0cacc14cade ("unknown") total: 0 errors, 3 warnings, 4 lines checked Link: http://lkml.kernel.org/r/20190711001640.13398-1-mcroce@redhat.com Signed-off-by: Matteo Croce <mcroce@redhat.com> Cc: Joe Perches <joe@perches.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
On Tue, Apr 14, 2020 at 09:11:41PM +0300, Leon Romanovsky wrote: > On Tue, Apr 14, 2020 at 12:17:44PM -0400, J. Bruce Fields wrote: > > On Tue, Apr 14, 2020 at 12:20:16PM -0300, Jason Gunthorpe wrote: > > > On Tue, Apr 14, 2020 at 11:13:03AM -0400, J. Bruce Fields wrote: > > > > On Tue, Apr 14, 2020 at 09:19:31AM -0300, Jason Gunthorpe wrote: > > > > > On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote: > > > > > > On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote: > > > > > > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote: > > > > > > > > The commit ID is what automation should key off of. The short > > > > > > > > description is only for human consumption. > > > > > > > > > > > > > > Right, so if the actual commit message isn't included so humans can > > > > > > > read it then what was the point of including anything? > > > > > > > > > > > > Personally as a human reading commits in a terminal window I prefer the > > > > > > abbreviated form. > > > > > > > > > > Frankly, I think they are useless, picking one of yours at random: > > > > > > > > > > Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... " > > > > > > > > > > And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so > > > > > > > > Ow, apologies. Looks like I rebased after writing that Fixes tag. > > > > > > > > I wonder if it's possible to make git warn.... > > > > > > > > Looks like a pre-rebase hook could check the branch being rebased for > > > > "Fixes:" lines referencing commits on the rebased branch. > > > > > > I have some silly stuff to check patches before pushing them and it > > > includes checking the fixes lines because they are very often > > > wrong, both with wrong commit IDs and wrong subjects! > > > > I'd be interested in seeing it. > > checkpatch.pl checks it or supposed to check. This doesn't do the --is-ancestor check, so it is quite weak. It would not have triggered for Chuck because he'd still have the commit in his local git from the rebase. Jason
On Tue, Apr 14, 2020 at 12:17:44PM -0400, J. Bruce Fields wrote: > On Tue, Apr 14, 2020 at 12:20:16PM -0300, Jason Gunthorpe wrote: > > On Tue, Apr 14, 2020 at 11:13:03AM -0400, J. Bruce Fields wrote: > > > On Tue, Apr 14, 2020 at 09:19:31AM -0300, Jason Gunthorpe wrote: > > > > On Mon, Apr 13, 2020 at 03:29:07PM -0400, J. Bruce Fields wrote: > > > > > On Thu, Apr 09, 2020 at 02:47:50PM -0300, Jason Gunthorpe wrote: > > > > > > On Thu, Apr 09, 2020 at 10:33:32AM -0400, Chuck Lever wrote: > > > > > > > The commit ID is what automation should key off of. The short > > > > > > > description is only for human consumption. > > > > > > > > > > > > Right, so if the actual commit message isn't included so humans can > > > > > > read it then what was the point of including anything? > > > > > > > > > > Personally as a human reading commits in a terminal window I prefer the > > > > > abbreviated form. > > > > > > > > Frankly, I think they are useless, picking one of yours at random: > > > > > > > > Fixes: 4e48f1cccab3 "NFSD: allow inter server COPY to have... " > > > > > > > > And sadly the '4e48f1cccab3' commit doesn't appear in Linus's tree so > > > > > > Ow, apologies. Looks like I rebased after writing that Fixes tag. > > > > > > I wonder if it's possible to make git warn.... > > > > > > Looks like a pre-rebase hook could check the branch being rebased for > > > "Fixes:" lines referencing commits on the rebased branch. > > > > I have some silly stuff to check patches before pushing them and it > > includes checking the fixes lines because they are very often > > wrong, both with wrong commit IDs and wrong subjects! > > I'd be interested in seeing it. Sure, let me put my scripts on github.. Jason
diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h index 78fe2ac6dc6c..cbcfbd0521e3 100644 --- a/include/linux/sunrpc/svc_rdma.h +++ b/include/linux/sunrpc/svc_rdma.h @@ -170,6 +170,7 @@ extern bool svc_rdma_post_recvs(struct svcxprt_rdma *rdma); extern void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma, struct svc_rdma_recv_ctxt *ctxt); extern void svc_rdma_flush_recv_queues(struct svcxprt_rdma *rdma); +extern void svc_rdma_release_rqst(struct svc_rqst *rqstp); extern int svc_rdma_recvfrom(struct svc_rqst *); /* svc_rdma_rw.c */ diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c index 54469b72b25f..efa5fcb5793f 100644 --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c @@ -223,6 +223,26 @@ void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma, svc_rdma_recv_ctxt_destroy(rdma, ctxt); } +/** + * svc_rdma_release_rqst - Release transport-specific per-rqst resources + * @rqstp: svc_rqst being released + * + * Ensure that the recv_ctxt is released whether or not a Reply + * was sent. For example, the client could close the connection, + * or svc_process could drop an RPC, before the Reply is sent. + */ +void svc_rdma_release_rqst(struct svc_rqst *rqstp) +{ + struct svc_rdma_recv_ctxt *ctxt = rqstp->rq_xprt_ctxt; + struct svc_xprt *xprt = rqstp->rq_xprt; + struct svcxprt_rdma *rdma = + container_of(xprt, struct svcxprt_rdma, sc_xprt); + + rqstp->rq_xprt_ctxt = NULL; + if (ctxt) + svc_rdma_recv_ctxt_put(rdma, ctxt); +} + static int __svc_rdma_post_recv(struct svcxprt_rdma *rdma, struct svc_rdma_recv_ctxt *ctxt) { @@ -820,6 +840,8 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp) __be32 *p; int ret; + rqstp->rq_xprt_ctxt = NULL; + spin_lock(&rdma_xprt->sc_rq_dto_lock); ctxt = svc_rdma_next_recv_ctxt(&rdma_xprt->sc_read_complete_q); if (ctxt) { diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c index 6a87a2379e91..b6c8643867f2 100644 --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c @@ -926,12 +926,7 @@ int svc_rdma_sendto(struct svc_rqst *rqstp) ret = svc_rdma_send_reply_msg(rdma, sctxt, rctxt, rqstp); if (ret < 0) goto err1; - ret = 0; - -out: - rqstp->rq_xprt_ctxt = NULL; - svc_rdma_recv_ctxt_put(rdma, rctxt); - return ret; + return 0; err2: if (ret != -E2BIG && ret != -EINVAL) @@ -940,16 +935,14 @@ int svc_rdma_sendto(struct svc_rqst *rqstp) ret = svc_rdma_send_error_msg(rdma, sctxt, rqstp); if (ret < 0) goto err1; - ret = 0; - goto out; + return 0; err1: svc_rdma_send_ctxt_put(rdma, sctxt); err0: trace_svcrdma_send_failed(rqstp, ret); set_bit(XPT_CLOSE, &xprt->xpt_flags); - ret = -ENOTCONN; - goto out; + return -ENOTCONN; } /** diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c index 8bb99980ae85..ea54785db4f8 100644 --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c @@ -71,7 +71,6 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv, struct sockaddr *sa, int salen, int flags); static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt); -static void svc_rdma_release_rqst(struct svc_rqst *); static void svc_rdma_detach(struct svc_xprt *xprt); static void svc_rdma_free(struct svc_xprt *xprt); static int svc_rdma_has_wspace(struct svc_xprt *xprt); @@ -552,10 +551,6 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt) return NULL; } -static void svc_rdma_release_rqst(struct svc_rqst *rqstp) -{ -} - /* * When connected, an svc_xprt has at least two references: *
Utilize the xpo_release_rqst transport method to ensure that each rqstp's svc_rdma_recv_ctxt object is released even when the server cannot return a Reply for that rqstp. Without this fix, each RPC whose Reply cannot be sent leaks one svc_rdma_recv_ctxt. This is a 2.5KB structure, a 4KB DMA-mapped Receive buffer, and any pages that might be part of the Reply message. The leak is infrequent unless the network fabric is unreliable or Kerberos is in use, as GSS sequence window overruns, which result in connection loss, are more common on fast transports. Fixes: 3a88092ee319 ("svcrdma: Preserve Receive buffer until ... ") Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- include/linux/sunrpc/svc_rdma.h | 1 + net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 22 ++++++++++++++++++++++ net/sunrpc/xprtrdma/svc_rdma_sendto.c | 13 +++---------- net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 ----- 4 files changed, 26 insertions(+), 15 deletions(-)