fix loop discard regression

fix loop discard regression

[Christoph Hellwig]

The first patch fixes a regression where discard request were accidentally
marked as aio after the req_op conversion.  This would have been mostly
harmless except for the god-awful parsing of request types later in the
actual I/O handler which turns them into a write, so the second patch
rewrites that handler using a proper switch statement as well.

[PATCH 1/2] loop: don't try to use AIO for discards

[Christoph Hellwig]

Fix a fat-fingered conversion to the req_op accessors, and also
use a switch statement to make it more obvious what is being checked.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Dave Chinner <david@fromorbit.com>
Fixes: c2df40 ("drivers: use req op accessor");
---
 drivers/block/loop.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 075377e..91c2c88 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1659,11 +1659,15 @@ static int loop_queue_rq(struct blk_mq_hw_ctx *hctx,
 	if (lo->lo_state != Lo_bound)
 		return -EIO;
 
-	if (lo->use_dio && (req_op(cmd->rq) != REQ_OP_FLUSH ||
-	    req_op(cmd->rq) == REQ_OP_DISCARD))
-		cmd->use_aio = true;
-	else
+	switch (req_op(cmd->rq)) {
+	case REQ_OP_FLUSH:
+	case REQ_OP_DISCARD:
 		cmd->use_aio = false;
+		break;
+	default:
+		cmd->use_aio = lo->use_dio;
+		break;
+	}
 
 	queue_kthread_work(&lo->worker, &cmd->work);
 
-- 
2.1.4

Re: [PATCH 1/2] loop: don't try to use AIO for discards

[Ming Lei]

On Thu, Aug 4, 2016 at 10:10 PM, Christoph Hellwig <hch@lst.de> wrote:
> Fix a fat-fingered conversion to the req_op accessors, and also
> use a switch statement to make it more obvious what is being checked.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Dave Chinner <david@fromorbit.com>
> Fixes: c2df40 ("drivers: use req op accessor");
> ---
>  drivers/block/loop.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 075377e..91c2c88 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -1659,11 +1659,15 @@ static int loop_queue_rq(struct blk_mq_hw_ctx *hctx,
>         if (lo->lo_state != Lo_bound)
>                 return -EIO;
>
> -       if (lo->use_dio && (req_op(cmd->rq) != REQ_OP_FLUSH ||
> -           req_op(cmd->rq) == REQ_OP_DISCARD))
> -               cmd->use_aio = true;
> -       else
> +       switch (req_op(cmd->rq)) {
> +       case REQ_OP_FLUSH:
> +       case REQ_OP_DISCARD:
>                 cmd->use_aio = false;
> +               break;
> +       default:
> +               cmd->use_aio = lo->use_dio;
> +               break;
> +       }

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

>
>         queue_kthread_work(&lo->worker, &cmd->work);
>
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] loop: make do_req_filebacked more robust

[Christoph Hellwig]

Use a switch statement to iterate over the possible operations and
error out if it's an incorrect one.
---
 drivers/block/loop.c | 55 +++++++++++++++++++++-------------------------------
 1 file changed, 22 insertions(+), 33 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 91c2c88..c9f2107 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -510,14 +510,10 @@ static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd,
 	return 0;
 }
 
-
-static inline int lo_rw_simple(struct loop_device *lo,
-		struct request *rq, loff_t pos, bool rw)
+static int do_req_filebacked(struct loop_device *lo, struct request *rq)
 {
 	struct loop_cmd *cmd = blk_mq_rq_to_pdu(rq);
-
-	if (cmd->use_aio)
-		return lo_rw_aio(lo, cmd, pos, rw);
+	loff_t pos = ((loff_t) blk_rq_pos(rq) << 9) + lo->lo_offset;
 
 	/*
 	 * lo_write_simple and lo_read_simple should have been covered
@@ -528,37 +524,30 @@ static inline int lo_rw_simple(struct loop_device *lo,
 	 * of the req at one time. And direct read IO doesn't need to
 	 * run flush_dcache_page().
 	 */
-	if (rw == WRITE)
-		return lo_write_simple(lo, rq, pos);
-	else
-		return lo_read_simple(lo, rq, pos);
-}
-
-static int do_req_filebacked(struct loop_device *lo, struct request *rq)
-{
-	loff_t pos;
-	int ret;
-
-	pos = ((loff_t) blk_rq_pos(rq) << 9) + lo->lo_offset;
-
-	if (op_is_write(req_op(rq))) {
-		if (req_op(rq) == REQ_OP_FLUSH)
-			ret = lo_req_flush(lo, rq);
-		else if (req_op(rq) == REQ_OP_DISCARD)
-			ret = lo_discard(lo, rq, pos);
-		else if (lo->transfer)
-			ret = lo_write_transfer(lo, rq, pos);
+	switch (req_op(rq)) {
+	case REQ_OP_FLUSH:
+		return lo_req_flush(lo, rq);
+	case REQ_OP_DISCARD:
+		return lo_discard(lo, rq, pos);
+	case REQ_OP_WRITE:
+		if (lo->transfer)
+			return lo_write_transfer(lo, rq, pos);
+		else if (cmd->use_aio)
+			return lo_rw_aio(lo, cmd, pos, WRITE);
 		else
-			ret = lo_rw_simple(lo, rq, pos, WRITE);
-
-	} else {
+			return lo_write_simple(lo, rq, pos);
+	case REQ_OP_READ:
 		if (lo->transfer)
-			ret = lo_read_transfer(lo, rq, pos);
+			return lo_read_transfer(lo, rq, pos);
+		else if (cmd->use_aio)
+			return lo_rw_aio(lo, cmd, pos, READ);
 		else
-			ret = lo_rw_simple(lo, rq, pos, READ);
+			return lo_read_simple(lo, rq, pos);
+	default:
+		WARN_ON_ONCE(1);
+		return -EIO;
+		break;
 	}
-
-	return ret;
 }
 
 struct switch_request {
-- 
2.1.4

Re: fix loop discard regression

[Jens Axboe]

On 08/04/2016 08:09 AM, Christoph Hellwig wrote:
> The first patch fixes a regression where discard request were accidentally
> marked as aio after the req_op conversion.  This would have been mostly
> harmless except for the god-awful parsing of request types later in the
> actual I/O handler which turns them into a write, so the second patch
> rewrites that handler using a proper switch statement as well.

Applied, thanks.

-- 
Jens Axboe

Re: fix loop discard regression

[Ross Zwisler]

On Thu, Aug 4, 2016 at 8:09 AM, Christoph Hellwig <hch@lst.de> wrote:
> The first patch fixes a regression where discard request were accidentally
> marked as aio after the req_op conversion.  This would have been mostly
> harmless except for the god-awful parsing of request types later in the
> actual I/O handler which turns them into a write, so the second patch
> rewrites that handler using a proper switch statement as well.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-block" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>

Also, your 2nd patch is missing a signed-off-by.

Re: fix loop discard regression

[Christoph Hellwig]

On Thu, Aug 04, 2016 at 12:55:26PM -0600, Ross Zwisler wrote:
> On Thu, Aug 4, 2016 at 8:09 AM, Christoph Hellwig <hch@lst.de> wrote:
> > The first patch fixes a regression where discard request were accidentally
> > marked as aio after the req_op conversion.  This would have been mostly
> > harmless except for the god-awful parsing of request types later in the
> > actual I/O handler which turns them into a write, so the second patch
> > rewrites that handler using a proper switch statement as well.
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-block" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> 
> Also, your 2nd patch is missing a signed-off-by.

Signed-off-by: Christoph Hellwig <hch@lst.de>

in case that Jens wants to bother with force-updating the tree.

Re: fix loop discard regression

[Jens Axboe]

On 08/05/2016 01:39 AM, Christoph Hellwig wrote:
> On Thu, Aug 04, 2016 at 12:55:26PM -0600, Ross Zwisler wrote:
>> On Thu, Aug 4, 2016 at 8:09 AM, Christoph Hellwig <hch@lst.de> wrote:
>>> The first patch fixes a regression where discard request were accidentally
>>> marked as aio after the req_op conversion.  This would have been mostly
>>> harmless except for the god-awful parsing of request types later in the
>>> actual I/O handler which turns them into a write, so the second patch
>>> rewrites that handler using a proper switch statement as well.
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-block" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
>>
>> Also, your 2nd patch is missing a signed-off-by.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> in case that Jens wants to bother with force-updating the tree.

Not really, was hoping to flush it out today. Don't see it as a huge
problem...


-- 
Jens Axboe