[RFC][PATCH] ensure i_ino uniqueness in filesystems without permanent inode numbers (via lazy hashing)

[RFC][PATCH] ensure i_ino uniqueness in filesystems without permanent inode numbers (via lazy hashing)

[Jeff Layton]

[-- Attachment #1: Type: text/plain, Size: 1409 bytes --]

Here is a different approach to the problem of i_ino uniqueness. Again,
I'll refer to my original email and patch for a description of the 
problem...

With this patch, I'm taking the approach of only assigning out an i_ino
value when it's actually needed. This adds a lazy_getattr function. If
i_ino is 0, then this does an iunique and hashes the inode before doing
the actual getattr.

This patch is still a proof-of-concept. For a "real" patch, we'd need to:

1) add lazy functions for readdir and encode_fh (maybe also others)
2) clean up how root inode i_ino values are handled in superblocks
3) probably just have new_inode not have a counter, so that new inodes
created by it always get i_ino=0
4) convert all callers of new_inode that don't have a scheme to ensure
uniqueness

This patch does hash inodes in the standard way, but the lazy
approach could also be combined with the earlier patch I posted that
uses IDR, or a different hashing scheme that keeps these out of the
normal inode_hashtable. I figured I'd go with the approach of not adding
more infrastructure than was necessary as a first stab.

If this general approach seems alright, then could someone confirm 
whether using generic_delete_inode as the drop_inode function is OK, or 
whether I need to have pipefs actually handle the nlink counter 
correctly and use generic_drop_inode?

Signed-off-by: Jeff Layton <jlayton@redhat.com>



[-- Attachment #2: lazy_ino_hash.patch --]
[-- Type: text/x-patch, Size: 3516 bytes --]

diff --git a/fs/inode.c b/fs/inode.c
index 26cdb11..9d7da59 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -524,7 +524,7 @@ repeat:
  */
 struct inode *new_inode(struct super_block *sb)
 {
-	static unsigned long last_ino;
+	static unsigned int last_ino;
 	struct inode * inode;
 
 	spin_lock_prefetch(&inode_lock);
@@ -683,7 +683,7 @@ static unsigned long hash(struct super_b
  */
 ino_t iunique(struct super_block *sb, ino_t max_reserved)
 {
-	static ino_t counter;
+	static unsigned int counter;
 	struct inode *inode;
 	struct hlist_head * head;
 	ino_t res;
diff --git a/fs/libfs.c b/fs/libfs.c
index bd08e0e..75f8371 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -20,6 +20,18 @@ int simple_getattr(struct vfsmount *mnt,
 	return 0;
 }
 
+/* same as simple_getattr, but get a unique inode number and hash the inode
+ * prior to doing the getattr */
+int lazy_getattr(struct vfsmount *mnt, struct dentry *dentry,
+		   struct kstat *stat)
+{
+	if (dentry->d_inode->i_ino == 0) {
+		dentry->d_inode->i_ino = iunique(dentry->d_sb, 0);
+		insert_inode_hash(dentry->d_inode);
+	}
+	return simple_getattr(mnt, dentry, stat);
+}
+
 int simple_statfs(struct dentry *dentry, struct kstatfs *buf)
 {
 	buf->f_type = dentry->d_sb->s_magic;
@@ -639,3 +651,4 @@ EXPORT_SYMBOL_GPL(simple_attr_open);
 EXPORT_SYMBOL_GPL(simple_attr_close);
 EXPORT_SYMBOL_GPL(simple_attr_read);
 EXPORT_SYMBOL_GPL(simple_attr_write);
+EXPORT_SYMBOL_GPL(lazy_getattr);
diff --git a/fs/pipe.c b/fs/pipe.c
index b1626f2..6910081 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -837,6 +837,10 @@ static struct dentry_operations pipefs_d
 	.d_delete	= pipefs_delete_dentry,
 };
 
+static struct inode_operations pipefs_inode_operations = {
+	.getattr	= lazy_getattr,
+};
+
 static struct inode * get_pipe_inode(void)
 {
 	struct inode *inode = new_inode(pipe_mnt->mnt_sb);
@@ -845,6 +849,10 @@ static struct inode * get_pipe_inode(voi
 	if (!inode)
 		goto fail_inode;
 
+	/* reset this to 0 to cue lazy_getattr to give us a unique i_ino value
+	 * if this ever has a getattr run against it */
+	inode->i_ino = 0;
+
 	pipe = alloc_pipe_info(inode);
 	if (!pipe)
 		goto fail_iput;
@@ -852,6 +860,7 @@ static struct inode * get_pipe_inode(voi
 
 	pipe->readers = pipe->writers = 1;
 	inode->i_fop = &rdwr_pipe_fops;
+	inode->i_op = &pipefs_inode_operations;
 
 	/*
 	 * Mark the inode dirty from the very beginning,
@@ -998,7 +1007,12 @@ static int pipefs_get_sb(struct file_sys
 			 int flags, const char *dev_name, void *data,
 			 struct vfsmount *mnt)
 {
-	return get_sb_pseudo(fs_type, "pipe:", NULL, PIPEFS_MAGIC, mnt);
+        static struct super_operations default_ops = {
+		.statfs	=	simple_statfs,
+		.drop_inode =	generic_delete_inode,
+	};
+	
+	return get_sb_pseudo(fs_type, "pipe:", &default_ops, PIPEFS_MAGIC, mnt);
 }
 
 static struct file_system_type pipe_fs_type = {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2fe6e3f..41571c1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1885,6 +1885,7 @@ extern int dcache_dir_close(struct inode
 extern loff_t dcache_dir_lseek(struct file *, loff_t, int);
 extern int dcache_readdir(struct file *, void *, filldir_t);
 extern int simple_getattr(struct vfsmount *, struct dentry *, struct kstat *);
+extern int lazy_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 *);
 extern int simple_unlink(struct inode *, struct dentry *);

Re: [RFC][PATCH] ensure i_ino uniqueness in filesystems without permanent inode numbers (via lazy hashing)

[Jeff Layton]

Jeff Layton wrote:
> Here is a different approach to the problem of i_ino uniqueness. Again,
> I'll refer to my original email and patch for a description of the 
> problem...
> 
> With this patch, I'm taking the approach of only assigning out an i_ino
> value when it's actually needed. This adds a lazy_getattr function. If
> i_ino is 0, then this does an iunique and hashes the inode before doing
> the actual getattr.
> 

Actually, after having a closer look at this, I don't think this turns 
out to be feasible either. Some filesystems (including pipefs) want an 
i_ino value early on for generating the qstr passed to d_alloc.

Since they want the value so early, there would be little benefit in 
attempting to delay assigning an i_ino value. It might be a win for some 
filesystems, but ones like pipefs and sockfs wouldn't be able to use 
this scheme, and those are the ones we're primarily concerned with 
performance-wise.

I'm going to plan to clean up my IDR patch and resubmit it, as I think 
it's probably the best scheme we have that seems to actually fix the 
problem.

Unless someone else has a better idea... :-)

-- Jeff