Re: [PATCH] MD: Do not increment resync_mismatches unless MD_RECOVERY_REQUESTED

[NeilBrown <neilb@suse.de>]

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

On Fri, 19 Apr 2013 15:09:21 -0500 Jonathan Brassow <jbrassow@redhat.com>
wrote:

> MD:  Do not increment resync_mismatches unless MD_RECOVERY_REQUESTED
> 
> resync_mismatches is used to display the number of differences that
> have been found or repaired during a scrubbing operation.  It is not
> meant to count anything during resync or repair operations.  (How
> much sense does it make to find resync_mismatches populated after an
> initial synchronization of the array?  After cleaning-up an unclean
> shutdown?  After [re]integrating a device into an existing array?)
> The incrementing of the variable must be restricted to when the user
> initiates a scrubbing operation (i.e. "check" or "repair").

How do you know what it is "meant" to do? :-)

While it might not be particularly useful, I see no point in hiding
information, and no desire to change what mismatch_cnt reports.

It may well be useful to somehow report what the real meaning for
mismatch_cnt is.  i.e. to report what the last sync_action was.
Then any use-space tool that reported mismatch_cnt, could adjust according to
whether the last operation was check/repair or resync/recovery/reshape. etc.

So if you were to provide a patch which adds "last_sync_action" or similar,
I'd certainly consider that.

Thanks,
NeilBrown

> 
> Signed-off-by: Jonathan Brassow <jbrassow@redhat.com>
> 
> Index: linux-upstream/drivers/md/raid1.c
> ===================================================================
> --- linux-upstream.orig/drivers/md/raid1.c
> +++ linux-upstream/drivers/md/raid1.c
> @@ -1878,7 +1878,8 @@ static int process_checks(struct r1bio *
>  			}
>  		} else
>  			j = 0;
> -		if (j >= 0)
> +		if ((j >= 0) &&
> +		    (test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery)))
>  			atomic64_add(r1_bio->sectors, &mddev->resync_mismatches);
>  		if (j < 0 || (test_bit(MD_RECOVERY_CHECK, &mddev->recovery)
>  			      && test_bit(BIO_UPTODATE, &sbio->bi_flags))) {
> Index: linux-upstream/drivers/md/raid10.c
> ===================================================================
> --- linux-upstream.orig/drivers/md/raid10.c
> +++ linux-upstream/drivers/md/raid10.c
> @@ -2071,7 +2071,10 @@ static void sync_request_write(struct md
>  					break;
>  			if (j == vcnt)
>  				continue;
> -			atomic64_add(r10_bio->sectors, &mddev->resync_mismatches);
> +
> +			if (test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery))
> +				atomic64_add(r10_bio->sectors,
> +					     &mddev->resync_mismatches);
>  			if (test_bit(MD_RECOVERY_CHECK, &mddev->recovery))
>  				/* Don't fix anything. */
>  				continue;
> Index: linux-upstream/drivers/md/raid5.c
> ===================================================================
> --- linux-upstream.orig/drivers/md/raid5.c
> +++ linux-upstream/drivers/md/raid5.c
> @@ -2989,7 +2989,10 @@ static void handle_parity_checks5(struct
>  			 */
>  			set_bit(STRIPE_INSYNC, &sh->state);
>  		else {
> -			atomic64_add(STRIPE_SECTORS, &conf->mddev->resync_mismatches);
> +			if (test_bit(MD_RECOVERY_REQUESTED,
> +				     &conf->mddev->recovery))
> +				atomic64_add(STRIPE_SECTORS,
> +					     &conf->mddev->resync_mismatches);
>  			if (test_bit(MD_RECOVERY_CHECK, &conf->mddev->recovery))
>  				/* don't try to repair!! */
>  				set_bit(STRIPE_INSYNC, &sh->state);
> @@ -3141,7 +3144,10 @@ static void handle_parity_checks6(struct
>  				 */
>  			}
>  		} else {
> -			atomic64_add(STRIPE_SECTORS, &conf->mddev->resync_mismatches);
> +			if (test_bit(MD_RECOVERY_REQUESTED,
> +				     &conf->mddev->recovery))
> +				atomic64_add(STRIPE_SECTORS,
> +					     &conf->mddev->resync_mismatches);
>  			if (test_bit(MD_RECOVERY_CHECK, &conf->mddev->recovery))
>  				/* don't try to repair!! */
>  				set_bit(STRIPE_INSYNC, &sh->state);
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]