2012/6/6 Jeff Layton <jlayton@xxxxxxxxxxxxxxx>:
> On Thu, 31 May 2012 16:50:54 +0400
> Pavel Shilovsky <pshilovsky@xxxxxxxxx> wrote:
>
>> Split all requests to echos, oplocks and others - each group uses
>> its own credit slot. This change is required to support SMB2 code.
>>
>> As for CIFS - we don't use this approach and the logic isn't changed.
>>
>> Signed-off-by: Pavel Shilovsky <pshilovsky@xxxxxxxxx>
>> ---
>> fs/cifs/cifsglob.h | 14 ++++++++++----
>> fs/cifs/cifsproto.h | 2 +-
>> fs/cifs/cifssmb.c | 12 ++++++------
>> fs/cifs/smb1ops.c | 12 ++++++++++--
>> fs/cifs/transport.c | 34 +++++++++++++++++++---------------
>> 5 files changed, 46 insertions(+), 28 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index 2aac4e5..560b011 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -171,9 +171,11 @@ struct smb_version_operations {
>> /* check response: verify signature, map error */
>> int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *,
>> bool);
>> - void (*add_credits)(struct TCP_Server_Info *, const unsigned int);
>> + void (*add_credits)(struct TCP_Server_Info *, const unsigned int,
>> + const int);
>> void (*set_credits)(struct TCP_Server_Info *, const int);
>> - int * (*get_credits_field)(struct TCP_Server_Info *);
>> + int * (*get_credits_field)(struct TCP_Server_Info *, const int);
>> + unsigned int (*get_credits)(struct mid_q_entry *);
>> __u64 (*get_next_mid)(struct TCP_Server_Info *);
>> /* data offset from read response message */
>> unsigned int (*read_data_offset)(char *);
>> @@ -392,9 +394,10 @@ has_credits(struct TCP_Server_Info *server, int *credits)
>> }
>>
>> static inline void
>> -add_credits(struct TCP_Server_Info *server, const unsigned int add)
>> +add_credits(struct TCP_Server_Info *server, const unsigned int add,
>> + const int optype)
>> {
>> - server->ops->add_credits(server, add);
>> + server->ops->add_credits(server, add, optype);
>> }
>>
>> static inline void
>> @@ -956,6 +959,9 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
>> #define CIFS_LOG_ERROR 0x010 /* log NT STATUS if non-zero */
>> #define CIFS_LARGE_BUF_OP 0x020 /* large request buffer */
>> #define CIFS_NO_RESP 0x040 /* no response buffer required */
>> +#define CIFS_ECHO_OP 0x080 /* echo request */
>> +#define CIFS_OBREAK_OP 0x0100 /* oplock break request */
>> +#define CIFS_REQ_MASK 0x0180 /* mask request type */
>>
>
>
> Ok, now that I went back and read the patch description, I think I
> understand why you're doing it this way. Still, it's generally good
> practice to add flags only when you're going to use them, or to explain
> why you're adding them without setting them anywhere.
Ok, it seems I need to add an extra description to the commit message.
>
>> /* Security Flags: indicate type of session setup needed */
>> #define CIFSSEC_MAY_SIGN 0x00001
>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> index 0a6cbfe..41d5a76 100644
>> --- a/fs/cifs/cifsproto.h
>> +++ b/fs/cifs/cifsproto.h
>> @@ -71,7 +71,7 @@ extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
>> extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>> unsigned int nvec, mid_receive_t *receive,
>> mid_callback_t *callback, void *cbdata,
>> - bool ignore_pend);
>> + const int optype);
>> extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *,
>> struct smb_hdr * /* input */ ,
>> struct smb_hdr * /* out */ ,
>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>> index 5b40073..65898cc 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -720,7 +720,7 @@ cifs_echo_callback(struct mid_q_entry *mid)
>> struct TCP_Server_Info *server = mid->callback_data;
>>
>> DeleteMidQEntry(mid);
>> - add_credits(server, 1);
>> + add_credits(server, 1, 0);
>> }
>>
>> int
>> @@ -747,7 +747,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
>> iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
>>
>> rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback,
>> - server, true);
>> + server, CIFS_ASYNC_OP);
>
> Even if the SMB1 operation doesn't use this, it's probably a good idea
> to set the ECHO flag here. If you ever change the behavior of the other
> operations, you might avoid a bug that way.
>
> Probably ditto for the oplock break operation.
We need to set CIFS_ASYNC_OP flag here because it indicates that we
can't wait until credits come (according to the echo behavior in CIFS
that we discussed earlier). So, setting ECHO flag here means that we
need to consume credit from 'echo' slot - that doesn't match the
reality. As for the oplock break operation - it is the same.
May be we should rename 'CIFS_ASYNC_OP' flag to make it more
informative (e.g. CIFS_NOCREDIT_OP or smth else) ?
--
Best regards,
Pavel Shilovsky.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
