[Drbd-dev] FW: [DRBD-cvs] svn commit by phil - r2534 - trunk/drbd - Added missinginitialisations of rs_failed.

[Drbd-dev] FW: [DRBD-cvs] svn commit by phil - r2534 - trunk/drbd - Added missinginitialisations of rs_failed.

[Graham, Simon]

Thanks for finding/fixing my bugs! I do have one question on this change though:

		lc_put(mdev->resync,&ext->lce);
 		// no race, we are within the al_lock!
 
-		if (ext->rs_left <= ext->rs_failed) {
-			ext->rs_left = ext->rs_failed = 0;
+		if (ext->rs_left == ext->rs_failed) {
+			ext->rs_failed = 0;

With this change, when rs_left==rs_failed, you do not clear rs_left but only rs_failed -- I cleared both so that next time we used the extent we'd start from scratch again - was my understanding incorrect?

Simon

PS: FWIW, I chose to use <= rather than == deliberately as I felt this is more tolerant of problems elsewhere in the code -- yes in theory rs_left should never be less than rs_failed but <= works as well and actually better if there is a bug elsewhere that decrements rs_left when it shouldn't...

Re: [Drbd-dev] FW: [DRBD-cvs] svn commit by phil - r2534 - trunk/drbd - Added missinginitialisations of rs_failed.

[Philipp Reisner]

Am Montag, 16. Oktober 2006 15:16 schrieb Graham, Simon:
> Thanks for finding/fixing my bugs! I do have one question on this change
> though:
>
> 		lc_put(mdev->resync,&ext->lce);
>  		// no race, we are within the al_lock!
>
> -		if (ext->rs_left <= ext->rs_failed) {
> -			ext->rs_left = ext->rs_failed = 0;
> +		if (ext->rs_left == ext->rs_failed) {
> +			ext->rs_failed = 0;
>
> With this change, when rs_left==rs_failed, you do not clear rs_left but
> only rs_failed -- I cleared both so that next time we used the extent we'd
> start from scratch again - was my understanding incorrect?

Yes, I just changed it from "cleanup after use" to "initialize before use" ;)
The "cleanup after use" semantics is for all cases correct, but not for
the first use.

>
> Simon
>
> PS: FWIW, I chose to use <= rather than == deliberately as I felt this is
> more tolerant of problems elsewhere in the code -- yes in theory rs_left
> should never be less than rs_failed but <= works as well and actually
> better if there is a bug elsewhere that decrements rs_left when it
> shouldn't...

I can really follow that, and would have done it the same probably. But
with the bug that hit me today, that "<=" caused more trouble than a
"==" does ;)

The "<" case is explicitly caught with the ERR("BAD!...) statement...

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