__bio_clone() behaviour

With the __bio_clone() bugfix by Jens Axboe (in the recent 2.6.10-ac kernels or SLES9 SP1) which causes __bio_clone() to copy the bi_io_vec, it would always try to copy the _maximum_ size, as defined by bio_src->bi_max_vecs:

inline void __bio_clone(struct bio *bio, struct bio *bio_src)

{
        request_queue_t *q = bdev_get_queue(bio_src->bi_bdev);
        memcpy(bio->bi_io_vec, bio_src->bi_io_vec, bio_src->bi_max_vecs * sizeof(struct bio_vec));

...

drbd however only has space for a single iovec (because it's all statically allocated right now), and so the memcpy would silently overwrite memory.

The attached patch 'fixes' this up.

Note that it is a bit ugly but safe, as drbd already asserts that bi_vcnt == 1 anyway.

FWIW, drbd seems to be the only user of __bio_clone() I could find, there's no in-tree users, everything goes through bio_clone() otherwise, which would have dynamically allocated the properly sized structures.

Sincerely,
Lars Marowsky-Brée <[email protected]>

High Availability & Clustering
SUSE Labs, Research and Development
SUSE LINUX Products GmbH - A Novell Business

Index: drbd_compat_wrappers.h
===================================================================
@@ -538,7 +538,9 @@
 	bio_init(bio); // bio->bi_flags   = 0;
 	bio->bi_io_vec = bvec;
 	bio->bi_max_vecs = 1;
-
+	
+	/* FIXME: __bio_clone() workaround, fix me properly later! */
+	bio_src->bi_max_vecs = 1;
 	__bio_clone(bio,bio_src);
 	bio->bi_bdev    = mdev->backing_bdev;
 	bio->bi_private = mdev;

@@ -559,6 +561,8 @@
 	bio->bi_io_vec = bvec;
 	bio->bi_max_vecs = 1;
 
+	/* FIXME: __bio_clone() workaround, fix me properly later! */
+	bio_src->bi_max_vecs = 1;
 	__bio_clone(bio,bio_src);
 	bio->bi_bdev    = mdev->backing_bdev;
 	bio->bi_private = mdev;

Am Mittwoch, 26. Januar 2005 17:46 schrieb Lars Marowsky-Bree:

This really gives me the feeling that I should not have done the 0.7.9 release. --- The only one to blame is myself, who thought it would be a nice idea to have the same release as SUSE. Bullshit -> The next release will only happen when I am convinced that the new release is necessary, and that it will not be a disaster.

BTW, regarind this patch: We now modify someone else's BIO.
Is this a good idea ?

-Philipp

: Dipl-Ing Philipp Reisner                      Tel +43-1-8178292-50 :
: LINBIT Information Technologies GmbH          Fax +43-1-8178292-82 :
: Schönbrunnerstr 244, 1120 Vienna, Austria    http://www.linbit.com :

Yeah, that's probably a good idea. We're used to releasing patched versions anyway ;-)

(The 0.7.9 we had picked up had to have it's version number (and only the version number) patched back to 0.7.5; can't update the version number in a released product, even if the rest of the code is the same. That's political games for you. *sigh*)

Well, quite frankly, it is not entirely a good idea. However, it's as sane as the other stuff drbd does with bios. It doesn't have any other side effect beyond making the __bio_clone() call not misbehave.

The bi_max_vecs doesn't have any other uses in that scenario and it's safe, but not sane ;-)

I checked with axboe and grepped the kernel source, and it's the best work around we can do right now short of converting drbd to use the "proper" APIs. Jens thinks the __bio_clone() call probably should be unexported; the "real" API (bio_clone()) would have automatically done the right thing, and in fact drbd seems to be the _only_ user of __bio_clone() anywhere. Everyone else goes via bio_alloc(), bio_clone(), bio_get/_put() et cetera.

Sincerely,
Lars Marowsky-Brée <[email protected]>

High Availability & Clustering
SUSE Labs, Research and Development
SUSE LINUX Products GmbH - A Novell Business