* [PATCH 1/3] btrfs: Remove unused arguments from btrfs_get_extent_fiemap
2018-12-12 7:42 [PATCH 0/3] Cleanups around btrfs_get_extent_fiemap Nikolay Borisov
@ 2018-12-12 7:42 ` Nikolay Borisov
2018-12-12 9:14 ` Johannes Thumshirn
2018-12-12 7:42 ` [PATCH 2/3] btrfs: Refactor btrfs_get_extent_fiemap Nikolay Borisov
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Nikolay Borisov @ 2018-12-12 7:42 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 2/3] btrfs: Refactor btrfs_get_extent_fiemap
2018-12-12 7:42 [PATCH 0/3] Cleanups around btrfs_get_extent_fiemap Nikolay Borisov
2018-12-12 7:42 ` [PATCH 1/3] btrfs: Remove unused arguments from btrfs_get_extent_fiemap Nikolay Borisov
@ 2018-12-12 7:42 ` Nikolay Borisov
2019-01-04 14:59 ` David Sterba
2018-12-12 7:42 ` [PATCH 3/3] btrfs: Remove redundant assignment Nikolay Borisov
2019-01-04 15:01 ` [PATCH 0/3] Cleanups around btrfs_get_extent_fiemap David Sterba
3 siblings, 1 reply; 8+ messages in thread
From: Nikolay Borisov @ 2018-12-12 7:42 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/3] btrfs: Refactor btrfs_get_extent_fiemap
2018-12-12 7:42 ` [PATCH 2/3] btrfs: Refactor btrfs_get_extent_fiemap Nikolay Borisov
@ 2019-01-04 14:59 ` David Sterba
0 siblings, 0 replies; 8+ messages in thread
From: David Sterba @ 2019-01-04 14:59 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] btrfs: Remove redundant assignment
2018-12-12 7:42 [PATCH 0/3] Cleanups around btrfs_get_extent_fiemap Nikolay Borisov
2018-12-12 7:42 ` [PATCH 1/3] btrfs: Remove unused arguments from btrfs_get_extent_fiemap Nikolay Borisov
2018-12-12 7:42 ` [PATCH 2/3] btrfs: Refactor btrfs_get_extent_fiemap Nikolay Borisov
@ 2018-12-12 7:42 ` Nikolay Borisov
2018-12-12 9:28 ` Johannes Thumshirn
2019-01-04 15:01 ` [PATCH 0/3] Cleanups around btrfs_get_extent_fiemap David Sterba
3 siblings, 1 reply; 8+ messages in thread
From: Nikolay Borisov @ 2018-12-12 7:42 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 0/3] Cleanups around btrfs_get_extent_fiemap
2018-12-12 7:42 [PATCH 0/3] Cleanups around btrfs_get_extent_fiemap Nikolay Borisov
` (2 preceding siblings ...)
2018-12-12 7:42 ` [PATCH 3/3] btrfs: Remove redundant assignment Nikolay Borisov
@ 2019-01-04 15:01 ` David Sterba
3 siblings, 0 replies; 8+ messages in thread
From: David Sterba @ 2019-01-04 15:01 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 8+ messages in thread