[PATCH] btrfs: extent_io read eb to dirty_metadata_bytes on ioerr

[PATCH] btrfs: extent_io read eb to dirty_metadata_bytes on ioerr

[Dennis Zhou]

Before, if a eb failed to write out, we would end up triggering a
BUG_ON(). As of f4340622e0226 ("btrfs: extent_io: Move the BUG_ON() in
flush_write_bio() one level up"), we no longer BUG_ON(), so we should
make life consistent and add back the unwritten bytes to
dirty_metadata_bytes.

Signed-off-by: Dennis Zhou <dennis@kernel.org>
Cc: Filipe Manana <fdmanana@kernel.org>
---
 fs/btrfs/extent_io.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 1ff438fd5bc2..b67133a23652 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3728,11 +3728,21 @@ static void end_extent_buffer_writeback(struct extent_buffer *eb)
 static void set_btree_ioerr(struct page *page)
 {
 	struct extent_buffer *eb = (struct extent_buffer *)page->private;
+	struct btrfs_fs_info *fs_info;
 
 	SetPageError(page);
 	if (test_and_set_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags))
 		return;
 
+	/*
+	 * If we error out, we should add back the dirty_metadata_bytes
+	 * to make it consistent.
+	 */
+	fs_info = eb->fs_info;
+	percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
+				 eb->len,
+				 fs_info->dirty_metadata_batch);
+
 	/*
 	 * If writeback for a btree extent that doesn't belong to a log tree
 	 * failed, increment the counter transaction->eb_write_errors.
-- 
2.17.1

Re: [PATCH] btrfs: extent_io read eb to dirty_metadata_bytes on ioerr

[Filipe Manana]

On Fri, Sep 13, 2019 at 2:54 PM Dennis Zhou <dennis@kernel.org> wrote:
>
> Before, if a eb failed to write out, we would end up triggering a
> BUG_ON(). As of f4340622e0226 ("btrfs: extent_io: Move the BUG_ON() in
> flush_write_bio() one level up"), we no longer BUG_ON(), so we should
> make life consistent and add back the unwritten bytes to
> dirty_metadata_bytes.
>
> Signed-off-by: Dennis Zhou <dennis@kernel.org>
> Cc: Filipe Manana <fdmanana@kernel.org>

Looks good.
However I find the subject very confusing and misleading.

"extent_io read eb to dirty_metadata_bytes on ioerr"

That gives the idea of reading the eb (like from disk? or its content,
reading from its pages?), and the "to dirty_metadata_bytes" also find
it confusing.
Something like:

 "btrfs: adjust dirty_metadata_bytes after writeback failure of extent buffer"

would make it clear and not confusing IMHO.
Perhaps it's something David can change when he picks the patch
(either to that or some other more clear subject).

Anyway, for the change itself,

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks!

> ---
>  fs/btrfs/extent_io.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 1ff438fd5bc2..b67133a23652 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3728,11 +3728,21 @@ static void end_extent_buffer_writeback(struct extent_buffer *eb)
>  static void set_btree_ioerr(struct page *page)
>  {
>         struct extent_buffer *eb = (struct extent_buffer *)page->private;
> +       struct btrfs_fs_info *fs_info;
>
>         SetPageError(page);
>         if (test_and_set_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags))
>                 return;
>
> +       /*
> +        * If we error out, we should add back the dirty_metadata_bytes
> +        * to make it consistent.
> +        */
> +       fs_info = eb->fs_info;
> +       percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
> +                                eb->len,
> +                                fs_info->dirty_metadata_batch);
> +
>         /*
>          * If writeback for a btree extent that doesn't belong to a log tree
>          * failed, increment the counter transaction->eb_write_errors.
> --
> 2.17.1
>

Re: [PATCH] btrfs: extent_io read eb to dirty_metadata_bytes on ioerr

[David Sterba]

On Fri, Sep 13, 2019 at 04:46:09PM +0100, Filipe Manana wrote:
> On Fri, Sep 13, 2019 at 2:54 PM Dennis Zhou <dennis@kernel.org> wrote:
> >
> > Before, if a eb failed to write out, we would end up triggering a
> > BUG_ON(). As of f4340622e0226 ("btrfs: extent_io: Move the BUG_ON() in
> > flush_write_bio() one level up"), we no longer BUG_ON(), so we should
> > make life consistent and add back the unwritten bytes to
> > dirty_metadata_bytes.
> >
> > Signed-off-by: Dennis Zhou <dennis@kernel.org>
> > Cc: Filipe Manana <fdmanana@kernel.org>
> 
> Looks good.
> However I find the subject very confusing and misleading.
> 
> "extent_io read eb to dirty_metadata_bytes on ioerr"
> 
> That gives the idea of reading the eb (like from disk? or its content,
> reading from its pages?), and the "to dirty_metadata_bytes" also find
> it confusing.
> Something like:
> 
>  "btrfs: adjust dirty_metadata_bytes after writeback failure of extent buffer"
> 
> would make it clear and not confusing IMHO.
> Perhaps it's something David can change when he picks the patch
> (either to that or some other more clear subject).

That's perfectly fine to suggest updates to subject or changelogs in
case you find them worth an improvement. Patch updated and queued for,
thanks.