[PATCH v2 0/2] loop: Better discard for block devices

[PATCH v2 0/2] loop: Better discard for block devices

[Evan Green]

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 discard.
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
discard 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 a discard operation on the
backing device. After this change, backing block devices that do support
discard will continue to work as before, and continue to get all the
benefits of doing that. Backing devices that do not support discard 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 v2:
- Unnested error if statement (Bart)

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

 drivers/block/loop.c | 70 ++++++++++++++++++++++++++++++--------------
 1 file changed, 48 insertions(+), 22 deletions(-)

-- 
2.20.1

[PATCH v2 1/2] loop: Report EOPNOTSUPP properly

[Evan Green]

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>
---

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

Ming, I opted to keep your Reviewed-by since the change since v1 was trivial.
Hope that's okay.

---
 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 cf5538942834..b6c2d8f3202b 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -458,7 +458,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;
 	}
@@ -1878,7 +1880,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.20.1

Re: [PATCH v2 1/2] loop: Report EOPNOTSUPP properly

[Bart Van Assche]

On Thu, 2019-02-07 at 12:57 -0800, Evan Green wrote:
> 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.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v2 1/2] loop: Report EOPNOTSUPP properly

[Martin K. Petersen]

Evan,

> 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.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 1/2] loop: Report EOPNOTSUPP properly

[Evan Green]

On Wed, Feb 13, 2019 at 6:32 PM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
>
> Evan,
>
> > 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.
>
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
>

Hi Jens,
I think this one at least is ready to go.
-Evan

[PATCH v2 2/2] loop: Better discard support for block devices

[Evan Green]

If the backing device for a loop device is a block device,
then mirror the discard properties of the underlying block
device into the loop device. While in there, differentiate
between REQ_OP_DISCARD and REQ_OP_WRITE_ZEROES, which are
different for block devices, but which the loop device had
just been lumping together.

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>
---

Changes in v2: None

 drivers/block/loop.c | 61 +++++++++++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index b6c2d8f3202b..9b1fb2d639f1 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -417,19 +417,14 @@ static int lo_read_transfer(struct loop_device *lo, struct request *rq,
 	return ret;
 }
 
-static int lo_discard(struct loop_device *lo, struct request *rq, loff_t pos)
+static int lo_discard(struct loop_device *lo, struct request *rq,
+		int mode, loff_t pos)
 {
-	/*
-	 * 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.
-	 */
 	struct file *file = lo->lo_backing_file;
-	int mode = FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;
+	struct request_queue *q = lo->lo_queue;
 	int ret;
 
-	if ((!file->f_op->fallocate) || lo->lo_encrypt_key_size) {
+	if (!blk_queue_discard(q)) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
@@ -599,8 +594,13 @@ static int do_req_filebacked(struct loop_device *lo, struct request *rq)
 	case REQ_OP_FLUSH:
 		return lo_req_flush(lo, rq);
 	case REQ_OP_DISCARD:
+		return lo_discard(lo, rq,
+			FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, pos);
+
 	case REQ_OP_WRITE_ZEROES:
-		return lo_discard(lo, rq, pos);
+		return lo_discard(lo, rq,
+			FALLOC_FL_ZERO_RANGE | FALLOC_FL_KEEP_SIZE, pos);
+
 	case REQ_OP_WRITE:
 		if (lo->transfer)
 			return lo_write_transfer(lo, rq, pos);
@@ -854,6 +854,25 @@ 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 discard
+	 * capabilities.
+	 */
+	if (S_ISBLK(inode->i_mode)) {
+		backingq = bdev_get_queue(inode->i_bdev);
+		blk_queue_max_discard_sectors(q,
+			backingq->limits.max_discard_sectors);
+
+		blk_queue_max_write_zeroes_sectors(q,
+			backingq->limits.max_write_zeroes_sectors);
+
+		q->limits.discard_granularity =
+			backingq->limits.discard_granularity;
+
+		q->limits.discard_alignment =
+			backingq->limits.discard_alignment;
 
 	/*
 	 * We use punch hole to reclaim the free space used by the
@@ -861,22 +880,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_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_discard_sectors || 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.20.1

Re: [PATCH v2 2/2] loop: Better discard support for block devices

[Martin K. Petersen]

Evan,

> If the backing device for a loop device is a block device, then mirror
> the discard properties of the underlying block device into the loop
> device. While in there, differentiate between REQ_OP_DISCARD and
> REQ_OP_WRITE_ZEROES, which are different for block devices, but which
> the loop device had just been lumping together.

Bubbling up the queue limits from the backing device is fine. However,
I'm not sure why you are requiring a filesystem to be on a
discard-capable device for REQ_OP_DISCARD to have an effect? Punching a
hole in a file is semantically the same as discarding.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 2/2] loop: Better discard support for block devices

[Evan Green]

On Wed, Feb 13, 2019 at 6:40 PM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
>
> Evan,
>
> > If the backing device for a loop device is a block device, then mirror
> > the discard properties of the underlying block device into the loop
> > device. While in there, differentiate between REQ_OP_DISCARD and
> > REQ_OP_WRITE_ZEROES, which are different for block devices, but which
> > the loop device had just been lumping together.
>
> Bubbling up the queue limits from the backing device is fine. However,
> I'm not sure why you are requiring a filesystem to be on a
> discard-capable device for REQ_OP_DISCARD to have an effect? Punching a
> hole in a file is semantically the same as discarding.
>

Hi Martin,
Thanks so much for taking a look at this patch, I was getting nervous
it was languishing again. I got confused by this comment though. My
intention was to not change behavior for loop devices backed by a
regular file system file. The changes in loop_config_discard() should
result in QUEUE_FLAG_DISCARD being set for backings of regular files
that support f_op->fallocate(), same as before my patch. The change in
lo_discard() to call blk_queue_discard() on the loopback queue itself
was just shorthand to avoid duplicating all those if statements from
loop_config_discard again. Am I missing a spot where I've implicitly
changed the behavior for file-backed loop devices?
-Evan

Re: [PATCH v2 2/2] loop: Better discard support for block devices

[Evan Green]

On Thu, Feb 14, 2019 at 10:00 AM Evan Green <evgreen@chromium.org> wrote:
>
> On Wed, Feb 13, 2019 at 6:40 PM Martin K. Petersen
> <martin.petersen@oracle.com> wrote:
> >
> >
> > Evan,
> >
> > > If the backing device for a loop device is a block device, then mirror
> > > the discard properties of the underlying block device into the loop
> > > device. While in there, differentiate between REQ_OP_DISCARD and
> > > REQ_OP_WRITE_ZEROES, which are different for block devices, but which
> > > the loop device had just been lumping together.
> >
> > Bubbling up the queue limits from the backing device is fine. However,
> > I'm not sure why you are requiring a filesystem to be on a
> > discard-capable device for REQ_OP_DISCARD to have an effect? Punching a
> > hole in a file is semantically the same as discarding.
> >
>
> Hi Martin,
> Thanks so much for taking a look at this patch, I was getting nervous
> it was languishing again. I got confused by this comment though. My
> intention was to not change behavior for loop devices backed by a
> regular file system file. The changes in loop_config_discard() should
> result in QUEUE_FLAG_DISCARD being set for backings of regular files
> that support f_op->fallocate(), same as before my patch. The change in
> lo_discard() to call blk_queue_discard() on the loopback queue itself
> was just shorthand to avoid duplicating all those if statements from
> loop_config_discard again. Am I missing a spot where I've implicitly
> changed the behavior for file-backed loop devices?
> -Evan

Hi Martin,
Did you get a chance to take a look at my reply? This error log
plagues our Chrome OS installer, and I'm hopeful I've found an
approach here that's upstream-friendly. But if it's not, let me know
and I can fix it.
-Evan