NFS: Serialize nfs_readdir()
diff mbox

Message ID a87df5d5ada87834a1cb9f1ab12ed5113d80eda7.1481117708.git.bcodding@redhat.com
State New
Headers show

Commit Message

Benjamin Coddington Dec. 7, 2016, 1:37 p.m. UTC
Optimizations to NFS to choose between READDIR and READDIRPLUS don't expect
concurrent users of nfs_readdir(), and can cause the pagecache to
repeatedly be invalidated unnecessarily for this case.  Fix this by
serializing nfs_readdir() on the directory's rwsem.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/dir.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Christoph Hellwig Dec. 7, 2016, 4:30 p.m. UTC | #1
On Wed, Dec 07, 2016 at 08:37:02AM -0500, Benjamin Coddington wrote:
> Optimizations to NFS to choose between READDIR and READDIRPLUS don't expect
> concurrent users of nfs_readdir(), and can cause the pagecache to
> repeatedly be invalidated unnecessarily for this case.  Fix this by
> serializing nfs_readdir() on the directory's rwsem.

Just implement .iterate instead of .iterate_shared and you'll get the
serialization for free.  While doing that please add a comment on why
it's serialized.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
kernel test robot Dec. 7, 2016, 5:01 p.m. UTC | #2
Hi Benjamin,

[auto build test ERROR on nfs/linux-next]
[also build test ERROR on v4.9-rc8 next-20161207]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Benjamin-Coddington/NFS-Serialize-nfs_readdir/20161208-001039
base:   git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: xtensa-common_defconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   fs/nfs/dir.c: In function 'nfs_readdir':
>> fs/nfs/dir.c:921:18: error: 'struct nfs_inode' has no member named 'rwsem'
     down_write(&nfsi->rwsem);
                     ^
   fs/nfs/dir.c:963:16: error: 'struct nfs_inode' has no member named 'rwsem'
     up_write(&nfsi->rwsem);
                   ^

vim +921 fs/nfs/dir.c

   915		 * *desc->dir_cookie has the cookie for the next entry. We have
   916		 * to either find the entry with the appropriate number or
   917		 * revalidate the cookie.
   918		 */
   919		memset(desc, 0, sizeof(*desc));
   920	
 > 921		down_write(&nfsi->rwsem);
   922		desc->file = file;
   923		desc->ctx = ctx;
   924		desc->dir_cookie = &dir_ctx->dir_cookie;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Benjamin Coddington Dec. 7, 2016, 7:40 p.m. UTC | #3
On 7 Dec 2016, at 11:30, Christoph Hellwig wrote:

> On Wed, Dec 07, 2016 at 08:37:02AM -0500, Benjamin Coddington wrote:
>> Optimizations to NFS to choose between READDIR and READDIRPLUS don't 
>> expect
>> concurrent users of nfs_readdir(), and can cause the pagecache to
>> repeatedly be invalidated unnecessarily for this case.  Fix this by
>> serializing nfs_readdir() on the directory's rwsem.
>
> Just implement .iterate instead of .iterate_shared and you'll get the
> serialization for free.  While doing that please add a comment on why
> it's serialized.

I had it in my head that Al was trying to get rid of .iterate, otherwise
yes, this would be more sensible.  I think it'll still benefit from
.iterate_shared in the regular case (the case where we're not trying to 
use
READDIRPLUS), so I'll try out Trond's suggestion to fix it up before 
chasing
this down.

Ben
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 53e6d6a4bc9c..8321252a4c8d 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -900,6 +900,7 @@  static int nfs_readdir(struct file *file, struct dir_context *ctx)
 {
 	struct dentry	*dentry = file_dentry(file);
 	struct inode	*inode = d_inode(dentry);
+	struct nfs_inode *nfsi = NFS_I(inode);
 	nfs_readdir_descriptor_t my_desc,
 			*desc = &my_desc;
 	struct nfs_open_dir_context *dir_ctx = file->private_data;
@@ -917,6 +918,7 @@  static int nfs_readdir(struct file *file, struct dir_context *ctx)
 	 */
 	memset(desc, 0, sizeof(*desc));
 
+	down_write(&nfsi->rwsem);
 	desc->file = file;
 	desc->ctx = ctx;
 	desc->dir_cookie = &dir_ctx->dir_cookie;
@@ -958,6 +960,7 @@  static int nfs_readdir(struct file *file, struct dir_context *ctx)
 			break;
 	} while (!desc->eof);
 out:
+	up_write(&nfsi->rwsem);
 	if (res > 0)
 		res = 0;
 	dfprintk(FILE, "NFS: readdir(%pD2) returns %d\n", file, res);