From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Steve French" Subject: Re: [PATCH] do not attempt to close cifs files which are already closed due to session reconnect Date: Wed, 19 Nov 2008 10:05:33 -0600 Message-ID: <524f69650811190805p2370dec3t603cff364cb362fd@mail.gmail.com> References: <524f69650811181946s79fdba88w11c8c4c6677df1db@mail.gmail.com> <20081119070429.1d977f72@tleilax.poochiereds.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: linux-fsdevel , "linux-cifs-client@lists.samba.org" To: "Jeff Layton" Return-path: Received: from ug-out-1314.google.com ([66.249.92.173]:39249 "EHLO ug-out-1314.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753856AbYKSQFf (ORCPT ); Wed, 19 Nov 2008 11:05:35 -0500 Received: by ug-out-1314.google.com with SMTP id 39so11676ugf.37 for ; Wed, 19 Nov 2008 08:05:33 -0800 (PST) In-Reply-To: <20081119070429.1d977f72@tleilax.poochiereds.net> Content-Disposition: inline Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 wrote: > On Tue, 18 Nov 2008 21:46:59 -0600 > "Steve French" 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 > -- Thanks, Steve