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

Dave Chinner
2014-06-02T03:30:52Z

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