Re: [patch 6/7 v2] MD: raid5 trim support

[NeilBrown <neilb@suse.de>]

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

On Fri, 10 Aug 2012 10:51:19 +0800 Shaohua Li <shli@fusionio.com> wrote:

> @@ -4094,6 +4159,19 @@ static void make_request(struct mddev *m
>  	bi->bi_next = NULL;
>  	bi->bi_phys_segments = 1;	/* over-loaded to count active stripes */
>  
> +	/* block layer doesn't correctly do alignment even we set correct alignment */
> +	if (unlikely(bi->bi_rw & REQ_DISCARD)) {
> +		int stripe_sectors = conf->chunk_sectors *
> +			(conf->raid_disks - conf->max_degraded);

This isn't right when an array is being reshaped.
I suspect that during a reshape we should only attempt DISCARD on the part of
the array which has already been reshaped.  On the other section we can
either fail the discard (is that a good idea?) or write zeros.

> +
> +		logical_sector = (logical_sector + stripe_sectors - 1);
> +		sector_div(logical_sector, stripe_sectors);

This would look better with DIV_ROUND_UP_SECTOR_T().


> +		sector_div(last_sector, stripe_sectors);
> +
> +		logical_sector *= stripe_sectors;
> +		last_sector *= stripe_sectors;
> +	}
> +
>  	for (;logical_sector < last_sector; logical_sector += STRIPE_SECTORS) {
>  		DEFINE_WAIT(w);
>  		int previous;
> @@ -4114,6 +4192,11 @@ static void make_request(struct mddev *m
>  			if (mddev->reshape_backwards
>  			    ? logical_sector < conf->reshape_progress
>  			    : logical_sector >= conf->reshape_progress) {
> +				/* The stripe will be reshaped soon, ignore it */
> +				if (bi->bi_rw & REQ_DISCARD) {
> +					spin_unlock_irq(&conf->device_lock);
> +					goto next_stripe;
> +				}
>  				previous = 1;
>  			} else {
>  				if (mddev->reshape_backwards
> @@ -4202,6 +4285,12 @@ static void make_request(struct mddev *m
>  			finish_wait(&conf->wait_for_overlap, &w);
>  			break;
>  		}
> +next_stripe:
> +		/* For discard, we always discard one stripe */
> +		if (unlikely((bi->bi_rw & REQ_DISCARD) &&
> +		   !((logical_sector + STRIPE_SECTORS) % conf->chunk_sectors)))

You are using '%' on a sector_t value.  That isn't right - need to use
sector_div().


Thanks,
NeilBrown


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