[PATCH] fs-writeback: Flush plug before next iteration in wb_writeback()

[PATCH] fs-writeback: Flush plug before next iteration in wb_writeback()

[Zhihao Cheng]

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
    .   __writeback_single_inode
    .   . generic_writepages
    .   .   __block_write_full_page
    .   .   . . 	    __generic_file_fsync
    .   .   . . 	      sync_inode_metadata
    .   .   . . 	        writeback_single_inode
    .   .   . . 		  __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
    .   .   .   redirty_page_for_writepage
    .   .   .     wbc->pages_skipped++
    .   .   --wbc->nr_to_write
    .   wrote += write_chunk - wbc.nr_to_write  // wrote > 0
    .   requeue_inode
    .     redirty_tail_locked
    if (progress)    // progress > 0
      continue;
  iter i+1:
      queue_io
      // similar process with iter i, infinite for-loop !
}
blk_finish_plug(&plug)   // flush plug won't be called

Above process triggers a hungtask like:
[  399.044861] INFO: task bb:2607 blocked for more than 30 seconds.
[  399.046824]       Not tainted 5.18.0-rc1-00005-gefae4d9eb6a2-dirty
[  399.051539] task:bb              state:D stack:    0 pid: 2607 ppid:
2426 flags:0x00004000
[  399.051556] Call Trace:
[  399.051570]  __schedule+0x480/0x1050
[  399.051592]  schedule+0x92/0x1a0
[  399.051602]  io_schedule+0x22/0x50
[  399.051613]  blk_mq_get_tag+0x1d3/0x3c0
[  399.051640]  __blk_mq_alloc_requests+0x21d/0x3f0
[  399.051657]  blk_mq_submit_bio+0x68d/0xca0
[  399.051674]  __submit_bio+0x1b5/0x2d0
[  399.051708]  submit_bio_noacct+0x34e/0x720
[  399.051718]  submit_bio+0x3b/0x150
[  399.051725]  submit_bh_wbc+0x161/0x230
[  399.051734]  __sync_dirty_buffer+0xd1/0x420
[  399.051744]  sync_dirty_buffer+0x17/0x20
[  399.051750]  __fat_write_inode+0x289/0x310
[  399.051766]  fat_write_inode+0x2a/0xa0
[  399.051783]  __writeback_single_inode+0x53c/0x6f0
[  399.051795]  writeback_single_inode+0x145/0x200
[  399.051803]  sync_inode_metadata+0x45/0x70
[  399.051856]  __generic_file_fsync+0xa3/0x150
[  399.051880]  fat_file_fsync+0x1d/0x80
[  399.051895]  vfs_fsync_range+0x40/0xb0
[  399.051929]  __x64_sys_fsync+0x18/0x30

In my test, 'need_resched()' (which is imported by 590dca3a71 "fs-writeback:
unplug before cond_resched in writeback_sb_inodes") in function
'writeback_sb_inodes()' seldom comes true, unless cond_resched() is deleted
from write_cache_pages().

Fix it by flush plug before next iteration in wb_writeback().

Goto Link to find a reproducer.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=215837
Cc: stable@vger.kernel.org # v4.3
Reported-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/fs-writeback.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 591fe9cf1659..e524c0a1749c 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -2036,8 +2036,21 @@ static long wb_writeback(struct bdi_writeback *wb,
 		 * mean the overall work is done. So we keep looping as long
 		 * as made some progress on cleaning pages or inodes.
 		 */
-		if (progress)
+		if (progress) {
+			/*
+			 * The progress may be false postive in page redirty
+			 * case (which is caused by failing to get buffer head
+			 * lock), which will requeue dirty inodes and start
+			 * next writeback iteration, and other tasks maybe
+			 * stuck for getting tags for new requests. So, flush
+			 * plug to schedule requests holding tags.
+			 *
+			 * The code can be removed after buffer head
+			 * disappering from linux.
+			 */
+			blk_flush_plug(current->plug, false);
 			continue;
+		}
 		/*
 		 * No more inodes for IO, bail
 		 */
-- 
2.31.1

Re: [PATCH] fs-writeback: Flush plug before next iteration in wb_writeback()

[Christoph Hellwig]

On Fri, Apr 15, 2022 at 09:37:35AM +0800, Zhihao Cheng wrote:
> +		if (progress) {
> +			/*
> +			 * The progress may be false postive in page redirty
> +			 * case (which is caused by failing to get buffer head
> +			 * lock), which will requeue dirty inodes and start
> +			 * next writeback iteration, and other tasks maybe
> +			 * stuck for getting tags for new requests. So, flush
> +			 * plug to schedule requests holding tags.
> +			 *
> +			 * The code can be removed after buffer head
> +			 * disappering from linux.
> +			 */
> +			blk_flush_plug(current->plug, false);

This basically removes plugging entirely, so we might as well stop
adding the plug if we can't solve it any other way.  But it seems
like that fake progress needs to be fixed instead.

Re: [PATCH] fs-writeback: Flush plug before next iteration in wb_writeback()

[Zhihao Cheng]

在 2022/4/15 14:39, Christoph Hellwig 写道:
Hi, Christoph
> This basically removes plugging entirely, so we might as well stop
> adding the plug if we can't solve it any other way.  But it seems
> like that fake progress needs to be fixed instead.
> 
Maybe there is a more ideal solution:
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index e524c0a1749c..9723f77841f8 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1855,7 +1855,7 @@ static long writeback_sb_inodes(struct super_block 
*sb,

                 wbc_detach_inode(&wbc);
                 work->nr_pages -= write_chunk - wbc.nr_to_write;
-               wrote += write_chunk - wbc.nr_to_write;
+               wrote += write_chunk - wbc.nr_to_write - wbc.pages_skipped;

                 if (need_resched()) {
                         /*

, or following is better(It looks awkward.):

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index e524c0a1749c..5f310e53bf1e 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1780,6 +1780,7 @@ static long writeback_sb_inodes(struct super_block 
*sb,
         while (!list_empty(&wb->b_io)) {
                 struct inode *inode = wb_inode(wb->b_io.prev);
                 struct bdi_writeback *tmp_wb;
+               long tmp_wrote;

                 if (inode->i_sb != sb) {
                         if (work->sb) {
@@ -1854,8 +1855,11 @@ static long writeback_sb_inodes(struct 
super_block *sb,
                 __writeback_single_inode(inode, &wbc);

                 wbc_detach_inode(&wbc);
-               work->nr_pages -= write_chunk - wbc.nr_to_write;
-               wrote += write_chunk - wbc.nr_to_write;
+               tmp_wrote = write_chunk - wbc.nr_to_write >= 
wbc.pages_skipped ?
+                           write_chunk - wbc.nr_to_write - 
wbc.pages_skipped :
+                           write_chunk - wbc.nr_to_write;workaround
+               work->nr_pages -= tmp_wrote;
+               wrote += tmp_wrote;

                 if (need_resched()) {
                         /*

It depends on how specific filesystem behaves after invoking 
redirty_page_for_writepage(). Most filesystems return 
AOP_WRITEPAGE_ACTIVATE or 0 after invoking redirty_page_for_writepage() 
in writepage, but there still exist accidential examples:
1. metapage_writepage() could returns -EIO after 
redirty_page_for_writepage()
2. ext4_writepage() could returns -ENOMEM after redirty_page_for_writepage()

write_cache_pages
   error = (*writepage)(page, wbc, data);
   if (unlikely(error)) {
     ...
     break;
   }
   --wbc->nr_to_write   // Skip if 'error < 0'. And if writepage invokes 
redirty_page_for_writepage(), wrote could be negative.


I think the root cause is fsync gets buffer head's lock without locking 
corresponding page, fixing 'progess' and flushing plug are both workarounds.

Re: [PATCH] fs-writeback: Flush plug before next iteration in wb_writeback()

[Christoph Hellwig]

> I think the root cause is fsync gets buffer head's lock without locking 
> corresponding page, fixing 'progess' and flushing plug are both 
> workarounds.

So let's fix that.

Re: [PATCH] fs-writeback: Flush plug before next iteration in wb_writeback()

[Zhihao Cheng]

在 2022/4/16 13:42, Christoph Hellwig 写道:
>> I think the root cause is fsync gets buffer head's lock without locking
>> corresponding page, fixing 'progess' and flushing plug are both
>> workarounds.
> 
> So let's fix that.
> 

I think adding page lock before locking buffer head is a little 
difficult and risky:
1. There are too many places getting buffer head before submitting bio, 
and not all filesystems behave same in readpage/writepage/write_inode. 
For example, ntfs_read_block() has locked page before locking buffer 
head and then submitting bh, ext4(no journal) and fat may lock buffer 
head without locking page while writing inode. It's a huge work to check 
all places.
2. Import page lock before locking buffer head may bring new unknown 
problem(other deadlocks about page ?). Taking page lock before locking 
buffer head(in all processes which can be concurrent with wb_writeback) 
is a dangerous thing.

So, how about applying the safe and simple method(flush plug) for the 
time being?
PS: Maybe someday buffer head is removed from all filesystems, then we 
can remove this superfluous blk_flush_plug.

Re: [PATCH] fs-writeback: Flush plug before next iteration in wb_writeback()

[Christoph Hellwig]

On Sat, Apr 16, 2022 at 04:41:35PM +0800, Zhihao Cheng wrote:
> So, how about applying the safe and simple method(flush plug) for the time 
> being?

As said before - if you flush everytime here the plug is basically
useless and we could remove it.  But it was added for a reason.  So let's
at least improve the accounting for the skipped writeback as suggested in
your last mail.