On Wed 04-05-22 00:54:21, 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")
> Reviewed-by: Jan Kara <jack@xxxxxxx>
You have significantly modified the patch since last time (which is good
since 0-day testing has found a problem I have missed). But in such case
please do not add Reviewed-by tags (and also we usually drop already
existing ones in the patch) because the patch needs a new review.
> @@ -317,6 +318,7 @@ locked_inode_to_wb_and_lock_list(struct inode *inode)
> /* i_wb may have changed inbetween, can't use inode_to_wb() */
> if (likely(wb == inode->i_wb)) {
> wb_put(wb); /* @inode already has ref */
> + spin_lock(&inode->i_lock);
> return wb;
> }
>
Please no. I agree the inode->i_lock and wb->list_lock handling in
fs/fs-writeback.c is ugly and perhaps deserves a cleanup but this isn't
making it easier to understand and it definitely belongs to a separate
patch. Also the problem in __mark_inode_dirty() is deeper than just not
holding inode->i_lock when calling inode_io_list_move_locked(). The problem
is that locked_inode_to_wb_and_lock_list() drops inode->i_lock and at that
moment inode can be moved to new lists, writeback can be started on it,
etc. So all the checks we have performed upto that moment are not valid
anymore. So what we need to do is to move
locked_inode_to_wb_and_lock_list() call up, perhaps just after
"inode->i_state |= flags;". Then lock inode->i_lock again, and then perform
all the checks we need and list moving etc.
> Sry for my insufficient tests and any inconvenience in lauguage, because
> my mother tongue is not english. It's my first commit to linux kernel
> community, some strage for me...
Yeah, no problem. You are doing good :)
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
