| Message ID | 20230609174659.60327-5-sprasad@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] cifs: fix status checks in cifs_tree_connect | expand |
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)
On Fri, Jun 23, 2023 at 9:30 PM Tom Talpey <tom@talpey.com> 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@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. > 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)
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)); /* 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)
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(-)