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