Re: [PATCH 5/6] cifs: fix max_credits implementation

[Tom Talpey <tom@talpey.com>]

On 6/9/2023 1:46 PM, Shyam Prasad N wrote:
> The current implementation of max_credits on the client does
> not work because the CreditRequest logic for several commands
> does not take max_credits into account.
> 
> Still, we can end up asking the server for more credits, depending
> on the number of credits in flight. For this, we need to
> limit the credits while parsing the responses too.
> 
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>   fs/smb/client/smb2ops.c |  2 ++
>   fs/smb/client/smb2pdu.c | 32 ++++++++++++++++++++++++++++----
>   2 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 43162915e03c..18faf267c54d 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -34,6 +34,8 @@ static int
>   change_conf(struct TCP_Server_Info *server)
>   {
>   	server->credits += server->echo_credits + server->oplock_credits;
> +	if (server->credits > server->max_credits)
> +		server->credits = server->max_credits;
>   	server->oplock_credits = server->echo_credits = 0;
>   	switch (server->credits) {
>   	case 0:
> diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
> index 7063b395d22f..17fe212ab895 100644
> --- a/fs/smb/client/smb2pdu.c
> +++ b/fs/smb/client/smb2pdu.c
> @@ -1305,7 +1305,12 @@ SMB2_sess_alloc_buffer(struct SMB2_sess_data *sess_data)
>   	}
>   
>   	/* enough to enable echos and oplocks and one max size write */
> -	req->hdr.CreditRequest = cpu_to_le16(130);
> +	if (server->credits >= server->max_credits)
> +		req->hdr.CreditRequest = cpu_to_le16(0);
> +	else
> +		req->hdr.CreditRequest = cpu_to_le16(
> +			min_t(int, server->max_credits -
> +			      server->credits, 130));

This identical processing appears several times below. It would be
very bad if the copies got out of sync. Let's factor these as a
single function, perhaps inline but it might make sense as a client
common entry.

Tom.


>   	/* only one of SMB2 signing flags may be set in SMB2 request */
>   	if (server->sign)
> @@ -1899,7 +1904,12 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
>   	rqst.rq_nvec = 2;
>   
>   	/* Need 64 for max size write so ask for more in case not there yet */
> -	req->hdr.CreditRequest = cpu_to_le16(64);
> +	if (server->credits >= server->max_credits)
> +		req->hdr.CreditRequest = cpu_to_le16(0);
> +	else
> +		req->hdr.CreditRequest = cpu_to_le16(
> +			min_t(int, server->max_credits -
> +			      server->credits, 64));
>   
>   	rc = cifs_send_recv(xid, ses, server,
>   			    &rqst, &resp_buftype, flags, &rsp_iov);
> @@ -4227,6 +4237,7 @@ smb2_async_readv(struct cifs_readdata *rdata)
>   	struct TCP_Server_Info *server;
>   	struct cifs_tcon *tcon = tlink_tcon(rdata->cfile->tlink);
>   	unsigned int total_len;
> +	int credit_request;
>   
>   	cifs_dbg(FYI, "%s: offset=%llu bytes=%u\n",
>   		 __func__, rdata->offset, rdata->bytes);
> @@ -4258,7 +4269,13 @@ smb2_async_readv(struct cifs_readdata *rdata)
>   	if (rdata->credits.value > 0) {
>   		shdr->CreditCharge = cpu_to_le16(DIV_ROUND_UP(rdata->bytes,
>   						SMB2_MAX_BUFFER_SIZE));
> -		shdr->CreditRequest = cpu_to_le16(le16_to_cpu(shdr->CreditCharge) + 8);
> +		credit_request = le16_to_cpu(shdr->CreditCharge) + 8;
> +		if (server->credits >= server->max_credits)
> +			shdr->CreditRequest = cpu_to_le16(0);
> +		else
> +			shdr->CreditRequest = cpu_to_le16(
> +				min_t(int, server->max_credits -
> +						server->credits, credit_request));
>   
>   		rc = adjust_credits(server, &rdata->credits, rdata->bytes);
>   		if (rc)
> @@ -4468,6 +4485,7 @@ smb2_async_writev(struct cifs_writedata *wdata,
>   	unsigned int total_len;
>   	struct cifs_io_parms _io_parms;
>   	struct cifs_io_parms *io_parms = NULL;
> +	int credit_request;
>   
>   	if (!wdata->server)
>   		server = wdata->server = cifs_pick_channel(tcon->ses);
> @@ -4572,7 +4590,13 @@ smb2_async_writev(struct cifs_writedata *wdata,
>   	if (wdata->credits.value > 0) {
>   		shdr->CreditCharge = cpu_to_le16(DIV_ROUND_UP(wdata->bytes,
>   						    SMB2_MAX_BUFFER_SIZE));
> -		shdr->CreditRequest = cpu_to_le16(le16_to_cpu(shdr->CreditCharge) + 8);
> +		credit_request = le16_to_cpu(shdr->CreditCharge) + 8;
> +		if (server->credits >= server->max_credits)
> +			shdr->CreditRequest = cpu_to_le16(0);
> +		else
> +			shdr->CreditRequest = cpu_to_le16(
> +				min_t(int, server->max_credits -
> +						server->credits, credit_request));
>   
>   		rc = adjust_credits(server, &wdata->credits, io_parms->length);
>   		if (rc)