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

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

[Lars Marowsky-Bree]

[-- Attachment #1.1: Type: text/plain, Size: 873 bytes --]

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 :-(


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

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


[-- Attachment #1.2: drbd-bio-on-stack --]
[-- Type: text/plain, Size: 1366 bytes --]

Index: drbd/drbd_actlog.c
===================================================================
--- drbd/drbd_actlog.c	(revision 1730)
+++ drbd/drbd_actlog.c	(working copy)
@@ -64,35 +64,29 @@
 STATIC int _drbd_md_sync_page_io(drbd_dev *mdev, struct page *page, 
 				 sector_t sector, int rw, int size)
 {
-	struct bio bio;
-	struct bio_vec vec;
+	struct bio *bio = bio_alloc(GFP_KERNEL, 1);
 	struct completion event;
 	int ok;
 
-	bio_init(&bio);
-	bio.bi_io_vec = &vec;
-	vec.bv_page = page;
-	vec.bv_offset = 0;
-	vec.bv_len =
-	bio.bi_size = size;
-	bio.bi_vcnt = 1;
-	bio.bi_idx = 0;
-	bio.bi_bdev = mdev->md_bdev;
-	bio.bi_sector = sector;
+	bio_get(bio);
+
+	bio->bi_bdev = mdev->md_bdev;
+	bio->bi_sector = sector;
+	bio_add_page(bio, page, size, 0);
 	init_completion(&event);
-	bio.bi_private = &event;
-	bio.bi_end_io = drbd_md_io_complete;
+	bio->bi_private = &event;
+	bio->bi_end_io = drbd_md_io_complete;
 
 #ifdef BIO_RW_SYNC
-	submit_bio(rw | (1 << BIO_RW_SYNC), &bio);
+	submit_bio(rw | (1 << BIO_RW_SYNC), bio);
 #else
-	submit_bio(rw, &bio);
+	submit_bio(rw, bio);
 	drbd_blk_run_queue(bdev_get_queue(mdev->md_bdev));
 #endif
 	wait_for_completion(&event);
 
-	ok = test_bit(BIO_UPTODATE, &bio.bi_flags);
-
+	ok = test_bit(BIO_UPTODATE, &bio->bi_flags);
+	bio_put(bio);
 	return ok;
 }
 #endif

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[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.

Patch looks fine, I'm sad to say that if drdb was just in the kernel
source this would have been found in time...

> 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 :-(

Sure, will do so this morning.

-- 
Jens Axboe

[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

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

[Lars Marowsky-Bree]

On 2005-01-24T10:10:07, Jens Axboe <axboe@suse.de> wrote:

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

Ok, I'll go through the code and clean this up now. Thanks!

I'll hassle you with more questions as my changes oops ;-)

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

Yeah, that's happening with 0.8, but this is a 'maintenance' branch,
with all what that entails :-( drbd 0.7 on 2.4 is (unfortunately ;-)
still making money for Linbit, or else I'd guess everyone would be just
too happy to rip it out.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

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

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

[Lars Marowsky-Bree]

On 2005-01-24T10:10:07, Jens Axboe <axboe@suse.de> wrote:

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

I may be missing something here, but the interaction between
drbd_ee_init() and drbd_ee_bio_prepare() doesn't map neatly to the
bio_init() + bio_add_page() API. Part of the setup happens in
_ee_init(), but the other half in _prepare(), so that neither one has
all information readily available to use the correct bio_*() API.

Philipp, is it guaranteed that the drbd_dev for which drbd_ee_init()
is invoked is the same one drbd_ee_bio_prepare will be called later?


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

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

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

[Jens Axboe]

On Mon, Jan 24 2005, Lars Marowsky-Bree wrote:
> On 2005-01-24T10:10:07, Jens Axboe <axboe@suse.de> wrote:
> 
> > 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.
> 
> I may be missing something here, but the interaction between
> drbd_ee_init() and drbd_ee_bio_prepare() doesn't map neatly to the
> bio_init() + bio_add_page() API. Part of the setup happens in
> _ee_init(), but the other half in _prepare(), so that neither one has
> all information readily available to use the correct bio_*() API.

Yes, that is what would be nice to fix :-)

Make drbd_ee_init() just init the bastard, and add the page in
prepare().

-- 
Jens Axboe

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

[Lars Marowsky-Bree]

On 2005-01-24T11:23:10, Jens Axboe <axboe@suse.de> wrote:

> Yes, that is what would be nice to fix :-)
> 
> Make drbd_ee_init() just init the bastard, and add the page in
> prepare().

In theory you're right, but drbd seems to use this as a cache and
allocate the page just once, but fills it with different data of
different sizes over time, or at least that's what it looks like.

I'm not sure that's easily fixed.

I wish lge wasn't hiding in New Zealand ;-)


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

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

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

[Lars Marowsky-Bree]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

On 2005-01-24T11:28:52, Lars Marowsky-Bree <lmb@suse.de> wrote:

> > Make drbd_ee_init() just init the bastard, and add the page in
> > prepare().
> In theory you're right, but drbd seems to use this as a cache and
> allocate the page just once, but fills it with different data of
> different sizes over time, or at least that's what it looks like.
> 
> I'm not sure that's easily fixed.

I couldn't easily and cleanly initialize the io vec in the bio w/o
calling bio_alloc(), as bvec_alloc() can't cleanly be called outside of
fs/bio.c.

So I converted that to use bio_alloc() and bio_put() etc correctly, but
this wonderful beautiful code in drbd_worker.c assumes that every bio it
gets is embedded in a epoch_entry struct and uses the container_of()
macro to convert from the bio to the enclosing struct.

Which of course no longer works if the private_bio is indeed a pointer.
Wonderful.

I've attached the partial patch (which because of that doesn't compile
yet, but shows the general direction of the changes. I've _tried_ not to
mess up the 2.4 compatibility code, but I'm not sure whether I've
succeeded.

I guess another (smaller) option would be to add a bio_init_on_stack()
accompanied by a fitting destructor to fs/bio.c, which would initialize
the bio_io_vec correctly and deallocate it later, but this would change
core kernel code and I wanted to fix this completely in drbd if
possible.

Any comments?


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

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


[-- Attachment #2: drbd-bio --]
[-- Type: text/plain, Size: 9156 bytes --]

Index: drbd_receiver.c
===================================================================
--- drbd_receiver.c	(revision 1730)
+++ drbd_receiver.c	(working copy)
@@ -163,7 +163,7 @@
 	e = kmem_cache_alloc(drbd_ee_cache, mask);
 	if( e == NULL ) return FALSE;
 
-	drbd_ee_init(e,page);
+	drbd_ee_init(e,page,mask);
 	spin_lock_irq(&mdev->ee_lock);
 	list_add(&e->w.list,&mdev->free_ee);
 	mdev->ee_vacant++;
@@ -202,13 +202,16 @@
 	list_del(le);
 
 	page = drbd_bio_get_page(&e->private_bio);
+	
 ONLY_IN_26(
-	D_ASSERT(page == e->ee_bvec.bv_page);
-	page = e->ee_bvec.bv_page;
+	D_ASSERT(page == e->page);
+	page = e->page;
 )
 	kmem_cache_free(drbd_ee_cache, e);
 	mdev->ee_vacant--;
 
+	bio_put(e->private_bio);
+
 	return page;
 }
 
@@ -334,7 +337,7 @@
 	e=list_entry(le, struct Tl_epoch_entry, w.list);
 ONLY_IN_26(
 	D_ASSERT(e->private_bio.bi_idx == 0);
-	drbd_ee_init(e,e->ee_bvec.bv_page); // reinitialize
+	drbd_ee_init(e,e->page, GFP_KERNEL); // reinitialize
 )
 	e->block_id = !ID_VACANT;
 	SET_MAGIC(e);
@@ -824,7 +827,7 @@
 read_in_block(drbd_dev *mdev, int data_size)
 {
 	struct Tl_epoch_entry *e;
-	drbd_bio_t *bio;
+	struct bio *bio;
 	int rr;
 
 	spin_lock_irq(&mdev->ee_lock);
@@ -869,7 +872,7 @@
 STATIC int recv_dless_read(drbd_dev *mdev, drbd_request_t *req,
 			   sector_t sector, int data_size)
 {
-	drbd_bio_t *bio;
+	struct bio *bio;
 	int ok,rr;
 
 	bio = req->master_bio;
@@ -1603,7 +1606,7 @@
 STATIC void drbd_fail_pending_reads(drbd_dev *mdev)
 {
 	struct list_head *le;
-	drbd_bio_t *bio;
+	struct bio *bio;
 	LIST_HEAD(workset);
 
 	/*
Index: drbd_actlog.c
===================================================================
--- drbd_actlog.c	(revision 1730)
+++ drbd_actlog.c	(working copy)
@@ -64,35 +64,29 @@
 STATIC int _drbd_md_sync_page_io(drbd_dev *mdev, struct page *page, 
 				 sector_t sector, int rw, int size)
 {
-	struct bio bio;
-	struct bio_vec vec;
+	struct bio *bio = bio_alloc(GFP_KERNEL, 1);
 	struct completion event;
 	int ok;
 
-	bio_init(&bio);
-	bio.bi_io_vec = &vec;
-	vec.bv_page = page;
-	vec.bv_offset = 0;
-	vec.bv_len =
-	bio.bi_size = size;
-	bio.bi_vcnt = 1;
-	bio.bi_idx = 0;
-	bio.bi_bdev = mdev->md_bdev;
-	bio.bi_sector = sector;
+	bio_get(bio);
+
+	bio->bi_bdev = mdev->md_bdev;
+	bio->bi_sector = sector;
+	bio_add_page(bio, page, size, 0);
 	init_completion(&event);
-	bio.bi_private = &event;
-	bio.bi_end_io = drbd_md_io_complete;
+	bio->bi_private = &event;
+	bio->bi_end_io = drbd_md_io_complete;
 
 #ifdef BIO_RW_SYNC
-	submit_bio(rw | (1 << BIO_RW_SYNC), &bio);
+	submit_bio(rw | (1 << BIO_RW_SYNC), bio);
 #else
-	submit_bio(rw, &bio);
+	submit_bio(rw, bio);
 	drbd_blk_run_queue(bdev_get_queue(mdev->md_bdev));
 #endif
 	wait_for_completion(&event);
 
-	ok = test_bit(BIO_UPTODATE, &bio.bi_flags);
-
+	ok = test_bit(BIO_UPTODATE, &bio->bi_flags);
+	bio_put(bio);
 	return ok;
 }
 #endif
Index: drbd_compat_wrappers.h
===================================================================
--- drbd_compat_wrappers.h	(revision 1730)
+++ drbd_compat_wrappers.h	(working copy)
@@ -113,7 +113,7 @@
 	bh_kunmap(bh);
 }
 
-static inline void drbd_ee_init(struct Tl_epoch_entry *e,struct page *page)
+static inline void drbd_ee_init(struct Tl_epoch_entry *e,struct page *page,int mask)
 {
 	struct buffer_head * const bh = &e->private_bio;
 	memset(e, 0, sizeof(*e));
@@ -387,7 +387,7 @@
 
 static inline drbd_dev* drbd_req_get_mdev(struct drbd_request *req)
 {
-	return (drbd_dev*) req->private_bio.bi_private;
+	return (drbd_dev*) req->private_bio->bi_private;
 }
 
 static inline sector_t drbd_req_get_sector(struct drbd_request *req)
@@ -397,7 +397,7 @@
 
 static inline unsigned short drbd_req_get_size(struct drbd_request *req)
 {
-	drbd_dev* mdev = req->private_bio.bi_private;
+	drbd_dev* mdev = req->private_bio->bi_private;
 	D_ASSERT(req->master_bio->bi_size);
 	return req->master_bio->bi_size;
 }
@@ -429,11 +429,9 @@
  */
 static inline char *drbd_bio_kmap(struct bio *bio)
 {
-	struct bio_vec *bvec;
+	struct bio_vec *bvec = bio_iovec(bio);
 	unsigned long addr;
 
-	bvec = bio_iovec_idx(bio, bio->bi_idx);
-
 	addr = (unsigned long) kmap(bvec->bv_page);
 
 	if (addr & ~PAGE_MASK)
@@ -444,16 +442,15 @@
 
 static inline void drbd_bio_kunmap(struct bio *bio)
 {
-	struct bio_vec *bvec;
+	struct bio_vec *bvec = bio_iovec(bio);
 
-	bvec = bio_iovec_idx(bio, bio->bi_idx);
 	kunmap(bvec->bv_page);
 }
 
 #else
 static inline char *drbd_bio_kmap(struct bio *bio)
 {
-	struct bio_vec *bvec = bio_iovec_idx(bio, bio->bi_idx);
+	struct bio_vec *bvec = bio_iovec(bio);
 	return page_address(bvec->bv_page) + bvec->bv_offset;
 }
 static inline void drbd_bio_kunmap(struct bio *bio)
@@ -462,21 +459,10 @@
 }
 #endif
 
-static inline void drbd_ee_init(struct Tl_epoch_entry *e,struct page *page)
+static inline void drbd_ee_init(struct Tl_epoch_entry *e,struct page *page,int mask)
 {
-	struct bio * const bio = &e->private_bio;
-	struct bio_vec * const vec = &e->ee_bvec;
-	memset(e, 0, sizeof(*e));
-
-	// bio_init(&bio); memset did it for us.
-	bio->bi_io_vec = vec;
-	vec->bv_page   = page;
-	vec->bv_len    =
-	bio->bi_size   = PAGE_SIZE;
-	bio->bi_max_vecs = 1;
-	bio->bi_destructor = NULL;
-	atomic_set(&bio->bi_cnt, 1);
-
+	e->private_bio = bio_alloc(mask, 1);
+	e->page = page;
 	e->block_id = ID_VACANT;
 }
 
@@ -494,19 +480,15 @@
 drbd_ee_bio_prepare(drbd_dev *mdev, struct Tl_epoch_entry* e,
 		    sector_t sector, int size)
 {
-	struct bio * const bio = &e->private_bio;
+	struct bio *bio = e->private_bio;
 
 	D_ASSERT(mdev->backing_bdev);
 
-	bio->bi_flags  = 1 << BIO_UPTODATE;
-	bio->bi_io_vec->bv_len =
-	bio->bi_size    = size;
 	bio->bi_bdev    = mdev->backing_bdev;
 	bio->bi_sector  = sector;
 	bio->bi_private = mdev;
-	bio->bi_next    = 0;
-	bio->bi_idx     = 0; // for blk_recount_segments
-	bio->bi_vcnt    = 1; // for blk_recount_segments
+	bio_add_page(bio, e->page, size, 0);
+
 	e->ee_sector = sector;
 	e->ee_size = size;
 }
@@ -516,7 +498,7 @@
 		      sector_t sector, int size)
 {
 	drbd_ee_bio_prepare(mdev,e,sector,size);
-	e->private_bio.bi_end_io = drbd_dio_end_sec;
+	e->private_bio->bi_end_io = drbd_dio_end_sec;
 }
 
 static inline void
@@ -524,21 +506,21 @@
 		     sector_t sector, int size)
 {
 	drbd_ee_bio_prepare(mdev,e,sector,size);
-	e->private_bio.bi_end_io = enslaved_read_bi_end_io;
+	e->private_bio->bi_end_io = enslaved_read_bi_end_io;
 }
 
 static inline void
 drbd_req_prepare_write(drbd_dev *mdev, struct drbd_request *req)
 {
-	struct bio * const bio     = &req->private_bio;
+	struct bio * const bio     = req->private_bio;
 	struct bio * const bio_src =  req->master_bio;
 
-	bio_init(bio); // bio->bi_flags   = 0;
+	/* bio_init(bio); // bio->bi_flags   = 0; */
 	__bio_clone(bio,bio_src);
 	bio->bi_bdev    = mdev->backing_bdev;
 	bio->bi_private = mdev;
 	bio->bi_end_io  = drbd_dio_end;
-	bio->bi_next    = 0;
+	bio->bi_next    = NULL;
 
 	req->rq_status = RQ_DRBD_NOTHING;
 }
@@ -546,22 +528,22 @@
 static inline void
 drbd_req_prepare_read(drbd_dev *mdev, struct drbd_request *req)
 {
-	struct bio * const bio     = &req->private_bio;
+	struct bio * const bio     =  req->private_bio;
 	struct bio * const bio_src =  req->master_bio;
 
-	bio_init(bio); // bio->bi_flags   = 0;
+	/* bio_init(bio); // bio->bi_flags   = 0; */
 	__bio_clone(bio,bio_src);
 	bio->bi_bdev    = mdev->backing_bdev;
 	bio->bi_private = mdev;
 	bio->bi_end_io  = drbd_read_bi_end_io;	// <- only difference
-	bio->bi_next    = 0;
+	bio->bi_next    = NULL;
 
 	req->rq_status = RQ_DRBD_NOTHING;
 }
 
 static inline struct page* drbd_bio_get_page(struct bio *bio)
 {
-	struct bio_vec *bvec = bio_iovec_idx(bio, bio->bi_idx);
+	struct bio_vec *bvec = bio_iovec(bio);
 	return bvec->bv_page;
 }
 
@@ -622,13 +604,13 @@
 
 static inline int _drbd_send_zc_bio(drbd_dev *mdev, struct bio *bio)
 {
-	struct bio_vec *bvec = bio_iovec_idx(bio, bio->bi_idx);
+	struct bio_vec *bvec = bio_iovec(bio);
 	return _drbd_send_page(mdev,bvec->bv_page,bvec->bv_offset,bvec->bv_len);
 }
 
 static inline int _drbd_send_bio(drbd_dev *mdev, struct bio *bio)
 {
-	struct bio_vec *bvec = bio_iovec_idx(bio, bio->bi_idx);
+	struct bio_vec *bvec = bio_iovec(bio);
 	struct page *page = bvec->bv_page;
 	size_t size = bvec->bv_len;
 	int offset = bvec->bv_offset;
Index: drbd_int.h
===================================================================
--- drbd_int.h	(revision 1730)
+++ drbd_int.h	(working copy)
@@ -629,7 +629,8 @@
 	int rq_status;
 	struct drbd_barrier *barrier; // The next barrier.
 	drbd_bio_t *master_bio;       // master bio pointer
-	drbd_bio_t private_bio;       // private bio struct
+	NOT_IN_26(drbd_bio_t private_bio;)
+	ONLY_IN_26(struct bio *private_bio;)
 };
 
 struct drbd_barrier {
@@ -664,13 +665,13 @@
  */
 struct Tl_epoch_entry {
 	struct drbd_work    w;
-	drbd_bio_t private_bio; // private bio struct, NOT a pointer
+	NOT_IN_26(drbd_bio_t private_bio;)
+	ONLY_IN_26(struct bio *private_bio;)
+	struct page *page;
 	u64    block_id;
 	long magic;
 	ONLY_IN_26(unsigned int ee_size;)
 	ONLY_IN_26(sector_t ee_sector;)
-	// THINK: maybe we rather want bio_alloc(GFP_*,1)
-	ONLY_IN_26(struct bio_vec ee_bvec;)
 };
 
 /* flag bits */

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

[Helmut Wollmersdorfer]

Jens Axboe wrote:

> Patch looks fine, I'm sad to say that if drdb was just in the kernel
> source this would have been found in time...

Is there any technical, organizational or sozial reason against an 
integration into kernel.org?

Helmut Wollmersdorfer

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

[Lars Marowsky-Bree]

On 2005-01-24T13:32:26, Helmut Wollmersdorfer <helmut@wollmersdorfer.at> wrote:

> Is there any technical, organizational or sozial reason against an 
> integration into kernel.org?

Technical + political. The 0.7 branch is in no shape for integration
right now, and won't be because of the 2.4 compatibility cruft.

drbd-latest in SVN (0.8) is being cleaned up with that goal, but that
doesn't help the current code base much in this respect.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

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

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

[Lars Marowsky-Bree]

On 2005-01-24T13:24:46, Lars Marowsky-Bree <lmb@suse.de> wrote:

> So I converted that to use bio_alloc() and bio_put() etc correctly, but
> this wonderful beautiful code in drbd_worker.c assumes that every bio it
> gets is embedded in a epoch_entry struct and uses the container_of()
> macro to convert from the bio to the enclosing struct.
> 
> Which of course no longer works if the private_bio is indeed a pointer.
> Wonderful.

We could point bi_private at the epoch entry and have the epoch entry
reference the mdev in the Tl_epoch_entry and make this work again.

Would that be ok?

Except of course of not being useful for drbd_req_prepare_write/read(),
where there is no Tl_epoch_entry available. Sigh.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

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

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

[Philipp Reisner]

Am Sonntag, 23. Januar 2005 17:16 schrieb Lars Marowsky-Bree:
> 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 :-(
>

Hi Lars,

Did you tested this patch ?

If yes -> I will commit it to SVN right now...

-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 :

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

[Philipp Reisner]

Am Montag, 24. Januar 2005 10:58 schrieb Lars Marowsky-Bree:
> On 2005-01-24T10:10:07, Jens Axboe <axboe@suse.de> wrote:
> > 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.
>
> I may be missing something here, but the interaction between
> drbd_ee_init() and drbd_ee_bio_prepare() doesn't map neatly to the
> bio_init() + bio_add_page() API. Part of the setup happens in
> _ee_init(), but the other half in _prepare(), so that neither one has
> all information readily available to use the correct bio_*() API.
>
> Philipp, is it guaranteed that the drbd_dev for which drbd_ee_init()
> is invoked is the same one drbd_ee_bio_prepare will be called later?
>

yes.

-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 :

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

[Philipp Reisner]

Am Montag, 24. Januar 2005 11:28 schrieb Lars Marowsky-Bree:
> On 2005-01-24T11:23:10, Jens Axboe <axboe@suse.de> wrote:
> > Yes, that is what would be nice to fix :-)
> >
> > Make drbd_ee_init() just init the bastard, and add the page in
> > prepare().
>
> In theory you're right, but drbd seems to use this as a cache and
> allocate the page just once, but fills it with different data of
> different sizes over time, or at least that's what it looks like.
>

This is true.

> I'm not sure that's easily fixed.
>

What ?

-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 :

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

[Lars Marowsky-Bree]

On 2005-01-24T15:27:03, Philipp Reisner <philipp.reisner@linbit.com> wrote:

> Hi Lars,
> 
> Did you tested this patch ?
> 
> If yes -> I will commit it to SVN right now...

The one part is correct, but the other cases of drbd using struct bio
need fixing too. If you look at the last diff (mostly converting
everything to struct bio * and the API which goes with it) you'll see
that mostly the container_of() macros are stumbling still, and that's
fairly deep within the drbd code.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

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

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

[Philipp Reisner]

Am Sonntag, 23. Januar 2005 17:16 schrieb Lars Marowsky-Bree:
> 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 :-(
>

Hi Guys,

I just see some hectic discussion going on here, it runs as it is on 
Linux-2.6.10 as from kernel.org . Could someone tell me what the 
actual problem is ?

-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 :

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

[Lars Marowsky-Bree]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

On 2005-01-24T15:38:33, Philipp Reisner <philipp.reisner@linbit.com> wrote:

> I just see some hectic discussion going on here, it runs as it is on 
> Linux-2.6.10 as from kernel.org . Could someone tell me what the 
> actual problem is ?

The problem is that the fix (attached) for a memory corruption in md
exposes some bugs (aka, not using struct bio as it's supposed to) in
drbd (as well as md).

This fix is merged in SLES9 SP1, and in the 2.6.10-ac series, and will
likely become part of 2.6.11 one day.

The proper way for fixing it would be to have drbd use bio_alloc() and
friends instead of messing with the bio directly, but that changes quite
a bit of the code...


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

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


[-- Attachment #2: bio-clone-duplicate-vec --]
[-- Type: text/plain, Size: 1982 bytes --]

From: Jens Axboe <axboe@suse.de>
Subject: bio clone must copy io_vec
Patch-mainline: 
References: 47777

The way md uses bio_clones, it's possible for the original bio to be freed
before the clone is freed. This means the clone ->bi_io_vec points to freed
memory potentially. Fix it by duplicating the io_vec as well.

Acked-by: 
Signed-off-by: Jens Axboe <axboe@suse.de>

--- linux-2.6.5/fs/bio.c~	2004-11-24 12:42:10.532343678 +0100
+++ linux-2.6.5/fs/bio.c	2004-11-24 12:46:49.308021403 +0100
@@ -98,12 +98,7 @@
 
 	BIO_BUG_ON(pool_idx >= BIOVEC_NR_POOLS);
 
-	/*
-	 * cloned bio doesn't own the veclist
-	 */
-	if (!bio_flagged(bio, BIO_CLONED))
-		mempool_free(bio->bi_io_vec, bp->pool);
-
+	mempool_free(bio->bi_io_vec, bp->pool);
 	mempool_free(bio, bio_pool);
 }
 
@@ -212,7 +207,9 @@
  */
 inline void __bio_clone(struct bio *bio, struct bio *bio_src)
 {
-	bio->bi_io_vec = bio_src->bi_io_vec;
+	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));
 
 	bio->bi_sector = bio_src->bi_sector;
 	bio->bi_bdev = bio_src->bi_bdev;
@@ -224,21 +221,9 @@
 	 * for the clone
 	 */
 	bio->bi_vcnt = bio_src->bi_vcnt;
-	bio->bi_idx = bio_src->bi_idx;
-	if (bio_flagged(bio, BIO_SEG_VALID)) {
-		bio->bi_phys_segments = bio_src->bi_phys_segments;
-		bio->bi_hw_segments = bio_src->bi_hw_segments;
-		bio->bi_flags |= (1 << BIO_SEG_VALID);
-	}
 	bio->bi_size = bio_src->bi_size;
-
-	/*
-	 * cloned bio does not own the bio_vec, so users cannot fiddle with
-	 * it. clear bi_max_vecs and clear the BIO_POOL_BITS to make this
-	 * apparent
-	 */
-	bio->bi_max_vecs = 0;
-	bio->bi_flags &= (BIO_POOL_MASK - 1);
+	bio_phys_segments(q, bio);
+	bio_hw_segments(q, bio);
 }
 
 /**
@@ -250,7 +235,7 @@
  */
 struct bio *bio_clone(struct bio *bio, int gfp_mask)
 {
-	struct bio *b = bio_alloc(gfp_mask, 0);
+	struct bio *b = bio_alloc(gfp_mask, bio->bi_max_vecs);
 
 	if (b)
 		__bio_clone(b, bio);

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

[Lars Marowsky-Bree]

On 2005-01-24T15:41:57, Lars Marowsky-Bree <lmb@suse.de> wrote:

> The proper way for fixing it would be to have drbd use bio_alloc() and
> friends instead of messing with the bio directly, but that changes quite
> a bit of the code...

I've decided to go for the cheaper (in terms of "lines of code changed")
now for the time being. I can however confirm LKCD is working very
reliably in SLES9 SP1 ;-)


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

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

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

[Lars Marowsky-Bree]

[-- Attachment #1.1: Type: text/plain, Size: 1192 bytes --]

On 2005-01-23T17:16:33, Lars Marowsky-Bree <lmb@suse.de> wrote:

The attached patch fixes drbds useage of bios up some. The proper fix
would be to indeed change it over to use bio_alloc(), bio_get/put(),
bio_add_page(), bio_clone (instead of __bio_clone) et cetera, but that
fix is too complex for the timeframe I have right now.

This should keep drbd-0.7.8 from oopsing not only on the SLES9 SP1
kernel but also the recent 2.6.10-ac series.

(I could possibly _code_ it, but it'd be too invasive and I'm weary of
the side-effects it might have and the QA would take too long. There's a
number of potential cleanups like further consolidation between
drbd_prepare_req_write/_read and others, but I'd propose to do that for
the drbd-0.8 branch instead when we can do away with 2.4.)

Please comment on the patch, I'd be grateful.

I have tested the patch in a standalone configuration, primary/secondary
(and switching around during resyncing etc), and it seems to be doing
quite well so far.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

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


[-- Attachment #1.2: drbd-50103 --]
[-- Type: text/plain, Size: 6890 bytes --]

Index: drbd/drbd_actlog.c
===================================================================
--- drbd/drbd_actlog.c	(revision 1730)
+++ drbd/drbd_actlog.c	(working copy)
@@ -64,35 +64,29 @@
 STATIC int _drbd_md_sync_page_io(drbd_dev *mdev, struct page *page, 
 				 sector_t sector, int rw, int size)
 {
-	struct bio bio;
-	struct bio_vec vec;
+	struct bio *bio = bio_alloc(GFP_KERNEL, 1);
 	struct completion event;
 	int ok;
 
-	bio_init(&bio);
-	bio.bi_io_vec = &vec;
-	vec.bv_page = page;
-	vec.bv_offset = 0;
-	vec.bv_len =
-	bio.bi_size = size;
-	bio.bi_vcnt = 1;
-	bio.bi_idx = 0;
-	bio.bi_bdev = mdev->md_bdev;
-	bio.bi_sector = sector;
+	bio_get(bio);
+
+	bio->bi_bdev = mdev->md_bdev;
+	bio->bi_sector = sector;
+	bio_add_page(bio, page, size, 0);
 	init_completion(&event);
-	bio.bi_private = &event;
-	bio.bi_end_io = drbd_md_io_complete;
+	bio->bi_private = &event;
+	bio->bi_end_io = drbd_md_io_complete;
 
 #ifdef BIO_RW_SYNC
-	submit_bio(rw | (1 << BIO_RW_SYNC), &bio);
+	submit_bio(rw | (1 << BIO_RW_SYNC), bio);
 #else
-	submit_bio(rw, &bio);
+	submit_bio(rw, bio);
 	drbd_blk_run_queue(bdev_get_queue(mdev->md_bdev));
 #endif
 	wait_for_completion(&event);
 
-	ok = test_bit(BIO_UPTODATE, &bio.bi_flags);
-
+	ok = test_bit(BIO_UPTODATE, &bio->bi_flags);
+	bio_put(bio);
 	return ok;
 }
 #endif
Index: drbd/drbd_compat_wrappers.h
===================================================================
--- drbd/drbd_compat_wrappers.h	(revision 1730)
+++ drbd/drbd_compat_wrappers.h	(working copy)
@@ -429,11 +429,9 @@
  */
 static inline char *drbd_bio_kmap(struct bio *bio)
 {
-	struct bio_vec *bvec;
+	struct bio_vec *bvec = bio_iovec(bio);
 	unsigned long addr;
 
-	bvec = bio_iovec_idx(bio, bio->bi_idx);
-
 	addr = (unsigned long) kmap(bvec->bv_page);
 
 	if (addr & ~PAGE_MASK)
@@ -444,16 +442,15 @@
 
 static inline void drbd_bio_kunmap(struct bio *bio)
 {
-	struct bio_vec *bvec;
+	struct bio_vec *bvec = bio_iovec(bio);
 
-	bvec = bio_iovec_idx(bio, bio->bi_idx);
 	kunmap(bvec->bv_page);
 }
 
 #else
 static inline char *drbd_bio_kmap(struct bio *bio)
 {
-	struct bio_vec *bvec = bio_iovec_idx(bio, bio->bi_idx);
+	struct bio_vec *bvec = bio_iovec(bio);
 	return page_address(bvec->bv_page) + bvec->bv_offset;
 }
 static inline void drbd_bio_kunmap(struct bio *bio)
@@ -466,16 +463,17 @@
 {
 	struct bio * const bio = &e->private_bio;
 	struct bio_vec * const vec = &e->ee_bvec;
+
 	memset(e, 0, sizeof(*e));
+	bio_init(bio);
 
-	// bio_init(&bio); memset did it for us.
 	bio->bi_io_vec = vec;
-	vec->bv_page   = page;
-	vec->bv_len    =
-	bio->bi_size   = PAGE_SIZE;
-	bio->bi_max_vecs = 1;
 	bio->bi_destructor = NULL;
-	atomic_set(&bio->bi_cnt, 1);
+	vec->bv_page = page;
+	bio->bi_size = vec->bv_len = PAGE_SIZE;
+	bio->bi_max_vecs = bio->bi_vcnt = 
+	bio->bi_phys_segments = bio->bi_hw_segments = 1;
+	vec->bv_offset = 0;
 
 	e->block_id = ID_VACANT;
 }
@@ -495,20 +493,25 @@
 		    sector_t sector, int size)
 {
 	struct bio * const bio = &e->private_bio;
-
+	struct bio_vec * const vec = &e->ee_bvec;
+	struct page * const page = vec->bv_page;
 	D_ASSERT(mdev->backing_bdev);
 
-	bio->bi_flags  = 1 << BIO_UPTODATE;
-	bio->bi_io_vec->bv_len =
-	bio->bi_size    = size;
-	bio->bi_bdev    = mdev->backing_bdev;
-	bio->bi_sector  = sector;
+	/* Clear plate. */
+	bio_init(bio);
+
+	bio->bi_io_vec = vec;
+	bio->bi_destructor = NULL;
+	vec->bv_page = page;
+	vec->bv_offset = 0;
+	bio->bi_max_vecs = bio->bi_vcnt = 
+	bio->bi_phys_segments = bio->bi_hw_segments = 1;
+
+	bio->bi_bdev = mdev->backing_bdev;
 	bio->bi_private = mdev;
-	bio->bi_next    = 0;
-	bio->bi_idx     = 0; // for blk_recount_segments
-	bio->bi_vcnt    = 1; // for blk_recount_segments
-	e->ee_sector = sector;
-	e->ee_size = size;
+
+	e->ee_sector = bio->bi_sector = sector;
+	e->ee_size = bio->bi_size = bio->bi_io_vec->bv_len = size;
 }
 
 static inline void
@@ -530,15 +533,19 @@
 static inline void
 drbd_req_prepare_write(drbd_dev *mdev, struct drbd_request *req)
 {
-	struct bio * const bio     = &req->private_bio;
-	struct bio * const bio_src =  req->master_bio;
+	struct bio * const bio      = &req->private_bio;
+	struct bio_vec * const bvec = &req->req_bvec;
+	struct bio * const bio_src  =  req->master_bio;
 
 	bio_init(bio); // bio->bi_flags   = 0;
+	bio->bi_io_vec = bvec;
+	bio->bi_max_vecs = 1;
+
 	__bio_clone(bio,bio_src);
 	bio->bi_bdev    = mdev->backing_bdev;
 	bio->bi_private = mdev;
 	bio->bi_end_io  = drbd_dio_end;
-	bio->bi_next    = 0;
+	bio->bi_next    = NULL;
 
 	req->rq_status = RQ_DRBD_NOTHING;
 }
@@ -546,22 +553,26 @@
 static inline void
 drbd_req_prepare_read(drbd_dev *mdev, struct drbd_request *req)
 {
-	struct bio * const bio     = &req->private_bio;
-	struct bio * const bio_src =  req->master_bio;
+	struct bio * const bio      = &req->private_bio;
+	struct bio_vec * const bvec = &req->req_bvec;
+	struct bio * const bio_src  =  req->master_bio;
 
 	bio_init(bio); // bio->bi_flags   = 0;
+	bio->bi_io_vec = bvec;
+	bio->bi_max_vecs = 1;
+
 	__bio_clone(bio,bio_src);
 	bio->bi_bdev    = mdev->backing_bdev;
 	bio->bi_private = mdev;
 	bio->bi_end_io  = drbd_read_bi_end_io;	// <- only difference
-	bio->bi_next    = 0;
+	bio->bi_next    = NULL;
 
 	req->rq_status = RQ_DRBD_NOTHING;
 }
 
 static inline struct page* drbd_bio_get_page(struct bio *bio)
 {
-	struct bio_vec *bvec = bio_iovec_idx(bio, bio->bi_idx);
+	struct bio_vec *bvec = bio_iovec(bio);
 	return bvec->bv_page;
 }
 
@@ -622,13 +633,13 @@
 
 static inline int _drbd_send_zc_bio(drbd_dev *mdev, struct bio *bio)
 {
-	struct bio_vec *bvec = bio_iovec_idx(bio, bio->bi_idx);
+	struct bio_vec *bvec = bio_iovec(bio);
 	return _drbd_send_page(mdev,bvec->bv_page,bvec->bv_offset,bvec->bv_len);
 }
 
 static inline int _drbd_send_bio(drbd_dev *mdev, struct bio *bio)
 {
-	struct bio_vec *bvec = bio_iovec_idx(bio, bio->bi_idx);
+	struct bio_vec *bvec = bio_iovec(bio);
 	struct page *page = bvec->bv_page;
 	size_t size = bvec->bv_len;
 	int offset = bvec->bv_offset;
Index: drbd/drbd_int.h
===================================================================
--- drbd/drbd_int.h	(revision 1730)
+++ drbd/drbd_int.h	(working copy)
@@ -630,6 +630,7 @@
 	struct drbd_barrier *barrier; // The next barrier.
 	drbd_bio_t *master_bio;       // master bio pointer
 	drbd_bio_t private_bio;       // private bio struct
+	ONLY_IN_26(struct bio_vec req_bvec;)
 };
 
 struct drbd_barrier {
@@ -669,7 +670,7 @@
 	long magic;
 	ONLY_IN_26(unsigned int ee_size;)
 	ONLY_IN_26(sector_t ee_sector;)
-	// THINK: maybe we rather want bio_alloc(GFP_*,1)
+	// TODO: we rather want bio_alloc(GFP_*,1) all through the code!
 	ONLY_IN_26(struct bio_vec ee_bvec;)
 };
 

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Philipp Reisner]

Am Dienstag, 25. Januar 2005 00:23 schrieb Lars Marowsky-Bree:
> On 2005-01-23T17:16:33, Lars Marowsky-Bree <lmb@suse.de> wrote:
>
> The attached patch fixes drbds useage of bios up some. The proper fix
> would be to indeed change it over to use bio_alloc(), bio_get/put(),
> bio_add_page(), bio_clone (instead of __bio_clone) et cetera, but that
> fix is too complex for the timeframe I have right now.
>
> This should keep drbd-0.7.8 from oopsing not only on the SLES9 SP1
> kernel but also the recent 2.6.10-ac series.
>
> (I could possibly _code_ it, but it'd be too invasive and I'm weary of
> the side-effects it might have and the QA would take too long. There's a
> number of potential cleanups like further consolidation between
> drbd_prepare_req_write/_read and others, but I'd propose to do that for
> the drbd-0.8 branch instead when we can do away with 2.4.)
>
> Please comment on the patch, I'd be grateful.
>

Hi Lars,

the patch looks good so far. I am really happy that you have choosen
to go the less intrusive way for drbd-07.

Changing it over to alloc_bio() is something for drbd-08.

[...]
  ONLY_IN_26(unsigned int ee_size;)
  ONLY_IN_26(sector_t ee_sector;)
- // THINK: maybe we rather want bio_alloc(GFP_*,1)
+ // TODO: we rather want bio_alloc(GFP_*,1) all through the code!
  ONLY_IN_26(struct bio_vec ee_bvec;)

I am wondering if with a private copy of the bio_vec if we already
have a copy of the IO operations size and start sector that is 
still in place after the IO operation completed.

 -> If this is the case we could drop the ee_size and ee_sector members
    and take them form e.g. ee_bvec 

Jens, I guess you can answer that question  easily.

I will commit it to SVN...

-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 :

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

[Jens Axboe]

On Tue, Jan 25 2005, Philipp Reisner wrote:
> Am Dienstag, 25. Januar 2005 00:23 schrieb Lars Marowsky-Bree:
> > On 2005-01-23T17:16:33, Lars Marowsky-Bree <lmb@suse.de> wrote:
> >
> > The attached patch fixes drbds useage of bios up some. The proper fix
> > would be to indeed change it over to use bio_alloc(), bio_get/put(),
> > bio_add_page(), bio_clone (instead of __bio_clone) et cetera, but that
> > fix is too complex for the timeframe I have right now.
> >
> > This should keep drbd-0.7.8 from oopsing not only on the SLES9 SP1
> > kernel but also the recent 2.6.10-ac series.
> >
> > (I could possibly _code_ it, but it'd be too invasive and I'm weary of
> > the side-effects it might have and the QA would take too long. There's a
> > number of potential cleanups like further consolidation between
> > drbd_prepare_req_write/_read and others, but I'd propose to do that for
> > the drbd-0.8 branch instead when we can do away with 2.4.)
> >
> > Please comment on the patch, I'd be grateful.
> >
> 
> Hi Lars,
> 
> the patch looks good so far. I am really happy that you have choosen
> to go the less intrusive way for drbd-07.
> 
> Changing it over to alloc_bio() is something for drbd-08.
> 
> [...]
>   ONLY_IN_26(unsigned int ee_size;)
>   ONLY_IN_26(sector_t ee_sector;)
> - // THINK: maybe we rather want bio_alloc(GFP_*,1)
> + // TODO: we rather want bio_alloc(GFP_*,1) all through the code!
>   ONLY_IN_26(struct bio_vec ee_bvec;)
> 
> I am wondering if with a private copy of the bio_vec if we already
> have a copy of the IO operations size and start sector that is 
> still in place after the IO operation completed.
> 
>  -> If this is the case we could drop the ee_size and ee_sector members
>     and take them form e.g. ee_bvec 
> 
> Jens, I guess you can answer that question  easily.

IO completion can change the vector offset and lengths for some cases of
partial io, notable where a device completes less than a complete
vector. So I'd say stay safe and retain your copy of those values.

-- 
Jens Axboe

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

[Lars Marowsky-Bree]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

On 2005-01-25T10:26:11, Philipp Reisner <philipp.reisner@linbit.com> wrote:

Jens suggested to not set the segment fields manually, but that's the
only small change left and now the patch should be ready for inclusion.

Is there anything else pending in 0.7.8 / 0.7-SVN still? I'm submitting
fixed packages to SLES9 today and would like to pick up any other
important fixes...


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

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


[-- Attachment #2: drbd-50103 --]
[-- Type: text/plain, Size: 6507 bytes --]

Index: drbd_actlog.c
===================================================================
--- drbd_actlog.c	(revision 1730)
+++ drbd_actlog.c	(working copy)
@@ -64,35 +64,29 @@
 STATIC int _drbd_md_sync_page_io(drbd_dev *mdev, struct page *page, 
 				 sector_t sector, int rw, int size)
 {
-	struct bio bio;
-	struct bio_vec vec;
+	struct bio *bio = bio_alloc(GFP_KERNEL, 1);
 	struct completion event;
 	int ok;
 
-	bio_init(&bio);
-	bio.bi_io_vec = &vec;
-	vec.bv_page = page;
-	vec.bv_offset = 0;
-	vec.bv_len =
-	bio.bi_size = size;
-	bio.bi_vcnt = 1;
-	bio.bi_idx = 0;
-	bio.bi_bdev = mdev->md_bdev;
-	bio.bi_sector = sector;
+	bio_get(bio);
+
+	bio->bi_bdev = mdev->md_bdev;
+	bio->bi_sector = sector;
+	bio_add_page(bio, page, size, 0);
 	init_completion(&event);
-	bio.bi_private = &event;
-	bio.bi_end_io = drbd_md_io_complete;
+	bio->bi_private = &event;
+	bio->bi_end_io = drbd_md_io_complete;
 
 #ifdef BIO_RW_SYNC
-	submit_bio(rw | (1 << BIO_RW_SYNC), &bio);
+	submit_bio(rw | (1 << BIO_RW_SYNC), bio);
 #else
-	submit_bio(rw, &bio);
+	submit_bio(rw, bio);
 	drbd_blk_run_queue(bdev_get_queue(mdev->md_bdev));
 #endif
 	wait_for_completion(&event);
 
-	ok = test_bit(BIO_UPTODATE, &bio.bi_flags);
-
+	ok = test_bit(BIO_UPTODATE, &bio->bi_flags);
+	bio_put(bio);
 	return ok;
 }
 #endif
Index: drbd_compat_wrappers.h
===================================================================
--- drbd_compat_wrappers.h	(revision 1730)
+++ drbd_compat_wrappers.h	(working copy)
@@ -429,11 +429,9 @@
  */
 static inline char *drbd_bio_kmap(struct bio *bio)
 {
-	struct bio_vec *bvec;
+	struct bio_vec *bvec = bio_iovec(bio);
 	unsigned long addr;
 
-	bvec = bio_iovec_idx(bio, bio->bi_idx);
-
 	addr = (unsigned long) kmap(bvec->bv_page);
 
 	if (addr & ~PAGE_MASK)
@@ -444,16 +442,15 @@
 
 static inline void drbd_bio_kunmap(struct bio *bio)
 {
-	struct bio_vec *bvec;
+	struct bio_vec *bvec = bio_iovec(bio);
 
-	bvec = bio_iovec_idx(bio, bio->bi_idx);
 	kunmap(bvec->bv_page);
 }
 
 #else
 static inline char *drbd_bio_kmap(struct bio *bio)
 {
-	struct bio_vec *bvec = bio_iovec_idx(bio, bio->bi_idx);
+	struct bio_vec *bvec = bio_iovec(bio);
 	return page_address(bvec->bv_page) + bvec->bv_offset;
 }
 static inline void drbd_bio_kunmap(struct bio *bio)
@@ -466,16 +463,16 @@
 {
 	struct bio * const bio = &e->private_bio;
 	struct bio_vec * const vec = &e->ee_bvec;
+
 	memset(e, 0, sizeof(*e));
+	bio_init(bio);
 
-	// bio_init(&bio); memset did it for us.
 	bio->bi_io_vec = vec;
-	vec->bv_page   = page;
-	vec->bv_len    =
-	bio->bi_size   = PAGE_SIZE;
-	bio->bi_max_vecs = 1;
 	bio->bi_destructor = NULL;
-	atomic_set(&bio->bi_cnt, 1);
+	vec->bv_page = page;
+	bio->bi_size = vec->bv_len = PAGE_SIZE;
+	bio->bi_max_vecs = bio->bi_vcnt = 1;
+	vec->bv_offset = 0;
 
 	e->block_id = ID_VACANT;
 }
@@ -495,20 +492,24 @@
 		    sector_t sector, int size)
 {
 	struct bio * const bio = &e->private_bio;
-
+	struct bio_vec * const vec = &e->ee_bvec;
+	struct page * const page = vec->bv_page;
 	D_ASSERT(mdev->backing_bdev);
 
-	bio->bi_flags  = 1 << BIO_UPTODATE;
-	bio->bi_io_vec->bv_len =
-	bio->bi_size    = size;
-	bio->bi_bdev    = mdev->backing_bdev;
-	bio->bi_sector  = sector;
+	/* Clear plate. */
+	bio_init(bio);
+
+	bio->bi_io_vec = vec;
+	bio->bi_destructor = NULL;
+	vec->bv_page = page;
+	vec->bv_offset = 0;
+	bio->bi_max_vecs = bio->bi_vcnt = 1;
+
+	bio->bi_bdev = mdev->backing_bdev;
 	bio->bi_private = mdev;
-	bio->bi_next    = 0;
-	bio->bi_idx     = 0; // for blk_recount_segments
-	bio->bi_vcnt    = 1; // for blk_recount_segments
-	e->ee_sector = sector;
-	e->ee_size = size;
+
+	e->ee_sector = bio->bi_sector = sector;
+	e->ee_size = bio->bi_size = bio->bi_io_vec->bv_len = size;
 }
 
 static inline void
@@ -530,15 +531,19 @@
 static inline void
 drbd_req_prepare_write(drbd_dev *mdev, struct drbd_request *req)
 {
-	struct bio * const bio     = &req->private_bio;
-	struct bio * const bio_src =  req->master_bio;
+	struct bio * const bio      = &req->private_bio;
+	struct bio_vec * const bvec = &req->req_bvec;
+	struct bio * const bio_src  =  req->master_bio;
 
 	bio_init(bio); // bio->bi_flags   = 0;
+	bio->bi_io_vec = bvec;
+	bio->bi_max_vecs = 1;
+
 	__bio_clone(bio,bio_src);
 	bio->bi_bdev    = mdev->backing_bdev;
 	bio->bi_private = mdev;
 	bio->bi_end_io  = drbd_dio_end;
-	bio->bi_next    = 0;
+	bio->bi_next    = NULL;
 
 	req->rq_status = RQ_DRBD_NOTHING;
 }
@@ -546,22 +551,26 @@
 static inline void
 drbd_req_prepare_read(drbd_dev *mdev, struct drbd_request *req)
 {
-	struct bio * const bio     = &req->private_bio;
-	struct bio * const bio_src =  req->master_bio;
+	struct bio * const bio      = &req->private_bio;
+	struct bio_vec * const bvec = &req->req_bvec;
+	struct bio * const bio_src  =  req->master_bio;
 
 	bio_init(bio); // bio->bi_flags   = 0;
+	bio->bi_io_vec = bvec;
+	bio->bi_max_vecs = 1;
+
 	__bio_clone(bio,bio_src);
 	bio->bi_bdev    = mdev->backing_bdev;
 	bio->bi_private = mdev;
 	bio->bi_end_io  = drbd_read_bi_end_io;	// <- only difference
-	bio->bi_next    = 0;
+	bio->bi_next    = NULL;
 
 	req->rq_status = RQ_DRBD_NOTHING;
 }
 
 static inline struct page* drbd_bio_get_page(struct bio *bio)
 {
-	struct bio_vec *bvec = bio_iovec_idx(bio, bio->bi_idx);
+	struct bio_vec *bvec = bio_iovec(bio);
 	return bvec->bv_page;
 }
 
@@ -622,13 +631,13 @@
 
 static inline int _drbd_send_zc_bio(drbd_dev *mdev, struct bio *bio)
 {
-	struct bio_vec *bvec = bio_iovec_idx(bio, bio->bi_idx);
+	struct bio_vec *bvec = bio_iovec(bio);
 	return _drbd_send_page(mdev,bvec->bv_page,bvec->bv_offset,bvec->bv_len);
 }
 
 static inline int _drbd_send_bio(drbd_dev *mdev, struct bio *bio)
 {
-	struct bio_vec *bvec = bio_iovec_idx(bio, bio->bi_idx);
+	struct bio_vec *bvec = bio_iovec(bio);
 	struct page *page = bvec->bv_page;
 	size_t size = bvec->bv_len;
 	int offset = bvec->bv_offset;
Index: drbd_int.h
===================================================================
--- drbd_int.h	(revision 1730)
+++ drbd_int.h	(working copy)
@@ -630,6 +630,7 @@
 	struct drbd_barrier *barrier; // The next barrier.
 	drbd_bio_t *master_bio;       // master bio pointer
 	drbd_bio_t private_bio;       // private bio struct
+	ONLY_IN_26(struct bio_vec req_bvec;)
 };
 
 struct drbd_barrier {
@@ -669,7 +670,7 @@
 	long magic;
 	ONLY_IN_26(unsigned int ee_size;)
 	ONLY_IN_26(sector_t ee_sector;)
-	// THINK: maybe we rather want bio_alloc(GFP_*,1)
+	// TODO: we rather want bio_alloc(GFP_*,1) all through the code!
 	ONLY_IN_26(struct bio_vec ee_bvec;)
 };
 

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

[Philipp Reisner]

Hi Lars,

When are you taking the code for SuSE ?

I guess it would make sense to tag drbd-0.7.8 at that moment, so
that this time SuSE and the rest of the world have the same
"drbd-0.7.8" :)

-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 :

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

[Lars Marowsky-Bree]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

On 2005-01-25T10:58:13, Philipp Reisner <philipp.reisner@linbit.com> wrote:

> When are you taking the code for SuSE ?

Today. We're in a bit of a hurry because we screwed up and shipped the
oopsing drbd in SP1 ;-)

> I guess it would make sense to tag drbd-0.7.8 at that moment, so
> that this time SuSE and the rest of the world have the same
> "drbd-0.7.8" :)

Sure. Attached is the minimal cleanup Jens suggested relative to svn
1732.


Sincerely,
    Lars Marowsky-Brée <lmb@suse.de>

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


[-- Attachment #2: drbd-50103 --]
[-- Type: text/plain, Size: 776 bytes --]

Index: drbd/drbd_compat_wrappers.h
===================================================================
--- drbd/drbd_compat_wrappers.h	(revision 1732)
+++ drbd/drbd_compat_wrappers.h	(working copy)
@@ -471,8 +471,7 @@
 	bio->bi_destructor = NULL;
 	vec->bv_page = page;
 	bio->bi_size = vec->bv_len = PAGE_SIZE;
-	bio->bi_max_vecs = bio->bi_vcnt = 
-	bio->bi_phys_segments = bio->bi_hw_segments = 1;
+	bio->bi_max_vecs = bio->bi_vcnt = 1;
 	vec->bv_offset = 0;
 
 	e->block_id = ID_VACANT;
@@ -504,8 +503,7 @@
 	bio->bi_destructor = NULL;
 	vec->bv_page = page;
 	vec->bv_offset = 0;
-	bio->bi_max_vecs = bio->bi_vcnt = 
-	bio->bi_phys_segments = bio->bi_hw_segments = 1;
+	bio->bi_max_vecs = bio->bi_vcnt = 1;
 
 	bio->bi_bdev = mdev->backing_bdev;
 	bio->bi_private = mdev;

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

[Philipp Reisner]

Am Dienstag, 25. Januar 2005 11:05 schrieb Lars Marowsky-Bree:
> On 2005-01-25T10:58:13, Philipp Reisner <philipp.reisner@linbit.com> wrote:
> > When are you taking the code for SuSE ?
>
> Today. We're in a bit of a hurry because we screwed up and shipped the
> oopsing drbd in SP1 ;-)
>

Ok, I tagged it. It is r1735.
http://svn.drbd.org/drbd/tags/drbd-0.7.9/

Will do the tarball etc.. tomorrow morning.

-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 :

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

[Lars Ellenberg]

/ 2005-01-24 11:28:52 +0100
\ Lars Marowsky-Bree:
>>> [ ... drbd code cleanup is due ... ]
>> [ ... hear hear ... ]

> I wish lge wasn't hiding in New Zealand ;-)

well, sorry about that.
 :-)

still alive!

been offline for several weeks in really remote locations -- even lost
all comunication completely: managed to ship out our only cell phone
with someone who left early... needed to kayak what would have been
a two-days-worth hike (one way) back and forth to get it back...
nice one!

some "online" days now (had to dig through an impressive pile of list
mails and spam. doh.)

won't be able to do serious work on drbd and ha before mid april.

	lge