| Message ID | 20200402170603.19649-3-andrzej.p@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Better discard support for block devices | expand |
On Thu, Apr 02, 2020 at 07:06:03PM +0200, Andrzej Pietrasiewicz wrote: > From: Evan Green <evgreen@chromium.org> > > 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@chromium.org> > Reviewed-by: Gwendal Grignou <gwendal@chromium.org> > Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com> > [used updated version of Evan's comment in loop_config_discard()] > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/block/loop.c | 41 ++++++++++++++++++++++++++++++----------- > 1 file changed, 30 insertions(+), 11 deletions(-) > > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index 6969be9a855a..d7f30338b8ec 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; > } > @@ -864,6 +865,22 @@ 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. 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. > + */ > + if (S_ISBLK(inode->i_mode) && !lo->lo_encrypt_key_size) { > + backingq = bdev_get_queue(inode->i_bdev); The backingq could move into this local scope. > + } else if ((!file->f_op->fallocate) || lo->lo_encrypt_key_size) { No need for the inner braces. But the actual functionality looks good to me.
Hi Christoph, W dniu 03.04.2020 o 08:39, Christoph Hellwig pisze: > On Thu, Apr 02, 2020 at 07:06:03PM +0200, Andrzej Pietrasiewicz wrote: >> From: Evan Green <evgreen@chromium.org> >> <snip> >> + struct request_queue *backingq; >> + >> + /* >> + * 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. >> + */ >> + if (S_ISBLK(inode->i_mode) && !lo->lo_encrypt_key_size) { >> + backingq = bdev_get_queue(inode->i_bdev); > > The backingq could move into this local scope. > >> + } else if ((!file->f_op->fallocate) || lo->lo_encrypt_key_size) { > > No need for the inner braces. > > But the actual functionality looks good to me. > Would you A-b or R-b if I corrected the two small issues which you found? Andrzej
On Fri, Apr 03, 2020 at 12:27:26PM +0200, Andrzej Pietrasiewicz wrote: > > The backingq could move into this local scope. > > > > > + } else if ((!file->f_op->fallocate) || lo->lo_encrypt_key_size) { > > > > No need for the inner braces. > > > > But the actual functionality looks good to me. > > > > Would you A-b or R-b if I corrected the two small issues which you found? Sure: Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/drivers/block/loop.c b/drivers/block/loop.c index 6969be9a855a..d7f30338b8ec 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; } @@ -864,6 +865,22 @@ 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. 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. + */ + 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); + + blk_queue_max_write_zeroes_sectors(q, + backingq->limits.max_write_zeroes_sectors); /* * We use punch hole to reclaim the free space used by the @@ -871,22 +888,24 @@ static void loop_config_discard(struct loop_device *lo) * encryption is enabled, because it may give an attacker * useful information. */ - if ((!file->f_op->fallocate) || - lo->lo_encrypt_key_size) { + } else if ((!file->f_op->fallocate) || lo->lo_encrypt_key_size) { q->limits.discard_granularity = 0; q->limits.discard_alignment = 0; blk_queue_max_discard_sectors(q, 0); blk_queue_max_write_zeroes_sectors(q, 0); - blk_queue_flag_clear(QUEUE_FLAG_DISCARD, q); - return; - } - q->limits.discard_granularity = inode->i_sb->s_blocksize; - q->limits.discard_alignment = 0; + } else { + q->limits.discard_granularity = inode->i_sb->s_blocksize; + q->limits.discard_alignment = 0; - blk_queue_max_discard_sectors(q, UINT_MAX >> 9); - blk_queue_max_write_zeroes_sectors(q, UINT_MAX >> 9); - blk_queue_flag_set(QUEUE_FLAG_DISCARD, q); + blk_queue_max_discard_sectors(q, UINT_MAX >> 9); + blk_queue_max_write_zeroes_sectors(q, UINT_MAX >> 9); + } + + if (q->limits.max_write_zeroes_sectors) + blk_queue_flag_set(QUEUE_FLAG_DISCARD, q); + else + blk_queue_flag_clear(QUEUE_FLAG_DISCARD, q); } static void loop_unprepare_queue(struct loop_device *lo)