[PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode

[PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode

[Jeff Layton]

This converts pipefs to use the new scheme. Here we're calling iunique to get
a unique i_ino value for the new inode, and then hashing it afterward. We
call iunique with a max_reserved value of 1 to avoid collision with the root
inode.  Since the inode is now hashed, we need to take care that we end up in
generic_delete_inode rather than generic_forget_inode or we'll create a nasty
leak, so we clear_nlink when we destroy the pipe info.

I'm not certain that this is the right place to add the clear_nlink, though
it does seem to work. I'm open to suggestions on a better place to put
this, or of a better way to make sure that we end up with i_nlink == 0 at
iput time.

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

diff --git a/fs/pipe.c b/fs/pipe.c
index 68090e8..1d44ff0 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -825,6 +825,7 @@ void free_pipe_info(struct inode *inode)
 {
 	__free_pipe_info(inode->i_pipe);
 	inode->i_pipe = NULL;
+	clear_nlink(inode);
 }
 
 static struct vfsmount *pipe_mnt __read_mostly;
@@ -871,6 +872,8 @@ static struct inode * get_pipe_inode(void)
 	inode->i_uid = current->fsuid;
 	inode->i_gid = current->fsgid;
 	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+	inode->i_ino = iunique(pipe_mnt->mnt_sb, 1);
+	insert_inode_hash(inode);
 
 	return inode;
 

Re: [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode

[Kirill Korotaev]

Jeff,

is 100% uniqeness is so much required for pipe inode numbers?
AFAIU, it is not that critical for pipefs (unlike smb, nfs etc.)

Thanks,
Kirill

> This converts pipefs to use the new scheme. Here we're calling iunique to get
> a unique i_ino value for the new inode, and then hashing it afterward. We
> call iunique with a max_reserved value of 1 to avoid collision with the root
> inode.  Since the inode is now hashed, we need to take care that we end up in
> generic_delete_inode rather than generic_forget_inode or we'll create a nasty
> leak, so we clear_nlink when we destroy the pipe info.
> 
> I'm not certain that this is the right place to add the clear_nlink, though
> it does seem to work. I'm open to suggestions on a better place to put
> this, or of a better way to make sure that we end up with i_nlink == 0 at
> iput time.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> 
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 68090e8..1d44ff0 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -825,6 +825,7 @@ void free_pipe_info(struct inode *inode)
>  {
>  	__free_pipe_info(inode->i_pipe);
>  	inode->i_pipe = NULL;
> +	clear_nlink(inode);
>  }
>  
>  static struct vfsmount *pipe_mnt __read_mostly;
> @@ -871,6 +872,8 @@ static struct inode * get_pipe_inode(void)
>  	inode->i_uid = current->fsuid;
>  	inode->i_gid = current->fsgid;
>  	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
> +	inode->i_ino = iunique(pipe_mnt->mnt_sb, 1);
> +	insert_inode_hash(inode);
>  
>  	return inode;
>  
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode

[Jeff Layton]

Kirill Korotaev wrote:
 > Jeff,
 >
 > is 100% uniqeness is so much required for pipe inode numbers?
 > AFAIU, it is not that critical for pipefs (unlike smb, nfs etc.)
 >
 > Thanks,
 > Kirill
 >

There is no in-kernel reason why i_ino uniqueness is important for
pipefs. Where it might matter is userspace. The i_ino value is used for:

1) the st_ino value returned in stat calls

2) the dentry name (generated as "[inode_number]")

So while it's certainly not "correct" to have multiple inodes with the same
number on any filesystem, it is probably more important in some places is
others. For pipefs, maybe it isn't, especially given a potential 6% performance
impact to fix it. Anyone else have thoughts?

-- Jeff

Re: [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode

[Eric Dumazet]

On Friday 26 January 2007 15:42, Jeff Layton wrote:
> Kirill Korotaev wrote:
>  > Jeff,
>  >
>  > is 100% uniqeness is so much required for pipe inode numbers?
>  > AFAIU, it is not that critical for pipefs (unlike smb, nfs etc.)
>  >
>  > Thanks,
>  > Kirill
>
> There is no in-kernel reason why i_ino uniqueness is important for
> pipefs. Where it might matter is userspace. The i_ino value is used for:
>
> 1) the st_ino value returned in stat calls
>
> 2) the dentry name (generated as "[inode_number]")
>
> So while it's certainly not "correct" to have multiple inodes with the same
> number on any filesystem, it is probably more important in some places is
> others. For pipefs, maybe it isn't, especially given a potential 6%
> performance impact to fix it. Anyone else have thoughts?

For dentry name, we certainly could use "[address of inode]" instead 
of "[inode number]" to get unicity, but do we care ?

For st_ino values on pipefs and sockets, I doubt any user application would 
care. I never had to fstat() a socket fd. Of course it's a file descriptor, 
but all we really want to do with this kind of file descriptor is to call 
socket API.

And for some heavy loaded internet servers , the additional cost of 
insert/delete a node in a machine shared tree could be a problem.

Some weeks ago I suggested to not hash pipe/socket dentries in order to save 
some locked ops (see commits 304e61e6fbadec586dfe002b535f169a04248e49 & 
b3423415fbc2e5461605826317da1c8dbbf21f97 ), so obviously I prefer not adding 
another tree ops. However, pipe/sockets creation/destroy are probably not 
that frequent, so the 6% perf impact is not that big :)

Eric

Re: [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode

[Jeff Layton]

Eric Dumazet wrote:
 >
 > For dentry name, we certainly could use "[address of inode]" instead
 > of "[inode number]" to get unicity, but do we care ?
 >
 > For st_ino values on pipefs and sockets, I doubt any user application would
 > care. I never had to fstat() a socket fd. Of course it's a file descriptor,
 > but all we really want to do with this kind of file descriptor is to call
 > socket API.
 >
 > And for some heavy loaded internet servers , the additional cost of
 > insert/delete a node in a machine shared tree could be a problem.
 >

Granted, I've never had to stat a pipe fd either, so maybe in the big scheme
of things, it's not that important. Still, I think we can probably do this
without a great deal of added complexity or performance impact.

If changing the stuff between the brackets in the dentry name to something
besides inode number is OK, then we can defer the assignment of the inode
number until it's actually needed. For pipefs calls, this means we can only
assign an inode number when a stat call is actually done. So anyone who needs
that info can get it, and anyone who doesn't care about it shouldn't be
greatly impacted by it.

I'll be following up this email with a couple of patches for comment...

-- Jeff

[PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode

[Jeff Layton]

This converts pipefs to use the new scheme. Here we're calling iunique to get
a unique i_ino value for the new inode, and then hashing it afterward. We
call iunique with a max_reserved value of 1 to avoid collision with the root
inode.  Since the inode is now hashed, we need to take care that we end up in
generic_delete_inode rather than generic_forget_inode or we'll create a nasty
leak, so we clear_nlink when we destroy the pipe info.

I'm not certain that this is the right place to add the clear_nlink, though
it does seem to work. I'm open to suggestions on a better place to put
this, or of a better way to make sure that we end up with i_nlink == 0 at
iput time.

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

diff --git a/fs/pipe.c b/fs/pipe.c
index 68090e8..1d44ff0 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -825,6 +825,7 @@ void free_pipe_info(struct inode *inode)
 {
 	__free_pipe_info(inode->i_pipe);
 	inode->i_pipe = NULL;
+	clear_nlink(inode);
 }
 
 static struct vfsmount *pipe_mnt __read_mostly;
@@ -871,6 +872,8 @@ static struct inode * get_pipe_inode(void)
 	inode->i_uid = current->fsuid;
 	inode->i_gid = current->fsgid;
 	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+	inode->i_ino = iunique(pipe_mnt->mnt_sb, 1);
+	insert_inode_hash(inode);
 
 	return inode;
 

Re: [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode

[Christoph Hellwig]

On Mon, Jan 08, 2007 at 03:47:17PM -0500, Jeff Layton wrote:
> This converts pipefs to use the new scheme. Here we're calling iunique to get
> a unique i_ino value for the new inode, and then hashing it afterward. We
> call iunique with a max_reserved value of 1 to avoid collision with the root
> inode.  Since the inode is now hashed, we need to take care that we end up in
> generic_delete_inode rather than generic_forget_inode or we'll create a nasty
> leak, so we clear_nlink when we destroy the pipe info.
> 
> I'm not certain that this is the right place to add the clear_nlink, though
> it does seem to work. I'm open to suggestions on a better place to put
> this, or of a better way to make sure that we end up with i_nlink == 0 at
> iput time.

You should Cc Dave Hansen as he's did the nlink helpers and works on
the per-mount readonly code that requires them.

> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> 
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 68090e8..1d44ff0 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -825,6 +825,7 @@ void free_pipe_info(struct inode *inode)
>  {
>  	__free_pipe_info(inode->i_pipe);
>  	inode->i_pipe = NULL;
> +	clear_nlink(inode);
>  }
>  
>  static struct vfsmount *pipe_mnt __read_mostly;
> @@ -871,6 +872,8 @@ static struct inode * get_pipe_inode(void)
>  	inode->i_uid = current->fsuid;
>  	inode->i_gid = current->fsgid;
>  	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
> +	inode->i_ino = iunique(pipe_mnt->mnt_sb, 1);
> +	insert_inode_hash(inode);
>  
>  	return inode;
>  
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---end quoted text---

[PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode

[Jeff Layton]

This converts pipefs to use the new scheme. Here we're calling iunique to get
a unique i_ino value for the new inode, and then hashing it afterward. We
call iunique with a max_reserved value of 1 to avoid collision with the root
inode.  Since the inode is now hashed, we need to take care that we end up in
generic_delete_inode rather than generic_forget_inode or we'll create a nasty
leak, so we clear_nlink when we destroy the pipe info.

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

diff --git a/fs/pipe.c b/fs/pipe.c
index 68090e8..1d44ff0 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -825,6 +825,7 @@ void free_pipe_info(struct inode *inode)
 {
 	__free_pipe_info(inode->i_pipe);
 	inode->i_pipe = NULL;
+	clear_nlink(inode);
 }
 
 static struct vfsmount *pipe_mnt __read_mostly;
@@ -871,6 +872,8 @@ static struct inode * get_pipe_inode(void)
 	inode->i_uid = current->fsuid;
 	inode->i_gid = current->fsgid;
 	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+	inode->i_ino = iunique(pipe_mnt->mnt_sb, 1);
+	insert_inode_hash(inode);
 
 	return inode;