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;
From:   Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
To:     Evan Green <evgreen@chromium.org>, Jens Axboe <axboe@kernel.dk>,
        Martin K Petersen <martin.petersen@oracle.com>
CC:     Bart Van Assche <bvanassche@acm.org>,
        Gwendal Grignou <gwendal@chromium.org>,
        Alexis Savery <asavery@chromium.org>,
        Ming Lei <ming.lei@redhat.com>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 2/2] loop: Better discard support for block devices
Thread-Topic: [PATCH v5 2/2] loop: Better discard support for block devices
Thread-Index: AQHVBDnQte6MG5PpIUSpV+iVZADCQg==
Date:   Thu, 9 May 2019 16:15:02 +0000
Message-ID: <SN6PR04MB45272BA1AE5D477892A08AE486330@SN6PR04MB4527.namprd04.prod.outlook.com>
References: <20190506182736.21064-1-evgreen@chromium.org>
 <20190506182736.21064-3-evgreen@chromium.org>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: a5af0fbd-2d72-4d5e-dfdc-08d6d4997df9
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 May 2019 16:15:02.7095
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR04MB3806
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Looks good to me.=0A=
=0A=
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>=0A=
=0A=
On 05/06/2019 11:30 AM, Evan Green wrote:=0A=
> If the backing device for a loop device is a block device,=0A=
> then mirror the "write zeroes" capabilities of the underlying=0A=
> block device into the loop device. Copy this capability into both=0A=
> max_write_zeroes_sectors and max_discard_sectors of the loop device.=0A=
>=0A=
> The reason for this is that REQ_OP_DISCARD on a loop device translates=0A=
> into blkdev_issue_zeroout(), rather than blkdev_issue_discard(). This=0A=
> presents a consistent interface for loop devices (that discarded data=0A=
> is zeroed), regardless of the backing device type of the loop device.=0A=
> There should be no behavior change for loop devices backed by regular=0A=
> files.=0A=
>=0A=
> While in there, differentiate between REQ_OP_DISCARD and=0A=
> REQ_OP_WRITE_ZEROES, which are different for block devices,=0A=
> but which the loop device had just been lumping together, since=0A=
> they're largely the same for files.=0A=
>=0A=
> This change fixes blktest block/003, and removes an extraneous=0A=
> error print in block/013 when testing on a loop device backed=0A=
> by a block device that does not support discard.=0A=
>=0A=
> Signed-off-by: Evan Green <evgreen@chromium.org>=0A=
> ---=0A=
>=0A=
> Changes in v5:=0A=
> - Don't mirror discard if lo_encrypt_key_size is non-zero (Gwendal)=0A=
>=0A=
> Changes in v4:=0A=
> - Mirror blkdev's write_zeroes into loopdev's discard_sectors.=0A=
>=0A=
> Changes in v3:=0A=
> - Updated commit description=0A=
>=0A=
> Changes in v2: None=0A=
>=0A=
>   drivers/block/loop.c | 57 ++++++++++++++++++++++++++++----------------=
=0A=
>   1 file changed, 37 insertions(+), 20 deletions(-)=0A=
>=0A=
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c=0A=
> index bbf21ebeccd3..a147210ed009 100644=0A=
> --- a/drivers/block/loop.c=0A=
> +++ b/drivers/block/loop.c=0A=
> @@ -417,19 +417,14 @@ static int lo_read_transfer(struct loop_device *lo,=
 struct request *rq,=0A=
>   	return ret;=0A=
>   }=0A=
>=0A=
> -static int lo_discard(struct loop_device *lo, struct request *rq, loff_t=
 pos)=0A=
> +static int lo_discard(struct loop_device *lo, struct request *rq,=0A=
> +		int mode, loff_t pos)=0A=
>   {=0A=
> -	/*=0A=
> -	 * We use punch hole to reclaim the free space used by the=0A=
> -	 * image a.k.a. discard. However we do not support discard if=0A=
> -	 * encryption is enabled, because it may give an attacker=0A=
> -	 * useful information.=0A=
> -	 */=0A=
>   	struct file *file =3D lo->lo_backing_file;=0A=
> -	int mode =3D FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;=0A=
> +	struct request_queue *q =3D lo->lo_queue;=0A=
>   	int ret;=0A=
>=0A=
> -	if ((!file->f_op->fallocate) || lo->lo_encrypt_key_size) {=0A=
> +	if (!blk_queue_discard(q)) {=0A=
>   		ret =3D -EOPNOTSUPP;=0A=
>   		goto out;=0A=
>   	}=0A=
> @@ -599,8 +594,13 @@ static int do_req_filebacked(struct loop_device *lo,=
 struct request *rq)=0A=
>   	case REQ_OP_FLUSH:=0A=
>   		return lo_req_flush(lo, rq);=0A=
>   	case REQ_OP_DISCARD:=0A=
> +		return lo_discard(lo, rq,=0A=
> +			FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, pos);=0A=
> +=0A=
>   	case REQ_OP_WRITE_ZEROES:=0A=
> -		return lo_discard(lo, rq, pos);=0A=
> +		return lo_discard(lo, rq,=0A=
> +			FALLOC_FL_ZERO_RANGE | FALLOC_FL_KEEP_SIZE, pos);=0A=
> +=0A=
>   	case REQ_OP_WRITE:=0A=
>   		if (lo->transfer)=0A=
>   			return lo_write_transfer(lo, rq, pos);=0A=
> @@ -854,6 +854,21 @@ static void loop_config_discard(struct loop_device *=
lo)=0A=
>   	struct file *file =3D lo->lo_backing_file;=0A=
>   	struct inode *inode =3D file->f_mapping->host;=0A=
>   	struct request_queue *q =3D lo->lo_queue;=0A=
> +	struct request_queue *backingq;=0A=
> +=0A=
> +	/*=0A=
> +	 * If the backing device is a block device, mirror its zeroing=0A=
> +	 * capability. REQ_OP_DISCARD translates to a zero-out even when backed=
=0A=
> +	 * by block devices to keep consistent behavior with file-backed loop=
=0A=
> +	 * devices.=0A=
> +	 */=0A=
> +	if (S_ISBLK(inode->i_mode) && !lo->lo_encrypt_key_size) {=0A=
> +		backingq =3D bdev_get_queue(inode->i_bdev);=0A=
> +		blk_queue_max_discard_sectors(q,=0A=
> +			backingq->limits.max_write_zeroes_sectors);=0A=
> +=0A=
> +		blk_queue_max_write_zeroes_sectors(q,=0A=
> +			backingq->limits.max_write_zeroes_sectors);=0A=
>=0A=
>   	/*=0A=
>   	 * We use punch hole to reclaim the free space used by the=0A=
> @@ -861,22 +876,24 @@ static void loop_config_discard(struct loop_device =
*lo)=0A=
>   	 * encryption is enabled, because it may give an attacker=0A=
>   	 * useful information.=0A=
>   	 */=0A=
> -	if ((!file->f_op->fallocate) ||=0A=
> -	    lo->lo_encrypt_key_size) {=0A=
> +	} else if ((!file->f_op->fallocate) || lo->lo_encrypt_key_size) {=0A=
>   		q->limits.discard_granularity =3D 0;=0A=
>   		q->limits.discard_alignment =3D 0;=0A=
>   		blk_queue_max_discard_sectors(q, 0);=0A=
>   		blk_queue_max_write_zeroes_sectors(q, 0);=0A=
> -		blk_queue_flag_clear(QUEUE_FLAG_DISCARD, q);=0A=
> -		return;=0A=
> -	}=0A=
>=0A=
> -	q->limits.discard_granularity =3D inode->i_sb->s_blocksize;=0A=
> -	q->limits.discard_alignment =3D 0;=0A=
> +	} else {=0A=
> +		q->limits.discard_granularity =3D inode->i_sb->s_blocksize;=0A=
> +		q->limits.discard_alignment =3D 0;=0A=
> +=0A=
> +		blk_queue_max_discard_sectors(q, UINT_MAX >> 9);=0A=
> +		blk_queue_max_write_zeroes_sectors(q, UINT_MAX >> 9);=0A=
> +	}=0A=
>=0A=
> -	blk_queue_max_discard_sectors(q, UINT_MAX >> 9);=0A=
> -	blk_queue_max_write_zeroes_sectors(q, UINT_MAX >> 9);=0A=
> -	blk_queue_flag_set(QUEUE_FLAG_DISCARD, q);=0A=
> +	if (q->limits.max_write_zeroes_sectors)=0A=
> +		blk_queue_flag_set(QUEUE_FLAG_DISCARD, q);=0A=
> +	else=0A=
> +		blk_queue_flag_clear(QUEUE_FLAG_DISCARD, q);=0A=
>   }=0A=
>=0A=
>   static void loop_unprepare_queue(struct loop_device *lo)=0A=
>=0A=
=0A=