[PATCH rc6] block: Fix CDROMEJECT to work in more cases

[PATCH rc6] block: Fix CDROMEJECT to work in more cases

[Ben Collins]

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 <bcollins@ubuntu.com>

--- a/block/scsi_ioctl.c~	2005-12-19 15:44:06.000000000 -0500
+++ b/block/scsi_ioctl.c	2005-12-19 15:46:43.000000000 -0500
@@ -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;

Re: [PATCH rc6] block: Fix CDROMEJECT to work in more cases

[Linus Torvalds]

On Mon, 19 Dec 2005, Ben Collins wrote:
>
> 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

Re: [PATCH rc6] block: Fix CDROMEJECT to work in more cases

[Jens Axboe]

On Tue, Dec 20 2005, Linus Torvalds wrote:
> 
> 
> On Mon, 19 Dec 2005, Ben Collins wrote:
> >
> > 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.

> 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.

-- 
Jens Axboe

Re: [PATCH rc6] block: Fix CDROMEJECT to work in more cases

[Linus Torvalds]

On Tue, 20 Dec 2005, Jens Axboe wrote:
> 
> 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

Re: [PATCH rc6] block: Fix CDROMEJECT to work in more cases

[Jens Axboe]

On Tue, Dec 20 2005, Linus Torvalds wrote:
> 
> 
> On Tue, 20 Dec 2005, Jens Axboe wrote:
> > 
> > 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

> 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.

> 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.

> 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!

> 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.

> 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.

-- 
Jens Axboe

Re: [PATCH rc6] block: Fix CDROMEJECT to work in more cases

[Linus Torvalds]

On Tue, 20 Dec 2005, Jens Axboe wrote:
> 
> 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.

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

Re: [PATCH rc6] block: Fix CDROMEJECT to work in more cases

[Jens Axboe]

On Tue, Dec 20 2005, Linus Torvalds wrote:
> 
> 
> On Tue, 20 Dec 2005, Jens Axboe wrote:
> > 
> > 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.
> 
> 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.

> 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.

-- 
Jens Axboe

Re: [PATCH rc6] block: Fix CDROMEJECT to work in more cases

[Ben Collins]

On Tue, 2005-12-20 at 10:08 -0800, Linus Torvalds wrote:
> 
> On Tue, 20 Dec 2005, Jens Axboe wrote:
> > 
> > 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.

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.

-- 
   Ben Collins <ben.collins@ubuntu.com>
   Developer
   Ubuntu Linux

Re: [PATCH rc6] block: Fix CDROMEJECT to work in more cases

[Jens Axboe]

On Tue, Dec 20 2005, Ben Collins wrote:
> 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.

> 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?

-- 
Jens Axboe

Re: [PATCH rc6] block: Fix CDROMEJECT to work in more cases

[Ben Collins]

On Tue, 2005-12-20 at 21:53 +0100, Jens Axboe wrote:
> On Tue, Dec 20 2005, Ben Collins wrote:
> > 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.

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.

-- 
   Ben Collins <ben.collins@ubuntu.com>
   Developer
   Ubuntu Linux

Re: [PATCH rc6] block: Fix CDROMEJECT to work in more cases

[Jens Axboe]

On Tue, Dec 20 2005, Ben Collins wrote:
> On Tue, 2005-12-20 at 21:53 +0100, Jens Axboe wrote:
> > On Tue, Dec 20 2005, Ben Collins wrote:
> > > 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.
> 
> 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.

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.

-- 
Jens Axboe