On Wed, Feb 12, 2020 at 7:44 PM Josef Bacik <josef@xxxxxxxxxxxxxx> wrote:
>
> On 2/12/20 1:44 PM, Filipe Manana wrote:
> > On Wed, Feb 12, 2020 at 6:40 PM Josef Bacik <josef@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> >> ---
> >> 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.”
