From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Chinner Subject: Re: [rfc][patch] fs: introduce iget, mark igrab as must_check Date: Thu, 25 Mar 2010 09:01:57 +1100 Message-ID: <20100324220157.GD7671@dastard> References: <20100317131111.GG2869@laptop> <20100320201338.GA27478@cynthia.pants.nu> <20100322015755.GD9733@laptop> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Brad Boyer , linux-fsdevel@vger.kernel.org To: Nick Piggin Return-path: Received: from bld-mail14.adl6.internode.on.net ([150.101.137.99]:40496 "EHLO mail.internode.on.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751105Ab0CXWCA (ORCPT ); Wed, 24 Mar 2010 18:02:00 -0400 Content-Disposition: inline In-Reply-To: <20100322015755.GD9733@laptop> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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