On Fri, 28 Oct 2011 23:54:16 +0400 Pavel Shilovsky <[email protected]> wrote:
> From: Steve French <[email protected]> > > 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 <[email protected]> > Signed-off-by: Pavel Shilovsky <[email protected]> > --- > 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 ([email protected]) > * > * 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. -- Jeff Layton <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
