Re: fstrim discarding too many or wrong blocks on Linux 5.1, leading to data loss

[Andrea Gelmini <andrea.gelmini@linux.it>]

Hi Mike,
   I'm doing setup to replicate and test the condition. I see your
patch is already in the 5.2 dev kernel.
   I'm going to try with latest git, and see what happens. Anyway,
don't you this it would be good
   to have this patch ( 51b86f9a8d1c4bb4e3862ee4b4c5f46072f7520d )
anyway in the 5.1 stable branch?

Thanks a lot for your time,
Gelma

Il giorno mar 21 mag 2019 alle ore 22:12 Mike Snitzer
<snitzer@redhat.com> ha scritto:
>
> On Tue, May 21 2019 at  3:00pm -0400,
> Andrea Gelmini <andrea.gelmini@linux.it> wrote:
>
> > On Tue, May 21, 2019 at 06:46:20PM +0200, Michael Laß wrote:
> > > > I finished bisecting. Here’s the responsible commit:
> > > >
> > > > commit 61697a6abd24acba941359c6268a94f4afe4a53d
> > > > Author: Mike Snitzer <snitzer@redhat.com>
> > > > Date:   Fri Jan 18 14:19:26 2019 -0500
> > > >
> > > >    dm: eliminate 'split_discard_bios' flag from DM target interface
> > > >
> > > >    There is no need to have DM core split discards on behalf of a DM target
> > > >    now that blk_queue_split() handles splitting discards based on the
> > > >    queue_limits.  A DM target just needs to set max_discard_sectors,
> > > >    discard_granularity, etc, in queue_limits.
> > > >
> > > >    Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> > >
> > > Reverting that commit solves the issue for me on Linux 5.1.3. Would
> > that be an option until the root cause has been identified? I’d rather
> > not let more people run into this issue.
> >
> > Thanks a lot Michael, for your time/work.
> >
> > This kind of bisecting are very boring and time consuming.
> >
> > I CC: also the patch author.
>
> Thanks for cc'ing me, this thread didn't catch my eye.
>
> Sorry for your troubles.  Can you please try this patch?
>
> Thanks,
> Mike
>
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 1fb1333fefec..997385c1ca54 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1469,7 +1469,7 @@ static unsigned get_num_write_zeroes_bios(struct dm_target *ti)
>  static int __send_changing_extent_only(struct clone_info *ci, struct dm_target *ti,
>                                        unsigned num_bios)
>  {
> -       unsigned len = ci->sector_count;
> +       unsigned len;
>
>         /*
>          * Even though the device advertised support for this type of
> @@ -1480,6 +1480,8 @@ static int __send_changing_extent_only(struct clone_info *ci, struct dm_target *
>         if (!num_bios)
>                 return -EOPNOTSUPP;
>
> +       len = min((sector_t)ci->sector_count, max_io_len_target_boundary(ci->sector, ti));
> +
>         __send_duplicate_bios(ci, ti, num_bios, &len);
>
>         ci->sector += len;
>