Re: [PATCH] btrfs: add a find_contiguous_extent_bit helper and use it for safe isize

[Filipe Manana <fdmanana@gmail.com>]

On Wed, Feb 12, 2020 at 7:44 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> On 2/12/20 1:44 PM, Filipe Manana wrote:
> > On Wed, Feb 12, 2020 at 6:40 PM Josef Bacik <josef@toxicpanda.com> wrote:
> >>
> >> Filipe noticed a race where we would sometimes get the wrong answer for
> >> the i_disk_size for !NO_HOLES with my patch.  That is because I expected
> >> that find_first_extent_bit() would find the contiguous range, since I'm
> >> only ever setting EXTENT_DIRTY.  However the way set_extent_bit() works
> >> is it'll temporarily split the range, loop around and set our bits, and
> >> then merge the state.  When it loops it drops the tree->lock, so there
> >> is a window where we can have two adjacent states instead of one large
> >> state.  Fix this by walking forward until we find a non-contiguous
> >> state, and set our end_ret to the end of our logically contiguous area.
> >> This fixes the problem without relying on specific behavior from
> >> set_extent_bit().
> >>
> >> Fixes: 79ceff7f6e5d ("btrfs: introduce per-inode file extent tree")
> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> >> ---
> >> Dave, I assume you'll want to fold this in to the referenced patch, if not let
> >> me know and I'll rework the series to include this as a different patch.
> >>
> >>   fs/btrfs/extent-io-tree.h |  2 ++
> >>   fs/btrfs/extent_io.c      | 36 ++++++++++++++++++++++++++++++++++++
> >>   fs/btrfs/file-item.c      |  4 ++--
> >>   3 files changed, 40 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h
> >> index 16fd403447eb..cc3037f9765e 100644
> >> --- a/fs/btrfs/extent-io-tree.h
> >> +++ b/fs/btrfs/extent-io-tree.h
> >> @@ -223,6 +223,8 @@ int find_first_extent_bit(struct extent_io_tree *tree, u64 start,
> >>                            struct extent_state **cached_state);
> >>   void find_first_clear_extent_bit(struct extent_io_tree *tree, u64 start,
> >>                                   u64 *start_ret, u64 *end_ret, unsigned bits);
> >> +int find_contiguous_extent_bit(struct extent_io_tree *tree, u64 start,
> >> +                              u64 *start_ret, u64 *end_ret, unsigned bits);
> >>   int extent_invalidatepage(struct extent_io_tree *tree,
> >>                            struct page *page, unsigned long offset);
> >>   bool btrfs_find_delalloc_range(struct extent_io_tree *tree, u64 *start,
> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> >> index d91a48d73e8f..50bbaf1c7cf0 100644
> >> --- a/fs/btrfs/extent_io.c
> >> +++ b/fs/btrfs/extent_io.c
> >> @@ -1578,6 +1578,42 @@ int find_first_extent_bit(struct extent_io_tree *tree, u64 start,
> >>          return ret;
> >>   }
> >>
> >> +/**
> >> + * find_contiguous_extent_bit: find a contiguous area of bits
> >> + * @tree - io tree to check
> >> + * @start - offset to start the search from
> >> + * @start_ret - the first offset we found with the bits set
> >> + * @end_ret - the final contiguous range of the bits that were set
> >> + *
> >> + * set_extent_bit anc clear_extent_bit can temporarily split contiguous ranges
> >> + * to set bits appropriately, and then merge them again.  During this time it
> >> + * will drop the tree->lock, so use this helper if you want to find the actual
> >> + * contiguous area for given bits.  We will search to the first bit we find, and
> >> + * then walk down the tree until we find a non-contiguous area.  The area
> >> + * returned will be the full contiguous area with the bits set.
> >> + */
> >> +int find_contiguous_extent_bit(struct extent_io_tree *tree, u64 start,
> >> +                              u64 *start_ret, u64 *end_ret, unsigned bits)
> >> +{
> >> +       struct extent_state *state;
> >> +       int ret = 1;
> >> +
> >> +       spin_lock(&tree->lock);
> >> +       state = find_first_extent_bit_state(tree, start, bits);
> >> +       if (state) {
> >> +               *start_ret = state->start;
> >> +               *end_ret = state->end;
> >> +               while ((state = next_state(state)) != NULL) {
> >> +                       if (state->start > (*end_ret + 1))
> >> +                               break;
> >> +                       *end_ret = state->end;
> >> +               }
> >> +               ret = 0;
> >
> > So as long as the tree is not empty, we will always be returning 0
> > (success), right?
> > If we break from the while loop we should return with 1, but this
> > makes us return 0.
> >
>
> Yeah, that's the same behavior we have with find_first_extent_bit, that's why we do
>
> if (!ret && start == 0)
>         i_size = min(i_size, end + 1);
> else
>         i_size = 0;

Yes, never mind, evening confusion.
It's correct indeed, and it fixes the problem as well (even without my
optimization patch).

>
> Thanks,
>
> Josef



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”