On Wed 11-05-22 07:15:18, 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.
Please expand the changelog a bit to mention to important fix in
__mark_inode_dirty(). Like:
This also fixes a race where __mark_inode_dirty() could move inode under
flush worker's hands and thus sync(2) could miss writing some inodes.
> Fixes: b35250c0816c ("writeback: Protect inode->i_io_list with inode->i_lock")
> Signed-off-by: Jchao Sun <sunjunchao2870@xxxxxxxxx>
...
> @@ -1365,9 +1366,9 @@ static int move_expired_inodes(struct list_head *delaying_queue,
> inode = wb_inode(delaying_queue->prev);
> if (inode_dirtied_after(inode, dirtied_before))
> break;
> + spin_lock(&inode->i_lock);
> list_move(&inode->i_io_list, &tmp);
> moved++;
> - spin_lock(&inode->i_lock);
> inode->i_state |= I_SYNC_QUEUED;
> spin_unlock(&inode->i_lock);
> if (sb_is_blkdev_sb(inode->i_sb))
> @@ -1383,6 +1384,7 @@ static int move_expired_inodes(struct list_head *delaying_queue,
> goto out;
> }
>
> + spin_lock(&inode->i_lock);
This is definitely wrong. 'inode' here is just something random left over
in the variable from the previous loop. As I wrote in my previous review, I
don't think taking 'i_lock' in the loop below is needed at all, although it
probably deserves a comment like:
/*
* Although inode's i_io_list is moved from 'tmp' to
* 'dispatch_queue', we don't take inode->i_lock here because it is
* just a pointless overhead. Inode is already marked as
* I_SYNC_QUEUED so writeback list handling is fully under our
* control.
*/
> /* Move inodes from one superblock together */
> while (!list_empty(&tmp)) {
> sb = wb_inode(tmp.prev)->i_sb;
> @@ -1392,6 +1394,7 @@ static int move_expired_inodes(struct list_head *delaying_queue,
> list_move(&inode->i_io_list, dispatch_queue);
> }
> }
> + spin_unlock(&inode->i_lock);
> out:
> return moved;
> }
...
> @@ -2402,6 +2406,11 @@ void __mark_inode_dirty(struct inode *inode, int flags)
> inode->i_state &= ~I_DIRTY_TIME;
> inode->i_state |= flags;
>
Perhaps add a comment here like:
/*
* Grab inode's wb early because it requires dropping i_lock and we
* need to make sure following checks happen atomically with dirty
* list handling so that we don't move inodes under flush worker's
* hands.
*/
> + if (!was_dirty) {
> + wb = locked_inode_to_wb_and_lock_list(inode);
> + spin_lock(&inode->i_lock);
> + }
> +
> /*
> * If the inode is queued for writeback by flush worker, just
> * update its dirty state. Once the flush worker is done with
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
