[dm-devel] questions about dm-thin and discard

[Mikulas Patocka]

On Mon, 16 Jul 2012, Mike Snitzer wrote:

> On Mon, Jul 16 2012 at  1:14pm -0400,
> Mikulas Patocka <mpatocka at redhat.com> wrote:
> 
> > Hi Joe
> > 
> > I would like to ask you about this code path: In process_discard, there is 
> > a branch with a comment "This path is hit if people are ignoring 
> > limits->discard_granularity." It trims the discard request so that it 
> > doesn't span a block boundary and submits it.
> > 
> > The question is: what if the block is shared? In this case, we can't 
> > submit discard to the block, because it would damage the other snapshot 
> > that is sharing this block. Shouldn't there be shomething like this?
> > if ((!lookup_result.shared) & pool->pf.discard_passdown) {
> > 	remap_and_issue(tc, bio, lookup_result.block);
> > } else {
> > 	bio_endio(bio, 0) 
> > } 
> > ... or is it tested elsewhere and am I missing something?
> 
> in process_discard:
> 
>  m->pass_discard = (!lookup_result.shared) && pool->pf.disard_passdown;
> 
> then in process_prepared_discard:
> 
>  if (m->pass_discard)
>         remap_and_issue(tc, m->bio, m->data_block);
>  else
>         bio_endio(m->bio, 0);

This is called in process_discard if io_overlaps_block returns true. But 
if io_overlaps_block returns false, this check is not done. There is: 

cell_release_singleton(cell, bio);
cell_release_singleton(cell2, bio); 
remap_and_issue(tc, bio, lookup_result.block);

... remap_and_issue calls remap (which just changes bio->bi_bdev and 
bio->bi_sector) and issue (which calls generic_make_request) - so we issue 
discard to a potentially shared block here.

> > Another question is about setting "ti->discards_supported = 1" in 
> > pool_ctr. ti->discards_supported means that the target supports discards 
> > even if the underlying disk doesn't. Since the pool device is passing 
> > anyth I/O unchanged to the underlying disk, ti->discards_supported 
> > shouldn't be set. Or is there any other reason why is it set?
> 
> The thin device's bios will be remapped to the pool device.
> 
> process_prepared_discard's remap_and_issue() will send the bio to the
> pool device via generic_make_request().

If the underlying device doesn't support discards, there is no poin in 
setting "ti->discards_supported = 1" on the pool device. You should set 
"ti->discards_supported = 1" should be set on the thin device because thin 
supports discards even if the underlying disk doesn't. But pool doesn't 
support discards if the underlying disk doesn't, so it shouldn't be set.

Mikulas