| Message ID | 20240208172024.23625-3-shikemeng@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes and cleanups to fs-writeback | expand |
On Fri, 2024-02-09 at 01:20 +0800, Kemeng Shi wrote: > For case there is no more inodes for IO in io list from last wb_writeback, > We may bail out early even there is inode in dirty list should be written > back. Only bail out when we queued once to avoid missing dirtied inode. > > This is from code reading... > > Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> > --- > fs/fs-writeback.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index a9a918972719..edb0cff51673 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -2086,6 +2086,7 @@ static long wb_writeback(struct bdi_writeback *wb, > struct inode *inode; > long progress; > struct blk_plug plug; > + bool queued = false; > > if (work->for_kupdate) > filter_expired_io(wb); > @@ -2131,8 +2132,10 @@ static long wb_writeback(struct bdi_writeback *wb, > dirtied_before = jiffies; > > trace_writeback_start(wb, work); > - if (list_empty(&wb->b_io)) > + if (list_empty(&wb->b_io)) { > queue_io(wb, work, dirtied_before); > + queued = true; > + } > if (work->sb) > progress = writeback_sb_inodes(work->sb, wb, work); > else > @@ -2155,7 +2158,7 @@ static long wb_writeback(struct bdi_writeback *wb, > /* > * No more inodes for IO, bail > */ > - if (list_empty(&wb->b_more_io)) { > + if (list_empty(&wb->b_more_io) && queued) { Wonder if we can simply do if (list_empty(&wb->b_more_io) && list_empty(&wb->b_io)) { if the intention is to not bail if there are still inodes to be be flushed. Tim > spin_unlock(&wb->list_lock); > break; > }
on 2/9/2024 3:21 AM, Tim Chen wrote: > On Fri, 2024-02-09 at 01:20 +0800, Kemeng Shi wrote: >> For case there is no more inodes for IO in io list from last wb_writeback, >> We may bail out early even there is inode in dirty list should be written >> back. Only bail out when we queued once to avoid missing dirtied inode. >> >> This is from code reading... >> >> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> >> --- >> fs/fs-writeback.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c >> index a9a918972719..edb0cff51673 100644 >> --- a/fs/fs-writeback.c >> +++ b/fs/fs-writeback.c >> @@ -2086,6 +2086,7 @@ static long wb_writeback(struct bdi_writeback *wb, >> struct inode *inode; >> long progress; >> struct blk_plug plug; >> + bool queued = false; >> >> if (work->for_kupdate) >> filter_expired_io(wb); >> @@ -2131,8 +2132,10 @@ static long wb_writeback(struct bdi_writeback *wb, >> dirtied_before = jiffies; >> >> trace_writeback_start(wb, work); >> - if (list_empty(&wb->b_io)) >> + if (list_empty(&wb->b_io)) { >> queue_io(wb, work, dirtied_before); >> + queued = true; >> + } >> if (work->sb) >> progress = writeback_sb_inodes(work->sb, wb, work); >> else >> @@ -2155,7 +2158,7 @@ static long wb_writeback(struct bdi_writeback *wb, >> /* >> * No more inodes for IO, bail >> */ >> - if (list_empty(&wb->b_more_io)) { >> + if (list_empty(&wb->b_more_io) && queued) { > > Wonder if we can simply do > if (list_empty(&wb->b_more_io) && list_empty(&wb->b_io)) { > > if the intention is to not bail if there are still inodes to be be flushed. I suppose not as there may be inodes in wb->b_dirty should be flushed. For case that there is a inode in wb->b_io which is not flushed in last wb_writeback and there are a lot of inodes in wb->dirty, the next background flush is supposed to make dirty pages under threshold however only the inode in wb->b_io is flushed. > > Tim > >> spin_unlock(&wb->list_lock); >> break; >> } >
On Fri 09-02-24 01:20:19, Kemeng Shi wrote: > For case there is no more inodes for IO in io list from last wb_writeback, > We may bail out early even there is inode in dirty list should be written > back. Only bail out when we queued once to avoid missing dirtied inode. > > This is from code reading... > > Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> Makes sense. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > fs/fs-writeback.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c > index a9a918972719..edb0cff51673 100644 > --- a/fs/fs-writeback.c > +++ b/fs/fs-writeback.c > @@ -2086,6 +2086,7 @@ static long wb_writeback(struct bdi_writeback *wb, > struct inode *inode; > long progress; > struct blk_plug plug; > + bool queued = false; > > if (work->for_kupdate) > filter_expired_io(wb); > @@ -2131,8 +2132,10 @@ static long wb_writeback(struct bdi_writeback *wb, > dirtied_before = jiffies; > > trace_writeback_start(wb, work); > - if (list_empty(&wb->b_io)) > + if (list_empty(&wb->b_io)) { > queue_io(wb, work, dirtied_before); > + queued = true; > + } > if (work->sb) > progress = writeback_sb_inodes(work->sb, wb, work); > else > @@ -2155,7 +2158,7 @@ static long wb_writeback(struct bdi_writeback *wb, > /* > * No more inodes for IO, bail > */ > - if (list_empty(&wb->b_more_io)) { > + if (list_empty(&wb->b_more_io) && queued) { > spin_unlock(&wb->list_lock); > break; > } > -- > 2.30.0 >
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index a9a918972719..edb0cff51673 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -2086,6 +2086,7 @@ static long wb_writeback(struct bdi_writeback *wb, struct inode *inode; long progress; struct blk_plug plug; + bool queued = false; if (work->for_kupdate) filter_expired_io(wb); @@ -2131,8 +2132,10 @@ static long wb_writeback(struct bdi_writeback *wb, dirtied_before = jiffies; trace_writeback_start(wb, work); - if (list_empty(&wb->b_io)) + if (list_empty(&wb->b_io)) { queue_io(wb, work, dirtied_before); + queued = true; + } if (work->sb) progress = writeback_sb_inodes(work->sb, wb, work); else @@ -2155,7 +2158,7 @@ static long wb_writeback(struct bdi_writeback *wb, /* * No more inodes for IO, bail */ - if (list_empty(&wb->b_more_io)) { + if (list_empty(&wb->b_more_io) && queued) { spin_unlock(&wb->list_lock); break; }
For case there is no more inodes for IO in io list from last wb_writeback, We may bail out early even there is inode in dirty list should be written back. Only bail out when we queued once to avoid missing dirtied inode. This is from code reading... Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> --- fs/fs-writeback.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)