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; Thanks, Josef
