[PATCH 0/3] Cleanups around btrfs_get_extent_fiemap

[PATCH 0/3] Cleanups around btrfs_get_extent_fiemap

[Nikolay Borisov]

Following a conversation with Johaness this is what fell out. Turns out the
signature of btrfs_get_extent_fiemap is needlessly complext. So the first patch 
fixes it by removing the unnecessary arguments. Patch 2 is a bit of a "catch-all"
mainly renaming variables, thus helping recognise what the code does and
refreshing comments and removing one extra if. Finally, patch 3 removes an
assignment making it (at least to me) more clear what context a particular
variable is used in.

This has survived multiple xfstest runs. 

Nikolay Borisov (3):
  btrfs: Remove unused arguments from btrfs_get_extent_fiemap
  btrfs: Refactor btrfs_get_extent_fiemap
  btrfs: Remove redundant assignment

 fs/btrfs/ctree.h     |  3 +-
 fs/btrfs/extent_io.c |  3 +-
 fs/btrfs/file.c      |  3 +-
 fs/btrfs/inode.c     | 92 +++++++++++++++++++++++---------------------
 4 files changed, 51 insertions(+), 50 deletions(-)

-- 
2.17.1

[PATCH 1/3] btrfs: Remove unused arguments from btrfs_get_extent_fiemap

[Nikolay Borisov]

This function is a simple wrapper over btrfs_get_extent that returns
either:

a) A real extent in the passed range or
b) Adjusted extent based on whether delalloc bytes are found backing up
a hole.

To support these semantics it doesn't need the page/pg_offset/create
arguments which are passed to btrfs_get_extent in case an extent is to
be created. So simplify the function by removing the unused arguments.
No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h     | 3 +--
 fs/btrfs/extent_io.c | 3 +--
 fs/btrfs/file.c      | 3 +--
 fs/btrfs/inode.c     | 6 ++----
 4 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 6520e8e70b09..2dc94698c2fa 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3177,8 +3177,7 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
 
 /* inode.c */
 struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
-		struct page *page, size_t pg_offset, u64 start,
-		u64 len, int create);
+					   u64 start, u64 len);
 noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
 			      u64 *orig_start, u64 *orig_block_len,
 			      u64 *ram_bytes);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index f7e167ad01c4..2a72a7b5f033 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4236,8 +4236,7 @@ static struct extent_map *get_extent_skip_holes(struct inode *inode,
 		if (len == 0)
 			break;
 		len = ALIGN(len, sectorsize);
-		em = btrfs_get_extent_fiemap(BTRFS_I(inode), NULL, 0, offset,
-				len, 0);
+		em = btrfs_get_extent_fiemap(BTRFS_I(inode), offset, len);
 		if (IS_ERR_OR_NULL(em))
 			return em;
 
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 81aae230d1a5..9b77c3a1b9c6 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -3218,8 +3218,7 @@ static int find_desired_extent(struct inode *inode, loff_t *offset, int whence)
 			 &cached_state);
 
 	while (start < inode->i_size) {
-		em = btrfs_get_extent_fiemap(BTRFS_I(inode), NULL, 0,
-				start, len, 0);
+		em = btrfs_get_extent_fiemap(BTRFS_I(inode), start, len);
 		if (IS_ERR(em)) {
 			ret = PTR_ERR(em);
 			em = NULL;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index de912d6e48e6..67eee4d345c9 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6961,9 +6961,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 }
 
 struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
-		struct page *page,
-		size_t pg_offset, u64 start, u64 len,
-		int create)
+					   u64 start, u64 len)
 {
 	struct extent_map *em;
 	struct extent_map *hole_em = NULL;
@@ -6973,7 +6971,7 @@ struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
 	u64 found_end;
 	int err = 0;
 
-	em = btrfs_get_extent(inode, page, pg_offset, start, len, create);
+	em = btrfs_get_extent(inode, NULL, 0, start, len, 0);
 	if (IS_ERR(em))
 		return em;
 	/*
-- 
2.17.1

[PATCH 2/3] btrfs: Refactor btrfs_get_extent_fiemap

[Nikolay Borisov]

Make btrfs_get_extent_fiemap a bit more friendly. First step is to
rename the closely related, yet arbitrary named
range_start/found_end/found variables. They define the delalloc range
that is found in case a real extent wasn't found. Subsequently remove
an unnecessary check for hole_em since it's guaranteed to be set i.e the
check is always true. Top it off by giving all comments a refresh.

No functional changes.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 67eee4d345c9..95eb18779c19 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6965,10 +6965,10 @@ struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
 {
 	struct extent_map *em;
 	struct extent_map *hole_em = NULL;
-	u64 range_start = start;
+	u64 delalloc_start = start;
 	u64 end;
-	u64 found;
-	u64 found_end;
+	u64 delalloc_len;
+	u64 delalloc_end;
 	int err = 0;
 
 	em = btrfs_get_extent(inode, NULL, 0, start, len, 0);
@@ -6996,37 +6996,42 @@ struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
 	em = NULL;
 
 	/* ok, we didn't find anything, lets look for delalloc */
-	found = count_range_bits(&inode->io_tree, &range_start,
+	delalloc_len = count_range_bits(&inode->io_tree, &delalloc_start,
 				 end, len, EXTENT_DELALLOC, 1);
-	found_end = range_start + found;
-	if (found_end < range_start)
-		found_end = (u64)-1;
+	delalloc_end = delalloc_start + delalloc_len;
+	if (delalloc_end < delalloc_start)
+		delalloc_end = (u64)-1;
 
 	/*
-	 * we didn't find anything useful, return
-	 * the original results from get_extent()
+	 * we didn't find anything useful, return the original results from
+	 * get_extent()
 	 */
-	if (range_start > end || found_end <= start) {
+	if (delalloc_start > end || delalloc_end <= start) {
 		em = hole_em;
 		hole_em = NULL;
 		goto out;
 	}
 
-	/* adjust the range_start to make sure it doesn't
-	 * go backwards from the start they passed in
+	/*
+	 * adjust the delalloc_start to make sure it doesn't go backwards from
+	 * the start they passed in
 	 */
-	range_start = max(start, range_start);
-	found = found_end - range_start;
+	delalloc_start = max(start, delalloc_start);
+	delalloc_len = delalloc_end - delalloc_start;
 
-	if (found > 0) {
-		u64 hole_start = start;
+	if (delalloc_len) {
+		u64 hole_start;
 		u64 hole_len = len;
+		u64 hole_end = extent_map_end(hole_em);
 
 		em = alloc_extent_map();
 		if (!em) {
 			err = -ENOMEM;
 			goto out;
 		}
+		em->bdev = NULL;
+
+		ASSERT(hole_em);
 		/*
 		 * when btrfs_get_extent can't find anything it
 		 * returns one huge hole
@@ -7035,41 +7040,42 @@ struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
 		 * adjust to make sure it is based on the start from
 		 * the caller
 		 */
-		if (hole_em) {
-			u64 calc_end = extent_map_end(hole_em);
-
-			if (calc_end <= start || (hole_em->start > end)) {
-				free_extent_map(hole_em);
-				hole_em = NULL;
-			} else {
-				hole_start = max(hole_em->start, start);
-				hole_len = calc_end - hole_start;
-			}
+		if (hole_end <= start || (hole_em->start > end)) {
+		       free_extent_map(hole_em);
+		       hole_em = NULL;
+		} else {
+		       hole_start = max(hole_em->start, start);
+		       hole_len = hole_end - hole_start;
 		}
-		em->bdev = NULL;
-		if (hole_em && range_start > hole_start) {
-			/* our hole starts before our delalloc, so we
-			 * have to return just the parts of the hole
-			 * that go until  the delalloc starts
+
+		if (hole_em && delalloc_start > hole_start) {
+			/*
+			 * our hole starts before our delalloc, so we have to
+			 * return just the parts of the hole that go until the
+			 * delalloc starts
 			 */
-			em->len = min(hole_len,
-				      range_start - hole_start);
+			em->len = min(hole_len, delalloc_start - hole_start);
 			em->start = hole_start;
 			em->orig_start = hole_start;
 			/*
-			 * don't adjust block start at all,
-			 * it is fixed at EXTENT_MAP_HOLE
+			 * don't adjust block start at all, it is fixed at
+			 * EXTENT_MAP_HOLE
 			 */
 			em->block_start = hole_em->block_start;
 			em->block_len = hole_len;
 			if (test_bit(EXTENT_FLAG_PREALLOC, &hole_em->flags))
 				set_bit(EXTENT_FLAG_PREALLOC, &em->flags);
 		} else {
-			em->start = range_start;
-			em->len = found;
-			em->orig_start = range_start;
+
+			/*
+			 * Hole is out of passed range or it starts after
+			 * delalloc range
+			 */
+			em->start = delalloc_start;
+			em->len = delalloc_len;
+			em->orig_start = delalloc_start;
 			em->block_start = EXTENT_MAP_DELALLOC;
-			em->block_len = found;
+			em->block_len = delalloc_len;
 		}
 	} else {
 		return hole_em;
-- 
2.17.1

[PATCH 3/3] btrfs: Remove redundant assignment

[Nikolay Borisov]

hole_len is only used if the hole falls within the requested range. Make
that explicitly clear by only assigning in the corresponding branch.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 95eb18779c19..73ae4428d0df 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7021,7 +7021,7 @@ struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
 
 	if (delalloc_len) {
 		u64 hole_start;
-		u64 hole_len = len;
+		u64 hole_len;
 		u64 hole_end = extent_map_end(hole_em);
 
 		em = alloc_extent_map();
-- 
2.17.1

Re: [PATCH 1/3] btrfs: Remove unused arguments from btrfs_get_extent_fiemap

[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 3/3] btrfs: Remove redundant assignment

[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 2/3] btrfs: Refactor btrfs_get_extent_fiemap

[David Sterba]

On Wed, Dec 12, 2018 at 09:42:33AM +0200, Nikolay Borisov wrote:
> Make btrfs_get_extent_fiemap a bit more friendly. First step is to
> rename the closely related, yet arbitrary named
> range_start/found_end/found variables. They define the delalloc range
> that is found in case a real extent wasn't found. Subsequently remove
> an unnecessary check for hole_em since it's guaranteed to be set i.e the
> check is always true. Top it off by giving all comments a refresh.
> 
> No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

> ---
>  fs/btrfs/inode.c | 84 ++++++++++++++++++++++++++----------------------
>  1 file changed, 45 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 67eee4d345c9..95eb18779c19 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6965,10 +6965,10 @@ struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
>  {
>  	struct extent_map *em;
>  	struct extent_map *hole_em = NULL;
> -	u64 range_start = start;
> +	u64 delalloc_start = start;
>  	u64 end;
> -	u64 found;
> -	u64 found_end;
> +	u64 delalloc_len;
> +	u64 delalloc_end;
>  	int err = 0;
>  
>  	em = btrfs_get_extent(inode, NULL, 0, start, len, 0);
> @@ -6996,37 +6996,42 @@ struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
>  	em = NULL;
>  
>  	/* ok, we didn't find anything, lets look for delalloc */
> -	found = count_range_bits(&inode->io_tree, &range_start,
> +	delalloc_len = count_range_bits(&inode->io_tree, &delalloc_start,
>  				 end, len, EXTENT_DELALLOC, 1);
> -	found_end = range_start + found;
> -	if (found_end < range_start)
> -		found_end = (u64)-1;
> +	delalloc_end = delalloc_start + delalloc_len;
> +	if (delalloc_end < delalloc_start)
> +		delalloc_end = (u64)-1;
>  
>  	/*
> -	 * we didn't find anything useful, return
> -	 * the original results from get_extent()
> +	 * we didn't find anything useful, return the original results from
> +	 * get_extent()
>  	 */
> -	if (range_start > end || found_end <= start) {
> +	if (delalloc_start > end || delalloc_end <= start) {
>  		em = hole_em;
>  		hole_em = NULL;
>  		goto out;
>  	}
>  
> -	/* adjust the range_start to make sure it doesn't
> -	 * go backwards from the start they passed in
> +	/*
> +	 * adjust the delalloc_start to make sure it doesn't go backwards from
> +	 * the start they passed in
>  	 */
> -	range_start = max(start, range_start);
> -	found = found_end - range_start;
> +	delalloc_start = max(start, delalloc_start);
> +	delalloc_len = delalloc_end - delalloc_start;
>  
> -	if (found > 0) {
> -		u64 hole_start = start;
> +	if (delalloc_len) {
> +		u64 hole_start;
>  		u64 hole_len = len;
> +		u64 hole_end = extent_map_end(hole_em);
>  
>  		em = alloc_extent_map();
>  		if (!em) {
>  			err = -ENOMEM;
>  			goto out;
>  		}
> +		em->bdev = NULL;
> +
> +		ASSERT(hole_em);
>  		/*
>  		 * when btrfs_get_extent can't find anything it
>  		 * returns one huge hole
> @@ -7035,41 +7040,42 @@ struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
>  		 * adjust to make sure it is based on the start from
>  		 * the caller
>  		 */
> -		if (hole_em) {
> -			u64 calc_end = extent_map_end(hole_em);
> -
> -			if (calc_end <= start || (hole_em->start > end)) {
> -				free_extent_map(hole_em);
> -				hole_em = NULL;
> -			} else {
> -				hole_start = max(hole_em->start, start);
> -				hole_len = calc_end - hole_start;
> -			}
> +		if (hole_end <= start || (hole_em->start > end)) {
> +		       free_extent_map(hole_em);
> +		       hole_em = NULL;
> +		} else {
> +		       hole_start = max(hole_em->start, start);
> +		       hole_len = hole_end - hole_start;
>  		}
> -		em->bdev = NULL;
> -		if (hole_em && range_start > hole_start) {
> -			/* our hole starts before our delalloc, so we
> -			 * have to return just the parts of the hole
> -			 * that go until  the delalloc starts
> +
> +		if (hole_em && delalloc_start > hole_start) {
> +			/*
> +			 * our hole starts before our delalloc, so we have to
> +			 * return just the parts of the hole that go until the
> +			 * delalloc starts
>  			 */
> -			em->len = min(hole_len,
> -				      range_start - hole_start);
> +			em->len = min(hole_len, delalloc_start - hole_start);
>  			em->start = hole_start;
>  			em->orig_start = hole_start;
>  			/*
> -			 * don't adjust block start at all,
> -			 * it is fixed at EXTENT_MAP_HOLE
> +			 * don't adjust block start at all, it is fixed at
> +			 * EXTENT_MAP_HOLE
>  			 */
>  			em->block_start = hole_em->block_start;
>  			em->block_len = hole_len;
>  			if (test_bit(EXTENT_FLAG_PREALLOC, &hole_em->flags))
>  				set_bit(EXTENT_FLAG_PREALLOC, &em->flags);
>  		} else {
> -			em->start = range_start;
> -			em->len = found;
> -			em->orig_start = range_start;
> +
> +			/*
> +			 * Hole is out of passed range or it starts after
> +			 * delalloc range
> +			 */
> +			em->start = delalloc_start;
> +			em->len = delalloc_len;
> +			em->orig_start = delalloc_start;
>  			em->block_start = EXTENT_MAP_DELALLOC;
> -			em->block_len = found;
> +			em->block_len = delalloc_len;
>  		}
>  	} else {
>  		return hole_em;
> -- 
> 2.17.1

Re: [PATCH 0/3] Cleanups around btrfs_get_extent_fiemap

[David Sterba]

On Wed, Dec 12, 2018 at 09:42:31AM +0200, Nikolay Borisov wrote:
> Following a conversation with Johaness this is what fell out. Turns out the
> signature of btrfs_get_extent_fiemap is needlessly complext. So the first patch 
> fixes it by removing the unnecessary arguments. Patch 2 is a bit of a "catch-all"
> mainly renaming variables, thus helping recognise what the code does and
> refreshing comments and removing one extra if. Finally, patch 3 removes an
> assignment making it (at least to me) more clear what context a particular
> variable is used in.
> 
> This has survived multiple xfstest runs. 
> 
> Nikolay Borisov (3):
>   btrfs: Remove unused arguments from btrfs_get_extent_fiemap
>   btrfs: Refactor btrfs_get_extent_fiemap
>   btrfs: Remove redundant assignment

1-3 added to misc-next, thanks. I did a few more adjustments to comments
in patch 2/3.