Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   Device-mapper Development (http://www.linux-archive.org/device-mapper-development/)
-   -   dm-thin: fix discard_granularity (http://www.linux-archive.org/device-mapper-development/684276-dm-thin-fix-discard_granularity.html)

Mikulas Patocka 07-16-2012 06:35 PM

dm-thin: fix discard_granularity
 
dm-thin: fix discard_granularity

The kernel expects that limits->discard_granularity is a power of two.
Set this limit only if we use a power of two block size.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
drivers/md/dm-thin.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-3.5-rc6-fast/drivers/md/dm-thin.c
================================================== =================
--- linux-3.5-rc6-fast.orig/drivers/md/dm-thin.c 2012-07-16 20:07:49.000000000 +0200
+++ linux-3.5-rc6-fast/drivers/md/dm-thin.c 2012-07-16 20:08:01.000000000 +0200
@@ -2502,7 +2502,8 @@ static void set_discard_limits(struct po
* bios cover a block partially. A discard that spans a block boundary
* is not sent to this target.
*/
- limits->discard_granularity = pool->sectors_per_block << SECTOR_SHIFT;
+ if (pool->sectors_per_block_shift >= 0)
+ limits->discard_granularity = pool->sectors_per_block << SECTOR_SHIFT;
limits->discard_zeroes_data = pool->pf.zero_new_blocks;
}


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

Mike Snitzer 07-17-2012 03:31 PM

dm-thin: fix discard_granularity
 
On Mon, Jul 16 2012 at 2:35pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> dm-thin: fix discard_granularity
>
> The kernel expects that limits->discard_granularity is a power of two.
> Set this limit only if we use a power of two block size.
>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
>
> ---
> drivers/md/dm-thin.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Index: linux-3.5-rc6-fast/drivers/md/dm-thin.c
> ================================================== =================
> --- linux-3.5-rc6-fast.orig/drivers/md/dm-thin.c 2012-07-16 20:07:49.000000000 +0200
> +++ linux-3.5-rc6-fast/drivers/md/dm-thin.c 2012-07-16 20:08:01.000000000 +0200
> @@ -2502,7 +2502,8 @@ static void set_discard_limits(struct po
> * bios cover a block partially. A discard that spans a block boundary
> * is not sent to this target.
> */
> - limits->discard_granularity = pool->sectors_per_block << SECTOR_SHIFT;
> + if (pool->sectors_per_block_shift >= 0)
> + limits->discard_granularity = pool->sectors_per_block << SECTOR_SHIFT;
> limits->discard_zeroes_data = pool->pf.zero_new_blocks;
> }

Given the block layer's assumption that discard_granularity is always a
power of 2: thinp should disable discard if the thinp blocksize is a non
power of 2. So this patch isn't correct (discard support should be
disabled in pool_ctr based on the specified blocksize).

Future work should be:
1) explore/eliminate the block layer's power of 2 constraints
2) divorce thinp's discard limits (for discard passdown) from the pool's
blocksize

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

Mikulas Patocka 07-17-2012 07:35 PM

dm-thin: fix discard_granularity
 
On Tue, 17 Jul 2012, Mike Snitzer wrote:

> On Mon, Jul 16 2012 at 2:35pm -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
>
> > dm-thin: fix discard_granularity
> >
> > The kernel expects that limits->discard_granularity is a power of two.
> > Set this limit only if we use a power of two block size.
> >
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> >
> > ---
> > drivers/md/dm-thin.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > Index: linux-3.5-rc6-fast/drivers/md/dm-thin.c
> > ================================================== =================
> > --- linux-3.5-rc6-fast.orig/drivers/md/dm-thin.c 2012-07-16 20:07:49.000000000 +0200
> > +++ linux-3.5-rc6-fast/drivers/md/dm-thin.c 2012-07-16 20:08:01.000000000 +0200
> > @@ -2502,7 +2502,8 @@ static void set_discard_limits(struct po
> > * bios cover a block partially. A discard that spans a block boundary
> > * is not sent to this target.
> > */
> > - limits->discard_granularity = pool->sectors_per_block << SECTOR_SHIFT;
> > + if (pool->sectors_per_block_shift >= 0)
> > + limits->discard_granularity = pool->sectors_per_block << SECTOR_SHIFT;
> > limits->discard_zeroes_data = pool->pf.zero_new_blocks;
> > }
>
> Given the block layer's assumption that discard_granularity is always a
> power of 2: thinp should disable discard if the thinp blocksize is a non
> power of 2. So this patch isn't correct (discard support should be
> disabled in pool_ctr based on the specified blocksize).

discard_granularity is just a hint (and IMHO quite useless hint).

The documentation says that it indicates a size of internal allocation
unit that may be larger than the block size. The code doesn't use it this
way - it is used in FITRIM ioctl where it specifies the minimum request
size to be sent. It is also used in blkdev_issue_discard where it is used
to round down the number of sectors to discard on discard_granularity
boundary - this is wrong, it aligns request size on discard_granularity
boundary, but it doesn't align request start on this boundary.

I don't see a reason why we should disable discard because of an a rarely
used hint.

Mikulas

> Future work should be:
> 1) explore/eliminate the block layer's power of 2 constraints
> 2) divorce thinp's discard limits (for discard passdown) from the pool's
> blocksize
>

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

Vivek Goyal 07-17-2012 09:52 PM

dm-thin: fix discard_granularity
 
On Tue, Jul 17, 2012 at 03:35:04PM -0400, Mikulas Patocka wrote:
>
>
> On Tue, 17 Jul 2012, Mike Snitzer wrote:
>
> > On Mon, Jul 16 2012 at 2:35pm -0400,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > > dm-thin: fix discard_granularity
> > >
> > > The kernel expects that limits->discard_granularity is a power of two.
> > > Set this limit only if we use a power of two block size.
> > >
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > >
> > > ---
> > > drivers/md/dm-thin.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > Index: linux-3.5-rc6-fast/drivers/md/dm-thin.c
> > > ================================================== =================
> > > --- linux-3.5-rc6-fast.orig/drivers/md/dm-thin.c 2012-07-16 20:07:49.000000000 +0200
> > > +++ linux-3.5-rc6-fast/drivers/md/dm-thin.c 2012-07-16 20:08:01.000000000 +0200
> > > @@ -2502,7 +2502,8 @@ static void set_discard_limits(struct po
> > > * bios cover a block partially. A discard that spans a block boundary
> > > * is not sent to this target.
> > > */
> > > - limits->discard_granularity = pool->sectors_per_block << SECTOR_SHIFT;
> > > + if (pool->sectors_per_block_shift >= 0)
> > > + limits->discard_granularity = pool->sectors_per_block << SECTOR_SHIFT;
> > > limits->discard_zeroes_data = pool->pf.zero_new_blocks;
> > > }
> >
> > Given the block layer's assumption that discard_granularity is always a
> > power of 2: thinp should disable discard if the thinp blocksize is a non
> > power of 2. So this patch isn't correct (discard support should be
> > disabled in pool_ctr based on the specified blocksize).
>
> discard_granularity is just a hint (and IMHO quite useless hint).
>
> The documentation says that it indicates a size of internal allocation
> unit that may be larger than the block size. The code doesn't use it this
> way - it is used in FITRIM ioctl where it specifies the minimum request
> size to be sent. It is also used in blkdev_issue_discard where it is used
> to round down the number of sectors to discard on discard_granularity
> boundary - this is wrong, it aligns request size on discard_granularity
> boundary, but it doesn't align request start on this boundary.

I am not sure I understand completely what you are trying to say. But
after paolo's patch, blkdev_issue_discard() will take into account
max_discard_sectors to limit max discard request size and use
discard_granularity and discard_alignment to determine aligned request start.

First request in the range will go as it is and can be unaligned but if
discard range is big, then rest of the request start will be aligned.

Because there might be an unligned requests at the start of range drivers
will still have to handle unaligned requests, i think.

Thanks
Vivek

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

Mikulas Patocka 07-18-2012 07:10 PM

dm-thin: fix discard_granularity
 
On Tue, 17 Jul 2012, Vivek Goyal wrote:

> On Tue, Jul 17, 2012 at 03:35:04PM -0400, Mikulas Patocka wrote:
> >
> >
> > On Tue, 17 Jul 2012, Mike Snitzer wrote:
> >
> > > On Mon, Jul 16 2012 at 2:35pm -0400,
> > > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > >
> > > > dm-thin: fix discard_granularity
> > > >
> > > > The kernel expects that limits->discard_granularity is a power of two.
> > > > Set this limit only if we use a power of two block size.
> > > >
> > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > >
> > > > ---
> > > > drivers/md/dm-thin.c | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > Index: linux-3.5-rc6-fast/drivers/md/dm-thin.c
> > > > ================================================== =================
> > > > --- linux-3.5-rc6-fast.orig/drivers/md/dm-thin.c 2012-07-16 20:07:49.000000000 +0200
> > > > +++ linux-3.5-rc6-fast/drivers/md/dm-thin.c 2012-07-16 20:08:01.000000000 +0200
> > > > @@ -2502,7 +2502,8 @@ static void set_discard_limits(struct po
> > > > * bios cover a block partially. A discard that spans a block boundary
> > > > * is not sent to this target.
> > > > */
> > > > - limits->discard_granularity = pool->sectors_per_block << SECTOR_SHIFT;
> > > > + if (pool->sectors_per_block_shift >= 0)
> > > > + limits->discard_granularity = pool->sectors_per_block << SECTOR_SHIFT;
> > > > limits->discard_zeroes_data = pool->pf.zero_new_blocks;
> > > > }
> > >
> > > Given the block layer's assumption that discard_granularity is always a
> > > power of 2: thinp should disable discard if the thinp blocksize is a non
> > > power of 2. So this patch isn't correct (discard support should be
> > > disabled in pool_ctr based on the specified blocksize).
> >
> > discard_granularity is just a hint (and IMHO quite useless hint).
> >
> > The documentation says that it indicates a size of internal allocation
> > unit that may be larger than the block size. The code doesn't use it this
> > way - it is used in FITRIM ioctl where it specifies the minimum request
> > size to be sent. It is also used in blkdev_issue_discard where it is used
> > to round down the number of sectors to discard on discard_granularity
> > boundary - this is wrong, it aligns request size on discard_granularity
> > boundary, but it doesn't align request start on this boundary.
>
> I am not sure I understand completely what you are trying to say.

I mean that it is used inconsistently - sometimes it is used as s minimum
request to be sent (requests smaller than discard_granularity are not
sent). And sometimes length is rounded down to discard_granularity
boundary.

> But
> after paolo's patch, blkdev_issue_discard() will take into account
> max_discard_sectors to limit max discard request size and use
> discard_granularity and discard_alignment to determine aligned request start.

The question is - how are we supposed to propagate these parameters
through linearly appended devices, raid0, raid1 or other mappings?

For example, if you have a logical volume that consists of two linearly
appended disks, disk1 with discard_granularity1,discard_alignment1 and
disk2 with discard_granularity2,discard_alignment2 - tell me, how do you
calculate discard_granularity and discard_alignment for the combined
logical device from these four numbers? How do you calculate it if those
two disks are in raid0 or raid1?

It seems to me that would be much better to state that discard request
size is unlimited and break one long discard to several smaller discard
requests at the physical disk driver - then, the problem with combining
the limits would go away.

> First request in the range will go as it is and can be unaligned but if
> discard range is big, then rest of the request start will be aligned.
>
> Because there might be an unligned requests at the start of range drivers
> will still have to handle unaligned requests, i think.
>
> Thanks
> Vivek

Mikulas

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

Vivek Goyal 07-19-2012 03:44 PM

dm-thin: fix discard_granularity
 
On Wed, Jul 18, 2012 at 03:10:21PM -0400, Mikulas Patocka wrote:
>
>
> On Tue, 17 Jul 2012, Vivek Goyal wrote:
>
> > On Tue, Jul 17, 2012 at 03:35:04PM -0400, Mikulas Patocka wrote:
> > >
> > >
> > > On Tue, 17 Jul 2012, Mike Snitzer wrote:
> > >
> > > > On Mon, Jul 16 2012 at 2:35pm -0400,
> > > > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > > >
> > > > > dm-thin: fix discard_granularity
> > > > >
> > > > > The kernel expects that limits->discard_granularity is a power of two.
> > > > > Set this limit only if we use a power of two block size.
> > > > >
> > > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > > >
> > > > > ---
> > > > > drivers/md/dm-thin.c | 3 ++-
> > > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > Index: linux-3.5-rc6-fast/drivers/md/dm-thin.c
> > > > > ================================================== =================
> > > > > --- linux-3.5-rc6-fast.orig/drivers/md/dm-thin.c 2012-07-16 20:07:49.000000000 +0200
> > > > > +++ linux-3.5-rc6-fast/drivers/md/dm-thin.c 2012-07-16 20:08:01.000000000 +0200
> > > > > @@ -2502,7 +2502,8 @@ static void set_discard_limits(struct po
> > > > > * bios cover a block partially. A discard that spans a block boundary
> > > > > * is not sent to this target.
> > > > > */
> > > > > - limits->discard_granularity = pool->sectors_per_block << SECTOR_SHIFT;
> > > > > + if (pool->sectors_per_block_shift >= 0)
> > > > > + limits->discard_granularity = pool->sectors_per_block << SECTOR_SHIFT;
> > > > > limits->discard_zeroes_data = pool->pf.zero_new_blocks;
> > > > > }
> > > >
> > > > Given the block layer's assumption that discard_granularity is always a
> > > > power of 2: thinp should disable discard if the thinp blocksize is a non
> > > > power of 2. So this patch isn't correct (discard support should be
> > > > disabled in pool_ctr based on the specified blocksize).
> > >
> > > discard_granularity is just a hint (and IMHO quite useless hint).
> > >
> > > The documentation says that it indicates a size of internal allocation
> > > unit that may be larger than the block size. The code doesn't use it this
> > > way - it is used in FITRIM ioctl where it specifies the minimum request
> > > size to be sent. It is also used in blkdev_issue_discard where it is used
> > > to round down the number of sectors to discard on discard_granularity
> > > boundary - this is wrong, it aligns request size on discard_granularity
> > > boundary, but it doesn't align request start on this boundary.
> >
> > I am not sure I understand completely what you are trying to say.
>
> I mean that it is used inconsistently

We can always fix inconsistent use.

> - sometimes it is used as s minimum
> request to be sent (requests smaller than discard_granularity are not
> sent). And sometimes length is rounded down to discard_granularity
> boundary.

Shouldn't it be used as both. If you use it only to determine the minimum
request size, how do you come up with alignment for your next request?

I thought these are hints so you try your best and then leave it to
physical device/driver to determine how to deal with it now.

>
> > But
> > after paolo's patch, blkdev_issue_discard() will take into account
> > max_discard_sectors to limit max discard request size and use
> > discard_granularity and discard_alignment to determine aligned request start.
>
> The question is - how are we supposed to propagate these parameters
> through linearly appended devices, raid0, raid1 or other mappings?

We already deal with it in stacking limits. (blk_stack_limits()). Now
one can question whether that's the most optimal way to do things or
not.

>
> For example, if you have a logical volume that consists of two linearly
> appended disks, disk1 with discard_granularity1,discard_alignment1 and
> disk2 with discard_granularity2,discard_alignment2 - tell me, how do you
> calculate discard_granularity and discard_alignment for the combined
> logical device from these four numbers? How do you calculate it if those
> two disks are in raid0 or raid1?

I am looking at current logic in blk_stack_limits().

- For discard granularity, it just takes maximum of granularity1 and
granularity2. So as long as one granularity is multiple of other
granularity, things are just fine.

- For discard_alignment we seem to be taking lcm() of both the values but
I can't wrap my head around it that why does that work.

- For max_discard_sectors, we take minimum of two devices.

So discard_alignment seems to be only odd piece w.r.t stacking.

>
> It seems to me that would be much better to state that discard request
> size is unlimited and break one long discard to several smaller discard
> requests at the physical disk driver - then, the problem with combining
> the limits would go away.

May be. But if hints can be propagated and bio's don't have be broken
down, then it should make life simpler for drivers writers.

Thanks
Vivek

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


All times are GMT. The time now is 08:14 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.