btrfs: extent_io read eb to dirty_metadata_bytes on ioerr
diff mbox series

Message ID 20190913135407.99353-1-dennis@kernel.org
State New
Headers show
Series
  • btrfs: extent_io read eb to dirty_metadata_bytes on ioerr
Related show

Commit Message

Dennis Zhou Sept. 13, 2019, 1:54 p.m. UTC
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(+)

Comments

Filipe Manana Sept. 13, 2019, 3:46 p.m. UTC | #1
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
>
David Sterba Sept. 23, 2019, 3:05 p.m. UTC | #2
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.

Patch
diff mbox series

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.