All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] btrfs: trim: Fix a bug certain range may not be trimmed properly
@ 2019-10-23  8:50 Qu Wenruo
  2019-10-23  8:50 ` [PATCH 1/2] btrfs: volumes: Return the mapped length for discard Qu Wenruo
  2019-10-23  8:50 ` [PATCH 2/2] btrfs: extent-tree: Ensure we trim ranges across block group boundary Qu Wenruo
  0 siblings, 2 replies; 7+ messages in thread
From: Qu Wenruo @ 2019-10-23  8:50 UTC (permalink / raw)
  To: linux-btrfs

There is a bug report about discard mount option not trimming some range
properly, and causing unexpected space usage for thin device.

It turns out to be that, if there are pinned extents across block group
boundary, we will only trim to the end of current block group, skipping
the remaining.

This patchset will fix it by ensuring btrfs_discard_extent() will
iterate the full range before exiting.

Meanwhile I'm still looking into how to craft such test case for btrfs,
so the test case may be late for several days.

Qu Wenruo (2):
  btrfs: volumes: Return the mapped length for discard
  btrfs: extent-tree: Ensure we trim ranges across block group boundary

 fs/btrfs/extent-tree.c | 40 ++++++++++++++++++++++++++++++----------
 fs/btrfs/volumes.c     |  8 +++++---
 2 files changed, 35 insertions(+), 13 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] btrfs: volumes: Return the mapped length for discard
  2019-10-23  8:50 [PATCH 0/2] btrfs: trim: Fix a bug certain range may not be trimmed properly Qu Wenruo
@ 2019-10-23  8:50 ` Qu Wenruo
  2019-10-23  9:47   ` Filipe Manana
  2019-10-23  8:50 ` [PATCH 2/2] btrfs: extent-tree: Ensure we trim ranges across block group boundary Qu Wenruo
  1 sibling, 1 reply; 7+ messages in thread
From: Qu Wenruo @ 2019-10-23  8:50 UTC (permalink / raw)
  To: linux-btrfs

For btrfs_map_block(), if we pass @op == BTRFS_MAP_DISCARD, the @length
parameter will not be updated to reflect the real mapped length.

This means for discard operation, we won't know how many bytes are
really mapped.

Fix this by changing assigning the mapped range length to @length
pointer, so btrfs_map_block() for BTRFS_MAP_DISCARD also return mapped
length.

During the change, also do a minor modification to make the length
calculation a little more straightforward.
Instead of previously calculated @offset, use "em->end - bytenr" to
calculate the actually mapped length.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/volumes.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index cdd7af424033..f66bd0d03f44 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5578,12 +5578,13 @@ void btrfs_put_bbio(struct btrfs_bio *bbio)
  * replace.
  */
 static int __btrfs_map_block_for_discard(struct btrfs_fs_info *fs_info,
-					 u64 logical, u64 length,
+					 u64 logical, u64 *length_ret,
 					 struct btrfs_bio **bbio_ret)
 {
 	struct extent_map *em;
 	struct map_lookup *map;
 	struct btrfs_bio *bbio;
+	u64 length = *length_ret;
 	u64 offset;
 	u64 stripe_nr;
 	u64 stripe_nr_end;
@@ -5616,7 +5617,8 @@ static int __btrfs_map_block_for_discard(struct btrfs_fs_info *fs_info,
 	}
 
 	offset = logical - em->start;
-	length = min_t(u64, em->len - offset, length);
+	length = min_t(u64, em->start + em->len - logical, length);
+	*length_ret = length;
 
 	stripe_len = map->stripe_len;
 	/*
@@ -6031,7 +6033,7 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
 
 	if (op == BTRFS_MAP_DISCARD)
 		return __btrfs_map_block_for_discard(fs_info, logical,
-						     *length, bbio_ret);
+						     length, bbio_ret);
 
 	ret = btrfs_get_io_geometry(fs_info, op, logical, *length, &geom);
 	if (ret < 0)
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] btrfs: extent-tree: Ensure we trim ranges across block group boundary
  2019-10-23  8:50 [PATCH 0/2] btrfs: trim: Fix a bug certain range may not be trimmed properly Qu Wenruo
  2019-10-23  8:50 ` [PATCH 1/2] btrfs: volumes: Return the mapped length for discard Qu Wenruo
@ 2019-10-23  8:50 ` Qu Wenruo
  2019-10-23  9:51   ` Filipe Manana
  1 sibling, 1 reply; 7+ messages in thread
From: Qu Wenruo @ 2019-10-23  8:50 UTC (permalink / raw)
  To: linux-btrfs

[BUG]
When deleting large files (which cross block group boundary) with discard
mount option, we find some btrfs_discard_extent() calls only trimmed part
of its space, not the whole range:

  btrfs_discard_extent: type=0x1 start=19626196992 len=2144530432 trimmed=1073741824 ratio=50%

type:		bbio->map_type, in above case, it's SINGLE DATA.
start:		Logical address of this trim
len:		Logical length of this trim
trimmed:	Physically trimmed bytes
ratio:		trimmed / len

Thus leading some unused space not discarded.

[CAUSE]
When discard mount option is specified, after a transaction is fully
committed (super block written to disk), we begin to cleanup pinned
extents in the following call chain:
btrfs_commit_transaction()
|- write_all_supers()
|- btrfs_finish_extent_commit()
   |- find_first_extent_bit(unpin, 0, &start, &end, EXTENT_DIRTY);
   |- btrfs_discard_extent()

However pinned extents are recorded in an extent_io_tree, which can
merge same extent states.

When a large file get deleted and it has adjacent file extents across
block group boundary, we will get a large merged range.

Then when we pass the large range into btrfs_discard_extent(),
btrfs_discard_extent() will just trim the first part, without trimming
the remaining part.

Furthermore, this bug is not that reliably observed, as if the whole
block group is empty, there will be another trim for that block group.

So the most obvious way to find this missing trim needs to delete large
extents at block group boundary without empting involved block groups.

[FIX]
Fix this bug by calling btrfs_map_block() in a loop, until we reach the
end location.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent-tree.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 49cb26fa7c63..45df45fa775b 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -1306,8 +1306,10 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
 int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
 			 u64 num_bytes, u64 *actual_bytes)
 {
-	int ret;
+	int ret = 0;
 	u64 discarded_bytes = 0;
+	u64 end = bytenr + num_bytes;
+	u64 cur = bytenr;
 	struct btrfs_bio *bbio = NULL;
 
 
@@ -1316,15 +1318,22 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
 	 * associated to its stripes that don't go away while we are discarding.
 	 */
 	btrfs_bio_counter_inc_blocked(fs_info);
-	/* Tell the block device(s) that the sectors can be discarded */
-	ret = btrfs_map_block(fs_info, BTRFS_MAP_DISCARD, bytenr, &num_bytes,
-			      &bbio, 0);
-	/* Error condition is -ENOMEM */
-	if (!ret) {
-		struct btrfs_bio_stripe *stripe = bbio->stripes;
+	while (cur < end) {
+		struct btrfs_bio_stripe *stripe;
 		int i;
 
+		/* Tell the block device(s) that the sectors can be discarded */
+		ret = btrfs_map_block(fs_info, BTRFS_MAP_DISCARD, cur,
+				      &num_bytes, &bbio, 0);
+		/*
+		 * Error can be -ENOMEM, -ENOENT (no such chunk mapping) or
+		 * -EOPNOTSUPP. For any such error, @num_bytes is not updated,
+		 * thus we can't continue anyway.
+		 */
+		if (ret < 0)
+			goto out;
 
+		stripe = bbio->stripes;
 		for (i = 0; i < bbio->num_stripes; i++, stripe++) {
 			u64 bytes;
 			struct request_queue *req_q;
@@ -1341,10 +1350,19 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
 						  stripe->physical,
 						  stripe->length,
 						  &bytes);
-			if (!ret)
+			if (!ret) {
 				discarded_bytes += bytes;
-			else if (ret != -EOPNOTSUPP)
-				break; /* Logic errors or -ENOMEM, or -EIO but I don't know how that could happen JDM */
+			} else if (ret != -EOPNOTSUPP) {
+				/*
+				 * Logic errors or -ENOMEM, or -EIO but I don't
+				 * know how that could happen JDM
+				 *
+				 * Ans since there are two loops, explicitly
+				 * goto out to avoid confusion.
+				 */
+				btrfs_put_bbio(bbio);
+				goto out;
+			}
 
 			/*
 			 * Just in case we get back EOPNOTSUPP for some reason,
@@ -1354,7 +1372,9 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
 			ret = 0;
 		}
 		btrfs_put_bbio(bbio);
+		cur += num_bytes;
 	}
+out:
 	btrfs_bio_counter_dec(fs_info);
 
 	if (actual_bytes)
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] btrfs: volumes: Return the mapped length for discard
  2019-10-23  8:50 ` [PATCH 1/2] btrfs: volumes: Return the mapped length for discard Qu Wenruo
@ 2019-10-23  9:47   ` Filipe Manana
  2019-10-23  9:51     ` Qu Wenruo
  0 siblings, 1 reply; 7+ messages in thread
From: Filipe Manana @ 2019-10-23  9:47 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Wed, Oct 23, 2019 at 9:53 AM Qu Wenruo <wqu@suse.com> wrote:
>

Hi Qu,

> For btrfs_map_block(), if we pass @op == BTRFS_MAP_DISCARD, the @length
> parameter will not be updated to reflect the real mapped length.
>
> This means for discard operation, we won't know how many bytes are
> really mapped.

Ok, and what's the consequence? The changelog should really say what
is the problem, what the bug is.
The cover letter and the second patch explain what problems are being
solved, but not this change.

>
> Fix this by changing assigning the mapped range length to @length
> pointer, so btrfs_map_block() for BTRFS_MAP_DISCARD also return mapped
> length.
>
> During the change, also do a minor modification to make the length
> calculation a little more straightforward.
> Instead of previously calculated @offset, use "em->end - bytenr" to
> calculate the actually mapped length.

I really don't like much mixing a cleanup with a fix. I would prefer
two separate patches, no matter how small or trivial it is.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Other than that, it looks good to me.
Thanks.

> ---
>  fs/btrfs/volumes.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index cdd7af424033..f66bd0d03f44 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -5578,12 +5578,13 @@ void btrfs_put_bbio(struct btrfs_bio *bbio)
>   * replace.
>   */
>  static int __btrfs_map_block_for_discard(struct btrfs_fs_info *fs_info,
> -                                        u64 logical, u64 length,
> +                                        u64 logical, u64 *length_ret,
>                                          struct btrfs_bio **bbio_ret)
>  {
>         struct extent_map *em;
>         struct map_lookup *map;
>         struct btrfs_bio *bbio;
> +       u64 length = *length_ret;
>         u64 offset;
>         u64 stripe_nr;
>         u64 stripe_nr_end;
> @@ -5616,7 +5617,8 @@ static int __btrfs_map_block_for_discard(struct btrfs_fs_info *fs_info,
>         }
>
>         offset = logical - em->start;
> -       length = min_t(u64, em->len - offset, length);
> +       length = min_t(u64, em->start + em->len - logical, length);
> +       *length_ret = length;
>
>         stripe_len = map->stripe_len;
>         /*
> @@ -6031,7 +6033,7 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
>
>         if (op == BTRFS_MAP_DISCARD)
>                 return __btrfs_map_block_for_discard(fs_info, logical,
> -                                                    *length, bbio_ret);
> +                                                    length, bbio_ret);
>
>         ret = btrfs_get_io_geometry(fs_info, op, logical, *length, &geom);
>         if (ret < 0)
> --
> 2.23.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] btrfs: extent-tree: Ensure we trim ranges across block group boundary
  2019-10-23  8:50 ` [PATCH 2/2] btrfs: extent-tree: Ensure we trim ranges across block group boundary Qu Wenruo
@ 2019-10-23  9:51   ` Filipe Manana
  0 siblings, 0 replies; 7+ messages in thread
From: Filipe Manana @ 2019-10-23  9:51 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Wed, Oct 23, 2019 at 9:52 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [BUG]
> When deleting large files (which cross block group boundary) with discard
> mount option, we find some btrfs_discard_extent() calls only trimmed part
> of its space, not the whole range:
>
>   btrfs_discard_extent: type=0x1 start=19626196992 len=2144530432 trimmed=1073741824 ratio=50%
>
> type:           bbio->map_type, in above case, it's SINGLE DATA.
> start:          Logical address of this trim
> len:            Logical length of this trim
> trimmed:        Physically trimmed bytes
> ratio:          trimmed / len
>
> Thus leading some unused space not discarded.
>
> [CAUSE]
> When discard mount option is specified, after a transaction is fully
> committed (super block written to disk), we begin to cleanup pinned
> extents in the following call chain:
> btrfs_commit_transaction()
> |- write_all_supers()
> |- btrfs_finish_extent_commit()
>    |- find_first_extent_bit(unpin, 0, &start, &end, EXTENT_DIRTY);
>    |- btrfs_discard_extent()
>
> However pinned extents are recorded in an extent_io_tree, which can
> merge same extent states.

same -> adjacent
'same' would imply duplicate entries (same start offset and lengths)

>
> When a large file get deleted and it has adjacent file extents across
> block group boundary, we will get a large merged range.
>
> Then when we pass the large range into btrfs_discard_extent(),
> btrfs_discard_extent() will just trim the first part, without trimming
> the remaining part.
>
> Furthermore, this bug is not that reliably observed, as if the whole
> block group is empty, there will be another trim for that block group.
>
> So the most obvious way to find this missing trim needs to delete large
> extents at block group boundary without empting involved block groups.
>
> [FIX]
> Fix this bug by calling btrfs_map_block() in a loop, until we reach the
> end location.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

With that small change:

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

> ---
>  fs/btrfs/extent-tree.c | 40 ++++++++++++++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 49cb26fa7c63..45df45fa775b 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -1306,8 +1306,10 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len,
>  int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
>                          u64 num_bytes, u64 *actual_bytes)
>  {
> -       int ret;
> +       int ret = 0;
>         u64 discarded_bytes = 0;
> +       u64 end = bytenr + num_bytes;
> +       u64 cur = bytenr;
>         struct btrfs_bio *bbio = NULL;
>
>
> @@ -1316,15 +1318,22 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
>          * associated to its stripes that don't go away while we are discarding.
>          */
>         btrfs_bio_counter_inc_blocked(fs_info);
> -       /* Tell the block device(s) that the sectors can be discarded */
> -       ret = btrfs_map_block(fs_info, BTRFS_MAP_DISCARD, bytenr, &num_bytes,
> -                             &bbio, 0);
> -       /* Error condition is -ENOMEM */
> -       if (!ret) {
> -               struct btrfs_bio_stripe *stripe = bbio->stripes;
> +       while (cur < end) {
> +               struct btrfs_bio_stripe *stripe;
>                 int i;
>
> +               /* Tell the block device(s) that the sectors can be discarded */
> +               ret = btrfs_map_block(fs_info, BTRFS_MAP_DISCARD, cur,
> +                                     &num_bytes, &bbio, 0);
> +               /*
> +                * Error can be -ENOMEM, -ENOENT (no such chunk mapping) or
> +                * -EOPNOTSUPP. For any such error, @num_bytes is not updated,
> +                * thus we can't continue anyway.
> +                */
> +               if (ret < 0)
> +                       goto out;
>
> +               stripe = bbio->stripes;
>                 for (i = 0; i < bbio->num_stripes; i++, stripe++) {
>                         u64 bytes;
>                         struct request_queue *req_q;
> @@ -1341,10 +1350,19 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
>                                                   stripe->physical,
>                                                   stripe->length,
>                                                   &bytes);
> -                       if (!ret)
> +                       if (!ret) {
>                                 discarded_bytes += bytes;
> -                       else if (ret != -EOPNOTSUPP)
> -                               break; /* Logic errors or -ENOMEM, or -EIO but I don't know how that could happen JDM */
> +                       } else if (ret != -EOPNOTSUPP) {
> +                               /*
> +                                * Logic errors or -ENOMEM, or -EIO but I don't
> +                                * know how that could happen JDM
> +                                *
> +                                * Ans since there are two loops, explicitly
> +                                * goto out to avoid confusion.
> +                                */
> +                               btrfs_put_bbio(bbio);
> +                               goto out;
> +                       }
>
>                         /*
>                          * Just in case we get back EOPNOTSUPP for some reason,
> @@ -1354,7 +1372,9 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
>                         ret = 0;
>                 }
>                 btrfs_put_bbio(bbio);
> +               cur += num_bytes;
>         }
> +out:
>         btrfs_bio_counter_dec(fs_info);
>
>         if (actual_bytes)
> --
> 2.23.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] btrfs: volumes: Return the mapped length for discard
  2019-10-23  9:47   ` Filipe Manana
@ 2019-10-23  9:51     ` Qu Wenruo
  2019-10-23  9:56       ` Filipe Manana
  0 siblings, 1 reply; 7+ messages in thread
From: Qu Wenruo @ 2019-10-23  9:51 UTC (permalink / raw)
  To: fdmanana, Qu Wenruo; +Cc: linux-btrfs


[-- Attachment #1.1: Type: text/plain, Size: 3292 bytes --]



On 2019/10/23 下午5:47, Filipe Manana wrote:
> On Wed, Oct 23, 2019 at 9:53 AM Qu Wenruo <wqu@suse.com> wrote:
>>
> 
> Hi Qu,
> 
>> For btrfs_map_block(), if we pass @op == BTRFS_MAP_DISCARD, the @length
>> parameter will not be updated to reflect the real mapped length.
>>
>> This means for discard operation, we won't know how many bytes are
>> really mapped.
> 
> Ok, and what's the consequence? The changelog should really say what
> is the problem, what the bug is.
> The cover letter and the second patch explain what problems are being
> solved, but not this change.

The problem is, no consequence at all, until the 2nd patch is taken in
to consideration.

This patch itself doesn't make any sense, just a plain dependency for
the 2nd patch.

I guess it's better to fold these two patches into one patch?

> 
>>
>> Fix this by changing assigning the mapped range length to @length
>> pointer, so btrfs_map_block() for BTRFS_MAP_DISCARD also return mapped
>> length.
>>
>> During the change, also do a minor modification to make the length
>> calculation a little more straightforward.
>> Instead of previously calculated @offset, use "em->end - bytenr" to
>> calculate the actually mapped length.
> 
> I really don't like much mixing a cleanup with a fix. I would prefer
> two separate patches, no matter how small or trivial it is.

Sure.

Thanks,
Qu

>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Other than that, it looks good to me.
> Thanks.
> 
>> ---
>>  fs/btrfs/volumes.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index cdd7af424033..f66bd0d03f44 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -5578,12 +5578,13 @@ void btrfs_put_bbio(struct btrfs_bio *bbio)
>>   * replace.
>>   */
>>  static int __btrfs_map_block_for_discard(struct btrfs_fs_info *fs_info,
>> -                                        u64 logical, u64 length,
>> +                                        u64 logical, u64 *length_ret,
>>                                          struct btrfs_bio **bbio_ret)
>>  {
>>         struct extent_map *em;
>>         struct map_lookup *map;
>>         struct btrfs_bio *bbio;
>> +       u64 length = *length_ret;
>>         u64 offset;
>>         u64 stripe_nr;
>>         u64 stripe_nr_end;
>> @@ -5616,7 +5617,8 @@ static int __btrfs_map_block_for_discard(struct btrfs_fs_info *fs_info,
>>         }
>>
>>         offset = logical - em->start;
>> -       length = min_t(u64, em->len - offset, length);
>> +       length = min_t(u64, em->start + em->len - logical, length);
>> +       *length_ret = length;
>>
>>         stripe_len = map->stripe_len;
>>         /*
>> @@ -6031,7 +6033,7 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
>>
>>         if (op == BTRFS_MAP_DISCARD)
>>                 return __btrfs_map_block_for_discard(fs_info, logical,
>> -                                                    *length, bbio_ret);
>> +                                                    length, bbio_ret);
>>
>>         ret = btrfs_get_io_geometry(fs_info, op, logical, *length, &geom);
>>         if (ret < 0)
>> --
>> 2.23.0
>>
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] btrfs: volumes: Return the mapped length for discard
  2019-10-23  9:51     ` Qu Wenruo
@ 2019-10-23  9:56       ` Filipe Manana
  0 siblings, 0 replies; 7+ messages in thread
From: Filipe Manana @ 2019-10-23  9:56 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: Qu Wenruo, linux-btrfs

On Wed, Oct 23, 2019 at 10:51 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> On 2019/10/23 下午5:47, Filipe Manana wrote:
> > On Wed, Oct 23, 2019 at 9:53 AM Qu Wenruo <wqu@suse.com> wrote:
> >>
> >
> > Hi Qu,
> >
> >> For btrfs_map_block(), if we pass @op == BTRFS_MAP_DISCARD, the @length
> >> parameter will not be updated to reflect the real mapped length.
> >>
> >> This means for discard operation, we won't know how many bytes are
> >> really mapped.
> >
> > Ok, and what's the consequence? The changelog should really say what
> > is the problem, what the bug is.
> > The cover letter and the second patch explain what problems are being
> > solved, but not this change.
>
> The problem is, no consequence at all, until the 2nd patch is taken in
> to consideration.
>
> This patch itself doesn't make any sense, just a plain dependency for
> the 2nd patch.
>
> I guess it's better to fold these two patches into one patch?

I wouldn't mind about that.
Or, if you keep them separate, just mention in the changelog that it's
used by another change to fix the problem of a range spanning two or
more block groups getting partially trimmed only.

Thanks.

>
> >
> >>
> >> Fix this by changing assigning the mapped range length to @length
> >> pointer, so btrfs_map_block() for BTRFS_MAP_DISCARD also return mapped
> >> length.
> >>
> >> During the change, also do a minor modification to make the length
> >> calculation a little more straightforward.
> >> Instead of previously calculated @offset, use "em->end - bytenr" to
> >> calculate the actually mapped length.
> >
> > I really don't like much mixing a cleanup with a fix. I would prefer
> > two separate patches, no matter how small or trivial it is.
>
> Sure.
>
> Thanks,
> Qu
>
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >
> > Other than that, it looks good to me.
> > Thanks.
> >
> >> ---
> >>  fs/btrfs/volumes.c | 8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >> index cdd7af424033..f66bd0d03f44 100644
> >> --- a/fs/btrfs/volumes.c
> >> +++ b/fs/btrfs/volumes.c
> >> @@ -5578,12 +5578,13 @@ void btrfs_put_bbio(struct btrfs_bio *bbio)
> >>   * replace.
> >>   */
> >>  static int __btrfs_map_block_for_discard(struct btrfs_fs_info *fs_info,
> >> -                                        u64 logical, u64 length,
> >> +                                        u64 logical, u64 *length_ret,
> >>                                          struct btrfs_bio **bbio_ret)
> >>  {
> >>         struct extent_map *em;
> >>         struct map_lookup *map;
> >>         struct btrfs_bio *bbio;
> >> +       u64 length = *length_ret;
> >>         u64 offset;
> >>         u64 stripe_nr;
> >>         u64 stripe_nr_end;
> >> @@ -5616,7 +5617,8 @@ static int __btrfs_map_block_for_discard(struct btrfs_fs_info *fs_info,
> >>         }
> >>
> >>         offset = logical - em->start;
> >> -       length = min_t(u64, em->len - offset, length);
> >> +       length = min_t(u64, em->start + em->len - logical, length);
> >> +       *length_ret = length;
> >>
> >>         stripe_len = map->stripe_len;
> >>         /*
> >> @@ -6031,7 +6033,7 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
> >>
> >>         if (op == BTRFS_MAP_DISCARD)
> >>                 return __btrfs_map_block_for_discard(fs_info, logical,
> >> -                                                    *length, bbio_ret);
> >> +                                                    length, bbio_ret);
> >>
> >>         ret = btrfs_get_io_geometry(fs_info, op, logical, *length, &geom);
> >>         if (ret < 0)
> >> --
> >> 2.23.0
> >>
> >
> >
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-10-23  9:56 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-23  8:50 [PATCH 0/2] btrfs: trim: Fix a bug certain range may not be trimmed properly Qu Wenruo
2019-10-23  8:50 ` [PATCH 1/2] btrfs: volumes: Return the mapped length for discard Qu Wenruo
2019-10-23  9:47   ` Filipe Manana
2019-10-23  9:51     ` Qu Wenruo
2019-10-23  9:56       ` Filipe Manana
2019-10-23  8:50 ` [PATCH 2/2] btrfs: extent-tree: Ensure we trim ranges across block group boundary Qu Wenruo
2019-10-23  9:51   ` Filipe Manana

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.