Attempt to get eject failures back to ioctl(CDROMEJECT)

PATCH In review Rusty Russell 2 participants 6 messages Nov 14, 2007 – Nov 15, 2007

Subsystems (4)

SCSI Block layer virtio core virtio block and SCSI drivers

lore

Share & tools

lore

Rusty Russell

author

2007-11-14 06:39 UTC #0 lore raw

PATCH 1/1 Attempt to get eject failures back to ioctl(CDROMEJECT)

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 <[email protected]>

block/ll_rw_blk.c +7 -2

diff -r a7da575b248f block/ll_rw_blk.c
@@ -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

drivers/block/virtio_blk.c +1 -1

diff -r a7da575b248f drivers/block/virtio_blk.c
@@ -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;

include/linux/blkdev.h +2 -0

diff -r a7da575b248f include/linux/blkdev.h
@@ -281,6 +281,8 @@ struct request {
 
 	int tag;
 	int errors;
+	/* If this isn't set, assume -EIO. */
+	int errno;
 
 	int ref_count;

Jens Axboe

2007-11-14 12:40 UTC #1 lore raw

Quoted 8 lines · 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.

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.

Signature · 1 line

Jens Axboe

Rusty Russell

author

2007-11-15 02:36 UTC #2 lore raw

Quoted 6 lines · On Wednesday 14 November 2007 23:39:31 Jens Axboe wrote:

Quoted 5 lines · 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.

Quoted 8 lines

Quoted 3 lines
    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 <[email protected]>
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 <[email protected]>

---
 drivers/scsi/scsi_tgt_lib.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

drivers/scsi/scsi_tgt_lib.c +3 -8

index a91761c..66266c8 100644
@@ -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 */

Signature · 1 line

1.5.2.5

Jens Axboe

2007-11-15 08:47 UTC #3 lore raw

Quoted 18 lines · Rusty Russell

Quoted 6 lines · On Wednesday 14 November 2007 23:39:31 Jens Axboe wrote:
Quoted 5 lines · 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.
Quoted 8 lines
Quoted 3 lines
    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!

Quoted 8 lines

Oh, I also noticed this in scsi_tgt_lib:
From: Rusty Russell <[email protected]>
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.

Signature · 1 line

Jens Axboe

Rusty Russell

author

2007-11-15 11:32 UTC #4 lore raw

Quoted 9 lines · On Thursday 15 November 2007 19:47:05 Jens Axboe wrote:

Quoted 3 lines · Rusty Russell
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 <[email protected]>

---
 drivers/scsi/scsi_tgt_lib.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

drivers/scsi/scsi_lib.c +3 ~~-12~~

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

drivers/scsi/scsi_tgt_lib.c +3 -8

@@ -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 */

Jens Axboe

Acked-by

2007-11-15 11:36 UTC #5 lore raw

Quoted 6 lines · Rusty Russell

Quoted 5 lines · On Thursday 15 November 2007 19:47:05 Jens Axboe wrote:
Quoted 3 lines · Rusty Russell
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.

Quoted 6 lines

Quoted 4 lines
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.

Quoted 3 lines

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

You can add my

Acked-by Jens Axboe <[email protected]>

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

Signature · 1 line

Jens Axboe