[PATCHSET 0/2] loop: dio fixup

[PATCHSET 0/2] loop: dio fixup

[Jens Axboe]

Wanted to do a v2 of the loop dio short read fix, since I figured
we should retain the zero fill for if we have to give up doing
reads. Also noticed that we have cmd->rq for some reason, so
killed that off as a cleanup.

-- 
Jens Axboe

[PATCH 1/2] loop: remove cmd->rq member

[Jens Axboe]

We can always get at the request from the payload, no need to store
a pointer to it.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/block/loop.c | 36 +++++++++++++++++++-----------------
 drivers/block/loop.h |  1 -
 2 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index c9d04497a415..8b2fde2109fc 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -452,9 +452,9 @@ static void lo_complete_rq(struct request *rq)
 {
 	struct loop_cmd *cmd = blk_mq_rq_to_pdu(rq);
 
-	if (unlikely(req_op(cmd->rq) == REQ_OP_READ && cmd->use_aio &&
-		     cmd->ret >= 0 && cmd->ret < blk_rq_bytes(cmd->rq))) {
-		struct bio *bio = cmd->rq->bio;
+	if (unlikely(req_op(rq) == REQ_OP_READ && cmd->use_aio &&
+		     cmd->ret >= 0 && cmd->ret < blk_rq_bytes(rq))) {
+		struct bio *bio = rq->bio;
 
 		bio_advance(bio, cmd->ret);
 		zero_fill_bio(bio);
@@ -465,11 +465,13 @@ static void lo_complete_rq(struct request *rq)
 
 static void lo_rw_aio_do_completion(struct loop_cmd *cmd)
 {
+	struct request *rq = blk_mq_rq_from_pdu(cmd);
+
 	if (!atomic_dec_and_test(&cmd->ref))
 		return;
 	kfree(cmd->bvec);
 	cmd->bvec = NULL;
-	blk_mq_complete_request(cmd->rq);
+	blk_mq_complete_request(rq);
 }
 
 static void lo_rw_aio_complete(struct kiocb *iocb, long ret, long ret2)
@@ -487,7 +489,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
 {
 	struct iov_iter iter;
 	struct bio_vec *bvec;
-	struct request *rq = cmd->rq;
+	struct request *rq = blk_mq_rq_from_pdu(cmd);
 	struct bio *bio = rq->bio;
 	struct file *file = lo->lo_backing_file;
 	unsigned int offset;
@@ -1702,15 +1704,16 @@ EXPORT_SYMBOL(loop_unregister_transfer);
 static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 		const struct blk_mq_queue_data *bd)
 {
-	struct loop_cmd *cmd = blk_mq_rq_to_pdu(bd->rq);
-	struct loop_device *lo = cmd->rq->q->queuedata;
+	struct request *rq = bd->rq;
+	struct loop_cmd *cmd = blk_mq_rq_to_pdu(rq);
+	struct loop_device *lo = rq->q->queuedata;
 
-	blk_mq_start_request(bd->rq);
+	blk_mq_start_request(rq);
 
 	if (lo->lo_state != Lo_bound)
 		return BLK_STS_IOERR;
 
-	switch (req_op(cmd->rq)) {
+	switch (req_op(rq)) {
 	case REQ_OP_FLUSH:
 	case REQ_OP_DISCARD:
 	case REQ_OP_WRITE_ZEROES:
@@ -1723,8 +1726,8 @@ static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 	/* always use the first bio's css */
 #ifdef CONFIG_BLK_CGROUP
-	if (cmd->use_aio && cmd->rq->bio && cmd->rq->bio->bi_css) {
-		cmd->css = cmd->rq->bio->bi_css;
+	if (cmd->use_aio && rq->bio && rq->bio->bi_css) {
+		cmd->css = rq->bio->bi_css;
 		css_get(cmd->css);
 	} else
 #endif
@@ -1736,8 +1739,9 @@ static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 static void loop_handle_cmd(struct loop_cmd *cmd)
 {
-	const bool write = op_is_write(req_op(cmd->rq));
-	struct loop_device *lo = cmd->rq->q->queuedata;
+	struct request *rq = blk_mq_rq_from_pdu(cmd);
+	const bool write = op_is_write(req_op(rq));
+	struct loop_device *lo = rq->q->queuedata;
 	int ret = 0;
 
 	if (write && (lo->lo_flags & LO_FLAGS_READ_ONLY)) {
@@ -1745,12 +1749,12 @@ static void loop_handle_cmd(struct loop_cmd *cmd)
 		goto failed;
 	}
 
-	ret = do_req_filebacked(lo, cmd->rq);
+	ret = do_req_filebacked(lo, rq);
  failed:
 	/* complete non-aio request */
 	if (!cmd->use_aio || ret) {
 		cmd->ret = ret ? -EIO : 0;
-		blk_mq_complete_request(cmd->rq);
+		blk_mq_complete_request(rq);
 	}
 }
 
@@ -1767,9 +1771,7 @@ static int loop_init_request(struct blk_mq_tag_set *set, struct request *rq,
 {
 	struct loop_cmd *cmd = blk_mq_rq_to_pdu(rq);
 
-	cmd->rq = rq;
 	kthread_init_work(&cmd->work, loop_queue_work);
-
 	return 0;
 }
 
diff --git a/drivers/block/loop.h b/drivers/block/loop.h
index 0f45416e4fcf..b78de9879f4f 100644
--- a/drivers/block/loop.h
+++ b/drivers/block/loop.h
@@ -66,7 +66,6 @@ struct loop_device {
 
 struct loop_cmd {
 	struct kthread_work work;
-	struct request *rq;
 	bool use_aio; /* use AIO interface to handle I/O */
 	atomic_t ref; /* only for aio */
 	long ret;
-- 
2.7.4

Re: [PATCH 1/2] loop: remove cmd->rq member

[Ming Lei]

On Fri, Apr 13, 2018 at 04:30:35PM -0600, Jens Axboe wrote:
> We can always get at the request from the payload, no need to store
> a pointer to it.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  drivers/block/loop.c | 36 +++++++++++++++++++-----------------
>  drivers/block/loop.h |  1 -
>  2 files changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index c9d04497a415..8b2fde2109fc 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -452,9 +452,9 @@ static void lo_complete_rq(struct request *rq)
>  {
>  	struct loop_cmd *cmd = blk_mq_rq_to_pdu(rq);
>  
> -	if (unlikely(req_op(cmd->rq) == REQ_OP_READ && cmd->use_aio &&
> -		     cmd->ret >= 0 && cmd->ret < blk_rq_bytes(cmd->rq))) {
> -		struct bio *bio = cmd->rq->bio;
> +	if (unlikely(req_op(rq) == REQ_OP_READ && cmd->use_aio &&
> +		     cmd->ret >= 0 && cmd->ret < blk_rq_bytes(rq))) {
> +		struct bio *bio = rq->bio;
>  
>  		bio_advance(bio, cmd->ret);
>  		zero_fill_bio(bio);
> @@ -465,11 +465,13 @@ static void lo_complete_rq(struct request *rq)
>  
>  static void lo_rw_aio_do_completion(struct loop_cmd *cmd)
>  {
> +	struct request *rq = blk_mq_rq_from_pdu(cmd);
> +
>  	if (!atomic_dec_and_test(&cmd->ref))
>  		return;
>  	kfree(cmd->bvec);
>  	cmd->bvec = NULL;
> -	blk_mq_complete_request(cmd->rq);
> +	blk_mq_complete_request(rq);
>  }
>  
>  static void lo_rw_aio_complete(struct kiocb *iocb, long ret, long ret2)
> @@ -487,7 +489,7 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
>  {
>  	struct iov_iter iter;
>  	struct bio_vec *bvec;
> -	struct request *rq = cmd->rq;
> +	struct request *rq = blk_mq_rq_from_pdu(cmd);
>  	struct bio *bio = rq->bio;
>  	struct file *file = lo->lo_backing_file;
>  	unsigned int offset;
> @@ -1702,15 +1704,16 @@ EXPORT_SYMBOL(loop_unregister_transfer);
>  static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
>  		const struct blk_mq_queue_data *bd)
>  {
> -	struct loop_cmd *cmd = blk_mq_rq_to_pdu(bd->rq);
> -	struct loop_device *lo = cmd->rq->q->queuedata;
> +	struct request *rq = bd->rq;
> +	struct loop_cmd *cmd = blk_mq_rq_to_pdu(rq);
> +	struct loop_device *lo = rq->q->queuedata;
>  
> -	blk_mq_start_request(bd->rq);
> +	blk_mq_start_request(rq);
>  
>  	if (lo->lo_state != Lo_bound)
>  		return BLK_STS_IOERR;
>  
> -	switch (req_op(cmd->rq)) {
> +	switch (req_op(rq)) {
>  	case REQ_OP_FLUSH:
>  	case REQ_OP_DISCARD:
>  	case REQ_OP_WRITE_ZEROES:
> @@ -1723,8 +1726,8 @@ static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
>  
>  	/* always use the first bio's css */
>  #ifdef CONFIG_BLK_CGROUP
> -	if (cmd->use_aio && cmd->rq->bio && cmd->rq->bio->bi_css) {
> -		cmd->css = cmd->rq->bio->bi_css;
> +	if (cmd->use_aio && rq->bio && rq->bio->bi_css) {
> +		cmd->css = rq->bio->bi_css;
>  		css_get(cmd->css);
>  	} else
>  #endif
> @@ -1736,8 +1739,9 @@ static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
>  
>  static void loop_handle_cmd(struct loop_cmd *cmd)
>  {
> -	const bool write = op_is_write(req_op(cmd->rq));
> -	struct loop_device *lo = cmd->rq->q->queuedata;
> +	struct request *rq = blk_mq_rq_from_pdu(cmd);
> +	const bool write = op_is_write(req_op(rq));
> +	struct loop_device *lo = rq->q->queuedata;
>  	int ret = 0;
>  
>  	if (write && (lo->lo_flags & LO_FLAGS_READ_ONLY)) {
> @@ -1745,12 +1749,12 @@ static void loop_handle_cmd(struct loop_cmd *cmd)
>  		goto failed;
>  	}
>  
> -	ret = do_req_filebacked(lo, cmd->rq);
> +	ret = do_req_filebacked(lo, rq);
>   failed:
>  	/* complete non-aio request */
>  	if (!cmd->use_aio || ret) {
>  		cmd->ret = ret ? -EIO : 0;
> -		blk_mq_complete_request(cmd->rq);
> +		blk_mq_complete_request(rq);
>  	}
>  }
>  
> @@ -1767,9 +1771,7 @@ static int loop_init_request(struct blk_mq_tag_set *set, struct request *rq,
>  {
>  	struct loop_cmd *cmd = blk_mq_rq_to_pdu(rq);
>  
> -	cmd->rq = rq;
>  	kthread_init_work(&cmd->work, loop_queue_work);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/block/loop.h b/drivers/block/loop.h
> index 0f45416e4fcf..b78de9879f4f 100644
> --- a/drivers/block/loop.h
> +++ b/drivers/block/loop.h
> @@ -66,7 +66,6 @@ struct loop_device {
>  
>  struct loop_cmd {
>  	struct kthread_work work;
> -	struct request *rq;
>  	bool use_aio; /* use AIO interface to handle I/O */
>  	atomic_t ref; /* only for aio */
>  	long ret;
> -- 
> 2.7.4
> 

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

[PATCH 2/2] loop: handle short DIO reads

[Jens Axboe]

We ran into an issue with loop and btrfs, where btrfs would complain about
checksum errors. It turns out that is because we don't handle short reads
at all, we just zero fill the remainder. Worse than that, we don't handle
the filling properly, which results in loop trying to advance a single
bio by much more than its size, since it doesn't take chaining into
account.

Handle short reads appropriately, by simply retrying at the new correct
offset. End the remainder of the request with EIO, if we get a 0 read.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/block/loop.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 8b2fde2109fc..5d4e31655d96 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -451,16 +451,36 @@ static int lo_req_flush(struct loop_device *lo, struct request *rq)
 static void lo_complete_rq(struct request *rq)
 {
 	struct loop_cmd *cmd = blk_mq_rq_to_pdu(rq);
+	blk_status_t ret = BLK_STS_OK;
 
-	if (unlikely(req_op(rq) == REQ_OP_READ && cmd->use_aio &&
-		     cmd->ret >= 0 && cmd->ret < blk_rq_bytes(rq))) {
-		struct bio *bio = rq->bio;
-
-		bio_advance(bio, cmd->ret);
-		zero_fill_bio(bio);
+	if (!cmd->use_aio || cmd->ret < 0 || cmd->ret == blk_rq_bytes(rq) ||
+	    req_op(rq) != REQ_OP_READ) {
+		if (cmd->ret < 0)
+			ret = BLK_STS_IOERR;
+		goto end_io;
 	}
 
-	blk_mq_end_request(rq, cmd->ret < 0 ? BLK_STS_IOERR : BLK_STS_OK);
+	/*
+	 * Short READ - if we got some data, advance our request and
+	 * retry it. If we got no data, end the rest with EIO.
+	 */
+	if (cmd->ret) {
+		blk_update_request(rq, BLK_STS_OK, cmd->ret);
+		cmd->ret = 0;
+		blk_mq_requeue_request(rq, true);
+	} else {
+		if (cmd->use_aio) {
+			struct bio *bio = rq->bio;
+
+			while (bio) {
+				zero_fill_bio(bio);
+				bio = bio->bi_next;
+			}
+		}
+		ret = BLK_STS_IOERR;
+end_io:
+		blk_mq_end_request(rq, ret);
+	}
 }
 
 static void lo_rw_aio_do_completion(struct loop_cmd *cmd)
-- 
2.7.4

Re: [PATCH 2/2] loop: handle short DIO reads

[Ming Lei]

On Fri, Apr 13, 2018 at 04:30:36PM -0600, Jens Axboe wrote:
> We ran into an issue with loop and btrfs, where btrfs would complain about
> checksum errors. It turns out that is because we don't handle short reads
> at all, we just zero fill the remainder. Worse than that, we don't handle
> the filling properly, which results in loop trying to advance a single
> bio by much more than its size, since it doesn't take chaining into
> account.
> 
> Handle short reads appropriately, by simply retrying at the new correct
> offset. End the remainder of the request with EIO, if we get a 0 read.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  drivers/block/loop.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 8b2fde2109fc..5d4e31655d96 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -451,16 +451,36 @@ static int lo_req_flush(struct loop_device *lo, struct request *rq)
>  static void lo_complete_rq(struct request *rq)
>  {
>  	struct loop_cmd *cmd = blk_mq_rq_to_pdu(rq);
> +	blk_status_t ret = BLK_STS_OK;
>  
> -	if (unlikely(req_op(rq) == REQ_OP_READ && cmd->use_aio &&
> -		     cmd->ret >= 0 && cmd->ret < blk_rq_bytes(rq))) {
> -		struct bio *bio = rq->bio;
> -
> -		bio_advance(bio, cmd->ret);
> -		zero_fill_bio(bio);
> +	if (!cmd->use_aio || cmd->ret < 0 || cmd->ret == blk_rq_bytes(rq) ||
> +	    req_op(rq) != REQ_OP_READ) {
> +		if (cmd->ret < 0)
> +			ret = BLK_STS_IOERR;
> +		goto end_io;
>  	}
>  
> -	blk_mq_end_request(rq, cmd->ret < 0 ? BLK_STS_IOERR : BLK_STS_OK);
> +	/*
> +	 * Short READ - if we got some data, advance our request and
> +	 * retry it. If we got no data, end the rest with EIO.
> +	 */
> +	if (cmd->ret) {
> +		blk_update_request(rq, BLK_STS_OK, cmd->ret);
> +		cmd->ret = 0;
> +		blk_mq_requeue_request(rq, true);
> +	} else {
> +		if (cmd->use_aio) {
> +			struct bio *bio = rq->bio;
> +
> +			while (bio) {
> +				zero_fill_bio(bio);
> +				bio = bio->bi_next;
> +			}
> +		}
> +		ret = BLK_STS_IOERR;
> +end_io:
> +		blk_mq_end_request(rq, ret);
> +	}
>  }
>  
>  static void lo_rw_aio_do_completion(struct loop_cmd *cmd)
> -- 
> 2.7.4
> 

Looks fine, short read will be guaranteed to complete when zero read
is triggered.

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming