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