Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
From:   Chunhai Guo <guochunhai@vivo.com>
To:     jack@suse.cz, chao@kernel.org, jaegeuk@kernel.org
Cc:     brauner@kernel.org, viro@zeniv.linux.org.uk,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fs-writeback: writeback_sb_inodes: Do not increase 'total_wrote' when nothing is written
Date:   Wed, 13 Sep 2023 07:15:01 -0600
Message-Id: <20230913131501.478516-1-guochunhai@vivo.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?3CuDhN7P6JKARB7v2s1B2ien0qNbj/4UOT1BiMsieGtlSz6CrOA25bVxKfFB?=
 =?us-ascii?Q?fa2ziRX0AUAFYtPFQrHS+q4mBxb0fOuefdMVw0GVaVeuYXFTFzAblNT6Pr5Q?=
 =?us-ascii?Q?71XGf2V+imLXzCo9o24EI/0ydnn5/uckv0Xpv6Vegttr6frUT6xLRxMXzjrz?=
 =?us-ascii?Q?iCcF3+Uq21OT14Nc6pnFnXeIu1VM4lPbaHmcaeRY/PuUS8s54OfZlJsGUGCJ?=
 =?us-ascii?Q?wZ7OlPo0z5tzPg7uijmVkO90PMtcrpABkaFQCbv68FtFfV5HslItH3sqUf9B?=
 =?us-ascii?Q?fhtWGUsidkE7on15PV+ZOMdUcUd26UPdg3qqu78QkOU3jlqu9r+7GEblYzw8?=
 =?us-ascii?Q?QL62fhf9ZBK4a294rwL47Fnn1YpINsYohXp1bPfHxz+Y1ue1zFy5n7cUgFyz?=
 =?us-ascii?Q?XjUTOxfGMD2iSn4SCt2IaXYF0jXQNzMkpUUGWu7/m+ZboHHRING/YhQnBVl/?=
 =?us-ascii?Q?S069tKl9z39AVzuN1swC/S6ftzTyP+LCsu+zOy4WNNL/mtmQfWxQgmxZHMrg?=
 =?us-ascii?Q?l/3vywertcBoFt8gk3QKjA6tcCMtx2PkTabi+hnOljEdjHrQA/JFxda+Vr3B?=
 =?us-ascii?Q?XR5825ZuiChatid+dm8QxZVENDLScLbufnum/VRytkk9mu2VW1N38j2VuXF5?=
 =?us-ascii?Q?RIxnXrwXCuZJO7Z+JqGnqFCdnogkzac+eqkfP9sea5NC8wuTN26LFCbOy+zw?=
 =?us-ascii?Q?O2z9bo8h7MaDerjRIYu4hLJ2XvTwH37CZ6WVLJZsMjr764aHf+mosR3mnF1O?=
 =?us-ascii?Q?WOBhbaC0YiekXYJhqzZCkG2pirxI/t/i67/PILQUyLvBsmPK5F1y9FrLXF0e?=
 =?us-ascii?Q?W4U8OZ1yGl2fyH2WfEr8/55wnr4J0zYGMyobtH6DuPTxYqkAJdmohx/bBQGy?=
 =?us-ascii?Q?34jZBddb4hsmTXwG4+nlk7E+feQq48qjfuCrgxfL1P7t1Zd/JJ0b+yBcap83?=
 =?us-ascii?Q?4d9ff9aObCvNSJeCAxf58pfOd8/7NCKeLnNRNm2+LlJAyXDUSU/AVdv/C6mo?=
 =?us-ascii?Q?RmNoNOJwwJ4+6k0s7GVYyevuFL2vD6KaUQCNxFVaMmu3a+Mw4jdkcfxcreEL?=
 =?us-ascii?Q?2GrRLWUvkau2ABchksMtF9ShuYz9WN8qZ/Os7E0mBtZaNl8wOfEj8v5L77QT?=
 =?us-ascii?Q?2N6h6CJxwIoRkFghNV28pqBL8/Cjm/R2B/A0gwL/S6v9ny7WE4h6QU3QG1jO?=
 =?us-ascii?Q?7VxI2BjhxDKKrb8SSHxD/j8dNz4jIXlX+gseXbVRcKkO+aR37mv+gsp9p5Hk?=
 =?us-ascii?Q?EXRWLa9hI3Iid38WhjPwVGuOiNqAbxaOI+fSInRNRBvYc41t0mXbhcyGqLhq?=
 =?us-ascii?Q?eQBc3CSlSzaxYFtuOUYDVlMi5lpdAVsUusq6gq7jkKxtDpgs1TEoYJS3H9Io?=
 =?us-ascii?Q?YaKjc3ORj/hIa1ffB56Moqbje63ambvTfMj77Ajr3TFrgZwZminsTdoNSHdJ?=
 =?us-ascii?Q?Ib05el/lh1gjH1yH6/tm3UTn/vh8I6xuMpfcG36/HVkaarYBxDagwdPHp52R?=
 =?us-ascii?Q?J800EZauw/1IkW7zeHgpcPV/cCncxfdn7ivseS7q3++htcKaTnFNQisoJdBq?=
 =?us-ascii?Q?sgEpu7PYRcK1nYs9Pvm+nKkUgAqdthfTY/3ryCIz?=
X-OriginatorOrg: vivo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f8a2b480-c88d-4f39-6b3c-08dbb45a5725
X-MS-Exchange-CrossTenant-AuthSource: TY2PR06MB3342.apcprd06.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Sep 2023 13:07:10.7306
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 923e42dc-48d5-4cbe-b582-1a797a6412ed
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: RiTGCzTZhG0yAnu9DSJ6M8J1cZ5uY1LY013l6bxIYs8iS5roHFrYhUxL7hPSGxd9BCvcCPMUwJrxz84PJXBYvg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SEYPR06MB6455
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Wed, 13 Sep 2023 06:07:39 -0700 (PDT)
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email

> On Wed 13-09-23 10:42:21, Christian Brauner wrote:
> > [+Cc Jan]
> 
> Thanks!
> 
> > On Tue, Sep 12, 2023 at 08:20:43AM -0600, Chunhai Guo wrote:
> > > I am encountering a deadlock issue as shown below. There is a commit 
> > > 344150999b7f ("f2fs: fix to avoid potential deadlock") can fix this
> > > issue.
> > > However, from log analysis, it appears that this is more likely a 
> > > fake progress issue similar to commit 68f4c6eba70d ("fs-writeback:
> > > writeback_sb_inodes: Recalculate 'wrote' according skipped pages"). 
> > > In each writeback iteration, nothing is written, while 
> > > writeback_sb_inodes() increases 'total_wrote' each time, causing an 
> > > infinite loop. This patch fixes this issue by not increasing
> > > 'total_wrote' when nothing is written.
> > >
> > >     wb_writeback        fsync (inode-Y)
> > > blk_start_plug(&plug)
> > > for (;;) {
> > >   iter i-1: some reqs with page-X added into plug->mq_list // f2fs node
> > >   page-X with PG_writeback
> > >                         filemap_fdatawrite
> > >                           __filemap_fdatawrite_range // write inode-Y
> > >                           with sync_mode WB_SYNC_ALL
> > >                            do_writepages
> > >                             f2fs_write_data_pages
> > >                              __f2fs_write_data_pages //
> > >                              wb_sync_req[DATA]++ for WB_SYNC_ALL
> > >                               f2fs_write_cache_pages
> > >                                f2fs_write_single_data_page
> > >                                 f2fs_do_write_data_page
> > >                                  f2fs_outplace_write_data
> > >                                   f2fs_update_data_blkaddr
> > >                                    f2fs_wait_on_page_writeback
> > >                                      wait_on_page_writeback // wait for
> > >                                      f2fs node page-X
> > >   iter i:
> > >     progress = __writeback_inodes_wb(wb, work)
> > >     . writeback_sb_inodes
> > >     .   __writeback_single_inode // write inode-Y with sync_mode
> > >     WB_SYNC_NONE
> > >     .   . do_writepages
> > >     .   .   f2fs_write_data_pages
> > >     .   .   .  __f2fs_write_data_pages // skip writepages due to
> > >     (wb_sync_req[DATA]>0)
> > >     .   .   .   wbc->pages_skipped += get_dirty_pages(inode) //
> > >     wbc->pages_skipped = 1
> > >     .   if (!(inode->i_state & I_DIRTY_ALL)) // i_state = I_SYNC |
> > >     I_SYNC_QUEUED
> > >     .    total_wrote++;  // total_wrote = 1
> > >     .   requeue_inode // requeue inode-Y to wb->b_dirty queue due to
> > >     non-zero pages_skipped
> > >     if (progress) // progress = 1
> > >       continue;
> > >   iter i+1:
> > >       queue_io
> > >       // similar process with iter i, infinite for-loop !
> > > }
> > > blk_finish_plug(&plug)   // flush plug won't be called
> > >
> > > Signed-off-by: Chunhai Guo <guochunhai@vivo.com>
> 
> Thanks for the patch but did you test this patch fixed your deadlock?
> Because the patch seems like a noop to me. Look:

Yes. I have tested this patch and it indeed fixed this deadlock issue, too.

> 
> > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index 
> > > 969ce991b0b0..54cdee906be9 100644
> > > --- a/fs/fs-writeback.c
> > > +++ b/fs/fs-writeback.c
> > > @@ -1820,6 +1820,7 @@ static long writeback_sb_inodes(struct
> > > super_block *sb,
> > >             struct inode *inode = wb_inode(wb->b_io.prev);
> > >             struct bdi_writeback *tmp_wb;
> > >             long wrote;
> > > +           bool is_dirty_before;
> > >
> > >             if (inode->i_sb != sb) {
> > >                     if (work->sb) {
> > > @@ -1881,6 +1882,7 @@ static long writeback_sb_inodes(struct
> > > super_block *sb,
> > >                     continue;
> > >             }
> > >             inode->i_state |= I_SYNC;
> > > +           is_dirty_before = inode->i_state & I_DIRTY_ALL;
> 
> is_dirty_before is going to be set if there's anything dirty - inode, page,
> timestamp. So it can be unset only if there are no dirty pages, in which
> case there are no pages that can be skipped during page writeback, which
> means that requeue_inode() will go and remove inode from b_io/b_dirty lists
> and it will not participate in writeback anymore.
> 
> So I don't see how this patch can be helping anything... Please correct me
> if I'm missing anything.
>                                                                 Honza

From the dump info, there are only two pages as shown below. One is updated
and another is under writeback. Maybe f2fs counts the writeback page as a
dirty one, so get_dirty_pages() got one. As you said, maybe this is
unreasonable.

Jaegeuk & Chao, what do you think of this?


crash_32> files -p 0xE5A44678
 INODE    NRPAGES
e5a44678        2

  PAGE    PHYSICAL   MAPPING    INDEX CNT FLAGS
e8d0e338  641de000  e5a44810         0  5 a095 locked,waiters,uptodate,lru,private,writeback
e8ad59a0  54528000  e5a44810         1  2 2036 referenced,uptodate,lru,active,private

Thanks,

> 
> 
> > >             wbc_attach_and_unlock_inode(&wbc, inode);
> > >
> > >             write_chunk = writeback_chunk_size(wb, work); @@ -1918,7 
> > > +1920,7 @@ static long writeback_sb_inodes(struct super_block *sb,
> > >              */
> > >             tmp_wb = inode_to_wb_and_lock_list(inode);
> > >             spin_lock(&inode->i_lock);
> > > -           if (!(inode->i_state & I_DIRTY_ALL))
> > > +           if (!(inode->i_state & I_DIRTY_ALL) && is_dirty_before)
> > >                     total_wrote++;
> > >             requeue_inode(inode, tmp_wb, &wbc);
> > >             inode_sync_complete(inode);
> > > --
> > > 2.25.1
> > >
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR