* [PATCH] btrfs: add a find_contiguous_extent_bit helper and use it for safe isize
@ 2020-02-12 18:38 Josef Bacik
2020-02-12 18:44 ` Filipe Manana
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Josef Bacik @ 2020-02-12 18:38 UTC (permalink / raw)
To: linux-btrfs, kernel-team
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: add a find_contiguous_extent_bit helper and use it for safe isize
2020-02-12 18:38 [PATCH] btrfs: add a find_contiguous_extent_bit helper and use it for safe isize Josef Bacik
@ 2020-02-12 18:44 ` Filipe Manana
2020-02-12 19:44 ` Josef Bacik
2020-02-13 10:00 ` Johannes Thumshirn
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Filipe Manana @ 2020-02-12 18:44 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team
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.”
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: add a find_contiguous_extent_bit helper and use it for safe isize
2020-02-12 18:44 ` Filipe Manana
@ 2020-02-12 19:44 ` Josef Bacik
2020-02-13 10:21 ` Filipe Manana
0 siblings, 1 reply; 7+ messages in thread
From: Josef Bacik @ 2020-02-12 19:44 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs, kernel-team
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: add a find_contiguous_extent_bit helper and use it for safe isize
2020-02-12 18:38 [PATCH] btrfs: add a find_contiguous_extent_bit helper and use it for safe isize Josef Bacik
2020-02-12 18:44 ` Filipe Manana
@ 2020-02-13 10:00 ` Johannes Thumshirn
2020-02-13 10:22 ` Filipe Manana
2020-02-14 21:17 ` David Sterba
3 siblings, 0 replies; 7+ messages in thread
From: Johannes Thumshirn @ 2020-02-13 10:00 UTC (permalink / raw)
To: Josef Bacik, linux-btrfs, kernel-team
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: add a find_contiguous_extent_bit helper and use it for safe isize
2020-02-12 19:44 ` Josef Bacik
@ 2020-02-13 10:21 ` Filipe Manana
0 siblings, 0 replies; 7+ messages in thread
From: Filipe Manana @ 2020-02-13 10:21 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team
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.”
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: add a find_contiguous_extent_bit helper and use it for safe isize
2020-02-12 18:38 [PATCH] btrfs: add a find_contiguous_extent_bit helper and use it for safe isize Josef Bacik
2020-02-12 18:44 ` Filipe Manana
2020-02-13 10:00 ` Johannes Thumshirn
@ 2020-02-13 10:22 ` Filipe Manana
2020-02-14 21:17 ` David Sterba
3 siblings, 0 replies; 7+ messages in thread
From: Filipe Manana @ 2020-02-13 10:22 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team
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.”
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: add a find_contiguous_extent_bit helper and use it for safe isize
2020-02-12 18:38 [PATCH] btrfs: add a find_contiguous_extent_bit helper and use it for safe isize Josef Bacik
` (2 preceding siblings ...)
2020-02-13 10:22 ` Filipe Manana
@ 2020-02-14 21:17 ` David Sterba
3 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2020-02-14 21:17 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-02-14 21:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-12 18:38 [PATCH] btrfs: add a find_contiguous_extent_bit helper and use it for safe isize Josef Bacik
2020-02-12 18:44 ` Filipe Manana
2020-02-12 19:44 ` Josef Bacik
2020-02-13 10:21 ` Filipe Manana
2020-02-13 10:00 ` Johannes Thumshirn
2020-02-13 10:22 ` Filipe Manana
2020-02-14 21:17 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).