On Mon, 6 Feb 2012 13:59:32 +0400
Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> 2012/2/3 Jeff Layton <jlayton@xxxxxxxxxx>:
> > On Fri, 20 Jan 2012 12:34:27 +0400
> > Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
> >
> >> From: Steve French <sfrench@xxxxxxxxxx>
> >>
> >> Convert wait_for_free_request to wait on available credits
> >> for SMB2 servers and decrement a credit number for every call.
> >> Increment the credit number in smb2_sendrcv2 with every response
> >> got from the server.
> >>
> >> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
> >> ---
> >> fs/cifs/cifsglob.h | 1 +
> >> fs/cifs/cifsproto.h | 2 +-
> >> fs/cifs/smb2transport.c | 6 ++++++
> >> fs/cifs/transport.c | 20 ++++++++++++++++++--
> >> 4 files changed, 26 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >> index bb2af48..e5f0b86 100644
> >> --- a/fs/cifs/cifsglob.h
> >> +++ b/fs/cifs/cifsglob.h
> >> @@ -318,6 +318,7 @@ struct TCP_Server_Info {
> >> atomic_t num_waiters; /* blocked waiting to get in sendrecv */
> >> #endif
> >> #ifdef CONFIG_CIFS_SMB2
> >> + atomic_t credits; /* send no more simultaneous requests than this */
> >> __u64 current_smb2_mid; /* multiplex id - rotating counter */
> >> #endif
> >> };
> >> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> >> index d1e30d9..b6c784b 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
> >> diff --git a/fs/cifs/smb2transport.c b/fs/cifs/smb2transport.c
> >> index d7f1c05..b5702f3 100644
> >> --- a/fs/cifs/smb2transport.c
> >> +++ b/fs/cifs/smb2transport.c
> >> @@ -148,6 +148,7 @@ smb2_sendrcv2(const unsigned int xid, struct cifs_ses *ses,
> >> unsigned int receive_len;
> >> struct mid_q_entry *midQ;
> >> struct smb2_hdr *buf = iov[0].iov_base;
> >> + unsigned int credits = 1;
> >>
> >> if (status)
> >> *status = STATUS_SUCCESS;
> >> @@ -186,6 +187,7 @@ smb2_sendrcv2(const unsigned int xid, struct cifs_ses *ses,
> >> if (rc) {
> >> mutex_unlock(&ses->server->srv_mutex);
> >> cifs_small_buf_release(buf);
> >> + atomic_inc(&ses->server->credits);
> >> wake_up(&ses->server->request_q);
> >> return rc;
> >> }
> >> @@ -222,6 +224,7 @@ smb2_sendrcv2(const unsigned int xid, struct cifs_ses *ses,
> >> midQ->callback = cifs_delete_mid;
> >> spin_unlock(&GlobalMid_Lock);
> >> cifs_small_buf_release(buf);
> >> + atomic_inc(&ses->server->credits);
> >> wake_up(&ses->server->request_q);
> >> return rc;
> >> }
> >> @@ -232,6 +235,7 @@ smb2_sendrcv2(const unsigned int xid, struct cifs_ses *ses,
> >>
> >> rc = cifs_sync_mid_result(midQ, ses->server);
> >> if (rc) {
> >> + atomic_inc(&ses->server->credits);
> >> wake_up(&ses->server->request_q);
> >> return rc;
> >> }
> >> @@ -271,7 +275,9 @@ smb2_sendrcv2(const unsigned int xid, struct cifs_ses *ses,
> >> /* mark it so buf will not be freed by free_smb2mid */
> >> midQ->resp_buf = NULL;
> >>
> >> + credits = le16_to_cpu(buf->CreditRequest);
> >> out:
> >> + atomic_add(credits, &ses->server->credits);
> >> cifs_delete_mid(midQ);
> >> wake_up(&ses->server->request_q);
> >> return rc;
> >> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> >> index f7ac3f0..4414df8 100644
> >> --- a/fs/cifs/transport.c
> >> +++ b/fs/cifs/transport.c
> >> @@ -287,7 +287,8 @@ cifs_wait_for_free_request(struct TCP_Server_Info *server, const int long_op)
> >>
> >> 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,
> >> @@ -295,6 +296,16 @@ cifs_wait_for_free_request(struct TCP_Server_Info *server, const int long_op)
> >> < 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);
> >> @@ -305,8 +316,13 @@ cifs_wait_for_free_request(struct TCP_Server_Info *server, const int long_op)
> >> 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;
> >> }
> >
> > I'd like to suggest an alternate approach here...
> >
> > Instead of considering this as a SMB1 vs. SMB2 either/or thing we
> > should consider the SMB1 case to be a special case of SMB2 credits. In
> > the SMB1 NEGOTIATE response, the server sends us a MaxMpx value. We
> > could consider that to be an initial set of credits. Every time we get
> > a SMB1 response from the server, we'll increment the credit value.
> >
> > If we do that, then there's a lot less protocol dependent code that's
> > needed, and if done correctly should also fix the current bug in
> > the SMB1 code where it ignores the MaxMpx value.
> >
> > Thoughts?
>
> As the initial step, I suggest we can set credit value to
> cifs_max_pending before negotiate and then decrement it for any
> out-coming request and increment for - incoming. Then we can change it
> to 1 respect MaxMpx value.
>
Exactly!
That sounds like a nice, incremental approach. Doing things in small
steps like that is much better. It may take more steps to get there,
but it will pay off later. It also makes it easier to look for places
where the SMB1 architecture can be improved as you make these changes.
I'd also encourage you to look at the rest of the SMB2 patchset and see
where it can be broken up into smaller sets of patches. It's fine if
the fs isn't "feature complete" at the end of a set. You just want to
ensure that SMB1 still works after each set and the new pieces that
you've merged also work within expectations.
The main thing we *don't* want to do is merge a huge blob of SMB2 code,
have it break something and then be stuck trying to look through that
huge set of changes to figure out what broke...
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
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
