diff mbox series

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

Message ID 20240208172024.23625-3-shikemeng@huaweicloud.com
State New
Headers
Series Fixes and cleanups to fs-writeback |

Commit Message

Kemeng Shi Feb. 8, 2024, 5:20 p.m. UTC 
  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(-)

Comments

Tim Chen Feb. 8, 2024, 7:21 p.m. UTC | #1 
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;
>  		}
Kemeng Shi Feb. 18, 2024, 2:11 a.m. UTC | #2 
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;
>>  		}
>
Jan Kara Feb. 23, 2024, 1:49 p.m. UTC | #3 
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 mbox series

Patch

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