Re: [PATCH v2 05/53] CIFS: wait_for_free_request needs to wait on credits returned by server (for SMB2)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2011/10/29 Jeff Layton <jlayton@xxxxxxxxx>:
> On Fri, 28 Oct 2011 23:54:16 +0400
> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>
>> From: Steve French <sfrench@xxxxxxxxxx>
>>
>> wait_for_free_request is called to ensure that we don't have more than the
>> configured maximum of requests in flight on the wire (for cifs), but for SMB2,
>> the number of requests that can be in flight varies dynamically (the client
>> may request that the server grant more "credits" in a request, and each
>> request uses at least one credit).   Instead of migrating over the original
>> smb2_wait_for_free_request function, simply add a check in cifs's original
>> wait_for_free_request for the is_smb2 case to make sure that we don't go
>> beyond the number of allocated "credits" (ie those that remain from those
>> the server has allocated for this tcp session).  Otherwise the logic is unchanged
>> (other than to make it extern so that the next function to be added, smb2_sendrcv2
>> in  smbtransport.c can call it - smb2_sendrcv2 will be migrated over in the next patch).
>>
>> Note that smb2 servers will typically allow many more requests in flight at one time
>> than cifs servers (which usually default to only 50) and can adjust this as needed.
>> This should provide significant performance benefit in some workloads (ie smb2
>> in many cases will get more parallelism than cifs and some other protocols
>> since some artificially constrain the maximum number of requests).
>>
>> Signed-off-by: Steve French <sfrench@xxxxxxxxxx>
>> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
>> ---
>>  fs/cifs/cifsglob.h  |    1 +
>>  fs/cifs/cifsproto.h |    3 ++-
>>  fs/cifs/transport.c |   22 +++++++++++++++++++---
>>  3 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index 179b784..b440329 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -311,6 +311,7 @@ struct TCP_Server_Info {
>>       wait_queue_head_t read_q; /* used by readpages */
>>       atomic_t active_readpage_req; /* used by readpages */
>>       atomic_t resp_rdy; /* used by readpages and demultiplex */
>> +     atomic_t credits;  /* send no more simultaneous requests than this */
>>       __le16 smb2_dialect_revision; /* SMB2.0 implemented, 2.1 recognized */
>>       struct task_struct *observe;
>>       char smb2_crypt_key[CIFS_CRYPTO_KEY_SIZE]; /* BB can we use cifs key */
>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> index ef4f631..84d1e4c 100644
>> --- a/fs/cifs/cifsproto.h
>> +++ b/fs/cifs/cifsproto.h
>> @@ -1,7 +1,7 @@
>>  /*
>>   *   fs/cifs/cifsproto.h
>>   *
>> - *   Copyright (c) International Business Machines  Corp., 2002,2008
>> + *   Copyright (c) International Business Machines  Corp., 2002,2011
>>   *   Author(s): Steve French (sfrench@xxxxxxxxxx)
>>   *
>>   *   This library is free software; you can redistribute it and/or modify
>> @@ -68,6 +68,7 @@ extern char *cifs_compose_mount_options(const char *sb_mountdata,
>>  extern struct mid_q_entry *AllocMidQEntry(const struct smb_hdr *smb_buffer,
>>                                       struct TCP_Server_Info *server);
>>  extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
>> +extern int wait_for_free_request(struct TCP_Server_Info *sv, const int long_op);
>>  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,
>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> index 0cc9584..25d04df 100644
>> --- a/fs/cifs/transport.c
>> +++ b/fs/cifs/transport.c
>> @@ -254,7 +254,7 @@ smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer,
>>       return smb_sendv(server, &iov, 1);
>>  }
>>
>> -static int wait_for_free_request(struct TCP_Server_Info *server,
>> +int wait_for_free_request(struct TCP_Server_Info *server,
>>                                const int long_op)
>>  {
>>       if (long_op == CIFS_ASYNC_OP) {
>> @@ -265,7 +265,8 @@ static int wait_for_free_request(struct TCP_Server_Info *server,
>>
>>       spin_lock(&GlobalMid_Lock);
>>       while (1) {
>> -             if (atomic_read(&server->inFlight) >= cifs_max_pending) {
>> +             if ((server->is_smb2 == false) &&
>> +                 atomic_read(&server->inFlight) >= cifs_max_pending) {
>>                       spin_unlock(&GlobalMid_Lock);
>>                       cifs_num_waiters_inc(server);
>>                       wait_event(server->request_q,
>> @@ -273,6 +274,16 @@ static int wait_for_free_request(struct TCP_Server_Info *server,
>>                                    < cifs_max_pending);
>>                       cifs_num_waiters_dec(server);
>>                       spin_lock(&GlobalMid_Lock);
>> +#ifdef CONFIG_CIFS_SMB2
>> +             } else if (server->is_smb2 &&
>> +                       (atomic_read(&server->credits) < 1)) {
>> +                     spin_unlock(&GlobalMid_Lock);
>> +                     cifs_num_waiters_inc(server);
>> +                     wait_event(server->request_q,
>> +                                atomic_read(&server->credits) > 0);
>> +                     cifs_num_waiters_dec(server);
>> +                     spin_lock(&GlobalMid_Lock);
>> +#endif /* CONFIG_CIFS_SMB2 */
>>               } else {
>>                       if (server->tcpStatus == CifsExiting) {
>>                               spin_unlock(&GlobalMid_Lock);
>> @@ -283,8 +294,13 @@ static int wait_for_free_request(struct TCP_Server_Info *server,
>>                          as they are allowed to block on server */
>>
>>                       /* update # of requests on the wire to server */
>> -                     if (long_op != CIFS_BLOCKING_OP)
>> +                     if (server->is_smb2 == false &&
>> +                         long_op != CIFS_BLOCKING_OP)
>>                               atomic_inc(&server->inFlight);
>> +#ifdef CONFIG_CIFS_SMB2
>> +                     else if (server->is_smb2)
>> +                             atomic_dec(&server->credits);
>> +#endif
>>                       spin_unlock(&GlobalMid_Lock);
>>                       break;
>>               }
>
> This patch doesn't make much sense...
>
> "credits" gets initialized to 0, then on the first request, you'll
> decrement it which will make it go negative. I then don't see any place
> where credits is ever incremented.
>
> Maybe that happens in a different patch, but splitting your patches out
> this way makes no sense. No one can reasonably review this patch for
> sanity since the lifecycle of "credits" is incomplete.

In this case we should move this patch after we add smb2_sendrcv
function (where credits are increased). Make sense?

-- 
Best regards,
Pavel Shilovsky.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux