On Mon, Jan 22, 2024 at 05:46:56PM -0500, Benjamin Coddington wrote:
> On 22 Jan 2024, at 17:44, [email protected] 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 <[email protected]>
> >>> ---
> >>> 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 <[email protected]>,Anna Schumaker <[email protected]>
> >>
> >> and copy [email protected]? You can also add my:
> >>
> >> Reviewed-by: Benjamin Coddington <[email protected]>
> >
> > 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?
--
Chuck Lever
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, [email protected] 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 <[email protected]>
>>>>> ---
>>>>> 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 <[email protected]>,Anna Schumaker <[email protected]>
>>>>
>>>> and copy [email protected]? You can also add my:
>>>>
>>>> Reviewed-by: Benjamin Coddington <[email protected]>
>>>
>>> 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, [email protected] 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 <[email protected]>
>>>>>> ---
>>>>>> 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 <[email protected]>,Anna Schumaker <[email protected]>
>>>>>
>>>>> and copy [email protected]? You can also add my:
>>>>>
>>>>> Reviewed-by: Benjamin Coddington <[email protected]>
>>>>
>>>> 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, [email protected] 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, [email protected] 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 <[email protected]>
>>>>>>> ---
>>>>>>> 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 <[email protected]>,Anna Schumaker <[email protected]>
>>>>>>
>>>>>> and copy [email protected]? You can also add my:
>>>>>>
>>>>>> Reviewed-by: Benjamin Coddington <[email protected]>
>>>>>
>>>>> 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