linux-kernel - Re: [PATCH] fs-writeback: Flush plug before next iteration in wb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 15 Apr 2022 16:39:12 +0800
From:   Zhihao Cheng <chengzhihao1@...wei.com>
To:     Christoph Hellwig <hch@....de>
CC:     <viro@...iv.linux.org.uk>, <torvalds@...ux-foundation.org>,
        <linux-fsdevel@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <yukuai3@...wei.com>
Subject: Re: [PATCH] fs-writeback: Flush plug before next iteration in
 wb_writeback()

在 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.