[linux-cifs-client] Re: [PATCH] do not attempt to close cifs files which are already closed due to session reconnect

[Steve French]

Although I doubt that we could force a failure in this case, it is
worth checking ... even though the close race with mark open files
invalid seems unlikely ... we are going to check for
tcon->need_reconnect too

On Wed, Nov 19, 2008 at 6:04 AM, Jeff Layton <jlayton at redhat.com> wrote:
> On Tue, 18 Nov 2008 21:46:59 -0600
> "Steve French" <smfrench at gmail.com> wrote:
>
>> In hunting down why we could get EBADF returned on close in some cases
>> after reconnect, I found out that cifs_close was checking to see if
>> the share (mounted server export) was valid (didn't need reconnect due
>> to session crash/timeout) but we weren't checking if the handle was
>> valid (ie the share was reconnected, but the file handle was not
>> reopened yet).  It also adds some locking around the updates/checks of
>> the cifs_file->invalidHandle flag
>>
>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> index 6449e1a..cd975fe 100644
>> --- a/fs/cifs/file.c
>> +++ b/fs/cifs/file.c
>> @@ -512,8 +512,9 @@ int cifs_close(struct inode *inode, struct file *file)
>>                               if (atomic_read(&pSMBFile->wrtPending))
>>                                       cERROR(1,
>>                                               ("close with pending writes"));
>> -                             rc = CIFSSMBClose(xid, pTcon,
>> -                                               pSMBFile->netfid);
>> +                             if (!pSMBFile->invalidHandle)
>> +                                     rc = CIFSSMBClose(xid, pTcon,
>> +                                                       pSMBFile->netfid);
>
>
> Do we need a lock around this check for invalidHandle? Could this race
> with mark_open_files_invalid()?
>
>>                       }
>>               }
>>
>> @@ -587,15 +588,18 @@ int cifs_closedir(struct inode *inode, struct file *file)
>>               pTcon = cifs_sb->tcon;
>>
>>               cFYI(1, ("Freeing private data in close dir"));
>> +             write_lock(&GlobalSMBSeslock);
>>               if (!pCFileStruct->srch_inf.endOfSearch &&
>>                   !pCFileStruct->invalidHandle) {
>>                       pCFileStruct->invalidHandle = true;
>> +                     write_unlock(&GlobalSMBSeslock);
>>                       rc = CIFSFindClose(xid, pTcon, pCFileStruct->netfid);
>>                       cFYI(1, ("Closing uncompleted readdir with rc %d",
>>                                rc));
>>                       /* not much we can do if it fails anyway, ignore rc */
>>                       rc = 0;
>> -             }
>> +             } else
>> +                     write_unlock(&GlobalSMBSeslock);
>>               ptmp = pCFileStruct->srch_inf.ntwrk_buf_start;
>>               if (ptmp) {
>>                       cFYI(1, ("closedir free smb buf in srch struct"));
>> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
>> index addd1dc..9ee3f68 100644
>> --- a/fs/cifs/misc.c
>> +++ b/fs/cifs/misc.c
>> @@ -555,12 +555,14 @@ is_valid_oplock_break(struct smb_hdr *buf,
>> struct TCP_Server_Info *srv)
>>                               continue;
>>
>>                       cifs_stats_inc(&tcon->num_oplock_brks);
>> +                     write_lock(&GlobalSMBSeslock);
>>                       list_for_each(tmp2, &tcon->openFileList) {
>>                               netfile = list_entry(tmp2, struct cifsFileInfo,
>>                                                    tlist);
>>                               if (pSMB->Fid != netfile->netfid)
>>                                       continue;
>>
>> +                             write_unlock(&GlobalSMBSeslock);
>>                               read_unlock(&cifs_tcp_ses_lock);
>>                               cFYI(1, ("file id match, oplock break"));
>>                               pCifsInode = CIFS_I(netfile->pInode);
>> @@ -576,6 +578,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct
>> TCP_Server_Info *srv)
>>
>>                               return true;
>>                       }
>> +                     write_unlock(&GlobalSMBSeslock);
>>                       read_unlock(&cifs_tcp_ses_lock);
>>                       cFYI(1, ("No matching file for oplock break"));
>>                       return true;
>> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
>> index 58d5729..9f51f9b 100644
>> --- a/fs/cifs/readdir.c
>> +++ b/fs/cifs/readdir.c
>> @@ -741,11 +741,14 @@ static int find_cifs_entry(const int xid, struct
>> cifsTconInfo *pTcon,
>>          (index_to_find < first_entry_in_buffer)) {
>>               /* close and restart search */
>>               cFYI(1, ("search backing up - close and restart search"));
>> +             write_lock(&GlobalSMBSeslock);
>>               if (!cifsFile->srch_inf.endOfSearch &&
>>                   !cifsFile->invalidHandle) {
>>                       cifsFile->invalidHandle = true;
>> +                     write_unlock(&GlobalSMBSeslock);
>>                       CIFSFindClose(xid, pTcon, cifsFile->netfid);
>> -             }
>> +             } else
>> +                     write_unlock(&GlobalSMBSeslock);
>>               if (cifsFile->srch_inf.ntwrk_buf_start) {
>>                       cFYI(1, ("freeing SMB ff cache buf on search rewind"));
>>                       if (cifsFile->srch_inf.smallBuf)
>>
>>
>>
>
> Also, initiate_cifs_search() allocates a cifsFileInfo struct and then
> sets invalidHandle to true. Is there a possible race between those
> operations? It may be safe, but it might be nice to comment why that
> is. In hindsight it might have been better to invert this flag (i.e.
> validHandle) so that it would be false immediately after kzalloc()
> until it is set true...
>
> --
> Jeff Layton <jlayton at redhat.com>
>



-- 
Thanks,

Steve