Re: [PATCH] cifs: consolidate reconnect logic in smb_init routines
Jeff Layton <[email protected]>
| Newsgroups | gmane.linux.file-systems.cifs |
|---|---|
| Message-ID | <[email protected]> |
On Wed, 2 Sep 2009 20:38:49 -0500 Steve French <[email protected]> wrote: > On Sat, Aug 15, 2009 at 11:06 AM, Jeff Layton<[email protected]> 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 > [email protected] > https://lists.samba.org/mailman/listinfo/linux-cifs-client > -- Jeff Layton <[email protected]> _______________________________________________ linux-cifs-client mailing list [email protected] https://lists.samba.org/mailman/listinfo/linux-cifs-client