Re: [PATCH] libata error handling fixes (ATAPI)

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On 11/16/05, Jens Axboe <axboe@suse.de> wrote:
> On Wed, Nov 16 2005, Bartlomiej Zolnierkiewicz wrote:
> > On 11/16/05, Jens Axboe <axboe@suse.de> wrote:
> > > On Wed, Nov 16 2005, Bartlomiej Zolnierkiewicz wrote:
> > > > On 11/16/05, Jens Axboe <axboe@suse.de> wrote:
> > > > > On Wed, Nov 16 2005, Bartlomiej Zolnierkiewicz wrote:
> > > > > > On 11/16/05, Jens Axboe <axboe@suse.de> 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).

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

Cheers,
Bartlomiej