Re: [PATCH 13/17] fs: Implement lazy LRU updates for inodes.

[Christoph Hellwig]

> @@ -1058,8 +1051,6 @@ static void wait_sb_inodes(struct super_block *sb)
>  	 */
>  	WARN_ON(!rwsem_is_locked(&sb->s_umount));
>  
> -	spin_lock(&sb_inode_list_lock);
> -
>  	/*
>  	 * Data integrity sync. Must wait for all pages under writeback,
>  	 * because there may have been pages dirtied before our sync
> @@ -1067,6 +1058,7 @@ static void wait_sb_inodes(struct super_block *sb)
>  	 * In which case, the inode may not be on the dirty list, but
>  	 * we still have to wait for that writeout.
>  	 */
> +	spin_lock(&sb_inode_list_lock);

I think this should be folded back into the patch introducing
sb_inode_list_lock.

> @@ -1083,10 +1075,10 @@ static void wait_sb_inodes(struct super_block *sb)
>  		spin_unlock(&sb_inode_list_lock);
>  		/*
>  		 * We hold a reference to 'inode' so it couldn't have been
> -		 * removed from s_inodes list while we dropped the
> -		 * sb_inode_list_lock.  We cannot iput the inode now as we can
> -		 * be holding the last reference and we cannot iput it under
> -		 * spinlock. So we keep the reference and iput it later.
> +		 * removed from s_inodes list while we dropped the i_lock.  We
> +		 * cannot iput the inode now as we can be holding the last
> +		 * reference and we cannot iput it under spinlock. So we keep
> +		 * the reference and iput it later.

This also looks like a hunk that got in by accident and should be merged
into an earlier patch.

> @@ -431,11 +412,12 @@ static int invalidate_list(struct list_head *head, struct list_head *dispose)
>  		invalidate_inode_buffers(inode);
>  		if (!inode->i_count) {
>  			spin_lock(&wb_inode_list_lock);
> -			list_move(&inode->i_list, dispose);
> +			list_del(&inode->i_list);
>  			spin_unlock(&wb_inode_list_lock);
>  			WARN_ON(inode->i_state & I_NEW);
>  			inode->i_state |= I_FREEING;
>  			spin_unlock(&inode->i_lock);
> +			list_add(&inode->i_list, dispose);

Moving the list_add out of the lock looks fine, but I can't really
see how it's related to the rest of the patch.

> +		if (inode->i_count || (inode->i_state & ~I_REFERENCED)) {
> +			list_del_init(&inode->i_list);
> +			spin_unlock(&inode->i_lock);
> +			atomic_dec(&inodes_stat.nr_unused);
> +			continue;
> +		}
> +		if (inode->i_state) {

Slightly confusing but okay given the only i_state that will get us here
is I_REFERENCED.  Do we really care about the additional cycle or two a
dumb compiler might generate when writing

	if (inode->i_state & I_REFERENCED)

?

>  		if (inode_has_buffers(inode) || inode->i_data.nrpages) {
> +			list_move(&inode->i_list, &inode_unused);

Why are we now moving the inode to the front of the list? 

> @@ -687,9 +652,6 @@ __inode_add_to_lists(struct super_block *sb, struct inode_hash_bucket *b,
>  	atomic_inc(&inodes_stat.nr_inodes);
>  	list_add(&inode->i_sb_list, &sb->s_inodes);
>  	spin_unlock(&sb_inode_list_lock);
> -	spin_lock(&wb_inode_list_lock);
> -	list_add(&inode->i_list, &inode_in_use);
> -	spin_unlock(&wb_inode_list_lock);
>  	if (b) {
>  		spin_lock_bucket(b);
>  		hlist_bl_add_head(&inode->i_hash, &b->head);

At some point it might be worth to split this into

	inode_add_to_sb_list

and

	__inode_add_to_hash

but that can be left for later.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/