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@xxxxxxxxxx> wrote:
> On Tue, 18 Nov 2008 21:46:59 -0600
> "Steve French" <smfrench@xxxxxxxxx> 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@xxxxxxxxxx>
>
--
Thanks,
Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
