2.5.30 IDE 113

Uz.ytkownik Petr Vandrovec napisa?:

Quoted 5 lines

Hi Marcin,
what synchronizes these accesses to make sure that you do not have
two ide_raw_taskfile requests on the flight, both using same drive->srequest? It looks to me like that nothing, so you can overwrite request's contents while somebody else already uses this buffer.

I don't think so. The queue lock is synchronizing them.
And then we usually add them just to the front of the queue in question and wait for finishment until the request is done.

After all ide_raw_taskfile only gets used for REQ_SPECIAL request types. This does *not* contain normal data request from block IO. As of master slave issues - well we have the data pre allocated per device not per channel! If q->request_fn would properly return the error count instead of void, we could even get rid ot the checking for rq->errors after finishment... But well that's entierly different story.

Quoted 7 lines

After all ide_raw_taskfile only gets used for REQ_SPECIAL request types. This does *not* contain normal data request from block IO. As of master slave issues - well we have the data pre allocated per device not per channel! If q->request_fn would properly return the error count instead of void, we could even get rid ot the checking for rq->errors after finishment... But well that's entierly different story.

For example do_cmd_ioctl() invokes ide_raw_taskfile, without any locking. Two programs, both issuing HDIO_DRIVE_CMD at same time, will compete over one drive->srequest struct: you'll get same drive->srequest structure submitted twice to blk_insert_request (hm, Jens, will this trigger BUG, or will this just damage request list?).

Quoted 9 lines

Quoted 8 lines

After all ide_raw_taskfile only gets used for REQ_SPECIAL request types. This does *not* contain normal data request from block IO. As of master slave issues - well we have the data pre allocated per device not per channel! If q->request_fn would properly return the error count instead of void, we could even get rid ot the checking for rq->errors after finishment... But well that's entierly different story.

Quoted 5 lines

For example do_cmd_ioctl() invokes ide_raw_taskfile, without any locking. Two programs, both issuing HDIO_DRIVE_CMD at same time, will compete over one drive->srequest struct: you'll get same drive->srequest structure submitted twice to blk_insert_request (hm, Jens, will this trigger BUG, or will this just damage request list?).

Just silently damage request list. We _could_ easily add code to detect this, but it's not been a problem in the past so not worth looking for.

AFAICS, Petr is completely right wrt this race.

Uz.ytkownik Jens Axboe napisa?:

Quoted 20 lines · Petr Vandrovec

Quoted 9 lines

Quoted 8 lines

After all ide_raw_taskfile only gets used for REQ_SPECIAL request types. This does *not* contain normal data request from block IO. As of master slave issues - well we have the data pre allocated per device not per channel! If q->request_fn would properly return the error count instead of void, we could even get rid ot the checking for rq->errors after finishment... But well that's entierly different story.

Quoted 5 lines

For example do_cmd_ioctl() invokes ide_raw_taskfile, without any locking. Two programs, both issuing HDIO_DRIVE_CMD at same time, will compete over one drive->srequest struct: you'll get same drive->srequest structure submitted twice to blk_insert_request (hm, Jens, will this trigger BUG, or will this just damage request list?).

Just silently damage request list. We _could_ easily add code to detect this, but it's not been a problem in the past so not worth looking for.

Quoted 1 line

AFAICS, Petr is completely right wrt this race.

For the ioctl case yes. But:

1. We already look for blk_queue_empty there.
2. We have just to deal properly with the queue plugging there to close it up.

3. I will just add spin locking on ide_lock to maintain that no two ioctl can overlapp at all.

From ide_raw_taskfile(), the right way to do it is:

OK?

Agrh god no. So you'll spin waiting for the ioctl to complete?

Quoted 1 line

From ide_raw_taskfile(), the right way to do it is:

	struct request *rq = blk_get_request(...);

This gets _everything_ right.

BTW, _glad to see you got rid of the horrible insert-and-execute stuff in ide_raw_taskfile(). That was a layering violation.

Quoted 1 line

OK?

Not likely :-)

Uz.ytkownik Jens Axboe napisa?:

Quoted 8 lines

Agrh god no. So you'll spin waiting for the ioctl to complete?

Quoted 1 line

From ide_raw_taskfile(), the right way to do it is:

	struct request *rq = blk_get_request(...);

This gets _everything_ right.

Quoted 8 lines

BTW, _glad to see you got rid of the horrible insert-and-execute stuff in ide_raw_taskfile(). That was a layering violation.

Quoted 1 line

OK?

Not likely :-)

Argh. Yes. Thank's for the back-head slap.
I was looking too much at the SCSI code again and got it wrong. But some time ago I was already thinking about blk_get_request(). How could I maintain that the blk_get_request() really returns? blk_get_request() does only drain up to maximum queue depth as far as I can read the code and then bad things wil happen :-). Or should I just not worry?