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@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
