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.
thanks
> + }
> + spin_unlock(&tree->lock);
> + return ret;
> +}
> +
> /**
> * find_first_clear_extent_bit - find the first range that has @bits not set.
> * This range could start before @start.
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index a73878051761..6c849e8fd5a1 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -51,8 +51,8 @@ void btrfs_inode_safe_disk_i_size_write(struct inode *inode, u64 new_i_size)
> }
>
> spin_lock(&BTRFS_I(inode)->lock);
> - ret = find_first_extent_bit(&BTRFS_I(inode)->file_extent_tree, 0,
> - &start, &end, EXTENT_DIRTY, NULL);
> + ret = find_contiguous_extent_bit(&BTRFS_I(inode)->file_extent_tree, 0,
> + &start, &end, EXTENT_DIRTY);
> if (!ret && start == 0)
> i_size = min(i_size, end + 1);
> else
> --
> 2.24.1
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
