Re: [RFC][PATCH 2/2] tux3: Use writeback hook to remove duplicated core code

[Dave Chinner <david@fromorbit.com>]

On Sun, Jun 01, 2014 at 02:42:48PM -0700, Daniel Phillips wrote:
> Instead of re-implementing part of fs/fs-writeback.c, use a proposed
> net ->writeback super operation to drive delta writeback. For each
> inode that is cleaned, call inode_writeback_done(inode). For each
> inode that will be kept dirty in cache, call inode_writeback_touch
> so that the inode appears young to fs-writeback and does not trigger
> repeated ->writeback flushes.
> 
> Signed-off-by: Daniel Phillips <daniel@tux3.org>

I have not looked at the sanity of the tux3 writeback algorithm, so
I'm not commenting on whether it works or not. However, this caught
my eye:

>  static void __tux3_clear_dirty_inode(struct inode *inode, unsigned delta)
>  {
>      struct tux3_inode *tuxnode = tux_inode(inode);
> -    tux3_inode_wb_lock(inode);
>      spin_lock(&inode->i_lock);
>      spin_lock(&tuxnode->lock);
>      tux3_clear_dirty_inode_nolock(inode, delta, 0);
>      spin_unlock(&tuxnode->lock);
>      spin_unlock(&inode->i_lock);
> -    tux3_inode_wb_unlock(inode);
> +    inode_writeback_done(inode);
>  }

I get very worried whenever I see locks inside inode->i_lock. In
general, i_lock is supposed to be the innermost lock that is taken,
and there are very few exceptions to that - the inode LRU list is
one of the few.

I don't know what the tuxnode->lock is, but I found this:

 *     inode->i_lock
 *         tuxnode->lock (to protect tuxnode data)
 *             tuxnode->dirty_inodes_lock (for i_ddc->dirty_inodes,
 *                                         Note: timestamp can be updated
 *                                         outside inode->i_mutex)

and this:

 *     inode->i_lock
 *         tuxnode->lock
 *         sb->dirty_inodes_lock

Which indicates that you take a filesystem global lock a couple of
layers underneath the VFS per-inode i_lock. I'd suggest you want to
separate the use of the vfs inode ilock from the locking heirarchy
of the tux3 inode....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com