[PATCH] btrfs: Remove impossible condition from mergable_maps

[PATCH] btrfs: Remove impossible condition from mergable_maps

[Nikolay Borisov]

We can never have extents marked as EXTENT_MAP_DELALLOC since this
value is only ever used by btrfs_get_extent_fiemap. In this case the
extent map is created by btrfs_get_extent_fiemap and is never really
published, this flag is used to return the corresponding userspace one.
Considering this, it's pointless having a check for EXTENT_MAP_DELALLOC
in mergable_maps. Just remove it.

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

diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 7eea8b6e2cd3..7cc893876f83 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -217,8 +217,6 @@ static int mergable_maps(struct extent_map *prev, struct extent_map *next)
 	      prev->block_start == EXTENT_MAP_HOLE) ||
 	     (next->block_start == EXTENT_MAP_INLINE &&
 	      prev->block_start == EXTENT_MAP_INLINE) ||
-	     (next->block_start == EXTENT_MAP_DELALLOC &&
-	      prev->block_start == EXTENT_MAP_DELALLOC) ||
 	     (next->block_start < EXTENT_MAP_LAST_BYTE - 1 &&
 	      next->block_start == extent_map_block_end(prev)))) {
 		return 1;
-- 
2.17.1

Re: [PATCH] btrfs: Remove impossible condition from mergable_maps

[David Sterba]

On Tue, Dec 18, 2018 at 03:10:02PM +0200, Nikolay Borisov wrote:
> We can never have extents marked as EXTENT_MAP_DELALLOC since this
> value is only ever used by btrfs_get_extent_fiemap. In this case the
> extent map is created by btrfs_get_extent_fiemap and is never really
> published, this flag is used to return the corresponding userspace one.
> Considering this, it's pointless having a check for EXTENT_MAP_DELALLOC
> in mergable_maps. Just remove it.

An assert would be good and a comment at EXTENT_MAP_DELALLOC definition
repeating what's in the changelog. Otherwise ok.

Re: [PATCH] btrfs: Remove impossible condition from mergable_maps

[Nikolay Borisov]

On 7.01.19 г. 21:10 ч., David Sterba wrote:
> On Tue, Dec 18, 2018 at 03:10:02PM +0200, Nikolay Borisov wrote:
>> We can never have extents marked as EXTENT_MAP_DELALLOC since this
>> value is only ever used by btrfs_get_extent_fiemap. In this case the
>> extent map is created by btrfs_get_extent_fiemap and is never really
>> published, this flag is used to return the corresponding userspace one.
>> Considering this, it's pointless having a check for EXTENT_MAP_DELALLOC
>> in mergable_maps. Just remove it.
> 
> An assert would be good and a comment at EXTENT_MAP_DELALLOC definition
> repeating what's in the changelog. Otherwise ok.

Do we really need the assert though, it's plainly obvious
EXTENT_MAP_DELALLOC is only set in a single function?

> 

Re: [PATCH] btrfs: Remove impossible condition from mergable_maps

[Qu Wenruo]

On 2019/1/8 下午3:30, Nikolay Borisov wrote:
> 
> 
> On 7.01.19 г. 21:10 ч., David Sterba wrote:
>> On Tue, Dec 18, 2018 at 03:10:02PM +0200, Nikolay Borisov wrote:
>>> We can never have extents marked as EXTENT_MAP_DELALLOC since this
>>> value is only ever used by btrfs_get_extent_fiemap. In this case the
>>> extent map is created by btrfs_get_extent_fiemap and is never really
>>> published, this flag is used to return the corresponding userspace one.
>>> Considering this, it's pointless having a check for EXTENT_MAP_DELALLOC
>>> in mergable_maps. Just remove it.
>>
>> An assert would be good and a comment at EXTENT_MAP_DELALLOC definition
>> repeating what's in the changelog. Otherwise ok.
> 
> Do we really need the assert though, it's plainly obvious
> EXTENT_MAP_DELALLOC is only set in a single function?

IMHO ASSERT() is not for now, but for the future.
Later on maybe some modification would change when and who sets DELALLOC
bit.
(And that's why there are a lot of seemingly stupid ASSERT(), it won't
hurt anyway).

Thanks,
Qu

> 
>>

Re: [PATCH] btrfs: Remove impossible condition from mergable_maps

[David Sterba]

On Tue, Jan 08, 2019 at 03:47:13PM +0800, Qu Wenruo wrote:
> On 2019/1/8 下午3:30, Nikolay Borisov wrote:
> > On 7.01.19 г. 21:10 ч., David Sterba wrote:
> >> On Tue, Dec 18, 2018 at 03:10:02PM +0200, Nikolay Borisov wrote:
> >>> We can never have extents marked as EXTENT_MAP_DELALLOC since this
> >>> value is only ever used by btrfs_get_extent_fiemap. In this case the
> >>> extent map is created by btrfs_get_extent_fiemap and is never really
> >>> published, this flag is used to return the corresponding userspace one.
> >>> Considering this, it's pointless having a check for EXTENT_MAP_DELALLOC
> >>> in mergable_maps. Just remove it.
> >>
> >> An assert would be good and a comment at EXTENT_MAP_DELALLOC definition
> >> repeating what's in the changelog. Otherwise ok.
> > 
> > Do we really need the assert though, it's plainly obvious
> > EXTENT_MAP_DELALLOC is only set in a single function?
> 
> IMHO ASSERT() is not for now, but for the future.
> Later on maybe some modification would change when and who sets DELALLOC
> bit.

Agreed, that's exactly the point. We want to catch when DELALLOC is
accidentally found during normal extent merging.

[PATCH v2] btrfs: Remove impossible condition from mergable_maps

[Nikolay Borisov]

We can never have extents marked as EXTENT_MAP_DELALLOC since this
value is only ever used by btrfs_get_extent_fiemap. In this case the
extent map is created by btrfs_get_extent_fiemap and is never really
published, this flag is used to return the corresponding userspace one.
Considering this, it's pointless having a check for EXTENT_MAP_DELALLOC
in mergable_maps. Just remove it.

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

V2: 
 * Add assert in mergable_maps to ensure EXTENT_MAP_DELALLOC is not set
 * Clarifty that EXTENT_MAP_DELALLOC is only used during fiemap in a comment

 fs/btrfs/extent_map.c | 4 ++--
 fs/btrfs/extent_map.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index a042a193c120..c65f208acfb3 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -210,6 +210,8 @@ static int mergable_maps(struct extent_map *prev, struct extent_map *next)
 	if (!list_empty(&prev->list) || !list_empty(&next->list))
 		return 0;
 
+	ASSERT(next->block_start != EXTENT_MAP_DELALLOC &&
+	       prev->block_start != EXTENT_MAP_DELALLOC);
 	if (extent_map_end(prev) == next->start &&
 	    prev->flags == next->flags &&
 	    prev->bdev == next->bdev &&
@@ -217,8 +219,6 @@ static int mergable_maps(struct extent_map *prev, struct extent_map *next)
 	      prev->block_start == EXTENT_MAP_HOLE) ||
 	     (next->block_start == EXTENT_MAP_INLINE &&
 	      prev->block_start == EXTENT_MAP_INLINE) ||
-	     (next->block_start == EXTENT_MAP_DELALLOC &&
-	      prev->block_start == EXTENT_MAP_DELALLOC) ||
 	     (next->block_start < EXTENT_MAP_LAST_BYTE - 1 &&
 	      next->block_start == extent_map_block_end(prev)))) {
 		return 1;
diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h
index ef05a0121652..9ef8766f478a 100644
--- a/fs/btrfs/extent_map.h
+++ b/fs/btrfs/extent_map.h
@@ -9,7 +9,7 @@
 #define EXTENT_MAP_LAST_BYTE ((u64)-4)
 #define EXTENT_MAP_HOLE ((u64)-3)
 #define EXTENT_MAP_INLINE ((u64)-2)
-#define EXTENT_MAP_DELALLOC ((u64)-1)
+#define EXTENT_MAP_DELALLOC ((u64)-1) /* used only during fiemap calls */
 
 /* bits for the extent_map::flags field */
 enum {
-- 
2.17.1

Re: [PATCH v2] btrfs: Remove impossible condition from mergable_maps

[David Sterba]

On Tue, Jan 08, 2019 at 04:53:46PM +0200, Nikolay Borisov wrote:
> We can never have extents marked as EXTENT_MAP_DELALLOC since this
> value is only ever used by btrfs_get_extent_fiemap. In this case the
> extent map is created by btrfs_get_extent_fiemap and is never really
> published, this flag is used to return the corresponding userspace one.
> Considering this, it's pointless having a check for EXTENT_MAP_DELALLOC
> in mergable_maps. Just remove it.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

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