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@xxxxxxxx>
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@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
