[dm-devel] dm-mirror: fix crash with mirror recovery and discard

[Mike Snitzer]

On Fri, Jul 06 2012 at  2:56pm -0400,
Mikulas Patocka <mpatocka at redhat.com> wrote:

> dm-mirror: fix crash with mirror recovery and discard
...
> These bypasses were improperly implemented for REQ_DISCARD. In
> do_writes, REQ_DISCARD requests is always added on sync queue and
> immediatelly dispatched (even if the region is in DM_RH_RECOVERING).
> However, dm_rh_inc and dm_rh_dec is called for REQ_DISCARD resusts.
> So it violates the rule that no I/Os are started on DM_RH_RECOVERING
> regions, and causes the list corruption described above.

Yeap, my fault.  Thanks for sorting this out.  So this bug has existed
since: 5fc2ffe v2.6.38-rc1 dm raid1: support discard

Best to reference that in the patch header so stable knows how far back
this issue goes.
 
> This patch changes it so that REQ_DISCARD requests follow the same path
> as REQ_FLUSH. This avoids the crash.
> 
> Note that we can't guarantee that REQ_DISCARD zeroes the data even if
> the underlying disks support zero on discard, so this patch also sets
> ti->discard_zeroes_data_unsupported.

...

> Index: linux-3.4.4-fast/drivers/md/dm-raid1.c
> ===================================================================
> --- linux-3.4.4-fast.orig/drivers/md/dm-raid1.c	2012-07-06 19:01:27.000000000 +0200
> +++ linux-3.4.4-fast/drivers/md/dm-raid1.c	2012-07-06 19:03:53.000000000 +0200
> @@ -1091,6 +1091,7 @@ static int mirror_ctr(struct dm_target *
>  
>  	ti->num_flush_requests = 1;
>  	ti->num_discard_requests = 1;
> +	ti->discard_zeroes_data_unsupported = 1;
>  
>  	ms->kmirrord_wq = alloc_workqueue("kmirrord",
>  					  WQ_NON_REENTRANT | WQ_MEM_RECLAIM, 0);

This should be split out to a separate patch and properly justified in
the patch header.  Is there something unique to dm-mirror that renders
the underlying device's zeroing unreliable?

> Index: linux-3.4.4-fast/drivers/md/dm-region-hash.c
> ===================================================================
> --- linux-3.4.4-fast.orig/drivers/md/dm-region-hash.c	2012-07-06 19:02:28.000000000 +0200
> +++ linux-3.4.4-fast/drivers/md/dm-region-hash.c	2012-07-06 19:02:57.000000000 +0200
> @@ -524,7 +527,7 @@ void dm_rh_inc_pending(struct dm_region_
>  	struct bio *bio;
>  
>  	for (bio = bios->head; bio; bio = bio->bi_next) {
> -		if (bio->bi_rw & REQ_FLUSH)
> +		if (bio->bi_rw & (REQ_FLUSH | REQ_FLUSH))
>  			continue;
>  		rh_inc(rh, dm_rh_bio_to_region(rh, bio));
>  	}

Typo, you meant: if (bio->bi_rw & (REQ_FLUSH | REQ_DISCARD))