On 11/16/05, Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx> wrote: > On 11/16/05, Jens Axboe <axboe@xxxxxxx> wrote: > > On Wed, Nov 16 2005, Bartlomiej Zolnierkiewicz wrote: > > > On 11/16/05, Jens Axboe <axboe@xxxxxxx> wrote: > > > > On Wed, Nov 16 2005, Bartlomiej Zolnierkiewicz wrote: > > > > > On 11/16/05, Jens Axboe <axboe@xxxxxxx> wrote: > > > > > > On Wed, Nov 16 2005, Bartlomiej Zolnierkiewicz wrote: > > > > > > > On 11/16/05, Jens Axboe <axboe@xxxxxxx> wrote: > > > > > > > > > > > > > > > I updated that patch, and converted IDE and SCSI to use it. See the > > > > > > > > results here: > > > > > > > > > > > > > > > > http://brick.kernel.dk/git/?p=linux-2.6-block.git;a=shortlog;h=blk-softirq > > > > > > > > > > > > > > I like it but: > > > > > > > > > > > > > > * "we know it's either an FS or PC request" assumption in > > > > > > > ide_softirq_done() is really wrong > > > > > > > > > > > > It used to be correct :-) > > > > > > > > > > Sorry but it has been always like that, > > > > > other requests also pass through ide_end_request() > > > > > (which of course needs fixing). > > > > > > > > You misunderstand, for calls to blk_complete_request() it wasn't true > > > > initially since it always obyed rq_all_done() (which returns 0 for > > > > non-fs and non-pc requests). > > > > > > from blk_complete_request() [ the only user of rq_all_done() ]: > > > > > > + /* > > > + * for partial completions, fall back to normal end io handling. > > > + */ > > > + if (unlikely(!partial_ok && !rq_all_done(req, nbytes))) > > > + if (end_that_request_chunk(req, uptodate, nbytes)) > > > + return 1; > > > > > > We still will end up with using ide_softirq_done() for !rq_all_done() > > > case (non FS/PC request) because majority of them (all?) don't use > > > partial completions. > > > > Yes, that's what it looks like now... Note I wrote "wasn't", it used to > > look like this: > > > > if (!rq_all_done(req, nbytes)) { > > end_that_request_chunk(..); > > return; > > } > > > > which of course didn't work, so it was changed to the above which then > > broke the assumption of what type of requests we expect to see in > > ide_softirq_done(). We can't generically handle this case, so it's > > probably best to just add this logic to __ide_end_request() - it's just > > another case for _not_ using the blk_complete_request() path, just like > > the partial case. > > Sounds better but I honestly think that you simply cannot obtain > reliable nr_sectors to complete for FS/PC requests just from the > request type. Two examples are: failed disk flush requests and > cd noretry requests (both are of FS type). first example is bad :-) > IMO the best way to fix it is to actually move more (not less!) of > the logic from driver->end_request() paths to ide_softirq_done(). Your latest patch is also a good way to fix it (now the only thing left is rq->errors/rq->retries discussed earlier). Bartlomiej - : send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
