* [PATCH] Btrfs-progs: fix infinite loop in find_free_extent
@ 2017-06-24 4:28 Liu Bo
2017-06-26 14:09 ` David Sterba
0 siblings, 1 reply; 4+ messages in thread
From: Liu Bo @ 2017-06-24 4:28 UTC (permalink / raw)
To: linux-btrfs; +Cc: Liu Bo
From: Liu Bo <boliu@localhost.localdomain>
%search_start is calculated in a wrong way, and if %ins is a cross-stripe
one, it'll search the same block group forever.
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
extent-tree.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/extent-tree.c b/extent-tree.c
index b12ee29..5e09274 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -2614,8 +2614,9 @@ check_failed:
goto no_bg_cache;
bg_offset = ins->objectid - bg_cache->key.objectid;
- search_start = round_up(bg_offset + num_bytes,
- BTRFS_STRIPE_LEN) + bg_offset;
+ search_start = round_up(
+ bg_offset + num_bytes, BTRFS_STRIPE_LEN) +
+ bg_cache->key.object;
goto new_group;
}
no_bg_cache:
--
2.9.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Btrfs-progs: fix infinite loop in find_free_extent
2017-06-24 4:28 [PATCH] Btrfs-progs: fix infinite loop in find_free_extent Liu Bo
@ 2017-06-26 14:09 ` David Sterba
2017-06-26 18:02 ` Liu Bo
0 siblings, 1 reply; 4+ messages in thread
From: David Sterba @ 2017-06-26 14:09 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-btrfs, Liu Bo
On Fri, Jun 23, 2017 at 10:28:31PM -0600, Liu Bo wrote:
> From: Liu Bo <boliu@localhost.localdomain>
>
> %search_start is calculated in a wrong way, and if %ins is a cross-stripe
> one, it'll search the same block group forever.
That's a bit terse description, so please check if my understanding is right:
search_start advances by at least one stripe len, but the math would be wrong
as using bg_offset would not move us to the next stripe. bg_cache->key.objectid
is the full length so this will reach the next stripe and will not loop forever.
Do you happen to have a test for that?
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
> extent-tree.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/extent-tree.c b/extent-tree.c
> index b12ee29..5e09274 100644
> --- a/extent-tree.c
> +++ b/extent-tree.c
> @@ -2614,8 +2614,9 @@ check_failed:
> goto no_bg_cache;
> bg_offset = ins->objectid - bg_cache->key.objectid;
>
> - search_start = round_up(bg_offset + num_bytes,
> - BTRFS_STRIPE_LEN) + bg_offset;
> + search_start = round_up(
> + bg_offset + num_bytes, BTRFS_STRIPE_LEN) +
> + bg_cache->key.object;
extent-tree.c: In function ‘find_free_extent’:
extent-tree.c:2617:18: error: ‘struct btrfs_key’ has no member named ‘object’; did you mean ‘objectid’?
bg_cache->key.object;
^
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Btrfs-progs: fix infinite loop in find_free_extent
2017-06-26 14:09 ` David Sterba
@ 2017-06-26 18:02 ` Liu Bo
2017-06-27 2:39 ` Qu Wenruo
0 siblings, 1 reply; 4+ messages in thread
From: Liu Bo @ 2017-06-26 18:02 UTC (permalink / raw)
To: dsterba, linux-btrfs, Liu Bo
On Mon, Jun 26, 2017 at 04:09:53PM +0200, David Sterba wrote:
> On Fri, Jun 23, 2017 at 10:28:31PM -0600, Liu Bo wrote:
> > From: Liu Bo <boliu@localhost.localdomain>
Ah, my From was broken again.
> >
> > %search_start is calculated in a wrong way, and if %ins is a cross-stripe
> > one, it'll search the same block group forever.
>
> That's a bit terse description, so please check if my understanding is right:
> search_start advances by at least one stripe len, but the math would be wrong
> as using bg_offset would not move us to the next stripe. bg_cache->key.objectid
> is the full length so this will reach the next stripe and will not loop forever.
Yes, it's correct, the code's logic is like, now that the returned %ins is a
cross-stripe one, it then calculates a BTRFS_STRIPE_LEN aligned one as the new
%search_start and see if there is any free block matching %search_start. The
current code is using a wrong offset, the offset really should be the start
position of a block group.
>
> Do you happen to have a test for that?
Unfortunately it's not a test with vanilla progs.
I found this when mkfs.btrfs with a 12K nodesize, but now kernel has a
power_of_2 limitation for nodesize and progs code is using a weird IS_ALIGNED()
which has the same effect with power_of_2(), mkfs.btrfs -n 12K is not allowed.
I changed IS_ALIGNED() to (blocksize % nodesize != 0) and got the above loop.
>
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> > extent-tree.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/extent-tree.c b/extent-tree.c
> > index b12ee29..5e09274 100644
> > --- a/extent-tree.c
> > +++ b/extent-tree.c
> > @@ -2614,8 +2614,9 @@ check_failed:
> > goto no_bg_cache;
> > bg_offset = ins->objectid - bg_cache->key.objectid;
> >
> > - search_start = round_up(bg_offset + num_bytes,
> > - BTRFS_STRIPE_LEN) + bg_offset;
> > + search_start = round_up(
> > + bg_offset + num_bytes, BTRFS_STRIPE_LEN) +
> > + bg_cache->key.object;
>
> extent-tree.c: In function ‘find_free_extent’:
> extent-tree.c:2617:18: error: ‘struct btrfs_key’ has no member named ‘object’; did you mean ‘objectid’?
> bg_cache->key.object;
> ^
Ouch, that's right, it's %objectid.
I'll send a updated one, thanks for the comments.
-liubo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Btrfs-progs: fix infinite loop in find_free_extent
2017-06-26 18:02 ` Liu Bo
@ 2017-06-27 2:39 ` Qu Wenruo
0 siblings, 0 replies; 4+ messages in thread
From: Qu Wenruo @ 2017-06-27 2:39 UTC (permalink / raw)
To: bo.li.liu, dsterba, linux-btrfs, Liu Bo
At 06/27/2017 02:02 AM, Liu Bo wrote:
> On Mon, Jun 26, 2017 at 04:09:53PM +0200, David Sterba wrote:
>> On Fri, Jun 23, 2017 at 10:28:31PM -0600, Liu Bo wrote:
>>> From: Liu Bo <boliu@localhost.localdomain>
>
> Ah, my From was broken again.
>
>>>
>>> %search_start is calculated in a wrong way, and if %ins is a cross-stripe
>>> one, it'll search the same block group forever.
>>
>> That's a bit terse description, so please check if my understanding is right:
>> search_start advances by at least one stripe len, but the math would be wrong
>> as using bg_offset would not move us to the next stripe. bg_cache->key.objectid
>> is the full length so this will reach the next stripe and will not loop forever.
>
> Yes, it's correct, the code's logic is like, now that the returned %ins is a
> cross-stripe one, it then calculates a BTRFS_STRIPE_LEN aligned one as the new
> %search_start and see if there is any free block matching %search_start. The
> current code is using a wrong offset, the offset really should be the start
> position of a block group.
>
>>
>> Do you happen to have a test for that?
>
> Unfortunately it's not a test with vanilla progs.
>
> I found this when mkfs.btrfs with a 12K nodesize, but now kernel has a
> power_of_2 limitation for nodesize and progs code is using a weird IS_ALIGNED()
Yes, btrfs_check_nodesize() is using (nodesize & (sectorsize - 1)) to
check if it's aligned, but it's only correct if sectorsize is power of 2.
It should also be fixed for btrfs-progs.
Thanks,
Qu
> which has the same effect with power_of_2(), mkfs.btrfs -n 12K is not allowed.
> I changed IS_ALIGNED() to (blocksize % nodesize != 0) and got the above loop.
>
>>
>>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>>> ---
>>> extent-tree.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/extent-tree.c b/extent-tree.c
>>> index b12ee29..5e09274 100644
>>> --- a/extent-tree.c
>>> +++ b/extent-tree.c
>>> @@ -2614,8 +2614,9 @@ check_failed:
>>> goto no_bg_cache;
>>> bg_offset = ins->objectid - bg_cache->key.objectid;
>>>
>>> - search_start = round_up(bg_offset + num_bytes,
>>> - BTRFS_STRIPE_LEN) + bg_offset;
>>> + search_start = round_up(
>>> + bg_offset + num_bytes, BTRFS_STRIPE_LEN) +
>>> + bg_cache->key.object;
>>
>> extent-tree.c: In function ‘find_free_extent’:
>> extent-tree.c:2617:18: error: ‘struct btrfs_key’ has no member named ‘object’; did you mean ‘objectid’?
>> bg_cache->key.object;
>> ^
>
> Ouch, that's right, it's %objectid.
>
> I'll send a updated one, thanks for the comments.
>
> -liubo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-27 2:39 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-24 4:28 [PATCH] Btrfs-progs: fix infinite loop in find_free_extent Liu Bo
2017-06-26 14:09 ` David Sterba
2017-06-26 18:02 ` Liu Bo
2017-06-27 2:39 ` Qu Wenruo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.