[v1] Fixes and cleanups to fs-writeback

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

Posted by Kemeng Shi 2 years ago

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) {
 			spin_unlock(&wb->list_lock);
 			break;
 		}
-- 
2.30.0

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

Posted by Jan Kara 1 year, 11 months ago

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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

Posted by Tim Chen 2 years ago

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;
>  		}

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

Posted by Kemeng Shi 1 year, 11 months ago


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;
>>  		}
>

[PATCH 1/7] fs/writeback: avoid to writeback non-expired inode in kupdate writeback
[PATCH 2/7] fs/writeback: bail out if there is no more inodes for IO and queued once
[PATCH 3/7] fs/writeback: remove unused parameter wb of finish_writeback_work
[PATCH 4/7] fs/writeback: remove unneeded check in writeback_single_inode
[PATCH 5/7] fs/writeback: only calculate dirtied_before when b_io is empty
[PATCH 6/7] fs/writeback: correct comment of __wakeup_flusher_threads_bdi
[PATCH 7/7] fs/writeback: remove unnecessary return in writeback_inodes_sb