[patch]anon_inodes.c: fix error check in anon_inode_getfd

[patch]anon_inodes.c: fix error check in anon_inode_getfd

[Yan Zheng]

Hello,

igrab return NULL on error.

Signed-off-by: Yan Zheng<yanzheng@21cn.com>
---
diff -ur linux-2.6.23-rc8/fs/anon_inodes.c linux/fs/anon_inodes.c
--- linux-2.6.23-rc8/fs/anon_inodes.c	2007-09-27 10:05:07.000000000 +0800
+++ linux/fs/anon_inodes.c	2007-09-27 10:18:26.000000000 +0800
@@ -87,8 +87,8 @@
 		return -ENFILE;

 	inode = igrab(anon_inode_inode);
-	if (IS_ERR(inode)) {
-		error = PTR_ERR(inode);
+	if (!inode) {
+		error = -ENOENT;
 		goto err_put_filp;
 	}

Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd

[Andrew Morton]

On Thu, 27 Sep 2007 10:30:50 +0800 "Yan Zheng" <yanzheng@21cn.com> wrote:

> Hello,
> 
> igrab return NULL on error.
> 
> Signed-off-by: Yan Zheng<yanzheng@21cn.com>
> ---
> diff -ur linux-2.6.23-rc8/fs/anon_inodes.c linux/fs/anon_inodes.c
> --- linux-2.6.23-rc8/fs/anon_inodes.c	2007-09-27 10:05:07.000000000 +0800
> +++ linux/fs/anon_inodes.c	2007-09-27 10:18:26.000000000 +0800
> @@ -87,8 +87,8 @@
>  		return -ENFILE;
> 
>  	inode = igrab(anon_inode_inode);
> -	if (IS_ERR(inode)) {
> -		error = PTR_ERR(inode);
> +	if (!inode) {
> +		error = -ENOENT;
>  		goto err_put_filp;
>  	}

hm, does that code actually need to exist?  igrab() is pretty expensive and
that fs is permanently mounted anyway...

Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd

[Davide Libenzi]

On Wed, 26 Sep 2007, Andrew Morton wrote:

> On Thu, 27 Sep 2007 10:30:50 +0800 "Yan Zheng" <yanzheng@21cn.com> wrote:
> 
> > Hello,
> > 
> > igrab return NULL on error.
> > 
> > Signed-off-by: Yan Zheng<yanzheng@21cn.com>
> > ---
> > diff -ur linux-2.6.23-rc8/fs/anon_inodes.c linux/fs/anon_inodes.c
> > --- linux-2.6.23-rc8/fs/anon_inodes.c	2007-09-27 10:05:07.000000000 +0800
> > +++ linux/fs/anon_inodes.c	2007-09-27 10:18:26.000000000 +0800
> > @@ -87,8 +87,8 @@
> >  		return -ENFILE;
> > 
> >  	inode = igrab(anon_inode_inode);
> > -	if (IS_ERR(inode)) {
> > -		error = PTR_ERR(inode);
> > +	if (!inode) {
> > +		error = -ENOENT;
> >  		goto err_put_filp;
> >  	}
> 
> hm, does that code actually need to exist?  igrab() is pretty expensive and
> that fs is permanently mounted anyway...

yes. The inode gets attached the the file*, and on its way out an iput() 
is done.
Funny, I noticed the non-ERR_PTR return code of igrab() just yesterday, 
and I was going to port a patch today.


Signed-off-by: Davide Libenzi <davidel@xmailserver.org>



- Davide

Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd

[Davide Libenzi]

On Thu, 27 Sep 2007, Davide Libenzi wrote:

> On Wed, 26 Sep 2007, Andrew Morton wrote:
> 
> > On Thu, 27 Sep 2007 10:30:50 +0800 "Yan Zheng" <yanzheng@21cn.com> wrote:
> > 
> > > Hello,
> > > 
> > > igrab return NULL on error.
> > > 
> > > Signed-off-by: Yan Zheng<yanzheng@21cn.com>
> > > ---
> > > diff -ur linux-2.6.23-rc8/fs/anon_inodes.c linux/fs/anon_inodes.c
> > > --- linux-2.6.23-rc8/fs/anon_inodes.c	2007-09-27 10:05:07.000000000 +0800
> > > +++ linux/fs/anon_inodes.c	2007-09-27 10:18:26.000000000 +0800
> > > @@ -87,8 +87,8 @@
> > >  		return -ENFILE;
> > > 
> > >  	inode = igrab(anon_inode_inode);
> > > -	if (IS_ERR(inode)) {
> > > -		error = PTR_ERR(inode);
> > > +	if (!inode) {
> > > +		error = -ENOENT;
> > >  		goto err_put_filp;
> > >  	}
> > 
> > hm, does that code actually need to exist?  igrab() is pretty expensive and
> > that fs is permanently mounted anyway...
> 
> yes. The inode gets attached the the file*, and on its way out an iput() 
> is done.

Wait. You mean "is it worth checking the igrab() return code at all"?



- Davide

Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd

[Andrew Morton]

On Thu, 27 Sep 2007 09:54:47 -0700 (PDT) Davide Libenzi <davidel@xmailserver.org> wrote:

> On Thu, 27 Sep 2007, Davide Libenzi wrote:
> 
> > On Wed, 26 Sep 2007, Andrew Morton wrote:
> > 
> > > On Thu, 27 Sep 2007 10:30:50 +0800 "Yan Zheng" <yanzheng@21cn.com> wrote:
> > > 
> > > > Hello,
> > > > 
> > > > igrab return NULL on error.
> > > > 
> > > > Signed-off-by: Yan Zheng<yanzheng@21cn.com>
> > > > ---
> > > > diff -ur linux-2.6.23-rc8/fs/anon_inodes.c linux/fs/anon_inodes.c
> > > > --- linux-2.6.23-rc8/fs/anon_inodes.c	2007-09-27 10:05:07.000000000 +0800
> > > > +++ linux/fs/anon_inodes.c	2007-09-27 10:18:26.000000000 +0800
> > > > @@ -87,8 +87,8 @@
> > > >  		return -ENFILE;
> > > > 
> > > >  	inode = igrab(anon_inode_inode);
> > > > -	if (IS_ERR(inode)) {
> > > > -		error = PTR_ERR(inode);
> > > > +	if (!inode) {
> > > > +		error = -ENOENT;
> > > >  		goto err_put_filp;
> > > >  	}
> > > 
> > > hm, does that code actually need to exist?  igrab() is pretty expensive and
> > > that fs is permanently mounted anyway...
> > 
> > yes. The inode gets attached the the file*, and on its way out an iput() 
> > is done.
> 
> Wait. You mean "is it worth checking the igrab() return code at all"?

Well I was more thinking "can we just leave that inode's refcount alone all
the time".  I guess that would be tricky, but I think we can at least use
the much less expensive __iget(), or an open-coded atomic_inc.

inode_lock is a heavily-used singleton.  I remain surprised that it hasn't
bitten us in the ass yet.

Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd

[Davide Libenzi]

On Thu, 27 Sep 2007, Andrew Morton wrote:

> On Thu, 27 Sep 2007 09:54:47 -0700 (PDT) Davide Libenzi <davidel@xmailserver.org> wrote:
> 
> > On Thu, 27 Sep 2007, Davide Libenzi wrote:
> > 
> > > On Wed, 26 Sep 2007, Andrew Morton wrote:
> > > 
> > > > On Thu, 27 Sep 2007 10:30:50 +0800 "Yan Zheng" <yanzheng@21cn.com> wrote:
> > > > 
> > > > > Hello,
> > > > > 
> > > > > igrab return NULL on error.
> > > > > 
> > > > > Signed-off-by: Yan Zheng<yanzheng@21cn.com>
> > > > > ---
> > > > > diff -ur linux-2.6.23-rc8/fs/anon_inodes.c linux/fs/anon_inodes.c
> > > > > --- linux-2.6.23-rc8/fs/anon_inodes.c	2007-09-27 10:05:07.000000000 +0800
> > > > > +++ linux/fs/anon_inodes.c	2007-09-27 10:18:26.000000000 +0800
> > > > > @@ -87,8 +87,8 @@
> > > > >  		return -ENFILE;
> > > > > 
> > > > >  	inode = igrab(anon_inode_inode);
> > > > > -	if (IS_ERR(inode)) {
> > > > > -		error = PTR_ERR(inode);
> > > > > +	if (!inode) {
> > > > > +		error = -ENOENT;
> > > > >  		goto err_put_filp;
> > > > >  	}
> > > > 
> > > > hm, does that code actually need to exist?  igrab() is pretty expensive and
> > > > that fs is permanently mounted anyway...
> > > 
> > > yes. The inode gets attached the the file*, and on its way out an iput() 
> > > is done.
> > 
> > Wait. You mean "is it worth checking the igrab() return code at all"?
> 
> Well I was more thinking "can we just leave that inode's refcount alone all
> the time".  I guess that would be tricky, but I think we can at least use
> the much less expensive __iget(), or an open-coded atomic_inc.
> 
> inode_lock is a heavily-used singleton.  I remain surprised that it hasn't
> bitten us in the ass yet.

Well, __iget() requires the inode lock to be held. Wrapping the anon 
inodes code with a lock would be even worse.
Relying on the fact that the count will never go to zero and call __iget() 
w/out held lock (to expoit the __iget fast path), is kinda nasty IMO.
Open coded atomic_inc()? Hmm, dunno...



- Davide

Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd

[Andrew Morton]

On Thu, 27 Sep 2007 10:56:09 -0700 (PDT) Davide Libenzi <davidel@xmailserver.org> wrote:

> Open coded atomic_inc()? Hmm, dunno...

box:/usr/src/25> grep 'atomic_inc.*->i_count' */*.c
fs/block_dev.c:         atomic_inc(&bdev->bd_inode->i_count);
fs/block_dev.c:                 atomic_inc(&bdev->bd_inode->i_count);
fs/inode.c:             atomic_inc(&inode->i_count);
fs/inode.c:     atomic_inc(&inode->i_count);
fs/libfs.c:     atomic_inc(&inode->i_count);
fs/namei.c:                     atomic_inc(&inode->i_count);
ipc/mqueue.c:           atomic_inc(&inode->i_count);
kernel/futex.c:                 atomic_inc(&key->shared.inode->i_count);
mm/shmem.c:     atomic_inc(&inode->i_count);    /* New dentry reference */

The refcount on that inode is never zero *by design*.  So all we'd be doing
here is relying upon our existing design, so I think it'd be an OK thing to do. 
With appropriate code comments, of course.

Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd

[Davide Libenzi]

On Thu, 27 Sep 2007, Andrew Morton wrote:

> On Thu, 27 Sep 2007 10:56:09 -0700 (PDT) Davide Libenzi <davidel@xmailserver.org> wrote:
> 
> > Open coded atomic_inc()? Hmm, dunno...
> 
> box:/usr/src/25> grep 'atomic_inc.*->i_count' */*.c
> fs/block_dev.c:         atomic_inc(&bdev->bd_inode->i_count);
> fs/block_dev.c:                 atomic_inc(&bdev->bd_inode->i_count);
> fs/inode.c:             atomic_inc(&inode->i_count);
> fs/inode.c:     atomic_inc(&inode->i_count);
> fs/libfs.c:     atomic_inc(&inode->i_count);
> fs/namei.c:                     atomic_inc(&inode->i_count);
> ipc/mqueue.c:           atomic_inc(&inode->i_count);
> kernel/futex.c:                 atomic_inc(&key->shared.inode->i_count);
> mm/shmem.c:     atomic_inc(&inode->i_count);    /* New dentry reference */
> 
> The refcount on that inode is never zero *by design*.  So all we'd be doing
> here is relying upon our existing design, so I think it'd be an OK thing to do. 
> With appropriate code comments, of course.

Ok then. Drop that patch and I'll post another with the open coded 
atomic_inc() ...



- Davide