On Wed, Apr 24, 2019 at 4:04 AM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
>
Thanks for the thoughtful review. Some comments below
> пн, 22 апр. 2019 г. в 21:40, Ronnie Sahlberg <lsahlber@xxxxxxxxxx>:
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
> > ---
> > fs/cifs/transport.c | 12 +++++++++---
> > 1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> > index 5c59c498f56a..3b80884ddf0f 100644
> > --- a/fs/cifs/transport.c
> > +++ b/fs/cifs/transport.c
> > @@ -971,7 +971,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> > const int flags, const int num_rqst, struct smb_rqst *rqst,
> > int *resp_buf_type, struct kvec *resp_iov)
> > {
> > - int i, j, optype, rc = 0;
> > + int i, j, optype, opmask, rc = 0;
> > struct mid_q_entry *midQ[MAX_COMPOUND];
> > bool cancelled_mid[MAX_COMPOUND] = {false};
> > struct cifs_credits credits[MAX_COMPOUND] = {
> > @@ -981,6 +981,7 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> > char *buf;
> >
> > optype = flags & CIFS_OP_MASK;
> > + opmask = flags & (CIFS_ASYNC_OP|CIFS_NO_RESP);
> >
> > for (i = 0; i < num_rqst; i++)
> > resp_buf_type[i] = CIFS_NO_BUFFER; /* no response buf yet */
> > @@ -1073,8 +1074,13 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> >
> > mutex_unlock(&ses->server->srv_mutex);
> >
> > - if (rc < 0) {
> > - /* Sending failed for some reason - return credits back */
> > + /*
> > + * If sending failed for some reason or it is an oplock break that we
> > + * will not receive a response to - return credits back
> > + */
> > + if (rc < 0 ||
> > + (optype == CIFS_OBREAK_OP &&
> > + opmask == (CIFS_ASYNC_OP|CIFS_NO_RESP))) {
> > for (i = 0; i < num_rqst; i++)
> > add_credits(ses->server, &credits[i], optype);
> > goto out;
> > --
> > 2.13.6
> >
> Thanks for looking into it. Let's me clarify the defines here because
> their names are not straightforward and require changes:
>
> #define CIFS_ASYNC_OP 2 /* do not wait for response */
>
> meaning that we don't need a response - probably there won't be a one
> (in SMB1 Oplock case). This is confusing because we also have
> cifs_call_async that does asynchronous processing of the responses.
CIFS_ASYNC_OP is overloaded.
In two places it means we will get a response eventually (in one of
which "eventually" might mean unbounded time) but don't wait for it.
In the third place it means that there is no response to this command.
>
> #define CIFS_NO_RESP 0x040 /* no response buffer required */
>
> meaning that a response buffer is not needed but the error code is.
>
> The problem with SMB1 oplocks was introduced when credits calculation
> was moved to the demultiplex thread. The server might not respond to
> such a request at all - that's why it is a special case.
>
> So, CIFS_NO_RESP flags should be handled like we do today - no changes
> are needed here. We wait for a response but just do not return a
> buffer to the caller. CIFS_ASYNC_OP handling has a bug that it missed
> to add credits back once we send such a request. Prior to credit
> movement to the demultiplex thread label "out" has the code to add
> credits back. So I think the only fix that is needed here is:
>
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 5c59c49..26e249d 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -1095,8 +1095,11 @@ compound_send_recv(const unsigned int xid,
> struct cifs_ses *ses,
> smb311_update_preauth_hash(ses, rqst[0].rq_iov,
> rqst[0].rq_nvec);
>
> - if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP)
> + if ((flags & CIFS_TIMEOUT_MASK) == CIFS_ASYNC_OP) {
> + for (i = 0; i < num_rqst; i++)
> + add_credits(ses->server, &credits[i], optype);
> goto out;
> + }
The reason I did it the way I did was that I wanted this change to
only affect the smb1 oplock breaks and leave all other commands and
behavior unchanged.
Maybe we need a new flag that means that there will never be a
response at all to this command so we must add all credits back
immediately once we have sent the pdu out on the wire.
We use CIFS_ASYNC_OP for three different cases.
When sending an oplock break, in which case we know we will never get
a response from the server (and thus we need to add the credits back
somehow which is not the demultiplex loop)
but also when we send an Echo request or when we send a Notify request
both cases where we WILL eventually receive a reply back from the
server and where we should add the credits back in the demultiplex
loop.
So if we add the credits back already here for all CIFS_ASYNC_OPs,
doesn't that mean that we could possibly add the credits back twice?
Once here and then once again in the demultiplex loop when we
get the response?
Since CIFS_ASYNC_OP suggests that it is an async operation but that we
will eventually get a reply (well that is how I parse the name) I feel
uncomfortable with that change since it conflates async operations
with
send-only-and-never-get-a-response operations.
Maybe we need a new flag to better describe the unique case for
requests we know will never get a reply ?
CIFS_NO_SERVER_RESPONSE or something?
What do you think?
>
> for (i = 0; i < num_rqst; i++) {
> rc = wait_for_response(ses->server, midQ[i]);
>
> It worked locally against Samba server for me.
>
> I would also suggest as a separate change to rename:
>
> CIFS_ASYNC_OP to CIFS_NO_WAIT, and
Not sure about that. I think "async operation" and "don't wait for
reply" are pretty equivalent so I don't see a need to rename here.
What is unique with smb1 oplock releases is not that they are async (I
could argue that they are not) but that they are requests we send that
will never have a response pdu from the server.
>
> CIFS_NO_RESP to CIFS_NO_RESP_BUF
I think this would be a good rename. But lets put it in a different
patch that only does a rename but does not change any logic.
>
> to reflect the exact meanings.
>
> --
> Best regards,
> Pavel Shilovsky
