[1/2] btrfs: volumes: Return the mapped length for discard
diff mbox series

Message ID 20191023085037.14838-2-wqu@suse.com
State New
Headers show
Series
  • btrfs: trim: Fix a bug certain range may not be trimmed properly
Related show

Commit Message

Qu Wenruo Oct. 23, 2019, 8:50 a.m. UTC
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(-)

Comments

Filipe Manana Oct. 23, 2019, 9:47 a.m. UTC | #1
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
>
Qu Wenruo Oct. 23, 2019, 9:51 a.m. UTC | #2
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
>>
> 
>
Filipe Manana Oct. 23, 2019, 9:56 a.m. UTC | #3
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
> >>
> >
> >
>

Patch
diff mbox series

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)