Skip to contentSkip to conversation

readdir() as an inode operation

RFC In review Jan Blunck 4 participants 6 messages Oct 20, 2007 – Oct 31, 2007

Subsystems (5)
Memory management VFS OCFS2 HugeTLB +1 more subsystems
lore
Share & tools
lore

Jan Blunck

author
#0 lore raw

cover readdir() as an inode operation

This is a first try to move readdir() to become an inode operation. This is necessary for a VFS implementation of "something like union-mounts" where a readdir() needs to read the directory contents of multiple directories. Besides that the new interface is no longer giving the struct file to the filesystem implementations anymore.

Comments, please?
Jan

author
#1 lore raw

RFC 1/2 i_op->readdir: Change readdir() to be an inode operation

This patch adds a new readdir() inode operation. The purpose of this patch is to enable the VFS to support directory reading on a stack of directories. The new interface isn't passing the struct file to the filesystem implementation anymore. Normally the filesystem implementation shouldn't depend on any information in struct file except for the dentry, the cookie (f_pos) and the users credentials.

The new interface for the readdir inode operation is as follows:

int (*readdir) (struct dentry *dentry, loff_t *pos, void *private,

                filldir_t filler, void *dirent);

@dentry: the dentry of the directory
@pos: pointer to the cookie
@private: the credentials (at the moment it is still filp->private_data @filler: the filldir to call
@dirent: the dirent buffer

Signed-off-by Jan Blunck <[email protected]>
---
 fs/readdir.c       |   14 ++++++++++++--
 include/linux/fs.h |    2 ++
 2 files changed, 14 insertions(+), 2 deletions(-)
fs/readdir.c +12 -2
Index: b/fs/readdir.c
===================================================================
@@ -16,6 +16,7 @@
 #include <linux/security.h>
 #include <linux/syscalls.h>
 #include <linux/unistd.h>
+#include <linux/kallsyms.h>
 
 #include <asm/uaccess.h>
 

@@ -23,7 +24,8 @@ int vfs_readdir(struct file *file, filld
 {
 	struct inode *inode = file->f_path.dentry->d_inode;
 	int res = -ENOTDIR;
-	if (!file->f_op || !file->f_op->readdir)
+	if ((!file->f_op || !file->f_op->readdir) &&
+	    (!inode->i_op || !inode->i_op->readdir))
 		goto out;
 
 	res = security_file_permission(file, MAY_READ);

@@ -33,7 +35,15 @@ int vfs_readdir(struct file *file, filld
 	mutex_lock(&inode->i_mutex);
 	res = -ENOENT;
 	if (!IS_DEADDIR(inode)) {
-		res = file->f_op->readdir(file, buf, filler);
+		if (inode->i_op->readdir) {
+			printk(KERN_DEBUG "i_op->readdir @ ");
+			print_ip_sym((unsigned long)inode->i_op);
+			res = inode->i_op->readdir(file->f_path.dentry,
+						   &file->f_pos,
+						   file->private_data,
+						   filler, buf);
+		} else
+			res = file->f_op->readdir(file, buf, filler);
 		file_accessed(file);
 	}
 	mutex_unlock(&inode->i_mutex);
include/linux/fs.h +2 -0
Index: b/include/linux/fs.h
===================================================================
@@ -1214,6 +1214,8 @@ struct inode_operations {
 	int (*mkdir) (struct inode *,struct dentry *,int);
 	int (*rmdir) (struct inode *,struct dentry *);
 	int (*mknod) (struct inode *,struct dentry *,int,dev_t);
+	/* readdir(dentry, position, private/credential, filler, buffer) */
+	int (*readdir) (struct dentry *, loff_t *, void *, filldir_t, void *);
 	int (*rename) (struct inode *, struct dentry *,
 			struct inode *, struct dentry *);
 	int (*readlink) (struct dentry *, char __user *,int);
author
#2 lore raw

RFC 2/2 i_op->readdir: Change libfs users to the new interface

In reply to Jan Blunck

This patch changes dcache_readdir() to the new inode operations readdir interface. Hence all the users of libfs.c are changed to use the new interface too.

Signed-off-by Jan Blunck <[email protected]>
---
 fs/autofs4/autofs_i.h |    5 ++---
 fs/autofs4/root.c     |   41 ++++++++++++++++++++++++-----------------
 fs/cifs/inode.c       |    1 +
 fs/hugetlbfs/inode.c  |    1 +
 fs/libfs.c            |   27 ++++++++++++++-------------
 fs/ocfs2/dlm/dlmfs.c  |    1 +
 fs/ramfs/inode.c      |    1 +
 include/linux/fs.h    |    3 ++-
 mm/shmem.c            |    1 +
 9 files changed, 47 insertions(+), 34 deletions(-)
fs/autofs4/autofs_i.h +2 -3
Index: b/fs/autofs4/autofs_i.h
===================================================================
@@ -168,10 +168,9 @@ static inline int autofs4_ispending(stru
 	return pending;
 }
 
-static inline void autofs4_copy_atime(struct file *src, struct file *dst)
+static inline void autofs4_copy_atime(struct inode *src, struct inode *dst)
 {
-	dst->f_path.dentry->d_inode->i_atime =
-		src->f_path.dentry->d_inode->i_atime;
+	dst->i_atime = src->i_atime;
 	return;
 }
fs/autofs4/root.c +24 -17

113 lines not on this page · raw message

fs/cifs/inode.c +1 -0
Index: b/fs/cifs/inode.c
===================================================================
@@ -577,6 +577,7 @@ int cifs_get_inode_info(struct inode **p
 
 static const struct inode_operations cifs_ipc_inode_ops = {
 	.lookup = cifs_lookup,
+	.readdir = dcache_inode_readdir,
 };
 
 /* gets root inode */
fs/hugetlbfs/inode.c +1 -0
Index: b/fs/hugetlbfs/inode.c
===================================================================
@@ -719,6 +719,7 @@ static const struct inode_operations hug
 	.symlink	= hugetlbfs_symlink,
 	.mkdir		= hugetlbfs_mkdir,
 	.rmdir		= simple_rmdir,
+	.readdir	= dcache_inode_readdir,
 	.mknod		= hugetlbfs_mknod,
 	.rename		= simple_rename,
 	.setattr	= hugetlbfs_setattr,
fs/libfs.c +14 -13
Index: b/fs/libfs.c
===================================================================
@@ -125,32 +125,33 @@ static inline unsigned char dt_type(stru
  * both impossible due to the lock on directory.
  */
 
-int dcache_readdir(struct file * filp, void * dirent, filldir_t filldir)
+int dcache_inode_readdir(struct dentry *dentry, loff_t *pos,
+			 void *private_data,
+			 filldir_t filldir, void *dirent)
 {
-	struct dentry *dentry = filp->f_path.dentry;
-	struct dentry *cursor = filp->private_data;
+	struct dentry *cursor = private_data;
 	struct list_head *p, *q = &cursor->d_u.d_child;
 	ino_t ino;
-	int i = filp->f_pos;
+	int i = *pos;
 
 	switch (i) {
 		case 0:
 			ino = dentry->d_inode->i_ino;
 			if (filldir(dirent, ".", 1, i, ino, DT_DIR) < 0)
 				break;
-			filp->f_pos++;
+			(*pos)++;
 			i++;
 			/* fallthrough */
 		case 1:
 			ino = parent_ino(dentry);
 			if (filldir(dirent, "..", 2, i, ino, DT_DIR) < 0)
 				break;
-			filp->f_pos++;
+			(*pos)++;
 			i++;
 			/* fallthrough */
 		default:
 			spin_lock(&dcache_lock);
-			if (filp->f_pos == 2)
+			if ((*pos) == 2)
 				list_move(q, &dentry->d_subdirs);
 
 			for (p=q->next; p != &dentry->d_subdirs; p=p->next) {

@@ -160,21 +161,22 @@ int dcache_readdir(struct file * filp, v
 					continue;
 
 				spin_unlock(&dcache_lock);
-				if (filldir(dirent, next->d_name.name, 
-					    next->d_name.len, filp->f_pos, 
-					    next->d_inode->i_ino, 
+				if (filldir(dirent, next->d_name.name,
+					    next->d_name.len, *pos,
+					    next->d_inode->i_ino,
 					    dt_type(next->d_inode)) < 0)
 					return 0;
 				spin_lock(&dcache_lock);
 				/* next is still alive */
 				list_move(q, p);
 				p = q;
-				filp->f_pos++;
+				(*pos)++;
 			}
 			spin_unlock(&dcache_lock);
 	}
 	return 0;
 }
+EXPORT_SYMBOL(dcache_inode_readdir);
 
 ssize_t generic_read_dir(struct file *filp, char __user *buf, size_t siz, loff_t *ppos)
 {

@@ -186,12 +188,12 @@ const struct file_operations simple_dir_
 	.release	= dcache_dir_close,
 	.llseek		= dcache_dir_lseek,
 	.read		= generic_read_dir,
-	.readdir	= dcache_readdir,
 	.fsync		= simple_sync_file,
 };
 
 const struct inode_operations simple_dir_inode_operations = {
 	.lookup		= simple_lookup,
+	.readdir	= dcache_inode_readdir,
 };
 
 static const struct super_operations simple_super_operations = {

@@ -769,7 +771,6 @@ EXPORT_SYMBOL_GPL(generic_fh_to_parent);
 EXPORT_SYMBOL(dcache_dir_close);
 EXPORT_SYMBOL(dcache_dir_lseek);
 EXPORT_SYMBOL(dcache_dir_open);
-EXPORT_SYMBOL(dcache_readdir);
 EXPORT_SYMBOL(generic_read_dir);
 EXPORT_SYMBOL(get_sb_pseudo);
 EXPORT_SYMBOL(simple_write_begin);
fs/ocfs2/dlm/dlmfs.c +1 -0
Index: b/fs/ocfs2/dlm/dlmfs.c
===================================================================
@@ -553,6 +553,7 @@ static const struct inode_operations dlm
 	.lookup		= simple_lookup,
 	.mkdir		= dlmfs_mkdir,
 	.rmdir		= simple_rmdir,
+	.readdir	= dcache_inode_readdir,
 };
 
 static const struct super_operations dlmfs_ops = {
fs/ramfs/inode.c +1 -0
Index: b/fs/ramfs/inode.c
===================================================================
@@ -151,6 +151,7 @@ static const struct inode_operations ram
 	.symlink	= ramfs_symlink,
 	.mkdir		= ramfs_mkdir,
 	.rmdir		= simple_rmdir,
+	.readdir	= dcache_inode_readdir,
 	.mknod		= ramfs_mknod,
 	.rename		= simple_rename,
 };
include/linux/fs.h +2 -1
Index: b/include/linux/fs.h
===================================================================
@@ -1964,7 +1964,8 @@ extern void drop_super(struct super_bloc
 extern int dcache_dir_open(struct inode *, struct file *);
 extern int dcache_dir_close(struct inode *, struct file *);
 extern loff_t dcache_dir_lseek(struct file *, loff_t, int);
-extern int dcache_readdir(struct file *, void *, filldir_t);
+extern int dcache_inode_readdir(struct dentry *, loff_t *, void *,
+				filldir_t , void *);
 extern int simple_getattr(struct vfsmount *, struct dentry *, struct kstat *);
 extern int simple_statfs(struct dentry *, struct kstatfs *);
 extern int simple_link(struct dentry *, struct inode *, struct dentry *);
mm/shmem.c +1 -0
Index: b/mm/shmem.c
===================================================================
@@ -2389,6 +2389,7 @@ static const struct inode_operations shm
 	.symlink	= shmem_symlink,
 	.mkdir		= shmem_mkdir,
 	.rmdir		= shmem_rmdir,
+	.readdir	= dcache_inode_readdir,
 	.mknod		= shmem_mknod,
 	.rename		= shmem_rename,
 #endif
reviewer
#3 lore raw
In reply to Jan Blunck
Quoted 7 lines

This is a first try to move readdir() to become an inode operation. This is necessary for a VFS implementation of "something like union-mounts" where a readdir() needs to read the directory contents of multiple directories. Besides that the new interface is no longer giving the struct file to the filesystem implementations anymore.

Comments, please?

Hmm, are you sure there are no users which keep some per-struct-file
information for directories? File offset is one such obvious thing which you've handled but actually filesystem with more complicated structure of directory may remember some hints about where we really are, keep some readahead information or so...

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SuSE CR Labs
#4 lore raw
Quoted 6 lines · Jan Kara
Quoted 6 lines

This is a first try to move readdir() to become an inode operation. This is necessary for a VFS implementation of "something like union-mounts" where a readdir() needs to read the directory contents of multiple directories. Besides that the new interface is no longer giving the struct file to the filesystem implementations anymore.

Quoted 6 lines
Quoted 1 line

Comments, please?

Hmm, are you sure there are no users which keep some per-struct-file
information for directories? File offset is one such obvious thing which you've handled but actually filesystem with more complicated structure of directory may remember some hints about where we really are, keep some readahead information or so...

The hfsplus code keeps some extra data in the ->private_data of directories, although I've lost track of the exact benefit from not looking at the code in some years. As I recall it was a performance enhancement due to the fact that the data for all directories is effectively in a single file. I believe it was easier to keep the current position as a structure rather than just an offset and be careful about the tracking. The hfs code is almost identical to the hfsplus code in this area.

Brad Boyer
[email protected]

#5 lore raw
In reply to Jan Kara
Quoted 6 lines · Jan Kara
Quoted 6 lines

This is a first try to move readdir() to become an inode operation. This is necessary for a VFS implementation of "something like union-mounts" where a readdir() needs to read the directory contents of multiple directories. Besides that the new interface is no longer giving the struct file to the filesystem implementations anymore.

Quoted 6 lines
Quoted 1 line

Comments, please?

Hmm, are you sure there are no users which keep some per-struct-file
information for directories? File offset is one such obvious thing which you've handled but actually filesystem with more complicated structure of directory may remember some hints about where we really are, keep some readahead information or so...

For example, the ext3 filesystem, when it is supported hash tree, does exactly this. See ext3_htree_store_dirent() in fs/ext3/dir.c and ext3_htree_fill_tree() in fs/ext3/namei.c.

So your patch would break ext3 htree support.

- Ted