Re: dm thin: fix pool target flags that control discard

[Mike Snitzer <snitzer@redhat.com>]

On Mon, Mar 26 2012 at 10:15am -0400,
Joe Thornber <thornber@redhat.com> wrote:

> On Fri, Mar 23, 2012 at 05:55:02PM -0400, Mike Snitzer wrote:
> > On Fri, Mar 23 2012 at  8:37am -0400,
> > Alasdair G Kergon <agk@redhat.com> wrote:
> > 
> > > On Fri, Mar 16, 2012 at 03:22:34PM +0000, Joe Thornber wrote:
> > > > +      'ignore_discard': disable discard support
> > > > +      'no_discard_passdown': don't pass discards down to the underlying data device
> > > 
> > > If someone reloads the pool target changing either of these options, how do connected
> > > thin targets pick up the change?  If they don't pick it up automatically, how do you
> > > determine whether they did or didn't pick it up - is that internal state not
> > > exposed to userspace yet?
> > 
> > Here are various fixes for dm_thin-add-pool-target-flags-to-control-discard.patch
> > 
> > o wire up 'discard_passdown' so that it does control whether or not the
> >   discard is passed to the pool's underlying data device
> 
> This already works, see test_{enable,disable}_passdown() tests here:
> 
>   https://github.com/jthornber/thinp-test-suite/blob/master/discard_tests.rb
> 
> You've got to remember that there are 2 different interacting targets:
> 
>  i) discard enabled in the 'thin' target will cause mappings to be removed from the btree.
> 
>  ii) discard enabled in the 'pool' device will cause discards to be passed down.
> 
> The following logic is in the pool_ctr:
> 
>         if (pf.discard_enabled && pf.discard_passdown) {
>                 ti->discards_supported = 1;
>                 ti->num_discard_requests = 1;
>         }

We reasoned through this already, your code did work.  But for the
benefit of others this is why it worked:

thin will process_discard() via bio being deferring in thin_bio_map() --
provided pf.discard_enabled.  Then thin will pass the discard on to the
pool device regardless of pf.discard_passdown.  dm-core will then drop
the discard because ti->num_discard_requests is 0; but that results in
-EOPNOTSUPP.

Thing is, we don't want to return -EOPNOTSUPP if passdown was disabled.
So I think my change is an improvement (because process_prepared_discard
will now properly bio_endio(m->bio, 0) the discard).

> > o disallow disabling discards if a pool was already configured with
> >   discards enabled (the default)
> >   - justification is inlined in the code
> >   - required pool_ctr knowing whether pool was created or not; so added
> >     'created' flag to __pool_find()
> 
> ack, this needs fixing.

We've discussed that it is easier to just disallow changing discard
configuration.  And that there was a bug in my early return.  So you'll
be sending a follow-up fixup patch.

> > o if 'discard_passdown' is enabled (the default) verify that the pool's
> >   data device supports discards; if not warn and disable discard_passdown
> 
> Why?  Do other targets do this?  (no)

The thin target is the first target to use ti->discards_supported.
Setting that will cause dm_table_supports_discards to skip checking if
the target's underlying device(s) natively support discards.

So passdown needs to only be allowed if the underlying data device
supports discard -- otherwise we'll get a bunch of failed discards from
the SCSI layer, etc.

> > o the pool device's discard support should _not_ be limited by whether
> >   'discard_passdown' is enabled or not.
> 
> Yes it should.

Yeah, like I said above your approach works (except for the
-EOPNOTSUPP).  I just happen to prefer dm-thin taking a more active role
by short-circuting the discard_passdown directly.