Re: [PATCH] dm thin: fix pool target flags that control discard

[Joe Thornber <thornber@redhat.com>]

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;
        }


> 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.

> 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)

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

Yes it should.

- Joe