diff mbox series

[RESEND,1/2] loop: Report EOPNOTSUPP properly

Message ID 20190131221328.85343-2-evgreen@chromium.org (mailing list archive)
State New, archived
Headers show
Series loop: Better discard for block devices | expand

Commit Message

Evan Green Jan. 31, 2019, 10:13 p.m. UTC 
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>
---

 drivers/block/loop.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Bart Van Assche Jan. 31, 2019, 11:31 p.m. UTC | #1 
On Thu, 2019-01-31 at 14:13 -0800, Evan Green wrote:
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index cf5538942834..a1ba555e3b92 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -458,8 +458,13 @@ 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)
> -			ret = BLK_STS_IOERR;
> +		if (cmd->ret < 0) {
> +			if (cmd->ret == -EOPNOTSUPP)
> +				ret = BLK_STS_NOTSUPP;
> +			else
> +				ret = BLK_STS_IOERR;
> +		}

Please do not nest if-conditions if that is not necessary. I think the
above code can be written more clearly as follows:

if (cmd->ret == -ENOTSUPP)
	ret = BLK_STS_NOTSUPP;
else if (cmd->ret < 0)
	ret = BLK_STS_IOERR;

Thanks,

Bart.
Evan Green Feb. 1, 2019, 6:18 p.m. UTC | #2 
On Thu, Jan 31, 2019 at 3:31 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On Thu, 2019-01-31 at 14:13 -0800, Evan Green wrote:
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index cf5538942834..a1ba555e3b92 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -458,8 +458,13 @@ 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)
> > -                     ret = BLK_STS_IOERR;
> > +             if (cmd->ret < 0) {
> > +                     if (cmd->ret == -EOPNOTSUPP)
> > +                             ret = BLK_STS_NOTSUPP;
> > +                     else
> > +                             ret = BLK_STS_IOERR;
> > +             }
>
> Please do not nest if-conditions if that is not necessary. I think the
> above code can be written more clearly as follows:
>
> if (cmd->ret == -ENOTSUPP)
>         ret = BLK_STS_NOTSUPP;
> else if (cmd->ret < 0)
>         ret = BLK_STS_IOERR;
>

Thanks for taking a look Bart. Will fix.
-Evan
diff mbox series

Patch

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index cf5538942834..a1ba555e3b92 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -458,8 +458,13 @@  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)
-			ret = BLK_STS_IOERR;
+		if (cmd->ret < 0) {
+			if (cmd->ret == -EOPNOTSUPP)
+				ret = BLK_STS_NOTSUPP;
+			else
+				ret = BLK_STS_IOERR;
+		}
+
 		goto end_io;
 	}
 
@@ -1878,7 +1883,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);
 	}
 }