[linux-cifs-client] [PATCH] cifs: consolidate reconnect logic in smb_init routines

[Jeff Layton]

On Wed, 2 Sep 2009 20:38:49 -0500
Steve French <smfrench at gmail.com> wrote:

> On Sat, Aug 15, 2009 at 11:06 AM, Jeff Layton<jlayton at redhat.com> wrote:
> > There's a large cut and paste chunk of code in smb_init and
> > small_smb_init to handle reconnects. Break it out into a separate
> > function, clean it up and have both routines call it.
> 
> As can be seen from the excerpt below, your patch does change behavior
> (not just do cleanup) - not sure if that is what you intended.
> 
> In the old code, for handle based operations, when we have to
> reconnect the socket in this path, after coming back from tcon
> whether or not there was an error - the rc was converted
> to EAGAIN - to signal the caller that he has to reopen the file (if
> another reconnect is needed due to tcon failure that
> will happen during the reopen).  The new code returns the tcon
> error back to the caller for handle based operations (only returns
> EAGAIN on handle based operations if tcon succeeds).
> 
> This will change behavior on paths when the session drops
> and tcon reconnection fails - which may be ok - but needs to be
> looked at.
> 

Good catch. Yeah, that might be an issue. I'll move the "out:" label
above the switch statement and that should correct it. When I have some
time, I'll retest and resend.

That said, I'm not sure the design here is quite right. If it always
returns -EAGAIN to the caller on a handle-based operation when the tcon
fails isn't that going to loop indefinitely when there's a hard failure
with the tcon?

Also, I'm not sure we have the list of handle-based ops correct. What
about the SetFileInfo calls?

> > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> > index 1866bc2..4bdca05 100644
> > --- a/fs/cifs/cifssmb.c
> > +++ b/fs/cifs/cifssmb.c
> ...
> > +
> > +       mark_open_files_invalid(tcon);
> > +       rc = CIFSTCon(0, ses, tcon->treeName, tcon, nls_codepage);
> > +       up(&ses->sesSem);
> > +       cFYI(1, ("reconnect tcon rc = %d", rc));
> > +
> > +       if (rc)
> > +               goto out;
> ....
> > -                               mark_open_files_invalid(tcon);
> > -                               rc = CIFSTCon(0, tcon->ses, tcon->treeName,
> > -                                             tcon, nls_codepage);
> > -                               up(&tcon->ses->sesSem);
> > -                               /* BB FIXME add code to check if wsize needs
> > -                               update due to negotiated smb buffer size
> > -                               shrinking */
> > -                               if (rc == 0) {
> > -                                       atomic_inc(&tconInfoReconnectCount);
> > -                                       /* tell server Unix caps we support */
> > -                                       if (tcon->ses->capabilities & CAP_UNIX)
> > -                                               reset_cifs_unix_caps(
> > -                                               0 /* no xid */,
> > -                                               tcon,
> > -                                               NULL /* do not know sb */,
> > -                                               NULL /* no vol info */);
> > -                               }
> > -
> > -                               cFYI(1, ("reconnect tcon rc = %d", rc));
> > -                               /* Removed call to reopen open files here.
> > -                                  It is safer (and faster) to reopen files
> > -                                  one at a time as needed in read and write */
> > -
> > -                               /* Check if handle based operation so we
> > -                                  know whether we can continue or not without
> > -                                  returning to caller to reset file handle */
> > -                               switch (smb_command) {
> > -                                       case SMB_COM_READ_ANDX:
> > -                                       case SMB_COM_WRITE_ANDX:
> > -                                       case SMB_COM_CLOSE:
> > -                                       case SMB_COM_FIND_CLOSE2:
> > -                                       case SMB_COM_LOCKING_ANDX: {
> > -                                               unload_nls(nls_codepage);
> > -                                               return -EAGAIN;
> > -                                       }
> 
> 
> -- 
> Thanks,
> 
> Steve
> _______________________________________________
> linux-cifs-client mailing list
> linux-cifs-client at lists.samba.org
> https://lists.samba.org/mailman/listinfo/linux-cifs-client
> 


-- 
Jeff Layton <jlayton at redhat.com>