[Devel,vz7] fs: avoid writeback busy-loop if redirty

Submitted by Maxim Patlasov on Dec. 14, 2016, 6:48 p.m.

Details

Message ID 148174126980.5880.15768348160568706375.stgit@maxim-thinkpad
State New
Series "fs: avoid writeback busy-loop if redirty"
Headers show

Commit Message

Maxim Patlasov Dec. 14, 2016, 6:48 p.m.
Alexey,

Can you please review this one-liner below. I think it can
be responsible for inexplicable delays and CPU peaks in bugs like
https://jira.sw.ru/browse/PSBM-55919. Thanks, Maxim

wb_writeback() bails out only if either nr_pages comes to zero or
"progress" (as reported by writeback_sb_inodes or __writeback_inodes_wb)
is zero.

If a page is temporarily unavailable (is under fuse-writeback),
fuse_writepages_fill calls redirty_page_for_writepage, unlock the page
and returns OK. This OK results in progress=1. So wb_writeback
loops over writeback_sb_inodes/__writeback_inodes_wb again and again
until all nr_pages are exhausted.

The patch ensures that progress=0 if fuse_writepages_fill did nothing.

Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
---
 fs/fs-writeback.c |    3 +++
 1 file changed, 3 insertions(+)

Patch hide | download patch | download mbox

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 5c9a082..483bbb9 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -774,6 +774,9 @@  static long writeback_sb_inodes(struct super_block *sb,
 			if (work->nr_pages <= 0)
 				break;
 		}
+
+		BUG_ON(wbc.pages_skipped > write_chunk - wbc.nr_to_write);
+		wrote -= wbc.pages_skipped;
 	}
 	return wrote;
 }

Comments

Konstantin Khorenko Dec. 15, 2016, 11:57 a.m.
BUG_ON() again.

Ok, let's assume it's WARN_ON() there. :)

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 12/14/2016 09:48 PM, Maxim Patlasov wrote:
> Alexey,
>
> Can you please review this one-liner below. I think it can
> be responsible for inexplicable delays and CPU peaks in bugs like
> https://jira.sw.ru/browse/PSBM-55919. Thanks, Maxim
>
> wb_writeback() bails out only if either nr_pages comes to zero or
> "progress" (as reported by writeback_sb_inodes or __writeback_inodes_wb)
> is zero.
>
> If a page is temporarily unavailable (is under fuse-writeback),
> fuse_writepages_fill calls redirty_page_for_writepage, unlock the page
> and returns OK. This OK results in progress=1. So wb_writeback
> loops over writeback_sb_inodes/__writeback_inodes_wb again and again
> until all nr_pages are exhausted.
>
> The patch ensures that progress=0 if fuse_writepages_fill did nothing.
>
> Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
> ---
>  fs/fs-writeback.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 5c9a082..483bbb9 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -774,6 +774,9 @@ static long writeback_sb_inodes(struct super_block *sb,
>  			if (work->nr_pages <= 0)
>  				break;
>  		}
> +
> +		BUG_ON(wbc.pages_skipped > write_chunk - wbc.nr_to_write);
> +		wrote -= wbc.pages_skipped;
>  	}
>  	return wrote;
>  }
>
> .
>
Maxim Patlasov Dec. 15, 2016, 8:32 p.m.
I think BUG_ON is better than WARN_ON in that particular place because: 
1) we won't hit it unless someone (in future) will introduce a bug 
returning error (instead of OK) after redirty_page_for_writepage; 2) if 
we hit WARN_ON, it will be very difficult to investigate root cause 
(much harder than having a crash dump after WARN_ON).


On 12/15/2016 03:57 AM, Konstantin Khorenko wrote:
> BUG_ON() again.
>
> Ok, let's assume it's WARN_ON() there. :)
>
> -- 
> Best regards,
>
> Konstantin Khorenko,
> Virtuozzo Linux Kernel Team
>
> On 12/14/2016 09:48 PM, Maxim Patlasov wrote:
>> Alexey,
>>
>> Can you please review this one-liner below. I think it can
>> be responsible for inexplicable delays and CPU peaks in bugs like
>> https://jira.sw.ru/browse/PSBM-55919. Thanks, Maxim
>>
>> wb_writeback() bails out only if either nr_pages comes to zero or
>> "progress" (as reported by writeback_sb_inodes or __writeback_inodes_wb)
>> is zero.
>>
>> If a page is temporarily unavailable (is under fuse-writeback),
>> fuse_writepages_fill calls redirty_page_for_writepage, unlock the page
>> and returns OK. This OK results in progress=1. So wb_writeback
>> loops over writeback_sb_inodes/__writeback_inodes_wb again and again
>> until all nr_pages are exhausted.
>>
>> The patch ensures that progress=0 if fuse_writepages_fill did nothing.
>>
>> Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
>> ---
>>  fs/fs-writeback.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
>> index 5c9a082..483bbb9 100644
>> --- a/fs/fs-writeback.c
>> +++ b/fs/fs-writeback.c
>> @@ -774,6 +774,9 @@ static long writeback_sb_inodes(struct 
>> super_block *sb,
>>              if (work->nr_pages <= 0)
>>                  break;
>>          }
>> +
>> +        BUG_ON(wbc.pages_skipped > write_chunk - wbc.nr_to_write);
>> +        wrote -= wbc.pages_skipped;
>>      }
>>      return wrote;
>>  }
>>
>> .
>>