On Wed, Sep 11, 2019 at 06:02:58PM +0100, Filipe Manana wrote:
> On Wed, Sep 11, 2019 at 5:54 PM Dennis Zhou <dennis@xxxxxxxxxx> wrote:
> >
> > On Wed, Sep 11, 2019 at 05:13:15PM +0100, Filipe Manana wrote:
> > > On Wed, Sep 11, 2019 at 5:04 PM Chris Mason <clm@xxxxxx> wrote:
> > > >
> > > > On 11 Sep 2019, at 15:55, fdmanana@xxxxxxxxxx wrote:
> > > >
> > > > > From: Filipe Manana <fdmanana@xxxxxxxx>
> > > > >
> > > > > So fix this by not overwriting the return value (ret) with the result
> > > > > from flush_write_bio(). We also need to clear the
> > > > > EXTENT_BUFFER_WRITEBACK
> > > > > bit in case flush_write_bio() returns an error, otherwise it will hang
> > > > > any future attempts to writeback the extent buffer.
> > > > >
> > > > > This is a regression introduced in the 5.2 kernel.
> > > > >
> > > > > Fixes: 2e3c25136adfb ("btrfs: extent_io: add proper error handling to
> > > > > lock_extent_buffer_for_io()")
> > > > > Fixes: f4340622e0226 ("btrfs: extent_io: Move the BUG_ON() in
> > > > > flush_write_bio() one level up")
> > > > > Reported-by: Zdenek Sojka <zsojka@xxxxxxxxx>
> > > > > Link:
> > > > > https://lore.kernel.org/linux-btrfs/GpO.2yos.3WGDOLpx6t%7D.1TUDYM@xxxxxxxxx/T/#u
> > > > > Reported-by: Stefan Priebe - Profihost AG <s.priebe@xxxxxxxxxxxx>
> > > > > Link:
> > > > > https://lore.kernel.org/linux-btrfs/5c4688ac-10a7-fb07-70e8-c5d31a3fbb38@xxxxxxxxxxxx/T/#t
> > > > > Reported-by: Drazen Kacar <drazen.kacar@xxxxxxxxxxx>
> > > > > Link:
> > > > > https://lore.kernel.org/linux-btrfs/DB8PR03MB562876ECE2319B3E579590F799C80@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> > > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204377
> > > > > Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
> > > > > ---
> > > > > fs/btrfs/extent_io.c | 23 ++++++++++++++---------
> > > > > 1 file changed, 14 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > > > > index 1ff438fd5bc2..1311ba0fc031 100644
> > > > > --- a/fs/btrfs/extent_io.c
> > > > > +++ b/fs/btrfs/extent_io.c
> > > > > @@ -3628,6 +3628,13 @@ void wait_on_extent_buffer_writeback(struct
> > > > > extent_buffer *eb)
> > > > > TASK_UNINTERRUPTIBLE);
> > > > > }
> > > > >
> > > > > +static void end_extent_buffer_writeback(struct extent_buffer *eb)
> > > > > +{
> > > > > + clear_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
> > > > > + smp_mb__after_atomic();
> > > > > + wake_up_bit(&eb->bflags, EXTENT_BUFFER_WRITEBACK);
> > > > > +}
> > > > > +
> > > > > /*
> > > > > * Lock eb pages and flush the bio if we can't the locks
> > > > > *
> > > > > @@ -3699,8 +3706,11 @@ static noinline_for_stack int
> > > > > lock_extent_buffer_for_io(struct extent_buffer *eb
> > > > >
> > > > > if (!trylock_page(p)) {
> > > > > if (!flush) {
> > > > > - ret = flush_write_bio(epd);
> > > > > - if (ret < 0) {
> > > > > + int err;
> > > > > +
> > > > > + err = flush_write_bio(epd);
> > > > > + if (err < 0) {
> > > > > + ret = err;
> > > > > failed_page_nr = i;
> > > > > goto err_unlock;
> > > > > }
> > > >
> > > >
> > > > Dennis (cc'd) has been trying a similar fix against this in production,
> > > > but sending it was interrupted by plumbing conferences. I think he
> > > > found that it needs to undo this as well:
> > > >
> > > > percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
> > > > -eb->len,
> > > > fs_info->dirty_metadata_batch);
> > > >
> > > > With the IO errors, we should end up aborting the FS. This function
> > > > also flips the the extent buffer written and dirty flags, and his patch
> > > > resets them as well. Given that we're aborting anyway, it's not
> > > > critical, but it's probably a good idea to fix things up in the goto
> > > > err_unlock just to make future bugs less likely.
> > >
> > > Yes, I considered that at some point (undo everything done so far,
> > > under the locks, etc) and thought it was pointless as well because we
> > > abort.
> > >
> > > But we may not abort - if we never start the writeback for an eb
> > > because we returned error from flush_write_bio(), we can leave
> > > btree_write_cache_pages() without noticing the error.
> > > Since writeback never started, and btree_write_cache_pages() didn't
> > > return the error, the transaction commit path may never get an error
> > > from filemap_fdatawrite_range,
> > > and we can commit the transaction despite failure to start writeback
> > > for some extent buffer.
> > >
> > > A problem that existed before that regression in 5.2 anyway. Sending
> > > it as separate.
> > >
> > > I'll include the undo of all operations in patch however, it doesn't
> > > hurt for sure.
> > >
> > > Thanks.
> > >
> > > >
> > > > -chris
> >
> > Hello,
> >
> > I should have pushed this upstream sooner, I was hoping to have one of
> > my test hosts hit my WARN_ON() in a separate patch, but it hasn't.
> >
> > The following is what I have to unblock 5.2 testing.
> >
> > I think your patch is missing resetting the header bits + the percpu
> > metadata counter.
>
> Yes, even though it's not a problem since we will end up aborting the
> transaction later.
> The v2 I sent some minutes ago does it:
> https://patchwork.kernel.org/patch/11141559/
>
I saw it as I refreshed the page after I sent my email :).
> > I think on error we should break out of
> > btree_write_cache_pages() and return it too.
>
> Yes, but that's a separate change.
> It makes the code clear but it doesn't fix any problem, the errors
> will be marked in the end io callback and transaction and log commits
> will see them and abort.
> Sent a patch for that as well some minutes ago:
> https://patchwork.kernel.org/patch/11141561/
>
> >
> > Thanks,
> > Dennis
> >
> > -----
> > From 1a57b5ee6e52c63bf7c8e3ae969c0df406e3cf69 Mon Sep 17 00:00:00 2001
> > From: Dennis Zhou <dennis@xxxxxxxxxx>
> > Date: Wed, 4 Sep 2019 10:49:53 -0700
> > Subject: [PATCH] btrfs: fix stall on writeback bit extent buffer
> >
> > In lock_extent_buffer_for_io(), if we encounter a blocking action, we
> > try and flush the currently held onto bio. The failure mode here used to
> > be a BUG_ON(). f4340622e022 changed this to move BUG_ON() up and
> > incorrectly reset the current ret code. However,
> > lock_extent_buffer_for_io() returns 1 on we should write out the pages.
> > This caused the buffer to be skipped while keeping the writeback bit
> > set.
> >
> > Now that we can fail here, we also need to fix up dirty_metadata_bytes,
> > clear BTRFS_HEADER_FLAG_WRITTEN and EXTENT_BUFFER_WRITEBACK, and set
> > EXTENT_BUFFER_DIRTY again.
> >
> > Fixes: f4340622e022 ("btrfs: extent_io: Move the BUG_ON() in flush_write_bio() one level up")
> > Signed-off-by: Dennis Zhou <dennis@xxxxxxxxxx>
> > ---
> > fs/btrfs/extent_io.c | 52 ++++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 43 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 43af8245c06e..4ba3cd972a2a 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -3636,6 +3636,13 @@ void wait_on_extent_buffer_writeback(struct extent_buffer *eb)
> > TASK_UNINTERRUPTIBLE);
> > }
> >
> > +static void end_extent_buffer_writeback(struct extent_buffer *eb)
> > +{
> > + clear_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
> > + smp_mb__after_atomic();
> > + wake_up_bit(&eb->bflags, EXTENT_BUFFER_WRITEBACK);
> > +}
> > +
> > /*
> > * Lock eb pages and flush the bio if we can't the locks
> > *
> > @@ -3707,9 +3714,11 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
> >
> > if (!trylock_page(p)) {
> > if (!flush) {
> > - ret = flush_write_bio(epd);
> > - if (ret < 0) {
> > + int flush_ret = flush_write_bio(epd);
> > +
> > + if (flush_ret < 0) {
> > failed_page_nr = i;
> > + ret = flush_ret;
> > goto err_unlock;
> > }
> > flush = 1;
> > @@ -3723,24 +3732,45 @@ static noinline_for_stack int lock_extent_buffer_for_io(struct extent_buffer *eb
> > /* Unlock already locked pages */
> > for (i = 0; i < failed_page_nr; i++)
> > unlock_page(eb->pages[i]);
> > - return ret;
> > -}
> >
> > -static void end_extent_buffer_writeback(struct extent_buffer *eb)
> > -{
> > - clear_bit(EXTENT_BUFFER_WRITEBACK, &eb->bflags);
> > - smp_mb__after_atomic();
> > - wake_up_bit(&eb->bflags, EXTENT_BUFFER_WRITEBACK);
> > + /* undo the above above because we failed */
> > + btrfs_tree_lock(eb);
> > +
> > + percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
> > + eb->len,
> > + fs_info->dirty_metadata_batch);
> > +
> > + btrfs_clear_header_flag(eb, BTRFS_HEADER_FLAG_WRITTEN);
> > +
> > + spin_lock(&eb->refs_lock);
> > + set_bit(EXTENT_BUFFER_DIRTY, &eb->bflags);
> > + spin_unlock(&eb->refs_lock);
>
> Clearing the writeback bit should also be done while holding eb->refs_lock.
> Everything else is equivalent to what I sent in v2.
>
> > +
> > + btrfs_tree_unlock(eb);
> > +
> > + end_extent_buffer_writeback(eb);
> > +
> > + return ret;
> > }
> >
> > 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;
> >
> > + /*
> > + * We just marked the extent as bad, that means we need retry
> > + * in the future, so fix up the dirty_metadata_bytes accounting.
> > + */
> > + fs_info = eb->fs_info;
> > + percpu_counter_add_batch(&fs_info->dirty_metadata_bytes,
> > + eb->len,
> > + fs_info->dirty_metadata_batch);
> > +
>
> This is a separate change from the 5.2 regression. Should be separate
> patch with a specific changelog IMHO.
> Can you please submit that?
>
Will do.
> > /*
> > * If writeback for a btree extent that doesn't belong to a log tree
> > * failed, increment the counter transaction->eb_write_errors.
> > @@ -3977,6 +4007,10 @@ int btree_write_cache_pages(struct address_space *mapping,
> > if (!ret) {
> > free_extent_buffer(eb);
> > continue;
> > + } else if (ret < 0) {
> > + done = 1;
> > + free_extent_buffer(eb);
> > + break;
> > }
>
> Separate change as well, not cause by the 5.2 regression. So separate
> patch with a specific changelog as well IMHO.
> Anyway, this hunk is exactly like the separate patch I sent some minutes ago.
>
> Thanks!
>
> >
> > ret = write_one_eb(eb, wbc, &epd);
> > --
> > 2.17.1
> >
Thanks,
Dennis
