On 2019/10/23 下午5:47, Filipe Manana wrote:
> On Wed, Oct 23, 2019 at 9:53 AM Qu Wenruo <wqu@xxxxxxxx> 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@xxxxxxxx>
>
> 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:
signature.asc
Description: OpenPGP digital signature
