dm thin: support for non power of 2 pool blocksize
Non power of 2 blocksize support is needed to properly align thinp IO
on storage that has non power of 2 optimal IO sizes (e.g. RAID6 10+2).
Use do_div wrappers to support non power of 2 blocksize for the pool's
data device. do_div provides comparable performance to the power of 2
math that was performed until now (as tested on modern x86_64 hardware).
Verify that the pool's blocksize is a multiple of 64K and that the
pool's data device is a multiple of blocksize.
Eliminate pool structure's 'sectors_per_block', 'block_shift' and
remaining 4 byte holes.
+ if (dm_thin_do_mod(ti->len, block_size)) {
+ ti->error = "Data device is not a multiple of block size";
+ r = -EINVAL;
+ goto out;
+ }
+
if (kstrtoull(argv[3], 10, (unsigned long long *)&low_water_blocks)) {
ti->error = "Invalid low water mark";
r = -EINVAL;
@@ -2089,7 +2108,7 @@ static int pool_preresume(struct dm_target *ti)
if (r)
return r;
- data_size = ti->len >> pool->block_shift;
+ data_size = dm_thin_do_div(ti->len, pool->sectors_per_block);
r = dm_pool_get_data_dev_size(pool->pmd, &sb_data_size);
if (r) {
DMERR("failed to retrieve data device size");
@@ -2709,17 +2728,18 @@ static int thin_iterate_devices(struct dm_target *ti,
{
dm_block_t blocks;
struct thin_c *tc = ti->private;
+ struct pool *pool = tc->pool;
/*
* We can't call dm_pool_get_data_dev_size() since that blocks. So
* we follow a more convoluted path through to the pool's target.
*/
- if (!tc->pool->ti)
+ if (!pool->ti)
return 0; /* nothing is bound */
> + if (dm_thin_do_mod(ti->len, block_size)) {
> + ti->error = "Data device is not a multiple of block size";
> + r = -EINVAL;
> + goto out;
> + }
I don't see the need for this check. If I have a disk that isn't a
multiple of the block size why should I have to layer a linear mapping
on it to truncate it before I can use it as a data volume? Any
partial block at the end of the device is already ignored (see the
data_size calculation in pool_preresume). Is this restriction causing
some of the changes you made to the test-suite?
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
04-30-2012, 05:33 PM
Mike Snitzer
dm thin: support for non power of 2 pool blocksize
On Mon, Apr 30 2012 at 5:55am -0400,
Joe Thornber <thornber@redhat.com> wrote:
> Hi Mike,
>
> In general this looks good. A lot cleaner now you've dropped the
> specialisation of the division. A few nit-picks below.
>
> - Joe
>
> On Sat, Apr 28, 2012 at 12:44:29AM -0400, Mike Snitzer wrote:
> > +/*
> > + * do_div wrappers that don't modify the dividend
> > + */
> > +static inline sector_t dm_thin_do_div(sector_t a, __u32 b)
> > +{
> > + sector_t r = a;
> > +
> > + do_div(r, b);
> > + return r;
> > +}
> > +
> > +static inline sector_t dm_thin_do_mod(sector_t a, __u32 b)
> > +{
> > + sector_t tmp = a;
> > +
> > + return do_div(tmp, b);
> > +}
>
> Please don't inline static functions. Let the compiler make the
> decision.
But in this instance we certainly want it inlined; regardless of whether
the compiler might do it anyway. Why would we ever want to allow the
compiler to not inline it?
> Also those sector_t's are passed by value, so you don't need to
> declare r or tmp. eg, it's enough to do this:
>
> static sector_t dm_thin_do_div(sector_t a, __u32 b)
> {
> do_div(a, b);
> return a;
> }
>
> static sector_t dm_thin_do_mod(sector_t a, __u32 b)
> {
> return do_div(a, b);
> }
OK, makes sense.
> > @@ -1941,12 +1954,18 @@ static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv)
>
> ...
>
> > + if (dm_thin_do_mod(ti->len, block_size)) {
> > + ti->error = "Data device is not a multiple of block size";
> > + r = -EINVAL;
> > + goto out;
> > + }
>
> I don't see the need for this check. If I have a disk that isn't a
> multiple of the block size why should I have to layer a linear mapping
> on it to truncate it before I can use it as a data volume? Any
> partial block at the end of the device is already ignored (see the
> data_size calculation in pool_preresume). Is this restriction causing
> some of the changes you made to the test-suite?
Yes. But imposing this restriction was motivated by and the "attempt to
access beyond end of device" failures I saw with test_origin_unchanged
(I shared more details about this in private mail to you and kabi on
4/25 -- making the pool size a multiple of the non power of 2 blocksize
resolved that failure).
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
05-01-2012, 09:41 AM
Joe Thornber
dm thin: support for non power of 2 pool blocksize
On Mon, Apr 30, 2012 at 01:33:47PM -0400, Mike Snitzer wrote:
> Yes. But imposing this restriction was motivated by and the "attempt to
> access beyond end of device" failures I saw with test_origin_unchanged
> (I shared more details about this in private mail to you and kabi on
> 4/25 -- making the pool size a multiple of the non power of 2 blocksize
> resolved that failure).
This is the wrong fix; you're just hiding the real issue.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
05-05-2012, 03:57 AM
Mike Snitzer
dm thin: support for non power of 2 pool blocksize
Non power of 2 blocksize support is needed to properly align thinp IO
on storage that has non power of 2 optimal IO sizes (e.g. RAID6 10+2).
Use do_div wrappers to support non power of 2 blocksize for the pool's
data device. do_div provides comparable performance to the power of 2
math that was performed until now (as tested on modern x86_64 hardware).
Eliminate pool structure's 'sectors_per_block', 'block_shift' and
remaining 4 byte holes.
- data_size = ti->len >> pool->block_shift;
+ data_size = dm_do_div(ti->len, pool->sectors_per_block);
r = dm_pool_get_data_dev_size(pool->pmd, &sb_data_size);
if (r) {
DMERR("failed to retrieve data device size");
@@ -2711,17 +2706,18 @@ static int thin_iterate_devices(struct d
{
dm_block_t blocks;
struct thin_c *tc = ti->private;
+ struct pool *pool = tc->pool;
/*
* We can't call dm_pool_get_data_dev_size() since that blocks. So
* we follow a more convoluted path through to the pool's target.
*/
- if (!tc->pool->ti)
+ if (!pool->ti)
return 0; /* nothing is bound */
dm thin: support for non power of 2 pool blocksize
