Re: dm-mirror: fix crash with mirror recovery and discard

[Mike Snitzer <snitzer@redhat.com>]

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

> > > +	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?
> 
> There are two possible approaches to handling REQ_DISCARD
> 
> 1. treat REQ_DISCARD as REQ_FLUSH (this is what the patch does) --- i.e. 
> do not synchronize it with region states, do not set mirror error on 
> failure. In this mode we must assume that there are uninitialized data 
> after a flush.
> 
> For example, if there is a region that is being resynchronized and we send 
> REQ_DISCARD that overlaps this region, there is no guarantee that data in 
> this region were zeroed. 
> 
> - kcopyd reads a few blocks for resynchronization
> - REQ_DISCARD is sent to both mirror legs, both disks overwrites the area 
> with zeroes
> - kcopyd writes those blocks to the other leg => the blocks are no longer 
> zero despite REQ_DISCARD being sent

OK, thanks, but my point still stands: this is worthy of a separate
patch (with the same type of backround you provided above).

> 2. treat REQ_DISCARD as writes (i.e. synchronize it with region states, 
> wait until resynchronization finishes, etc.) --- it is possible to do it 
> this way to, but if we do it this way, we have to split REQ_DISCARD on 
> region boundaries (it is currently split only on target boundaries, 
> which is insufficient).

I think discards should be treated as writes.