2024-02-08 09:26:15

by Kemeng Shi

Subject: [PATCH 2/7] fs/writeback: bail out if there is no more inodes for IO and queued once

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 <[email protected]>
---
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

2024-02-08 22:22:43

by Tim Chen

Subject: Re: [PATCH 2/7] fs/writeback: bail out if there is no more inodes for IO and queued once

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 <[email protected]>
> ---
> 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;
> }

2024-02-18 02:11:55

by Kemeng Shi

Subject: Re: [PATCH 2/7] fs/writeback: bail out if there is no more inodes for IO and queued once

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 <[email protected]>
>> ---
>> 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;
>> }
>

2024-02-23 13:56:59

by Jan Kara

Subject: Re: [PATCH 2/7] fs/writeback: bail out if there is no more inodes for IO and queued once

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 <[email protected]>

Makes sense. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

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
>
--
Jan Kara <[email protected]>
SUSE Labs, CR