Re: [PATCH btrfs/for-next] btrfs: fix fatal extent_buffer readahead vs releasepage race

[Chris Mason]

On 17 Jun 2020, at 13:20, Filipe Manana wrote:

> On Wed, Jun 17, 2020 at 5:32 PM Boris Burkov <boris@xxxxxx> wrote:
>
> > ---
> >  fs/btrfs/extent_io.c | 45 
> > ++++++++++++++++++++++++++++----------------
> >  1 file changed, 29 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index c59e07360083..f6758ebbb6a2 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -3927,6 +3927,11 @@ static noinline_for_stack int 
> > write_one_eb(struct extent_buffer *eb,
> >         clear_bit(EXTENT_BUFFER_WRITE_ERR, &eb->bflags);
> >         num_pages = num_extent_pages(eb);
> >         atomic_set(&eb->io_pages, num_pages);
> > +       /*
> > +        * It is possible for releasepage to clear the TREE_REF bit 
> > before we
> > +        * set io_pages. See check_buffer_tree_ref for a more 
> > detailed comment.
> > +        */
> > +       check_buffer_tree_ref(eb);
>
> This is a whole different case from the one described in the
> changelog, as this is in the write path.
> Why do we need this one?

This was Josefâs idea, but I really like the symmetry.  You set 
io_pages, you do the tree_ref dance.  Everyone fiddling with the write 
back bit right now correctly clears writeback after doing the atomic_dec 
on io_pages, but the race is tiny and prone to getting exposed again by 
shifting code around.  Tree ref checks around io_pages are the most 
reliable way to prevent this bug from coming back again later.

-chris