On Thu, Apr 25, 2019 at 5:28 AM Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
>
> вт, 23 апр. 2019 г. в 17:45, ronnie sahlberg <ronniesahlberg@xxxxxxxxx>:
> >
> > 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.
>
> Agree, we should define another one.
I added a new flag for this in the respin I sent.
There are more things we need to do but that should wait until the
next merge window I think.
>
> >
> > >
> > > #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.
>
> Only SMB1 oplocks uses compound_send_recv() to send CIFS_ASYNC_OP.
> Notify uses SendReceive() but Notify itself is not being called
> anywhere. Echo uses cifs_call_async which is a completely different
> code path.
This is all for a separate patch for the next merge window, but:
We can remove Notify() since it is not used.
Since this is the only callsite for SendReceive() that uses CIFS_ASYNC_OP
we can then also remove the check for it in SendReceive()
With the new flag I added we no longer need to check for CIFS_ASYNC_OP
in compound_send_recv()
either so I removed it in the patch.
This leaves CIFS_ASYNC_OP used in only two places and the only thing
it is used for is to indicate that
we do not want to block in wait_for_credits().
Thus possibly we should rename this to CIFS_NON_BLOCK.
The semantics then become :
* CIFS_NO_SRV_RSP : indicating that we will never get a response.
I.e. smb1 oplock break.
* cifs_call_async() : this is what is used to request async processing
of the reply.
* CIFS_NON_BLOCK : in both/all cases used to indicate we don't want to
block in wait_for_credit()
and we might also rename CIFS_NO_RSP to CIFS_NO_SRV_RSP to make it
clearer what this flag indicates.
>
> > 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?
>
> You are right. In most cases we will manage to delete the mid going to
> "out" label but it is still possible that the server will respond too
> fast for SMB1 oplock and the demultiplex thread will add the credits
> twice. We need a special no-op callback to set for such mid in the
> same loop I propose, so we do not add credits twice.
>
> > 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.
>
> Agree. CIFS_ASYNC_OP is being parsed for me as "return immediately
> after sending a request and handle a response asynchronously". We
> should use this one only in cifs_call_async(), I guess.
>
> >
> > 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?
>
> Let's do CIFS_NO_SRV_RSP to make it shorter - it will be set by SMB1
> oplock only and compound_send_recv won't be handle CIFS_ASYNC_OP
> operations at all truly reflecting its name - we have cifs_call_async
> for those purposes.
>
> > 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.
>
> Ok. Another option is to let cifs_call_async know about the new
> CIFS_NO_SRV_RSP flag and pass a combination of
> CIFS_ASYNC_OP | CIFS_NO_SRV_RSP to the function that will just send a
> request and then delete a mid. No-op callback is still needed there.
>
> >
> > >
> > > 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.
> >
>
> +1 for a separate patch. Probably RSP instead of RESP, but don't have
> strong preferences here.
>
> --
> Best regards,
> Pavel Shilovsky
