All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/15] cifs: clean up management of open filehandle (try #3)
@ 2010-10-08 17:30 Jeff Layton
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  0 siblings, 1 reply; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:30 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

This is the third attempt at the patchset to clean up management of
open filehandles in CIFS. The main changes from try #2 are:

1) the cifs_file_list_lock has been converted to a spinlock after much
discussion about it on-list.

2) the list order for the inode->openFileList has been eliminated. After
reviewing the code more thoroughly, I came to the conclusion that the
list order was essentially meaningless anyway.

The rest of the set is essentially unchanged. I've left Suresh and
Shaggy's Acked-by/Reviewed-by lines in place except on patches that
have had substantial changes.

Jeff Layton (15):
  cifs: keep dentry reference in cifsFileInfo instead of inode
    reference
  cifs: don't use vfsmount to pin superblock for oplock breaks
  cifs: eliminate cifs_posix_open_inode_helper
  cifs: eliminate oflags option from cifs_new_fileinfo
  cifs: eliminate the inode argument from cifs_new_fileinfo
  cifs: clean up cifs_reopen_file
  cifs: cifs_write argument change and cleanup
  cifs: eliminate pfile pointer from cifsFileInfo
  cifs: move cifs_new_fileinfo to file.c
  cifs: convert GlobalSMBSeslock from a rwlock to regular spinlock
  cifs: move cifsFileInfo_put to file.c
  cifs: move close processing from cifs_close to cifsFileInfo_put
  cifs: convert cifsFileInfo->count to non-atomic counter
  cifs: wait for writeback to complete in cifs_flush
  cifs: eliminate cifsInodeInfo->write_behind_rc

 fs/cifs/cifs_fs_sb.h |    1 +
 fs/cifs/cifsfs.c     |   21 ++-
 fs/cifs/cifsfs.h     |    6 +-
 fs/cifs/cifsglob.h   |   28 +--
 fs/cifs/cifsproto.h  |    6 +-
 fs/cifs/cifssmb.c    |    4 +-
 fs/cifs/dir.c        |   60 +-----
 fs/cifs/file.c       |  570 +++++++++++++++++++-------------------------------
 fs/cifs/inode.c      |   15 +-
 fs/cifs/misc.c       |   18 +--
 fs/cifs/readdir.c    |    6 +-
 11 files changed, 261 insertions(+), 474 deletions(-)

-- 
1.7.2.3

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 01/15] cifs: keep dentry reference in cifsFileInfo instead of inode reference
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2010-10-08 17:30   ` Jeff Layton
  2010-10-08 17:30   ` [PATCH 02/15] cifs: don't use vfsmount to pin superblock for oplock breaks Jeff Layton
                     ` (13 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:30 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

cifsFileInfo is a bit problematic. It contains a reference back to the
struct file itself. This makes it difficult for a cifsFileInfo to exist
without a corresponding struct file.

It would be better instead of the cifsFileInfo just held info pertaining
to the open file on the server instead without any back refrences to the
struct file. This would allow it to exist after the filp to which it was
originally attached was closed.

Much of the use of the file pointer in this struct is to get at the
dentry.  Begin divorcing the cifsFileInfo from the struct file by
keeping a reference to the dentry. Since the dentry will have a
reference to the inode, we can eliminate the "pInode" field too and
convert the igrab/iput to dget/dput.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
Acked-by: Dave Kleikamp <shaggy-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 fs/cifs/cifsglob.h |    4 ++--
 fs/cifs/dir.c      |    3 ++-
 fs/cifs/file.c     |    2 +-
 fs/cifs/misc.c     |    2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index eef5df1..8fa4ccf 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -387,7 +387,7 @@ struct cifsFileInfo {
 	/* BB add lock scope info here if needed */ ;
 	/* lock scope id (0 if none) */
 	struct file *pfile; /* needed for writepage */
-	struct inode *pInode; /* needed for oplock break */
+	struct dentry *dentry;
 	struct vfsmount *mnt;
 	struct tcon_link *tlink;
 	struct mutex lock_mutex;
@@ -412,7 +412,7 @@ static inline void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 {
 	if (atomic_dec_and_test(&cifs_file->count)) {
 		cifs_put_tlink(cifs_file->tlink);
-		iput(cifs_file->pInode);
+		dput(cifs_file->dentry);
 		kfree(cifs_file);
 	}
 }
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index e249b56..6887c41 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -135,6 +135,7 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
 		  struct vfsmount *mnt, struct tcon_link *tlink,
 		  unsigned int oflags, __u32 oplock)
 {
+	struct dentry *dentry = file->f_path.dentry;
 	struct cifsFileInfo *pCifsFile;
 	struct cifsInodeInfo *pCifsInode;
 
@@ -145,7 +146,7 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
 	pCifsFile->netfid = fileHandle;
 	pCifsFile->pid = current->tgid;
 	pCifsFile->uid = current_fsuid();
-	pCifsFile->pInode = igrab(newinode);
+	pCifsFile->dentry = dget(dentry);
 	pCifsFile->mnt = mnt;
 	pCifsFile->pfile = file;
 	pCifsFile->invalidHandle = false;
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 80856f1..c302b9c 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2335,7 +2335,7 @@ void cifs_oplock_break(struct work_struct *work)
 {
 	struct cifsFileInfo *cfile = container_of(work, struct cifsFileInfo,
 						  oplock_break);
-	struct inode *inode = cfile->pInode;
+	struct inode *inode = cfile->dentry->d_inode;
 	struct cifsInodeInfo *cinode = CIFS_I(inode);
 	int rc, waitrc = 0;
 
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 252f276..9bac3e7 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -578,7 +578,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
 				}
 
 				cFYI(1, "file id match, oplock break");
-				pCifsInode = CIFS_I(netfile->pInode);
+				pCifsInode = CIFS_I(netfile->dentry->d_inode);
 				pCifsInode->clientCanCacheAll = false;
 				if (pSMB->OplockLevel == 0)
 					pCifsInode->clientCanCacheRead = false;
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 02/15] cifs: don't use vfsmount to pin superblock for oplock breaks
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  2010-10-08 17:30   ` [PATCH 01/15] cifs: keep dentry reference in cifsFileInfo instead of inode reference Jeff Layton
@ 2010-10-08 17:30   ` Jeff Layton
  2010-10-08 17:31   ` [PATCH 03/15] cifs: eliminate cifs_posix_open_inode_helper Jeff Layton
                     ` (12 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:30 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

Filesystems aren't really supposed to do anything with a vfsmount. It's
considered a layering violation since vfsmounts are entirely managed at
the VFS layer.

CIFS currently keeps an active reference to a vfsmount in order to
prevent the superblock vanishing before an oplock break has completed.
What we really want to do instead is to keep sb->s_active high until the
oplock break has completed. This patch borrows the scheme that NFS uses
for handling sillyrenames.

An atomic_t is added to the cifs_sb_info. When it transitions from 0 to
1, an extra reference to the superblock is taken (by bumping the
s_active value). When it transitions from 1 to 0, that reference is
dropped and a the superblock teardown may proceed if there are no more
references to it.

Also, the vfsmount pointer is removed from cifsFileInfo and from
cifs_new_fileinfo, and some bogus forward declarations are removed from
cifsfs.h.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
Acked-by: Dave Kleikamp <shaggy-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 fs/cifs/cifs_fs_sb.h |    1 +
 fs/cifs/cifsfs.c     |   18 ++++++++++++++++++
 fs/cifs/cifsfs.h     |    6 ++----
 fs/cifs/cifsglob.h   |    1 -
 fs/cifs/cifsproto.h  |    2 +-
 fs/cifs/dir.c        |   10 +++-------
 fs/cifs/file.c       |    9 ++++-----
 7 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
index 586ee3d..525ba59 100644
--- a/fs/cifs/cifs_fs_sb.h
+++ b/fs/cifs/cifs_fs_sb.h
@@ -48,6 +48,7 @@ struct cifs_sb_info {
 	struct nls_table *local_nls;
 	unsigned int rsize;
 	unsigned int wsize;
+	atomic_t active;
 	uid_t	mnt_uid;
 	gid_t	mnt_gid;
 	mode_t	mnt_file_mode;
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 1aca2d7..3facf25 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -83,6 +83,24 @@ extern mempool_t *cifs_sm_req_poolp;
 extern mempool_t *cifs_req_poolp;
 extern mempool_t *cifs_mid_poolp;
 
+void
+cifs_sb_active(struct super_block *sb)
+{
+	struct cifs_sb_info *server = CIFS_SB(sb);
+
+	if (atomic_inc_return(&server->active) == 1)
+		atomic_inc(&sb->s_active);
+}
+
+void
+cifs_sb_deactive(struct super_block *sb)
+{
+	struct cifs_sb_info *server = CIFS_SB(sb);
+
+	if (atomic_dec_and_test(&server->active))
+		deactivate_super(sb);
+}
+
 static int
 cifs_read_super(struct super_block *sb, void *data,
 		const char *devname, int silent)
diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 786bdf3..85544bc 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -42,10 +42,8 @@ extern const struct address_space_operations cifs_addr_ops;
 extern const struct address_space_operations cifs_addr_ops_smallbuf;
 
 /* Functions related to super block operations */
-/* extern const struct super_operations cifs_super_ops;*/
-extern void cifs_read_inode(struct inode *);
-/*extern void cifs_delete_inode(struct inode *);*/  /* BB not needed yet */
-/* extern void cifs_write_inode(struct inode *); */ /* BB not needed yet */
+extern void cifs_sb_active(struct super_block *sb);
+extern void cifs_sb_deactive(struct super_block *sb);
 
 /* Functions related to inodes */
 extern const struct inode_operations cifs_dir_inode_ops;
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 8fa4ccf..1e24ddc 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -388,7 +388,6 @@ struct cifsFileInfo {
 	/* lock scope id (0 if none) */
 	struct file *pfile; /* needed for writepage */
 	struct dentry *dentry;
-	struct vfsmount *mnt;
 	struct tcon_link *tlink;
 	struct mutex lock_mutex;
 	struct list_head llist; /* list of byte range locks we have. */
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 29a2ee8..7f416ab 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -107,7 +107,7 @@ extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time,
 
 extern struct cifsFileInfo *cifs_new_fileinfo(struct inode *newinode,
 				__u16 fileHandle, struct file *file,
-				struct vfsmount *mnt, struct tcon_link *tlink,
+				struct tcon_link *tlink,
 				unsigned int oflags, __u32 oplock);
 extern int cifs_posix_open(char *full_path, struct inode **pinode,
 				struct super_block *sb,
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 6887c41..c205ec9 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -132,8 +132,7 @@ cifs_bp_rename_retry:
 
 struct cifsFileInfo *
 cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
-		  struct vfsmount *mnt, struct tcon_link *tlink,
-		  unsigned int oflags, __u32 oplock)
+		  struct tcon_link *tlink, unsigned int oflags, __u32 oplock)
 {
 	struct dentry *dentry = file->f_path.dentry;
 	struct cifsFileInfo *pCifsFile;
@@ -147,7 +146,6 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
 	pCifsFile->pid = current->tgid;
 	pCifsFile->uid = current_fsuid();
 	pCifsFile->dentry = dget(dentry);
-	pCifsFile->mnt = mnt;
 	pCifsFile->pfile = file;
 	pCifsFile->invalidHandle = false;
 	pCifsFile->closePend = false;
@@ -485,8 +483,7 @@ cifs_create_set_dentry:
 		}
 
 		pfile_info = cifs_new_fileinfo(newinode, fileHandle, filp,
-						nd->path.mnt, tlink, oflags,
-						oplock);
+						tlink, oflags, oplock);
 		if (pfile_info == NULL) {
 			fput(filp);
 			CIFSSMBClose(xid, tcon, fileHandle);
@@ -760,8 +757,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 			}
 
 			cfile = cifs_new_fileinfo(newInode, fileHandle, filp,
-						  nd->path.mnt, tlink,
-						  nd->intent.open.flags,
+						  tlink, nd->intent.open.flags,
 						  oplock);
 			if (cfile == NULL) {
 				fput(filp);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index c302b9c..fd78a35 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -282,7 +282,6 @@ int cifs_open(struct inode *inode, struct file *file)
 			}
 
 			pCifsFile = cifs_new_fileinfo(inode, netfid, file,
-							file->f_path.mnt,
 							tlink, oflags, oplock);
 			if (pCifsFile == NULL) {
 				CIFSSMBClose(xid, tcon, netfid);
@@ -375,8 +374,8 @@ int cifs_open(struct inode *inode, struct file *file)
 	if (rc != 0)
 		goto out;
 
-	pCifsFile = cifs_new_fileinfo(inode, netfid, file, file->f_path.mnt,
-					tlink, file->f_flags, oplock);
+	pCifsFile = cifs_new_fileinfo(inode, netfid, file, tlink,
+					file->f_flags, oplock);
 	if (pCifsFile == NULL) {
 		rc = -ENOMEM;
 		goto out;
@@ -2381,14 +2380,14 @@ void cifs_oplock_break(struct work_struct *work)
 
 void cifs_oplock_break_get(struct cifsFileInfo *cfile)
 {
-	mntget(cfile->mnt);
+	cifs_sb_active(cfile->dentry->d_sb);
 	cifsFileInfo_get(cfile);
 }
 
 void cifs_oplock_break_put(struct cifsFileInfo *cfile)
 {
-	mntput(cfile->mnt);
 	cifsFileInfo_put(cfile);
+	cifs_sb_deactive(cfile->dentry->d_sb);
 }
 
 const struct address_space_operations cifs_addr_ops = {
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 03/15] cifs: eliminate cifs_posix_open_inode_helper
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  2010-10-08 17:30   ` [PATCH 01/15] cifs: keep dentry reference in cifsFileInfo instead of inode reference Jeff Layton
  2010-10-08 17:30   ` [PATCH 02/15] cifs: don't use vfsmount to pin superblock for oplock breaks Jeff Layton
@ 2010-10-08 17:31   ` Jeff Layton
  2010-10-08 17:31   ` [PATCH 04/15] cifs: eliminate oflags option from cifs_new_fileinfo Jeff Layton
                     ` (11 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:31 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

This function is redundant. The only thing it does is set the canCache
flags, but those get set in cifs_new_fileinfo anyway.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
Acked-by: Dave Kleikamp <shaggy-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 fs/cifs/file.c |   67 --------------------------------------------------------
 1 files changed, 0 insertions(+), 67 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index fd78a35..c10f085 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -106,64 +106,6 @@ static inline int cifs_get_disposition(unsigned int flags)
 		return FILE_OPEN;
 }
 
-/* all arguments to this function must be checked for validity in caller */
-static inline int
-cifs_posix_open_inode_helper(struct inode *inode, struct file *file,
-			     struct cifsInodeInfo *pCifsInode, __u32 oplock,
-			     u16 netfid)
-{
-
-	write_lock(&GlobalSMBSeslock);
-
-	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
-	if (pCifsInode == NULL) {
-		write_unlock(&GlobalSMBSeslock);
-		return -EINVAL;
-	}
-
-	if (pCifsInode->clientCanCacheRead) {
-		/* we have the inode open somewhere else
-		   no need to discard cache data */
-		goto psx_client_can_cache;
-	}
-
-	/* BB FIXME need to fix this check to move it earlier into posix_open
-	   BB  fIX following section BB FIXME */
-
-	/* if not oplocked, invalidate inode pages if mtime or file
-	   size changed */
-/*	temp = cifs_NTtimeToUnix(le64_to_cpu(buf->LastWriteTime));
-	if (timespec_equal(&file->f_path.dentry->d_inode->i_mtime, &temp) &&
-			   (file->f_path.dentry->d_inode->i_size ==
-			    (loff_t)le64_to_cpu(buf->EndOfFile))) {
-		cFYI(1, "inode unchanged on server");
-	} else {
-		if (file->f_path.dentry->d_inode->i_mapping) {
-			rc = filemap_write_and_wait(file->f_path.dentry->d_inode->i_mapping);
-			if (rc != 0)
-				CIFS_I(file->f_path.dentry->d_inode)->write_behind_rc = rc;
-		}
-		cFYI(1, "invalidating remote inode since open detected it "
-			 "changed");
-		invalidate_remote_inode(file->f_path.dentry->d_inode);
-	} */
-
-psx_client_can_cache:
-	if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
-		pCifsInode->clientCanCacheAll = true;
-		pCifsInode->clientCanCacheRead = true;
-		cFYI(1, "Exclusive Oplock granted on inode %p",
-			 file->f_path.dentry->d_inode);
-	} else if ((oplock & 0xF) == OPLOCK_READ)
-		pCifsInode->clientCanCacheRead = true;
-
-	/* will have to change the unlock if we reenable the
-	   filemap_fdatawrite (which does not seem necessary */
-	write_unlock(&GlobalSMBSeslock);
-	return 0;
-}
-
-/* all arguments to this function must be checked for validity in caller */
 static inline int cifs_open_inode_helper(struct inode *inode,
 	struct cifsTconInfo *pTcon, __u32 oplock, FILE_ALL_INFO *buf,
 	char *full_path, int xid)
@@ -271,15 +213,6 @@ int cifs_open(struct inode *inode, struct file *file)
 				oflags, &oplock, &netfid, xid);
 		if (rc == 0) {
 			cFYI(1, "posix open succeeded");
-			/* no need for special case handling of setting mode
-			   on read only files needed here */
-
-			rc = cifs_posix_open_inode_helper(inode, file,
-					pCifsInode, oplock, netfid);
-			if (rc != 0) {
-				CIFSSMBClose(xid, tcon, netfid);
-				goto out;
-			}
 
 			pCifsFile = cifs_new_fileinfo(inode, netfid, file,
 							tlink, oflags, oplock);
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 04/15] cifs: eliminate oflags option from cifs_new_fileinfo
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (2 preceding siblings ...)
  2010-10-08 17:31   ` [PATCH 03/15] cifs: eliminate cifs_posix_open_inode_helper Jeff Layton
@ 2010-10-08 17:31   ` Jeff Layton
       [not found]     ` <1286559072-29032-5-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  2010-10-08 17:31   ` [PATCH 05/15] cifs: eliminate the inode argument " Jeff Layton
                     ` (10 subsequent siblings)
  14 siblings, 1 reply; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:31 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

Eliminate the poor, misunderstood "oflags" option from cifs_new_fileinfo.
The callers mostly pass in the filp->f_flags here, except for one place
that passes in the flags after they've been converted to their SMB_O_*
equivalents.

None of these are correct however since we're checking that value for
the presence of FMODE_READ. Luckily that only affects how the f_list is
ordered. What it really wants here is the file->f_mode, but this check
really makes no sense whatsoever. FMODE_READ will be set for O_RDWR or
O_RDONLY opens. So this in effect just moves those to the front of the
list and leaves O_WRONLY at the end.

That might make some sense if anything actually paid attention to this
list order, but nothing does. find_readable_file and find_writable_file
both walk through the list in the same direction.

Let's just keep this simple and just put things on the list with
list_add.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsproto.h |    3 +--
 fs/cifs/dir.c       |   15 ++++-----------
 fs/cifs/file.c      |    5 ++---
 3 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 7f416ab..bed004c 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -107,8 +107,7 @@ extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time,
 
 extern struct cifsFileInfo *cifs_new_fileinfo(struct inode *newinode,
 				__u16 fileHandle, struct file *file,
-				struct tcon_link *tlink,
-				unsigned int oflags, __u32 oplock);
+				struct tcon_link *tlink, __u32 oplock);
 extern int cifs_posix_open(char *full_path, struct inode **pinode,
 				struct super_block *sb,
 				int mode, int oflags,
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index c205ec9..452c9b5 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -132,7 +132,7 @@ cifs_bp_rename_retry:
 
 struct cifsFileInfo *
 cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
-		  struct tcon_link *tlink, unsigned int oflags, __u32 oplock)
+		  struct tcon_link *tlink, __u32 oplock)
 {
 	struct dentry *dentry = file->f_path.dentry;
 	struct cifsFileInfo *pCifsFile;
@@ -160,13 +160,7 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
 	list_add(&pCifsFile->tlist, &(tlink_tcon(tlink)->openFileList));
 	pCifsInode = CIFS_I(newinode);
 	if (pCifsInode) {
-		/* if readable file instance put first in list*/
-		if (oflags & FMODE_READ)
-			list_add(&pCifsFile->flist, &pCifsInode->openFileList);
-		else
-			list_add_tail(&pCifsFile->flist,
-				      &pCifsInode->openFileList);
-
+		list_add(&pCifsFile->flist, &pCifsInode->openFileList);
 		if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
 			pCifsInode->clientCanCacheAll = true;
 			pCifsInode->clientCanCacheRead = true;
@@ -483,7 +477,7 @@ cifs_create_set_dentry:
 		}
 
 		pfile_info = cifs_new_fileinfo(newinode, fileHandle, filp,
-						tlink, oflags, oplock);
+						tlink, oplock);
 		if (pfile_info == NULL) {
 			fput(filp);
 			CIFSSMBClose(xid, tcon, fileHandle);
@@ -757,8 +751,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 			}
 
 			cfile = cifs_new_fileinfo(newInode, fileHandle, filp,
-						  tlink, nd->intent.open.flags,
-						  oplock);
+						  tlink, oplock);
 			if (cfile == NULL) {
 				fput(filp);
 				CIFSSMBClose(xid, pTcon, fileHandle);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index c10f085..e863d80 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -215,7 +215,7 @@ int cifs_open(struct inode *inode, struct file *file)
 			cFYI(1, "posix open succeeded");
 
 			pCifsFile = cifs_new_fileinfo(inode, netfid, file,
-							tlink, oflags, oplock);
+							tlink, oplock);
 			if (pCifsFile == NULL) {
 				CIFSSMBClose(xid, tcon, netfid);
 				rc = -ENOMEM;
@@ -307,8 +307,7 @@ int cifs_open(struct inode *inode, struct file *file)
 	if (rc != 0)
 		goto out;
 
-	pCifsFile = cifs_new_fileinfo(inode, netfid, file, tlink,
-					file->f_flags, oplock);
+	pCifsFile = cifs_new_fileinfo(inode, netfid, file, tlink, oplock);
 	if (pCifsFile == NULL) {
 		rc = -ENOMEM;
 		goto out;
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 05/15] cifs: eliminate the inode argument from cifs_new_fileinfo
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (3 preceding siblings ...)
  2010-10-08 17:31   ` [PATCH 04/15] cifs: eliminate oflags option from cifs_new_fileinfo Jeff Layton
@ 2010-10-08 17:31   ` Jeff Layton
  2010-10-08 17:31   ` [PATCH 06/15] cifs: clean up cifs_reopen_file Jeff Layton
                     ` (9 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:31 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

It already takes a file pointer. The inode associated with that had damn
well better be the same one we're passing in anyway. Thus, there's no
need for a separate argument here.

Also, get rid of the bogus check for a null pCifsInode pointer. The
CIFS_I macro uses container_of(), and that will virtually never return a
NULL pointer anyway.

Finally, move the setting of the canCache* flags outside of the lock.
Other places in the code don't hold that lock when setting it, so I
assume it's not really needed here either.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
Acked-by: Dave Kleikamp <shaggy-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 fs/cifs/cifsproto.h |    3 +--
 fs/cifs/dir.c       |   30 ++++++++++++++----------------
 fs/cifs/file.c      |    6 +++---
 3 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index bed004c..a2bdfa2 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -105,8 +105,7 @@ extern u64 cifs_UnixTimeToNT(struct timespec);
 extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time,
 				      int offset);
 
-extern struct cifsFileInfo *cifs_new_fileinfo(struct inode *newinode,
-				__u16 fileHandle, struct file *file,
+extern struct cifsFileInfo *cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 				struct tcon_link *tlink, __u32 oplock);
 extern int cifs_posix_open(char *full_path, struct inode **pinode,
 				struct super_block *sb,
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 452c9b5..6c38a41 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -131,12 +131,13 @@ cifs_bp_rename_retry:
 }
 
 struct cifsFileInfo *
-cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
+cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 		  struct tcon_link *tlink, __u32 oplock)
 {
 	struct dentry *dentry = file->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+	struct cifsInodeInfo *pCifsInode = CIFS_I(inode);
 	struct cifsFileInfo *pCifsFile;
-	struct cifsInodeInfo *pCifsInode;
 
 	pCifsFile = kzalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
 	if (pCifsFile == NULL)
@@ -158,18 +159,16 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
 
 	write_lock(&GlobalSMBSeslock);
 	list_add(&pCifsFile->tlist, &(tlink_tcon(tlink)->openFileList));
-	pCifsInode = CIFS_I(newinode);
-	if (pCifsInode) {
-		list_add(&pCifsFile->flist, &pCifsInode->openFileList);
-		if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
-			pCifsInode->clientCanCacheAll = true;
-			pCifsInode->clientCanCacheRead = true;
-			cFYI(1, "Exclusive Oplock inode %p", newinode);
-		} else if ((oplock & 0xF) == OPLOCK_READ)
-				pCifsInode->clientCanCacheRead = true;
-	}
+	list_add(&pCifsFile->flist, &pCifsInode->openFileList);
 	write_unlock(&GlobalSMBSeslock);
 
+	if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
+		pCifsInode->clientCanCacheAll = true;
+		pCifsInode->clientCanCacheRead = true;
+		cFYI(1, "Exclusive Oplock inode %p", inode);
+	} else if ((oplock & 0xF) == OPLOCK_READ)
+		pCifsInode->clientCanCacheRead = true;
+
 	file->private_data = pCifsFile;
 
 	return pCifsFile;
@@ -476,8 +475,7 @@ cifs_create_set_dentry:
 			goto cifs_create_out;
 		}
 
-		pfile_info = cifs_new_fileinfo(newinode, fileHandle, filp,
-						tlink, oplock);
+		pfile_info = cifs_new_fileinfo(fileHandle, filp, tlink, oplock);
 		if (pfile_info == NULL) {
 			fput(filp);
 			CIFSSMBClose(xid, tcon, fileHandle);
@@ -750,8 +748,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 				goto lookup_out;
 			}
 
-			cfile = cifs_new_fileinfo(newInode, fileHandle, filp,
-						  tlink, oplock);
+			cfile = cifs_new_fileinfo(fileHandle, filp, tlink,
+						  oplock);
 			if (cfile == NULL) {
 				fput(filp);
 				CIFSSMBClose(xid, pTcon, fileHandle);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index e863d80..1edec19 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -214,8 +214,8 @@ int cifs_open(struct inode *inode, struct file *file)
 		if (rc == 0) {
 			cFYI(1, "posix open succeeded");
 
-			pCifsFile = cifs_new_fileinfo(inode, netfid, file,
-							tlink, oplock);
+			pCifsFile = cifs_new_fileinfo(netfid, file, tlink,
+						      oplock);
 			if (pCifsFile == NULL) {
 				CIFSSMBClose(xid, tcon, netfid);
 				rc = -ENOMEM;
@@ -307,7 +307,7 @@ int cifs_open(struct inode *inode, struct file *file)
 	if (rc != 0)
 		goto out;
 
-	pCifsFile = cifs_new_fileinfo(inode, netfid, file, tlink, oplock);
+	pCifsFile = cifs_new_fileinfo(netfid, file, tlink, oplock);
 	if (pCifsFile == NULL) {
 		rc = -ENOMEM;
 		goto out;
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 06/15] cifs: clean up cifs_reopen_file
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (4 preceding siblings ...)
  2010-10-08 17:31   ` [PATCH 05/15] cifs: eliminate the inode argument " Jeff Layton
@ 2010-10-08 17:31   ` Jeff Layton
  2010-10-08 17:31   ` [PATCH 07/15] cifs: cifs_write argument change and cleanup Jeff Layton
                     ` (8 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:31 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

Add a f_flags field that holds the f_flags field from the filp. We'll
need this info in case the filp ever goes away before the cifsFileInfo
does. Have cifs_reopen_file use that value instead of filp->f_flags
too and have it take a cifsFileInfo arg instead of a filp.

While we're at it, get rid of some bogus cargo-cult NULL pointer
checks in that function and reduce the level of indentation.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
Acked-by: Dave Kleikamp <shaggy-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 fs/cifs/cifsglob.h |    1 +
 fs/cifs/dir.c      |    1 +
 fs/cifs/file.c     |  128 ++++++++++++++++++++++------------------------------
 3 files changed, 56 insertions(+), 74 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 1e24ddc..3c8904e 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -388,6 +388,7 @@ struct cifsFileInfo {
 	/* lock scope id (0 if none) */
 	struct file *pfile; /* needed for writepage */
 	struct dentry *dentry;
+	unsigned int f_flags;
 	struct tcon_link *tlink;
 	struct mutex lock_mutex;
 	struct list_head llist; /* list of byte range locks we have. */
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 6c38a41..5fe1d89 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -147,6 +147,7 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 	pCifsFile->pid = current->tgid;
 	pCifsFile->uid = current_fsuid();
 	pCifsFile->dentry = dget(dentry);
+	pCifsFile->f_flags = file->f_flags;
 	pCifsFile->pfile = file;
 	pCifsFile->invalidHandle = false;
 	pCifsFile->closePend = false;
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 1edec19..db38547 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -354,14 +354,13 @@ static int cifs_relock_file(struct cifsFileInfo *cifsFile)
 	return rc;
 }
 
-static int cifs_reopen_file(struct file *file, bool can_flush)
+static int cifs_reopen_file(struct cifsFileInfo *pCifsFile, bool can_flush)
 {
 	int rc = -EACCES;
 	int xid;
 	__u32 oplock;
 	struct cifs_sb_info *cifs_sb;
 	struct cifsTconInfo *tcon;
-	struct cifsFileInfo *pCifsFile;
 	struct cifsInodeInfo *pCifsInode;
 	struct inode *inode;
 	char *full_path = NULL;
@@ -369,11 +368,6 @@ static int cifs_reopen_file(struct file *file, bool can_flush)
 	int disposition = FILE_OPEN;
 	__u16 netfid;
 
-	if (file->private_data)
-		pCifsFile = file->private_data;
-	else
-		return -EBADF;
-
 	xid = GetXid();
 	mutex_lock(&pCifsFile->fh_mutex);
 	if (!pCifsFile->invalidHandle) {
@@ -383,21 +377,7 @@ static int cifs_reopen_file(struct file *file, bool can_flush)
 		return rc;
 	}
 
-	if (file->f_path.dentry == NULL) {
-		cERROR(1, "no valid name if dentry freed");
-		dump_stack();
-		rc = -EBADF;
-		goto reopen_error_exit;
-	}
-
-	inode = file->f_path.dentry->d_inode;
-	if (inode == NULL) {
-		cERROR(1, "inode not valid");
-		dump_stack();
-		rc = -EBADF;
-		goto reopen_error_exit;
-	}
-
+	inode = pCifsFile->dentry->d_inode;
 	cifs_sb = CIFS_SB(inode->i_sb);
 	tcon = tlink_tcon(pCifsFile->tlink);
 
@@ -405,17 +385,16 @@ static int cifs_reopen_file(struct file *file, bool can_flush)
    those that already have the rename sem can end up causing writepage
    to get called and if the server was down that means we end up here,
    and we can never tell if the caller already has the rename_sem */
-	full_path = build_path_from_dentry(file->f_path.dentry);
+	full_path = build_path_from_dentry(pCifsFile->dentry);
 	if (full_path == NULL) {
 		rc = -ENOMEM;
-reopen_error_exit:
 		mutex_unlock(&pCifsFile->fh_mutex);
 		FreeXid(xid);
 		return rc;
 	}
 
 	cFYI(1, "inode = 0x%p file flags 0x%x for %s",
-		 inode, file->f_flags, full_path);
+		 inode, pCifsFile->f_flags, full_path);
 
 	if (oplockEnabled)
 		oplock = REQ_OPLOCK;
@@ -425,7 +404,7 @@ reopen_error_exit:
 	if (tcon->unix_ext && (tcon->ses->capabilities & CAP_UNIX) &&
 	    (CIFS_UNIX_POSIX_PATH_OPS_CAP &
 			le64_to_cpu(tcon->fsUnixInfo.Capability))) {
-		int oflags = (int) cifs_posix_convert_flags(file->f_flags);
+		int oflags = (int) cifs_posix_convert_flags(pCifsFile->f_flags);
 		/* can not refresh inode info since size could be stale */
 		rc = cifs_posix_open(full_path, NULL, inode->i_sb,
 				cifs_sb->mnt_file_mode /* ignored */,
@@ -438,7 +417,7 @@ reopen_error_exit:
 		   in the reconnect path it is important to retry hard */
 	}
 
-	desiredAccess = cifs_convert_flags(file->f_flags);
+	desiredAccess = cifs_convert_flags(pCifsFile->f_flags);
 
 	/* Can not refresh inode by passing in file_info buf to be returned
 	   by SMBOpen and then calling get_inode_info with returned buf
@@ -454,49 +433,50 @@ reopen_error_exit:
 		mutex_unlock(&pCifsFile->fh_mutex);
 		cFYI(1, "cifs_open returned 0x%x", rc);
 		cFYI(1, "oplock: %d", oplock);
-	} else {
+		goto reopen_error_exit;
+	}
+
 reopen_success:
-		pCifsFile->netfid = netfid;
-		pCifsFile->invalidHandle = false;
-		mutex_unlock(&pCifsFile->fh_mutex);
-		pCifsInode = CIFS_I(inode);
-		if (pCifsInode) {
-			if (can_flush) {
-				rc = filemap_write_and_wait(inode->i_mapping);
-				if (rc != 0)
-					CIFS_I(inode)->write_behind_rc = rc;
-			/* temporarily disable caching while we
-			   go to server to get inode info */
-				pCifsInode->clientCanCacheAll = false;
-				pCifsInode->clientCanCacheRead = false;
-				if (tcon->unix_ext)
-					rc = cifs_get_inode_info_unix(&inode,
-						full_path, inode->i_sb, xid);
-				else
-					rc = cifs_get_inode_info(&inode,
-						full_path, NULL, inode->i_sb,
-						xid, NULL);
-			} /* else we are writing out data to server already
-			     and could deadlock if we tried to flush data, and
-			     since we do not know if we have data that would
-			     invalidate the current end of file on the server
-			     we can not go to the server to get the new inod
-			     info */
-			if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
-				pCifsInode->clientCanCacheAll = true;
-				pCifsInode->clientCanCacheRead = true;
-				cFYI(1, "Exclusive Oplock granted on inode %p",
-					 file->f_path.dentry->d_inode);
-			} else if ((oplock & 0xF) == OPLOCK_READ) {
-				pCifsInode->clientCanCacheRead = true;
-				pCifsInode->clientCanCacheAll = false;
-			} else {
-				pCifsInode->clientCanCacheRead = false;
-				pCifsInode->clientCanCacheAll = false;
-			}
-			cifs_relock_file(pCifsFile);
-		}
+	pCifsFile->netfid = netfid;
+	pCifsFile->invalidHandle = false;
+	mutex_unlock(&pCifsFile->fh_mutex);
+	pCifsInode = CIFS_I(inode);
+
+	if (can_flush) {
+		rc = filemap_write_and_wait(inode->i_mapping);
+		if (rc != 0)
+			CIFS_I(inode)->write_behind_rc = rc;
+
+		pCifsInode->clientCanCacheAll = false;
+		pCifsInode->clientCanCacheRead = false;
+		if (tcon->unix_ext)
+			rc = cifs_get_inode_info_unix(&inode,
+				full_path, inode->i_sb, xid);
+		else
+			rc = cifs_get_inode_info(&inode,
+				full_path, NULL, inode->i_sb,
+				xid, NULL);
+	} /* else we are writing out data to server already
+	     and could deadlock if we tried to flush data, and
+	     since we do not know if we have data that would
+	     invalidate the current end of file on the server
+	     we can not go to the server to get the new inod
+	     info */
+	if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
+		pCifsInode->clientCanCacheAll = true;
+		pCifsInode->clientCanCacheRead = true;
+		cFYI(1, "Exclusive Oplock granted on inode %p",
+			 pCifsFile->dentry->d_inode);
+	} else if ((oplock & 0xF) == OPLOCK_READ) {
+		pCifsInode->clientCanCacheRead = true;
+		pCifsInode->clientCanCacheAll = false;
+	} else {
+		pCifsInode->clientCanCacheRead = false;
+		pCifsInode->clientCanCacheAll = false;
 	}
+	cifs_relock_file(pCifsFile);
+
+reopen_error_exit:
 	kfree(full_path);
 	FreeXid(xid);
 	return rc;
@@ -935,7 +915,7 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
 				   filemap_fdatawait from here so tell
 				   reopen_file not to flush data to server
 				   now */
-				rc = cifs_reopen_file(file, false);
+				rc = cifs_reopen_file(open_file, false);
 				if (rc != 0)
 					break;
 			}
@@ -1033,7 +1013,7 @@ static ssize_t cifs_write(struct file *file, const char *write_data,
 				   filemap_fdatawait from here so tell
 				   reopen_file not to flush data to
 				   server now */
-				rc = cifs_reopen_file(file, false);
+				rc = cifs_reopen_file(open_file, false);
 				if (rc != 0)
 					break;
 			}
@@ -1181,7 +1161,7 @@ refind_writable:
 
 			read_unlock(&GlobalSMBSeslock);
 			/* Had to unlock since following call can block */
-			rc = cifs_reopen_file(open_file->pfile, false);
+			rc = cifs_reopen_file(open_file, false);
 			if (!rc) {
 				if (!open_file->closePend)
 					return open_file;
@@ -1729,7 +1709,7 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
 			int buf_type = CIFS_NO_BUFFER;
 			if ((open_file->invalidHandle) &&
 			    (!open_file->closePend)) {
-				rc = cifs_reopen_file(file, true);
+				rc = cifs_reopen_file(open_file, true);
 				if (rc != 0)
 					break;
 			}
@@ -1815,7 +1795,7 @@ static ssize_t cifs_read(struct file *file, char *read_data, size_t read_size,
 		while (rc == -EAGAIN) {
 			if ((open_file->invalidHandle) &&
 			    (!open_file->closePend)) {
-				rc = cifs_reopen_file(file, true);
+				rc = cifs_reopen_file(open_file, true);
 				if (rc != 0)
 					break;
 			}
@@ -1980,7 +1960,7 @@ static int cifs_readpages(struct file *file, struct address_space *mapping,
 		while (rc == -EAGAIN) {
 			if ((open_file->invalidHandle) &&
 			    (!open_file->closePend)) {
-				rc = cifs_reopen_file(file, true);
+				rc = cifs_reopen_file(open_file, true);
 				if (rc != 0)
 					break;
 			}
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 07/15] cifs: cifs_write argument change and cleanup
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (5 preceding siblings ...)
  2010-10-08 17:31   ` [PATCH 06/15] cifs: clean up cifs_reopen_file Jeff Layton
@ 2010-10-08 17:31   ` Jeff Layton
  2010-10-08 17:31   ` [PATCH 08/15] cifs: eliminate pfile pointer from cifsFileInfo Jeff Layton
                     ` (7 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:31 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

Have cifs_write take a cifsFileInfo pointer instead of a filp. Since
cifsFileInfo holds references on the dentry, and that holds one to
the inode, we can eliminate some unneeded NULL pointer checks.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
Acked-by: Dave Kleikamp <shaggy-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 fs/cifs/file.c |   51 +++++++++++++++++----------------------------------
 1 files changed, 17 insertions(+), 34 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index db38547..57fbfad 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -963,8 +963,9 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
 	return total_written;
 }
 
-static ssize_t cifs_write(struct file *file, const char *write_data,
-			  size_t write_size, loff_t *poffset)
+static ssize_t cifs_write(struct cifsFileInfo *open_file,
+			  const char *write_data, size_t write_size,
+			  loff_t *poffset)
 {
 	int rc = 0;
 	unsigned int bytes_written = 0;
@@ -972,17 +973,14 @@ static ssize_t cifs_write(struct file *file, const char *write_data,
 	struct cifs_sb_info *cifs_sb;
 	struct cifsTconInfo *pTcon;
 	int xid, long_op;
-	struct cifsFileInfo *open_file;
-	struct cifsInodeInfo *cifsi = CIFS_I(file->f_path.dentry->d_inode);
+	struct dentry *dentry = open_file->dentry;
+	struct cifsInodeInfo *cifsi = CIFS_I(dentry->d_inode);
 
-	cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
+	cifs_sb = CIFS_SB(dentry->d_sb);
 
 	cFYI(1, "write %zd bytes to offset %lld of %s", write_size,
-	   *poffset, file->f_path.dentry->d_name.name);
+	   *poffset, dentry->d_name.name);
 
-	if (file->private_data == NULL)
-		return -EBADF;
-	open_file = file->private_data;
 	pTcon = tlink_tcon(open_file->tlink);
 
 	xid = GetXid();
@@ -992,15 +990,6 @@ static ssize_t cifs_write(struct file *file, const char *write_data,
 	     total_written += bytes_written) {
 		rc = -EAGAIN;
 		while (rc == -EAGAIN) {
-			if (file->private_data == NULL) {
-				/* file has been closed on us */
-				FreeXid(xid);
-			/* if we have gotten here we have written some data
-			   and blocked, and the file has been freed on us
-			   while we blocked so return what we managed to
-			   write */
-				return total_written;
-			}
 			if (open_file->closePend) {
 				FreeXid(xid);
 				if (total_written)
@@ -1060,20 +1049,13 @@ static ssize_t cifs_write(struct file *file, const char *write_data,
 
 	cifs_stats_bytes_written(pTcon, total_written);
 
-	/* since the write may have blocked check these pointers again */
-	if ((file->f_path.dentry) && (file->f_path.dentry->d_inode)) {
-/*BB We could make this contingent on superblock ATIME flag too */
-/*		file->f_path.dentry->d_inode->i_ctime =
-		file->f_path.dentry->d_inode->i_mtime = CURRENT_TIME;*/
-		if (total_written > 0) {
-			spin_lock(&file->f_path.dentry->d_inode->i_lock);
-			if (*poffset > file->f_path.dentry->d_inode->i_size)
-				i_size_write(file->f_path.dentry->d_inode,
-					     *poffset);
-			spin_unlock(&file->f_path.dentry->d_inode->i_lock);
-		}
-		mark_inode_dirty_sync(file->f_path.dentry->d_inode);
+	if (total_written > 0) {
+		spin_lock(&dentry->d_inode->i_lock);
+		if (*poffset > dentry->d_inode->i_size)
+			i_size_write(dentry->d_inode, *poffset);
+		spin_unlock(&dentry->d_inode->i_lock);
 	}
+	mark_inode_dirty_sync(dentry->d_inode);
 	FreeXid(xid);
 	return total_written;
 }
@@ -1244,8 +1226,8 @@ static int cifs_partialpagewrite(struct page *page, unsigned from, unsigned to)
 
 	open_file = find_writable_file(CIFS_I(mapping->host), false);
 	if (open_file) {
-		bytes_written = cifs_write(open_file->pfile, write_data,
-					   to-from, &offset);
+		bytes_written = cifs_write(open_file, write_data,
+					   to - from, &offset);
 		cifsFileInfo_put(open_file);
 		/* Does mm or vfs already set times? */
 		inode->i_atime = inode->i_mtime = current_fs_time(inode->i_sb);
@@ -1560,7 +1542,8 @@ static int cifs_write_end(struct file *file, struct address_space *mapping,
 		/* BB check if anything else missing out of ppw
 		   such as updating last write time */
 		page_data = kmap(page);
-		rc = cifs_write(file, page_data + offset, copied, &pos);
+		rc = cifs_write(file->private_data, page_data + offset,
+				copied, &pos);
 		/* if (rc < 0) should we set writebehind rc? */
 		kunmap(page);
 
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 08/15] cifs: eliminate pfile pointer from cifsFileInfo
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (6 preceding siblings ...)
  2010-10-08 17:31   ` [PATCH 07/15] cifs: cifs_write argument change and cleanup Jeff Layton
@ 2010-10-08 17:31   ` Jeff Layton
  2010-10-08 17:31   ` [PATCH 09/15] cifs: move cifs_new_fileinfo to file.c Jeff Layton
                     ` (6 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:31 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

All the remaining users of cifsFileInfo->pfile just use it to get
at the f_flags/f_mode. Now that we store that separately in the
cifsFileInfo, there's no need to consult the pfile at all from
a cifsFileInfo pointer.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
Acked-by: Dave Kleikamp <shaggy-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 fs/cifs/cifsglob.h |    1 -
 fs/cifs/dir.c      |    1 -
 fs/cifs/file.c     |   11 +++--------
 3 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 3c8904e..6dcd911 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -386,7 +386,6 @@ struct cifsFileInfo {
 	__u16 netfid;		/* file id from remote */
 	/* BB add lock scope info here if needed */ ;
 	/* lock scope id (0 if none) */
-	struct file *pfile; /* needed for writepage */
 	struct dentry *dentry;
 	unsigned int f_flags;
 	struct tcon_link *tlink;
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 5fe1d89..0ad8eef 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -148,7 +148,6 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 	pCifsFile->uid = current_fsuid();
 	pCifsFile->dentry = dget(dentry);
 	pCifsFile->f_flags = file->f_flags;
-	pCifsFile->pfile = file;
 	pCifsFile->invalidHandle = false;
 	pCifsFile->closePend = false;
 	pCifsFile->tlink = cifs_get_tlink(tlink);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 57fbfad..83e16d6 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1080,8 +1080,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
 			continue;
 		if (fsuid_only && open_file->uid != current_fsuid())
 			continue;
-		if (open_file->pfile && ((open_file->pfile->f_flags & O_RDWR) ||
-		    (open_file->pfile->f_flags & O_RDONLY))) {
+		if (OPEN_FMODE(open_file->f_flags) & FMODE_READ) {
 			if (!open_file->invalidHandle) {
 				/* found a good file */
 				/* lock it so it will not be closed on us */
@@ -1130,9 +1129,7 @@ refind_writable:
 			continue;
 		if (fsuid_only && open_file->uid != current_fsuid())
 			continue;
-		if (open_file->pfile &&
-		    ((open_file->pfile->f_flags & O_RDWR) ||
-		     (open_file->pfile->f_flags & O_WRONLY))) {
+		if (OPEN_FMODE(open_file->f_flags) & FMODE_WRITE) {
 			cifsFileInfo_get(open_file);
 
 			if (!open_file->invalidHandle) {
@@ -2096,9 +2093,7 @@ static int is_inode_writable(struct cifsInodeInfo *cifs_inode)
 	list_for_each_entry(open_file, &cifs_inode->openFileList, flist) {
 		if (open_file->closePend)
 			continue;
-		if (open_file->pfile &&
-		    ((open_file->pfile->f_flags & O_RDWR) ||
-		     (open_file->pfile->f_flags & O_WRONLY))) {
+		if (OPEN_FMODE(open_file->f_flags) & FMODE_WRITE) {
 			read_unlock(&GlobalSMBSeslock);
 			return 1;
 		}
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 09/15] cifs: move cifs_new_fileinfo to file.c
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (7 preceding siblings ...)
  2010-10-08 17:31   ` [PATCH 08/15] cifs: eliminate pfile pointer from cifsFileInfo Jeff Layton
@ 2010-10-08 17:31   ` Jeff Layton
  2010-10-08 17:31   ` [PATCH 10/15] cifs: convert GlobalSMBSeslock from a rwlock to regular spinlock Jeff Layton
                     ` (5 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:31 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

It's currently in dir.c which makes little sense...

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
Acked-by: Dave Kleikamp <shaggy-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 fs/cifs/dir.c  |   44 --------------------------------------------
 fs/cifs/file.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 44 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 0ad8eef..983c483 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -130,50 +130,6 @@ cifs_bp_rename_retry:
 	return full_path;
 }
 
-struct cifsFileInfo *
-cifs_new_fileinfo(__u16 fileHandle, struct file *file,
-		  struct tcon_link *tlink, __u32 oplock)
-{
-	struct dentry *dentry = file->f_path.dentry;
-	struct inode *inode = dentry->d_inode;
-	struct cifsInodeInfo *pCifsInode = CIFS_I(inode);
-	struct cifsFileInfo *pCifsFile;
-
-	pCifsFile = kzalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
-	if (pCifsFile == NULL)
-		return pCifsFile;
-
-	pCifsFile->netfid = fileHandle;
-	pCifsFile->pid = current->tgid;
-	pCifsFile->uid = current_fsuid();
-	pCifsFile->dentry = dget(dentry);
-	pCifsFile->f_flags = file->f_flags;
-	pCifsFile->invalidHandle = false;
-	pCifsFile->closePend = false;
-	pCifsFile->tlink = cifs_get_tlink(tlink);
-	mutex_init(&pCifsFile->fh_mutex);
-	mutex_init(&pCifsFile->lock_mutex);
-	INIT_LIST_HEAD(&pCifsFile->llist);
-	atomic_set(&pCifsFile->count, 1);
-	INIT_WORK(&pCifsFile->oplock_break, cifs_oplock_break);
-
-	write_lock(&GlobalSMBSeslock);
-	list_add(&pCifsFile->tlist, &(tlink_tcon(tlink)->openFileList));
-	list_add(&pCifsFile->flist, &pCifsInode->openFileList);
-	write_unlock(&GlobalSMBSeslock);
-
-	if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
-		pCifsInode->clientCanCacheAll = true;
-		pCifsInode->clientCanCacheRead = true;
-		cFYI(1, "Exclusive Oplock inode %p", inode);
-	} else if ((oplock & 0xF) == OPLOCK_READ)
-		pCifsInode->clientCanCacheRead = true;
-
-	file->private_data = pCifsFile;
-
-	return pCifsFile;
-}
-
 int cifs_posix_open(char *full_path, struct inode **pinode,
 			struct super_block *sb, int mode, int oflags,
 			__u32 *poplock, __u16 *pnetfid, int xid)
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 83e16d6..86a1597 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -159,6 +159,49 @@ client_can_cache:
 	return rc;
 }
 
+struct cifsFileInfo *
+cifs_new_fileinfo(__u16 fileHandle, struct file *file,
+		  struct tcon_link *tlink, __u32 oplock)
+{
+	struct dentry *dentry = file->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+	struct cifsInodeInfo *pCifsInode = CIFS_I(inode);
+	struct cifsFileInfo *pCifsFile;
+
+	pCifsFile = kzalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
+	if (pCifsFile == NULL)
+		return pCifsFile;
+
+	pCifsFile->netfid = fileHandle;
+	pCifsFile->pid = current->tgid;
+	pCifsFile->uid = current_fsuid();
+	pCifsFile->dentry = dget(dentry);
+	pCifsFile->f_flags = file->f_flags;
+	pCifsFile->invalidHandle = false;
+	pCifsFile->closePend = false;
+	pCifsFile->tlink = cifs_get_tlink(tlink);
+	mutex_init(&pCifsFile->fh_mutex);
+	mutex_init(&pCifsFile->lock_mutex);
+	INIT_LIST_HEAD(&pCifsFile->llist);
+	atomic_set(&pCifsFile->count, 1);
+	INIT_WORK(&pCifsFile->oplock_break, cifs_oplock_break);
+
+	write_lock(&GlobalSMBSeslock);
+	list_add(&pCifsFile->tlist, &(tlink_tcon(tlink)->openFileList));
+	list_add(&pCifsFile->flist, &pCifsInode->openFileList);
+	write_unlock(&GlobalSMBSeslock);
+
+	if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
+		pCifsInode->clientCanCacheAll = true;
+		pCifsInode->clientCanCacheRead = true;
+		cFYI(1, "Exclusive Oplock inode %p", inode);
+	} else if ((oplock & 0xF) == OPLOCK_READ)
+		pCifsInode->clientCanCacheRead = true;
+
+	file->private_data = pCifsFile;
+	return pCifsFile;
+}
+
 int cifs_open(struct inode *inode, struct file *file)
 {
 	int rc = -EACCES;
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 10/15] cifs: convert GlobalSMBSeslock from a rwlock to regular spinlock
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (8 preceding siblings ...)
  2010-10-08 17:31   ` [PATCH 09/15] cifs: move cifs_new_fileinfo to file.c Jeff Layton
@ 2010-10-08 17:31   ` Jeff Layton
       [not found]     ` <1286559072-29032-11-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  2010-10-08 17:31   ` [PATCH 11/15] cifs: move cifsFileInfo_put to file.c Jeff Layton
                     ` (4 subsequent siblings)
  14 siblings, 1 reply; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:31 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

Convert this lock to a regular spinlock

A rwlock_t offers little value here. It's more expensive than a regular
spinlock unless you have a fairly large section of code that runs under
the read lock and can benefit from the concurrency.

Additionally, we need to ensure that the refcounting for files isn't
racy and to do that we need to lock areas that can increment it for
write. That means that the areas that can actually use a read_lock are
very few and relatively infrequently used.

While we're at it, change the name to something easier to type, and fix
a bug in find_writable_file. cifsFileInfo_put can sleep and shouldn't be
called while holding the lock.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsfs.c   |    2 +-
 fs/cifs/cifsglob.h |    2 +-
 fs/cifs/cifssmb.c  |    4 +-
 fs/cifs/file.c     |   54 ++++++++++++++++++++++++++--------------------------
 fs/cifs/misc.c     |    8 +++---
 fs/cifs/readdir.c  |    6 ++--
 6 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 3facf25..c25e928 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -942,8 +942,8 @@ init_cifs(void)
 	GlobalTotalActiveXid = 0;
 	GlobalMaxActiveXid = 0;
 	memset(Local_System_Name, 0, 15);
-	rwlock_init(&GlobalSMBSeslock);
 	rwlock_init(&cifs_tcp_ses_lock);
+	spin_lock_init(&cifs_file_list_lock);
 	spin_lock_init(&GlobalMid_Lock);
 
 	if (cifs_max_pending < 2) {
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 6dcd911..c29ef5f 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -720,7 +720,7 @@ GLOBAL_EXTERN rwlock_t		cifs_tcp_ses_lock;
  * 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 spinlock_t	cifs_file_list_lock;
 
 /* Outstanding dir notify requests */
 GLOBAL_EXTERN struct list_head GlobalDnotifyReqList;
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 54bd83a..d0aed27 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -91,13 +91,13 @@ static void mark_open_files_invalid(struct cifsTconInfo *pTcon)
 	struct list_head *tmp1;
 
 /* list all files open on tree connection and mark them invalid */
-	write_lock(&GlobalSMBSeslock);
+	spin_lock(&cifs_file_list_lock);
 	list_for_each_safe(tmp, tmp1, &pTcon->openFileList) {
 		open_file = list_entry(tmp, struct cifsFileInfo, tlist);
 		open_file->invalidHandle = true;
 		open_file->oplock_break_cancelled = true;
 	}
-	write_unlock(&GlobalSMBSeslock);
+	spin_unlock(&cifs_file_list_lock);
 	/* BB Add call to invalidate_inodes(sb) for all superblocks mounted
 	   to this tcon */
 }
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 86a1597..b6b88fe 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -186,10 +186,10 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 	atomic_set(&pCifsFile->count, 1);
 	INIT_WORK(&pCifsFile->oplock_break, cifs_oplock_break);
 
-	write_lock(&GlobalSMBSeslock);
+	spin_lock(&cifs_file_list_lock);
 	list_add(&pCifsFile->tlist, &(tlink_tcon(tlink)->openFileList));
 	list_add(&pCifsFile->flist, &pCifsInode->openFileList);
-	write_unlock(&GlobalSMBSeslock);
+	spin_unlock(&cifs_file_list_lock);
 
 	if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
 		pCifsInode->clientCanCacheAll = true;
@@ -539,13 +539,13 @@ int cifs_close(struct inode *inode, struct file *file)
 	pTcon = tlink_tcon(pSMBFile->tlink);
 	if (pSMBFile) {
 		struct cifsLockInfo *li, *tmp;
-		write_lock(&GlobalSMBSeslock);
+		spin_lock(&cifs_file_list_lock);
 		pSMBFile->closePend = true;
 		if (pTcon) {
 			/* no sense reconnecting to close a file that is
 			   already closed */
 			if (!pTcon->need_reconnect) {
-				write_unlock(&GlobalSMBSeslock);
+				spin_unlock(&cifs_file_list_lock);
 				timeout = 2;
 				while ((atomic_read(&pSMBFile->count) != 1)
 					&& (timeout <= 2048)) {
@@ -565,9 +565,9 @@ int cifs_close(struct inode *inode, struct file *file)
 					rc = CIFSSMBClose(xid, pTcon,
 						  pSMBFile->netfid);
 			} else
-				write_unlock(&GlobalSMBSeslock);
+				spin_unlock(&cifs_file_list_lock);
 		} else
-			write_unlock(&GlobalSMBSeslock);
+			spin_unlock(&cifs_file_list_lock);
 
 		/* Delete any outstanding lock records.
 		   We'll lose them when the file is closed anyway. */
@@ -578,16 +578,16 @@ int cifs_close(struct inode *inode, struct file *file)
 		}
 		mutex_unlock(&pSMBFile->lock_mutex);
 
-		write_lock(&GlobalSMBSeslock);
+		spin_lock(&cifs_file_list_lock);
 		list_del(&pSMBFile->flist);
 		list_del(&pSMBFile->tlist);
-		write_unlock(&GlobalSMBSeslock);
+		spin_unlock(&cifs_file_list_lock);
 		cifsFileInfo_put(file->private_data);
 		file->private_data = NULL;
 	} else
 		rc = -EBADF;
 
-	read_lock(&GlobalSMBSeslock);
+	spin_lock(&cifs_file_list_lock);
 	if (list_empty(&(CIFS_I(inode)->openFileList))) {
 		cFYI(1, "closing last open instance for inode %p", inode);
 		/* if the file is not open we do not know if we can cache info
@@ -595,7 +595,7 @@ int cifs_close(struct inode *inode, struct file *file)
 		CIFS_I(inode)->clientCanCacheRead = false;
 		CIFS_I(inode)->clientCanCacheAll  = false;
 	}
-	read_unlock(&GlobalSMBSeslock);
+	spin_unlock(&cifs_file_list_lock);
 	if ((rc == 0) && CIFS_I(inode)->write_behind_rc)
 		rc = CIFS_I(inode)->write_behind_rc;
 	FreeXid(xid);
@@ -617,18 +617,18 @@ int cifs_closedir(struct inode *inode, struct file *file)
 		struct cifsTconInfo *pTcon = tlink_tcon(pCFileStruct->tlink);
 
 		cFYI(1, "Freeing private data in close dir");
-		write_lock(&GlobalSMBSeslock);
+		spin_lock(&cifs_file_list_lock);
 		if (!pCFileStruct->srch_inf.endOfSearch &&
 		    !pCFileStruct->invalidHandle) {
 			pCFileStruct->invalidHandle = true;
-			write_unlock(&GlobalSMBSeslock);
+			spin_unlock(&cifs_file_list_lock);
 			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);
+			spin_unlock(&cifs_file_list_lock);
 		ptmp = pCFileStruct->srch_inf.ntwrk_buf_start;
 		if (ptmp) {
 			cFYI(1, "closedir free smb buf in srch struct");
@@ -1114,7 +1114,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
 	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER))
 		fsuid_only = false;
 
-	read_lock(&GlobalSMBSeslock);
+	spin_lock(&cifs_file_list_lock);
 	/* we could simply get the first_list_entry since write-only entries
 	   are always at the end of the list but since the first entry might
 	   have a close pending, we go through the whole list */
@@ -1128,7 +1128,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
 				/* found a good file */
 				/* lock it so it will not be closed on us */
 				cifsFileInfo_get(open_file);
-				read_unlock(&GlobalSMBSeslock);
+				spin_unlock(&cifs_file_list_lock);
 				return open_file;
 			} /* else might as well continue, and look for
 			     another, or simply have the caller reopen it
@@ -1136,7 +1136,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
 		} else /* write only file */
 			break; /* write only files are last so must be done */
 	}
-	read_unlock(&GlobalSMBSeslock);
+	spin_unlock(&cifs_file_list_lock);
 	return NULL;
 }
 #endif
@@ -1163,7 +1163,7 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
 	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER))
 		fsuid_only = false;
 
-	read_lock(&GlobalSMBSeslock);
+	spin_lock(&cifs_file_list_lock);
 refind_writable:
 	list_for_each_entry(open_file, &cifs_inode->openFileList, flist) {
 		if (open_file->closePend)
@@ -1177,11 +1177,11 @@ refind_writable:
 
 			if (!open_file->invalidHandle) {
 				/* found a good writable file */
-				read_unlock(&GlobalSMBSeslock);
+				spin_unlock(&cifs_file_list_lock);
 				return open_file;
 			}
 
-			read_unlock(&GlobalSMBSeslock);
+			spin_unlock(&cifs_file_list_lock);
 			/* Had to unlock since following call can block */
 			rc = cifs_reopen_file(open_file, false);
 			if (!rc) {
@@ -1189,7 +1189,7 @@ refind_writable:
 					return open_file;
 				else { /* start over in case this was deleted */
 				       /* since the list could be modified */
-					read_lock(&GlobalSMBSeslock);
+					spin_lock(&cifs_file_list_lock);
 					cifsFileInfo_put(open_file);
 					goto refind_writable;
 				}
@@ -1203,7 +1203,7 @@ refind_writable:
 			to hold up writepages here (rather than
 			in caller) with continuous retries */
 			cFYI(1, "wp failed on reopen file");
-			read_lock(&GlobalSMBSeslock);
+			spin_lock(&cifs_file_list_lock);
 			/* can not use this handle, no write
 			   pending on this one after all */
 			cifsFileInfo_put(open_file);
@@ -1224,7 +1224,7 @@ refind_writable:
 		any_available = true;
 		goto refind_writable;
 	}
-	read_unlock(&GlobalSMBSeslock);
+	spin_unlock(&cifs_file_list_lock);
 	return NULL;
 }
 
@@ -2132,16 +2132,16 @@ static int is_inode_writable(struct cifsInodeInfo *cifs_inode)
 {
 	struct cifsFileInfo *open_file;
 
-	read_lock(&GlobalSMBSeslock);
+	spin_lock(&cifs_file_list_lock);
 	list_for_each_entry(open_file, &cifs_inode->openFileList, flist) {
 		if (open_file->closePend)
 			continue;
 		if (OPEN_FMODE(open_file->f_flags) & FMODE_WRITE) {
-			read_unlock(&GlobalSMBSeslock);
+			spin_unlock(&cifs_file_list_lock);
 			return 1;
 		}
 	}
-	read_unlock(&GlobalSMBSeslock);
+	spin_unlock(&cifs_file_list_lock);
 	return 0;
 }
 
@@ -2305,8 +2305,8 @@ void cifs_oplock_break(struct work_struct *work)
 	 * finished grabbing reference for us.  Make sure it's done by
 	 * waiting for GlobalSMSSeslock.
 	 */
-	write_lock(&GlobalSMBSeslock);
-	write_unlock(&GlobalSMBSeslock);
+	spin_lock(&cifs_file_list_lock);
+	spin_unlock(&cifs_file_list_lock);
 
 	cifs_oplock_break_put(cfile);
 }
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 9bac3e7..de6073c 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -560,7 +560,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
 				continue;
 
 			cifs_stats_inc(&tcon->num_oplock_brks);
-			read_lock(&GlobalSMBSeslock);
+			spin_lock(&cifs_file_list_lock);
 			list_for_each(tmp2, &tcon->openFileList) {
 				netfile = list_entry(tmp2, struct cifsFileInfo,
 						     tlist);
@@ -572,7 +572,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
 				 * closed anyway.
 				 */
 				if (netfile->closePend) {
-					read_unlock(&GlobalSMBSeslock);
+					spin_unlock(&cifs_file_list_lock);
 					read_unlock(&cifs_tcp_ses_lock);
 					return true;
 				}
@@ -594,11 +594,11 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
 					cifs_oplock_break_get(netfile);
 				netfile->oplock_break_cancelled = false;
 
-				read_unlock(&GlobalSMBSeslock);
+				spin_unlock(&cifs_file_list_lock);
 				read_unlock(&cifs_tcp_ses_lock);
 				return true;
 			}
-			read_unlock(&GlobalSMBSeslock);
+			spin_unlock(&cifs_file_list_lock);
 			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 078c625..dbc1c64 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -529,14 +529,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);
+		spin_lock(&cifs_file_list_lock);
 		if (!cifsFile->srch_inf.endOfSearch &&
 		    !cifsFile->invalidHandle) {
 			cifsFile->invalidHandle = true;
-			write_unlock(&GlobalSMBSeslock);
+			spin_unlock(&cifs_file_list_lock);
 			CIFSFindClose(xid, pTcon, cifsFile->netfid);
 		} else
-			write_unlock(&GlobalSMBSeslock);
+			spin_unlock(&cifs_file_list_lock);
 		if (cifsFile->srch_inf.ntwrk_buf_start) {
 			cFYI(1, "freeing SMB ff cache buf on search rewind");
 			if (cifsFile->srch_inf.smallBuf)
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 11/15] cifs: move cifsFileInfo_put to file.c
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (9 preceding siblings ...)
  2010-10-08 17:31   ` [PATCH 10/15] cifs: convert GlobalSMBSeslock from a rwlock to regular spinlock Jeff Layton
@ 2010-10-08 17:31   ` Jeff Layton
  2010-10-08 17:31   ` [PATCH 12/15] cifs: move close processing from cifs_close to cifsFileInfo_put Jeff Layton
                     ` (3 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:31 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

...and make it non-inlined in preparation for the move of most of
cifs_close to it.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
---
 fs/cifs/cifsglob.h |   10 +---------
 fs/cifs/file.c     |   10 ++++++++++
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index c29ef5f..01e2f56 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -406,15 +406,7 @@ static inline void cifsFileInfo_get(struct cifsFileInfo *cifs_file)
 	atomic_inc(&cifs_file->count);
 }
 
-/* Release a reference on the file private data */
-static inline void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
-{
-	if (atomic_dec_and_test(&cifs_file->count)) {
-		cifs_put_tlink(cifs_file->tlink);
-		dput(cifs_file->dentry);
-		kfree(cifs_file);
-	}
-}
+void cifsFileInfo_put(struct cifsFileInfo *cifs_file);
 
 /*
  * One of these for each file inode
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index b6b88fe..2518711 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -202,6 +202,16 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 	return pCifsFile;
 }
 
+/* Release a reference on the file private data */
+void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
+{
+	if (atomic_dec_and_test(&cifs_file->count)) {
+		cifs_put_tlink(cifs_file->tlink);
+		dput(cifs_file->dentry);
+		kfree(cifs_file);
+	}
+}
+
 int cifs_open(struct inode *inode, struct file *file)
 {
 	int rc = -EACCES;
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 12/15] cifs: move close processing from cifs_close to cifsFileInfo_put
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (10 preceding siblings ...)
  2010-10-08 17:31   ` [PATCH 11/15] cifs: move cifsFileInfo_put to file.c Jeff Layton
@ 2010-10-08 17:31   ` Jeff Layton
  2010-10-08 17:31   ` [PATCH 13/15] cifs: convert cifsFileInfo->count to non-atomic counter Jeff Layton
                     ` (2 subsequent siblings)
  14 siblings, 0 replies; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:31 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

Now that it's feasible for a cifsFileInfo to outlive the filp under
which it was created, move the close processing into cifsFileInfo_put.

This means that the last user of the filehandle always does the actual
on the wire close call. This also allows us to get rid of the closePend
flag from cifsFileInfo. If we have an active reference to the file
then it's never going to have a close pending.

cifs_close is converted to simply put the filehandle.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
---
 fs/cifs/cifsglob.h |    1 -
 fs/cifs/file.c     |  187 +++++++++++++++++-----------------------------------
 fs/cifs/misc.c     |   10 ---
 3 files changed, 60 insertions(+), 138 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 01e2f56..c928e39 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -391,7 +391,6 @@ struct cifsFileInfo {
 	struct tcon_link *tlink;
 	struct mutex lock_mutex;
 	struct list_head llist; /* list of byte range locks we have. */
-	bool closePend:1;	/* file is marked to close */
 	bool invalidHandle:1;	/* file closed via session abend */
 	bool oplock_break_cancelled:1;
 	atomic_t count;		/* reference count */
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 2518711..8cedcd7 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -178,7 +178,6 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 	pCifsFile->dentry = dget(dentry);
 	pCifsFile->f_flags = file->f_flags;
 	pCifsFile->invalidHandle = false;
-	pCifsFile->closePend = false;
 	pCifsFile->tlink = cifs_get_tlink(tlink);
 	mutex_init(&pCifsFile->fh_mutex);
 	mutex_init(&pCifsFile->lock_mutex);
@@ -202,14 +201,55 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 	return pCifsFile;
 }
 
-/* Release a reference on the file private data */
+/*
+ * Release a reference on the file private data. This may involve closing
+ * the filehandle out on the server.
+ */
 void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 {
-	if (atomic_dec_and_test(&cifs_file->count)) {
-		cifs_put_tlink(cifs_file->tlink);
-		dput(cifs_file->dentry);
-		kfree(cifs_file);
+	struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
+	struct cifsInodeInfo *cifsi = CIFS_I(cifs_file->dentry->d_inode);
+	struct cifsLockInfo *li, *tmp;
+
+	spin_lock(&cifs_file_list_lock);
+	if (!atomic_dec_and_test(&cifs_file->count)) {
+		spin_unlock(&cifs_file_list_lock);
+		return;
+	}
+
+	/* remove it from the lists */
+	list_del(&cifs_file->flist);
+	list_del(&cifs_file->tlist);
+
+	if (list_empty(&cifsi->openFileList)) {
+		cFYI(1, "closing last open instance for inode %p",
+			cifs_file->dentry->d_inode);
+		cifsi->clientCanCacheRead = false;
+		cifsi->clientCanCacheAll  = false;
+	}
+	spin_unlock(&cifs_file_list_lock);
+
+	if (!tcon->need_reconnect && !cifs_file->invalidHandle) {
+		int xid, rc;
+
+		xid = GetXid();
+		rc = CIFSSMBClose(xid, tcon, cifs_file->netfid);
+		FreeXid(xid);
+	}
+
+	/* Delete any outstanding lock records. We'll lose them when the file
+	 * is closed anyway.
+	 */
+	mutex_lock(&cifs_file->lock_mutex);
+	list_for_each_entry_safe(li, tmp, &cifs_file->llist, llist) {
+		list_del(&li->llist);
+		kfree(li);
 	}
+	mutex_unlock(&cifs_file->lock_mutex);
+
+	cifs_put_tlink(cifs_file->tlink);
+	dput(cifs_file->dentry);
+	kfree(cifs_file);
 }
 
 int cifs_open(struct inode *inode, struct file *file)
@@ -537,79 +577,11 @@ reopen_error_exit:
 
 int cifs_close(struct inode *inode, struct file *file)
 {
-	int rc = 0;
-	int xid, timeout;
-	struct cifs_sb_info *cifs_sb;
-	struct cifsTconInfo *pTcon;
-	struct cifsFileInfo *pSMBFile = file->private_data;
-
-	xid = GetXid();
-
-	cifs_sb = CIFS_SB(inode->i_sb);
-	pTcon = tlink_tcon(pSMBFile->tlink);
-	if (pSMBFile) {
-		struct cifsLockInfo *li, *tmp;
-		spin_lock(&cifs_file_list_lock);
-		pSMBFile->closePend = true;
-		if (pTcon) {
-			/* no sense reconnecting to close a file that is
-			   already closed */
-			if (!pTcon->need_reconnect) {
-				spin_unlock(&cifs_file_list_lock);
-				timeout = 2;
-				while ((atomic_read(&pSMBFile->count) != 1)
-					&& (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
-					increase the memory utilization as
-					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 (!pTcon->need_reconnect &&
-				    !pSMBFile->invalidHandle)
-					rc = CIFSSMBClose(xid, pTcon,
-						  pSMBFile->netfid);
-			} else
-				spin_unlock(&cifs_file_list_lock);
-		} else
-			spin_unlock(&cifs_file_list_lock);
-
-		/* Delete any outstanding lock records.
-		   We'll lose them when the file is closed anyway. */
-		mutex_lock(&pSMBFile->lock_mutex);
-		list_for_each_entry_safe(li, tmp, &pSMBFile->llist, llist) {
-			list_del(&li->llist);
-			kfree(li);
-		}
-		mutex_unlock(&pSMBFile->lock_mutex);
+	cifsFileInfo_put(file->private_data);
+	file->private_data = NULL;
 
-		spin_lock(&cifs_file_list_lock);
-		list_del(&pSMBFile->flist);
-		list_del(&pSMBFile->tlist);
-		spin_unlock(&cifs_file_list_lock);
-		cifsFileInfo_put(file->private_data);
-		file->private_data = NULL;
-	} else
-		rc = -EBADF;
-
-	spin_lock(&cifs_file_list_lock);
-	if (list_empty(&(CIFS_I(inode)->openFileList))) {
-		cFYI(1, "closing last open instance for inode %p", inode);
-		/* if the file is not open we do not know if we can cache info
-		   on this inode, much less write behind and read ahead */
-		CIFS_I(inode)->clientCanCacheRead = false;
-		CIFS_I(inode)->clientCanCacheAll  = false;
-	}
-	spin_unlock(&cifs_file_list_lock);
-	if ((rc == 0) && CIFS_I(inode)->write_behind_rc)
-		rc = CIFS_I(inode)->write_behind_rc;
-	FreeXid(xid);
-	return rc;
+	/* return code from the ->release op is always ignored */
+	return 0;
 }
 
 int cifs_closedir(struct inode *inode, struct file *file)
@@ -956,13 +928,6 @@ ssize_t cifs_user_write(struct file *file, const char __user *write_data,
 			   we blocked so return what we managed to write */
 				return total_written;
 			}
-			if (open_file->closePend) {
-				FreeXid(xid);
-				if (total_written)
-					return total_written;
-				else
-					return -EBADF;
-			}
 			if (open_file->invalidHandle) {
 				/* we could deadlock if we called
 				   filemap_fdatawait from here so tell
@@ -1043,13 +1008,6 @@ static ssize_t cifs_write(struct cifsFileInfo *open_file,
 	     total_written += bytes_written) {
 		rc = -EAGAIN;
 		while (rc == -EAGAIN) {
-			if (open_file->closePend) {
-				FreeXid(xid);
-				if (total_written)
-					return total_written;
-				else
-					return -EBADF;
-			}
 			if (open_file->invalidHandle) {
 				/* we could deadlock if we called
 				   filemap_fdatawait from here so tell
@@ -1129,8 +1087,6 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
 	   are always at the end of the list but since the first entry might
 	   have a close pending, we go through the whole list */
 	list_for_each_entry(open_file, &cifs_inode->openFileList, flist) {
-		if (open_file->closePend)
-			continue;
 		if (fsuid_only && open_file->uid != current_fsuid())
 			continue;
 		if (OPEN_FMODE(open_file->f_flags) & FMODE_READ) {
@@ -1176,8 +1132,6 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
 	spin_lock(&cifs_file_list_lock);
 refind_writable:
 	list_for_each_entry(open_file, &cifs_inode->openFileList, flist) {
-		if (open_file->closePend)
-			continue;
 		if (!any_available && open_file->pid != current->tgid)
 			continue;
 		if (fsuid_only && open_file->uid != current_fsuid())
@@ -1192,34 +1146,18 @@ refind_writable:
 			}
 
 			spin_unlock(&cifs_file_list_lock);
+
 			/* Had to unlock since following call can block */
 			rc = cifs_reopen_file(open_file, false);
-			if (!rc) {
-				if (!open_file->closePend)
-					return open_file;
-				else { /* start over in case this was deleted */
-				       /* since the list could be modified */
-					spin_lock(&cifs_file_list_lock);
-					cifsFileInfo_put(open_file);
-					goto refind_writable;
-				}
-			}
+			if (!rc)
+				return open_file;
 
-			/* if it fails, try another handle if possible -
-			(we can not do this if closePending since
-			loop could be modified - in which case we
-			have to start at the beginning of the list
-			again. Note that it would be bad
-			to hold up writepages here (rather than
-			in caller) with continuous retries */
+			/* if it fails, try another handle if possible */
 			cFYI(1, "wp failed on reopen file");
-			spin_lock(&cifs_file_list_lock);
-			/* can not use this handle, no write
-			   pending on this one after all */
 			cifsFileInfo_put(open_file);
 
-			if (open_file->closePend) /* list could have changed */
-				goto refind_writable;
+			spin_lock(&cifs_file_list_lock);
+
 			/* else we simply continue to the next entry. Thus
 			   we do not loop on reopen errors.  If we
 			   can not reopen the file, for example if we
@@ -1740,8 +1678,7 @@ ssize_t cifs_user_read(struct file *file, char __user *read_data,
 		smb_read_data = NULL;
 		while (rc == -EAGAIN) {
 			int buf_type = CIFS_NO_BUFFER;
-			if ((open_file->invalidHandle) &&
-			    (!open_file->closePend)) {
+			if (open_file->invalidHandle) {
 				rc = cifs_reopen_file(open_file, true);
 				if (rc != 0)
 					break;
@@ -1826,8 +1763,7 @@ static ssize_t cifs_read(struct file *file, char *read_data, size_t read_size,
 		}
 		rc = -EAGAIN;
 		while (rc == -EAGAIN) {
-			if ((open_file->invalidHandle) &&
-			    (!open_file->closePend)) {
+			if (open_file->invalidHandle) {
 				rc = cifs_reopen_file(open_file, true);
 				if (rc != 0)
 					break;
@@ -1991,8 +1927,7 @@ static int cifs_readpages(struct file *file, struct address_space *mapping,
 				read_size, contig_pages);
 		rc = -EAGAIN;
 		while (rc == -EAGAIN) {
-			if ((open_file->invalidHandle) &&
-			    (!open_file->closePend)) {
+			if (open_file->invalidHandle) {
 				rc = cifs_reopen_file(open_file, true);
 				if (rc != 0)
 					break;
@@ -2144,8 +2079,6 @@ static int is_inode_writable(struct cifsInodeInfo *cifs_inode)
 
 	spin_lock(&cifs_file_list_lock);
 	list_for_each_entry(open_file, &cifs_inode->openFileList, flist) {
-		if (open_file->closePend)
-			continue;
 		if (OPEN_FMODE(open_file->f_flags) & FMODE_WRITE) {
 			spin_unlock(&cifs_file_list_lock);
 			return 1;
@@ -2304,7 +2237,7 @@ void cifs_oplock_break(struct work_struct *work)
 	 * not bother sending an oplock release if session to server still is
 	 * disconnected since oplock already released by the server
 	 */
-	if (!cfile->closePend && !cfile->oplock_break_cancelled) {
+	if (!cfile->oplock_break_cancelled) {
 		rc = CIFSSMBLock(0, tlink_tcon(cfile->tlink), cfile->netfid, 0,
 				 0, 0, 0, LOCKING_ANDX_OPLOCK_RELEASE, false);
 		cFYI(1, "Oplock release rc = %d", rc);
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index de6073c..b86b423 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -567,16 +567,6 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
 				if (pSMB->Fid != netfile->netfid)
 					continue;
 
-				/*
-				 * don't do anything if file is about to be
-				 * closed anyway.
-				 */
-				if (netfile->closePend) {
-					spin_unlock(&cifs_file_list_lock);
-					read_unlock(&cifs_tcp_ses_lock);
-					return true;
-				}
-
 				cFYI(1, "file id match, oplock break");
 				pCifsInode = CIFS_I(netfile->dentry->d_inode);
 				pCifsInode->clientCanCacheAll = false;
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 13/15] cifs: convert cifsFileInfo->count to non-atomic counter
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (11 preceding siblings ...)
  2010-10-08 17:31   ` [PATCH 12/15] cifs: move close processing from cifs_close to cifsFileInfo_put Jeff Layton
@ 2010-10-08 17:31   ` Jeff Layton
       [not found]     ` <1286559072-29032-14-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  2010-10-08 17:31   ` [PATCH 14/15] cifs: wait for writeback to complete in cifs_flush Jeff Layton
  2010-10-08 17:31   ` [PATCH 15/15] cifs: eliminate cifsInodeInfo->write_behind_rc Jeff Layton
  14 siblings, 1 reply; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:31 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

The count for cifsFileInfo is currently an atomic, but that just adds
complexity for little value. We generally need to hold cifs_file_list_lock
to traverse the lists anyway so we might as well make this counter
non-atomic and simply use the cifs_file_list_lock to protect it.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifsglob.h |    9 ++++++---
 fs/cifs/file.c     |    8 +++++---
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index c928e39..2327dcb 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -393,16 +393,19 @@ struct cifsFileInfo {
 	struct list_head llist; /* list of byte range locks we have. */
 	bool invalidHandle:1;	/* file closed via session abend */
 	bool oplock_break_cancelled:1;
-	atomic_t count;		/* reference count */
+	int count;		/* refcount -- protected by cifs_file_list_lock */
 	struct mutex fh_mutex; /* prevents reopen race after dead ses*/
 	struct cifs_search_info srch_inf;
 	struct work_struct oplock_break; /* work for oplock breaks */
 };
 
-/* Take a reference on the file private data */
+/*
+ * Take a reference on the file private data. Must be called with
+ * cifs_file_list_lock held.
+ */
 static inline void cifsFileInfo_get(struct cifsFileInfo *cifs_file)
 {
-	atomic_inc(&cifs_file->count);
+	++cifs_file->count;
 }
 
 void cifsFileInfo_put(struct cifsFileInfo *cifs_file);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 8cedcd7..7b8122b 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -172,6 +172,7 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 	if (pCifsFile == NULL)
 		return pCifsFile;
 
+	pCifsFile->count = 1;
 	pCifsFile->netfid = fileHandle;
 	pCifsFile->pid = current->tgid;
 	pCifsFile->uid = current_fsuid();
@@ -182,7 +183,6 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 	mutex_init(&pCifsFile->fh_mutex);
 	mutex_init(&pCifsFile->lock_mutex);
 	INIT_LIST_HEAD(&pCifsFile->llist);
-	atomic_set(&pCifsFile->count, 1);
 	INIT_WORK(&pCifsFile->oplock_break, cifs_oplock_break);
 
 	spin_lock(&cifs_file_list_lock);
@@ -203,7 +203,8 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
 
 /*
  * Release a reference on the file private data. This may involve closing
- * the filehandle out on the server.
+ * the filehandle out on the server. Must be called without holding
+ * cifs_file_list_lock.
  */
 void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 {
@@ -212,7 +213,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	struct cifsLockInfo *li, *tmp;
 
 	spin_lock(&cifs_file_list_lock);
-	if (!atomic_dec_and_test(&cifs_file->count)) {
+	if (--cifs_file->count > 0) {
 		spin_unlock(&cifs_file_list_lock);
 		return;
 	}
@@ -2254,6 +2255,7 @@ void cifs_oplock_break(struct work_struct *work)
 	cifs_oplock_break_put(cfile);
 }
 
+/* must be called while holding cifs_file_list_lock */
 void cifs_oplock_break_get(struct cifsFileInfo *cfile)
 {
 	cifs_sb_active(cfile->dentry->d_sb);
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 14/15] cifs: wait for writeback to complete in cifs_flush
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (12 preceding siblings ...)
  2010-10-08 17:31   ` [PATCH 13/15] cifs: convert cifsFileInfo->count to non-atomic counter Jeff Layton
@ 2010-10-08 17:31   ` Jeff Layton
  2010-10-08 17:31   ` [PATCH 15/15] cifs: eliminate cifsInodeInfo->write_behind_rc Jeff Layton
  14 siblings, 0 replies; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:31 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

The f_op->flush operation is the last chance to return a writeback
related error when closing a file. Ensure that we don't miss reporting
any errors by waiting for writeback to complete in cifs_flush before
proceeding.

There's no reason to do this when the file isn't open for write
however.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
---
 fs/cifs/file.c |   21 +++++++--------------
 1 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 7b8122b..ce57953 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1620,20 +1620,13 @@ int cifs_flush(struct file *file, fl_owner_t id)
 	struct inode *inode = file->f_path.dentry->d_inode;
 	int rc = 0;
 
-	/* Rather than do the steps manually:
-	   lock the inode for writing
-	   loop through pages looking for write behind data (dirty pages)
-	   coalesce into contiguous 16K (or smaller) chunks to write to server
-	   send to server (prefer in parallel)
-	   deal with writebehind errors
-	   unlock inode for writing
-	   filemapfdatawrite appears easier for the time being */
-
-	rc = filemap_fdatawrite(inode->i_mapping);
-	/* reset wb rc if we were able to write out dirty pages */
-	if (!rc) {
-		rc = CIFS_I(inode)->write_behind_rc;
-		CIFS_I(inode)->write_behind_rc = 0;
+	if (file->f_mode & FMODE_WRITE) {
+		rc = filemap_write_and_wait(inode->i_mapping);
+		/* reset wb rc if we were able to write out dirty pages */
+		if (!rc) {
+			rc = CIFS_I(inode)->write_behind_rc;
+			CIFS_I(inode)->write_behind_rc = 0;
+		}
 	}
 
 	cFYI(1, "Flush inode %p file %p rc %d", inode, file, rc);
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 15/15] cifs: eliminate cifsInodeInfo->write_behind_rc
       [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
                     ` (13 preceding siblings ...)
  2010-10-08 17:31   ` [PATCH 14/15] cifs: wait for writeback to complete in cifs_flush Jeff Layton
@ 2010-10-08 17:31   ` Jeff Layton
  14 siblings, 0 replies; 25+ messages in thread
From: Jeff Layton @ 2010-10-08 17:31 UTC (permalink / raw)
  To: smfrench-Re5JQEeQqe8AvxtiuMwx3w; +Cc: linux-cifs-u79uwXL29TY76Z2rM5mHXA

write_behind_rc is redundant and just adds complexity to the code. What
we really want to do instead is to use mapping_set_error to reset the
flags on the mapping when we find a writeback error and can't report it
to userspace yet.

For cifs_flush and cifs_fsync, we shouldn't reset the flags since errors
returned there do get reported to userspace.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
---
 fs/cifs/cifsfs.c   |    1 -
 fs/cifs/cifsglob.h |    1 -
 fs/cifs/file.c     |   34 ++++++++--------------------------
 fs/cifs/inode.c    |   15 +++++----------
 4 files changed, 13 insertions(+), 38 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index c25e928..6e3e2e8 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -323,7 +323,6 @@ cifs_alloc_inode(struct super_block *sb)
 		return NULL;
 	cifs_inode->cifsAttrs = 0x20;	/* default */
 	cifs_inode->time = 0;
-	cifs_inode->write_behind_rc = 0;
 	/* Until the file is open and we have gotten oplock
 	info back from the server, can not assume caching of
 	file data or metadata */
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 2327dcb..78db6f8 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -418,7 +418,6 @@ struct cifsInodeInfo {
 	struct list_head lockList;
 	/* BB add in lists for dirty pages i.e. write caching info for oplock */
 	struct list_head openFileList;
-	int write_behind_rc;
 	__u32 cifsAttrs; /* e.g. DOS archive bit, sparse, compressed, system */
 	unsigned long time;	/* jiffies of last update/check of inode */
 	bool clientCanCacheRead:1;	/* read oplock */
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index ce57953..8d13299 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -133,8 +133,7 @@ static inline int cifs_open_inode_helper(struct inode *inode,
 			/* BB no need to lock inode until after invalidate
 			since namei code should already have it locked? */
 			rc = filemap_write_and_wait(inode->i_mapping);
-			if (rc != 0)
-				pCifsInode->write_behind_rc = rc;
+			mapping_set_error(inode->i_mapping, rc);
 		}
 		cFYI(1, "invalidating remote inode since open detected it "
 			 "changed");
@@ -538,8 +537,7 @@ reopen_success:
 
 	if (can_flush) {
 		rc = filemap_write_and_wait(inode->i_mapping);
-		if (rc != 0)
-			CIFS_I(inode)->write_behind_rc = rc;
+		mapping_set_error(inode->i_mapping, rc);
 
 		pCifsInode->clientCanCacheAll = false;
 		pCifsInode->clientCanCacheRead = false;
@@ -1421,12 +1419,7 @@ retry:
 			if (rc || bytes_written < bytes_to_write) {
 				cERROR(1, "Write2 ret %d, wrote %d",
 					  rc, bytes_written);
-				/* BB what if continued retry is
-				   requested via mount flags? */
-				if (rc == -ENOSPC)
-					set_bit(AS_ENOSPC, &mapping->flags);
-				else
-					set_bit(AS_EIO, &mapping->flags);
+				mapping_set_error(mapping, rc);
 			} else {
 				cifs_stats_bytes_written(tcon, bytes_written);
 			}
@@ -1571,10 +1564,8 @@ int cifs_fsync(struct file *file, int datasync)
 
 	rc = filemap_write_and_wait(inode->i_mapping);
 	if (rc == 0) {
-		rc = CIFS_I(inode)->write_behind_rc;
-		CIFS_I(inode)->write_behind_rc = 0;
 		tcon = tlink_tcon(smbfile->tlink);
-		if (!rc && tcon && smbfile &&
+		if (rc == 0 &&
 		   !(CIFS_SB(inode->i_sb)->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
 			rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
 	}
@@ -1620,14 +1611,8 @@ int cifs_flush(struct file *file, fl_owner_t id)
 	struct inode *inode = file->f_path.dentry->d_inode;
 	int rc = 0;
 
-	if (file->f_mode & FMODE_WRITE) {
+	if (file->f_mode & FMODE_WRITE)
 		rc = filemap_write_and_wait(inode->i_mapping);
-		/* reset wb rc if we were able to write out dirty pages */
-		if (!rc) {
-			rc = CIFS_I(inode)->write_behind_rc;
-			CIFS_I(inode)->write_behind_rc = 0;
-		}
-	}
 
 	cFYI(1, "Flush inode %p file %p rc %d", inode, file, rc);
 
@@ -2206,7 +2191,7 @@ void cifs_oplock_break(struct work_struct *work)
 						  oplock_break);
 	struct inode *inode = cfile->dentry->d_inode;
 	struct cifsInodeInfo *cinode = CIFS_I(inode);
-	int rc, waitrc = 0;
+	int rc = 0;
 
 	if (inode && S_ISREG(inode->i_mode)) {
 		if (cinode->clientCanCacheRead)
@@ -2215,13 +2200,10 @@ void cifs_oplock_break(struct work_struct *work)
 			break_lease(inode, O_WRONLY);
 		rc = filemap_fdatawrite(inode->i_mapping);
 		if (cinode->clientCanCacheRead == 0) {
-			waitrc = filemap_fdatawait(inode->i_mapping);
+			rc = filemap_fdatawait(inode->i_mapping);
+			mapping_set_error(inode->i_mapping, rc);
 			invalidate_remote_inode(inode);
 		}
-		if (!rc)
-			rc = waitrc;
-		if (rc)
-			cinode->write_behind_rc = rc;
 		cFYI(1, "Oplock flush inode %p rc %d", inode, rc);
 	}
 
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index cfe2339..ed5558b 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1688,8 +1688,7 @@ cifs_invalidate_mapping(struct inode *inode)
 	/* write back any cached data */
 	if (inode->i_mapping && inode->i_mapping->nrpages != 0) {
 		rc = filemap_write_and_wait(inode->i_mapping);
-		if (rc)
-			cifs_i->write_behind_rc = rc;
+		mapping_set_error(inode->i_mapping, rc);
 	}
 	invalidate_remote_inode(inode);
 	cifs_fscache_reset_inode_cookie(inode);
@@ -1949,10 +1948,8 @@ cifs_setattr_unix(struct dentry *direntry, struct iattr *attrs)
 	 * the flush returns error?
 	 */
 	rc = filemap_write_and_wait(inode->i_mapping);
-	if (rc != 0) {
-		cifsInode->write_behind_rc = rc;
-		rc = 0;
-	}
+	mapping_set_error(inode->i_mapping, rc);
+	rc = 0;
 
 	if (attrs->ia_valid & ATTR_SIZE) {
 		rc = cifs_set_file_size(inode, attrs, xid, full_path);
@@ -2093,10 +2090,8 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
 	 * the flush returns error?
 	 */
 	rc = filemap_write_and_wait(inode->i_mapping);
-	if (rc != 0) {
-		cifsInode->write_behind_rc = rc;
-		rc = 0;
-	}
+	mapping_set_error(inode->i_mapping, rc);
+	rc = 0;
 
 	if (attrs->ia_valid & ATTR_SIZE) {
 		rc = cifs_set_file_size(inode, attrs, xid, full_path);
-- 
1.7.2.3

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH 04/15] cifs: eliminate oflags option from cifs_new_fileinfo
       [not found]     ` <1286559072-29032-5-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2010-10-11  5:41       ` Suresh Jayaraman
       [not found]         ` <4CB2A392.9030400-l3A5Bk7waGM@public.gmane.org>
  0 siblings, 1 reply; 25+ messages in thread
From: Suresh Jayaraman @ 2010-10-11  5:41 UTC (permalink / raw)
  To: Jeff Layton
  Cc: smfrench-Re5JQEeQqe8AvxtiuMwx3w, linux-cifs-u79uwXL29TY76Z2rM5mHXA

On 10/08/2010 11:01 PM, Jeff Layton wrote:
> Eliminate the poor, misunderstood "oflags" option from cifs_new_fileinfo.
> The callers mostly pass in the filp->f_flags here, except for one place
> that passes in the flags after they've been converted to their SMB_O_*
> equivalents.
> 
> None of these are correct however since we're checking that value for
> the presence of FMODE_READ. Luckily that only affects how the f_list is
> ordered. What it really wants here is the file->f_mode, but this check
> really makes no sense whatsoever. FMODE_READ will be set for O_RDWR or
> O_RDONLY opens. So this in effect just moves those to the front of the
> list and leaves O_WRONLY at the end.
> 
> That might make some sense if anything actually paid attention to this
> list order, but nothing does. find_readable_file and find_writable_file
> both walk through the list in the same direction.
> 
> Let's just keep this simple and just put things on the list with
> list_add.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsproto.h |    3 +--
>  fs/cifs/dir.c       |   15 ++++-----------
>  fs/cifs/file.c      |    5 ++---
>  3 files changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 7f416ab..bed004c 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -107,8 +107,7 @@ extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time,
>  
>  extern struct cifsFileInfo *cifs_new_fileinfo(struct inode *newinode,
>  				__u16 fileHandle, struct file *file,
> -				struct tcon_link *tlink,
> -				unsigned int oflags, __u32 oplock);
> +				struct tcon_link *tlink, __u32 oplock);
>  extern int cifs_posix_open(char *full_path, struct inode **pinode,
>  				struct super_block *sb,
>  				int mode, int oflags,
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index c205ec9..452c9b5 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -132,7 +132,7 @@ cifs_bp_rename_retry:
>  
>  struct cifsFileInfo *
>  cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
> -		  struct tcon_link *tlink, unsigned int oflags, __u32 oplock)
> +		  struct tcon_link *tlink, __u32 oplock)
>  {
>  	struct dentry *dentry = file->f_path.dentry;
>  	struct cifsFileInfo *pCifsFile;
> @@ -160,13 +160,7 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
>  	list_add(&pCifsFile->tlist, &(tlink_tcon(tlink)->openFileList));
>  	pCifsInode = CIFS_I(newinode);
>  	if (pCifsInode) {
> -		/* if readable file instance put first in list*/
> -		if (oflags & FMODE_READ)
> -			list_add(&pCifsFile->flist, &pCifsInode->openFileList);
> -		else
> -			list_add_tail(&pCifsFile->flist,
> -				      &pCifsInode->openFileList);
> -
> +		list_add(&pCifsFile->flist, &pCifsInode->openFileList);

find_readable_file() assumes that if it is a write-only file, it will be
in the tail and it can break out of the loop. That needs to be fixed as
well?



-- 
Suresh Jayaraman

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 10/15] cifs: convert GlobalSMBSeslock from a rwlock to regular spinlock
       [not found]     ` <1286559072-29032-11-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2010-10-11  5:45       ` Suresh Jayaraman
       [not found]         ` <4CB2A478.50401-l3A5Bk7waGM@public.gmane.org>
  0 siblings, 1 reply; 25+ messages in thread
From: Suresh Jayaraman @ 2010-10-11  5:45 UTC (permalink / raw)
  To: Jeff Layton
  Cc: smfrench-Re5JQEeQqe8AvxtiuMwx3w, linux-cifs-u79uwXL29TY76Z2rM5mHXA

On 10/08/2010 11:01 PM, Jeff Layton wrote:
> Convert this lock to a regular spinlock
> 
> A rwlock_t offers little value here. It's more expensive than a regular
> spinlock unless you have a fairly large section of code that runs under
> the read lock and can benefit from the concurrency.
> 
> Additionally, we need to ensure that the refcounting for files isn't
> racy and to do that we need to lock areas that can increment it for
> write. That means that the areas that can actually use a read_lock are
> very few and relatively infrequently used.
> 
> While we're at it, change the name to something easier to type, and fix
> a bug in find_writable_file. cifsFileInfo_put can sleep and shouldn't be
> called while holding the lock.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c   |    2 +-
>  fs/cifs/cifsglob.h |    2 +-
>  fs/cifs/cifssmb.c  |    4 +-
>  fs/cifs/file.c     |   54 ++++++++++++++++++++++++++--------------------------
>  fs/cifs/misc.c     |    8 +++---
>  fs/cifs/readdir.c  |    6 ++--
>  6 files changed, 38 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 3facf25..c25e928 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -942,8 +942,8 @@ init_cifs(void)
>  	GlobalTotalActiveXid = 0;
>  	GlobalMaxActiveXid = 0;
>  	memset(Local_System_Name, 0, 15);
> -	rwlock_init(&GlobalSMBSeslock);
>  	rwlock_init(&cifs_tcp_ses_lock);
> +	spin_lock_init(&cifs_file_list_lock);
>  	spin_lock_init(&GlobalMid_Lock);
>  
>  	if (cifs_max_pending < 2) {
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 6dcd911..c29ef5f 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -720,7 +720,7 @@ GLOBAL_EXTERN rwlock_t		cifs_tcp_ses_lock;
>   * 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 spinlock_t	cifs_file_list_lock;
>  
>  /* Outstanding dir notify requests */
>  GLOBAL_EXTERN struct list_head GlobalDnotifyReqList;
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 54bd83a..d0aed27 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -91,13 +91,13 @@ static void mark_open_files_invalid(struct cifsTconInfo *pTcon)
>  	struct list_head *tmp1;
>  
>  /* list all files open on tree connection and mark them invalid */
> -	write_lock(&GlobalSMBSeslock);
> +	spin_lock(&cifs_file_list_lock);
>  	list_for_each_safe(tmp, tmp1, &pTcon->openFileList) {
>  		open_file = list_entry(tmp, struct cifsFileInfo, tlist);
>  		open_file->invalidHandle = true;
>  		open_file->oplock_break_cancelled = true;
>  	}
> -	write_unlock(&GlobalSMBSeslock);
> +	spin_unlock(&cifs_file_list_lock);
>  	/* BB Add call to invalidate_inodes(sb) for all superblocks mounted
>  	   to this tcon */
>  }
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 86a1597..b6b88fe 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -186,10 +186,10 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
>  	atomic_set(&pCifsFile->count, 1);
>  	INIT_WORK(&pCifsFile->oplock_break, cifs_oplock_break);
>  
> -	write_lock(&GlobalSMBSeslock);
> +	spin_lock(&cifs_file_list_lock);
>  	list_add(&pCifsFile->tlist, &(tlink_tcon(tlink)->openFileList));
>  	list_add(&pCifsFile->flist, &pCifsInode->openFileList);
> -	write_unlock(&GlobalSMBSeslock);
> +	spin_unlock(&cifs_file_list_lock);
>  
>  	if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
>  		pCifsInode->clientCanCacheAll = true;
> @@ -539,13 +539,13 @@ int cifs_close(struct inode *inode, struct file *file)
>  	pTcon = tlink_tcon(pSMBFile->tlink);
>  	if (pSMBFile) {
>  		struct cifsLockInfo *li, *tmp;
> -		write_lock(&GlobalSMBSeslock);
> +		spin_lock(&cifs_file_list_lock);
>  		pSMBFile->closePend = true;
>  		if (pTcon) {
>  			/* no sense reconnecting to close a file that is
>  			   already closed */
>  			if (!pTcon->need_reconnect) {
> -				write_unlock(&GlobalSMBSeslock);
> +				spin_unlock(&cifs_file_list_lock);
>  				timeout = 2;
>  				while ((atomic_read(&pSMBFile->count) != 1)
>  					&& (timeout <= 2048)) {
> @@ -565,9 +565,9 @@ int cifs_close(struct inode *inode, struct file *file)
>  					rc = CIFSSMBClose(xid, pTcon,
>  						  pSMBFile->netfid);
>  			} else
> -				write_unlock(&GlobalSMBSeslock);
> +				spin_unlock(&cifs_file_list_lock);
>  		} else
> -			write_unlock(&GlobalSMBSeslock);
> +			spin_unlock(&cifs_file_list_lock);
>  
>  		/* Delete any outstanding lock records.
>  		   We'll lose them when the file is closed anyway. */
> @@ -578,16 +578,16 @@ int cifs_close(struct inode *inode, struct file *file)
>  		}
>  		mutex_unlock(&pSMBFile->lock_mutex);
>  
> -		write_lock(&GlobalSMBSeslock);
> +		spin_lock(&cifs_file_list_lock);
>  		list_del(&pSMBFile->flist);
>  		list_del(&pSMBFile->tlist);
> -		write_unlock(&GlobalSMBSeslock);
> +		spin_unlock(&cifs_file_list_lock);
>  		cifsFileInfo_put(file->private_data);
>  		file->private_data = NULL;
>  	} else
>  		rc = -EBADF;
>  
> -	read_lock(&GlobalSMBSeslock);
> +	spin_lock(&cifs_file_list_lock);
>  	if (list_empty(&(CIFS_I(inode)->openFileList))) {
>  		cFYI(1, "closing last open instance for inode %p", inode);
>  		/* if the file is not open we do not know if we can cache info
> @@ -595,7 +595,7 @@ int cifs_close(struct inode *inode, struct file *file)
>  		CIFS_I(inode)->clientCanCacheRead = false;
>  		CIFS_I(inode)->clientCanCacheAll  = false;
>  	}
> -	read_unlock(&GlobalSMBSeslock);
> +	spin_unlock(&cifs_file_list_lock);
>  	if ((rc == 0) && CIFS_I(inode)->write_behind_rc)
>  		rc = CIFS_I(inode)->write_behind_rc;
>  	FreeXid(xid);
> @@ -617,18 +617,18 @@ int cifs_closedir(struct inode *inode, struct file *file)
>  		struct cifsTconInfo *pTcon = tlink_tcon(pCFileStruct->tlink);
>  
>  		cFYI(1, "Freeing private data in close dir");
> -		write_lock(&GlobalSMBSeslock);
> +		spin_lock(&cifs_file_list_lock);
>  		if (!pCFileStruct->srch_inf.endOfSearch &&
>  		    !pCFileStruct->invalidHandle) {
>  			pCFileStruct->invalidHandle = true;
> -			write_unlock(&GlobalSMBSeslock);
> +			spin_unlock(&cifs_file_list_lock);
>  			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);
> +			spin_unlock(&cifs_file_list_lock);
>  		ptmp = pCFileStruct->srch_inf.ntwrk_buf_start;
>  		if (ptmp) {
>  			cFYI(1, "closedir free smb buf in srch struct");
> @@ -1114,7 +1114,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
>  	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER))
>  		fsuid_only = false;
>  
> -	read_lock(&GlobalSMBSeslock);
> +	spin_lock(&cifs_file_list_lock);
>  	/* we could simply get the first_list_entry since write-only entries
>  	   are always at the end of the list but since the first entry might
>  	   have a close pending, we go through the whole list */
> @@ -1128,7 +1128,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
>  				/* found a good file */
>  				/* lock it so it will not be closed on us */
>  				cifsFileInfo_get(open_file);
> -				read_unlock(&GlobalSMBSeslock);
> +				spin_unlock(&cifs_file_list_lock);
>  				return open_file;
>  			} /* else might as well continue, and look for
>  			     another, or simply have the caller reopen it
> @@ -1136,7 +1136,7 @@ struct cifsFileInfo *find_readable_file(struct cifsInodeInfo *cifs_inode,
>  		} else /* write only file */
>  			break; /* write only files are last so must be done */
>  	}
> -	read_unlock(&GlobalSMBSeslock);
> +	spin_unlock(&cifs_file_list_lock);
>  	return NULL;
>  }
>  #endif
> @@ -1163,7 +1163,7 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
>  	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER))
>  		fsuid_only = false;
>  
> -	read_lock(&GlobalSMBSeslock);
> +	spin_lock(&cifs_file_list_lock);
>  refind_writable:
>  	list_for_each_entry(open_file, &cifs_inode->openFileList, flist) {
>  		if (open_file->closePend)
> @@ -1177,11 +1177,11 @@ refind_writable:
>  
>  			if (!open_file->invalidHandle) {
>  				/* found a good writable file */
> -				read_unlock(&GlobalSMBSeslock);
> +				spin_unlock(&cifs_file_list_lock);
>  				return open_file;
>  			}
>  
> -			read_unlock(&GlobalSMBSeslock);
> +			spin_unlock(&cifs_file_list_lock);
>  			/* Had to unlock since following call can block */
>  			rc = cifs_reopen_file(open_file, false);
>  			if (!rc) {
> @@ -1189,7 +1189,7 @@ refind_writable:
>  					return open_file;
>  				else { /* start over in case this was deleted */
>  				       /* since the list could be modified */
> -					read_lock(&GlobalSMBSeslock);
> +					spin_lock(&cifs_file_list_lock);
>  					cifsFileInfo_put(open_file);
>  					goto refind_writable;
>  				}
> @@ -1203,7 +1203,7 @@ refind_writable:
>  			to hold up writepages here (rather than
>  			in caller) with continuous retries */
>  			cFYI(1, "wp failed on reopen file");
> -			read_lock(&GlobalSMBSeslock);
> +			spin_lock(&cifs_file_list_lock);
>  			/* can not use this handle, no write
>  			   pending on this one after all */
>  			cifsFileInfo_put(open_file);
> @@ -1224,7 +1224,7 @@ refind_writable:
>  		any_available = true;
>  		goto refind_writable;
>  	}
> -	read_unlock(&GlobalSMBSeslock);
> +	spin_unlock(&cifs_file_list_lock);
>  	return NULL;
>  }
>  
> @@ -2132,16 +2132,16 @@ static int is_inode_writable(struct cifsInodeInfo *cifs_inode)
>  {
>  	struct cifsFileInfo *open_file;
>  
> -	read_lock(&GlobalSMBSeslock);
> +	spin_lock(&cifs_file_list_lock);
>  	list_for_each_entry(open_file, &cifs_inode->openFileList, flist) {
>  		if (open_file->closePend)
>  			continue;
>  		if (OPEN_FMODE(open_file->f_flags) & FMODE_WRITE) {
> -			read_unlock(&GlobalSMBSeslock);
> +			spin_unlock(&cifs_file_list_lock);
>  			return 1;
>  		}
>  	}
> -	read_unlock(&GlobalSMBSeslock);
> +	spin_unlock(&cifs_file_list_lock);
>  	return 0;
>  }
>  
> @@ -2305,8 +2305,8 @@ void cifs_oplock_break(struct work_struct *work)
>  	 * finished grabbing reference for us.  Make sure it's done by
>  	 * waiting for GlobalSMSSeslock.
>  	 */
> -	write_lock(&GlobalSMBSeslock);
> -	write_unlock(&GlobalSMBSeslock);
> +	spin_lock(&cifs_file_list_lock);
> +	spin_unlock(&cifs_file_list_lock);
>  
>  	cifs_oplock_break_put(cfile);
>  }
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index 9bac3e7..de6073c 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -560,7 +560,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
>  				continue;
>  
>  			cifs_stats_inc(&tcon->num_oplock_brks);
> -			read_lock(&GlobalSMBSeslock);
> +			spin_lock(&cifs_file_list_lock);
>  			list_for_each(tmp2, &tcon->openFileList) {
>  				netfile = list_entry(tmp2, struct cifsFileInfo,
>  						     tlist);
> @@ -572,7 +572,7 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
>  				 * closed anyway.
>  				 */
>  				if (netfile->closePend) {
> -					read_unlock(&GlobalSMBSeslock);
> +					spin_unlock(&cifs_file_list_lock);
>  					read_unlock(&cifs_tcp_ses_lock);
>  					return true;
>  				}
> @@ -594,11 +594,11 @@ is_valid_oplock_break(struct smb_hdr *buf, struct TCP_Server_Info *srv)
>  					cifs_oplock_break_get(netfile);
>  				netfile->oplock_break_cancelled = false;
>  
> -				read_unlock(&GlobalSMBSeslock);
> +				spin_unlock(&cifs_file_list_lock);
>  				read_unlock(&cifs_tcp_ses_lock);
>  				return true;
>  			}
> -			read_unlock(&GlobalSMBSeslock);
> +			spin_unlock(&cifs_file_list_lock);
>  			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 078c625..dbc1c64 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -529,14 +529,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);
> +		spin_lock(&cifs_file_list_lock);
>  		if (!cifsFile->srch_inf.endOfSearch &&
>  		    !cifsFile->invalidHandle) {
>  			cifsFile->invalidHandle = true;
> -			write_unlock(&GlobalSMBSeslock);
> +			spin_unlock(&cifs_file_list_lock);
>  			CIFSFindClose(xid, pTcon, cifsFile->netfid);
>  		} else
> -			write_unlock(&GlobalSMBSeslock);
> +			spin_unlock(&cifs_file_list_lock);
>  		if (cifsFile->srch_inf.ntwrk_buf_start) {
>  			cFYI(1, "freeing SMB ff cache buf on search rewind");
>  			if (cifsFile->srch_inf.smallBuf)

Looks fine to me.


Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 13/15] cifs: convert cifsFileInfo->count to non-atomic counter
       [not found]     ` <1286559072-29032-14-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2010-10-11  5:46       ` Suresh Jayaraman
  0 siblings, 0 replies; 25+ messages in thread
From: Suresh Jayaraman @ 2010-10-11  5:46 UTC (permalink / raw)
  To: Jeff Layton
  Cc: smfrench-Re5JQEeQqe8AvxtiuMwx3w, linux-cifs-u79uwXL29TY76Z2rM5mHXA

On 10/08/2010 11:01 PM, Jeff Layton wrote:
> The count for cifsFileInfo is currently an atomic, but that just adds
> complexity for little value. We generally need to hold cifs_file_list_lock
> to traverse the lists anyway so we might as well make this counter
> non-atomic and simply use the cifs_file_list_lock to protect it.
> 
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsglob.h |    9 ++++++---
>  fs/cifs/file.c     |    8 +++++---
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index c928e39..2327dcb 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -393,16 +393,19 @@ struct cifsFileInfo {
>  	struct list_head llist; /* list of byte range locks we have. */
>  	bool invalidHandle:1;	/* file closed via session abend */
>  	bool oplock_break_cancelled:1;
> -	atomic_t count;		/* reference count */
> +	int count;		/* refcount -- protected by cifs_file_list_lock */
>  	struct mutex fh_mutex; /* prevents reopen race after dead ses*/
>  	struct cifs_search_info srch_inf;
>  	struct work_struct oplock_break; /* work for oplock breaks */
>  };
>  
> -/* Take a reference on the file private data */
> +/*
> + * Take a reference on the file private data. Must be called with
> + * cifs_file_list_lock held.
> + */
>  static inline void cifsFileInfo_get(struct cifsFileInfo *cifs_file)
>  {
> -	atomic_inc(&cifs_file->count);
> +	++cifs_file->count;
>  }
>  
>  void cifsFileInfo_put(struct cifsFileInfo *cifs_file);
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 8cedcd7..7b8122b 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -172,6 +172,7 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
>  	if (pCifsFile == NULL)
>  		return pCifsFile;
>  
> +	pCifsFile->count = 1;
>  	pCifsFile->netfid = fileHandle;
>  	pCifsFile->pid = current->tgid;
>  	pCifsFile->uid = current_fsuid();
> @@ -182,7 +183,6 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
>  	mutex_init(&pCifsFile->fh_mutex);
>  	mutex_init(&pCifsFile->lock_mutex);
>  	INIT_LIST_HEAD(&pCifsFile->llist);
> -	atomic_set(&pCifsFile->count, 1);
>  	INIT_WORK(&pCifsFile->oplock_break, cifs_oplock_break);
>  
>  	spin_lock(&cifs_file_list_lock);
> @@ -203,7 +203,8 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
>  
>  /*
>   * Release a reference on the file private data. This may involve closing
> - * the filehandle out on the server.
> + * the filehandle out on the server. Must be called without holding
> + * cifs_file_list_lock.
>   */
>  void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>  {
> @@ -212,7 +213,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>  	struct cifsLockInfo *li, *tmp;
>  
>  	spin_lock(&cifs_file_list_lock);
> -	if (!atomic_dec_and_test(&cifs_file->count)) {
> +	if (--cifs_file->count > 0) {
>  		spin_unlock(&cifs_file_list_lock);
>  		return;
>  	}
> @@ -2254,6 +2255,7 @@ void cifs_oplock_break(struct work_struct *work)
>  	cifs_oplock_break_put(cfile);
>  }
>  
> +/* must be called while holding cifs_file_list_lock */
>  void cifs_oplock_break_get(struct cifsFileInfo *cfile)
>  {
>  	cifs_sb_active(cfile->dentry->d_sb);

Looks correct to me.


Reviewed-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 04/15] cifs: eliminate oflags option from cifs_new_fileinfo
       [not found]         ` <4CB2A392.9030400-l3A5Bk7waGM@public.gmane.org>
@ 2010-10-11 11:13           ` Jeff Layton
       [not found]             ` <20101011071322.3a6e090c-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
  0 siblings, 1 reply; 25+ messages in thread
From: Jeff Layton @ 2010-10-11 11:13 UTC (permalink / raw)
  To: Suresh Jayaraman
  Cc: smfrench-Re5JQEeQqe8AvxtiuMwx3w, linux-cifs-u79uwXL29TY76Z2rM5mHXA

On Mon, 11 Oct 2010 11:11:38 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 10/08/2010 11:01 PM, Jeff Layton wrote:
> > Eliminate the poor, misunderstood "oflags" option from cifs_new_fileinfo.
> > The callers mostly pass in the filp->f_flags here, except for one place
> > that passes in the flags after they've been converted to their SMB_O_*
> > equivalents.
> > 
> > None of these are correct however since we're checking that value for
> > the presence of FMODE_READ. Luckily that only affects how the f_list is
> > ordered. What it really wants here is the file->f_mode, but this check
> > really makes no sense whatsoever. FMODE_READ will be set for O_RDWR or
> > O_RDONLY opens. So this in effect just moves those to the front of the
> > list and leaves O_WRONLY at the end.
> > 
> > That might make some sense if anything actually paid attention to this
> > list order, but nothing does. find_readable_file and find_writable_file
> > both walk through the list in the same direction.
> > 
> > Let's just keep this simple and just put things on the list with
> > list_add.
> > 
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/cifsproto.h |    3 +--
> >  fs/cifs/dir.c       |   15 ++++-----------
> >  fs/cifs/file.c      |    5 ++---
> >  3 files changed, 7 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> > index 7f416ab..bed004c 100644
> > --- a/fs/cifs/cifsproto.h
> > +++ b/fs/cifs/cifsproto.h
> > @@ -107,8 +107,7 @@ extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time,
> >  
> >  extern struct cifsFileInfo *cifs_new_fileinfo(struct inode *newinode,
> >  				__u16 fileHandle, struct file *file,
> > -				struct tcon_link *tlink,
> > -				unsigned int oflags, __u32 oplock);
> > +				struct tcon_link *tlink, __u32 oplock);
> >  extern int cifs_posix_open(char *full_path, struct inode **pinode,
> >  				struct super_block *sb,
> >  				int mode, int oflags,
> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > index c205ec9..452c9b5 100644
> > --- a/fs/cifs/dir.c
> > +++ b/fs/cifs/dir.c
> > @@ -132,7 +132,7 @@ cifs_bp_rename_retry:
> >  
> >  struct cifsFileInfo *
> >  cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
> > -		  struct tcon_link *tlink, unsigned int oflags, __u32 oplock)
> > +		  struct tcon_link *tlink, __u32 oplock)
> >  {
> >  	struct dentry *dentry = file->f_path.dentry;
> >  	struct cifsFileInfo *pCifsFile;
> > @@ -160,13 +160,7 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
> >  	list_add(&pCifsFile->tlist, &(tlink_tcon(tlink)->openFileList));
> >  	pCifsInode = CIFS_I(newinode);
> >  	if (pCifsInode) {
> > -		/* if readable file instance put first in list*/
> > -		if (oflags & FMODE_READ)
> > -			list_add(&pCifsFile->flist, &pCifsInode->openFileList);
> > -		else
> > -			list_add_tail(&pCifsFile->flist,
> > -				      &pCifsInode->openFileList);
> > -
> > +		list_add(&pCifsFile->flist, &pCifsInode->openFileList);
> 
> find_readable_file() assumes that if it is a write-only file, it will be
> in the tail and it can break out of the loop. That needs to be fixed as
> well?
> 

Good catch. You're correct...

So I guess my assertion that nothing cares about the list order isn't
true. Is this optimization worth preserving?

My gut feeling is "no". The operations inside of find_readable_file are
sufficiently cheap that it's not worth the extra complexity, but I'm
willing to listen to arguments to the contrary...

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 04/15] cifs: eliminate oflags option from cifs_new_fileinfo
       [not found]             ` <20101011071322.3a6e090c-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
@ 2010-10-11 17:04               ` Steve French
       [not found]                 ` <AANLkTik4=achQnm=8XN+GWWKFL8QOddz4xVVaBs4X3sX-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 25+ messages in thread
From: Steve French @ 2010-10-11 17:04 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Suresh Jayaraman, linux-cifs-u79uwXL29TY76Z2rM5mHXA

find readable file is a common operation and the number of open files
can be huge (thousands)

On Mon, Oct 11, 2010 at 6:13 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, 11 Oct 2010 11:11:38 +0530
> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
>
>> On 10/08/2010 11:01 PM, Jeff Layton wrote:
>> > Eliminate the poor, misunderstood "oflags" option from cifs_new_fileinfo.
>> > The callers mostly pass in the filp->f_flags here, except for one place
>> > that passes in the flags after they've been converted to their SMB_O_*
>> > equivalents.
>> >
>> > None of these are correct however since we're checking that value for
>> > the presence of FMODE_READ. Luckily that only affects how the f_list is
>> > ordered. What it really wants here is the file->f_mode, but this check
>> > really makes no sense whatsoever. FMODE_READ will be set for O_RDWR or
>> > O_RDONLY opens. So this in effect just moves those to the front of the
>> > list and leaves O_WRONLY at the end.
>> >
>> > That might make some sense if anything actually paid attention to this
>> > list order, but nothing does. find_readable_file and find_writable_file
>> > both walk through the list in the same direction.
>> >
>> > Let's just keep this simple and just put things on the list with
>> > list_add.
>> >
>> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> > ---
>> >  fs/cifs/cifsproto.h |    3 +--
>> >  fs/cifs/dir.c       |   15 ++++-----------
>> >  fs/cifs/file.c      |    5 ++---
>> >  3 files changed, 7 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> > index 7f416ab..bed004c 100644
>> > --- a/fs/cifs/cifsproto.h
>> > +++ b/fs/cifs/cifsproto.h
>> > @@ -107,8 +107,7 @@ extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time,
>> >
>> >  extern struct cifsFileInfo *cifs_new_fileinfo(struct inode *newinode,
>> >                             __u16 fileHandle, struct file *file,
>> > -                           struct tcon_link *tlink,
>> > -                           unsigned int oflags, __u32 oplock);
>> > +                           struct tcon_link *tlink, __u32 oplock);
>> >  extern int cifs_posix_open(char *full_path, struct inode **pinode,
>> >                             struct super_block *sb,
>> >                             int mode, int oflags,
>> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
>> > index c205ec9..452c9b5 100644
>> > --- a/fs/cifs/dir.c
>> > +++ b/fs/cifs/dir.c
>> > @@ -132,7 +132,7 @@ cifs_bp_rename_retry:
>> >
>> >  struct cifsFileInfo *
>> >  cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
>> > -             struct tcon_link *tlink, unsigned int oflags, __u32 oplock)
>> > +             struct tcon_link *tlink, __u32 oplock)
>> >  {
>> >     struct dentry *dentry = file->f_path.dentry;
>> >     struct cifsFileInfo *pCifsFile;
>> > @@ -160,13 +160,7 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle, struct file *file,
>> >     list_add(&pCifsFile->tlist, &(tlink_tcon(tlink)->openFileList));
>> >     pCifsInode = CIFS_I(newinode);
>> >     if (pCifsInode) {
>> > -           /* if readable file instance put first in list*/
>> > -           if (oflags & FMODE_READ)
>> > -                   list_add(&pCifsFile->flist, &pCifsInode->openFileList);
>> > -           else
>> > -                   list_add_tail(&pCifsFile->flist,
>> > -                                 &pCifsInode->openFileList);
>> > -
>> > +           list_add(&pCifsFile->flist, &pCifsInode->openFileList);
>>
>> find_readable_file() assumes that if it is a write-only file, it will be
>> in the tail and it can break out of the loop. That needs to be fixed as
>> well?
>>
>
> Good catch. You're correct...
>
> So I guess my assertion that nothing cares about the list order isn't
> true. Is this optimization worth preserving?
>
> My gut feeling is "no". The operations inside of find_readable_file are
> sufficiently cheap that it's not worth the extra complexity, but I'm
> willing to listen to arguments to the contrary...
>
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 04/15] cifs: eliminate oflags option from cifs_new_fileinfo
       [not found]                 ` <AANLkTik4=achQnm=8XN+GWWKFL8QOddz4xVVaBs4X3sX-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-10-11 17:17                   ` Jeff Layton
       [not found]                     ` <20101011131707.646b8532-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
  0 siblings, 1 reply; 25+ messages in thread
From: Jeff Layton @ 2010-10-11 17:17 UTC (permalink / raw)
  To: Steve French; +Cc: Suresh Jayaraman, linux-cifs-u79uwXL29TY76Z2rM5mHXA

On Mon, 11 Oct 2010 12:04:09 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> find readable file is a common operation and the number of open files
> can be huge (thousands)
> 

I don't think so. It gets called from:

    get_cifs_acl
    set_cifs_acl

...and those are only used when the cifsacl mount option is used, which
no one in their right mind does.

Opening the same inode thousands of times is a pretty atypical
workload. Even if it isn't though, we still return as soon as we find
the first usable open file.

But lets assume worst case -- we have some application that opens the
same inode thousands of times and none of them are usable. I'd still
argue that walking the entire list is a trivial amount of cpu time.

Keeping a list ordered like this is fragile and easily broken. I think
we shouldn't rely on it here.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 04/15] cifs: eliminate oflags option from cifs_new_fileinfo
       [not found]                     ` <20101011131707.646b8532-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
@ 2010-10-11 17:27                       ` Steve French
       [not found]                         ` <AANLkTik6tc0iJwDACT-nctOi2Ui5E31AihWN8-vCM2zo-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 25+ messages in thread
From: Steve French @ 2010-10-11 17:27 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Suresh Jayaraman, linux-cifs-u79uwXL29TY76Z2rM5mHXA

You are right - find_writeable_file is the common one, not
find_readable_file (might make sense to reverse the ordering -
readonly files at end instead of writeonly files at end - given
find_readable_file is called less often).   There are cases with a
file opened many times from different processes that I have seen.
Not sure why removing the ordering matters much - it is a possible,
albeit usually slight, performance benefit to keep file handles we are
most likely to want at the top of the list.

On Mon, Oct 11, 2010 at 12:17 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, 11 Oct 2010 12:04:09 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> find readable file is a common operation and the number of open files
>> can be huge (thousands)
>>
>
> I don't think so. It gets called from:
>
>    get_cifs_acl
>    set_cifs_acl
>
> ...and those are only used when the cifsacl mount option is used, which
> no one in their right mind does.
>
> Opening the same inode thousands of times is a pretty atypical
> workload. Even if it isn't though, we still return as soon as we find
> the first usable open file.
>
> But lets assume worst case -- we have some application that opens the
> same inode thousands of times and none of them are usable. I'd still
> argue that walking the entire list is a trivial amount of cpu time.
>
> Keeping a list ordered like this is fragile and easily broken. I think
> we shouldn't rely on it here.
>
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 04/15] cifs: eliminate oflags option from cifs_new_fileinfo
       [not found]                         ` <AANLkTik6tc0iJwDACT-nctOi2Ui5E31AihWN8-vCM2zo-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2010-10-11 18:52                           ` Jeff Layton
  0 siblings, 0 replies; 25+ messages in thread
From: Jeff Layton @ 2010-10-11 18:52 UTC (permalink / raw)
  To: Steve French; +Cc: Suresh Jayaraman, linux-cifs-u79uwXL29TY76Z2rM5mHXA

On Mon, 11 Oct 2010 12:27:47 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> You are right - find_writeable_file is the common one, not
> find_readable_file (might make sense to reverse the ordering -
> readonly files at end instead of writeonly files at end - given
> find_readable_file is called less often).   There are cases with a
> file opened many times from different processes that I have seen.
> Not sure why removing the ordering matters much - it is a possible,
> albeit usually slight, performance benefit to keep file handles we are
> most likely to want at the top of the list.
> 

If we were really concerned about performance here, fine-grained
locking would get us much farther toward that goal. I'm concerned about
people breaking the list order, which has obviously been broken for
some time and no one has noticed.

Eventually I want to overhaul all of the find_readable/writable_file
code with a better interface. That will be simpler if I don't have
to deal with unnecessary complexity like the list ordering here.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH 10/15] cifs: convert GlobalSMBSeslock from a rwlock to regular spinlock
       [not found]         ` <4CB2A478.50401-l3A5Bk7waGM@public.gmane.org>
@ 2010-10-12 13:28           ` Christoph Hellwig
  0 siblings, 0 replies; 25+ messages in thread
From: Christoph Hellwig @ 2010-10-12 13:28 UTC (permalink / raw)
  To: Suresh Jayaraman
  Cc: Jeff Layton, smfrench-Re5JQEeQqe8AvxtiuMwx3w,
	linux-cifs-u79uwXL29TY76Z2rM5mHXA

Suresh, any chance you could only quote short, relevant portions
of the patch?  Your full quotes are almost unreadable.

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2010-10-12 13:28 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-08 17:30 [PATCH 00/15] cifs: clean up management of open filehandle (try #3) Jeff Layton
     [not found] ` <1286559072-29032-1-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2010-10-08 17:30   ` [PATCH 01/15] cifs: keep dentry reference in cifsFileInfo instead of inode reference Jeff Layton
2010-10-08 17:30   ` [PATCH 02/15] cifs: don't use vfsmount to pin superblock for oplock breaks Jeff Layton
2010-10-08 17:31   ` [PATCH 03/15] cifs: eliminate cifs_posix_open_inode_helper Jeff Layton
2010-10-08 17:31   ` [PATCH 04/15] cifs: eliminate oflags option from cifs_new_fileinfo Jeff Layton
     [not found]     ` <1286559072-29032-5-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2010-10-11  5:41       ` Suresh Jayaraman
     [not found]         ` <4CB2A392.9030400-l3A5Bk7waGM@public.gmane.org>
2010-10-11 11:13           ` Jeff Layton
     [not found]             ` <20101011071322.3a6e090c-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2010-10-11 17:04               ` Steve French
     [not found]                 ` <AANLkTik4=achQnm=8XN+GWWKFL8QOddz4xVVaBs4X3sX-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-10-11 17:17                   ` Jeff Layton
     [not found]                     ` <20101011131707.646b8532-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2010-10-11 17:27                       ` Steve French
     [not found]                         ` <AANLkTik6tc0iJwDACT-nctOi2Ui5E31AihWN8-vCM2zo-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2010-10-11 18:52                           ` Jeff Layton
2010-10-08 17:31   ` [PATCH 05/15] cifs: eliminate the inode argument " Jeff Layton
2010-10-08 17:31   ` [PATCH 06/15] cifs: clean up cifs_reopen_file Jeff Layton
2010-10-08 17:31   ` [PATCH 07/15] cifs: cifs_write argument change and cleanup Jeff Layton
2010-10-08 17:31   ` [PATCH 08/15] cifs: eliminate pfile pointer from cifsFileInfo Jeff Layton
2010-10-08 17:31   ` [PATCH 09/15] cifs: move cifs_new_fileinfo to file.c Jeff Layton
2010-10-08 17:31   ` [PATCH 10/15] cifs: convert GlobalSMBSeslock from a rwlock to regular spinlock Jeff Layton
     [not found]     ` <1286559072-29032-11-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2010-10-11  5:45       ` Suresh Jayaraman
     [not found]         ` <4CB2A478.50401-l3A5Bk7waGM@public.gmane.org>
2010-10-12 13:28           ` Christoph Hellwig
2010-10-08 17:31   ` [PATCH 11/15] cifs: move cifsFileInfo_put to file.c Jeff Layton
2010-10-08 17:31   ` [PATCH 12/15] cifs: move close processing from cifs_close to cifsFileInfo_put Jeff Layton
2010-10-08 17:31   ` [PATCH 13/15] cifs: convert cifsFileInfo->count to non-atomic counter Jeff Layton
     [not found]     ` <1286559072-29032-14-git-send-email-jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2010-10-11  5:46       ` Suresh Jayaraman
2010-10-08 17:31   ` [PATCH 14/15] cifs: wait for writeback to complete in cifs_flush Jeff Layton
2010-10-08 17:31   ` [PATCH 15/15] cifs: eliminate cifsInodeInfo->write_behind_rc Jeff Layton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.