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

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

[Josef Bacik]

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;
+	}
+	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

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

[Filipe Manana]

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.

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.”

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

[Josef Bacik]

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;

Thanks,

Josef

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

[Filipe Manana]

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.”

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

[Johannes Thumshirn]

On 12/02/2020 19:38, Josef Bacik wrote:
[...]
> +/**
> + * 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
> + *

Nit: The above it missing @bits

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

[Filipe Manana]

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>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Alright, survived overnight tests (without my optimization patch), so
it's all good.
Thanks.

> ---
> 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;
> +       }
> +       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.”

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

[David Sterba]

On Wed, Feb 12, 2020 at 01:38:31PM -0500, Josef Bacik 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.

Folding looks like a good option, the patch adds other helper for
btrfs_inode_clear_file_extent_range so this patch fits in nicely.

> +/**
> + * 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.
> + */

This summarizes why it's needed so the changelog does not need to be
updated AFAICS. Patch updated and misc-next pushed. Thanks.