[PATCH] Attempt to get eject failures back to ioctl(CDROMEJECT)

[PATCH] Attempt to get eject failures back to ioctl(CDROMEJECT)

[Rusty Russell]

Hi Jens,

    As you asked for some time ago.  Of course, it turns out that the eject
command ignores the error anyway, but it's nice that it now errors.

    Not entirely comfortable with this patch: there's a req->errors but
that seems to have some existing semantics I'm not sure of, so I simply
added a new way of flagging an error.

Cheers,
Rusty.
---

Currently the CDROMEJECT ioctl always succeeds; it'd be nice if we
managed to get the errno passed back through the request.

This patch tries to do that.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r a7da575b248f block/ll_rw_blk.c
--- a/block/ll_rw_blk.c	Wed Nov 14 15:44:39 2007 +1100
+++ b/block/ll_rw_blk.c	Wed Nov 14 17:33:55 2007 +1100
@@ -261,6 +261,7 @@ static void rq_init(struct request_queue
 	rq->end_io_data = NULL;
 	rq->completion_data = NULL;
 	rq->next_rq = NULL;
+	rq->errno = 0;
 }
 
 /**
@@ -2657,7 +2658,9 @@ int blk_execute_rq(struct request_queue 
 	blk_execute_rq_nowait(q, bd_disk, rq, at_head, blk_end_sync_rq);
 	wait_for_completion(&wait);
 
-	if (rq->errors)
+	if (rq->errno)
+		err = rq->errno;
+	else if (rq->errors)
 		err = -EIO;
 
 	return err;
@@ -3427,8 +3430,10 @@ static int __end_that_request_first(stru
 	 * extend uptodate bool to allow < 0 value to be direct io error
 	 */
 	error = 0;
-	if (end_io_error(uptodate))
+	if (end_io_error(uptodate)) {
+		req->errno = uptodate;
 		error = !uptodate ? -EIO : uptodate;
+	}
 
 	/*
 	 * for a REQ_BLOCK_PC request, we want to carry any eventual
diff -r a7da575b248f drivers/block/virtio_blk.c
--- a/drivers/block/virtio_blk.c	Wed Nov 14 15:44:39 2007 +1100
+++ b/drivers/block/virtio_blk.c	Wed Nov 14 17:33:55 2007 +1100
@@ -51,7 +51,7 @@ static bool blk_done(struct virtqueue *v
 			uptodate = 1;
 			break;
 		case VIRTIO_BLK_S_UNSUPP:
-			uptodate = -ENOTTY;
+			uptodate = -ENOSYS;
 			break;
 		default:
 			uptodate = 0;
diff -r a7da575b248f include/linux/blkdev.h
--- a/include/linux/blkdev.h	Wed Nov 14 15:44:39 2007 +1100
+++ b/include/linux/blkdev.h	Wed Nov 14 17:33:55 2007 +1100
@@ -281,6 +281,8 @@ struct request {
 
 	int tag;
 	int errors;
+	/* If this isn't set, assume -EIO. */
+	int errno;
 
 	int ref_count;
 

Re: [PATCH] Attempt to get eject failures back to ioctl(CDROMEJECT)

[Jens Axboe]

On Wed, Nov 14 2007, Rusty Russell wrote:
> Hi Jens,
> 
>     As you asked for some time ago.  Of course, it turns out that the eject
> command ignores the error anyway, but it's nice that it now errors.
> 
>     Not entirely comfortable with this patch: there's a req->errors but
> that seems to have some existing semantics I'm not sure of, so I simply
> added a new way of flagging an error.

It is a bit of a hack, but it's not really your fault. ->errors is
somewhat messy and has different meaning depending on the request type.
I'll add your patch and then do a sanitize on top of it, so that we can
switch things over to a unified ->errno instead.

-- 
Jens Axboe

Re: [PATCH] Attempt to get eject failures back to ioctl(CDROMEJECT)

[Rusty Russell]

On Wednesday 14 November 2007 23:39:31 Jens Axboe wrote:
> On Wed, Nov 14 2007, Rusty Russell wrote:
> > Hi Jens,
> >
> >     As you asked for some time ago.  Of course, it turns out that the
> > eject command ignores the error anyway, but it's nice that it now errors.
> >
> >     Not entirely comfortable with this patch: there's a req->errors but
> > that seems to have some existing semantics I'm not sure of, so I simply
> > added a new way of flagging an error.
>
> It is a bit of a hack, but it's not really your fault. ->errors is
> somewhat messy and has different meaning depending on the request type.
> I'll add your patch and then do a sanitize on top of it, so that we can
> switch things over to a unified ->errno instead.

Thanks!

Oh, I also noticed this in scsi_tgt_lib:

From: Rusty Russell <rusty@rustcorp.com.au>
Date: Fri, 9 Nov 2007 20:04:54 +1100
Subject: [PATCH] scsi_tgt_lib: BUG_ON() impossible condition.

If blk_rq_map_sg returns more than was allocated, it's a bug, and something's
already been overwritten.  BUG_ON() is probably the right thing here.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/scsi/scsi_tgt_lib.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
index a91761c..66266c8 100644
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -367,14 +367,9 @@ static int scsi_tgt_init_cmd(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 
 	dprintk("cmd %p cnt %d %lu\n", cmd, cmd->use_sg, rq_data_dir(rq));
 	count = blk_rq_map_sg(rq->q, rq, cmd->request_buffer);
-	if (likely(count <= cmd->use_sg)) {
-		cmd->use_sg = count;
-		return 0;
-	}
-
-	eprintk("cmd %p cnt %d\n", cmd, cmd->use_sg);
-	scsi_free_sgtable(cmd);
-	return -EINVAL;
+	BUG_ON(count > cmd->use_sg);
+	cmd->use_sg = count;
+	return 0;
 }
 
 /* TODO: test this crap and replace bio_map_user with new interface maybe */
-- 
1.5.2.5

Re: [PATCH] Attempt to get eject failures back to ioctl(CDROMEJECT)

[Jens Axboe]

On Thu, Nov 15 2007, Rusty Russell wrote:
> On Wednesday 14 November 2007 23:39:31 Jens Axboe wrote:
> > On Wed, Nov 14 2007, Rusty Russell wrote:
> > > Hi Jens,
> > >
> > >     As you asked for some time ago.  Of course, it turns out that the
> > > eject command ignores the error anyway, but it's nice that it now errors.
> > >
> > >     Not entirely comfortable with this patch: there's a req->errors but
> > > that seems to have some existing semantics I'm not sure of, so I simply
> > > added a new way of flagging an error.
> >
> > It is a bit of a hack, but it's not really your fault. ->errors is
> > somewhat messy and has different meaning depending on the request type.
> > I'll add your patch and then do a sanitize on top of it, so that we can
> > switch things over to a unified ->errno instead.
> 
> Thanks!
> 
> Oh, I also noticed this in scsi_tgt_lib:
> 
> From: Rusty Russell <rusty@rustcorp.com.au>
> Date: Fri, 9 Nov 2007 20:04:54 +1100
> Subject: [PATCH] scsi_tgt_lib: BUG_ON() impossible condition.
> 
> If blk_rq_map_sg returns more than was allocated, it's a bug, and something's
> already been overwritten.  BUG_ON() is probably the right thing here.

It really just means that it mapped more segments than the block layer
said it would. Usually that wont overwrite memory here since scsi rounds
up on allocating the sg list, but it indeed can. Similar code has been
in scsi_lib.c for ages, I'd suggest covering that in the same patch.

-- 
Jens Axboe

Re: [PATCH] Attempt to get eject failures back to ioctl(CDROMEJECT)

[Rusty Russell]

On Thursday 15 November 2007 19:47:05 Jens Axboe wrote:
> On Thu, Nov 15 2007, Rusty Russell wrote:
> > If blk_rq_map_sg returns more than was allocated, it's a bug, and
> > something's already been overwritten.  BUG_ON() is probably the right
> > thing here.
>
> It really just means that it mapped more segments than the block layer
> said it would. Usually that wont overwrite memory here since scsi rounds
> up on allocating the sg list, but it indeed can. Similar code has been
> in scsi_lib.c for ages, I'd suggest covering that in the same patch.

Good point.  I assume that you've not seen these printks in recent memory?

This covers both cases:

Subject: [PATCH] scsi: BUG_ON() impossible condition.

If blk_rq_map_sg wrote more than was allocated in the scatterlist,
BUG_ON() is probably the right thing to do.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/scsi/scsi_tgt_lib.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1102,7 +1102,6 @@ void scsi_io_completion(struct scsi_cmnd
  *
  * Returns:     0 on success
  *		BLKPREP_DEFER if the failure is retryable
- *		BLKPREP_KILL if the failure is fatal
  */
 static int scsi_init_io(struct scsi_cmnd *cmd)
 {
@@ -1136,17 +1135,9 @@ static int scsi_init_io(struct scsi_cmnd
 	 * each segment.
 	 */
 	count = blk_rq_map_sg(req->q, req, cmd->request_buffer);
-	if (likely(count <= cmd->use_sg)) {
-		cmd->use_sg = count;
-		return BLKPREP_OK;
-	}
-
-	printk(KERN_ERR "Incorrect number of segments after building list\n");
-	printk(KERN_ERR "counted %d, received %d\n", count, cmd->use_sg);
-	printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
-			req->current_nr_sectors);
-
-	return BLKPREP_KILL;
+	BUG_ON(count > cmd->use_sg);
+	cmd->use_sg = count;
+	return BLKPREP_OK;
 }
 
 static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c
--- a/drivers/scsi/scsi_tgt_lib.c
+++ b/drivers/scsi/scsi_tgt_lib.c
@@ -367,14 +367,9 @@ static int scsi_tgt_init_cmd(struct scsi
 
 	dprintk("cmd %p cnt %d %lu\n", cmd, cmd->use_sg, rq_data_dir(rq));
 	count = blk_rq_map_sg(rq->q, rq, cmd->request_buffer);
-	if (likely(count <= cmd->use_sg)) {
-		cmd->use_sg = count;
-		return 0;
-	}
-
-	eprintk("cmd %p cnt %d\n", cmd, cmd->use_sg);
-	scsi_free_sgtable(cmd);
-	return -EINVAL;
+	BUG_ON(count > cmd->use_sg);
+	cmd->use_sg = count;
+	return 0;
 }
 
 /* TODO: test this crap and replace bio_map_user with new interface maybe */

Re: [PATCH] Attempt to get eject failures back to ioctl(CDROMEJECT)

[Jens Axboe]

On Thu, Nov 15 2007, Rusty Russell wrote:
> On Thursday 15 November 2007 19:47:05 Jens Axboe wrote:
> > On Thu, Nov 15 2007, Rusty Russell wrote:
> > > If blk_rq_map_sg returns more than was allocated, it's a bug, and
> > > something's already been overwritten.  BUG_ON() is probably the right
> > > thing here.
> >
> > It really just means that it mapped more segments than the block layer
> > said it would. Usually that wont overwrite memory here since scsi rounds
> > up on allocating the sg list, but it indeed can. Similar code has been
> > in scsi_lib.c for ages, I'd suggest covering that in the same patch.
> 
> Good point.  I assume that you've not seen these printks in recent memory?

I have not, they usually show up if we have bugs in the merge accounting
logic in the block layer (the merge functions and blk_rq_map_sg() not
agreeing). It's been ages since that was an issue, so I'm fine with the
bug.

> This covers both cases:
> 
> Subject: [PATCH] scsi: BUG_ON() impossible condition.

You can add my

Acked-by: Jens Axboe <jens.axboe@oracle.com>

when you pass it through James, it should go in that way.

-- 
Jens Axboe