Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191114235008.185111-1-evgreen@chromium.org> <20191114154903.v7.2.I4d476bddbf41a61422ad51502f4361e237d60ad4@changeid>
 <20191120022518.GU6235@magnolia> <CAE=gft4mjKc4QKFKxp2FX9G2rUMuE3_eDuW_3Oq7NqTYBQwEjg@mail.gmail.com>
 <20191120191302.GV6235@magnolia> <CAE=gft6x1TmkkNTj+gktYMkHcysYyuYL50cavYusQ7hd9zChvA@mail.gmail.com>
 <20191120194507.GW6235@magnolia> <CAE=gft4OcxPP7srBe_2bj8K_0jHGD8Ae_PbV1Rq-Nz4F8GtkQA@mail.gmail.com>
 <20191121212512.GA2981917@magnolia>
In-Reply-To: <20191121212512.GA2981917@magnolia>
From:   Evan Green <evgreen@chromium.org>
Date:   Thu, 21 Nov 2019 14:35:56 -0800
Message-ID: <CAE=gft4uLmLKexyCdRi2N8iJxzuBMXW-rcw+ob_pxBfyLVNijg@mail.gmail.com>
Subject: Re: [PATCH v7 2/2] loop: Better discard support for block devices
To:     "Darrick J. Wong" <darrick.wong@oracle.com>
Cc:     Jens Axboe <axboe@kernel.dk>,
        Martin K Petersen <martin.petersen@oracle.com>,
        Gwendal Grignou <gwendal@chromium.org>,
        Christoph Hellwig <hch@infradead.org>,
        Ming Lei <ming.lei@redhat.com>,
        Alexis Savery <asavery@chromium.org>,
        Douglas Anderson <dianders@chromium.org>,
        Bart Van Assche <bvanassche@acm.org>,
        Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>,
        linux-block <linux-block@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Nov 21, 2019 at 1:25 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Thu, Nov 21, 2019 at 01:18:51PM -0800, Evan Green wrote:
> > On Wed, Nov 20, 2019 at 11:45 AM Darrick J. Wong
> > <darrick.wong@oracle.com> wrote:
> > >
> > > On Wed, Nov 20, 2019 at 11:25:48AM -0800, Evan Green wrote:
> > > > On Wed, Nov 20, 2019 at 11:13 AM Darrick J. Wong
> > > > <darrick.wong@oracle.com> wrote:
> > > > >
> > > > > On Wed, Nov 20, 2019 at 10:56:30AM -0800, Evan Green wrote:
> > > > > > On Tue, Nov 19, 2019 at 6:25 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > > > > > >
> > > > > > > On Thu, Nov 14, 2019 at 03:50:08PM -0800, Evan Green wrote:
> > > > > > > > If the backing device for a loop device is itself a block device,
> > > > > > > > then mirror the "write zeroes" capabilities of the underlying
> > > > > > > > block device into the loop device. Copy this capability into both
> > > > > > > > max_write_zeroes_sectors and max_discard_sectors of the loop device.
> > > > > > > >
> > > > > > > > The reason for this is that REQ_OP_DISCARD on a loop device translates
> > > > > > > > into blkdev_issue_zeroout(), rather than blkdev_issue_discard(). This
> > > > > > > > presents a consistent interface for loop devices (that discarded data
> > > > > > > > is zeroed), regardless of the backing device type of the loop device.
> > > > > > > > There should be no behavior change for loop devices backed by regular
> > > > > > > > files.
> > > >
> > > > (marking this spot for below)
> > > >
> > > > > > > >
> > > > > > > > This change fixes blktest block/003, and removes an extraneous
> > > > > > > > error print in block/013 when testing on a loop device backed
> > > > > > > > by a block device that does not support discard.
> > > > > > > >
> > > > > > > > Signed-off-by: Evan Green <evgreen@chromium.org>
> > > > > > > > Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> > > > > > > > Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> > > > > > > > ---
> > > > > > > >
> > > > > > > > Changes in v7:
> > > > > > > > - Rebase on top of Darrick's patch
> > > > > > > > - Tweak opening line of commit description (Darrick)
> > > > > > > >
> > > > > > > > Changes in v6: None
> > > > > > > > Changes in v5:
> > > > > > > > - Don't mirror discard if lo_encrypt_key_size is non-zero (Gwendal)
> > > > > > > >
> > > > > > > > Changes in v4:
> > > > > > > > - Mirror blkdev's write_zeroes into loopdev's discard_sectors.
> > > > > > > >
> > > > > > > > Changes in v3:
> > > > > > > > - Updated commit description
> > > > > > > >
> > > > > > > > Changes in v2: None
> > > > > > > >
> > > > > > > >  drivers/block/loop.c | 40 +++++++++++++++++++++++++++++-----------
> > > > > > > >  1 file changed, 29 insertions(+), 11 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > > > > > > > index 6a9fe1f9fe84..e8f23e4b78f7 100644
> > > > > > > > --- a/drivers/block/loop.c
> > > > > > > > +++ b/drivers/block/loop.c
> > > > > > > > @@ -427,11 +427,12 @@ static int lo_fallocate(struct loop_device *lo, struct request *rq, loff_t pos,
> > > > > > > >        * information.
> > > > > > > >        */
> > > > > > > >       struct file *file = lo->lo_backing_file;
> > > > > > > > +     struct request_queue *q = lo->lo_queue;
> > > > > > > >       int ret;
> > > > > > > >
> > > > > > > >       mode |= FALLOC_FL_KEEP_SIZE;
> > > > > > > >
> > > > > > > > -     if ((!file->f_op->fallocate) || lo->lo_encrypt_key_size) {
> > > > > > > > +     if (!blk_queue_discard(q)) {
> > > > > > > >               ret = -EOPNOTSUPP;
> > > > > > > >               goto out;
> > > > > > > >       }
> > > > > > > > @@ -862,6 +863,21 @@ static void loop_config_discard(struct loop_device *lo)
> > > > > > > >       struct file *file = lo->lo_backing_file;
> > > > > > > >       struct inode *inode = file->f_mapping->host;
> > > > > > > >       struct request_queue *q = lo->lo_queue;
> > > > > > > > +     struct request_queue *backingq;
> > > > > > > > +
> > > > > > > > +     /*
> > > > > > > > +      * If the backing device is a block device, mirror its zeroing
> > > > > > > > +      * capability. REQ_OP_DISCARD translates to a zero-out even when backed
> > > > > > > > +      * by block devices to keep consistent behavior with file-backed loop
> > > > > > > > +      * devices.
> > > > > > > > +      */
> >
> > Wait, I went to make this change and realized there's already a comment here.
> >
> > I can tweak the language a bit, but this is pretty much what you wanted, right?
>
> Yep.
>

Jens, any opinions? I'm happy to spin one more time to clarify the
comment as follows if desired, or leave it as-is too!

        /*
         * If the backing device is a block device, mirror its zeroing
         * capability. Set the discard sectors to the block device's zeroing
         * capabilities because loop discards result in blkdev_issue_zeroout(),
         * not blkdev_issue_discard(). This maintains consistent behavior with
         * file-backed loop devices: discarded regions read back as zero.
         */