Re: [PATCH 2/2] dm thin: support for non power of 2 pool blocksize

[Mike Snitzer <snitzer@redhat.com>]

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