On Wed, 15 Feb 2012 18:18:11 +0400
Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> 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.
>
But why is it this way? If it's atomic why bother with the locking at
all? If you really need the lock then why bother making it atomic?
Any time I see inconsistent locking like this, it's usually an indicator
that something is wrong. If it's not wrong then it needs to be
documented at the very least.
My suspicion is that the existing code was slapped together without
much thought as to what locks were necessary. Since you're in here now
though, this ought to be corrected before you move forward with adding
new 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?
>
> 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.
>
I suppose, but I worry that adding the new complexity of credits
without fixing that bug is going to make this really difficult to
debug...
> >
> >> 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?
>
Ugh, I guess. Is this fixed later in this patchset? If not, then I
suggest fixing that problem first before you introduce the new
credit-based algorithm. Fixing this problem is really my immediate goal
with this patchset. If we can do it in a way that also paves the way
for the for SMB2 code then that's ideal.
> >> 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!
>
--
Jeff Layton <jlayton@xxxxxxxxx>
--
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
