[PATCH 0/4] Misc cleanups of btrfs_get_extent

[PATCH 0/4] Misc cleanups of btrfs_get_extent

[Nikolay Borisov]

Here is a series that hopefully makes the code a bit more obvious. First patch 
gives a variable a more appropraite name, since it's going to be used only for 
holding an extent type and not the type of a found item. 

Patch 2 simply consolidated separate 'if' statements that check the retval of
the same function into a single 'if() {} else if () {}' statement. IMO this is 
cleaner.

Patch 3 massages the abhorrent code that deals with btrfs_lookup_file_extent
retval. It groups everything in a coherent 'if() {} else if () {} else {}' 
construct and now it's obvious under what conditions specific code is executed. 

Finally, Patch 4 removes the not_found_em labelin the same function.

Nikolay Borisov (4):
  btrfs: Rename found_type to extent_type
  btrfs: Consolidate retval checking of core btree functions
  btrfs: Refactor retval handling of btrfs_lookup_file_extent in
    btrfs_get_extent
  btrfs: Remove not_found_em label from btrfs_get_extent

 fs/btrfs/inode.c | 67 ++++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 33 deletions(-)

-- 
2.17.1

[PATCH 1/4] btrfs: Rename found_type to extent_type

[Nikolay Borisov]

found_type really holds the type of extent and is guaranteed to to have
a value between [0, 2]. The only time it can contain anything different
is if btrfs_lookup_file_extent returned a positive value and the
previous item is different than an extent. Avoid this situation by
simply checking found_key.type rather than assigning the item type to
found_type intermittently. Also make the variable an u8 to reduce stack
usage. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 73ae4428d0df..fe25f66a98d9 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6746,7 +6746,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	u64 extent_start = 0;
 	u64 extent_end = 0;
 	u64 objectid = btrfs_ino(inode);
-	u32 found_type;
+	u8 extent_type;
 	struct btrfs_path *path = NULL;
 	struct btrfs_root *root = inode->root;
 	struct btrfs_file_extent_item *item;
@@ -6812,11 +6812,9 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	leaf = path->nodes[0];
 	item = btrfs_item_ptr(leaf, path->slots[0],
 			      struct btrfs_file_extent_item);
-	/* are we inside the extent that was found? */
 	btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
-	found_type = found_key.type;
 	if (found_key.objectid != objectid ||
-	    found_type != BTRFS_EXTENT_DATA_KEY) {
+	    found_key.type != BTRFS_EXTENT_DATA_KEY) {
 		/*
 		 * If we backup past the first extent we want to move forward
 		 * and see if there is an extent in front of us, otherwise we'll
@@ -6827,16 +6825,16 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 		goto next;
 	}
 
-	found_type = btrfs_file_extent_type(leaf, item);
+	extent_type = btrfs_file_extent_type(leaf, item);
 	extent_start = found_key.offset;
-	if (found_type == BTRFS_FILE_EXTENT_REG ||
-	    found_type == BTRFS_FILE_EXTENT_PREALLOC) {
+	if (extent_type == BTRFS_FILE_EXTENT_REG ||
+	    extent_type == BTRFS_FILE_EXTENT_PREALLOC) {
 		extent_end = extent_start +
 		       btrfs_file_extent_num_bytes(leaf, item);
 
 		trace_btrfs_get_extent_show_fi_regular(inode, leaf, item,
 						       extent_start);
-	} else if (found_type == BTRFS_FILE_EXTENT_INLINE) {
+	} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
 		size_t size;
 
 		size = btrfs_file_extent_ram_bytes(leaf, item);
@@ -6877,10 +6875,10 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	btrfs_extent_item_to_extent_map(inode, path, item,
 			new_inline, em);
 
-	if (found_type == BTRFS_FILE_EXTENT_REG ||
-	    found_type == BTRFS_FILE_EXTENT_PREALLOC) {
+	if (extent_type == BTRFS_FILE_EXTENT_REG ||
+	    extent_type == BTRFS_FILE_EXTENT_PREALLOC) {
 		goto insert;
-	} else if (found_type == BTRFS_FILE_EXTENT_INLINE) {
+	} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
 		unsigned long ptr;
 		char *map;
 		size_t size;
-- 
2.17.1

Re: [PATCH 1/4] btrfs: Rename found_type to extent_type

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 1/4] btrfs: Rename found_type to extent_type

[Qu Wenruo]

On 2018/12/17 下午4:35, Nikolay Borisov wrote:
> found_type really holds the type of extent and is guaranteed to to have
> a value between [0, 2]. The only time it can contain anything different
> is if btrfs_lookup_file_extent returned a positive value and the
> previous item is different than an extent. Avoid this situation by
> simply checking found_key.type rather than assigning the item type to
> found_type intermittently. Also make the variable an u8 to reduce stack
> usage. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Indeed, abusing u64 for u8 makes it pretty confusing.

Now looks much better.

Thanks,
Qu

> ---
>  fs/btrfs/inode.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 73ae4428d0df..fe25f66a98d9 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6746,7 +6746,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>  	u64 extent_start = 0;
>  	u64 extent_end = 0;
>  	u64 objectid = btrfs_ino(inode);
> -	u32 found_type;
> +	u8 extent_type;
>  	struct btrfs_path *path = NULL;
>  	struct btrfs_root *root = inode->root;
>  	struct btrfs_file_extent_item *item;
> @@ -6812,11 +6812,9 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>  	leaf = path->nodes[0];
>  	item = btrfs_item_ptr(leaf, path->slots[0],
>  			      struct btrfs_file_extent_item);
> -	/* are we inside the extent that was found? */
>  	btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
> -	found_type = found_key.type;
>  	if (found_key.objectid != objectid ||
> -	    found_type != BTRFS_EXTENT_DATA_KEY) {
> +	    found_key.type != BTRFS_EXTENT_DATA_KEY) {
>  		/*
>  		 * If we backup past the first extent we want to move forward
>  		 * and see if there is an extent in front of us, otherwise we'll
> @@ -6827,16 +6825,16 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>  		goto next;
>  	}
>  
> -	found_type = btrfs_file_extent_type(leaf, item);
> +	extent_type = btrfs_file_extent_type(leaf, item);
>  	extent_start = found_key.offset;
> -	if (found_type == BTRFS_FILE_EXTENT_REG ||
> -	    found_type == BTRFS_FILE_EXTENT_PREALLOC) {
> +	if (extent_type == BTRFS_FILE_EXTENT_REG ||
> +	    extent_type == BTRFS_FILE_EXTENT_PREALLOC) {
>  		extent_end = extent_start +
>  		       btrfs_file_extent_num_bytes(leaf, item);
>  
>  		trace_btrfs_get_extent_show_fi_regular(inode, leaf, item,
>  						       extent_start);
> -	} else if (found_type == BTRFS_FILE_EXTENT_INLINE) {
> +	} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
>  		size_t size;
>  
>  		size = btrfs_file_extent_ram_bytes(leaf, item);
> @@ -6877,10 +6875,10 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>  	btrfs_extent_item_to_extent_map(inode, path, item,
>  			new_inline, em);
>  
> -	if (found_type == BTRFS_FILE_EXTENT_REG ||
> -	    found_type == BTRFS_FILE_EXTENT_PREALLOC) {
> +	if (extent_type == BTRFS_FILE_EXTENT_REG ||
> +	    extent_type == BTRFS_FILE_EXTENT_PREALLOC) {
>  		goto insert;
> -	} else if (found_type == BTRFS_FILE_EXTENT_INLINE) {
> +	} else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
>  		unsigned long ptr;
>  		char *map;
>  		size_t size;
> 

[PATCH 2/4] btrfs: Consolidate retval checking of core btree functions

[Nikolay Borisov]

Core btree functions in btrfs generally return 0 for error and 1 in case
the sought item cannot be found. Consolidate the checks for those
conditions in one 'if () {} else if () {}' struct rather than 2 separate
'if () {}' statements. This emphasizes that the handling code pertains
to a single function. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index fe25f66a98d9..511d3b314af2 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6801,9 +6801,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	if (ret < 0) {
 		err = ret;
 		goto out;
-	}
-
-	if (ret != 0) {
+	} else if (ret > 0) {
 		if (path->slots[0] == 0)
 			goto not_found;
 		path->slots[0]--;
@@ -6853,8 +6851,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 			if (ret < 0) {
 				err = ret;
 				goto out;
-			}
-			if (ret > 0)
+			} else if (ret > 0)
 				goto not_found;
 			leaf = path->nodes[0];
 		}
-- 
2.17.1

Re: [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions

[Johannes Thumshirn]

On 17/12/2018 09:36, Nikolay Borisov wrote:
> Core btree functions in btrfs generally return 0 for error and 1 in case
> the sought item cannot be found. Consolidate the checks for those
> conditions in one 'if () {} else if () {}' struct rather than 2 separate
                                     construct? ^

Otherwise,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions

[Nikolay Borisov]

On 17.12.18 г. 10:55 ч., Johannes Thumshirn wrote:
>                                      construct? ^

Good catch :) David, will you fix it on the way in to the tree or shall
I resend.

Re: [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions

[Qu Wenruo]

On 2018/12/17 下午4:36, Nikolay Borisov wrote:
> Core btree functions in btrfs generally return 0 for error

Isn't 0 for key found? It's <0 for error.

 and 1 in case
> the sought item cannot be found. Consolidate the checks for those
> conditions in one 'if () {} else if () {}' struct rather than 2 separate
> 'if () {}' statements. This emphasizes that the handling code pertains
> to a single function. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
The code itself is OK, but the commit message is really confusing.

Thanks,
Qu

> ---
>  fs/btrfs/inode.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index fe25f66a98d9..511d3b314af2 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6801,9 +6801,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>  	if (ret < 0) {
>  		err = ret;
>  		goto out;
> -	}
> -
> -	if (ret != 0) {
> +	} else if (ret > 0) {
>  		if (path->slots[0] == 0)
>  			goto not_found;
>  		path->slots[0]--;
> @@ -6853,8 +6851,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>  			if (ret < 0) {
>  				err = ret;
>  				goto out;
> -			}
> -			if (ret > 0)
> +			} else if (ret > 0)
>  				goto not_found;
>  			leaf = path->nodes[0];
>  		}
> 

Re: [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions

[Nikolay Borisov]

On 17.12.18 г. 11:09 ч., Qu Wenruo wrote:
> 
> 
> On 2018/12/17 下午4:36, Nikolay Borisov wrote:
>> Core btree functions in btrfs generally return 0 for error
> 
> Isn't 0 for key found? It's <0 for error.
> 
>  and 1 in case

Shit, you are right, 0 found, < 0 error, >0 where it must be inserted
>> the sought item cannot be found. Consolidate the checks for those
>> conditions in one 'if () {} else if () {}' struct rather than 2 separate
>> 'if () {}' statements. This emphasizes that the handling code pertains
>> to a single function. No functional changes.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> The code itself is OK, but the commit message is really confusing.
> 
> Thanks,
> Qu
> 
>> ---
>>  fs/btrfs/inode.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index fe25f66a98d9..511d3b314af2 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -6801,9 +6801,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>>  	if (ret < 0) {
>>  		err = ret;
>>  		goto out;
>> -	}
>> -
>> -	if (ret != 0) {
>> +	} else if (ret > 0) {
>>  		if (path->slots[0] == 0)
>>  			goto not_found;
>>  		path->slots[0]--;
>> @@ -6853,8 +6851,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>>  			if (ret < 0) {
>>  				err = ret;
>>  				goto out;
>> -			}
>> -			if (ret > 0)
>> +			} else if (ret > 0)
>>  				goto not_found;
>>  			leaf = path->nodes[0];
>>  		}
>>
> 

[PATCH v2 2/4] btrfs: Consolidate retval checking of core btree functions

[Nikolay Borisov]

Core btree functions in btrfs generally return 0 when an item is found, 1 in
case the sought item cannot be found and <0 when an error happens. Consolidate
the checks for those conditions in one 'if () {} else if () {}' struct rather
than 2 separate 'if () {}' statements. This emphasizes that the handling code
pertains to a single function. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

V2: 
 * Properly describe return value convention in changelog no other changes. 

 fs/btrfs/inode.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index fe25f66a98d9..511d3b314af2 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6801,9 +6801,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	if (ret < 0) {
 		err = ret;
 		goto out;
-	}
-
-	if (ret != 0) {
+	} else if (ret > 0) {
 		if (path->slots[0] == 0)
 			goto not_found;
 		path->slots[0]--;
@@ -6853,8 +6851,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 			if (ret < 0) {
 				err = ret;
 				goto out;
-			}
-			if (ret > 0)
+			} else if (ret > 0)
 				goto not_found;
 			leaf = path->nodes[0];
 		}
-- 
2.17.1

Re: [PATCH v2 2/4] btrfs: Consolidate retval checking of core btree functions

[Qu Wenruo]

On 2018/12/17 下午5:49, Nikolay Borisov wrote:
> Core btree functions in btrfs generally return 0 when an item is found, 1 in
> case the sought item cannot be found and <0 when an error happens. Consolidate
> the checks for those conditions in one 'if () {} else if () {}' struct rather
> than 2 separate 'if () {}' statements. This emphasizes that the handling code
> pertains to a single function. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
> 
> V2: 
>  * Properly describe return value convention in changelog no other changes. 
> 
>  fs/btrfs/inode.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index fe25f66a98d9..511d3b314af2 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6801,9 +6801,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>  	if (ret < 0) {
>  		err = ret;
>  		goto out;
> -	}
> -
> -	if (ret != 0) {
> +	} else if (ret > 0) {
>  		if (path->slots[0] == 0)
>  			goto not_found;
>  		path->slots[0]--;
> @@ -6853,8 +6851,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>  			if (ret < 0) {
>  				err = ret;
>  				goto out;
> -			}
> -			if (ret > 0)
> +			} else if (ret > 0)
>  				goto not_found;
>  			leaf = path->nodes[0];
>  		}
> 

[PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent

[Nikolay Borisov]

The code which executes if path->slots[0] points to an item not
belonging to the inode we are interested in or is not na extent at all
could really trigger if btrfs_lookup_file_extent returned > 0 and
subsequently path->slots[0] was decremented. Make this explicit by
moving the respective code in the correct if branch. No functional
changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 511d3b314af2..e8284cd0a122 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6805,22 +6805,26 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 		if (path->slots[0] == 0)
 			goto not_found;
 		path->slots[0]--;
-	}
-
-	leaf = path->nodes[0];
-	item = btrfs_item_ptr(leaf, path->slots[0],
-			      struct btrfs_file_extent_item);
-	btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
-	if (found_key.objectid != objectid ||
-	    found_key.type != BTRFS_EXTENT_DATA_KEY) {
-		/*
-		 * If we backup past the first extent we want to move forward
-		 * and see if there is an extent in front of us, otherwise we'll
-		 * say there is a hole for our whole search range which can
-		 * cause problems.
-		 */
-		extent_end = start;
-		goto next;
+		leaf = path->nodes[0];
+		item = btrfs_item_ptr(leaf, path->slots[0],
+				      struct btrfs_file_extent_item);
+		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
+		if (found_key.objectid != objectid ||
+		    found_key.type != BTRFS_EXTENT_DATA_KEY) {
+			/*
+			 * If we backup past the first extent we want to move
+			 * forward and see if there is an extent in front of us,
+			 * otherwise we'll say there is a hole for our whole
+			 * search range which can cause problems.
+			 */
+			extent_end = start;
+			goto next;
+		}
+	} else {
+		leaf = path->nodes[0];
+		item = btrfs_item_ptr(leaf, path->slots[0],
+				      struct btrfs_file_extent_item);
+		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
 	}
 
 	extent_type = btrfs_file_extent_type(leaf, item);
-- 
2.17.1

Re: [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent

[Johannes Thumshirn]

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent

[David Sterba]

On Mon, Dec 17, 2018 at 10:36:01AM +0200, Nikolay Borisov wrote:
> The code which executes if path->slots[0] points to an item not
> belonging to the inode we are interested in or is not na extent at all
> could really trigger if btrfs_lookup_file_extent returned > 0 and
> subsequently path->slots[0] was decremented. Make this explicit by
> moving the respective code in the correct if branch. No functional
> changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/inode.c | 36 ++++++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 511d3b314af2..e8284cd0a122 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6805,22 +6805,26 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
>  		if (path->slots[0] == 0)
>  			goto not_found;
>  		path->slots[0]--;
> -	}
> -
> -	leaf = path->nodes[0];
> -	item = btrfs_item_ptr(leaf, path->slots[0],
> -			      struct btrfs_file_extent_item);
> -	btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
> -	if (found_key.objectid != objectid ||
> -	    found_key.type != BTRFS_EXTENT_DATA_KEY) {
> -		/*
> -		 * If we backup past the first extent we want to move forward
> -		 * and see if there is an extent in front of us, otherwise we'll
> -		 * say there is a hole for our whole search range which can
> -		 * cause problems.
> -		 */
> -		extent_end = start;
> -		goto next;
> +		leaf = path->nodes[0];
> +		item = btrfs_item_ptr(leaf, path->slots[0],
> +				      struct btrfs_file_extent_item);
> +		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
> +		if (found_key.objectid != objectid ||
> +		    found_key.type != BTRFS_EXTENT_DATA_KEY) {
> +			/*
> +			 * If we backup past the first extent we want to move
> +			 * forward and see if there is an extent in front of us,
> +			 * otherwise we'll say there is a hole for our whole
> +			 * search range which can cause problems.
> +			 */
> +			extent_end = start;
> +			goto next;
> +		}
> +	} else {
> +		leaf = path->nodes[0];
> +		item = btrfs_item_ptr(leaf, path->slots[0],
> +				      struct btrfs_file_extent_item);

This is repeating code and could be simplified ... to the original code.
I'm not sure this patch is an improvement.

Re: [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent

[Nikolay Borisov]

On 2.01.19 г. 19:05 ч., David Sterba wrote:
> This is repeating code and could be simplified ... to the original code.

It does and this is intentional. What I've  strived to do is make the
idea of the code obvious and not try to reduce the total line of code.
It is a massive improvement given the code which modifies extent_end
triggers only in case ret is > 0. I discussed this with Johannes and he
agreed with my assessment.

> I'm not sure this patch is an improvement.

Re: [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent

[Johannes Thumshirn]

On 02/01/2019 18:43, Nikolay Borisov wrote:
> 
> 
> On 2.01.19 г. 19:05 ч., David Sterba wrote:
>> This is repeating code and could be simplified ... to the original code.
> 
> It does and this is intentional. What I've  strived to do is make the
> idea of the code obvious and not try to reduce the total line of code.
> It is a massive improvement given the code which modifies extent_end
> triggers only in case ret is > 0. I discussed this with Johannes and he
> agreed with my assessment.
> 
>> I'm not sure this patch is an improvement.

I think it really helps when trying to understand the code.

This is why the patch was created in the first place, I asked Nik about
something and we went over the code together.

In the end it's your call, obviously.

Byte,
	Johannes
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent

[David Sterba]

On Mon, Jan 07, 2019 at 04:22:01PM +0100, Johannes Thumshirn wrote:
> On 02/01/2019 18:43, Nikolay Borisov wrote:
> > On 2.01.19 г. 19:05 ч., David Sterba wrote:
> >> This is repeating code and could be simplified ... to the original code.
> > 
> > It does and this is intentional. What I've  strived to do is make the
> > idea of the code obvious and not try to reduce the total line of code.
> > It is a massive improvement given the code which modifies extent_end
> > triggers only in case ret is > 0. I discussed this with Johannes and he
> > agreed with my assessment.
> > 
> >> I'm not sure this patch is an improvement.
> 
> I think it really helps when trying to understand the code.
> 
> This is why the patch was created in the first place, I asked Nik about
> something and we went over the code together.

I've tried to read the new version again with fresh eyes and still don't
see any improvement in readability. Not that the whole function is easy
to follow, quite the opposite, but I'd rather see continued refactoring
that untangles the gotos.

The return codes of btrfs_lookup_file_extent follow the common
btrfs_search_slot pattern < 0 error, 0 found, > 0 not found. And in the
last case there's an adjustment needed. Read the item type, check and
branch if necessary. No surprises there.

The amount of duplicated code is also not something trivial like
a variable asisgnment, there are arguments passed etc.

Re: [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent

[Nikolay Borisov]

On 7.01.19 г. 20:46 ч., David Sterba wrote:
> On Mon, Jan 07, 2019 at 04:22:01PM +0100, Johannes Thumshirn wrote:
>> On 02/01/2019 18:43, Nikolay Borisov wrote:
>>> On 2.01.19 г. 19:05 ч., David Sterba wrote:
>>>> This is repeating code and could be simplified ... to the original code.
>>>
>>> It does and this is intentional. What I've  strived to do is make the
>>> idea of the code obvious and not try to reduce the total line of code.
>>> It is a massive improvement given the code which modifies extent_end
>>> triggers only in case ret is > 0. I discussed this with Johannes and he
>>> agreed with my assessment.
>>>
>>>> I'm not sure this patch is an improvement.
>>
>> I think it really helps when trying to understand the code.
>>
>> This is why the patch was created in the first place, I asked Nik about
>> something and we went over the code together.
> 
> I've tried to read the new version again with fresh eyes and still don't
> see any improvement in readability. Not that the whole function is easy
> to follow, quite the opposite, but I'd rather see continued refactoring
> that untangles the gotos.
> 
> The return codes of btrfs_lookup_file_extent follow the common
> btrfs_search_slot pattern < 0 error, 0 found, > 0 not found. And in the
> last case there's an adjustment needed. Read the item type, check and
> branch if necessary. No surprises there.

Actually there is a surprise, this surprise is due to the fact that it's
not explicitly clear the adjustments happens when > 0 is returned. With
my patch it's clear which code gets executed only  when the respective
extent item wasn't found.

> 
> The amount of duplicated code is also not something trivial like
> a variable asisgnment, there are arguments passed etc.

The amount of duplication are 3 lines of linear code, I think this is
just being overly nitpicky given the (IMO) upside of this change.

> 

[PATCH 4/4] btrfs: Remove not_found_em label from btrfs_get_extent

[Nikolay Borisov]

In order to avoid duplicating init code for em there is an additional
label, not_found_em, which is used to only set ->block_start. The only
case when it will be used is if the extent we are adding overlaps with
an existing extent. Make that case more obvious by :
 1. Adding a comment hinting at what's going on
 2. Assigning EXTENT_MAP_HOLE and directly going to insert.

 No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e8284cd0a122..0b9855c2df21 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6867,10 +6867,13 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 			goto not_found;
 		if (start > found_key.offset)
 			goto next;
+
+		/* New extent overlaps with existing one */
 		em->start = start;
 		em->orig_start = start;
 		em->len = found_key.offset - start;
-		goto not_found_em;
+		em->block_start = EXTENT_MAP_HOLE;
+		goto insert;
 	}
 
 	btrfs_extent_item_to_extent_map(inode, path, item,
@@ -6930,7 +6933,6 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 	em->start = start;
 	em->orig_start = start;
 	em->len = len;
-not_found_em:
 	em->block_start = EXTENT_MAP_HOLE;
 insert:
 	btrfs_release_path(path);
-- 
2.17.1

Re: [PATCH 4/4] btrfs: Remove not_found_em label from btrfs_get_extent

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 0/4] Misc cleanups of btrfs_get_extent

[David Sterba]

On Mon, Dec 17, 2018 at 10:35:58AM +0200, Nikolay Borisov wrote:
> Here is a series that hopefully makes the code a bit more obvious. First patch 
> gives a variable a more appropraite name, since it's going to be used only for 
> holding an extent type and not the type of a found item. 
> 
> Patch 2 simply consolidated separate 'if' statements that check the retval of
> the same function into a single 'if() {} else if () {}' statement. IMO this is 
> cleaner.
> 
> Patch 3 massages the abhorrent code that deals with btrfs_lookup_file_extent
> retval. It groups everything in a coherent 'if() {} else if () {} else {}' 
> construct and now it's obvious under what conditions specific code is executed. 
> 
> Finally, Patch 4 removes the not_found_em labelin the same function.
> 
> Nikolay Borisov (4):
>   btrfs: Rename found_type to extent_type
>   btrfs: Consolidate retval checking of core btree functions
>   btrfs: Refactor retval handling of btrfs_lookup_file_extent in
>     btrfs_get_extent
>   btrfs: Remove not_found_em label from btrfs_get_extent

1, 2 and 4 now added to for-next, thanks.