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

[Dave Chinner <david@fromorbit.com>]

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