[PATCH 0/8] cifs: patches for 2.6.35

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/8] cifs: patches for 2.6.35

Jeff Layton-2
Hi Steve,

The following is a comprehensive set of patches that I have queued up
for 2.6.35. They fix a number of important bugs, including:

- a page refcount leak introduced in a recent patch by Nick P.

- a long-standing bug involving busy-file renames

- a set of fixes for all known "Busy inodes after umount..." problems
  (see https://bugzilla.samba.org/show_bug.cgi?id=7433)

It also contains the "drop_inode" patch which helps reduce memory
utilization when server inode numbers aren't used.

It may be easiest to pull these from my git repo on kernel.org. Pull
request follows:

The following changes since commit 67a3e12b05e055c0415c556a315a3d3eb637e29e:
  Linus Torvalds (1):
        Linux 2.6.35-rc1

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jlayton/linux.git for-sfrench

Jeff Layton (7):
      cifs: fix page refcount leak
      cifs: don't attempt busy-file rename unless it's in same directory
      cifs: implement drop_inode superblock op
      cifs: move cifs_new_fileinfo call out of cifs_posix_open
      cifs: pass instantiated filp back after open call
      cifs: clean up arguments to cifs_open_inode_helper
      cifs: don't call cifs_new_fileinfo unless cifs_open succeeds

Suresh Jayaraman (1):
      cifs: don't ignore cifs_posix_open_inode_helper return value

 fs/cifs/cifsfs.c    |   16 +++++++--
 fs/cifs/cifsproto.h |    1 -
 fs/cifs/dir.c       |   76 ++++++++++++++++++++++----------------
 fs/cifs/file.c      |  101 ++++++++++++++++++---------------------------------
 fs/cifs/inode.c     |    4 ++
 5 files changed, 97 insertions(+), 101 deletions(-)

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/8] cifs: fix page refcount leak

Jeff Layton-2
Commit 315e995c63a15cb4d4efdbfd70fe2db191917f7a is causing OOM kills
when stress-testing a CIFS filesystem. The VFS readpages operation takes
a page reference. The older code just handed this reference off to the
page cache, but the new code takes an extra one. The simplest fix is to
put the new reference after add_to_page_cache_lru.

Signed-off-by: Jeff Layton <[hidden email]>
Acked-by: Nick Piggin <[hidden email]>
---
 fs/cifs/file.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index f1ff785..75541af 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1952,6 +1952,7 @@ static void cifs_copy_cache_pages(struct address_space *mapping,
  bytes_read -= PAGE_CACHE_SIZE;
  continue;
  }
+ page_cache_release(page);
 
  target = kmap_atomic(page, KM_USER0);
 
--
1.6.6.1

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/8] cifs: don't attempt busy-file rename unless it's in same directory

Jeff Layton-2
In reply to this post by Jeff Layton-2
Busy-file renames don't actually work across directories, so we need
to limit this code to renames within the same dir.

This fixes the bug detailed here:

    https://bugzilla.redhat.com/show_bug.cgi?id=591938

Signed-off-by: Jeff Layton <[hidden email]>
---
 fs/cifs/inode.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 62b324f..6f0683c 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1401,6 +1401,10 @@ cifs_do_rename(int xid, struct dentry *from_dentry, const char *fromPath,
  if (rc == 0 || rc != -ETXTBSY)
  return rc;
 
+ /* open-file renames don't work across directories */
+ if (to_dentry->d_parent != from_dentry->d_parent)
+ return rc;
+
  /* open the file to be renamed -- we need DELETE perms */
  rc = CIFSSMBOpen(xid, pTcon, fromPath, FILE_OPEN, DELETE,
  CREATE_NOT_DIR, &srcfid, &oplock, NULL,
--
1.6.6.1

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/8] cifs: implement drop_inode superblock op

Jeff Layton-2
In reply to this post by Jeff Layton-2
The standard behavior for drop_inode is to delete the inode when the
last reference to it is put and the nlink count goes to 0. This helps
keep inodes that are still considered "not deleted" in cache as long as
possible even when there aren't dentries attached to them.

When server inode numbers are disabled, it's not possible for cifs_iget
to ever match an existing inode (since inode numbers are generated via
iunique). In this situation, cifs can keep a lot of inodes in cache that
will never be used again.

Implement a drop_inode routine that deletes the inode if server inode
numbers are disabled on the mount. This helps keep the cifs inode
caches down to a more manageable size when server inode numbers are
disabled.

Signed-off-by: Jeff Layton <[hidden email]>
---
 fs/cifs/cifsfs.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 78c02eb..484e52b 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -473,14 +473,24 @@ static int cifs_remount(struct super_block *sb, int *flags, char *data)
  return 0;
 }
 
+void cifs_drop_inode(struct inode *inode)
+{
+ struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
+
+ if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM)
+ return generic_drop_inode(inode);
+
+ return generic_delete_inode(inode);
+}
+
 static const struct super_operations cifs_super_ops = {
  .put_super = cifs_put_super,
  .statfs = cifs_statfs,
  .alloc_inode = cifs_alloc_inode,
  .destroy_inode = cifs_destroy_inode,
-/* .drop_inode    = generic_delete_inode,
- .delete_inode = cifs_delete_inode,  */  /* Do not need above two
- functions unless later we add lazy close of inodes or unless the
+ .drop_inode = cifs_drop_inode,
+/* .delete_inode = cifs_delete_inode,  */  /* Do not need above
+ function unless later we add lazy close of inodes or unless the
  kernel forgets to call us with the same number of releases (closes)
  as opens */
  .show_options = cifs_show_options,
--
1.6.6.1

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

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

Jeff Layton-2
In reply to this post by Jeff Layton-2
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
struct cifsFileInfo 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.

Finally, in the event that a cifsFileInfo isn't or can't be created, we
always want to close the filehandle out on the server as the client
won't have a record of the filehandle and can't actually use it. Make
sure that CIFSSMBClose is called in those cases.

Signed-off-by: Jeff Layton <[hidden email]>
---
 fs/cifs/cifsproto.h |    1 -
 fs/cifs/dir.c       |   43 ++++++++++++++++++-------------------------
 fs/cifs/file.c      |   17 ++++++++++++-----
 3 files changed, 30 insertions(+), 31 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 391816b..f49afb9 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,19 +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;
- }
-
 posix_open_ret:
  kfree(presp_data);
  return rc;
@@ -298,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
@@ -339,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
@@ -347,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 */
@@ -478,11 +461,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 (newinode && nd && (nd->flags & LOOKUP_OPEN)) {
  struct cifsFileInfo *pfile_info;
  /*
  * cifs_fill_filedata() takes care of setting cifsFileInfo
@@ -492,7 +471,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);
@@ -636,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;
@@ -703,7 +686,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,
@@ -733,8 +716,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);
+ rc = -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);
@@ -755,6 +747,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
  is a common return code */
  }
 
+lookup_out:
  kfree(full_path);
  FreeXid(xid);
  return ERR_PTR(rc);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 75541af..542e0c8 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

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

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

Jeff Layton-2
In reply to this post by Jeff Layton-2
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 <[hidden email]>
---
 fs/cifs/dir.c  |   35 +++++++++++++++++++++++++++--------
 fs/cifs/file.c |   44 ++------------------------------------------
 2 files changed, 29 insertions(+), 50 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index f49afb9..e7ae78b 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 && nd && (nd->flags & LOOKUP_OPEN)) {
  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);
  }
@@ -717,15 +728,23 @@ 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)) {
+ rc = PTR_ERR(filp);
+ CIFSSMBClose(xid, pTcon, fileHandle);
+ 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);
  rc = -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 542e0c8..9cbf0f0 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

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

[PATCH 6/8] cifs: clean up arguments to cifs_open_inode_helper

Jeff Layton-2
In reply to this post by Jeff Layton-2
...which takes a ton of unneeded arguments and does a lot more pointer
dereferencing than is really needed.

Signed-off-by: Jeff Layton <[hidden email]>
---
 fs/cifs/file.c |   30 ++++++++++++++----------------
 1 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 9cbf0f0..5b9d1f2 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -163,11 +163,11 @@ psx_client_can_cache:
 }
 
 /* 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,
+static inline int cifs_open_inode_helper(struct inode *inode,
  struct cifsTconInfo *pTcon, int *oplock, FILE_ALL_INFO *buf,
  char *full_path, int xid)
 {
+ struct cifsInodeInfo *pCifsInode = CIFS_I(inode);
  struct timespec temp;
  int rc;
 
@@ -181,36 +181,35 @@ static inline int cifs_open_inode_helper(struct inode *inode, struct file *file,
  /* if not oplocked, invalidate inode pages if mtime or file
    size changed */
  temp = cifs_NTtimeToUnix(buf->LastWriteTime);
- if (timespec_equal(&file->f_path.dentry->d_inode->i_mtime, &temp) &&
-   (file->f_path.dentry->d_inode->i_size ==
+ if (timespec_equal(&inode->i_mtime, &temp) &&
+   (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) {
+ if (inode->i_mapping) {
  /* BB no need to lock inode until after invalidate
  since namei code should already have it locked? */
- rc = filemap_write_and_wait(file->f_path.dentry->d_inode->i_mapping);
+ rc = filemap_write_and_wait(inode->i_mapping);
  if (rc != 0)
- CIFS_I(file->f_path.dentry->d_inode)->write_behind_rc = rc;
+ pCifsInode->write_behind_rc = rc;
  }
  cFYI(1, "invalidating remote inode since open detected it "
  "changed");
- invalidate_remote_inode(file->f_path.dentry->d_inode);
+ invalidate_remote_inode(inode);
  }
 
 client_can_cache:
  if (pTcon->unix_ext)
- rc = cifs_get_inode_info_unix(&file->f_path.dentry->d_inode,
- full_path, inode->i_sb, xid);
+ rc = cifs_get_inode_info_unix(&inode, full_path, inode->i_sb,
+      xid);
  else
- rc = cifs_get_inode_info(&file->f_path.dentry->d_inode,
- full_path, buf, inode->i_sb, xid, NULL);
+ rc = cifs_get_inode_info(&inode, full_path, buf, inode->i_sb,
+ xid, NULL);
 
  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);
+ cFYI(1, "Exclusive Oplock granted on inode %p", inode);
  } else if ((*oplock & 0xF) == OPLOCK_READ)
  pCifsInode->clientCanCacheRead = true;
 
@@ -367,8 +366,7 @@ int cifs_open(struct inode *inode, struct file *file)
  goto out;
  }
 
- rc = cifs_open_inode_helper(inode, file, pCifsInode, pCifsFile, tcon,
-    &oplock, buf, full_path, xid);
+ rc = cifs_open_inode_helper(inode, tcon, &oplock, buf, full_path, xid);
 
  if (oplock & CIFS_CREATE_ACTION) {
  /* time to set mode which we can not set earlier due to
--
1.6.6.1

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

[PATCH 7/8] cifs: don't ignore cifs_posix_open_inode_helper return value

Jeff Layton-2
In reply to this post by Jeff Layton-2
From: Suresh Jayaraman <[hidden email]>

...and ensure that we propagate the error back to avoid any surprises.

Signed-off-by: Suresh Jayaraman <[hidden email]>
---
 fs/cifs/file.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 5b9d1f2..02a2df9 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -277,8 +277,8 @@ int cifs_open(struct inode *inode, struct file *file)
  goto out;
  }
 
- cifs_posix_open_inode_helper(inode, file, pCifsInode,
-     oplock, netfid);
+ rc = cifs_posix_open_inode_helper(inode, file,
+ pCifsInode, oplock, netfid);
  goto out;
  } else if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
  if (tcon->ses->serverNOS)
--
1.6.6.1

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

[PATCH 8/8] cifs: don't call cifs_new_fileinfo unless cifs_open succeeds

Jeff Layton-2
In reply to this post by Jeff Layton-2
It's currently possible for cifs_open to fail after it has already
called cifs_new_fileinfo. In that situation, the new fileinfo will be
leaked as the caller doesn't call fput. That in turn leads to a busy
inodes after umount problem since the fileinfo holds and extra inode
reference now. Shuffle cifs_open around a bit so that it only calls
cifs_new_fileinfo if it's going to succeed.

Signed-off-by: Jeff Layton <[hidden email]>
---
 fs/cifs/file.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 02a2df9..409e4f5 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -268,17 +268,20 @@ int cifs_open(struct inode *inode, struct file *file)
  /* 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,
  file->f_path.mnt,
  oflags);
  if (pCifsFile == NULL) {
  CIFSSMBClose(xid, tcon, netfid);
  rc = -ENOMEM;
- goto out;
  }
-
- rc = cifs_posix_open_inode_helper(inode, file,
- pCifsInode, oplock, netfid);
  goto out;
  } else if ((rc == -EINVAL) || (rc == -EOPNOTSUPP)) {
  if (tcon->ses->serverNOS)
@@ -359,6 +362,10 @@ int cifs_open(struct inode *inode, struct file *file)
  goto out;
  }
 
+ rc = cifs_open_inode_helper(inode, tcon, &oplock, buf, full_path, xid);
+ if (rc != 0)
+ goto out;
+
  pCifsFile = cifs_new_fileinfo(inode, netfid, file, file->f_path.mnt,
  file->f_flags);
  if (pCifsFile == NULL) {
@@ -366,8 +373,6 @@ int cifs_open(struct inode *inode, struct file *file)
  goto out;
  }
 
- rc = cifs_open_inode_helper(inode, tcon, &oplock, buf, full_path, xid);
-
  if (oplock & CIFS_CREATE_ACTION) {
  /* time to set mode which we can not set earlier due to
    problems creating new read-only files */
--
1.6.6.1

_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/8] cifs: fix page refcount leak

Steve French-2
In reply to this post by Jeff Layton-2
merged. Will wait a few days before request upstream.

 Reviewing and checking the others.

On Tue, Jun 1, 2010 at 9:54 AM, Jeff Layton <[hidden email]> wrote:

> Commit 315e995c63a15cb4d4efdbfd70fe2db191917f7a is causing OOM kills
> when stress-testing a CIFS filesystem. The VFS readpages operation takes
> a page reference. The older code just handed this reference off to the
> page cache, but the new code takes an extra one. The simplest fix is to
> put the new reference after add_to_page_cache_lru.
>
> Signed-off-by: Jeff Layton <[hidden email]>
> Acked-by: Nick Piggin <[hidden email]>
> ---
>  fs/cifs/file.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index f1ff785..75541af 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -1952,6 +1952,7 @@ static void cifs_copy_cache_pages(struct address_space *mapping,
>                        bytes_read -= PAGE_CACHE_SIZE;
>                        continue;
>                }
> +               page_cache_release(page);
>
>                target = kmap_atomic(page, KM_USER0);
>
> --
> 1.6.6.1
>
>



--
Thanks,

Steve
_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/8] cifs: patches for 2.6.35

Suresh Jayaraman-2
In reply to this post by Jeff Layton-2
On 06/01/2010 08:24 PM, Jeff Layton wrote:

> Hi Steve,
>
> The following is a comprehensive set of patches that I have queued up
> for 2.6.35. They fix a number of important bugs, including:
>
> - a page refcount leak introduced in a recent patch by Nick P.
>
> - a long-standing bug involving busy-file renames
>
> - a set of fixes for all known "Busy inodes after umount..." problems
>   (see https://bugzilla.samba.org/show_bug.cgi?id=7433)
>
> It also contains the "drop_inode" patch which helps reduce memory
> utilization when server inode numbers aren't used.
>
> It may be easiest to pull these from my git repo on kernel.org. Pull
> request follows:
>
> The following changes since commit 67a3e12b05e055c0415c556a315a3d3eb637e29e:
>   Linus Torvalds (1):
>         Linux 2.6.35-rc1
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/jlayton/linux.git for-sfrench
>
> Jeff Layton (7):
>       cifs: fix page refcount leak
>       cifs: don't attempt busy-file rename unless it's in same directory
>       cifs: implement drop_inode superblock op
>       cifs: move cifs_new_fileinfo call out of cifs_posix_open
>       cifs: pass instantiated filp back after open call
>       cifs: clean up arguments to cifs_open_inode_helper
>       cifs: don't call cifs_new_fileinfo unless cifs_open succeeds

I have reviewed and tested [4/8], [5/8] [6/8] and [8/8] and I could no
longer reproduce the "VFS busy inodes" errors using my reproducer and I
don't see the leaks anymore..

Reviewed-and-Tested-by: Suresh Jayaraman <[hidden email]>


Thanks,

--
Suresh Jayaraman
_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client