* [patch]anon_inodes.c: fix error check in anon_inode_getfd
@ 2007-09-27 2:30 Yan Zheng
2007-09-27 3:59 ` Andrew Morton
0 siblings, 1 reply; 8+ messages in thread
From: Yan Zheng @ 2007-09-27 2:30 UTC (permalink / raw)
To: linux-fsdevel, akpm
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;
}
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd 2007-09-27 2:30 [patch]anon_inodes.c: fix error check in anon_inode_getfd Yan Zheng @ 2007-09-27 3:59 ` Andrew Morton 2007-09-27 16:41 ` Davide Libenzi 0 siblings, 1 reply; 8+ messages in thread From: Andrew Morton @ 2007-09-27 3:59 UTC (permalink / raw) To: Yan Zheng; +Cc: linux-fsdevel, Davide Libenzi 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... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd 2007-09-27 3:59 ` Andrew Morton @ 2007-09-27 16:41 ` Davide Libenzi 2007-09-27 16:54 ` Davide Libenzi 0 siblings, 1 reply; 8+ messages in thread From: Davide Libenzi @ 2007-09-27 16:41 UTC (permalink / raw) To: Andrew Morton; +Cc: Yan Zheng, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd 2007-09-27 16:41 ` Davide Libenzi @ 2007-09-27 16:54 ` Davide Libenzi 2007-09-27 17:40 ` Andrew Morton 0 siblings, 1 reply; 8+ messages in thread From: Davide Libenzi @ 2007-09-27 16:54 UTC (permalink / raw) To: Andrew Morton; +Cc: Yan Zheng, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd 2007-09-27 16:54 ` Davide Libenzi @ 2007-09-27 17:40 ` Andrew Morton 2007-09-27 17:56 ` Davide Libenzi 0 siblings, 1 reply; 8+ messages in thread From: Andrew Morton @ 2007-09-27 17:40 UTC (permalink / raw) To: Davide Libenzi; +Cc: Yan Zheng, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd 2007-09-27 17:40 ` Andrew Morton @ 2007-09-27 17:56 ` Davide Libenzi 2007-09-27 18:20 ` Andrew Morton 0 siblings, 1 reply; 8+ messages in thread From: Davide Libenzi @ 2007-09-27 17:56 UTC (permalink / raw) To: Andrew Morton; +Cc: Yan Zheng, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd 2007-09-27 17:56 ` Davide Libenzi @ 2007-09-27 18:20 ` Andrew Morton 2007-09-27 18:25 ` Davide Libenzi 0 siblings, 1 reply; 8+ messages in thread From: Andrew Morton @ 2007-09-27 18:20 UTC (permalink / raw) To: Davide Libenzi; +Cc: Yan Zheng, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch]anon_inodes.c: fix error check in anon_inode_getfd 2007-09-27 18:20 ` Andrew Morton @ 2007-09-27 18:25 ` Davide Libenzi 0 siblings, 0 replies; 8+ messages in thread From: Davide Libenzi @ 2007-09-27 18:25 UTC (permalink / raw) To: Andrew Morton; +Cc: Yan Zheng, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2007-09-27 18:25 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-09-27 2:30 [patch]anon_inodes.c: fix error check in anon_inode_getfd Yan Zheng 2007-09-27 3:59 ` Andrew Morton 2007-09-27 16:41 ` Davide Libenzi 2007-09-27 16:54 ` Davide Libenzi 2007-09-27 17:40 ` Andrew Morton 2007-09-27 17:56 ` Davide Libenzi 2007-09-27 18:20 ` Andrew Morton 2007-09-27 18:25 ` Davide Libenzi
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).