FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 07-16-2012, 06:01 PM
Mike Snitzer
 
Default questions about dm-thin and discard

On Mon, Jul 16 2012 at 1:14pm -0400,
Mikulas Patocka <mpatocka@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);

> 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().

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 07-16-2012, 06:32 PM
Mikulas Patocka
 
Default questions about dm-thin and discard

On Mon, 16 Jul 2012, Mike Snitzer wrote:

> On Mon, Jul 16 2012 at 1:14pm -0400,
> Mikulas Patocka <mpatocka@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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 07-16-2012, 07:51 PM
Mike Snitzer
 
Default questions about dm-thin and discard

On Mon, Jul 16 2012 at 2:32pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

>
>
> On Mon, 16 Jul 2012, Mike Snitzer wrote:
>
> > On Mon, Jul 16 2012 at 1:14pm -0400,
> > Mikulas Patocka <mpatocka@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.

That is a fair point, it does look like there should be a check for
sharing. But I could be missing something implicit with the bio prison
code (though I don't think I am).

> > > 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.

The pool only sets "ti->discards_supported = 1" if (pf.discard_enabled
&& pf.discard_passdown).

The comment provides some insight:
/*
* Setting 'discards_supported' circumvents the normal
* stacking of discard limits (this keeps the pool and
* thin devices' discard limits consistent).
*/

All being said, there is currently a disconnect on the discard limits
that are imposed for thin/pool devices vs what the underlying
data device's discard limits are. So "circumvents the normal stacking"
is treated as a feature here but it really is suspect in my view. I
just haven't circled back to look at this area closer yet.

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 07:05 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org