[PATCH] CIFS: Wait for credits if at least one request is in flight

[PATCH] CIFS: Wait for credits if at least one request is in flight

[Pavel Shilovsky]

Currently we try to guess if a compound request is going to succeed
waiting for credits or not based on the number of requests in flight.
This approach doesn't work correctly all the time because there may
be only one request in flight which is going to bring multiple credits
satisfying the compound request.

Change the behavior to fail a request only if there are no requests
in flight at all and proceed waiting for credits otherwise.

Cc: <stable@vger.kernel.org> # 5.1+
Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
---
 fs/cifs/transport.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 4ffbf8f965814..84f33fdd1f4e0 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -659,10 +659,10 @@ wait_for_compound_request(struct TCP_Server_Info *server, int num,
 	spin_lock(&server->req_lock);
 	if (*credits < num) {
 		/*
-		 * Return immediately if not too many requests in flight since
-		 * we will likely be stuck on waiting for credits.
+		 * Return immediately if no requests in flight since
+		 * we will be stuck on waiting for credits.
 		 */
-		if (server->in_flight < num - *credits) {
+		if (server->in_flight == 0) {
 			spin_unlock(&server->req_lock);
 			return -ENOTSUPP;
 		}
-- 
2.25.1

Re: [PATCH] CIFS: Wait for credits if at least one request is in flight

[Tom Talpey]

It's reasonable to fail a request that can never have sufficient
credits to send, but EOPNOTSUPP is a really strange error to return.
The operation might work if the payload were smaller, right? So,
would a resource error such as EDEADLK be more meaningful, and allow
the caller to recover, even?

Also, can you elaborate on why this is only triggered when no
requests at all are in flight? Or is this some kind of corner
case for requests that need every credit the server currently
is offering?

Tom.

On 2/1/2021 8:01 PM, Pavel Shilovsky wrote:
> Currently we try to guess if a compound request is going to succeed
> waiting for credits or not based on the number of requests in flight.
> This approach doesn't work correctly all the time because there may
> be only one request in flight which is going to bring multiple credits
> satisfying the compound request.
> 
> Change the behavior to fail a request only if there are no requests
> in flight at all and proceed waiting for credits otherwise.
> 
> Cc: <stable@vger.kernel.org> # 5.1+
> Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
> ---
>   fs/cifs/transport.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 4ffbf8f965814..84f33fdd1f4e0 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -659,10 +659,10 @@ wait_for_compound_request(struct TCP_Server_Info *server, int num,
>   	spin_lock(&server->req_lock);
>   	if (*credits < num) {
>   		/*
> -		 * Return immediately if not too many requests in flight since
> -		 * we will likely be stuck on waiting for credits.
> +		 * Return immediately if no requests in flight since
> +		 * we will be stuck on waiting for credits.
>   		 */
> -		if (server->in_flight < num - *credits) {
> +		if (server->in_flight == 0) {
>   			spin_unlock(&server->req_lock);
>   			return -ENOTSUPP;
>   		}
> 

Re: [PATCH] CIFS: Wait for credits if at least one request is in flight

[Pavel Shilovsky]

I agree that the error code may not be ideal. Shyam has a WIPto
replace it with EBUSY but EDEADLK may be a good alternative too. For
this patch I would prefer not to change the error code and only fix
the bug to allow less ricky backporting.

Yes. If the server is tight on resources or just gives us less credits
for other reasons (e.g. requests are coming out of order and the
server delays granting more credits until it receives a missing mid)
and we exhausted most available credits there may be situations when
we try to send a compound request but we don't have enough credits. At
this point the client needs to decide if it should wait for credits or
fail a request. If at least one request is in flight there is a high
probability that the server returns enough credits to satisfy the
compound request (which are usually 3-4 credits long). So, we don't
want to fail the request in this case.

--
Best regards,
Pavel Shilovsky

пн, 1 февр. 2021 г. в 19:39, Tom Talpey <tom@talpey.com>:
>
> It's reasonable to fail a request that can never have sufficient
> credits to send, but EOPNOTSUPP is a really strange error to return.
> The operation might work if the payload were smaller, right? So,
> would a resource error such as EDEADLK be more meaningful, and allow
> the caller to recover, even?
>
> Also, can you elaborate on why this is only triggered when no
> requests at all are in flight? Or is this some kind of corner
> case for requests that need every credit the server currently
> is offering?
>
> Tom.
>
> On 2/1/2021 8:01 PM, Pavel Shilovsky wrote:
> > Currently we try to guess if a compound request is going to succeed
> > waiting for credits or not based on the number of requests in flight.
> > This approach doesn't work correctly all the time because there may
> > be only one request in flight which is going to bring multiple credits
> > satisfying the compound request.
> >
> > Change the behavior to fail a request only if there are no requests
> > in flight at all and proceed waiting for credits otherwise.
> >
> > Cc: <stable@vger.kernel.org> # 5.1+
> > Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
> > ---
> >   fs/cifs/transport.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index 4ffbf8f965814..84f33fdd1f4e0 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -659,10 +659,10 @@ wait_for_compound_request(struct TCP_Server_Info *server, int num,
> >       spin_lock(&server->req_lock);
> >       if (*credits < num) {
> >               /*
> > -              * Return immediately if not too many requests in flight since
> > -              * we will likely be stuck on waiting for credits.
> > +              * Return immediately if no requests in flight since
> > +              * we will be stuck on waiting for credits.
> >                */
> > -             if (server->in_flight < num - *credits) {
> > +             if (server->in_flight == 0) {
> >                       spin_unlock(&server->req_lock);
> >                       return -ENOTSUPP;
> >               }
> >

Re: [PATCH] CIFS: Wait for credits if at least one request is in flight

[Tom Talpey]

On 2/2/2021 1:17 PM, Pavel Shilovsky wrote:
> I agree that the error code may not be ideal. Shyam has a WIPto
> replace it with EBUSY but EDEADLK may be a good alternative too. For
> this patch I would prefer not to change the error code and only fix
> the bug to allow less ricky backporting.

Sounds good to consider separately.

> Yes. If the server is tight on resources or just gives us less credits
> for other reasons (e.g. requests are coming out of order and the
> server delays granting more credits until it receives a missing mid)
> and we exhausted most available credits there may be situations when
> we try to send a compound request but we don't have enough credits. At
> this point the client needs to decide if it should wait for credits or
> fail a request. If at least one request is in flight there is a high
> probability that the server returns enough credits to satisfy the
> compound request (which are usually 3-4 credits long). So, we don't
> want to fail the request in this case.

Ah, yes it's true that with no requests in flight, the wait would be
unbounded, and uncertain to gain credits even then. I think that is
well worth capturing in a code comment where the failure is returned.

It's still a concern that large requests may fall victim to being
locked out by small ones, in the low-credit case. A "scheduler", or
at least a credit reservation, would seem important in future. Or,
as mentioned, modifying the requests to consume fewer credits each.

It's easy to envision a situation where a low-credit server can be
up, but a credit-greedy client might refuse to send it any requests
at all.

Tom.

> --
> Best regards,
> Pavel Shilovsky
> 
> пн, 1 февр. 2021 г. в 19:39, Tom Talpey <tom@talpey.com>:
>>
>> It's reasonable to fail a request that can never have sufficient
>> credits to send, but EOPNOTSUPP is a really strange error to return.
>> The operation might work if the payload were smaller, right? So,
>> would a resource error such as EDEADLK be more meaningful, and allow
>> the caller to recover, even?
>>
>> Also, can you elaborate on why this is only triggered when no
>> requests at all are in flight? Or is this some kind of corner
>> case for requests that need every credit the server currently
>> is offering?
>>
>> Tom.
>>
>> On 2/1/2021 8:01 PM, Pavel Shilovsky wrote:
>>> Currently we try to guess if a compound request is going to succeed
>>> waiting for credits or not based on the number of requests in flight.
>>> This approach doesn't work correctly all the time because there may
>>> be only one request in flight which is going to bring multiple credits
>>> satisfying the compound request.
>>>
>>> Change the behavior to fail a request only if there are no requests
>>> in flight at all and proceed waiting for credits otherwise.
>>>
>>> Cc: <stable@vger.kernel.org> # 5.1+
>>> Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com>
>>> ---
>>>    fs/cifs/transport.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>>> index 4ffbf8f965814..84f33fdd1f4e0 100644
>>> --- a/fs/cifs/transport.c
>>> +++ b/fs/cifs/transport.c
>>> @@ -659,10 +659,10 @@ wait_for_compound_request(struct TCP_Server_Info *server, int num,
>>>        spin_lock(&server->req_lock);
>>>        if (*credits < num) {
>>>                /*
>>> -              * Return immediately if not too many requests in flight since
>>> -              * we will likely be stuck on waiting for credits.
>>> +              * Return immediately if no requests in flight since
>>> +              * we will be stuck on waiting for credits.
>>>                 */
>>> -             if (server->in_flight < num - *credits) {
>>> +             if (server->in_flight == 0) {
>>>                        spin_unlock(&server->req_lock);
>>>                        return -ENOTSUPP;
>>>                }
>>>
> 

Re: [PATCH] CIFS: Wait for credits if at least one request is in flight

[Pavel Shilovsky]

вт, 2 февр. 2021 г. в 11:05, Tom Talpey <tom@talpey.com>:
>
> > Yes. If the server is tight on resources or just gives us less credits
> > for other reasons (e.g. requests are coming out of order and the
> > server delays granting more credits until it receives a missing mid)
> > and we exhausted most available credits there may be situations when
> > we try to send a compound request but we don't have enough credits. At
> > this point the client needs to decide if it should wait for credits or
> > fail a request. If at least one request is in flight there is a high
> > probability that the server returns enough credits to satisfy the
> > compound request (which are usually 3-4 credits long). So, we don't
> > want to fail the request in this case.
>
> Ah, yes it's true that with no requests in flight, the wait would be
> unbounded, and uncertain to gain credits even then. I think that is
> well worth capturing in a code comment where the failure is returned.

Make sense. Will add a comment and re-send the patch.

>
> It's still a concern that large requests may fall victim to being
> locked out by small ones, in the low-credit case. A "scheduler", or
> at least a credit reservation, would seem important in future. Or,
> as mentioned, modifying the requests to consume fewer credits each.
>
> It's easy to envision a situation where a low-credit server can be
> up, but a credit-greedy client might refuse to send it any requests
> at all.
>

Agree. Falling back to sequentially sending compound components is
needed to resolve such a situation.

--
Best regards,
Pavel Shilovsky