[rfc][patch] fs: introduce iget, mark igrab as must_check

[rfc][patch] fs: introduce iget, mark igrab as must_check

[Nick Piggin]

I thought I would ask for opinions on this because it can come in handy
to avoid locking to use an iget() rather than igrab() if we do lazy LRU
(like the dcache), then iget() just increments the refcount and doesn't
require any more locking.

I thought it might be useful anyway to avoid igrab misuse by
filesystems? So after this patch we should use igrab for when we don't
already have the inode pinned. Introduce iget for when we do.
The interfaces don't change, but __must_check will prompt filesystems to
use the correct API.

Index: linux-2.6/fs/inode.c
===================================================================
--- linux-2.6.orig/fs/inode.c
+++ linux-2.6/fs/inode.c
@@ -296,6 +296,15 @@ void __iget(struct inode *inode)
 	inodes_stat.nr_unused--;
 }
 
+void iget(struct inode *inode)
+{
+	spin_lock(&inode_lock);
+	__iget(inode);
+	spin_unlock(&inode_lock);
+	BUG_ON(inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE));
+}
+EXPORT_SYMBOL(iget);
+
 /**
  * clear_inode - clear an inode
  * @inode: inode to clear
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h
+++ linux-2.6/include/linux/fs.h
@@ -2146,7 +2146,7 @@ extern int inode_init_always(struct supe
 extern void inode_init_once(struct inode *);
 extern void inode_add_to_lists(struct super_block *, struct inode *);
 extern void iput(struct inode *);
-extern struct inode * igrab(struct inode *);
+extern struct inode * __must_check igrab(struct inode *);
 extern ino_t iunique(struct super_block *, ino_t);
 extern int inode_needs_sync(struct inode *inode);
 extern void generic_delete_inode(struct inode *inode);
@@ -2166,6 +2166,7 @@ extern int insert_inode_locked4(struct i
 extern int insert_inode_locked(struct inode *);
 extern void unlock_new_inode(struct inode *);
 
+extern void iget(struct inode * inode);
 extern void __iget(struct inode * inode);
 extern void iget_failed(struct inode *);
 extern void clear_inode(struct inode *);

Re: [rfc][patch] fs: introduce iget, mark igrab as must_check

[Brad Boyer]

On Thu, Mar 18, 2010 at 12:11:12AM +1100, Nick Piggin wrote:
> I thought I would ask for opinions on this because it can come in handy
> to avoid locking to use an iget() rather than igrab() if we do lazy LRU
> (like the dcache), then iget() just increments the refcount and doesn't
> require any more locking.
> 
> I thought it might be useful anyway to avoid igrab misuse by
> filesystems? So after this patch we should use igrab for when we don't
> already have the inode pinned. Introduce iget for when we do.
> The interfaces don't change, but __must_check will prompt filesystems to
> use the correct API.

I'm not sure if this matters, but I have to say I don't like the name. In
particular, iget used to exist and be iget(sb, ino). A lot of the code
still has leftovers from this including iget_locked. I'm not sure if
there is a more obvious name, especially since __iget is effectively the
unlocked version of what you added. It's something to consider if you
haven't already.

Also, are you really avoiding any locking? It looks the same to me.

	Brad Boyer
	flar@allandria.com

Re: [rfc][patch] fs: introduce iget, mark igrab as must_check

[Nick Piggin]

On Sat, Mar 20, 2010 at 01:13:38PM -0700, Brad Boyer wrote:
> On Thu, Mar 18, 2010 at 12:11:12AM +1100, Nick Piggin wrote:
> > I thought I would ask for opinions on this because it can come in handy
> > to avoid locking to use an iget() rather than igrab() if we do lazy LRU
> > (like the dcache), then iget() just increments the refcount and doesn't
> > require any more locking.
> > 
> > I thought it might be useful anyway to avoid igrab misuse by
> > filesystems? So after this patch we should use igrab for when we don't
> > already have the inode pinned. Introduce iget for when we do.
> > The interfaces don't change, but __must_check will prompt filesystems to
> > use the correct API.
> 
> I'm not sure if this matters, but I have to say I don't like the name. In
> particular, iget used to exist and be iget(sb, ino). A lot of the code
> still has leftovers from this including iget_locked. I'm not sure if
> there is a more obvious name, especially since __iget is effectively the
> unlocked version of what you added. It's something to consider if you
> haven't already.

Yeah, I can't think of a better name. It would be annoying to introduce
a worse name because of legacy reasons.

> 
> Also, are you really avoiding any locking? It looks the same to me.

Not in this patch, but I have a patch in my icache scalability series
(which can be broken out of course) that basically does a "lazy lru"
for inodes, like we do for dentries now.

So iget will become basically what dget is now. It would not have to
check inode state or move it between lists.

Whether or not we actually do that is another question, but either way
I think this patch is worthwhile.

Thanks,
Nick

Re: [rfc][patch] fs: introduce iget, mark igrab as must_check

[Brad Boyer]

On Mon, Mar 22, 2010 at 12:57:56PM +1100, Nick Piggin wrote:
> On Sat, Mar 20, 2010 at 01:13:38PM -0700, Brad Boyer wrote:
> > I'm not sure if this matters, but I have to say I don't like the name. In
> > particular, iget used to exist and be iget(sb, ino). A lot of the code
> > still has leftovers from this including iget_locked. I'm not sure if
> > there is a more obvious name, especially since __iget is effectively the
> > unlocked version of what you added. It's something to consider if you
> > haven't already.
> 
> Yeah, I can't think of a better name. It would be annoying to introduce
> a worse name because of legacy reasons.

That is true. I suppose it's been gone long enough that it doesn't
really matter. I hope nobody is still trying to port code that old.

> > Also, are you really avoiding any locking? It looks the same to me.
> 
> Not in this patch, but I have a patch in my icache scalability series
> (which can be broken out of course) that basically does a "lazy lru"
> for inodes, like we do for dentries now.
> 
> So iget will become basically what dget is now. It would not have to
> check inode state or move it between lists.
> 
> Whether or not we actually do that is another question, but either way
> I think this patch is worthwhile.

That makes sense. This wasn't clear to me from the original message.

	Brad Boyer
	flar@allandria.com

Re: [rfc][patch] fs: introduce iget, mark igrab as must_check

[Dave Chinner]

On Mon, Mar 22, 2010 at 12:57:56PM +1100, Nick Piggin wrote:
> On Sat, Mar 20, 2010 at 01:13:38PM -0700, Brad Boyer wrote:
> > On Thu, Mar 18, 2010 at 12:11:12AM +1100, Nick Piggin wrote:
> > > I thought I would ask for opinions on this because it can come in handy
> > > to avoid locking to use an iget() rather than igrab() if we do lazy LRU
> > > (like the dcache), then iget() just increments the refcount and doesn't
> > > require any more locking.
> > > 
> > > I thought it might be useful anyway to avoid igrab misuse by
> > > filesystems? So after this patch we should use igrab for when we don't
> > > already have the inode pinned. Introduce iget for when we do.
> > > The interfaces don't change, but __must_check will prompt filesystems to
> > > use the correct API.
> > 
> > I'm not sure if this matters, but I have to say I don't like the name. In
> > particular, iget used to exist and be iget(sb, ino). A lot of the code
> > still has leftovers from this including iget_locked. I'm not sure if
> > there is a more obvious name, especially since __iget is effectively the
> > unlocked version of what you added. It's something to consider if you
> > haven't already.
> 
> Yeah, I can't think of a better name. It would be annoying to introduce
> a worse name because of legacy reasons.
> 
> > 
> > Also, are you really avoiding any locking? It looks the same to me.
> 
> Not in this patch, but I have a patch in my icache scalability series
> (which can be broken out of course) that basically does a "lazy lru"
> for inodes, like we do for dentries now.
> 
> So iget will become basically what dget is now. It would not have to
> check inode state or move it between lists.
> 
> Whether or not we actually do that is another question, but either way
> I think this patch is worthwhile.

I've had a patch hanging around that avoids the inode_lock for igrab
in the case of already pinned inodes. It doesn't address the
__must_check you want to add, though...

Basically, it added an unlocked atomic_inc_not_zero() to igrab()
like so::

 struct inode *igrab(struct inode *inode)
 {
+	if (atomic_inc_not_zero(&inode->i_count))
+		return inode;
+
 	spin_lock(&inode_lock);
 	if (!(inode->i_state & (I_FREEING|I_CLEAR|I_WILL_FREE)))
 		__iget(inode);

The patch also changed __iget() to use atomic_inc_not_zero()
rather than atomic_read/atomic_inc as well...

Cheers,

Dave,
-- 
Dave Chinner
david@fromorbit.com