This patch adds 2 new libfs functions that allow for us to defer assignment of
an i_ino value until such time that it's actually used. This allows us to
ensure uniqueness without actually impacting the cases that don't really care
about it. With this i_ino == 0 has a special meaning of "unassigned".
I have 2 things I'm not sure of with this patch. The first is the #defined
value for MAX_RESERVED_INODE. I'm not thrilled with that, but didn't see a way
to pass in a max_reserved value for iunique through the standard filesystem
call (maybe we could hang the value off of the superblock, but I kind of
don't like doing that either).
The other thing I'm not sure of is whether the locking I'm doing in case 1 of
__dcache_readdir (where I replace the parent_ino call) is correct.
Signed-off-by: Jeff Layton <[email protected]>
diff --git a/fs/libfs.c b/fs/libfs.c
index 9b77f89..f6cd382 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -11,6 +11,17 @@
#include <asm/uaccess.h>
+/* highest inode number that should be assigned by simple_fill_super */
+#define MAX_RESERVED_INODE 100
+
+static inline void lazy_hash_inode(struct inode *inode)
+{
+ if (unlikely(inode->i_ino == 0)) {
+ inode->i_ino = iunique(inode->i_sb, MAX_RESERVED_INODE);
+ insert_inode_hash(inode);
+ }
+}
+
int simple_getattr(struct vfsmount *mnt, struct dentry *dentry,
struct kstat *stat)
{
@@ -20,6 +31,13 @@ int simple_getattr(struct vfsmount *mnt, struct dentry *dentry,
return 0;
}
+int lazy_getattr(struct vfsmount *mnt, struct dentry *dentry,
+ struct kstat *stat)
+{
+ lazy_hash_inode(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;
@@ -124,24 +142,34 @@ static inline unsigned char dt_type(struct inode *inode)
* both impossible due to the lock on directory.
*/
-int dcache_readdir(struct file * filp, void * dirent, filldir_t filldir)
+static int __dcache_readdir(struct file * filp, void * dirent,
+ filldir_t filldir, bool lazy)
{
struct dentry *dentry = filp->f_path.dentry;
struct dentry *cursor = filp->private_data;
+ struct inode *inode;
struct list_head *p, *q = &cursor->d_u.d_child;
ino_t ino;
int i = filp->f_pos;
switch (i) {
case 0:
- ino = dentry->d_inode->i_ino;
+ inode = dentry->d_inode;
+ if (lazy)
+ lazy_hash_inode(inode);
+ ino = inode->i_ino;
if (filldir(dirent, ".", 1, i, ino, DT_DIR) < 0)
break;
filp->f_pos++;
i++;
/* fallthrough */
case 1:
- ino = parent_ino(dentry);
+ spin_lock(&dentry->d_lock);
+ inode = dentry->d_parent->d_inode;
+ spin_unlock(&dentry->d_lock);
+ if (lazy)
+ lazy_hash_inode(inode);
+ ino = inode->i_ino;
if (filldir(dirent, "..", 2, i, ino, DT_DIR) < 0)
break;
filp->f_pos++;
@@ -155,11 +183,17 @@ int dcache_readdir(struct file * filp, void * dirent, filldir_t filldir)
for (p=q->next; p != &dentry->d_subdirs; p=p->next) {
struct dentry *next;
next = list_entry(p, struct dentry, d_u.d_child);
- if (d_unhashed(next) || !next->d_inode)
+ inode = next->d_inode;
+ if (d_unhashed(next) || !inode)
continue;
spin_unlock(&dcache_lock);
- if (filldir(dirent, next->d_name.name, next->d_name.len, filp->f_pos, next->d_inode->i_ino, dt_type(next->d_inode)) < 0)
+
+ if (lazy)
+ lazy_hash_inode(inode);
+ if (filldir(dirent, next->d_name.name,
+ next->d_name.len, filp->f_pos,
+ inode->i_ino, dt_type(inode)) < 0)
return 0;
spin_lock(&dcache_lock);
/* next is still alive */
@@ -172,6 +206,16 @@ int dcache_readdir(struct file * filp, void * dirent, filldir_t filldir)
return 0;
}
+int dcache_readdir(struct file * filp, void * dirent, filldir_t filldir)
+{
+ return __dcache_readdir(filp, dirent, filldir, false);
+}
+
+int lazy_readdir(struct file * filp, void * dirent, filldir_t filldir)
+{
+ return __dcache_readdir(filp, dirent, filldir, true);
+}
+
ssize_t generic_read_dir(struct file *filp, char __user *buf, size_t siz, loff_t *ppos)
{
return -EISDIR;
@@ -634,6 +678,7 @@ EXPORT_SYMBOL(dcache_dir_close);
EXPORT_SYMBOL(dcache_dir_lseek);
EXPORT_SYMBOL(dcache_dir_open);
EXPORT_SYMBOL(dcache_readdir);
+EXPORT_SYMBOL(lazy_readdir);
EXPORT_SYMBOL(generic_read_dir);
EXPORT_SYMBOL(get_sb_pseudo);
EXPORT_SYMBOL(simple_commit_write);
@@ -643,6 +688,7 @@ EXPORT_SYMBOL(simple_empty);
EXPORT_SYMBOL(d_alloc_name);
EXPORT_SYMBOL(simple_fill_super);
EXPORT_SYMBOL(simple_getattr);
+EXPORT_SYMBOL(lazy_getattr);
EXPORT_SYMBOL(simple_link);
EXPORT_SYMBOL(simple_lookup);
EXPORT_SYMBOL(simple_pin_fs);
Jeff,
taking into account the discussion about unawarness/uncertainty
of whether *unique* inode number is needed at all on pipe fds and such
do we need this at all?
Thanks,
Kirill
> This patch adds 2 new libfs functions that allow for us to defer assignment of
> an i_ino value until such time that it's actually used. This allows us to
> ensure uniqueness without actually impacting the cases that don't really care
> about it. With this i_ino == 0 has a special meaning of "unassigned".
>
> I have 2 things I'm not sure of with this patch. The first is the #defined
> value for MAX_RESERVED_INODE. I'm not thrilled with that, but didn't see a way
> to pass in a max_reserved value for iunique through the standard filesystem
> call (maybe we could hang the value off of the superblock, but I kind of
> don't like doing that either).
>
> The other thing I'm not sure of is whether the locking I'm doing in case 1 of
> __dcache_readdir (where I replace the parent_ino call) is correct.
>
> Signed-off-by: Jeff Layton <[email protected]>
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 9b77f89..f6cd382 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -11,6 +11,17 @@
>
> #include <asm/uaccess.h>
>
> +/* highest inode number that should be assigned by simple_fill_super */
> +#define MAX_RESERVED_INODE 100
> +
> +static inline void lazy_hash_inode(struct inode *inode)
> +{
> + if (unlikely(inode->i_ino == 0)) {
> + inode->i_ino = iunique(inode->i_sb, MAX_RESERVED_INODE);
> + insert_inode_hash(inode);
> + }
> +}
> +
> int simple_getattr(struct vfsmount *mnt, struct dentry *dentry,
> struct kstat *stat)
> {
> @@ -20,6 +31,13 @@ int simple_getattr(struct vfsmount *mnt, struct dentry *dentry,
> return 0;
> }
>
> +int lazy_getattr(struct vfsmount *mnt, struct dentry *dentry,
> + struct kstat *stat)
> +{
> + lazy_hash_inode(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;
> @@ -124,24 +142,34 @@ static inline unsigned char dt_type(struct inode *inode)
> * both impossible due to the lock on directory.
> */
>
> -int dcache_readdir(struct file * filp, void * dirent, filldir_t filldir)
> +static int __dcache_readdir(struct file * filp, void * dirent,
> + filldir_t filldir, bool lazy)
> {
> struct dentry *dentry = filp->f_path.dentry;
> struct dentry *cursor = filp->private_data;
> + struct inode *inode;
> struct list_head *p, *q = &cursor->d_u.d_child;
> ino_t ino;
> int i = filp->f_pos;
>
> switch (i) {
> case 0:
> - ino = dentry->d_inode->i_ino;
> + inode = dentry->d_inode;
> + if (lazy)
> + lazy_hash_inode(inode);
> + ino = inode->i_ino;
> if (filldir(dirent, ".", 1, i, ino, DT_DIR) < 0)
> break;
> filp->f_pos++;
> i++;
> /* fallthrough */
> case 1:
> - ino = parent_ino(dentry);
> + spin_lock(&dentry->d_lock);
> + inode = dentry->d_parent->d_inode;
> + spin_unlock(&dentry->d_lock);
> + if (lazy)
> + lazy_hash_inode(inode);
> + ino = inode->i_ino;
> if (filldir(dirent, "..", 2, i, ino, DT_DIR) < 0)
> break;
> filp->f_pos++;
> @@ -155,11 +183,17 @@ int dcache_readdir(struct file * filp, void * dirent, filldir_t filldir)
> for (p=q->next; p != &dentry->d_subdirs; p=p->next) {
> struct dentry *next;
> next = list_entry(p, struct dentry, d_u.d_child);
> - if (d_unhashed(next) || !next->d_inode)
> + inode = next->d_inode;
> + if (d_unhashed(next) || !inode)
> continue;
>
> spin_unlock(&dcache_lock);
> - if (filldir(dirent, next->d_name.name, next->d_name.len, filp->f_pos, next->d_inode->i_ino, dt_type(next->d_inode)) < 0)
> +
> + if (lazy)
> + lazy_hash_inode(inode);
> + if (filldir(dirent, next->d_name.name,
> + next->d_name.len, filp->f_pos,
> + inode->i_ino, dt_type(inode)) < 0)
> return 0;
> spin_lock(&dcache_lock);
> /* next is still alive */
> @@ -172,6 +206,16 @@ int dcache_readdir(struct file * filp, void * dirent, filldir_t filldir)
> return 0;
> }
>
> +int dcache_readdir(struct file * filp, void * dirent, filldir_t filldir)
> +{
> + return __dcache_readdir(filp, dirent, filldir, false);
> +}
> +
> +int lazy_readdir(struct file * filp, void * dirent, filldir_t filldir)
> +{
> + return __dcache_readdir(filp, dirent, filldir, true);
> +}
> +
> ssize_t generic_read_dir(struct file *filp, char __user *buf, size_t siz, loff_t *ppos)
> {
> return -EISDIR;
> @@ -634,6 +678,7 @@ EXPORT_SYMBOL(dcache_dir_close);
> EXPORT_SYMBOL(dcache_dir_lseek);
> EXPORT_SYMBOL(dcache_dir_open);
> EXPORT_SYMBOL(dcache_readdir);
> +EXPORT_SYMBOL(lazy_readdir);
> EXPORT_SYMBOL(generic_read_dir);
> EXPORT_SYMBOL(get_sb_pseudo);
> EXPORT_SYMBOL(simple_commit_write);
> @@ -643,6 +688,7 @@ EXPORT_SYMBOL(simple_empty);
> EXPORT_SYMBOL(d_alloc_name);
> EXPORT_SYMBOL(simple_fill_super);
> EXPORT_SYMBOL(simple_getattr);
> +EXPORT_SYMBOL(lazy_getattr);
> EXPORT_SYMBOL(simple_link);
> EXPORT_SYMBOL(simple_lookup);
> EXPORT_SYMBOL(simple_pin_fs);
>
Kirill Korotaev wrote:
> Jeff,
>
> taking into account the discussion about unawarness/uncertainty
> of whether *unique* inode number is needed at all on pipe fds and such
> do we need this at all?
>
> Thanks,
> Kirill
>
Fair enough, perhaps we should just not worry about it, and assume that there
might be collisions.
If so, I should probably just have Andrew withdraw the patch I submitted earlier
to hash the inodes for pipefs. I'll look at other callers of new_inode and fix
up any of the ones that need fixing.
Does that seem like the most reasonable approach?
-- Jeff
>>Jeff,
>>
>>taking into account the discussion about unawarness/uncertainty
>>of whether *unique* inode number is needed at all on pipe fds and such
>>do we need this at all?
>>
>>Thanks,
>>Kirill
>>
>
>
> Fair enough, perhaps we should just not worry about it, and assume that there
> might be collisions.
>
> If so, I should probably just have Andrew withdraw the patch I submitted earlier
> to hash the inodes for pipefs. I'll look at other callers of new_inode and fix
> up any of the ones that need fixing.
>
> Does that seem like the most reasonable approach?
yep!
Thanks,
Kirill