Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v2] fs-writeback: writeback_sb_inodes: Recalculate 'wrote'
 according skipped pages
To:     Hillf Danton <hdanton@sina.com>
CC:     <hch@lst.de>, <torvalds@linux-foundation.org>,
        <linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <yukuai3@huawei.com>
References: <20220418112745.1761-1-hdanton@sina.com>
From:   Zhihao Cheng <chengzhihao1@huawei.com>
Message-ID: <4df77267-09f0-c4be-116e-86e447cbd292@huawei.com>
Date:   Mon, 18 Apr 2022 21:27:09 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <20220418112745.1761-1-hdanton@sina.com>
Content-Type: text/plain; charset="gbk"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.