Re: Status of inode scaling work

[Dave Chinner <david@fromorbit.com>]

On Thu, Jan 27, 2011 at 04:11:24PM +1100, Nick Piggin wrote:
> On Thu, Jan 27, 2011 at 3:57 PM, Dave Chinner <david@fromorbit.com> wrote:
> > On Thu, Jan 27, 2011 at 11:40:57AM +1100, Nick Piggin wrote:
> >> On Tue, Jan 25, 2011 at 6:10 PM, Dave Chinner <david@fromorbit.com> wrote:
> >> > On Fri, Jan 14, 2011 at 08:02:57PM +1100, Nick Piggin wrote:
> >> >> I'd like to know about the status and plans of the inode scaling work.
> >> >>
> >> >> Aiming for 2.6.39 or 40 might be an idea.
> >> >
> >> > It was ready for 2.6.38....
> >>
> >> Can it go into linux-next? Unless Al can put it in his for-next branch?
> >
> > It'll go into linux-next whenever it is sufficiently reviewed and
> > tested to get pulled into an upstream branch.
> >
> >> >> I'd like to be able to see what patches are proposed to finish breaking
> >> >> inode_lock into its constituent sub classes, and we can go from there.
> >> >
> >> > Already done. I pointed you at the tree a while back:
> >> >
> >> > The following changes since commit c723fdab8aa728dc2bf0da6a0de8bb9c3f588d84:
> >> >
> >> >  Merge git://git.kernel.org/pub/scm/linux/kernel/git/sfrench/cifs-2.6 (2011-01-25 14:23:54 +1000)
> >> >
> >> > are available in the git repository at:
> >> >
> >> >  git://git.kernel.org/pub/scm/linux/kernel/git/dgc/xfsdev.git inode-scale
> >> >
> >> > Dave Chinner (9):
> >> >      fs: protect inode->i_state with inode->i_lock
> >>
> >> The places where you take i_lock to initialize i_state in newly
> >> allocated inodes should not be required, because they can't be found
> >> until they get added to lists, and adding to the list must have the right
> >> barriers or lock synchronisation to make sure traversals don't see
> >> uninitialised inodes.
> >>
> >> eg.
> >>                          spin_lock(&inode->i_lock);
> >>                          inode->i_state = I_NEW;
> >>                          hlist_add_head(&inode->i_hash, head);
> >>                          spin_unlock(&inode->i_lock);
> >>                          inode_sb_list_add(inode);
> >>                          spin_unlock(&inode_lock);
> >>
> >> That lightens new inode paths a little bit.
> >
> > Not sure what you are try to say here - the above code fragment is
> > exactly how the code ends up after the patch....
> 
> I meant that the i_lock can be removed from that section.
> 
> >
> > As it is, the reason it was done this way was suggested by Al:
> >
> > http://marc.info/?l=linux-fsdevel&m=128832928023866&w=2
> >
> > We use ->i_lock when checking inode_unhashed(), so we need to hold
> > the ->i_lock when adding or removing the inode from the hash list.
> 
> Ok right. I thought you were avoiding that approach in favour of
> checking i_state bits and using the data structure locks (as opposed
> to i_lock).

I've done whatever has seemed sane to do. What Al suggested
simplified the locking, fit in simply with the existing code, was
easy to understand from reading the code and consistent. Seemed like
a no-brainer to me...

> > This is also important for when hash lookups get converted to RCU
> > traversals, because then the only thing protecting a newly added
> > inode from a concurrent traversal is the ->i_lock...
> 
> Well rcu_assign_pointer has a memory barrier in it. So it would be
> possible for rcu lookups to do
> 
> spin_lock(&inode->i_lock);
> if (inode->i_state & I_HASHED) {
>   /* do stuff */
> }
> 
> Even without the spin lock in the inode init code.

Well, we don't have an I_HASHED bit, and this is the first
anyone has mentioned introducing it. Given that we have many
different ways of allocating new and hashing inodes, it seems
unnecessary to change the way we track how inodes are hashed right
now. Especially as some filesystems fake it (see the use of
hlist_add_fake()) because some core functions will only work on
hashed inodes. The proposed method handles this all just fine
without needing to change everything.

> >> "Also remove the unlock_new_inode() memory barrier optimisation
> >> required to avoid taking the inode_lock when clearing I_NEW.
> >> Simplify the code by simply taking the inode->i_lock around the
> >> state change and wakeup. Because the wakeup is no longer tricky,
> >> remove the wake_up_inode() function and open code the wakeup where
> >> necessary."
> >>
> >> Can you do that bit separately?
> >
> > We'd still need to modify that code to protect the i_state
> > manipulations with ->i_lock in this patch, so it doesn't really make
> > sense to me to undo the optimisation in one patch, then remove the
> > rest of it in another patch...
> 
> I'm not sure that I understand. The unlock_new_inode() path, for
> example, could retain the optimisation, couldn't it?

Seeing as it is not a necessary optimisation once the inode-lock is
removed, I much prefer to simplify the code first. If taking the
inode->i_lock proves to cause loss of performance or contention,
then we can re-introduce the optimisation. Otherwise it's just
complexity for the sake of saying "look how smart and tricky we can
be".

> >> >      fs: factor inode disposal
> >>
> >> -       spin_unlock(&inode->i_lock);
> >> -
> >> -       /*
> >> -        * Move the inode off the IO lists and LRU once I_FREEING is
> >> -        * set so that it won't get moved back on there if it is dirty.
> >> -        */
> >>         inode_lru_list_del(inode);
> >> -       list_del_init(&inode->i_wb_list);
> >> -
> >> -       __inode_sb_list_del(inode);
> >> +       spin_unlock(&inode->i_lock);
> >>         spin_unlock(&inode_lock);
> >> +
> >>
> >> Why do you change i_lock to cover inode_lru_list_del in this
> >> patch?
> >
> > So that all LRU manipultions in the function are done consiѕtently
> > i.e. with the ->i_lock held.
> 
> Why do you need that?

Just for consistency. Makes the code easier to understand because
when people look at the function they don't have to go digging to
work out why the differences in locking order isn't a bug....

> >> >      fs: Lock the inode LRU list separately
> >>
> >> BTW. have you found any issues with nesting locks inside i_lock? We
> >> do that extensively in dcache now, but it can be trivially made to use
> >> another lock (eg. i_dcache_lock).
> >
> > No new ones.
> 
> But did you see actual problems that you thought might appear?

What, that people can't rely on the i_lock being innermost anymore?
That's a design issue, so it will 'll take time to shake out as
people add and change code. More than likely it will mean that
filesystems grow their own spinlocks rather than using
inode->i_lock...

> >> >      fs: remove inode_lock from iput_final and prune_icache
> >> >      fs: move i_sb_list out from under inode_lock
> >>
> >> I don't know if we should be including ../internal.h, although you
> >> could argue that quota and notify is core code.
> >
> > I don't think there is much to argue about there.
> 
> So, what is the use of internal.h? And why not just put it in fs.h (like
> other things which are only to be used by core code)?

fs.h is hardly for "only to be used by core code" definitions. Every
man and his dog include it, it defines userspace interfaces (e.g.
ioctls, fcntls, etc), anything defined in it must be exported, etc.
Hence it's not the place to put an external definition for a lock
that is a) not exported and b) should only be used by internal VFS
code....

> I'm not saying one way or the other is "correct", but a single policy
> should be applied.

Yup, and I think I followed the policy for using fs/internal.h here...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html