[Drbd-dev] Re: drbd uses wrong API for struct bio

[Jens Axboe]

On Sun, Jan 23 2005, Lars Marowsky-Bree wrote:
> Hi all,
> 
> drbd is a quite bad offender for using bio's on stack or embedded in
> other internal structs instead of the pointer interface.
> 
> The attached patch shows the 'right' way as an example, which was pretty easy
> because I could use Jens' patch for md with minimal modifications ;-) However,
> the other offending code lines are within the transfer log and other places,
> and I'm not sure Philipp wants me to mess around with that.
> 
> Jens, on chip.suse.de:/local/lmb/drbd-07/drbd/ you can find the most
> recent drbd kernel code. Could you look over drbd_compat_wrappers.h
> line 326ff in particular and give me a rough rundown of what's broken?
> We need to fix this by Tuesday latest :-(

drbd_ee_init() doesn't look good. First of all, it makes assumptions
about what bio_init() would do - it doesn't clear the entire bio to 0.
This is the type of thing that exposes bugs when we add or change parts
of the bio stuff, please uncomment that bio_init() call. It does set
bio->bi_max_vecs, however it needs changing to passing the bdev in so we
can use bio_add_page() instead. drbd_ee_bio_prepare() looks strange, it
sets vec length but doesn't assign the page or offset. Again, it should
use the proper api (bio_add_page()). If it had used bio_init(), it would
not have to set BIO_UPTODATE manually either.

bio->bi_next is a pointer, not an integer. Use NULL.

drbd_blk_run_queue() doesn't look legal, unless it's only ever run on
drbd's own queue. Even if so, it should remove the plug. I'd suggest
just using generic_unplug_device().

drdb should just use bio_iovec(bio) when it uses bio->bi_idx as the
index anyways.

The bio embedding may not be the best idea or the prettiest, but as long
as it sets ->bi_max_vecs it should be ok. In general the code could
really do with someone cleaning it up in preperation for submitting to
mainline...

-- 
Jens Axboe