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

[Jeff Layton <jlayton@redhat.com>]

On Tue, 18 Nov 2008 21:46:59 -0600
"Steve French" <smfrench@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@redhat.com>