Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] loop: Better discard support for block devices
@ 2020-03-17 15:11 Andrzej Pietrasiewicz
  2020-03-17 15:11 ` [PATCH 1/2] loop: Report EOPNOTSUPP properly Andrzej Pietrasiewicz
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-03-17 15:11 UTC (permalink / raw)
  To: linux-block; +Cc: Jens Axboe, kernel, andrzej.p

This is a respin of the series from Evan, after rebasing it
on a current tree (v5.6-rc6). In the current upstream tree
there already is code which differentiates between REQ_OP_DISCARD
and REQ_OP_WRITE_ZEROS. Since applying the second patch required
dropping some code inside it, I have also dropped the A-b/R-b tags.

Below is the cover letter of the previous iteration of the series:

This series addresses some errors seen when using the loop
device directly backed by a block device. The first change plumbs
out the correct error message, and the second change prevents the
error from occurring in many cases.

The errors look like this:
[   90.880875] print_req_error: I/O error, dev loop5, sector 0

The errors occur when trying to do a discard or write zeroes operation
on a loop device backed by a block device that does not support write zeroes.
Firstly, the error itself is incorrectly reported as I/O error, but is
actually EOPNOTSUPP. The first patch plumbs out EOPNOTSUPP to properly
report the error.

The second patch prevents these errors from occurring by mirroring the
zeroing capabilities of the underlying block device into the loop device.
Before this change, discard was always reported as being supported, and
the loop device simply turns around and does an fallocate operation on the
backing device. After this change, backing block devices that do support
zeroing will continue to work as before, and continue to get all the
benefits of doing that. Backing devices that do not support zeroing will
fail earlier, avoiding hitting the loop device at all and ultimately
avoiding this error in the logs.

I can also confirm that this fixes test block/003 in the blktests, when
running blktests on a loop device backed by a block device.


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 tags
- Updated commit description

Changes in v2:
- Unnested error if statement (Bart)

Evan Green (2):
  loop: Report EOPNOTSUPP properly
  loop: Better discard support for block devices

 drivers/block/loop.c | 51 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 13 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] loop: Report EOPNOTSUPP properly
  2020-03-17 15:11 [PATCH 0/2] loop: Better discard support for block devices Andrzej Pietrasiewicz
@ 2020-03-17 15:11 ` Andrzej Pietrasiewicz
  2020-03-17 16:51   ` [PATCH RESEND " Andrzej Pietrasiewicz
  2020-03-17 15:11 ` [PATCH 2/2] loop: Better discard support for block devices Andrzej Pietrasiewicz
  2020-03-26 11:48 ` [PATCH 0/2] " Andrzej Pietrasiewicz
  2 siblings, 1 reply; 9+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-03-17 15:11 UTC (permalink / raw)
  To: linux-block; +Cc: Jens Axboe, kernel, andrzej.p, Evan Green

From: Evan Green <evgreen@chromium.org>

Properly plumb out EOPNOTSUPP from loop driver operations, which may
get returned when for instance a discard operation is attempted but not
supported by the underlying block device. Before this change, everything
was reported in the log as an I/O error, which is scary and not
helpful in debugging.

Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
 drivers/block/loop.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 739b372a5112..87307454f768 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -460,7 +460,9 @@ static void lo_complete_rq(struct request *rq)
 
 	if (!cmd->use_aio || cmd->ret < 0 || cmd->ret == blk_rq_bytes(rq) ||
 	    req_op(rq) != REQ_OP_READ) {
-		if (cmd->ret < 0)
+		if (cmd->ret == -EOPNOTSUPP)
+			ret = BLK_STS_NOTSUPP;
+		else if (cmd->ret < 0)
 			ret = BLK_STS_IOERR;
 		goto end_io;
 	}
@@ -1953,7 +1955,10 @@ static void loop_handle_cmd(struct loop_cmd *cmd)
  failed:
 	/* complete non-aio request */
 	if (!cmd->use_aio || ret) {
-		cmd->ret = ret ? -EIO : 0;
+		if (ret == -EOPNOTSUPP)
+			cmd->ret = ret;
+		else
+			cmd->ret = ret ? -EIO : 0;
 		blk_mq_complete_request(rq);
 	}
 }
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] loop: Better discard support for block devices
  2020-03-17 15:11 [PATCH 0/2] loop: Better discard support for block devices Andrzej Pietrasiewicz
  2020-03-17 15:11 ` [PATCH 1/2] loop: Report EOPNOTSUPP properly Andrzej Pietrasiewicz
@ 2020-03-17 15:11 ` Andrzej Pietrasiewicz
  2020-03-26 11:48 ` [PATCH 0/2] " Andrzej Pietrasiewicz
  2 siblings, 0 replies; 9+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-03-17 15:11 UTC (permalink / raw)
  To: linux-block; +Cc: Jens Axboe, kernel, andrzej.p, Evan Green

From: Evan Green <evgreen@chromium.org>

If the backing device for a loop device is 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>
[rebase onto v5.6-rc6]
Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

---

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 | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 87307454f768..8de3b2b098d3 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;
 	}
@@ -867,28 +868,47 @@ static void loop_config_discard(struct loop_device *lo)
 	struct inode *inode = file->f_mapping->host;
 	struct request_queue *q = lo->lo_queue;
 
+
+	/*
+	 * 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) {
+		struct request_queue *backingq;
+
+		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
 	 * image a.k.a. discard. However we do not support discard if
 	 * 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_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);
+	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)
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH RESEND 1/2] loop: Report EOPNOTSUPP properly
  2020-03-17 15:11 ` [PATCH 1/2] loop: Report EOPNOTSUPP properly Andrzej Pietrasiewicz
@ 2020-03-17 16:51   ` Andrzej Pietrasiewicz
  2020-03-26 14:52     ` Christoph Hellwig
  0 siblings, 1 reply; 9+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-03-17 16:51 UTC (permalink / raw)
  To: linux-block; +Cc: Jens Axboe, kernel, andrzej.p, Evan Green

From: Evan Green <evgreen@chromium.org>

Properly plumb out EOPNOTSUPP from loop driver operations, which may
get returned when for instance a discard operation is attempted but not
supported by the underlying block device. Before this change, everything
was reported in the log as an I/O error, which is scary and not
helpful in debugging.

Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
Add missing S-o-b from me as sender.

 drivers/block/loop.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 739b372a5112..87307454f768 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -460,7 +460,9 @@ static void lo_complete_rq(struct request *rq)
 
 	if (!cmd->use_aio || cmd->ret < 0 || cmd->ret == blk_rq_bytes(rq) ||
 	    req_op(rq) != REQ_OP_READ) {
-		if (cmd->ret < 0)
+		if (cmd->ret == -EOPNOTSUPP)
+			ret = BLK_STS_NOTSUPP;
+		else if (cmd->ret < 0)
 			ret = BLK_STS_IOERR;
 		goto end_io;
 	}
@@ -1953,7 +1955,10 @@ static void loop_handle_cmd(struct loop_cmd *cmd)
  failed:
 	/* complete non-aio request */
 	if (!cmd->use_aio || ret) {
-		cmd->ret = ret ? -EIO : 0;
+		if (ret == -EOPNOTSUPP)
+			cmd->ret = ret;
+		else
+			cmd->ret = ret ? -EIO : 0;
 		blk_mq_complete_request(rq);
 	}
 }
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] loop: Better discard support for block devices
  2020-03-17 15:11 [PATCH 0/2] loop: Better discard support for block devices Andrzej Pietrasiewicz
  2020-03-17 15:11 ` [PATCH 1/2] loop: Report EOPNOTSUPP properly Andrzej Pietrasiewicz
  2020-03-17 15:11 ` [PATCH 2/2] loop: Better discard support for block devices Andrzej Pietrasiewicz
@ 2020-03-26 11:48 ` Andrzej Pietrasiewicz
  2 siblings, 0 replies; 9+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-03-26 11:48 UTC (permalink / raw)
  To: linux-block; +Cc: Jens Axboe, kernel

Hi All,

I gentle reminder about this short patch series.

Regards,

Andrzej

W dniu 17.03.2020 o 16:11, Andrzej Pietrasiewicz pisze:
> This is a respin of the series from Evan, after rebasing it
> on a current tree (v5.6-rc6). In the current upstream tree
> there already is code which differentiates between REQ_OP_DISCARD
> and REQ_OP_WRITE_ZEROS. Since applying the second patch required
> dropping some code inside it, I have also dropped the A-b/R-b tags.
> 
> Below is the cover letter of the previous iteration of the series:
> 
> This series addresses some errors seen when using the loop
> device directly backed by a block device. The first change plumbs
> out the correct error message, and the second change prevents the
> error from occurring in many cases.
> 
> The errors look like this:
> [   90.880875] print_req_error: I/O error, dev loop5, sector 0
> 
> The errors occur when trying to do a discard or write zeroes operation
> on a loop device backed by a block device that does not support write zeroes.
> Firstly, the error itself is incorrectly reported as I/O error, but is
> actually EOPNOTSUPP. The first patch plumbs out EOPNOTSUPP to properly
> report the error.
> 
> The second patch prevents these errors from occurring by mirroring the
> zeroing capabilities of the underlying block device into the loop device.
> Before this change, discard was always reported as being supported, and
> the loop device simply turns around and does an fallocate operation on the
> backing device. After this change, backing block devices that do support
> zeroing will continue to work as before, and continue to get all the
> benefits of doing that. Backing devices that do not support zeroing will
> fail earlier, avoiding hitting the loop device at all and ultimately
> avoiding this error in the logs.
> 
> I can also confirm that this fixes test block/003 in the blktests, when
> running blktests on a loop device backed by a block device.
> 
> 
> 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 tags
> - Updated commit description
> 
> Changes in v2:
> - Unnested error if statement (Bart)
> 
> Evan Green (2):
>    loop: Report EOPNOTSUPP properly
>    loop: Better discard support for block devices
> 
>   drivers/block/loop.c | 51 +++++++++++++++++++++++++++++++++-----------
>   1 file changed, 38 insertions(+), 13 deletions(-)
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND 1/2] loop: Report EOPNOTSUPP properly
  2020-03-17 16:51   ` [PATCH RESEND " Andrzej Pietrasiewicz
@ 2020-03-26 14:52     ` Christoph Hellwig
  2020-03-26 15:51       ` Evan Green
  0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2020-03-26 14:52 UTC (permalink / raw)
  To: Andrzej Pietrasiewicz; +Cc: linux-block, Jens Axboe, kernel, Evan Green

On Tue, Mar 17, 2020 at 05:51:06PM +0100, Andrzej Pietrasiewicz wrote:
> From: Evan Green <evgreen@chromium.org>
> 
> Properly plumb out EOPNOTSUPP from loop driver operations, which may
> get returned when for instance a discard operation is attempted but not
> supported by the underlying block device. Before this change, everything
> was reported in the log as an I/O error, which is scary and not
> helpful in debugging.

This really should be using errno_to_blk_status.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND 1/2] loop: Report EOPNOTSUPP properly
  2020-03-26 14:52     ` Christoph Hellwig
@ 2020-03-26 15:51       ` Evan Green
  2020-03-26 15:55         ` Christoph Hellwig
  0 siblings, 1 reply; 9+ messages in thread
From: Evan Green @ 2020-03-26 15:51 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Andrzej Pietrasiewicz, linux-block, Jens Axboe, kernel

On Thu, Mar 26, 2020 at 7:53 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Mar 17, 2020 at 05:51:06PM +0100, Andrzej Pietrasiewicz wrote:
> > From: Evan Green <evgreen@chromium.org>
> >
> > Properly plumb out EOPNOTSUPP from loop driver operations, which may
> > get returned when for instance a discard operation is attempted but not
> > supported by the underlying block device. Before this change, everything
> > was reported in the log as an I/O error, which is scary and not
> > helpful in debugging.
>
> This really should be using errno_to_blk_status.

I had that here in v7:
https://lore.kernel.org/lkml/20191114235008.185111-1-evgreen@chromium.org/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND 1/2] loop: Report EOPNOTSUPP properly
  2020-03-26 15:51       ` Evan Green
@ 2020-03-26 15:55         ` Christoph Hellwig
  2020-03-26 16:36           ` Andrzej Pietrasiewicz
  0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2020-03-26 15:55 UTC (permalink / raw)
  To: Evan Green
  Cc: Christoph Hellwig, Andrzej Pietrasiewicz, linux-block,
	Jens Axboe, kernel

On Thu, Mar 26, 2020 at 08:51:21AM -0700, Evan Green wrote:
> On Thu, Mar 26, 2020 at 7:53 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Tue, Mar 17, 2020 at 05:51:06PM +0100, Andrzej Pietrasiewicz wrote:
> > > From: Evan Green <evgreen@chromium.org>
> > >
> > > Properly plumb out EOPNOTSUPP from loop driver operations, which may
> > > get returned when for instance a discard operation is attempted but not
> > > supported by the underlying block device. Before this change, everything
> > > was reported in the log as an I/O error, which is scary and not
> > > helpful in debugging.
> >
> > This really should be using errno_to_blk_status.
> 
> I had that here in v7:
> https://lore.kernel.org/lkml/20191114235008.185111-1-evgreen@chromium.org/

Well, it wasn't in the version you sent the ping for..

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH RESEND 1/2] loop: Report EOPNOTSUPP properly
  2020-03-26 15:55         ` Christoph Hellwig
@ 2020-03-26 16:36           ` Andrzej Pietrasiewicz
  0 siblings, 0 replies; 9+ messages in thread
From: Andrzej Pietrasiewicz @ 2020-03-26 16:36 UTC (permalink / raw)
  To: Christoph Hellwig, Evan Green; +Cc: linux-block, Jens Axboe, kernel

Hi,

W dniu 26.03.2020 o 16:55, Christoph Hellwig pisze:
> On Thu, Mar 26, 2020 at 08:51:21AM -0700, Evan Green wrote:
>> On Thu, Mar 26, 2020 at 7:53 AM Christoph Hellwig <hch@infradead.org> wrote:
>>>
>>> On Tue, Mar 17, 2020 at 05:51:06PM +0100, Andrzej Pietrasiewicz wrote:
>>>> From: Evan Green <evgreen@chromium.org>
>>>>
>>>> Properly plumb out EOPNOTSUPP from loop driver operations, which may
>>>> get returned when for instance a discard operation is attempted but not
>>>> supported by the underlying block device. Before this change, everything
>>>> was reported in the log as an I/O error, which is scary and not
>>>> helpful in debugging.
>>>
>>> This really should be using errno_to_blk_status.
>>
>> I had that here in v7:
>> https://lore.kernel.org/lkml/20191114235008.185111-1-evgreen@chromium.org/
> 
> Well, it wasn't in the version you sent the ping for..
> 

It was me who pinged. I didn't notice the v7, sorry. Is it merged yet?

Andrzej

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-17 15:11 [PATCH 0/2] loop: Better discard support for block devices Andrzej Pietrasiewicz
2020-03-17 15:11 ` [PATCH 1/2] loop: Report EOPNOTSUPP properly Andrzej Pietrasiewicz
2020-03-17 16:51   ` [PATCH RESEND " Andrzej Pietrasiewicz
2020-03-26 14:52     ` Christoph Hellwig
2020-03-26 15:51       ` Evan Green
2020-03-26 15:55         ` Christoph Hellwig
2020-03-26 16:36           ` Andrzej Pietrasiewicz
2020-03-17 15:11 ` [PATCH 2/2] loop: Better discard support for block devices Andrzej Pietrasiewicz
2020-03-26 11:48 ` [PATCH 0/2] " Andrzej Pietrasiewicz

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git