2012/2/12 Jeff Layton <jlayton@xxxxxxxxx>:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Thu, 9 Feb 2012 11:41:24 +0300
> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>
>> and send no more than credits value requests at once. For SMB/CIFS
>> it's trivial: increment this value by receiving any message and
>> decrement by sending one.
>>
>> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
>
> Yes! Nice work. This is clearly the right approach for merging SMB2
> support into the kernel. Extend the CIFS code to do what we need and
> share as much code as possible between the SMB1 and SMB2 support. By
> doing this piecemeal too we can avoid a wholesale "code dump" into the
> kernel.
>
> Some comments below.
>
>> ---
>> fs/cifs/cifsglob.h | 1 +
>> fs/cifs/cifssmb.c | 3 +++
>> fs/cifs/connect.c | 10 ++++------
>> fs/cifs/transport.c | 20 ++++++++++++++++----
>> 4 files changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index 76e7d8b..f96752a 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -256,6 +256,7 @@ struct TCP_Server_Info {
>> bool noautotune; /* do not autotune send buf sizes */
>> bool tcp_nodelay;
>> atomic_t inFlight; /* number of requests on the wire to server */
>> + atomic_t credits; /* send no more requests at once */
>
> Currently, it's not clear whether the inFlight counter is protected by
> the GlobalMid_lock or not. Ditto for this new counter. If it is
> protected by it, then there's no need for it to be an atomic value. The
> locking around this needs to be explicitly clear to avoid later bugs
> when we tinker with this code.
We decrement inFlight value without holding GlobalMid_lock - so it
should be atomic.
>
> Also, should we just replace the inFlight counter with the credits
> counter? Is there some way to calculate the # of requests in flight given
> the value of "credits" in the SMB2 case?
I don't think so.
>
>> struct mutex srv_mutex;
>> struct task_struct *tsk;
>> char server_GUID[16];
>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>> index 8b7794c..fbc4437 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -717,6 +717,7 @@ cifs_echo_callback(struct mid_q_entry *mid)
>>
>> DeleteMidQEntry(mid);
>> atomic_dec(&server->inFlight);
>> + atomic_inc(&server->credits);
>> wake_up(&server->request_q);
>> }
>>
>> @@ -1670,6 +1671,7 @@ cifs_readv_callback(struct mid_q_entry *mid)
>> queue_work(system_nrt_wq, &rdata->work);
>> DeleteMidQEntry(mid);
>> atomic_dec(&server->inFlight);
>> + atomic_inc(&server->credits);
>> wake_up(&server->request_q);
>> }
>>
>> @@ -2111,6 +2113,7 @@ cifs_writev_callback(struct mid_q_entry *mid)
>> queue_work(system_nrt_wq, &wdata->work);
>> DeleteMidQEntry(mid);
>> atomic_dec(&tcon->ses->server->inFlight);
>> + atomic_inc(&tcon->ses->server->credits);
>> wake_up(&tcon->ses->server->request_q);
>> }
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 602f77c..6b95304 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -648,12 +648,8 @@ static void clean_demultiplex_info(struct TCP_Server_Info *server)
>> * time to as little as two.
>> */
>> spin_lock(&GlobalMid_Lock);
>> - if (atomic_read(&server->inFlight) >= cifs_max_pending)
>> - atomic_set(&server->inFlight, cifs_max_pending - 1);
>> - /*
>> - * We do not want to set the max_pending too low or we could end up
>> - * with the counter going negative.
>> - */
>> + if (atomic_read(&server->credits) == 0)
>> + atomic_set(&server->credits, 1);
>> spin_unlock(&GlobalMid_Lock);
>> /*
>> * Although there should not be any requests blocked on this queue it
>> @@ -3759,9 +3755,11 @@ int cifs_negotiate_protocol(unsigned int xid, struct cifs_ses *ses)
>> if (server->maxBuf != 0)
>> return 0;
>>
>> + atomic_set(&ses->server->credits, cifs_max_pending);
>> rc = CIFSSMBNegotiate(xid, ses);
>> if (rc == -EAGAIN) {
>> /* retry only once on 1st time connection */
>> + atomic_set(&ses->server->credits, cifs_max_pending);
>> rc = CIFSSMBNegotiate(xid, ses);
>> if (rc == -EAGAIN)
>> rc = -EHOSTDOWN;
>> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
>> index 0cc9584..f1f927f 100644
>> --- a/fs/cifs/transport.c
>> +++ b/fs/cifs/transport.c
>> @@ -260,17 +260,17 @@ static int wait_for_free_request(struct TCP_Server_Info *server,
>> if (long_op == CIFS_ASYNC_OP) {
>> /* oplock breaks must not be held up */
>> atomic_inc(&server->inFlight);
>> + atomic_dec(&server->credits);
>> return 0;
>> }
>>
>
> I haven't gotten to your later patches so I may be reviewing
> prematurely, but I think we need to rethink the code above. Allowing
> the client to exceed the maxmpx is clearly problematic. Rather than
> doing that we ought to attempt to keep 2 credits in reserve at any
> given time -- one for oplock breaks and one for echo requests.
>
> If MaxMpx 2, then we should disable oplocks. If it's 1, then we
> should disable echoes and oplocks. The latter case may take some overhaul
> of how the echo timeout handling works.
I agree that allowing to exceed the maxmps is wrong, but I think we
can leave it as it is for this patch.
>
>> spin_lock(&GlobalMid_Lock);
>> while (1) {
>> - if (atomic_read(&server->inFlight) >= cifs_max_pending) {
>> + if (atomic_read(&server->credits) == 0) {
Now I think that (according to echo and oplock problem) we need this to be:
if (atomic_read(&server->credits) <= 0) {
Right?
>> spin_unlock(&GlobalMid_Lock);
>> cifs_num_waiters_inc(server);
>> wait_event(server->request_q,
>> - atomic_read(&server->inFlight)
>> - < cifs_max_pending);
>> + atomic_read(&server->credits) > 0);
>> cifs_num_waiters_dec(server);
>> spin_lock(&GlobalMid_Lock);
>> } else {
>> @@ -283,8 +283,10 @@ 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 (long_op != CIFS_BLOCKING_OP) {
>> atomic_inc(&server->inFlight);
>> + atomic_dec(&server->credits);
>> + }
>> spin_unlock(&GlobalMid_Lock);
>> break;
>> }
>> @@ -360,6 +362,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>> if (mid == NULL) {
>> mutex_unlock(&server->srv_mutex);
>> atomic_dec(&server->inFlight);
>> + atomic_inc(&server->credits);
>> wake_up(&server->request_q);
>> return -ENOMEM;
>> }
>> @@ -393,6 +396,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
>> out_err:
>> delete_mid(mid);
>> atomic_dec(&server->inFlight);
>> + atomic_inc(&server->credits);
>> wake_up(&server->request_q);
>> return rc;
>> }
>> @@ -565,6 +569,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
>> cifs_small_buf_release(in_buf);
>> /* Update # of requests on wire to server */
>> atomic_dec(&ses->server->inFlight);
>> + atomic_inc(&ses->server->credits);
>> wake_up(&ses->server->request_q);
>> return rc;
>> }
>> @@ -602,6 +607,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
>> spin_unlock(&GlobalMid_Lock);
>> cifs_small_buf_release(in_buf);
>> atomic_dec(&ses->server->inFlight);
>> + atomic_inc(&ses->server->credits);
>> wake_up(&ses->server->request_q);
>> return rc;
>> }
>> @@ -613,6 +619,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
>> rc = cifs_sync_mid_result(midQ, ses->server);
>> if (rc != 0) {
>> atomic_dec(&ses->server->inFlight);
>> + atomic_inc(&ses->server->credits);
>> wake_up(&ses->server->request_q);
>> return rc;
>> }
>> @@ -638,6 +645,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
>> out:
>> delete_mid(midQ);
>> atomic_dec(&ses->server->inFlight);
>> + atomic_inc(&ses->server->credits);
>> wake_up(&ses->server->request_q);
>>
>> return rc;
>> @@ -689,6 +697,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
>> mutex_unlock(&ses->server->srv_mutex);
>> /* Update # of requests on wire to server */
>> atomic_dec(&ses->server->inFlight);
>> + atomic_inc(&ses->server->credits);
>> wake_up(&ses->server->request_q);
>> return rc;
>> }
>> @@ -722,6 +731,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
>> midQ->callback = DeleteMidQEntry;
>> spin_unlock(&GlobalMid_Lock);
>> atomic_dec(&ses->server->inFlight);
>> + atomic_inc(&ses->server->credits);
>> wake_up(&ses->server->request_q);
>> return rc;
>> }
>> @@ -731,6 +741,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
>> rc = cifs_sync_mid_result(midQ, ses->server);
>> if (rc != 0) {
>> atomic_dec(&ses->server->inFlight);
>> + atomic_inc(&ses->server->credits);
>> wake_up(&ses->server->request_q);
>> return rc;
>> }
>> @@ -748,6 +759,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
>> out:
>> delete_mid(midQ);
>> atomic_dec(&ses->server->inFlight);
>> + atomic_inc(&ses->server->credits);
>> wake_up(&ses->server->request_q);
>>
>> return rc;
>
>
> - --
> Jeff Layton <jlayton@xxxxxxxxx>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.18 (GNU/Linux)
>
> iQIcBAEBAgAGBQJPN7Y1AAoJEAAOaEEZVoIVil4P/1IfpsJswGldw1/ufTuoMw4Y
> pT80BdlFuNQ612hVXq3xrzrP8WYHlwnhfADS5n+JCdeBJwfJzV//bah5I5/4cLfY
> htvajLzlhL0aCIK57Sxa1GoHiXkadaO9TI+vA6x5hM2SALhJT2UA/4QVNGsC5jhG
> nDs07RjcGRzFT2nZ8/LLDliYmQXZk5FvnieShEdiDofcod2ILwaBUTIbJ5hz+CGL
> GXTFmo2GKKyZGIuGCeg+8AkV3oeFXdfakjzjkTBFZSqLnGZoJyHe4vzKhob1mGtO
> Xw7szAlFDxCLylccfid+ila60u2EHQiGZkPjRJCTcZmb+BpUuKo+5ShXBLOItIxQ
> gWLsydodP9G5uIHsrWakY/tfaQSUZzbfmIQMJKrJ31MKo/EtVdPgSdRGJKlPukjS
> 0oc+hW1o6Gkdqz98lL4sBiT6Mn6/ekB3CXdhOR1TTC+5tyOdLwxoI6DejDp0HlDs
> wypfNgbrhLarV/BF/LpY/5MXI8kGyOLZT584bO2Bl2QTRwfGUwgmMAwluf0MEMs2
> JJO5KLN+uozfLi/6b7YcT4/Uqy5D1BwWowT/0Lf2IpZYuLkyaKiWZ/Q/K6mMDKNo
> p7P+qsyF2RjwYpltSWG6zPPldLPczkWvy9BiQKXMFWI2EVNHbbPMUKRaWvIw66hX
> 28PtjNXqCfDFfIhrHE86
> =aqY+
> -----END PGP SIGNATURE-----
Thanks for reviewing this one!
--
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
