On Fri, Jun 23, 2023 at 9:30 PM Tom Talpey <tom@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxx>
> > ---
> > 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.
>
Thanks for the review, Tom.
I'll prepare an updated patch.
>
> > /* 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)
--
Regards,
Shyam
