* [PATCH] btrfs: Remove impossible condition from mergable_maps
@ 2018-12-18 13:10 Nikolay Borisov
2019-01-07 19:10 ` David Sterba
0 siblings, 1 reply; 7+ messages in thread
From: Nikolay Borisov @ 2018-12-18 13:10 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: Remove impossible condition from mergable_maps
2018-12-18 13:10 [PATCH] btrfs: Remove impossible condition from mergable_maps Nikolay Borisov
@ 2019-01-07 19:10 ` David Sterba
2019-01-08 7:30 ` Nikolay Borisov
2019-01-08 14:53 ` [PATCH v2] " Nikolay Borisov
0 siblings, 2 replies; 7+ messages in thread
From: David Sterba @ 2019-01-07 19:10 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: Remove impossible condition from mergable_maps
2019-01-07 19:10 ` David Sterba
@ 2019-01-08 7:30 ` Nikolay Borisov
2019-01-08 7:47 ` Qu Wenruo
2019-01-08 14:53 ` [PATCH v2] " Nikolay Borisov
1 sibling, 1 reply; 7+ messages in thread
From: Nikolay Borisov @ 2019-01-08 7:30 UTC (permalink / raw)
To: dsterba, linux-btrfs
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?
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: Remove impossible condition from mergable_maps
2019-01-08 7:30 ` Nikolay Borisov
@ 2019-01-08 7:47 ` Qu Wenruo
2019-01-08 14:28 ` David Sterba
0 siblings, 1 reply; 7+ messages in thread
From: Qu Wenruo @ 2019-01-08 7:47 UTC (permalink / raw)
To: Nikolay Borisov, dsterba, linux-btrfs
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
>
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: Remove impossible condition from mergable_maps
2019-01-08 7:47 ` Qu Wenruo
@ 2019-01-08 14:28 ` David Sterba
0 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2019-01-08 14:28 UTC (permalink / raw)
To: Qu Wenruo; +Cc: Nikolay Borisov, dsterba, linux-btrfs
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] btrfs: Remove impossible condition from mergable_maps
2019-01-07 19:10 ` David Sterba
2019-01-08 7:30 ` Nikolay Borisov
@ 2019-01-08 14:53 ` Nikolay Borisov
2019-01-09 14:00 ` David Sterba
1 sibling, 1 reply; 7+ messages in thread
From: Nikolay Borisov @ 2019-01-08 14:53 UTC (permalink / raw)
To: dsterba; +Cc: linux-btrfs, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] btrfs: Remove impossible condition from mergable_maps
2019-01-08 14:53 ` [PATCH v2] " Nikolay Borisov
@ 2019-01-09 14:00 ` David Sterba
0 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2019-01-09 14:00 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: dsterba, linux-btrfs
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-01-09 14:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-18 13:10 [PATCH] btrfs: Remove impossible condition from mergable_maps Nikolay Borisov
2019-01-07 19:10 ` David Sterba
2019-01-08 7:30 ` Nikolay Borisov
2019-01-08 7:47 ` Qu Wenruo
2019-01-08 14:28 ` David Sterba
2019-01-08 14:53 ` [PATCH v2] " Nikolay Borisov
2019-01-09 14:00 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).