Linux-BTRFS Archive on lore.kernel.org
 help / Atom feed
* [PATCH 0/4] Misc cleanups of btrfs_get_extent
@ 2018-12-17  8:35 Nikolay Borisov
  2018-12-17  8:35 ` [PATCH 1/4] btrfs: Rename found_type to extent_type Nikolay Borisov
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Nikolay Borisov @ 2018-12-17  8:35 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/4] btrfs: Rename found_type to extent_type
  2018-12-17  8:35 [PATCH 0/4] Misc cleanups of btrfs_get_extent Nikolay Borisov
@ 2018-12-17  8:35 ` Nikolay Borisov
  2018-12-17  8:54   ` Johannes Thumshirn
  2018-12-17  9:05   ` Qu Wenruo
  2018-12-17  8:36 ` [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions Nikolay Borisov
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 21+ messages in thread
From: Nikolay Borisov @ 2018-12-17  8:35 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions
  2018-12-17  8:35 [PATCH 0/4] Misc cleanups of btrfs_get_extent Nikolay Borisov
  2018-12-17  8:35 ` [PATCH 1/4] btrfs: Rename found_type to extent_type Nikolay Borisov
@ 2018-12-17  8:36 ` Nikolay Borisov
  2018-12-17  8:55   ` Johannes Thumshirn
                     ` (2 more replies)
  2018-12-17  8:36 ` [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent Nikolay Borisov
                   ` (2 subsequent siblings)
  4 siblings, 3 replies; 21+ messages in thread
From: Nikolay Borisov @ 2018-12-17  8:36 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent
  2018-12-17  8:35 [PATCH 0/4] Misc cleanups of btrfs_get_extent Nikolay Borisov
  2018-12-17  8:35 ` [PATCH 1/4] btrfs: Rename found_type to extent_type Nikolay Borisov
  2018-12-17  8:36 ` [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions Nikolay Borisov
@ 2018-12-17  8:36 ` Nikolay Borisov
  2018-12-17  9:07   ` Johannes Thumshirn
  2019-01-02 17:05   ` David Sterba
  2018-12-17  8:36 ` [PATCH 4/4] btrfs: Remove not_found_em label from btrfs_get_extent Nikolay Borisov
  2019-01-02 17:08 ` [PATCH 0/4] Misc cleanups of btrfs_get_extent David Sterba
  4 siblings, 2 replies; 21+ messages in thread
From: Nikolay Borisov @ 2018-12-17  8:36 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 4/4] btrfs: Remove not_found_em label from btrfs_get_extent
  2018-12-17  8:35 [PATCH 0/4] Misc cleanups of btrfs_get_extent Nikolay Borisov
                   ` (2 preceding siblings ...)
  2018-12-17  8:36 ` [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent Nikolay Borisov
@ 2018-12-17  8:36 ` Nikolay Borisov
  2018-12-17  9:08   ` Johannes Thumshirn
  2019-01-02 17:08 ` [PATCH 0/4] Misc cleanups of btrfs_get_extent David Sterba
  4 siblings, 1 reply; 21+ messages in thread
From: Nikolay Borisov @ 2018-12-17  8:36 UTC (permalink / raw)
  To: linux-btrfs; +Cc: 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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] btrfs: Rename found_type to extent_type
  2018-12-17  8:35 ` [PATCH 1/4] btrfs: Rename found_type to extent_type Nikolay Borisov
@ 2018-12-17  8:54   ` Johannes Thumshirn
  2018-12-17  9:05   ` Qu Wenruo
  1 sibling, 0 replies; 21+ messages in thread
From: Johannes Thumshirn @ 2018-12-17  8:54 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions
  2018-12-17  8:36 ` [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions Nikolay Borisov
@ 2018-12-17  8:55   ` Johannes Thumshirn
  2018-12-17  8:57     ` Nikolay Borisov
  2018-12-17  9:09   ` Qu Wenruo
  2018-12-17  9:49   ` [PATCH v2 " Nikolay Borisov
  2 siblings, 1 reply; 21+ messages in thread
From: Johannes Thumshirn @ 2018-12-17  8:55 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions
  2018-12-17  8:55   ` Johannes Thumshirn
@ 2018-12-17  8:57     ` Nikolay Borisov
  0 siblings, 0 replies; 21+ messages in thread
From: Nikolay Borisov @ 2018-12-17  8:57 UTC (permalink / raw)
  To: Johannes Thumshirn, linux-btrfs



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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] btrfs: Rename found_type to extent_type
  2018-12-17  8:35 ` [PATCH 1/4] btrfs: Rename found_type to extent_type Nikolay Borisov
  2018-12-17  8:54   ` Johannes Thumshirn
@ 2018-12-17  9:05   ` Qu Wenruo
  1 sibling, 0 replies; 21+ messages in thread
From: Qu Wenruo @ 2018-12-17  9:05 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs



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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent
  2018-12-17  8:36 ` [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent Nikolay Borisov
@ 2018-12-17  9:07   ` Johannes Thumshirn
  2019-01-02 17:05   ` David Sterba
  1 sibling, 0 replies; 21+ messages in thread
From: Johannes Thumshirn @ 2018-12-17  9:07 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/4] btrfs: Remove not_found_em label from btrfs_get_extent
  2018-12-17  8:36 ` [PATCH 4/4] btrfs: Remove not_found_em label from btrfs_get_extent Nikolay Borisov
@ 2018-12-17  9:08   ` Johannes Thumshirn
  0 siblings, 0 replies; 21+ messages in thread
From: Johannes Thumshirn @ 2018-12-17  9:08 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions
  2018-12-17  8:36 ` [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions Nikolay Borisov
  2018-12-17  8:55   ` Johannes Thumshirn
@ 2018-12-17  9:09   ` Qu Wenruo
  2018-12-17  9:23     ` Nikolay Borisov
  2018-12-17  9:49   ` [PATCH v2 " Nikolay Borisov
  2 siblings, 1 reply; 21+ messages in thread
From: Qu Wenruo @ 2018-12-17  9:09 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs



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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions
  2018-12-17  9:09   ` Qu Wenruo
@ 2018-12-17  9:23     ` Nikolay Borisov
  0 siblings, 0 replies; 21+ messages in thread
From: Nikolay Borisov @ 2018-12-17  9:23 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs



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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 2/4] btrfs: Consolidate retval checking of core btree functions
  2018-12-17  8:36 ` [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions Nikolay Borisov
  2018-12-17  8:55   ` Johannes Thumshirn
  2018-12-17  9:09   ` Qu Wenruo
@ 2018-12-17  9:49   ` " Nikolay Borisov
  2018-12-17 11:00     ` Qu Wenruo
  2 siblings, 1 reply; 21+ messages in thread
From: Nikolay Borisov @ 2018-12-17  9:49 UTC (permalink / raw)
  To: linux-btrfs; +Cc: quwenruo.btrfs, 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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/4] btrfs: Consolidate retval checking of core btree functions
  2018-12-17  9:49   ` [PATCH v2 " Nikolay Borisov
@ 2018-12-17 11:00     ` Qu Wenruo
  0 siblings, 0 replies; 21+ messages in thread
From: Qu Wenruo @ 2018-12-17 11:00 UTC (permalink / raw)
  To: Nikolay Borisov, linux-btrfs



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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent
  2018-12-17  8:36 ` [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent Nikolay Borisov
  2018-12-17  9:07   ` Johannes Thumshirn
@ 2019-01-02 17:05   ` David Sterba
  2019-01-02 17:43     ` Nikolay Borisov
  1 sibling, 1 reply; 21+ messages in thread
From: David Sterba @ 2019-01-02 17:05 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: linux-btrfs

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 0/4] Misc cleanups of btrfs_get_extent
  2018-12-17  8:35 [PATCH 0/4] Misc cleanups of btrfs_get_extent Nikolay Borisov
                   ` (3 preceding siblings ...)
  2018-12-17  8:36 ` [PATCH 4/4] btrfs: Remove not_found_em label from btrfs_get_extent Nikolay Borisov
@ 2019-01-02 17:08 ` David Sterba
  4 siblings, 0 replies; 21+ messages in thread
From: David Sterba @ 2019-01-02 17:08 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: linux-btrfs

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent
  2019-01-02 17:05   ` David Sterba
@ 2019-01-02 17:43     ` Nikolay Borisov
  2019-01-07 15:22       ` Johannes Thumshirn
  0 siblings, 1 reply; 21+ messages in thread
From: Nikolay Borisov @ 2019-01-02 17:43 UTC (permalink / raw)
  To: dsterba, linux-btrfs



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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent
  2019-01-02 17:43     ` Nikolay Borisov
@ 2019-01-07 15:22       ` Johannes Thumshirn
  2019-01-07 18:46         ` David Sterba
  0 siblings, 1 reply; 21+ messages in thread
From: Johannes Thumshirn @ 2019-01-07 15:22 UTC (permalink / raw)
  To: Nikolay Borisov, dsterba, linux-btrfs

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent
  2019-01-07 15:22       ` Johannes Thumshirn
@ 2019-01-07 18:46         ` David Sterba
  2019-01-08  7:43           ` Nikolay Borisov
  0 siblings, 1 reply; 21+ messages in thread
From: David Sterba @ 2019-01-07 18:46 UTC (permalink / raw)
  To: Johannes Thumshirn; +Cc: Nikolay Borisov, dsterba, linux-btrfs

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent
  2019-01-07 18:46         ` David Sterba
@ 2019-01-08  7:43           ` Nikolay Borisov
  0 siblings, 0 replies; 21+ messages in thread
From: Nikolay Borisov @ 2019-01-08  7:43 UTC (permalink / raw)
  To: dsterba, Johannes Thumshirn, linux-btrfs



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.

> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-17  8:35 [PATCH 0/4] Misc cleanups of btrfs_get_extent Nikolay Borisov
2018-12-17  8:35 ` [PATCH 1/4] btrfs: Rename found_type to extent_type Nikolay Borisov
2018-12-17  8:54   ` Johannes Thumshirn
2018-12-17  9:05   ` Qu Wenruo
2018-12-17  8:36 ` [PATCH 2/4] btrfs: Consolidate retval checking of core btree functions Nikolay Borisov
2018-12-17  8:55   ` Johannes Thumshirn
2018-12-17  8:57     ` Nikolay Borisov
2018-12-17  9:09   ` Qu Wenruo
2018-12-17  9:23     ` Nikolay Borisov
2018-12-17  9:49   ` [PATCH v2 " Nikolay Borisov
2018-12-17 11:00     ` Qu Wenruo
2018-12-17  8:36 ` [PATCH 3/4] btrfs: Refactor retval handling of btrfs_lookup_file_extent in btrfs_get_extent Nikolay Borisov
2018-12-17  9:07   ` Johannes Thumshirn
2019-01-02 17:05   ` David Sterba
2019-01-02 17:43     ` Nikolay Borisov
2019-01-07 15:22       ` Johannes Thumshirn
2019-01-07 18:46         ` David Sterba
2019-01-08  7:43           ` Nikolay Borisov
2018-12-17  8:36 ` [PATCH 4/4] btrfs: Remove not_found_em label from btrfs_get_extent Nikolay Borisov
2018-12-17  9:08   ` Johannes Thumshirn
2019-01-02 17:08 ` [PATCH 0/4] Misc cleanups of btrfs_get_extent David Sterba

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox