Re: Status of inode scaling work

[Dave Chinner <david@fromorbit.com>]

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....

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.
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...

> "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...

> >      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.

> >      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.

> >      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.

> >      fs: move i_wb_list out from under inode_lock
> >      fs: rename inode_lock to inode_hash_lock
> >      fs: Clean up documentation references to inode_lock
> 
> Can these be merged with the patches as they change the locking?
> Some places also call it "inode lock" (eg. find_inode())

Yeah, probably should.

> >      fs: pull inode->i_lock up out of writeback_single_inode
> 
> I think the series looks pretty good overall. Thanks.

Don't thank me, thank Christoph, Al, Andrew, Eric and others for
reviewing it over and over to beat it into this shape.

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