19 марта 2012 г. 23:27 пользователь Jeff Layton <[email protected]> написал:
> On Fri, 16 Mar 2012 18:09:28 +0300
> Pavel Shilovsky <[email protected]> wrote:
>
>> that is essential for CIFS/SMB/SMB2 oplock breaks and SMB2 echos.
>>
>> Signed-off-by: Pavel Shilovsky <[email protected]>
>> ---
>> fs/cifs/cifsglob.h | 14 ++++++++++++--
>> fs/cifs/transport.c | 22 ++++++++++++++--------
>> 2 files changed, 26 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index d55de96..2309a67 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -315,12 +315,22 @@ in_flight(struct TCP_Server_Info *server)
>> return num;
>> }
>>
>> +static inline int*
>> +get_credits_field(struct TCP_Server_Info *server)
>> +{
>> + /*
>> + * This will change to switch statement when we reserve slots for echos
>> + * and oplock breaks.
>> + */
>> + return &server->credits;
>> +}
>> +
>> static inline bool
>> -has_credits(struct TCP_Server_Info *server)
>> +has_credits(struct TCP_Server_Info *server, int *credits)
>
> Why are you passing "credits" by reference here? Might as well pass by
> value...
This was done because we need to be sure that we are doing credits
pointer dereferencing under server->req_lock - no one is changing it
at that time. We wake up by the signal, lock req_lock and then
dereference credits pointer (that may refer to reserved credits slot
in the future).
>
>> {
>> int num;
>> spin_lock(&server->req_lock);
>> - num = server->credits;
>> + num = *credits;
>> spin_unlock(&server->req_lock);
>> return num > 0;
>> }
>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> index 11787cc..20dc7ba 100644
>> --- a/fs/cifs/transport.c
>> +++ b/fs/cifs/transport.c
>> @@ -255,26 +255,26 @@ smb_send(struct TCP_Server_Info *server, struct
>> smb_hdr *smb_buffer,
>> }
>>
>> static int
>> -wait_for_free_request(struct TCP_Server_Info *server, const int long_op)
>> +wait_for_free_credits(struct TCP_Server_Info *server, const int optype,
>> + int *credits)
>> {
>> int rc;
>>
>> spin_lock(&server->req_lock);
>> -
>> - if (long_op == CIFS_ASYNC_OP) {
>> + if (optype == CIFS_ASYNC_OP) {
>> /* oplock breaks must not be held up */
>> server->in_flight++;
>> - server->credits--;
>> + *credits -= 1;
>> spin_unlock(&server->req_lock);
>> return 0;
>> }
>>
>> while (1) {
>> - if (server->credits <= 0) {
>> + if (*credits <= 0) {
>> spin_unlock(&server->req_lock);
>> cifs_num_waiters_inc(server);
>> rc = wait_event_killable(server->request_q,
>> - has_credits(server));
>> + has_credits(server, credits));
>> cifs_num_waiters_dec(server);
>> if (rc)
>> return rc;
>> @@ -291,8 +291,8 @@ wait_for_free_request(struct TCP_Server_Info *server,
>> const int long_op)
>> */
>>
>> /* update # of requests on the wire to server */
>> - if (long_op != CIFS_BLOCKING_OP) {
>> - server->credits--;
>> + if (optype != CIFS_BLOCKING_OP) {
>> + *credits -= 1;
>> server->in_flight++;
>> }
>> spin_unlock(&server->req_lock);
>> @@ -302,6 +302,12 @@ wait_for_free_request(struct TCP_Server_Info *server,
>> const int long_op)
>> return 0;
>> }
>>
>> +static int
>> +wait_for_free_request(struct TCP_Server_Info *server, const int optype)
>> +{
>> + return wait_for_free_credits(server, optype,
>> get_credits_field(server));
>> +}
>> +
>> static int allocate_mid(struct cifs_ses *ses, struct smb_hdr *in_buf,
>> struct mid_q_entry **ppmidQ)
>> {
>
>
> Other than that nit, this looks fine...
>
> Reviewed-by: Jeff Layton <[email protected]>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best regards,
Pavel Shilovsky.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
