Re: [PATCH] 2.5.20 IDE 85

[Martin Dalecki]

Jens Axboe wrote:
> On Wed, Jun 05 2002, Martin Dalecki wrote:
> 
>>Jens Axboe wrote:
>>
>>>On Wed, Jun 05 2002, Jens Axboe wrote:
>>>
>>>
>>>>On Wed, Jun 05 2002, Martin Dalecki wrote:
>>>>
>>>>
>>>>>Jens Axboe wrote:
>>>>>
>>>>>
>>>>>>On Wed, Jun 05 2002, Martin Dalecki wrote:
>>>>>>
>>>>>>AFAICS, you just introduced some nasty list races in the interrupt
>>>>>>handlers. You must hold the queue locks when calling
>>>>>>blkdev_dequeue_request() and end_that_request_last(), for instance.
>>>>>>
>>>>>
>>>>>No. Please be more accurate. Becouse:
>>>>>
>>>>>1. If anything I have made existing races only "obvious".
>>>>
>>>>If anything, you've made a race you introduced earlier more obvious.
>>>>
>>>>
>>>>
>>>>>2. It is called in the context of do_ide_request or ide_raw_taskfile
>>>>> where we already have the lock.
>>>>
>>>>?? Both tcq and ata_special_intr look like interrupt handlers to me.
>>>
>>>
>>>BTW, I wanted to look at the code (and not just read the patch), but
>>>it's not clear from the patch what it is against. Where do you keep
>>>older patches so I can get them? Maybe the ide code could do with a bit
>>>of peer review :-)
>>>
>>
>>Well IDE 83 and 84 are already inside the bk repository at linux.bkbits.com.
>>No as far as of now I don't have any public FTP or whatever area for
>>the patches (Well send you everything in one go.)
> 
> 
> Thanks. Just ask hpa for a kernel.org dir, if you don't have anywhere
> else to keep it.
> 
> 
>>And I of course agree that the code needs a peer review in this area.
>>Adding the locking isn't difficult.
> 
> 
> Of course not, discovering the missing locking is most of the work. And
> of course acknowedging that there's a problem :-)

Just to make sure that we understand each other. I admitt that
you are right there have to be locks there. As well as around any host chip
access. Thanks for the reminder.
And well please note that the last patches in esp. are trying
hard to reducde the number of possible code flow branch cases.

>>However I wonder a bit whatever we couldn't just blkdev_dequeue_request()
>>once at request handling start? We drag drive->rq around anyway...
> 
> 
> I did that once a long time ago, but it was very broken because the IDE
> code would end up in ide_do_request() several times at times before a
> request was started. I think there are advantages both ways: leaving the
> request on the queue until it is done allows the i/o scheduler to know
> what the disk is currently working on. Removing it is potentially a bit
> cleaner, however most of the reason for that has long been reworked in
> 2.5 (the plugging and head active stuff).

Yes I remember - the IDE/SCSI queue head differencies. Making
them to behave entierly equal would have some charm too...
However the multi ide_do_request entry problems should
have got *much* better due to the reduction of the multiple
submitted/injected artifical request (REQ_ and friends) types.
The remaining one is now only ide_raw_taskfile
and the regular onces. Well ide-tape and packet command handling aside...
but whom I'm telling this ... I'm sure you already know...