-----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.
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?
> 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.
> spin_lock(&GlobalMid_Lock);
> while (1) {
> - if (atomic_read(&server->inFlight) >= cifs_max_pending) {
> + if (atomic_read(&server->credits) == 0) {
> 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-----
ÿôèº{.nÇ+?·?®??+%?Ëÿ±éݶ¥?wÿº{.nÇ+?·¥?{±ýÈ?³ø§¶?¡Ü¨}©?²Æ zÚ&j:+v?¨þø ¯ù ®w¥þ?à2?Þ?¨èÚ&¢)ß¡«a¶Úÿÿûàz¿äz¹Þ?ú+?ù???Ý¢jÿ?wèþf
