On Wed 04-05-22 11:25:14, Jchao Sun wrote:
> Commit b35250c0816c ("writeback: Protect inode->i_io_list with
> inode->i_lock") made inode->i_io_list not only protected by
> wb->list_lock but also inode->i_lock, but inode_io_list_move_locked()
> was missed. Add lock there and also update comment describing things
> protected by inode->i_lock.
>
> Fixes: b35250c0816c ("writeback: Protect inode->i_io_list with inode->i_lock")
> Signed-off-by: Jchao Sun <sunjunchao2870@xxxxxxxxx>
Almost there :). A few comments below:
> @@ -2402,6 +2404,9 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> inode->i_state &= ~I_DIRTY_TIME;
> inode->i_state |= flags;
>
> + wb = locked_inode_to_wb_and_lock_list(inode);
> + spin_lock(&inode->i_lock);
> +
We don't want to lock wb->list_lock if the inode was already dirty (which
is a common path). So you want something like:
if (was_dirty)
wb = locked_inode_to_wb_and_lock_list(inode);
(and initialize wb to NULL to make sure it does not contain stale value).
> @@ -2409,7 +2414,7 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> * list, based upon its state.
> */
> if (inode->i_state & I_SYNC_QUEUED)
> - goto out_unlock_inode;
> + goto out_unlock;
>
> /*
> * Only add valid (hashed) inodes to the superblock's
> @@ -2417,22 +2422,19 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> */
> if (!S_ISBLK(inode->i_mode)) {
> if (inode_unhashed(inode))
> - goto out_unlock_inode;
> + goto out_unlock;
> }
> if (inode->i_state & I_FREEING)
> - goto out_unlock_inode;
> + goto out_unlock;
>
> /*
> * If the inode was already on b_dirty/b_io/b_more_io, don't
> * reposition it (that would break b_dirty time-ordering).
> */
> if (!was_dirty) {
> - struct bdi_writeback *wb;
> struct list_head *dirty_list;
> bool wakeup_bdi = false;
>
> - wb = locked_inode_to_wb_and_lock_list(inode);
> -
> inode->dirtied_when = jiffies;
> if (dirtytime)
> inode->dirtied_time_when = jiffies;
> @@ -2446,6 +2448,7 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> dirty_list);
>
> spin_unlock(&wb->list_lock);
> + spin_unlock(&inode->i_lock);
> trace_writeback_dirty_inode_enqueue(inode);
>
> /*
> @@ -2460,6 +2463,8 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> return;
> }
> }
> +out_unlock:
> + spin_unlock(&wb->list_lock);
wb->list lock will not be locked in some cases here. So you have to be more
careful about when you need to unlock it. Probably something like:
if (wb)
spin_unlock(&wb->list_lock);
and you can put this at the end inside the block "if ((inode->i_state &
flags) != flags)".
Also I'd note it is good to test your changes (it would likely uncover the
locking problem). For these filesystem related things xfstests are useful:
https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
