Re: Nbd is broken

From: Andrea Arcangeli (andrea@suse.de)
Date: Mon Mar 06 2000 - 09:17:27 EST


On Mon, 6 Mar 2000, Alexander Viro wrote:

>AFAICS the original logics looked so:
> a) you update ->sector and ->nr_sectors in the same place where
>you do the IO. E.g. in the interrupt handler.

This make no sense IMHO. Why don't you also update the rest? Without
updating the rest only updating such two fields is useless. You lose
synchrony with the fields updating them in different places.

head_active drivers could have no knowledge at all about merging an
end_request could do all the work as it does when there's an error.

> c) if you got an error you _skip_ the rest of segment. Thus the
>code in end_request().

No, if there's an error you don't skip the rest of segment because
end_that_request_first returns zero if there are more bhs to process in
the request. This also make sense since if there's a defective sector it
doesn't mean the following sector is defective too.

> d) if you got more segments - just reinsert the thing into queue.

That is a special implementation case of loop. Normal headactive devices
left the req into the queue. Smart devices like scsi removes it forever
and never reinserts it.

>Now, it worked fine for almost everything. Two exceptions were loop and
>nbd (monkied up from loop). Here the thing didn't do (a) (since there was
>no obvious place for that? Hell knows) and was explicitly protected from
>multi-segment requests. When that protection went away the shit had hit
>the fan. Badly. Especially since it got (d), so it overwrote the first
>segment with latter ones instead of skipping them. It became really bad
>when plugging logics changed (couple of releases after the first change),
>because now requests got merged much more often.

That problem has anything to do with plugging logic. If the request_fn
blocks dropping the lock even if you avoid plugging you can have merged
requests in the queue.

If we want to provide backwards compatibility we must add a
"merging" field in the blk_dev_struct and skip the merging path if it's
set to zero.

>> end_request should really hide the merging trasparently. Fixing that is
>> trivial but all the blockdevices that was wokarounding that design bug
>> will have to be ported to the new semantic and that's some time not
>> obvious. It's certainly a cleanup to do in the long term IMHO.
>
>I'm not sure. Updating the ->sector and ->nr_sectors at the actual IO time
>looks perfectly sane to me. Code in end_request() is rather a "OK, we got
>an error, so let's skip the rest of this segment" thing.

It's very insane IMHO (see first paragraph).

Andrea

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/



This archive was generated by hypermail 2b29 : Tue Mar 07 2000 - 21:00:19 EST