On Wed, Oct 23, 2019 at 10:51 AM Qu Wenruo <[email protected]> wrote: > > > > 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?
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 <[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 > >> > > > > > -- Filipe David Manana, “Whether you think you can, or you think you can't — you're right.”
