Re: [PATCH 1/9] btrfs: don't stop integrity writeback too early

[Boris Burkov <boris@bur.io>]

On Mon, Jul 24, 2023 at 06:26:53AM -0700, Christoph Hellwig wrote:
> extent_write_cache_pages stops writing pages as soon as nr_to_write hits
> zero.  That is the right thing for opportunistic writeback, but incorrect
> for data integrity writeback, which needs to ensure that no dirty pages
> are left in the range.  Thus only stop the writeback for WB_SYNC_NONE
> if nr_to_write hits 0.
> 
> This is a port of write_cache_pages changes in commit 05fe478dd04e
> ("mm: write_cache_pages integrity fix").

This makes sense to me. What is the reason the same reasoning doesn't
apply to btree_write_cache_pages? Does the issue only happen in practice
with fsync that no one is doing on the btree inode? It feels, in theory,
we could do a writepages with SYNC_ALL and hit the same issue with pages
going dirty and stealing the nr_writes.

> 
> Note that I've only trigger the problem with other changes to the btrfs
> writeback code, but this condition seems worthwhile fixing anyway.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/extent_io.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index c0440a0988c9a8..231e620e6c497d 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2098,7 +2098,8 @@ static int extent_write_cache_pages(struct address_space *mapping,
>  			 * We have to make sure to honor the new nr_to_write
>  			 * at any time
>  			 */
> -			nr_to_write_done = wbc->nr_to_write <= 0;
> +			nr_to_write_done = wbc->sync_mode == WB_SYNC_NONE &&
> +						wbc->nr_to_write <= 0;
>  		}
>  		folio_batch_release(&fbatch);
>  		cond_resched();
> -- 
> 2.39.2
>