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

[Jens Axboe <axboe@suse.de>]

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.

-- 
Jens Axboe