[PATCH] Btrfs: fix find_free_dev_extent() malfunction in case device tree has hole

[PATCH] Btrfs: fix find_free_dev_extent() malfunction in case device tree has hole

[Forrest Liu]

If device tree has hole, find_free_dev_extent() cannot find available
address properly.

The example below, has one BIG hole in device tree, and can only
allocate just one chunk in a transaction.

    item 9 key (1 DEV_EXTENT 273841913856) itemoff 15811 itemsize 48
        dev extent chunk_tree 3
        chunk objectid 256 chunk offset 272759783424 length 1073741824
    item 10 key (1 DEV_EXTENT 1071632089088) itemoff 15763 itemsize 48
        dev extent chunk_tree 3
        chunk objectid 256 chunk offset 1070549958656 length 1073741824
    item 11 key (1 DEV_EXTENT 1072705830912) itemoff 15715 itemsize 48
            dev extent chunk_tree 3
            chunk objectid 256 chunk offset 1071623700480 length

Signed-off-by: Forrest Liu <forrestl@synology.com>
---
 fs/btrfs/volumes.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index da7e0e1..61be789 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1060,6 +1060,7 @@ static int contains_pending_extent(struct btrfs_trans_handle *trans,
 	struct extent_map *em;
 	struct list_head *search_list = &trans->transaction->pending_chunks;
 	int ret = 0;
+	u64 physical_start = *start;
 
 again:
 	list_for_each_entry(em, search_list, list) {
@@ -1070,9 +1071,9 @@ again:
 		for (i = 0; i < map->num_stripes; i++) {
 			if (map->stripes[i].dev != device)
 				continue;
-			if (map->stripes[i].physical >= *start + len ||
+			if (map->stripes[i].physical >= physical_start + len ||
 			    map->stripes[i].physical + em->orig_block_len <=
-			    *start)
+			    physical_start)
 				continue;
 			*start = map->stripes[i].physical +
 				em->orig_block_len;
@@ -1195,8 +1196,14 @@ again:
 			 */
 			if (contains_pending_extent(trans, device,
 						    &search_start,
-						    hole_size))
-				hole_size = 0;
+						    hole_size)) {
+				if (key.offset >= search_start)
+					hole_size = key_offset - search_start;
+				else {
+					WARN_ON(1);
+					hole_size = 0;
+				}
+			}
 
 			if (hole_size > max_hole_size) {
 				max_hole_start = search_start;
-- 
1.9.1

Re: [PATCH] Btrfs: fix find_free_dev_extent() malfunction in case device tree has hole

[Ed Tomlinson]

Hi,

Found a problem compile testing this.  

hole_size = key_offset - search_start;

Should not that be key.offset ?

TIA
Ed Tomlinson


On Monday, February 2, 2015 2:31:39 AM EST, Forrest Liu wrote:
> If device tree has hole, find_free_dev_extent() cannot find available
> address properly.
>
> The example below, has one BIG hole in device tree, and can only
> allocate just one chunk in a transaction.
>
>     item 9 key (1 DEV_EXTENT 273841913856) itemoff 15811 itemsize 48
>         dev extent chunk_tree 3
>         chunk objectid 256 chunk offset 272759783424 length 1073741824
>     item 10 key (1 DEV_EXTENT 1071632089088) itemoff 15763 itemsize 48
>         dev extent chunk_tree 3
>         chunk objectid 256 chunk offset 1070549958656 length 1073741824
>     item 11 key (1 DEV_EXTENT 1072705830912) itemoff 15715 itemsize 48
>             dev extent chunk_tree 3
>             chunk objectid 256 chunk offset 1071623700480 length
>
> Signed-off-by: Forrest Liu <forrestl@synology.com>
> ---
>  fs/btrfs/volumes.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index da7e0e1..61be789 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1060,6 +1060,7 @@ static int contains_pending_extent(struct 
> btrfs_trans_handle *trans,
>  	struct extent_map *em;
>  	struct list_head *search_list = &trans->transaction->pending_chunks;
>  	int ret = 0;
> +	u64 physical_start = *start;
>  
>  again:
>  	list_for_each_entry(em, search_list, list) {
> @@ -1070,9 +1071,9 @@ again:
>  		for (i = 0; i < map->num_stripes; i++) {
>  			if (map->stripes[i].dev != device)
>  				continue;
> -			if (map->stripes[i].physical >= *start + len ||
> +			if (map->stripes[i].physical >= physical_start + len ||
>  			    map->stripes[i].physical + em->orig_block_len <=
> -			    *start)
> +			    physical_start)
>  				continue;
>  			*start = map->stripes[i].physical +
>  				em->orig_block_len;
> @@ -1195,8 +1196,14 @@ again:
>  			 */
>  			if (contains_pending_extent(trans, device,
>  						    &search_start,
> -						    hole_size))
> -				hole_size = 0;
> +						    hole_size)) {
> +				if (key.offset >= search_start)
> +					hole_size = key_offset - search_start;
> +				else {
> +					WARN_ON(1);
> +					hole_size = 0;
> +				}
> +			}
>  
>  			if (hole_size > max_hole_size) {
>  				max_hole_start = search_start;

Re: [PATCH] Btrfs: fix find_free_dev_extent() malfunction in case device tree has hole

[Ed Tomlinson]

On Monday, February 2, 2015 9:39:06 AM EST, Ed Tomlinson wrote:

Hi

Booting a kernel with the three patches:
[PATCH] Btrfs: fix find_free_dev_extent() malfunction in case device tree 
has hole
[PATCH] Btrfs: btrfs_release_extent_buffer_page() didn't free pages of 
dummy extent
[PATCH] Btrfs: fix BUG_ON in btrfs_orphan_add() when delete unused block 
group

generates lots of opps here (I hate to post an anemic report but my serial 
console was not recording so I do not have the opps).  They occured when 
starting X and, If I read them correctly, had something to do with extents.

Anyone else?

Thanks
Ed Tomlinson

> Hi,
>
> Found a problem compile testing this.  
>
> hole_size = key_offset - search_start;
>
> Should not that be key.offset ?
>
> TIA
> Ed Tomlinson
>
>
> On Monday, February 2, 2015 2:31:39 AM EST, Forrest Liu wrote:
>> If device tree has hole, find_free_dev_extent() cannot find available
>> address properly.
>> 
>> The example below, has one BIG hole in device tree, and can only
>> allocate just one chunk in a transaction.
>> 
>>     item 9 key (1 DEV_EXTENT 273841913856) itemoff 15811 itemsize 48 ...
>
> --
> 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
>
>
>

Re: [PATCH] Btrfs: fix find_free_dev_extent() malfunction in case device tree has hole

[Forrest Liu]

2015-02-03 2:40 GMT+08:00 Ed Tomlinson <edt@aei.ca>:
> On Monday, February 2, 2015 9:39:06 AM EST, Ed Tomlinson wrote:
>
> Hi
>
> Booting a kernel with the three patches:
> [PATCH] Btrfs: fix find_free_dev_extent() malfunction in case device tree
> has hole
> [PATCH] Btrfs: btrfs_release_extent_buffer_page() didn't free pages of dummy
> extent
> [PATCH] Btrfs: fix BUG_ON in btrfs_orphan_add() when delete unused block
> group
>
> generates lots of opps here (I hate to post an anemic report but my serial
> console was not recording so I do not have the opps).  They occured when
> starting X and, If I read them correctly, had something to do with extents.
>

My fault, i should test these patches before i submit these patches.
The oops was caused by patch
"Btrfs: btrfs_release_extent_buffer_page() didn't free pages of dummy extent"

I will resend these patches after test on linux-3.19-rc7.

Thanks
Forrest

> Anyone else?
>
> Thanks
> Ed Tomlinson
>
>> Hi,
>>
>> Found a problem compile testing this.
>> hole_size = key_offset - search_start;
>>
>> Should not that be key.offset ?
>>
>> TIA
>> Ed Tomlinson
>>
>>
>> On Monday, February 2, 2015 2:31:39 AM EST, Forrest Liu wrote:
>>>
>>> If device tree has hole, find_free_dev_extent() cannot find available
>>> address properly.
>>>
>>> The example below, has one BIG hole in device tree, and can only
>>> allocate just one chunk in a transaction.
>>>
>>>     item 9 key (1 DEV_EXTENT 273841913856) itemoff 15811 itemsize 48 ...
>>
>>
>> --
>> 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
>>
>>
>>
>

Re: [PATCH] Btrfs: fix find_free_dev_extent() malfunction in case device tree has hole

[Ed Tomlinson]

Hi,

Its a great idea to test the patches before submitting.  However I think 
its even more importent to tell us the state of testing.  eg. this is an 
RFC or in production in abc's kernel, and this version is untested or has 
been compile tested, boot tested, improves xfstests (before x failures out 
of z, after the patch the number of failures was y, where y<x).

A little bit of info like this will help those of us using the info in the 
lists and should aid getting your patches into the kernel faster.

Thanks
Ed

On Tuesday, February 3, 2015 6:36:40 AM EST, Forrest Liu wrote:
> 2015-02-03 2:40 GMT+08:00 Ed Tomlinson <edt@aei.ca>:
>> On Monday, February 2, 2015 9:39:06 AM EST, Ed Tomlinson wrote:
>> 
>> Hi
>> 
>> Booting a kernel with the three patches:
>> [PATCH] Btrfs: fix find_free_dev_extent() malfunction in case device tree
>> has hole ...
>
> My fault, i should test these patches before i submit these patches.
> The oops was caused by patch
> "Btrfs: btrfs_release_extent_buffer_page() didn't free pages of 
> dummy extent"
>
> I will resend these patches after test on linux-3.19-rc7.
>
> Thanks
> Forrest
>
>> Anyone else?
>> 
>> Thanks
>> Ed Tomlinson
>>  ...
> --
> 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
>
>
>

Re: [PATCH] Btrfs: fix find_free_dev_extent() malfunction in case device tree has hole

[Liu Bo]

On Mon, Feb 02, 2015 at 03:31:39PM +0800, Forrest Liu wrote:
> If device tree has hole, find_free_dev_extent() cannot find available
> address properly.
> 
> The example below, has one BIG hole in device tree, and can only
> allocate just one chunk in a transaction.
> 
>     item 9 key (1 DEV_EXTENT 273841913856) itemoff 15811 itemsize 48
>         dev extent chunk_tree 3
>         chunk objectid 256 chunk offset 272759783424 length 1073741824
>     item 10 key (1 DEV_EXTENT 1071632089088) itemoff 15763 itemsize 48
>         dev extent chunk_tree 3
>         chunk objectid 256 chunk offset 1070549958656 length 1073741824
>     item 11 key (1 DEV_EXTENT 1072705830912) itemoff 15715 itemsize 48
>             dev extent chunk_tree 3
>             chunk objectid 256 chunk offset 1071623700480 length
> 
> Signed-off-by: Forrest Liu <forrestl@synology.com>
> ---
>  fs/btrfs/volumes.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index da7e0e1..61be789 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1060,6 +1060,7 @@ static int contains_pending_extent(struct btrfs_trans_handle *trans,
>  	struct extent_map *em;
>  	struct list_head *search_list = &trans->transaction->pending_chunks;
>  	int ret = 0;
> +	u64 physical_start = *start;
>  
>  again:
>  	list_for_each_entry(em, search_list, list) {
> @@ -1070,9 +1071,9 @@ again:
>  		for (i = 0; i < map->num_stripes; i++) {
>  			if (map->stripes[i].dev != device)
>  				continue;
> -			if (map->stripes[i].physical >= *start + len ||
> +			if (map->stripes[i].physical >= physical_start + len ||
>  			    map->stripes[i].physical + em->orig_block_len <=
> -			    *start)
> +			    physical_start)
>  				continue;
>  			*start = map->stripes[i].physical +
>  				em->orig_block_len;

This part looks good.

> @@ -1195,8 +1196,14 @@ again:
>  			 */
>  			if (contains_pending_extent(trans, device,
>  						    &search_start,
> -						    hole_size))
> -				hole_size = 0;
> +						    hole_size)) {
> +				if (key.offset >= search_start)
> +					hole_size = key_offset - search_start;
> +				else {
> +					WARN_ON(1);
> +					hole_size = 0;

I'd prefer WARN_ON_ONCE(1) since key.offset should not be less than search_start by design.

Other looks good.

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

Thanks,

-liubo

> +				}
> +			}
>  
>  			if (hole_size > max_hole_size) {
>  				max_hole_start = search_start;
> -- 
> 1.9.1
> 
> --
> 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