* [PATCH] writeback: Avoid postponing periodic data writeback indefinitely
@ 2026-10-26 21:10 Jan Kara
2026-10-27 6:08 ` Hongyu Jin
0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2026-10-26 21:10 UTC (permalink / raw)
To: Christian Brauner; +Cc: linux-fsdevel, Jan Kara, Hongyu Jin
Hongyu Jin has reported that f2fs periodically calls
sync_inode_metadata() for some inodes. The logic in
writeback_single_inode() thus periodically calls to redirty_tail() and
the inode's dirtied_when timestamp never gets old enough for the inode
to be eligible for periodic data writeback. Thus dirty data from it is
only ever written on explicit request (fsync(2), sync(2), unmount) or
on dirty limit pressure.
The redirty_tail() logic in writeback_single_inode() actually should not
be necessary. Since this function is used only for once-off writeback,
there's no need for any livelock protection among inodes and
writeback_single_inode() is only responsible for demoting inode to "less
dirty" list based on dirty bits cleared as a result of writeback. So
remove the redirty_tail() logic and add a comment explaining the rules.
Link: https://lore.kernel.org/all/20261023023707.263101-1-hongyu.jin@unisoc.com
Reported-by: Hongyu Jin <hongyu.jin.cn@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/fs-writeback.c | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 58a6403780aa..de86c9055ffb 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1946,22 +1946,31 @@ static int writeback_single_inode(struct inode *inode,
* as it can be finally deleted at this moment.
*/
if (!(inode_state_read(inode) & I_FREEING)) {
+ enum inode_state_flags_enum istate = inode_state_read(inode);
+
/*
- * If the inode is now fully clean, then it can be safely
- * removed from its writeback list (if any). Otherwise the
- * flusher threads are responsible for the writeback lists.
+ * __mark_inode_dirty() is responsible for promoting inode to
+ * the proper list (b_attached -> b_dirty_time -> b_dirty).
+ * Here we are responsible for demoting inode after some dirty
+ * bits were cleared.
+ *
+ * If the inode is now fully clean, then it can be safely moved
+ * to b_attached list. If the inode is dirty, we don't want to
+ * touch it - it should be already put to the dirty list by
+ * __mark_inode_dirty() and we'd just unnecessarily clobber the
+ * dirtied_when stamp (which is particularly problematic when
+ * we were called from sync_inode_metadata()). We also cannot
+ * touch the dirty list association when I_SYNC_QUEUED is set
+ * and flush worker is responsible for it. Otherwise move the
+ * inode to b_dirty_time list as that's the only remaining
+ * possibility.
*/
- if (!(inode_state_read(inode) & I_DIRTY_ALL))
+ if (!(istate & I_DIRTY_ALL))
inode_cgwb_move_to_attached(inode, wb);
- else if (!(inode_state_read(inode) & I_SYNC_QUEUED)) {
- if ((inode_state_read(inode) & I_DIRTY))
- redirty_tail_locked(inode, wb);
- else if (inode_state_read(inode) & I_DIRTY_TIME) {
- inode->dirtied_when = jiffies;
- inode_io_list_move_locked(inode,
- wb,
- &wb->b_dirty_time);
- }
+ else if (!(istate & I_SYNC_QUEUED) && !(istate & I_DIRTY)) {
+ WARN_ON_ONCE(!(istate & I_DIRTY_TIME));
+ inode->dirtied_when = jiffies;
+ inode_io_list_move_locked(inode, wb, &wb->b_dirty_time);
}
}
--
2.51.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] writeback: Avoid postponing periodic data writeback indefinitely
2026-10-26 21:10 [PATCH] writeback: Avoid postponing periodic data writeback indefinitely Jan Kara
@ 2026-10-27 6:08 ` Hongyu Jin
2026-10-27 12:16 ` Jan Kara
0 siblings, 1 reply; 3+ messages in thread
From: Hongyu Jin @ 2026-10-27 6:08 UTC (permalink / raw)
To: Jan Kara; +Cc: Christian Brauner, linux-fsdevel
Commit 846 a3351ddfe4a86eede4bb26a205c3f38ef84d3 problem need to be considered:
We have run into an issue that a task gets stuck in
balance_dirty_pages_ratelimited() when perform I/O stress testing.
The reason we observed is that an I_DIRTY_PAGES inode with lots
of dirty pages is in b_dirty_time list and standard background
writeback cannot writeback the inode.
After studing the relevant code, the following scenario may lead
to the issue:
task1 task2
----- -----
fuse_flush
write_inode_now // in b_dirty_time
writeback_single_inode
__writeback_single_inode
fuse_write_end
filemap_dirty_folio
__xa_set_mark:PAGECACHE_TAG_DIRTY
lock inode->i_lock
if mapping tagged PAGECACHE_TAG_DIRTY
inode->i_state |= I_DIRTY_PAGES
unlock inode->i_lock
__mark_inode_dirty:I_DIRTY_PAGES
lock inode->i_lock
-was dirty, inode stays in b_dirty_time
unlock inode->i_lock
if(!(inode->i_state & I_DIRTY_All))
-not true,so nothing done
Jan Kara <jack@suse.cz> 于2026年10月27日周二 05:10写道：
>
> Hongyu Jin has reported that f2fs periodically calls
> sync_inode_metadata() for some inodes. The logic in
> writeback_single_inode() thus periodically calls to redirty_tail() and
> the inode's dirtied_when timestamp never gets old enough for the inode
> to be eligible for periodic data writeback. Thus dirty data from it is
> only ever written on explicit request (fsync(2), sync(2), unmount) or
> on dirty limit pressure.
>
> The redirty_tail() logic in writeback_single_inode() actually should not
> be necessary. Since this function is used only for once-off writeback,
> there's no need for any livelock protection among inodes and
> writeback_single_inode() is only responsible for demoting inode to "less
> dirty" list based on dirty bits cleared as a result of writeback. So
> remove the redirty_tail() logic and add a comment explaining the rules.
>
> Link: https://lore.kernel.org/all/20261023023707.263101-1-hongyu.jin@unisoc.com
> Reported-by: Hongyu Jin <hongyu.jin.cn@gmail.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/fs-writeback.c | 35 ++++++++++++++++++++++-------------
> 1 file changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 58a6403780aa..de86c9055ffb 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -1946,22 +1946,31 @@ static int writeback_single_inode(struct inode *inode,
> * as it can be finally deleted at this moment.
> */
> if (!(inode_state_read(inode) & I_FREEING)) {
> + enum inode_state_flags_enum istate = inode_state_read(inode);
> +
> /*
> - * If the inode is now fully clean, then it can be safely
> - * removed from its writeback list (if any). Otherwise the
> - * flusher threads are responsible for the writeback lists.
> + * __mark_inode_dirty() is responsible for promoting inode to
> + * the proper list (b_attached -> b_dirty_time -> b_dirty).
> + * Here we are responsible for demoting inode after some dirty
> + * bits were cleared.
> + *
> + * If the inode is now fully clean, then it can be safely moved
> + * to b_attached list. If the inode is dirty, we don't want to
> + * touch it - it should be already put to the dirty list by
> + * __mark_inode_dirty() and we'd just unnecessarily clobber the
> + * dirtied_when stamp (which is particularly problematic when
> + * we were called from sync_inode_metadata()). We also cannot
> + * touch the dirty list association when I_SYNC_QUEUED is set
> + * and flush worker is responsible for it. Otherwise move the
> + * inode to b_dirty_time list as that's the only remaining
> + * possibility.
> */
> - if (!(inode_state_read(inode) & I_DIRTY_ALL))
> + if (!(istate & I_DIRTY_ALL))
> inode_cgwb_move_to_attached(inode, wb);
> - else if (!(inode_state_read(inode) & I_SYNC_QUEUED)) {
> - if ((inode_state_read(inode) & I_DIRTY))
> - redirty_tail_locked(inode, wb);
> - else if (inode_state_read(inode) & I_DIRTY_TIME) {
> - inode->dirtied_when = jiffies;
> - inode_io_list_move_locked(inode,
> - wb,
> - &wb->b_dirty_time);
> - }
> + else if (!(istate & I_SYNC_QUEUED) && !(istate & I_DIRTY)) {
> + WARN_ON_ONCE(!(istate & I_DIRTY_TIME));
> + inode->dirtied_when = jiffies;
> + inode_io_list_move_locked(inode, wb, &wb->b_dirty_time);
> }
> }
>
> --
> 2.51.0
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] writeback: Avoid postponing periodic data writeback indefinitely
2026-10-27 6:08 ` Hongyu Jin
@ 2026-10-27 12:16 ` Jan Kara
0 siblings, 0 replies; 3+ messages in thread
From: Jan Kara @ 2026-10-27 12:16 UTC (permalink / raw)
To: Hongyu Jin; +Cc: Jan Kara, Christian Brauner, linux-fsdevel
On Tue 27-10-26 14:08:09, Hongyu Jin wrote:
> Commit 846 a3351ddfe4a86eede4bb26a205c3f38ef84d3 problem need to be considered:
>
> We have run into an issue that a task gets stuck in
> balance_dirty_pages_ratelimited() when perform I/O stress testing.
> The reason we observed is that an I_DIRTY_PAGES inode with lots
> of dirty pages is in b_dirty_time list and standard background
> writeback cannot writeback the inode.
> After studing the relevant code, the following scenario may lead
> to the issue:
>
> task1 task2
> ----- -----
> fuse_flush
> write_inode_now // in b_dirty_time
> writeback_single_inode
> __writeback_single_inode
> fuse_write_end
> filemap_dirty_folio
> __xa_set_mark:PAGECACHE_TAG_DIRTY
> lock inode->i_lock
> if mapping tagged PAGECACHE_TAG_DIRTY
> inode->i_state |= I_DIRTY_PAGES
> unlock inode->i_lock
> __mark_inode_dirty:I_DIRTY_PAGES
> lock inode->i_lock
> -was dirty, inode stays in b_dirty_time
> unlock inode->i_lock
>
> if(!(inode->i_state & I_DIRTY_All))
> -not true,so nothing done
Hum, right. Thanks for testing! Back to drawing board...
Honza
> Jan Kara <jack@suse.cz> 于2026年10月27日周二 05:10写道：
> >
> > Hongyu Jin has reported that f2fs periodically calls
> > sync_inode_metadata() for some inodes. The logic in
> > writeback_single_inode() thus periodically calls to redirty_tail() and
> > the inode's dirtied_when timestamp never gets old enough for the inode
> > to be eligible for periodic data writeback. Thus dirty data from it is
> > only ever written on explicit request (fsync(2), sync(2), unmount) or
> > on dirty limit pressure.
> >
> > The redirty_tail() logic in writeback_single_inode() actually should not
> > be necessary. Since this function is used only for once-off writeback,
> > there's no need for any livelock protection among inodes and
> > writeback_single_inode() is only responsible for demoting inode to "less
> > dirty" list based on dirty bits cleared as a result of writeback. So
> > remove the redirty_tail() logic and add a comment explaining the rules.
> >
> > Link: https://lore.kernel.org/all/20261023023707.263101-1-hongyu.jin@unisoc.com
> > Reported-by: Hongyu Jin <hongyu.jin.cn@gmail.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > fs/fs-writeback.c | 35 ++++++++++++++++++++++-------------
> > 1 file changed, 22 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index 58a6403780aa..de86c9055ffb 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -1946,22 +1946,31 @@ static int writeback_single_inode(struct inode *inode,
> > * as it can be finally deleted at this moment.
> > */
> > if (!(inode_state_read(inode) & I_FREEING)) {
> > + enum inode_state_flags_enum istate = inode_state_read(inode);
> > +
> > /*
> > - * If the inode is now fully clean, then it can be safely
> > - * removed from its writeback list (if any). Otherwise the
> > - * flusher threads are responsible for the writeback lists.
> > + * __mark_inode_dirty() is responsible for promoting inode to
> > + * the proper list (b_attached -> b_dirty_time -> b_dirty).
> > + * Here we are responsible for demoting inode after some dirty
> > + * bits were cleared.
> > + *
> > + * If the inode is now fully clean, then it can be safely moved
> > + * to b_attached list. If the inode is dirty, we don't want to
> > + * touch it - it should be already put to the dirty list by
> > + * __mark_inode_dirty() and we'd just unnecessarily clobber the
> > + * dirtied_when stamp (which is particularly problematic when
> > + * we were called from sync_inode_metadata()). We also cannot
> > + * touch the dirty list association when I_SYNC_QUEUED is set
> > + * and flush worker is responsible for it. Otherwise move the
> > + * inode to b_dirty_time list as that's the only remaining
> > + * possibility.
> > */
> > - if (!(inode_state_read(inode) & I_DIRTY_ALL))
> > + if (!(istate & I_DIRTY_ALL))
> > inode_cgwb_move_to_attached(inode, wb);
> > - else if (!(inode_state_read(inode) & I_SYNC_QUEUED)) {
> > - if ((inode_state_read(inode) & I_DIRTY))
> > - redirty_tail_locked(inode, wb);
> > - else if (inode_state_read(inode) & I_DIRTY_TIME) {
> > - inode->dirtied_when = jiffies;
> > - inode_io_list_move_locked(inode,
> > - wb,
> > - &wb->b_dirty_time);
> > - }
> > + else if (!(istate & I_SYNC_QUEUED) && !(istate & I_DIRTY)) {
> > + WARN_ON_ONCE(!(istate & I_DIRTY_TIME));
> > + inode->dirtied_when = jiffies;
> > + inode_io_list_move_locked(inode, wb, &wb->b_dirty_time);
> > }
> > }
> >
> > --
> > 2.51.0
> >
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-27 12:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-26 21:10 [PATCH] writeback: Avoid postponing periodic data writeback indefinitely Jan Kara
2026-10-27 6:08 ` Hongyu Jin
2026-10-27 12:16 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox