Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220915164826.1396245-1-sarthakkukreti@google.com>
 <20220915164826.1396245-3-sarthakkukreti@google.com> <Yy3BXc9wf4PH6Rby@redhat.com>
In-Reply-To: <Yy3BXc9wf4PH6Rby@redhat.com>
From:   Sarthak Kukreti <sarthakkukreti@chromium.org>
Date:   Thu, 29 Dec 2022 00:22:32 -0800
Message-ID: <CAG9=OMMgtscWZyZYHsY0pp-9we+yxs=88sr_yMb6pfz7e75nyA@mail.gmail.com>
Subject: Re: [PATCH RFC 2/8] dm: Add support for block provisioning
To:     Mike Snitzer <snitzer@redhat.com>
Cc:     dm-devel@redhat.com, linux-block@vger.kernel.org,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
        virtualization@lists.linux-foundation.org,
        Jens Axboe <axboe@kernel.dk>,
        "Michael S . Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Stefan Hajnoczi <stefanha@redhat.com>,
        Alasdair Kergon <agk@redhat.com>,
        Mike Snitzer <snitzer@kernel.org>,
        "Theodore Ts'o" <tytso@mit.edu>,
        Andreas Dilger <adilger.kernel@dilger.ca>,
        Bart Van Assche <bvanassche@google.com>,
        Daniil Lunev <dlunev@google.com>,
        Evan Green <evgreen@google.com>,
        Gwendal Grignou <gwendal@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Sep 23, 2022 at 7:23 AM Mike Snitzer <snitzer@redhat.com> wrote:
>
> On Thu, Sep 15 2022 at 12:48P -0400,
> Sarthak Kukreti <sarthakkukreti@chromium.org> wrote:
>
> > From: Sarthak Kukreti <sarthakkukreti@chromium.org>
> >
> > Add support to dm devices for REQ_OP_PROVISION. The default mode
> > is to pass through the request and dm-thin will utilize it to provision
> > blocks.
> >
> > Signed-off-by: Sarthak Kukreti <sarthakkukreti@chromium.org>
> > ---
> >  drivers/md/dm-crypt.c         |  4 +-
> >  drivers/md/dm-linear.c        |  1 +
> >  drivers/md/dm-table.c         | 17 +++++++
> >  drivers/md/dm-thin.c          | 86 +++++++++++++++++++++++++++++++++--
> >  drivers/md/dm.c               |  4 ++
> >  include/linux/device-mapper.h |  6 +++
> >  6 files changed, 113 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> > index 159c6806c19b..357f0899cfb6 100644
> > --- a/drivers/md/dm-crypt.c
> > +++ b/drivers/md/dm-crypt.c
> > @@ -3081,6 +3081,8 @@ static int crypt_ctr_optional(struct dm_target *ti, unsigned int argc, char **ar
> >       if (ret)
> >               return ret;
> >
> > +     ti->num_provision_bios = 1;
> > +
> >       while (opt_params--) {
> >               opt_string = dm_shift_arg(&as);
> >               if (!opt_string) {
> > @@ -3384,7 +3386,7 @@ static int crypt_map(struct dm_target *ti, struct bio *bio)
> >        * - for REQ_OP_DISCARD caller must use flush if IO ordering matters
> >        */
> >       if (unlikely(bio->bi_opf & REQ_PREFLUSH ||
> > -         bio_op(bio) == REQ_OP_DISCARD)) {
> > +         bio_op(bio) == REQ_OP_DISCARD || bio_op(bio) == REQ_OP_PROVISION)) {
> >               bio_set_dev(bio, cc->dev->bdev);
> >               if (bio_sectors(bio))
> >                       bio->bi_iter.bi_sector = cc->start +
> > diff --git a/drivers/md/dm-linear.c b/drivers/md/dm-linear.c
> > index 3212ef6aa81b..1aa782149428 100644
> > --- a/drivers/md/dm-linear.c
> > +++ b/drivers/md/dm-linear.c
> > @@ -61,6 +61,7 @@ static int linear_ctr(struct dm_target *ti, unsigned int argc, char **argv)
> >       ti->num_discard_bios = 1;
> >       ti->num_secure_erase_bios = 1;
> >       ti->num_write_zeroes_bios = 1;
> > +     ti->num_provision_bios = 1;
> >       ti->private = lc;
> >       return 0;
> >
> > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > index 332f96b58252..b7f9cb66b7ba 100644
> > --- a/drivers/md/dm-table.c
> > +++ b/drivers/md/dm-table.c
> > @@ -1853,6 +1853,18 @@ static bool dm_table_supports_write_zeroes(struct dm_table *t)
> >       return true;
> >  }
> >
> > +static bool dm_table_supports_provision(struct dm_table *t)
> > +{
> > +     for (unsigned int i = 0; i < t->num_targets; i++) {
> > +             struct dm_target *ti = dm_table_get_target(t, i);
> > +
> > +             if (ti->num_provision_bios)
> > +                     return true;
> > +     }
> > +
> > +     return false;
> > +}
> > +
>
> This needs to go a step further and verify a device in the stack
> actually services REQ_OP_PROVISION.
>
> Please see dm_table_supports_discards(): it iterates all devices in
> the table and checks that support is advertised.
>
> For discard, DM requires that _all_ devices in a table advertise
> support (that is pretty strict and likely could be relaxed to _any_).
>
> You'll need ti->provision_supported (like ->discards_supported) to
> advertise actual support is provided by dm-thinp (even if underlying
> devices don't support it).
>
> And yeah, dm-thinp passdown support for REQ_OP_PROVISION can follow
> later as needed (if there actual HW that would benefit from
> REQ_OP_PROVISION).
>
Done, thanks (the provision support, not the passdown)! I think the
one case where passdown might help is to build images with dm-thinp
already set up on one of the partitions (I have something in the works
for ChromiumOS images to do VM tests with preset state :)). That would
allow us to preallocate space for thin logical volumes inside the
image file.

> Mike
>