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@xxxxxxxxxx> 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.
> > >
> > > 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@xxxxxxxxxxxx>
> > > Reviewed-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
> > > Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@xxxxxxx>
> > > ---
> > >
> > > 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.
> > > + */
> > > + if (S_ISBLK(inode->i_mode) && !lo->lo_encrypt_key_size) {
> > > + backingq = bdev_get_queue(inode->i_bdev);
> > > + blk_queue_max_discard_sectors(q,
> > > + backingq->limits.max_write_zeroes_sectors);
> >
> > max_discard_sectors?
>
> I didn't plumb max_discard_sectors because for my scenario it never
> ends up hitting the block device that way.
>
> The loop device either uses FL_ZERO_RANGE or FL_PUNCH_HOLE. When
> backed by a block device, that ends up in blkdev_fallocate(), which
> always translates both of those into blkdev_issue_zeroout(), not
> blkdev_issue_discard(). So it's really the zeroing capabilities of the
> block device that matters, even for loop discard operations. It seems
> weird, but I think this is the right thing because it presents a
> consistent interface to loop device users whether backed by a file
> system file, or directly by a block device. That is, a previously
> discarded range will read back as zeroes.
Ah, right. Could you add this paragraph as a comment explaining why
we're setting max_discard_sectors from max_write_zeroes_sectors?
--D
> -Evan
