[Drbd-dev] DRBD-8 (My) first problem with new requests handling code...

[Drbd-dev] DRBD-8 (My) first problem with new requests handling code...

[Graham, Simon]

Got a pretty quick death running the latest trunk - I think it's this
code in the completed_ok case of _req_done:

		bio_put(req->private_bio);
		req->private_bio = NULL;	<<=== Whoops!
		dec_local(mdev);

		if (bio_data_dir(req->private_bio) == WRITE)  <<===
BANG!
			mdev->writ_cnt += req->size>>9;
		else
			mdev->read_cnt += req->size>>9;

Presumably the bio_put and clear of req->private_bio should be after
testing the direction ;-)

BTW: It took me quite a while to find this from the panic -- it actually
crashed in drbd_endio_pri and the code generated for this is humungous
and unbelievably complicated (it's about 12KB!) -- I wonder if you are
trying to inline too much and it mightn't be better to have req_mod be a
non-inline function and also have each case be a separate real
function... I'd even suggest having _req_may_be_done() as a separate
function.

Simon

Re: [Drbd-dev] DRBD-8 (My) first problem with new requests handling code...

[Lars Ellenberg]

/ 2006-09-10 18:58:32 -0400
\ Graham, Simon:
> Got a pretty quick death running the latest trunk - I think it's this
> code in the completed_ok case of _req_done:
> 
> 		bio_put(req->private_bio);
> 		req->private_bio = NULL;	<<=== Whoops!
> 		dec_local(mdev);
> 
> 		if (bio_data_dir(req->private_bio) == WRITE)  <<===
> BANG!
> 			mdev->writ_cnt += req->size>>9;
> 		else
> 			mdev->read_cnt += req->size>>9;
> 
> Presumably the bio_put and clear of req->private_bio should be after
> testing the direction ;-)

as I said untested...
and "obvious" :)
thanks.

> BTW: It took me quite a while to find this from the panic -- it actually
> crashed in drbd_endio_pri and the code generated for this is humungous
> and unbelievably complicated (it's about 12KB!) -- I wonder if you are
> trying to inline too much and it mightn't be better to have req_mod be a
> non-inline function and also have each case be a separate real
> function... I'd even suggest having _req_may_be_done() as a separate
> function.

probably the way to go.  let _req_may_be_done() be a non-inline
function, so the compiler can decide itself.

and, as the comment at the head of _req_mod says: we probably should
split the case branches out into "STATIC" functions, and only have the
_req_mod dispatch, or even remove the _req_mod switch statement again,
and only have the branch functions...

otherwise, what do you thin/have found out about that monster patch?

-- 
: Lars Ellenberg                                  Tel +43-1-8178292-55 :
: LINBIT Information Technologies GmbH            Fax +43-1-8178292-82 :
: Schoenbrunner Str. 244, A-1120 Vienna/Europe   http://www.linbit.com :