linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff Layton <jlayton@redhat.com>
To: linux-cifs-client@lists.samba.org
Cc: linux-fsdevel@vger.kernel.org
Subject: [PATCH 4/4] cifs: pass instantiated filp back after open call
Date: Fri, 21 May 2010 14:25:17 -0400	[thread overview]
Message-ID: <1274466317-28231-5-git-send-email-jlayton@redhat.com> (raw)
In-Reply-To: <1274466317-28231-1-git-send-email-jlayton@redhat.com>

The current scheme of sticking open files on a list and assuming that
cifs_open will scoop them off of it is broken and leads to "Busy
inodes after umount..." errors at unmount time.

The problem is that there is no guarantee that cifs_open will always
be called after a ->lookup or ->create operation. If there are
permissions or other problems, then it's quite likely that it *won't*
be called.

Fix this by fully instantiating the filp whenever the file is created
and pass that filp back to the VFS. If there is a problem, the VFS
can clean up the references.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/dir.c  |   34 ++++++++++++++++++++++++++--------
 fs/cifs/file.c |   44 ++------------------------------------------
 2 files changed, 28 insertions(+), 50 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index fc3129b..ea35d74 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -25,6 +25,7 @@
 #include <linux/slab.h>
 #include <linux/namei.h>
 #include <linux/mount.h>
+#include <linux/file.h>
 #include "cifsfs.h"
 #include "cifspdu.h"
 #include "cifsglob.h"
@@ -184,6 +185,8 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle,
 	}
 	write_unlock(&GlobalSMBSeslock);
 
+	file->private_data = pCifsFile;
+
 	return pCifsFile;
 }
 
@@ -463,14 +466,22 @@ cifs_create_set_dentry:
 
 	if (newinode) {
 		struct cifsFileInfo *pfile_info;
-		/*
-		 * cifs_fill_filedata() takes care of setting cifsFileInfo
-		 * pointer to file->private_data.
-		 */
-		pfile_info = cifs_new_fileinfo(newinode, fileHandle, NULL,
+		struct file *filp;
+
+		filp = lookup_instantiate_filp(nd, direntry, generic_file_open);
+		if (IS_ERR(filp)) {
+			rc = PTR_ERR(filp);
+			CIFSSMBClose(xid, tcon, fileHandle);
+			goto cifs_create_out;
+		}
+
+		pfile_info = cifs_new_fileinfo(newinode, fileHandle, filp,
 					       nd->path.mnt, oflags);
-		if (pfile_info == NULL)
+		if (pfile_info == NULL) {
+			fput(filp);
+			CIFSSMBClose(xid, tcon, fileHandle);
 			rc = -ENOMEM;
+		}
 	} else {
 		CIFSSMBClose(xid, tcon, fileHandle);
 	}
@@ -718,15 +729,22 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 			direntry->d_op = &cifs_dentry_ops;
 		d_add(direntry, newInode);
 		if (posix_open) {
-			cfile = cifs_new_fileinfo(newInode, fileHandle, NULL,
+			filp = lookup_instantiate_filp(nd, direntry,
+						       generic_file_open);
+			if (IS_ERR(filp)) {
+				res = (struct dentry *)filp;
+				goto lookup_out;
+			}
+
+			cfile = cifs_new_fileinfo(newInode, fileHandle, filp,
 						  nd->path.mnt,
 						  nd->intent.open.flags);
 			if (cfile == NULL) {
+				fput(filp);
 				CIFSSMBClose(xid, pTcon, fileHandle);
 				res = ERR_PTR(ENOMEM);
 				goto lookup_out;
 			}
-			filp = lookup_instantiate_filp(nd, direntry, NULL);
 		}
 		/* since paths are not looked up by component - the parent
 		   directories are presumed to be good here */
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 001e916..310533d 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -162,38 +162,6 @@ psx_client_can_cache:
 	return 0;
 }
 
-static struct cifsFileInfo *
-cifs_fill_filedata(struct file *file)
-{
-	struct list_head *tmp;
-	struct cifsFileInfo *pCifsFile = NULL;
-	struct cifsInodeInfo *pCifsInode = NULL;
-
-	/* search inode for this file and fill in file->private_data */
-	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
-	read_lock(&GlobalSMBSeslock);
-	list_for_each(tmp, &pCifsInode->openFileList) {
-		pCifsFile = list_entry(tmp, struct cifsFileInfo, flist);
-		if ((pCifsFile->pfile == NULL) &&
-		    (pCifsFile->pid == current->tgid)) {
-			/* mode set in cifs_create */
-
-			/* needed for writepage */
-			pCifsFile->pfile = file;
-			file->private_data = pCifsFile;
-			break;
-		}
-	}
-	read_unlock(&GlobalSMBSeslock);
-
-	if (file->private_data != NULL) {
-		return pCifsFile;
-	} else if ((file->f_flags & O_CREAT) && (file->f_flags & O_EXCL))
-			cERROR(1, "could not find file instance for "
-				   "new file %p", file);
-	return NULL;
-}
-
 /* all arguments to this function must be checked for validity in caller */
 static inline int cifs_open_inode_helper(struct inode *inode, struct file *file,
 	struct cifsInodeInfo *pCifsInode, struct cifsFileInfo *pCifsFile,
@@ -256,7 +224,7 @@ int cifs_open(struct inode *inode, struct file *file)
 	__u32 oplock;
 	struct cifs_sb_info *cifs_sb;
 	struct cifsTconInfo *tcon;
-	struct cifsFileInfo *pCifsFile;
+	struct cifsFileInfo *pCifsFile = NULL;
 	struct cifsInodeInfo *pCifsInode;
 	char *full_path = NULL;
 	int desiredAccess;
@@ -270,12 +238,6 @@ int cifs_open(struct inode *inode, struct file *file)
 	tcon = cifs_sb->tcon;
 
 	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
-	pCifsFile = cifs_fill_filedata(file);
-	if (pCifsFile) {
-		rc = 0;
-		FreeXid(xid);
-		return rc;
-	}
 
 	full_path = build_path_from_dentry(file->f_path.dentry);
 	if (full_path == NULL) {
@@ -315,7 +277,6 @@ int cifs_open(struct inode *inode, struct file *file)
 				rc = -ENOMEM;
 				goto out;
 			}
-			file->private_data = pCifsFile;
 
 			cifs_posix_open_inode_helper(inode, file, pCifsInode,
 						     oplock, netfid);
@@ -401,8 +362,7 @@ int cifs_open(struct inode *inode, struct file *file)
 
 	pCifsFile = cifs_new_fileinfo(inode, netfid, file, file->f_path.mnt,
 					file->f_flags);
-	file->private_data = pCifsFile;
-	if (file->private_data == NULL) {
+	if (pCifsFile == NULL) {
 		rc = -ENOMEM;
 		goto out;
 	}
-- 
1.6.6.1


  parent reply	other threads:[~2010-05-21 18:25 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-05-21 18:25 [PATCH 0/4] cifs: fix "Busy inodes after umount" issues (RFC) Jeff Layton
2010-05-21 18:25 ` [PATCH 1/4] cifs: make cifs_lookup return a dentry Jeff Layton
2010-05-21 18:45   ` Josef Bacik
2010-05-21 18:42     ` Jeff Layton
2010-05-22 13:30   ` Al Viro
2010-05-22 14:08     ` Jeff Layton
2010-05-22 14:46       ` Al Viro
2010-05-22 15:23         ` Jeff Layton
2010-05-21 18:25 ` [PATCH 2/4] cifs: don't leave open files dangling Jeff Layton
2010-05-24  6:50   ` [linux-cifs-client] " Suresh Jayaraman
2010-05-24 10:49     ` Jeff Layton
2010-05-21 18:25 ` [PATCH 3/4] cifs: move cifs_new_fileinfo call out of cifs_posix_open Jeff Layton
2010-05-24  6:50   ` [linux-cifs-client] " Suresh Jayaraman
2010-05-24 10:48     ` Jeff Layton
2010-05-21 18:25 ` Jeff Layton [this message]
2010-05-24  7:13 ` [PATCH 0/4] cifs: fix "Busy inodes after umount" issues (RFC) Suresh Jayaraman
2010-05-24 10:52   ` [linux-cifs-client] " Jeff Layton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1274466317-28231-5-git-send-email-jlayton@redhat.com \
    --to=jlayton@redhat.com \
    --cc=linux-cifs-client@lists.samba.org \
    --cc=linux-fsdevel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).