block: Fix CDROMEJECT to work in more cases

PATCH In review Ben Collins 4 participants 11 messages Dec 19, 2005 – Dec 21, 2005 Block layer

Share & tools

Ben Collins

author

2005-12-19 20:59 UTC #0 lore raw

PATCH 1/1 block: Fix CDROMEJECT to work in more cases

This changes the request to a READ instead of WRITE. Also adds and calls blk_send_allow_medium_removal() for CDROMEJECT case.

Signed-off-by Ben Collins <[email protected]>

block/scsi_ioctl.c renamed from block/scsi_ioctl.c~ +11 -2

@@ -449,7 +449,7 @@
 	struct request *rq;
 	int err;
 
-	rq = blk_get_request(q, WRITE, __GFP_WAIT);
+	rq = blk_get_request(q, READ, __GFP_WAIT);
 	rq->flags |= REQ_BLOCK_PC;
 	rq->data = NULL;
 	rq->data_len = 0;

@@ -469,6 +469,11 @@
 	return __blk_send_generic(q, bd_disk, GPCMD_START_STOP_UNIT, data);
 }
 
+static inline int blk_send_allow_medium_removal(request_queue_t *q, struct gendisk *bd_disk)
+{
+	return __blk_send_generic(q, bd_disk, GPCMD_PREVENT_ALLOW_MEDIUM_REMOVAL, 0);
+}
+
 int scsi_cmd_ioctl(struct file *file, struct gendisk *bd_disk, unsigned int cmd, void __user *arg)
 {
 	request_queue_t *q;

@@ -593,7 +598,11 @@
 			err = blk_send_start_stop(q, bd_disk, 0x03);
 			break;
 		case CDROMEJECT:
-			err = blk_send_start_stop(q, bd_disk, 0x02);
+			err = 0;
+
+			err |= blk_send_allow_medium_removal(q, bd_disk);
+			err |= blk_send_start_stop(q, bd_disk, 0x01);
+			err |= blk_send_start_stop(q, bd_disk, 0x02);
 			break;
 		default:
 			err = -ENOTTY;

Linus Torvalds

2005-12-20 17:34 UTC #1 lore raw

Quoted 3 lines · Ben Collins

This changes the request to a READ instead of WRITE. Also adds and calls blk_send_allow_medium_removal() for CDROMEJECT case.

Can you tell why it also does that START_STOP/1 thing? That looks a bit strange.

Also, can somebody go through the READ/WRITE difference for me for a zero-length command? If the _only_ difference is a protection one (WRITE commands need write permissions), then I'm ok with this (I think it's very reasonable that somebody who can read a cd-rom can also eject it), but if there's some SCSI layer logic that says "writes cannot have length 0", then I think that's a bug.

Linus

Jens Axboe

2005-12-20 17:48 UTC #2 lore raw

Quoted 9 lines · Linus Torvalds

Quoted 3 lines · Ben Collins
This changes the request to a READ instead of WRITE. Also adds and calls blk_send_allow_medium_removal() for CDROMEJECT case.
Can you tell why it also does that START_STOP/1 thing? That looks a bit strange.

I still think that is weird and not something that should be merged. The 0x01 bit means load the tray and read TOC.

Quoted 6 lines

Also, can somebody go through the READ/WRITE difference for me for a zero-length command? If the _only_ difference is a protection one (WRITE commands need write permissions), then I'm ok with this (I think it's very reasonable that somebody who can read a cd-rom can also eject it), but if there's some SCSI layer logic that says "writes cannot have length 0", then I think that's a bug.

It has no logical implications other than from what pool it allocates, and we've always used the WRITE pool for these requests. There's no protection implications.

WRITEs cannot have length 0, and READs cannot as well. Since it's just one bit for direction, those are the rules.

Signature · 1 line

Jens Axboe

Linus Torvalds

2005-12-20 18:08 UTC #3 lore raw

Quoted 3 lines · Jens Axboe

WRITEs cannot have length 0, and READs cannot as well. Since it's just one bit for direction, those are the rules.

Jens, your logic doesn't make sense.

There clearly _are_ commands with a 0 data-length.

And commands _have_ to be either READ or WRITE. We don't have a choice.
ll_rw_block: blk_get_request() even has a BIG_ON() that enforces that.

So claiming that reads and writes cannot have zere data-length is INSANE.

So reads and writes HAVE to accept a zero data length. End of story. If there is some path in the SCSI layer that refuses it, that part must be fixed, or you have to add a new "NONE" (and perhaps "BOTH") direction.

Linus

Jens Axboe

2005-12-20 18:37 UTC #4 lore raw

Quoted 8 lines · Linus Torvalds

Quoted 3 lines · Jens Axboe
WRITEs cannot have length 0, and READs cannot as well. Since it's just one bit for direction, those are the rules.
Jens, your logic doesn't make sense.

It does

Quoted 1 line

There clearly _are_ commands with a 0 data-length.

Of course, that's not what I'm saying. What I am saying is interpreted from the driver or further down in the io stack.

Quoted 2 lines

And commands _have_ to be either READ or WRITE. We don't have a choice.
ll_rw_block: blk_get_request() even has a BIG_ON() that enforces that.

Yes, it has to choose one of the two pools.

Quoted 1 line

So claiming that reads and writes cannot have zere data-length is INSANE.

There are two sides to this - looking at the request allocations pools, yes if you want a request you have to tell from which pool you want it from. But a request that originates from that particular pool (in this case the write pool), does _not_ have to imply any transfer of data from a device!

Quoted 1 line

So reads and writes HAVE to accept a zero data length. End of story. If

That's not up for debate, of course that is the case. Otherwise we could not issue any request unless it needed to transfer data from a device. Just because an empty request happens to have the data direction bit set, does not mean it wants to transfer data to the device. By definition, that is an impossibility since there's nothing to transfer.

Quoted 2 lines

there is some path in the SCSI layer that refuses it, that part must be fixed, or you have to add a new "NONE" (and perhaps "BOTH") direction.

There _was_ a bug in the SCSI layer, because it had logic like this:

        if (rq_data_dir(req) == WRITE)
                DMA_TO_DEVICE
        else if (req->data_len)
                DMA_FROM_DEVICE
        else
                DMA_NONE

which was buggy, because for it to transfer data to the device, both the direction bit _and_ a data length must be set.

Signature · 1 line

Jens Axboe

Linus Torvalds

2005-12-20 18:53 UTC #5 lore raw

Quoted 10 lines · Jens Axboe

There _was_ a bug in the SCSI layer, because it had logic like this:

        if (rq_data_dir(req) == WRITE)
                DMA_TO_DEVICE
        else if (req->data_len)
                DMA_FROM_DEVICE
        else
                DMA_NONE

Quoted 2 lines

which was buggy, because for it to transfer data to the device, both the direction bit _and_ a data length must be set.

So this is fixed? Is that the iPod panic fix, or something else?

If so, I'll drop that patch (although the "allow_removal" part of it sounds sane to me still.. comments?)

Linus

Jens Axboe

2005-12-20 19:07 UTC #6 lore raw

Quoted 13 lines · Linus Torvalds

Quoted 10 lines · Jens Axboe
There _was_ a bug in the SCSI layer, because it had logic like this:
        if (rq_data_dir(req) == WRITE)
                DMA_TO_DEVICE
        else if (req->data_len)
                DMA_FROM_DEVICE
        else
                DMA_NONE

Quoted 4 lines

Quoted 2 lines
which was buggy, because for it to transfer data to the device, both the direction bit _and_ a data length must be set.
So this is fixed? Is that the iPod panic fix, or something else?

Yes it's fixed, James merged the fix(es) with you last week.

Quoted 2 lines

If so, I'll drop that patch (although the "allow_removal" part of it sounds sane to me still.. comments?)

I guess that's fine with me, the only thing I reject to is the 0x01 bit.

Signature · 1 line

Jens Axboe

Ben Collins

2005-12-20 20:39 UTC #7 lore raw

In reply to Linus Torvalds

Quoted 10 lines · Linus Torvalds

Quoted 3 lines · Jens Axboe
WRITEs cannot have length 0, and READs cannot as well. Since it's just one bit for direction, those are the rules.
Jens, your logic doesn't make sense.
There clearly _are_ commands with a 0 data-length.

Quoted 8 lines

And commands _have_ to be either READ or WRITE. We don't have a choice.
ll_rw_block: blk_get_request() even has a BIG_ON() that enforces that.
So claiming that reads and writes cannot have zere data-length is INSANE.
So reads and writes HAVE to accept a zero data length. End of story. If there is some path in the SCSI layer that refuses it, that part must be fixed, or you have to add a new "NONE" (and perhaps "BOTH") direction.

I think most of the problem is that once it got down to the scsi layer, there were some bugs with data direction, and it confused things like usb-storage and sbp2 on firewire.

Those bugs were fixed. Note, I did not test the ALLOW_MEDIUM_REMOVAL fix with WRITE commands after going to -rc6 (I used -rc5 for testing), so those direction fixes may actually make ALLOW_MEDIUM_REMOVAL work.

However, I don't see the issue with using READ. We know this isn't a write operation, we are sending a single command with no data. I know you say reads are precious, but 3 requests for something that isn't going to happen very often doesn't seem that bad.

As for the 0x01, I don't know. The eject -s code does the exact same thing (AMR, SS:0x01, SS:0x02), so I copied the same mechanism because it is known to work.

Signature · 3 lines

   Ben Collins <[email protected]>
   Developer
   Ubuntu Linux

Jens Axboe

2005-12-20 20:51 UTC #8 lore raw

Quoted 4 lines · Ben Collins

However, I don't see the issue with using READ. We know this isn't a write operation, we are sending a single command with no data. I know you say reads are precious, but 3 requests for something that isn't going to happen very often doesn't seem that bad.

It's not a READ either!

Yes I'm being stubborn, but my point stands. I'm not changing something that is perfectly valid, "just because". If it finds a bug (you mentioned ide-cd, I still want the details on that when you have the time), then it's all for the better since it would bite us for other paths as well.

In summary - it's not a bug, it doesn't need fixing.

Quoted 3 lines

As for the 0x01, I don't know. The eject -s code does the exact same thing (AMR, SS:0x01, SS:0x02), so I copied the same mechanism because it is known to work.

Lets leave that out for now then, yes?

Signature · 1 line

Jens Axboe

Ben Collins

2005-12-20 21:47 UTC #9 lore raw

Quoted 8 lines · Jens Axboe

Quoted 4 lines · Ben Collins
However, I don't see the issue with using READ. We know this isn't a write operation, we are sending a single command with no data. I know you say reads are precious, but 3 requests for something that isn't going to happen very often doesn't seem that bad.
It's not a READ either!

Quoted 7 lines

Yes I'm being stubborn, but my point stands. I'm not changing something that is perfectly valid, "just because". If it finds a bug (you mentioned ide-cd, I still want the details on that when you have the time), then it's all for the better since it would bite us for other paths as well.
In summary - it's not a bug, it doesn't need fixing.

Then for the sake of nothing other than consistency, fix sg_io() to use WRITE for cases where data_len==0? That means it would use READ only when data is actually being read, and WRITE for everything else, including all zero data commands (sounds sort of backwards to me, though). Currently, it does the opposite. The main point being that sending these commands from SG_IO ioctl should be the same as they get sent from CDROMEJECT ioctl.

I wonder how many bugs will pop up if you do that. Probably less now that the scsi code is fixed.

Signature · 3 lines

   Ben Collins <[email protected]>
   Developer
   Ubuntu Linux

Jens Axboe

2005-12-21 07:02 UTC #10 lore raw

Quoted 25 lines · Ben Collins

Quoted 8 lines · Jens Axboe
Quoted 4 lines · Ben Collins
However, I don't see the issue with using READ. We know this isn't a write operation, we are sending a single command with no data. I know you say reads are precious, but 3 requests for something that isn't going to happen very often doesn't seem that bad.
It's not a READ either!
Quoted 7 lines
Yes I'm being stubborn, but my point stands. I'm not changing something that is perfectly valid, "just because". If it finds a bug (you mentioned ide-cd, I still want the details on that when you have the time), then it's all for the better since it would bite us for other paths as well.
In summary - it's not a bug, it doesn't need fixing.
Then for the sake of nothing other than consistency, fix sg_io() to use WRITE for cases where data_len==0? That means it would use READ only when data is actually being read, and WRITE for everything else, including all zero data commands (sounds sort of backwards to me, though). Currently, it does the opposite. The main point being that sending these commands from SG_IO ioctl should be the same as they get sent from CDROMEJECT ioctl.

Quoted 2 lines

I wonder how many bugs will pop up if you do that. Probably less now that the scsi code is fixed.

We can try that for the next -rc1, but I don't think it will find a lot of bugs to be honest. I'm really surprised that the SCSI bug was there, it's one of those pretty basic things that should have been caught sooner. Perhaps with more coverage, we can try changing sg_io() after 2.6.15 release.

Signature · 1 line

Jens Axboe