LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode
@ 2007-01-08 20:47 Jeff Layton
  2007-01-10 21:24 ` Christoph Hellwig
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff Layton @ 2007-01-08 20:47 UTC (permalink / raw)
  To: linux-fsdevel, linux-kernel; +Cc: esandeen, aviro

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;
 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode
  2007-01-08 20:47 [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode Jeff Layton
@ 2007-01-10 21:24 ` Christoph Hellwig
  0 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2007-01-10 21:24 UTC (permalink / raw)
  To: Jeff Layton; +Cc: linux-fsdevel, linux-kernel, esandeen, aviro, haveblue

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---

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode
  2007-01-26 15:12     ` Eric Dumazet
@ 2007-01-30 15:04       ` Jeff Layton
  0 siblings, 0 replies; 8+ messages in thread
From: Jeff Layton @ 2007-01-30 15:04 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Kirill Korotaev, linux-fsdevel, linux-kernel

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode
  2007-01-26 14:42   ` Jeff Layton
@ 2007-01-26 15:12     ` Eric Dumazet
  2007-01-30 15:04       ` Jeff Layton
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2007-01-26 15:12 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Kirill Korotaev, linux-fsdevel, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode
  2007-01-26 12:51 ` Kirill Korotaev
@ 2007-01-26 14:42   ` Jeff Layton
  2007-01-26 15:12     ` Eric Dumazet
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff Layton @ 2007-01-26 14:42 UTC (permalink / raw)
  To: Kirill Korotaev; +Cc: linux-fsdevel, linux-kernel

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode
  2006-12-29 19:11 Jeff Layton
@ 2007-01-26 12:51 ` Kirill Korotaev
  2007-01-26 14:42   ` Jeff Layton
  0 siblings, 1 reply; 8+ messages in thread
From: Kirill Korotaev @ 2007-01-26 12:51 UTC (permalink / raw)
  To: Jeff Layton; +Cc: linux-fsdevel, linux-kernel

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/
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode
@ 2007-01-16 18:57 Jeff Layton
  0 siblings, 0 replies; 8+ messages in thread
From: Jeff Layton @ 2007-01-16 18:57 UTC (permalink / raw)
  To: akpm; +Cc: linux-fsdevel, linux-kernel

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;
 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode
@ 2006-12-29 19:11 Jeff Layton
  2007-01-26 12:51 ` Kirill Korotaev
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff Layton @ 2006-12-29 19:11 UTC (permalink / raw)
  To: linux-fsdevel, linux-kernel

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;
 

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2007-01-30 15:05 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-01-08 20:47 [PATCH 3/3] have pipefs ensure i_ino uniqueness by calling iunique and hashing the inode Jeff Layton
2007-01-10 21:24 ` Christoph Hellwig
  -- strict thread matches above, loose matches on Subject: below --
2007-01-16 18:57 Jeff Layton
2006-12-29 19:11 Jeff Layton
2007-01-26 12:51 ` Kirill Korotaev
2007-01-26 14:42   ` Jeff Layton
2007-01-26 15:12     ` Eric Dumazet
2007-01-30 15:04       ` Jeff Layton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).