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

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

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 3360 bytes --]

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);
 			}
 		}

@@ -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)



-- 
Thanks,

Steve

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: do-not-attempt-to-close-invalid-handle.patch --]
[-- Type: text/x-diff; name=do-not-attempt-to-close-invalid-handle.patch, Size: 2894 bytes --]

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);
 			}
 		}
 
@@ -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)

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

[Jeff Layton]

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>

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@redhat.com> wrote:
> 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>
>



-- 
Thanks,

Steve

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

[Steve French]

On Wed, Nov 19, 2008 at 6:04 AM, Jeff Layton <jlayton@redhat.com> wrote:
> 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
>>

>
> Do we need a lock around this check for invalidHandle? Could this race
> with mark_open_files_invalid()?
The attached patch may reduce the window of opportunity for the
race you describe.   Do you think we need another flag?  (one
to keep requests other than a write retry from using this
handle, and one to prevent reopen when the handle is about to be closed
after we have given up on write retries getting through?

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 6449e1a..6ccb9c7 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -489,14 +489,13 @@ int cifs_close(struct inode *inode, struct file *file)
 	if (pSMBFile) {
 		struct cifsLockInfo *li, *tmp;

+		write_lock(&GlobalSMBSeslock);
 		pSMBFile->closePend = true;
+		write_unlock(&GlobalSMBSeslock);
 		if (pTcon) {
-			/* no sense reconnecting to close a file that is
-			   already closed */
-			if (!pTcon->need_reconnect) {
-				timeout = 2;
-				while ((atomic_read(&pSMBFile->wrtPending) != 0)
-					&& (timeout <= 2048)) {
+			timeout = 2;
+			while ((atomic_read(&pSMBFile->wrtPending) != 0)
+				&& (timeout <= 2048)) {
 					/* Give write a better chance to get to
 					server ahead of the close.  We do not
 					want to add a wait_q here as it would
@@ -504,19 +503,28 @@ int cifs_close(struct inode *inode, struct file *file)
 					the struct would be in each open file,
 					but this should give enough time to
 					clear the socket */
-					cFYI(DBG2,
-						("close delay, write pending"));
-					msleep(timeout);
-					timeout *= 4;
-				}
-				if (atomic_read(&pSMBFile->wrtPending))
-					cERROR(1,
-						("close with pending writes"));
-				rc = CIFSSMBClose(xid, pTcon,
-						  pSMBFile->netfid);
+				cFYI(DBG2, ("close delay, write pending"));
+				msleep(timeout);
+				timeout *= 4;
 			}
+			if (atomic_read(&pSMBFile->wrtPending))
+				cERROR(1, ("close with pending writes"));
+
+			/* no sense reconnecting to close a file that is
+			   already closed */
+			write_lock(&GlobalSMBSeslock);
+			if (!pTcon->need_reconnect &&
+			    !pSMBFile->invalidHandle) {
+				write_unlock(&GlobalSMBSeslock);
+				rc = CIFSSMBClose(xid, pTcon, pSMBFile->netfid);
+			} else
+				write_unlock(&GlobalSMBSeslock);
 		}

+		write_lock(&GlobalSMBSeslock);
+		list_del(&pSMBFile->flist);
+		list_del(&pSMBFile->tlist);
+		write_unlock(&GlobalSMBSeslock);
 		/* Delete any outstanding lock records.
 		   We'll lose them when the file is closed anyway. */
 		mutex_lock(&pSMBFile->lock_mutex);
@@ -525,11 +533,6 @@ int cifs_close(struct inode *inode, struct file *file)
 			kfree(li);
 		}
 		mutex_unlock(&pSMBFile->lock_mutex);
-
-		write_lock(&GlobalSMBSeslock);
-		list_del(&pSMBFile->flist);
-		list_del(&pSMBFile->tlist);
-		write_unlock(&GlobalSMBSeslock);
 		timeout = 10;
 		/* We waited above to give the SMBWrite a chance to issue
 		   on the wire (so we do not get SMBWrite returning EBADF
@@ -587,15 +590,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...
Reversing the flag might be cleared but in the case you describe
setting invalidHandle shouldn't race because the only place it
is checked in the readdir related path would be on close of
the directory and we a race with close and readdir with search
rewind is unlikely to be an issue.


-- 
Thanks,

Steve

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

[Jeff Layton]

On Wed, 19 Nov 2008 23:24:47 -0600
"Steve French" <smfrench@gmail.com> wrote:

> On Wed, Nov 19, 2008 at 6:04 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > 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
> >>
> 
> >
> > Do we need a lock around this check for invalidHandle? Could this race
> > with mark_open_files_invalid()?
> The attached patch may reduce the window of opportunity for the
> race you describe.   Do you think we need another flag?  (one
> to keep requests other than a write retry from using this
> handle, and one to prevent reopen when the handle is about to be closed
> after we have given up on write retries getting through?
> 


So that I make sure I understand the problem...

We have a file that is getting ready to be closed (closePend is set),
but the tcon has been reconnected and the filehandle is now invalid.
You only want to reopen the file in order to flush data out of the
cache, but only if there are actually dirty pages to be flushed.

If closePend is set then the userspace filehandle is already dead? No
further pages can be dirtied, right?

Rather than a new flag, I suggest checking for whether there are dirty
pages attached to the inode. If so, then we'll want to reopen the file
and flush it before finally closing it.

Or am I missing something here?

> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 6449e1a..6ccb9c7 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -489,14 +489,13 @@ int cifs_close(struct inode *inode, struct file *file)
>  	if (pSMBFile) {
>  		struct cifsLockInfo *li, *tmp;
> 
> +		write_lock(&GlobalSMBSeslock);
>  		pSMBFile->closePend = true;
> +		write_unlock(&GlobalSMBSeslock);
>  		if (pTcon) {
> -			/* no sense reconnecting to close a file that is
> -			   already closed */
> -			if (!pTcon->need_reconnect) {
> -				timeout = 2;
> -				while ((atomic_read(&pSMBFile->wrtPending) != 0)
> -					&& (timeout <= 2048)) {
> +			timeout = 2;
> +			while ((atomic_read(&pSMBFile->wrtPending) != 0)
> +				&& (timeout <= 2048)) {
>  					/* Give write a better chance to get to
>  					server ahead of the close.  We do not
>  					want to add a wait_q here as it would
> @@ -504,19 +503,28 @@ int cifs_close(struct inode *inode, struct file *file)
>  					the struct would be in each open file,
>  					but this should give enough time to
>  					clear the socket */
> -					cFYI(DBG2,
> -						("close delay, write pending"));
> -					msleep(timeout);
> -					timeout *= 4;
> -				}
> -				if (atomic_read(&pSMBFile->wrtPending))
> -					cERROR(1,
> -						("close with pending writes"));
> -				rc = CIFSSMBClose(xid, pTcon,
> -						  pSMBFile->netfid);
> +				cFYI(DBG2, ("close delay, write pending"));
> +				msleep(timeout);
> +				timeout *= 4;
>  			}
> +			if (atomic_read(&pSMBFile->wrtPending))
> +				cERROR(1, ("close with pending writes"));
> +
> +			/* no sense reconnecting to close a file that is
> +			   already closed */
> +			write_lock(&GlobalSMBSeslock);
> +			if (!pTcon->need_reconnect &&
> +			    !pSMBFile->invalidHandle) {
> +				write_unlock(&GlobalSMBSeslock);
> +				rc = CIFSSMBClose(xid, pTcon, pSMBFile->netfid);
> +			} else
> +				write_unlock(&GlobalSMBSeslock);
>  		}
> 
> +		write_lock(&GlobalSMBSeslock);
> +		list_del(&pSMBFile->flist);
> +		list_del(&pSMBFile->tlist);
> +		write_unlock(&GlobalSMBSeslock);
>  		/* Delete any outstanding lock records.
>  		   We'll lose them when the file is closed anyway. */
>  		mutex_lock(&pSMBFile->lock_mutex);
> @@ -525,11 +533,6 @@ int cifs_close(struct inode *inode, struct file *file)
>  			kfree(li);
>  		}
>  		mutex_unlock(&pSMBFile->lock_mutex);
> -
> -		write_lock(&GlobalSMBSeslock);
> -		list_del(&pSMBFile->flist);
> -		list_del(&pSMBFile->tlist);
> -		write_unlock(&GlobalSMBSeslock);
>  		timeout = 10;
>  		/* We waited above to give the SMBWrite a chance to issue
>  		   on the wire (so we do not get SMBWrite returning EBADF
> @@ -587,15 +590,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...
> Reversing the flag might be cleared but in the case you describe
> setting invalidHandle shouldn't race because the only place it
> is checked in the readdir related path would be on close of
> the directory and we a race with close and readdir with search
> rewind is unlikely to be an issue.
> 
> 
> -- 
> Thanks,
> 
> Steve


-- 
Jeff Layton <jlayton@redhat.com>

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

[Steve French]

On Thu, Nov 20, 2008 at 7:02 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Wed, 19 Nov 2008 23:24:47 -0600
> "Steve French" <smfrench@gmail.com> wrote:
>
>> On Wed, Nov 19, 2008 at 6:04 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> > 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
>> >>
>>
>> >
>> > Do we need a lock around this check for invalidHandle? Could this race
>> > with mark_open_files_invalid()?
>> The attached patch may reduce the window of opportunity for the
>> race you describe.   Do you think we need another flag?  (one
>> to keep requests other than a write retry from using this
>> handle, and one to prevent reopen when the handle is about to be closed
>> after we have given up on write retries getting through?
>>
>
>
> So that I make sure I understand the problem...
>
> We have a file that is getting ready to be closed (closePend is set),
> but the tcon has been reconnected and the filehandle is now invalid.
> You only want to reopen the file in order to flush data out of the
> cache, but only if there are actually dirty pages to be flushed.
I don't think we have to worry about normal case of flushing dirty pages, that
happens already before we get to cifs_close (fput calls flush/fsync).
The case I was thinking about was a write on this handle that
has hung, reconnected, and we are waiting for this pending write to complete.

> If closePend is set then the userspace filehandle is already dead? No
> further pages can be dirtied, right?
They could be dirtied from other handles, and writepages picks
the first handle that it can since writepages does not
specify which handle to use (writepages won't pick a handle that
that is close pending and it may be ok on retry because we look
for a valid handle each time we retry so shouldn't pick this one)

> Rather than a new flag, I suggest checking for whether there are dirty
> pages attached to the inode. If so, then we'll want to reopen the file
> and flush it before finally closing it.
There shouldn't be dirty pages if this is the last handle on the inode
being closed




-- 
Thanks,

Steve

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

[Jeff Layton]

On Thu, 20 Nov 2008 08:04:08 -0600
"Steve French" <smfrench@gmail.com> wrote:

> On Thu, Nov 20, 2008 at 7:02 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > On Wed, 19 Nov 2008 23:24:47 -0600
> > "Steve French" <smfrench@gmail.com> wrote:
> >
> >> On Wed, Nov 19, 2008 at 6:04 AM, Jeff Layton <jlayton@redhat.com> wrote:
> >> > 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
> >> >>
> >>
> >> >
> >> > Do we need a lock around this check for invalidHandle? Could this race
> >> > with mark_open_files_invalid()?
> >> The attached patch may reduce the window of opportunity for the
> >> race you describe.   Do you think we need another flag?  (one
> >> to keep requests other than a write retry from using this
> >> handle, and one to prevent reopen when the handle is about to be closed
> >> after we have given up on write retries getting through?
> >>
> >
> >
> > So that I make sure I understand the problem...
> >
> > We have a file that is getting ready to be closed (closePend is set),
> > but the tcon has been reconnected and the filehandle is now invalid.
> > You only want to reopen the file in order to flush data out of the
> > cache, but only if there are actually dirty pages to be flushed.
> I don't think we have to worry about normal case of flushing dirty pages, that
> happens already before we get to cifs_close (fput calls flush/fsync).
> The case I was thinking about was a write on this handle that
> has hung, reconnected, and we are waiting for this pending write to complete.
> 
> > If closePend is set then the userspace filehandle is already dead? No
> > further pages can be dirtied, right?
> They could be dirtied from other handles, and writepages picks
> the first handle that it can since writepages does not
> specify which handle to use (writepages won't pick a handle that
> that is close pending and it may be ok on retry because we look
> for a valid handle each time we retry so shouldn't pick this one)
> 

Right, I was assuming that the inode has no other open filehandles...

Even if there are any other open filehandles though, we still want to
flush whatever dirty pages we have, correct? Or at least start
writeback on them...

> > Rather than a new flag, I suggest checking for whether there are dirty
> > pages attached to the inode. If so, then we'll want to reopen the file
> > and flush it before finally closing it.
> There shouldn't be dirty pages if this is the last handle on the inode
> being closed
> 

At the time that the "release" op is called (which is cifs_close in
this case), there may still be dirty pages, even if this is the last
filehandle, right?

If so then it seems reasonable to just check to see if there are any
dirty pages, reopen the file and start writeback if so.

Alternately, I suppose we could consider skipping the reopen/writeback
if there are other open filehandles for the inode. The idea would be
that we could assume that the pages would get flushed when the last fh
is closed. I'm not sure if this violates any close-to-open attribute
semantics though.

-- 
Jeff Layton <jlayton@redhat.com>

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

[Steve French]

On Thu, Nov 20, 2008 at 8:39 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Thu, 20 Nov 2008 08:04:08 -0600
> "Steve French" <smfrench@gmail.com> wrote:
>
>> On Thu, Nov 20, 2008 at 7:02 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> > On Wed, 19 Nov 2008 23:24:47 -0600
>> > "Steve French" <smfrench@gmail.com> wrote:
>> >
>> >> On Wed, Nov 19, 2008 at 6:04 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> >> > 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
>> >> >>
>> >>
>> >> >
>> >> > Do we need a lock around this check for invalidHandle? Could this race
>> >> > with mark_open_files_invalid()?
>> >> The attached patch may reduce the window of opportunity for the
>> >> race you describe.   Do you think we need another flag?  (one
>> >> to keep requests other than a write retry from using this
>> >> handle, and one to prevent reopen when the handle is about to be closed
>> >> after we have given up on write retries getting through?
>> >>
>> >
>> >
>> > So that I make sure I understand the problem...
>> >
>> > We have a file that is getting ready to be closed (closePend is set),
>> > but the tcon has been reconnected and the filehandle is now invalid.
>> > You only want to reopen the file in order to flush data out of the
>> > cache, but only if there are actually dirty pages to be flushed.
>> I don't think we have to worry about normal case of flushing dirty pages, that
>> happens already before we get to cifs_close (fput calls flush/fsync).
>> The case I was thinking about was a write on this handle that
>> has hung, reconnected, and we are waiting for this pending write to complete.
>>
>> > If closePend is set then the userspace filehandle is already dead? No
>> > further pages can be dirtied, right?
>> They could be dirtied from other handles, and writepages picks
>> the first handle that it can since writepages does not
>> specify which handle to use (writepages won't pick a handle that
>> that is close pending and it may be ok on retry because we look
>> for a valid handle each time we retry so shouldn't pick this one)
>>
>
> Right, I was assuming that the inode has no other open filehandles...
>
> Even if there are any other open filehandles though, we still want to
> flush whatever dirty pages we have, correct? Or at least start
> writeback on them...
>
>> > Rather than a new flag, I suggest checking for whether there are dirty
>> > pages attached to the inode. If so, then we'll want to reopen the file
>> > and flush it before finally closing it.
>> There shouldn't be dirty pages if this is the last handle on the inode
>> being closed
>>
>
> At the time that the "release" op is called (which is cifs_close in
> this case), there may still be dirty pages, even if this is the last
> filehandle, right?
I don't see how we could have dirty pages on that inode,
filemap_fdatawrite was called (by cifs_flush) before we got to release
and writes on different handles would not have oplock (if there are
any other handles) and we would call filemap_fdatawrite on each of
those (non-cached) writes on another handle.

> If so then it seems reasonable to just check to see if there are any
> dirty pages, reopen the file and start writeback if so.
>
> Alternately, I suppose we could consider skipping the reopen/writeback
> if there are other open filehandles for the inode. The idea would be
> that we could assume that the pages would get flushed when the last fh
> is closed. I'm not sure if this violates any close-to-open attribute
> semantics though.
I don't think it matters much.  We only have the write pending flag
when we are actually using the file handle (find_writable_file
increments it) for write ... if we failed timing out on write to that
handle we would use a different handle or fail.

-- 
Thanks,

Steve

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

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 40 bytes --]

Slightly updated




-- 
Thanks,

Steve

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: do-not-close-invalid-handle.patch --]
[-- Type: text/x-diff; name=do-not-close-invalid-handle.patch, Size: 4536 bytes --]

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index f1ae1f5..c57c056 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -606,7 +606,15 @@ GLOBAL_EXTERN struct list_head		cifs_tcp_ses_list;
  * changes to the tcon->tidStatus should be done while holding this lock.
  */
 GLOBAL_EXTERN rwlock_t		cifs_tcp_ses_lock;
-GLOBAL_EXTERN rwlock_t GlobalSMBSeslock;  /* protects list inserts on 3 above */
+
+/*
+ * This lock protects the cifs_file->llist and cifs_file->flist
+ * list operations, and updates to some flags (cifs_file->invalidHandle)
+ * It will be moved to either use the tcon->stat_lock or equivalent later.
+ * If cifs_tcp_ses_lock and the lock below are both needed to be held, then
+ * the cifs_tcp_ses_lock must be grabbed first and released last.
+ */
+GLOBAL_EXTERN rwlock_t GlobalSMBSeslock;
 
 GLOBAL_EXTERN struct list_head GlobalOplock_Q;
 
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 6449e1a..b691b89 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -488,12 +488,13 @@ int cifs_close(struct inode *inode, struct file *file)
 	pTcon = cifs_sb->tcon;
 	if (pSMBFile) {
 		struct cifsLockInfo *li, *tmp;
-
+		write_lock(&GlobalSMBSeslock);
 		pSMBFile->closePend = true;
 		if (pTcon) {
 			/* no sense reconnecting to close a file that is
 			   already closed */
 			if (!pTcon->need_reconnect) {
+				write_unlock(&GlobalSMBSeslock);
 				timeout = 2;
 				while ((atomic_read(&pSMBFile->wrtPending) != 0)
 					&& (timeout <= 2048)) {
@@ -510,12 +511,15 @@ int cifs_close(struct inode *inode, struct file *file)
 					timeout *= 4;
 				}
 				if (atomic_read(&pSMBFile->wrtPending))
-					cERROR(1,
-						("close with pending writes"));
-				rc = CIFSSMBClose(xid, pTcon,
+					cERROR(1, ("close with pending write"));
+				if (!pTcon->need_reconnect &&
+				    !pSMBFile->invalidHandle)
+					rc = CIFSSMBClose(xid, pTcon,
 						  pSMBFile->netfid);
-			}
-		}
+			} else
+				write_unlock(&GlobalSMBSeslock);
+		} else
+			write_unlock(&GlobalSMBSeslock);
 
 		/* Delete any outstanding lock records.
 		   We'll lose them when the file is closed anyway. */
@@ -587,15 +591,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)