HI Hillf,
> On Mon, 18 Apr 2022 17:28:24 +0800 Zhihao Cheng wrote:
>> Commit 505a666ee3fc ("writeback: plug writeback in wb_writeback() and
>> writeback_inodes_wb()") has us holding a plug during wb_writeback, which
>> may cause a potential ABBA dead lock:
>>
>> wb_writeback fat_file_fsync
>> blk_start_plug(&plug)
>> for (;;) {
>> iter i-1: some reqs have been added into plug->mq_list // LOCK A
>> iter i:
>> progress = __writeback_inodes_wb(wb, work)
>> . writeback_sb_inodes // fat's bdev
See comments " fat's bdev".
>
> if (inode->i_state & I_SYNC) {
> /* Wait for I_SYNC. This function drops i_lock... */
> inode_sleep_on_writeback(inode);
> /* Inode may be gone, start again */
> spin_lock(&wb->list_lock);
> continue;
> }
> inode->i_state |= I_SYNC;
This inode is fat's bdev's inode.
>
>> . __writeback_single_inode
>> . . generic_writepages
>> . . __block_write_full_page
>> . . . . __generic_file_fsync
>> . . . . sync_inode_metadata
>> . . . . writeback_single_inode
>
> if (inode->i_state & I_SYNC) {
This inode is fat's inode.
> /*
> * Writeback is already running on the inode. For WB_SYNC_NONE,
> * that's enough and we can just return. For WB_SYNC_ALL, we
> * must wait for the existing writeback to complete, then do
> * writeback again if there's anything left.
> */
> if (wbc->sync_mode != WB_SYNC_ALL)
> goto out;
> __inode_wait_for_writeback(inode);
> }
> inode->i_state |= I_SYNC;
>
>> . . . . __writeback_single_inode
>> . . . . fat_write_inode
>> . . . . __fat_write_inode
>> . . . . sync_dirty_buffer // fat's bdev
>> . . . . lock_buffer(bh) // LOCK B
>> . . . . submit_bh
>> . . . . blk_mq_get_tag // LOCK A
>> . . . trylock_buffer(bh) // LOCK B
>
> Given I_SYNC checked on both sides, the chance for ABBA deadlock is zero.
Above two inodes are not same.