Linux-BTRFS Archive on lore.kernel.org
 help / Atom feed
* [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	[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	[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, back to index

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

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