Re: [PATCH v2 05/53] CIFS: wait_for_free_request needs to wait on credits returned by server (for SMB2)

[Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>]

On Sun, 30 Oct 2011 00:00:15 +0400
Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:

> 2011/10/29 Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> > On Fri, 28 Oct 2011 23:54:16 +0400
> > Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> >
> >> From: Steve French <sfrench-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> >>
> >> 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 <sfrench-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> >> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> >> ---
> >>  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 (sfrench-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org)
> >>   *
> >>   *   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.
> 
> In this case we should move this patch after we add smb2_sendrcv
> function (where credits are increased). Make sense?
> 

I haven't gotten that far in the series yet, so I'm not sure if that
makes sense or not. That's the problem: we as reviewers can't review
this set as a whole. Most of us don't have brains that big. We have to
review these patches a set of individual changes that are working
toward a goal.

I think what you need to do is to figure out a way to break the
patchset up more logically. One would think that doing that would be a
set of steps that you could follow, but it's really more of an art. You
have to look at this from the viewpoint of potential reviewers.

A large patchset like this needs to have a clear "flow" that evolves
the code incrementally. Cleanups or other changes that make sense on
their own should usually be broken out and done first, and often in a
separate set. Further patches should make a set of logically complete
changes to the code.

A case in point: when I did the patchset to clean up the demultiplex
codepath, I added code to kmalloc a buffer on each pass through
readv_from_socket. In the next patch, I ripped that kmalloc back out
and added code to keep a reusable buffer for that purpose.

I could have done that in a different way that didn't "churn" the code
so much, but it made more sense to do it that way for a reviewer walking
the patches in order. Adding the new buffer management was logically
separate from the earlier change, so I broke the patches up along the
same lines.

Doing this also pays off later. It also allows for someone bisecting
the code to have a tree that reasonably works if they stop at that
point, and makes it easier to find bugs since the change is more
logical.

Sometimes it is necessary to have a patch that doesn't make a lot of
sense on its own in order to prepare for later patches. If you do that
though, you really need to have a commit message that explains what
you're doing and why.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>