| Message ID | 20240122172353.2859254-1-samasth.norway.ananda@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] NFSv4.1: Assign retries to timeout.to_retries instead of timeout.to_initval | expand |
On 22 Jan 2024, at 12:23, Samasth Norway Ananda wrote: > In the else block we are assigning the req->rq_xprt->timeout->to_retries > value to timeout.to_initval, whereas it should have been assigned to > timeout.to_retries instead. > > Fixes: 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc timeouts for backchannel") > Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com> > --- > Hi, > > I came across the patch 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc > timeouts for backchannel") which assigns value to same variable in the > else block. Can I please get your input on the patch? Oh yes, this a good fix. Usually the maintainers won't pick up a patch that's only sent to the list, rather the patch should be addressed to them directly and copied to the list. Can you re-send this patch to: Trond Myklebust <trond.myklebust@hammerspace.com>,Anna Schumaker <anna@kernel.org> and copy linux-nfs@vger.kernel.org? You can also add my: Reviewed-by: Benjamin Coddington <bcodding@redhat.com> Ben
On 1/22/24 2:41 PM, Benjamin Coddington wrote: > On 22 Jan 2024, at 12:23, Samasth Norway Ananda wrote: > >> In the else block we are assigning the req->rq_xprt->timeout->to_retries >> value to timeout.to_initval, whereas it should have been assigned to >> timeout.to_retries instead. >> >> Fixes: 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc timeouts for backchannel") >> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com> >> --- >> Hi, >> >> I came across the patch 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc >> timeouts for backchannel") which assigns value to same variable in the >> else block. Can I please get your input on the patch? > > Oh yes, this a good fix. Usually the maintainers won't pick up a patch > that's only sent to the list, rather the patch should be addressed to them > directly and copied to the list. Can you re-send this patch to: > > Trond Myklebust <trond.myklebust@hammerspace.com>,Anna Schumaker <anna@kernel.org> > > and copy linux-nfs@vger.kernel.org? You can also add my: > > Reviewed-by: Benjamin Coddington <bcodding@redhat.com> Sure, I will do that. Thanks for the review. Samasth. > > Ben >
On 22 Jan 2024, at 17:44, samasth.norway.ananda@oracle.com wrote: > On 1/22/24 2:41 PM, Benjamin Coddington wrote: >> On 22 Jan 2024, at 12:23, Samasth Norway Ananda wrote: >> >>> In the else block we are assigning the req->rq_xprt->timeout->to_retries >>> value to timeout.to_initval, whereas it should have been assigned to >>> timeout.to_retries instead. >>> >>> Fixes: 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc timeouts for backchannel") >>> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com> >>> --- >>> Hi, >>> >>> I came across the patch 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc >>> timeouts for backchannel") which assigns value to same variable in the >>> else block. Can I please get your input on the patch? >> >> Oh yes, this a good fix. Usually the maintainers won't pick up a patch >> that's only sent to the list, rather the patch should be addressed to them >> directly and copied to the list. Can you re-send this patch to: >> >> Trond Myklebust <trond.myklebust@hammerspace.com>,Anna Schumaker <anna@kernel.org> >> >> and copy linux-nfs@vger.kernel.org? You can also add my: >> >> Reviewed-by: Benjamin Coddington <bcodding@redhat.com> > > Sure, I will do that. Thanks for the review. Can you also fixup the block above the hunk you posted? Its backwards there too!
On Mon, Jan 22, 2024 at 05:46:56PM -0500, Benjamin Coddington wrote: > On 22 Jan 2024, at 17:44, samasth.norway.ananda@oracle.com wrote: > > > On 1/22/24 2:41 PM, Benjamin Coddington wrote: > >> On 22 Jan 2024, at 12:23, Samasth Norway Ananda wrote: > >> > >>> In the else block we are assigning the req->rq_xprt->timeout->to_retries > >>> value to timeout.to_initval, whereas it should have been assigned to > >>> timeout.to_retries instead. > >>> > >>> Fixes: 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc timeouts for backchannel") > >>> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com> > >>> --- > >>> Hi, > >>> > >>> I came across the patch 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc > >>> timeouts for backchannel") which assigns value to same variable in the > >>> else block. Can I please get your input on the patch? > >> > >> Oh yes, this a good fix. Usually the maintainers won't pick up a patch > >> that's only sent to the list, rather the patch should be addressed to them > >> directly and copied to the list. Can you re-send this patch to: > >> > >> Trond Myklebust <trond.myklebust@hammerspace.com>,Anna Schumaker <anna@kernel.org> > >> > >> and copy linux-nfs@vger.kernel.org? You can also add my: > >> > >> Reviewed-by: Benjamin Coddington <bcodding@redhat.com> > > > > Sure, I will do that. Thanks for the review. > > Can you also fixup the block above the hunk you posted? Its backwards there too! It's backwards in a different way. And should you set to_maxval in both places as well?
On 22 Jan 2024, at 17:53, Chuck Lever wrote: > On Mon, Jan 22, 2024 at 05:46:56PM -0500, Benjamin Coddington wrote: >> On 22 Jan 2024, at 17:44, samasth.norway.ananda@oracle.com wrote: >> >>> On 1/22/24 2:41 PM, Benjamin Coddington wrote: >>>> On 22 Jan 2024, at 12:23, Samasth Norway Ananda wrote: >>>> >>>>> In the else block we are assigning the req->rq_xprt->timeout->to_retries >>>>> value to timeout.to_initval, whereas it should have been assigned to >>>>> timeout.to_retries instead. >>>>> >>>>> Fixes: 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc timeouts for backchannel") >>>>> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com> >>>>> --- >>>>> Hi, >>>>> >>>>> I came across the patch 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc >>>>> timeouts for backchannel") which assigns value to same variable in the >>>>> else block. Can I please get your input on the patch? >>>> >>>> Oh yes, this a good fix. Usually the maintainers won't pick up a patch >>>> that's only sent to the list, rather the patch should be addressed to them >>>> directly and copied to the list. Can you re-send this patch to: >>>> >>>> Trond Myklebust <trond.myklebust@hammerspace.com>,Anna Schumaker <anna@kernel.org> >>>> >>>> and copy linux-nfs@vger.kernel.org? You can also add my: >>>> >>>> Reviewed-by: Benjamin Coddington <bcodding@redhat.com> >>> >>> Sure, I will do that. Thanks for the review. >> >> Can you also fixup the block above the hunk you posted? Its backwards there too! > > It's backwards in a different way. Its an artifact of my text editing.. > And should you set to_maxval in both places as well? IIRC it does not need to be set there. Ben
On 1/22/24 3:03 PM, Benjamin Coddington wrote: > On 22 Jan 2024, at 17:53, Chuck Lever wrote: > >> On Mon, Jan 22, 2024 at 05:46:56PM -0500, Benjamin Coddington wrote: >>> On 22 Jan 2024, at 17:44, samasth.norway.ananda@oracle.com wrote: >>> >>>> On 1/22/24 2:41 PM, Benjamin Coddington wrote: >>>>> On 22 Jan 2024, at 12:23, Samasth Norway Ananda wrote: >>>>> >>>>>> In the else block we are assigning the req->rq_xprt->timeout->to_retries >>>>>> value to timeout.to_initval, whereas it should have been assigned to >>>>>> timeout.to_retries instead. >>>>>> >>>>>> Fixes: 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc timeouts for backchannel") >>>>>> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com> >>>>>> --- >>>>>> Hi, >>>>>> >>>>>> I came across the patch 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc >>>>>> timeouts for backchannel") which assigns value to same variable in the >>>>>> else block. Can I please get your input on the patch? >>>>> >>>>> Oh yes, this a good fix. Usually the maintainers won't pick up a patch >>>>> that's only sent to the list, rather the patch should be addressed to them >>>>> directly and copied to the list. Can you re-send this patch to: >>>>> >>>>> Trond Myklebust <trond.myklebust@hammerspace.com>,Anna Schumaker <anna@kernel.org> >>>>> >>>>> and copy linux-nfs@vger.kernel.org? You can also add my: >>>>> >>>>> Reviewed-by: Benjamin Coddington <bcodding@redhat.com> >>>> >>>> Sure, I will do that. Thanks for the review. >>> >>> Can you also fixup the block above the hunk you posted? Its backwards there too! >> >> It's backwards in a different way. > > Its an artifact of my text editing.. > >> And should you set to_maxval in both places as well? > > IIRC it does not need to be set there. Ah okay. So it should be like this right? if (rqstp->bc_to_initval > 0) { timeout.to_initval = rqstp->bc_to_initval; timeout.to_retries = rqstp->bc_to_retries; } else { timeout.to_initval = req->rq_xprt->timeout->to_initval; timeout.to_retries = req->rq_xprt->timeout->to_retries; } Thanks, Samasth. > > Ben >
On 22 Jan 2024, at 18:07, samasth.norway.ananda@oracle.com wrote: > On 1/22/24 3:03 PM, Benjamin Coddington wrote: >> On 22 Jan 2024, at 17:53, Chuck Lever wrote: >> >>> On Mon, Jan 22, 2024 at 05:46:56PM -0500, Benjamin Coddington wrote: >>>> On 22 Jan 2024, at 17:44, samasth.norway.ananda@oracle.com wrote: >>>> >>>>> On 1/22/24 2:41 PM, Benjamin Coddington wrote: >>>>>> On 22 Jan 2024, at 12:23, Samasth Norway Ananda wrote: >>>>>> >>>>>>> In the else block we are assigning the req->rq_xprt->timeout->to_retries >>>>>>> value to timeout.to_initval, whereas it should have been assigned to >>>>>>> timeout.to_retries instead. >>>>>>> >>>>>>> Fixes: 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc timeouts for backchannel") >>>>>>> Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com> >>>>>>> --- >>>>>>> Hi, >>>>>>> >>>>>>> I came across the patch 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc >>>>>>> timeouts for backchannel") which assigns value to same variable in the >>>>>>> else block. Can I please get your input on the patch? >>>>>> >>>>>> Oh yes, this a good fix. Usually the maintainers won't pick up a patch >>>>>> that's only sent to the list, rather the patch should be addressed to them >>>>>> directly and copied to the list. Can you re-send this patch to: >>>>>> >>>>>> Trond Myklebust <trond.myklebust@hammerspace.com>,Anna Schumaker <anna@kernel.org> >>>>>> >>>>>> and copy linux-nfs@vger.kernel.org? You can also add my: >>>>>> >>>>>> Reviewed-by: Benjamin Coddington <bcodding@redhat.com> >>>>> >>>>> Sure, I will do that. Thanks for the review. >>>> >>>> Can you also fixup the block above the hunk you posted? Its backwards there too! >>> >>> It's backwards in a different way. >> >> Its an artifact of my text editing.. >> >>> And should you set to_maxval in both places as well? >> >> IIRC it does not need to be set there. > > Ah okay. So it should be like this right? Yes! Good catch. Thank you! Ben
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c index f60c93e5a25d..295266a50244 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -1601,7 +1601,7 @@ void svc_process_bc(struct rpc_rqst *req, struct svc_rqst *rqstp) timeout.to_retries = rqstp->bc_to_initval; } else { timeout.to_initval = req->rq_xprt->timeout->to_initval; - timeout.to_initval = req->rq_xprt->timeout->to_retries; + timeout.to_retries = req->rq_xprt->timeout->to_retries; } memcpy(&req->rq_snd_buf, &rqstp->rq_res, sizeof(req->rq_snd_buf)); task = rpc_run_bc_task(req, &timeout);
In the else block we are assigning the req->rq_xprt->timeout->to_retries value to timeout.to_initval, whereas it should have been assigned to timeout.to_retries instead. Fixes: 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc timeouts for backchannel") Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com> --- Hi, I came across the patch 57331a59ac0d (“NFSv4.1: Use the nfs_client's rpc timeouts for backchannel") which assigns value to same variable in the else block. Can I please get your input on the patch? Thank you. --- net/sunrpc/svc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)