[PATCH 0/4] cifs: fix "Busy inodes after umount" issues (RFC)

[PATCH 0/4] cifs: fix "Busy inodes after umount" issues (RFC)

[Jeff Layton]

We've had a spate of "Busy inodes after umount" problems in recent
kernels. With the help of a reproducer that Suresh provided, I tracked
down the cause of one of these and wrote it up here:

    https://bugzilla.samba.org/show_bug.cgi?id=7433

The main problem is that CIFS opens files during create operations,
puts these files on a list and then expects that a subsequent open
operation will find those on the list and make them full, productive
members of society.

This expectation is wrong however. There's no guarantee that cifs_open
will be called at all after a create. There are several scenarios that
can prevent it from occuring. When this happens, these entries get left
dangling on the list and nothing will ever clean them up. Recent changes
have made it so that cifsFileInfo structs hold an inode reference as
well, which is what actually leads to the busy inodes after umount
problems.

This patch is intended to fix this in the right way. It has the create
operations properly use lookup_instantiate_filp to create an open file
during the operation that does the actual create. With this, there's
no need to have cifs_open scrape these entries off the list.

This fixes the busy inodes problem I was able to reproduce. It's not
very well tested yet however, and I could stand for someone else to
review it and help test it.

Jeff Layton (4):
  cifs: make cifs_lookup return a dentry
  cifs: don't leave open files dangling
  cifs: move cifs_new_fileinfo call out of cifs_posix_open
  cifs: pass instantiated filp back after open call

 fs/cifs/cifsproto.h |    1 -
 fs/cifs/dir.c       |   88 ++++++++++++++++++++++++++++++---------------------
 fs/cifs/file.c      |   59 +++++++--------------------------
 3 files changed, 65 insertions(+), 83 deletions(-)

[PATCH 1/4] cifs: make cifs_lookup return a dentry

[Jeff Layton]

cifs_lookup doesn't actually return a dentry. It instantiates the one
that's passed in, but callers don't have any way to know if the lookup
succeeded.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/dir.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 391816b..54de8e5 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -639,6 +639,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 	struct inode *newInode = NULL;
 	char *full_path = NULL;
 	struct file *filp;
+	struct dentry *res;
 
 	xid = GetXid();
 
@@ -738,7 +739,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 		/* since paths are not looked up by component - the parent
 		   directories are presumed to be good here */
 		renew_parental_timestamps(direntry);
-
+		res = direntry;
+		dget(res);
 	} else if (rc == -ENOENT) {
 		rc = 0;
 		direntry->d_time = jiffies;
@@ -747,17 +749,20 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 		else
 			direntry->d_op = &cifs_dentry_ops;
 		d_add(direntry, NULL);
-	/*	if it was once a directory (but how can we tell?) we could do
-		shrink_dcache_parent(direntry); */
+		res = direntry;
+		dget(res);
 	} else if (rc != -EACCES) {
 		cERROR(1, "Unexpected lookup error %d", rc);
 		/* We special case check for Access Denied - since that
 		is a common return code */
+		res = ERR_PTR(rc);
+	} else {
+		res = ERR_PTR(rc);
 	}
 
 	kfree(full_path);
 	FreeXid(xid);
-	return ERR_PTR(rc);
+	return res;
 }
 
 static int
-- 
1.6.6.1

Re: [PATCH 1/4] cifs: make cifs_lookup return a dentry

[Josef Bacik]

On Fri, May 21, 2010 at 02:25:14PM -0400, Jeff Layton wrote:
> cifs_lookup doesn't actually return a dentry. It instantiates the one
> that's passed in, but callers don't have any way to know if the lookup
> succeeded.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/dir.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 391816b..54de8e5 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -639,6 +639,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>  	struct inode *newInode = NULL;
>  	char *full_path = NULL;
>  	struct file *filp;
> +	struct dentry *res;
>  
>  	xid = GetXid();
>  
> @@ -738,7 +739,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>  		/* since paths are not looked up by component - the parent
>  		   directories are presumed to be good here */
>  		renew_parental_timestamps(direntry);
> -
> +		res = direntry;
> +		dget(res);
>  	} else if (rc == -ENOENT) {
>  		rc = 0;
>  		direntry->d_time = jiffies;
> @@ -747,17 +749,20 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>  		else
>  			direntry->d_op = &cifs_dentry_ops;
>  		d_add(direntry, NULL);
> -	/*	if it was once a directory (but how can we tell?) we could do
> -		shrink_dcache_parent(direntry); */
> +		res = direntry;
> +		dget(res);

Should probably do

res = dget(direntry)

here and above.  Thanks,

Josef

Re: [PATCH 1/4] cifs: make cifs_lookup return a dentry

[Jeff Layton]

On Fri, 21 May 2010 14:45:55 -0400
Josef Bacik <josef@redhat.com> wrote:

> On Fri, May 21, 2010 at 02:25:14PM -0400, Jeff Layton wrote:
> > cifs_lookup doesn't actually return a dentry. It instantiates the one
> > that's passed in, but callers don't have any way to know if the lookup
> > succeeded.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/cifs/dir.c |   13 +++++++++----
> >  1 files changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > index 391816b..54de8e5 100644
> > --- a/fs/cifs/dir.c
> > +++ b/fs/cifs/dir.c
> > @@ -639,6 +639,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> >  	struct inode *newInode = NULL;
> >  	char *full_path = NULL;
> >  	struct file *filp;
> > +	struct dentry *res;
> >  
> >  	xid = GetXid();
> >  
> > @@ -738,7 +739,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> >  		/* since paths are not looked up by component - the parent
> >  		   directories are presumed to be good here */
> >  		renew_parental_timestamps(direntry);
> > -
> > +		res = direntry;
> > +		dget(res);
> >  	} else if (rc == -ENOENT) {
> >  		rc = 0;
> >  		direntry->d_time = jiffies;
> > @@ -747,17 +749,20 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> >  		else
> >  			direntry->d_op = &cifs_dentry_ops;
> >  		d_add(direntry, NULL);
> > -	/*	if it was once a directory (but how can we tell?) we could do
> > -		shrink_dcache_parent(direntry); */
> > +		res = direntry;
> > +		dget(res);
> 
> Should probably do
> 
> res = dget(direntry)
> 


Ahh good catch. Will fix.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/4] cifs: make cifs_lookup return a dentry

[Al Viro]

On Fri, May 21, 2010 at 02:25:14PM -0400, Jeff Layton wrote:
> cifs_lookup doesn't actually return a dentry. It instantiates the one
> that's passed in, but callers don't have any way to know if the lookup
> succeeded.

Huh?  Of course they do - ->lookup() has every right to do just that;
d_add() and return NULL is perfectly legitimate.

Re: [PATCH 1/4] cifs: make cifs_lookup return a dentry

[Jeff Layton]

On Sat, 22 May 2010 14:30:51 +0100
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Fri, May 21, 2010 at 02:25:14PM -0400, Jeff Layton wrote:
> > cifs_lookup doesn't actually return a dentry. It instantiates the one
> > that's passed in, but callers don't have any way to know if the lookup
> > succeeded.
> 
> Huh?  Of course they do - ->lookup() has every right to do just that;
> d_add() and return NULL is perfectly legitimate.

OK, bad description on my part... Is one way of doing this preferred
over another?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/4] cifs: make cifs_lookup return a dentry

[Al Viro]

On Sat, May 22, 2010 at 10:08:36AM -0400, Jeff Layton wrote:
> On Sat, 22 May 2010 14:30:51 +0100
> Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> > On Fri, May 21, 2010 at 02:25:14PM -0400, Jeff Layton wrote:
> > > cifs_lookup doesn't actually return a dentry. It instantiates the one
> > > that's passed in, but callers don't have any way to know if the lookup
> > > succeeded.
> > 
> > Huh?  Of course they do - ->lookup() has every right to do just that;
> > d_add() and return NULL is perfectly legitimate.
> 
> OK, bad description on my part... Is one way of doing this preferred
> over another?

Non-NULL, non ERR_PTR() strongly implies that you have found a different
dentry and tell the caller to use it instead; returning the argument (after
having cached it and bumped its refcount) will work, but it's pretty much
a deliberate obfuscation.

Re: [PATCH 1/4] cifs: make cifs_lookup return a dentry

[Jeff Layton]

On Sat, 22 May 2010 15:46:15 +0100
Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Sat, May 22, 2010 at 10:08:36AM -0400, Jeff Layton wrote:
> > On Sat, 22 May 2010 14:30:51 +0100
> > Al Viro <viro@ZenIV.linux.org.uk> wrote:
> > 
> > > On Fri, May 21, 2010 at 02:25:14PM -0400, Jeff Layton wrote:
> > > > cifs_lookup doesn't actually return a dentry. It instantiates the one
> > > > that's passed in, but callers don't have any way to know if the lookup
> > > > succeeded.
> > > 
> > > Huh?  Of course they do - ->lookup() has every right to do just that;
> > > d_add() and return NULL is perfectly legitimate.
> > 
> > OK, bad description on my part... Is one way of doing this preferred
> > over another?
> 
> Non-NULL, non ERR_PTR() strongly implies that you have found a different
> dentry and tell the caller to use it instead; returning the argument (after
> having cached it and bumped its refcount) will work, but it's pretty much
> a deliberate obfuscation.

Ok, thanks for the explanation. In that case, I'll plan to drop this
patch as the existing behavior is more clear.

Cheers,
-- 
Jeff Layton <jlayton@redhat.com>

[PATCH 2/4] cifs: don't leave open files dangling

[Jeff Layton]

If we aren't going to call cifs_new_fileinfo to put the files on
the openFileList for an inode, then they should be closed -- full stop.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/dir.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 54de8e5..e11ee2e 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -269,6 +269,8 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
 					       oflags);
 		if (pfile_info == NULL)
 			rc = -ENOMEM;
+	} else {
+		CIFSSMBClose(xid, cifs_sb->tcon, *pnetfid);
 	}
 
 posix_open_ret:
@@ -478,11 +480,7 @@ cifs_create_set_dentry:
 	else
 		cFYI(1, "Create worked, get_inode_info failed rc = %d", rc);
 
-	/* nfsd case - nfs srv does not set nd */
-	if ((nd == NULL) || (!(nd->flags & LOOKUP_OPEN))) {
-		/* mknod case - do not leave file open */
-		CIFSSMBClose(xid, tcon, fileHandle);
-	} else if (!(posix_create) && (newinode)) {
+	if (!posix_create && newinode) {
 		struct cifsFileInfo *pfile_info;
 		/*
 		 * cifs_fill_filedata() takes care of setting cifsFileInfo
@@ -492,7 +490,10 @@ cifs_create_set_dentry:
 					       nd->path.mnt, oflags);
 		if (pfile_info == NULL)
 			rc = -ENOMEM;
+	} else {
+		CIFSSMBClose(xid, tcon, fileHandle);
 	}
+
 cifs_create_out:
 	kfree(buf);
 	kfree(full_path);
-- 
1.6.6.1

Re: [linux-cifs-client] [PATCH 2/4] cifs: don't leave open files dangling

[Suresh Jayaraman]

On 05/21/2010 11:55 PM, Jeff Layton wrote:
> If we aren't going to call cifs_new_fileinfo to put the files on
> the openFileList for an inode, then they should be closed -- full stop.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/dir.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 54de8e5..e11ee2e 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -269,6 +269,8 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
>  					       oflags);
>  		if (pfile_info == NULL)
>  			rc = -ENOMEM;
> +	} else {
> +		CIFSSMBClose(xid, cifs_sb->tcon, *pnetfid);

May be this not needed here as a subsequent patch pushes
cifs_new_fileinfo to the caller..?

>  	}
>  
>  posix_open_ret:
> @@ -478,11 +480,7 @@ cifs_create_set_dentry:
>  	else
>  		cFYI(1, "Create worked, get_inode_info failed rc = %d", rc);
>  
> -	/* nfsd case - nfs srv does not set nd */
> -	if ((nd == NULL) || (!(nd->flags & LOOKUP_OPEN))) {
> -		/* mknod case - do not leave file open */
> -		CIFSSMBClose(xid, tcon, fileHandle);
> -	} else if (!(posix_create) && (newinode)) {
> +	if (!posix_create && newinode) {

Here too, posix_create is going to go..

May be lining this patch after [3/4] is a good idea..? Would make the
patchset simpler, I think.


>  		struct cifsFileInfo *pfile_info;
>  		/*
>  		 * cifs_fill_filedata() takes care of setting cifsFileInfo
> @@ -492,7 +490,10 @@ cifs_create_set_dentry:
>  					       nd->path.mnt, oflags);
>  		if (pfile_info == NULL)
>  			rc = -ENOMEM;
> +	} else {
> +		CIFSSMBClose(xid, tcon, fileHandle);
>  	}

minor nit:     ^^^ unneeded braces..

> +
>  cifs_create_out:
>  	kfree(buf);
>  	kfree(full_path);


-- 
Suresh Jayaraman

Re: [linux-cifs-client] [PATCH 2/4] cifs: don't leave open files dangling

[Jeff Layton]

On Mon, 24 May 2010 12:20:30 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> On 05/21/2010 11:55 PM, Jeff Layton wrote:
> > If we aren't going to call cifs_new_fileinfo to put the files on
> > the openFileList for an inode, then they should be closed -- full stop.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/cifs/dir.c |   11 ++++++-----
> >  1 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > index 54de8e5..e11ee2e 100644
> > --- a/fs/cifs/dir.c
> > +++ b/fs/cifs/dir.c
> > @@ -269,6 +269,8 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
> >  					       oflags);
> >  		if (pfile_info == NULL)
> >  			rc = -ENOMEM;
> > +	} else {
> > +		CIFSSMBClose(xid, cifs_sb->tcon, *pnetfid);
> 
> May be this not needed here as a subsequent patch pushes
> cifs_new_fileinfo to the caller..?
> 
> >  	}
> >  
> >  posix_open_ret:
> > @@ -478,11 +480,7 @@ cifs_create_set_dentry:
> >  	else
> >  		cFYI(1, "Create worked, get_inode_info failed rc = %d", rc);
> >  
> > -	/* nfsd case - nfs srv does not set nd */
> > -	if ((nd == NULL) || (!(nd->flags & LOOKUP_OPEN))) {
> > -		/* mknod case - do not leave file open */
> > -		CIFSSMBClose(xid, tcon, fileHandle);
> > -	} else if (!(posix_create) && (newinode)) {
> > +	if (!posix_create && newinode) {
> 
> Here too, posix_create is going to go..
> 
> May be lining this patch after [3/4] is a good idea..? Would make the
> patchset simpler, I think.
> 
> 
> >  		struct cifsFileInfo *pfile_info;
> >  		/*
> >  		 * cifs_fill_filedata() takes care of setting cifsFileInfo
> > @@ -492,7 +490,10 @@ cifs_create_set_dentry:
> >  					       nd->path.mnt, oflags);
> >  		if (pfile_info == NULL)
> >  			rc = -ENOMEM;
> > +	} else {
> > +		CIFSSMBClose(xid, tcon, fileHandle);
> >  	}
> 
> minor nit:     ^^^ unneeded braces..
> 
> > +
> >  cifs_create_out:
> >  	kfree(buf);
> >  	kfree(full_path);
> 
> 

Good point. I'll move this one to near the end of the set.

-- 
Jeff Layton <jlayton@redhat.com>

[PATCH 3/4] cifs: move cifs_new_fileinfo call out of cifs_posix_open

[Jeff Layton]

Having cifs_posix_open call cifs_new_fileinfo is problematic and
inconsistent with how "regular" opens work. It's also buggy as
cifs_reopen_file calls this function on a reconnect, which creates a new
fileinfo struct that just gets leaked.

Push it out into the callers. This also allows us to get rid of the
"mnt" arg to cifs_posix_open.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifsproto.h |    1 -
 fs/cifs/dir.c       |   38 +++++++++++++++-----------------------
 fs/cifs/file.c      |   17 ++++++++++++-----
 3 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index fb1657e..fb6318b 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -106,7 +106,6 @@ extern struct cifsFileInfo *cifs_new_fileinfo(struct inode *newinode,
 				__u16 fileHandle, struct file *file,
 				struct vfsmount *mnt, unsigned int oflags);
 extern int cifs_posix_open(char *full_path, struct inode **pinode,
-				struct vfsmount *mnt,
 				struct super_block *sb,
 				int mode, int oflags,
 				__u32 *poplock, __u16 *pnetfid, int xid);
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index e11ee2e..fc3129b 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -188,8 +188,7 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle,
 }
 
 int cifs_posix_open(char *full_path, struct inode **pinode,
-			struct vfsmount *mnt, struct super_block *sb,
-			int mode, int oflags,
+			struct super_block *sb, int mode, int oflags,
 			__u32 *poplock, __u16 *pnetfid, int xid)
 {
 	int rc;
@@ -258,21 +257,6 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
 		cifs_fattr_to_inode(*pinode, &fattr);
 	}
 
-	/*
-	 * cifs_fill_filedata() takes care of setting cifsFileInfo pointer to
-	 * file->private_data.
-	 */
-	if (mnt) {
-		struct cifsFileInfo *pfile_info;
-
-		pfile_info = cifs_new_fileinfo(*pinode, *pnetfid, NULL, mnt,
-					       oflags);
-		if (pfile_info == NULL)
-			rc = -ENOMEM;
-	} else {
-		CIFSSMBClose(xid, cifs_sb->tcon, *pnetfid);
-	}
-
 posix_open_ret:
 	kfree(presp_data);
 	return rc;
@@ -300,7 +284,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
 	int create_options = CREATE_NOT_DIR;
 	__u32 oplock = 0;
 	int oflags;
-	bool posix_create = false;
 	/*
 	 * BB below access is probably too much for mknod to request
 	 *    but we have to do query and setpathinfo so requesting
@@ -341,7 +324,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
 	    (CIFS_UNIX_POSIX_PATH_OPS_CAP &
 			le64_to_cpu(tcon->fsUnixInfo.Capability))) {
 		rc = cifs_posix_open(full_path, &newinode,
-			nd ? nd->path.mnt : NULL,
 			inode->i_sb, mode, oflags, &oplock, &fileHandle, xid);
 		/* EIO could indicate that (posix open) operation is not
 		   supported, despite what server claimed in capability
@@ -349,7 +331,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
 		   handled in posix open */
 
 		if (rc == 0) {
-			posix_create = true;
 			if (newinode == NULL) /* query inode info */
 				goto cifs_create_get_file_info;
 			else /* success, no need to query */
@@ -480,7 +461,7 @@ cifs_create_set_dentry:
 	else
 		cFYI(1, "Create worked, get_inode_info failed rc = %d", rc);
 
-	if (!posix_create && newinode) {
+	if (newinode) {
 		struct cifsFileInfo *pfile_info;
 		/*
 		 * cifs_fill_filedata() takes care of setting cifsFileInfo
@@ -637,6 +618,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 	bool posix_open = false;
 	struct cifs_sb_info *cifs_sb;
 	struct cifsTconInfo *pTcon;
+	struct cifsFileInfo *cfile;
 	struct inode *newInode = NULL;
 	char *full_path = NULL;
 	struct file *filp;
@@ -705,7 +687,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 		if (nd && !(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY)) &&
 		     (nd->flags & LOOKUP_OPEN) && !pTcon->broken_posix_open &&
 		     (nd->intent.open.flags & O_CREAT)) {
-			rc = cifs_posix_open(full_path, &newInode, nd->path.mnt,
+			rc = cifs_posix_open(full_path, &newInode,
 					parent_dir_inode->i_sb,
 					nd->intent.open.create_mode,
 					nd->intent.open.flags, &oplock,
@@ -735,8 +717,17 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 		else
 			direntry->d_op = &cifs_dentry_ops;
 		d_add(direntry, newInode);
-		if (posix_open)
+		if (posix_open) {
+			cfile = cifs_new_fileinfo(newInode, fileHandle, NULL,
+						  nd->path.mnt,
+						  nd->intent.open.flags);
+			if (cfile == NULL) {
+				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 */
 		renew_parental_timestamps(direntry);
@@ -761,6 +752,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 		res = ERR_PTR(rc);
 	}
 
+lookup_out:
 	kfree(full_path);
 	FreeXid(xid);
 	return res;
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index a83541e..001e916 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -299,8 +299,7 @@ int cifs_open(struct inode *inode, struct file *file)
 		int oflags = (int) cifs_posix_convert_flags(file->f_flags);
 		oflags |= SMB_O_CREAT;
 		/* can not refresh inode info since size could be stale */
-		rc = cifs_posix_open(full_path, &inode, file->f_path.mnt,
-				inode->i_sb,
+		rc = cifs_posix_open(full_path, &inode, inode->i_sb,
 				cifs_sb->mnt_file_mode /* ignored */,
 				oflags, &oplock, &netfid, xid);
 		if (rc == 0) {
@@ -308,7 +307,16 @@ int cifs_open(struct inode *inode, struct file *file)
 			/* no need for special case handling of setting mode
 			   on read only files needed here */
 
-			pCifsFile = cifs_fill_filedata(file);
+			pCifsFile = cifs_new_fileinfo(inode, netfid, file,
+							file->f_path.mnt,
+							oflags);
+			if (pCifsFile == NULL) {
+				CIFSSMBClose(xid, tcon, netfid);
+				rc = -ENOMEM;
+				goto out;
+			}
+			file->private_data = pCifsFile;
+
 			cifs_posix_open_inode_helper(inode, file, pCifsInode,
 						     oplock, netfid);
 			goto out;
@@ -513,8 +521,7 @@ reopen_error_exit:
 			le64_to_cpu(tcon->fsUnixInfo.Capability))) {
 		int oflags = (int) cifs_posix_convert_flags(file->f_flags);
 		/* can not refresh inode info since size could be stale */
-		rc = cifs_posix_open(full_path, NULL, file->f_path.mnt,
-				inode->i_sb,
+		rc = cifs_posix_open(full_path, NULL, inode->i_sb,
 				cifs_sb->mnt_file_mode /* ignored */,
 				oflags, &oplock, &netfid, xid);
 		if (rc == 0) {
-- 
1.6.6.1

Re: [linux-cifs-client] [PATCH 3/4] cifs: move cifs_new_fileinfo call out of cifs_posix_open

[Suresh Jayaraman]

On 05/21/2010 11:55 PM, Jeff Layton wrote:
> Having cifs_posix_open call cifs_new_fileinfo is problematic and
> inconsistent with how "regular" opens work. It's also buggy as
> cifs_reopen_file calls this function on a reconnect, which creates a new
> fileinfo struct that just gets leaked.
> 
> Push it out into the callers. This also allows us to get rid of the
> "mnt" arg to cifs_posix_open.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/cifsproto.h |    1 -
>  fs/cifs/dir.c       |   38 +++++++++++++++-----------------------
>  fs/cifs/file.c      |   17 ++++++++++++-----
>  3 files changed, 27 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index fb1657e..fb6318b 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -106,7 +106,6 @@ extern struct cifsFileInfo *cifs_new_fileinfo(struct inode *newinode,
>  				__u16 fileHandle, struct file *file,
>  				struct vfsmount *mnt, unsigned int oflags);
>  extern int cifs_posix_open(char *full_path, struct inode **pinode,
> -				struct vfsmount *mnt,
>  				struct super_block *sb,
>  				int mode, int oflags,
>  				__u32 *poplock, __u16 *pnetfid, int xid);
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index e11ee2e..fc3129b 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -188,8 +188,7 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle,
>  }
>  
>  int cifs_posix_open(char *full_path, struct inode **pinode,
> -			struct vfsmount *mnt, struct super_block *sb,
> -			int mode, int oflags,
> +			struct super_block *sb, int mode, int oflags,
>  			__u32 *poplock, __u16 *pnetfid, int xid)
>  {
>  	int rc;
> @@ -258,21 +257,6 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
>  		cifs_fattr_to_inode(*pinode, &fattr);
>  	}
>  
> -	/*
> -	 * cifs_fill_filedata() takes care of setting cifsFileInfo pointer to
> -	 * file->private_data.
> -	 */
> -	if (mnt) {
> -		struct cifsFileInfo *pfile_info;
> -
> -		pfile_info = cifs_new_fileinfo(*pinode, *pnetfid, NULL, mnt,
> -					       oflags);
> -		if (pfile_info == NULL)
> -			rc = -ENOMEM;
> -	} else {
> -		CIFSSMBClose(xid, cifs_sb->tcon, *pnetfid);
> -	}
> -
>  posix_open_ret:
>  	kfree(presp_data);
>  	return rc;
> @@ -300,7 +284,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
>  	int create_options = CREATE_NOT_DIR;
>  	__u32 oplock = 0;
>  	int oflags;
> -	bool posix_create = false;
>  	/*
>  	 * BB below access is probably too much for mknod to request
>  	 *    but we have to do query and setpathinfo so requesting
> @@ -341,7 +324,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
>  	    (CIFS_UNIX_POSIX_PATH_OPS_CAP &
>  			le64_to_cpu(tcon->fsUnixInfo.Capability))) {
>  		rc = cifs_posix_open(full_path, &newinode,
> -			nd ? nd->path.mnt : NULL,
>  			inode->i_sb, mode, oflags, &oplock, &fileHandle, xid);
>  		/* EIO could indicate that (posix open) operation is not
>  		   supported, despite what server claimed in capability
> @@ -349,7 +331,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
>  		   handled in posix open */
>  
>  		if (rc == 0) {
> -			posix_create = true;
>  			if (newinode == NULL) /* query inode info */
>  				goto cifs_create_get_file_info;
>  			else /* success, no need to query */
> @@ -480,7 +461,7 @@ cifs_create_set_dentry:
>  	else
>  		cFYI(1, "Create worked, get_inode_info failed rc = %d", rc);
>  
> -	if (!posix_create && newinode) {
> +	if (newinode) {
>  		struct cifsFileInfo *pfile_info;
>  		/*
>  		 * cifs_fill_filedata() takes care of setting cifsFileInfo
> @@ -637,6 +618,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>  	bool posix_open = false;
>  	struct cifs_sb_info *cifs_sb;
>  	struct cifsTconInfo *pTcon;
> +	struct cifsFileInfo *cfile;
>  	struct inode *newInode = NULL;
>  	char *full_path = NULL;
>  	struct file *filp;
> @@ -705,7 +687,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>  		if (nd && !(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY)) &&
>  		     (nd->flags & LOOKUP_OPEN) && !pTcon->broken_posix_open &&
>  		     (nd->intent.open.flags & O_CREAT)) {
> -			rc = cifs_posix_open(full_path, &newInode, nd->path.mnt,
> +			rc = cifs_posix_open(full_path, &newInode,
>  					parent_dir_inode->i_sb,
>  					nd->intent.open.create_mode,
>  					nd->intent.open.flags, &oplock,
> @@ -735,8 +717,17 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>  		else
>  			direntry->d_op = &cifs_dentry_ops;
>  		d_add(direntry, newInode);
> -		if (posix_open)
> +		if (posix_open) {
> +			cfile = cifs_new_fileinfo(newInode, fileHandle, NULL,
> +						  nd->path.mnt,
> +						  nd->intent.open.flags);
> +			if (cfile == NULL) {
> +				CIFSSMBClose(xid, pTcon, fileHandle);
> +				res = ERR_PTR(ENOMEM);

Should be ERR_PTR(-ENOMEM)?

If you are planning to drop patch [1/4], res goes undeclared.

> +				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 */
>  		renew_parental_timestamps(direntry);
> @@ -761,6 +752,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
>  		res = ERR_PTR(rc);
>  	}
>  
> +lookup_out:
>  	kfree(full_path);
>  	FreeXid(xid);
>  	return res;
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index a83541e..001e916 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -299,8 +299,7 @@ int cifs_open(struct inode *inode, struct file *file)
>  		int oflags = (int) cifs_posix_convert_flags(file->f_flags);
>  		oflags |= SMB_O_CREAT;
>  		/* can not refresh inode info since size could be stale */
> -		rc = cifs_posix_open(full_path, &inode, file->f_path.mnt,
> -				inode->i_sb,
> +		rc = cifs_posix_open(full_path, &inode, inode->i_sb,
>  				cifs_sb->mnt_file_mode /* ignored */,
>  				oflags, &oplock, &netfid, xid);
>  		if (rc == 0) {
> @@ -308,7 +307,16 @@ int cifs_open(struct inode *inode, struct file *file)
>  			/* no need for special case handling of setting mode
>  			   on read only files needed here */
>  
> -			pCifsFile = cifs_fill_filedata(file);
> +			pCifsFile = cifs_new_fileinfo(inode, netfid, file,
> +							file->f_path.mnt,
> +							oflags);
> +			if (pCifsFile == NULL) {
> +				CIFSSMBClose(xid, tcon, netfid);
> +				rc = -ENOMEM;
> +				goto out;
> +			}
> +			file->private_data = pCifsFile;
> +
>  			cifs_posix_open_inode_helper(inode, file, pCifsInode,
>  						     oplock, netfid);
>  			goto out;
> @@ -513,8 +521,7 @@ reopen_error_exit:
>  			le64_to_cpu(tcon->fsUnixInfo.Capability))) {
>  		int oflags = (int) cifs_posix_convert_flags(file->f_flags);
>  		/* can not refresh inode info since size could be stale */
> -		rc = cifs_posix_open(full_path, NULL, file->f_path.mnt,
> -				inode->i_sb,
> +		rc = cifs_posix_open(full_path, NULL, inode->i_sb,
>  				cifs_sb->mnt_file_mode /* ignored */,
>  				oflags, &oplock, &netfid, xid);
>  		if (rc == 0) {


-- 
Suresh Jayaraman

Re: [PATCH 3/4] cifs: move cifs_new_fileinfo call out of cifs_posix_open

[Jeff Layton]

On Mon, 24 May 2010 12:20:28 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> On 05/21/2010 11:55 PM, Jeff Layton wrote:
> > Having cifs_posix_open call cifs_new_fileinfo is problematic and
> > inconsistent with how "regular" opens work. It's also buggy as
> > cifs_reopen_file calls this function on a reconnect, which creates a new
> > fileinfo struct that just gets leaked.
> > 
> > Push it out into the callers. This also allows us to get rid of the
> > "mnt" arg to cifs_posix_open.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  fs/cifs/cifsproto.h |    1 -
> >  fs/cifs/dir.c       |   38 +++++++++++++++-----------------------
> >  fs/cifs/file.c      |   17 ++++++++++++-----
> >  3 files changed, 27 insertions(+), 29 deletions(-)
> > 
> > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> > index fb1657e..fb6318b 100644
> > --- a/fs/cifs/cifsproto.h
> > +++ b/fs/cifs/cifsproto.h
> > @@ -106,7 +106,6 @@ extern struct cifsFileInfo *cifs_new_fileinfo(struct inode *newinode,
> >  				__u16 fileHandle, struct file *file,
> >  				struct vfsmount *mnt, unsigned int oflags);
> >  extern int cifs_posix_open(char *full_path, struct inode **pinode,
> > -				struct vfsmount *mnt,
> >  				struct super_block *sb,
> >  				int mode, int oflags,
> >  				__u32 *poplock, __u16 *pnetfid, int xid);
> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > index e11ee2e..fc3129b 100644
> > --- a/fs/cifs/dir.c
> > +++ b/fs/cifs/dir.c
> > @@ -188,8 +188,7 @@ cifs_new_fileinfo(struct inode *newinode, __u16 fileHandle,
> >  }
> >  
> >  int cifs_posix_open(char *full_path, struct inode **pinode,
> > -			struct vfsmount *mnt, struct super_block *sb,
> > -			int mode, int oflags,
> > +			struct super_block *sb, int mode, int oflags,
> >  			__u32 *poplock, __u16 *pnetfid, int xid)
> >  {
> >  	int rc;
> > @@ -258,21 +257,6 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
> >  		cifs_fattr_to_inode(*pinode, &fattr);
> >  	}
> >  
> > -	/*
> > -	 * cifs_fill_filedata() takes care of setting cifsFileInfo pointer to
> > -	 * file->private_data.
> > -	 */
> > -	if (mnt) {
> > -		struct cifsFileInfo *pfile_info;
> > -
> > -		pfile_info = cifs_new_fileinfo(*pinode, *pnetfid, NULL, mnt,
> > -					       oflags);
> > -		if (pfile_info == NULL)
> > -			rc = -ENOMEM;
> > -	} else {
> > -		CIFSSMBClose(xid, cifs_sb->tcon, *pnetfid);
> > -	}
> > -
> >  posix_open_ret:
> >  	kfree(presp_data);
> >  	return rc;
> > @@ -300,7 +284,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
> >  	int create_options = CREATE_NOT_DIR;
> >  	__u32 oplock = 0;
> >  	int oflags;
> > -	bool posix_create = false;
> >  	/*
> >  	 * BB below access is probably too much for mknod to request
> >  	 *    but we have to do query and setpathinfo so requesting
> > @@ -341,7 +324,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
> >  	    (CIFS_UNIX_POSIX_PATH_OPS_CAP &
> >  			le64_to_cpu(tcon->fsUnixInfo.Capability))) {
> >  		rc = cifs_posix_open(full_path, &newinode,
> > -			nd ? nd->path.mnt : NULL,
> >  			inode->i_sb, mode, oflags, &oplock, &fileHandle, xid);
> >  		/* EIO could indicate that (posix open) operation is not
> >  		   supported, despite what server claimed in capability
> > @@ -349,7 +331,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
> >  		   handled in posix open */
> >  
> >  		if (rc == 0) {
> > -			posix_create = true;
> >  			if (newinode == NULL) /* query inode info */
> >  				goto cifs_create_get_file_info;
> >  			else /* success, no need to query */
> > @@ -480,7 +461,7 @@ cifs_create_set_dentry:
> >  	else
> >  		cFYI(1, "Create worked, get_inode_info failed rc = %d", rc);
> >  
> > -	if (!posix_create && newinode) {
> > +	if (newinode) {
> >  		struct cifsFileInfo *pfile_info;
> >  		/*
> >  		 * cifs_fill_filedata() takes care of setting cifsFileInfo
> > @@ -637,6 +618,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> >  	bool posix_open = false;
> >  	struct cifs_sb_info *cifs_sb;
> >  	struct cifsTconInfo *pTcon;
> > +	struct cifsFileInfo *cfile;
> >  	struct inode *newInode = NULL;
> >  	char *full_path = NULL;
> >  	struct file *filp;
> > @@ -705,7 +687,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> >  		if (nd && !(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY)) &&
> >  		     (nd->flags & LOOKUP_OPEN) && !pTcon->broken_posix_open &&
> >  		     (nd->intent.open.flags & O_CREAT)) {
> > -			rc = cifs_posix_open(full_path, &newInode, nd->path.mnt,
> > +			rc = cifs_posix_open(full_path, &newInode,
> >  					parent_dir_inode->i_sb,
> >  					nd->intent.open.create_mode,
> >  					nd->intent.open.flags, &oplock,
> > @@ -735,8 +717,17 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> >  		else
> >  			direntry->d_op = &cifs_dentry_ops;
> >  		d_add(direntry, newInode);
> > -		if (posix_open)
> > +		if (posix_open) {
> > +			cfile = cifs_new_fileinfo(newInode, fileHandle, NULL,
> > +						  nd->path.mnt,
> > +						  nd->intent.open.flags);
> > +			if (cfile == NULL) {
> > +				CIFSSMBClose(xid, pTcon, fileHandle);
> > +				res = ERR_PTR(ENOMEM);
> 
> Should be ERR_PTR(-ENOMEM)?
> 
> If you are planning to drop patch [1/4], res goes undeclared.
> 

Good catch, will fix in next set.

> > +				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 */
> >  		renew_parental_timestamps(direntry);
> > @@ -761,6 +752,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
> >  		res = ERR_PTR(rc);
> >  	}
> >  
> > +lookup_out:
> >  	kfree(full_path);
> >  	FreeXid(xid);
> >  	return res;
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index a83541e..001e916 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -299,8 +299,7 @@ int cifs_open(struct inode *inode, struct file *file)
> >  		int oflags = (int) cifs_posix_convert_flags(file->f_flags);
> >  		oflags |= SMB_O_CREAT;
> >  		/* can not refresh inode info since size could be stale */
> > -		rc = cifs_posix_open(full_path, &inode, file->f_path.mnt,
> > -				inode->i_sb,
> > +		rc = cifs_posix_open(full_path, &inode, inode->i_sb,
> >  				cifs_sb->mnt_file_mode /* ignored */,
> >  				oflags, &oplock, &netfid, xid);
> >  		if (rc == 0) {
> > @@ -308,7 +307,16 @@ int cifs_open(struct inode *inode, struct file *file)
> >  			/* no need for special case handling of setting mode
> >  			   on read only files needed here */
> >  
> > -			pCifsFile = cifs_fill_filedata(file);
> > +			pCifsFile = cifs_new_fileinfo(inode, netfid, file,
> > +							file->f_path.mnt,
> > +							oflags);
> > +			if (pCifsFile == NULL) {
> > +				CIFSSMBClose(xid, tcon, netfid);
> > +				rc = -ENOMEM;
> > +				goto out;
> > +			}
> > +			file->private_data = pCifsFile;
> > +
> >  			cifs_posix_open_inode_helper(inode, file, pCifsInode,
> >  						     oplock, netfid);
> >  			goto out;
> > @@ -513,8 +521,7 @@ reopen_error_exit:
> >  			le64_to_cpu(tcon->fsUnixInfo.Capability))) {
> >  		int oflags = (int) cifs_posix_convert_flags(file->f_flags);
> >  		/* can not refresh inode info since size could be stale */
> > -		rc = cifs_posix_open(full_path, NULL, file->f_path.mnt,
> > -				inode->i_sb,
> > +		rc = cifs_posix_open(full_path, NULL, inode->i_sb,
> >  				cifs_sb->mnt_file_mode /* ignored */,
> >  				oflags, &oplock, &netfid, xid);
> >  		if (rc == 0) {
> 
> 


-- 
Jeff Layton <jlayton@redhat.com>

[PATCH 4/4] cifs: pass instantiated filp back after open call

[Jeff Layton]

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

Re: [PATCH 0/4] cifs: fix "Busy inodes after umount" issues (RFC)

[Suresh Jayaraman]

On 05/21/2010 11:55 PM, Jeff Layton wrote:
> We've had a spate of "Busy inodes after umount" problems in recent
> kernels. With the help of a reproducer that Suresh provided, I tracked
> down the cause of one of these and wrote it up here:
> 
>     https://bugzilla.samba.org/show_bug.cgi?id=7433
> 
> The main problem is that CIFS opens files during create operations,
> puts these files on a list and then expects that a subsequent open
> operation will find those on the list and make them full, productive
> members of society.
> 
> This expectation is wrong however. There's no guarantee that cifs_open
> will be called at all after a create. There are several scenarios that
> can prevent it from occuring. When this happens, these entries get left
> dangling on the list and nothing will ever clean them up. Recent changes
> have made it so that cifsFileInfo structs hold an inode reference as
> well, which is what actually leads to the busy inodes after umount
> problems.
> 
> This patch is intended to fix this in the right way. It has the create
> operations properly use lookup_instantiate_filp to create an open file
> during the operation that does the actual create. With this, there's
> no need to have cifs_open scrape these entries off the list.

I think this is how we should do it. This makes the code a lot cleaner
and simpler to follow.

> This fixes the busy inodes problem I was able to reproduce. It's not
> very well tested yet however, and I could stand for someone else to
> review it and help test it.
> 

However, I still see "VFS: Busy inode" errors with my reproducer (with
all patches except 1/4). Perhaps it has made the problem less frequent
or it has got something to do with quick inode recyling.

Nevertheless, I think this patchset is a good step in the right direction.

Thanks,

-- 
Suresh Jayaraman

Re: [linux-cifs-client] [PATCH 0/4] cifs: fix "Busy inodes after umount" issues (RFC)

[Jeff Layton]

On Mon, 24 May 2010 12:43:52 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> On 05/21/2010 11:55 PM, Jeff Layton wrote:
> > We've had a spate of "Busy inodes after umount" problems in recent
> > kernels. With the help of a reproducer that Suresh provided, I tracked
> > down the cause of one of these and wrote it up here:
> > 
> >     https://bugzilla.samba.org/show_bug.cgi?id=7433
> > 
> > The main problem is that CIFS opens files during create operations,
> > puts these files on a list and then expects that a subsequent open
> > operation will find those on the list and make them full, productive
> > members of society.
> > 
> > This expectation is wrong however. There's no guarantee that cifs_open
> > will be called at all after a create. There are several scenarios that
> > can prevent it from occuring. When this happens, these entries get left
> > dangling on the list and nothing will ever clean them up. Recent changes
> > have made it so that cifsFileInfo structs hold an inode reference as
> > well, which is what actually leads to the busy inodes after umount
> > problems.
> > 
> > This patch is intended to fix this in the right way. It has the create
> > operations properly use lookup_instantiate_filp to create an open file
> > during the operation that does the actual create. With this, there's
> > no need to have cifs_open scrape these entries off the list.
> 
> I think this is how we should do it. This makes the code a lot cleaner
> and simpler to follow.
> 
> > This fixes the busy inodes problem I was able to reproduce. It's not
> > very well tested yet however, and I could stand for someone else to
> > review it and help test it.
> > 
> 
> However, I still see "VFS: Busy inode" errors with my reproducer (with
> all patches except 1/4). Perhaps it has made the problem less frequent
> or it has got something to do with quick inode recyling.
> 
> Nevertheless, I think this patchset is a good step in the right direction.

Good to know. That probably means there's more than one problem. You
may need to get out a debugger and see if you can figure out why you're
seeing that on your machines.

With my test for this (just running fsstress on the mount), I'm also
seeing a memory leak in the kmalloc-512 slab that's potentially
related as well. I'm not sure yet though whether that preexists this
patchset or not.

-- 
Jeff Layton <jlayton@redhat.com>