dm: update max_io_len to support a split_io that is not a power of 2

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.

Quoted 7 lines · Mike Snitzer

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

Quoted 2 lines

Verify that the pool's blocksize is a multiple of 64K and that the pool's data device is a multiple of blocksize.

The non power of 2 support patch required quite a few thinp-test-suite fixes; this works for me but there may be more clever/clean ways to address rounding down the pool size to a multiple of blocksize...

On Sat, Apr 28 2012 at 12:44am -0400,
Mike Snitzer <[email protected]> wrote:
The non power of 2 support patch required quite a few thinp-test-suite fixes; this works for me but there may be more clever/clean ways to address rounding down the pool size to a multiple of blocksize...

drivers/md/dm-thin.c +7 -0

+/*
+ * 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;
+

(quoted diff) +0 -0

@@ -1941,12 +1954,18 @@ static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv)

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

Quoted 12 lines · Mike Snitzer

drivers/md/dm-thin.c +12 -0

+/*
+ * 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)

drivers/md/dm-thin.c static void requeue_io(struct thin_c *tc) · line 690

Quoted 5 lines

drivers/md/dm-thin.c +5 -0

+{
+	sector_t tmp = a;
+
+	return do_div(tmp, b);
+}

Please don't inline static functions. Let the compiler make the decision.

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);
}

Quoted 1 line

(quoted diff) +0 -0

@@ -1941,12 +1954,18 @@ static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv)

...

drivers/md/dm-thin.c static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv) · line 1967

Quoted 5 lines

drivers/md/dm-thin.c +5 -0

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

Required to support a target's use of a non power of 2 blocksize.

Quoted 1 line · Mike Snitzer

Required to support a target's use of a non power of 2 blocksize.

For which targets?

(merge_bvec supported?)

drivers/md/dm.c static sector_t max_io_len(sector_t sector, struct dm_target *ti) · line 979

Quoted 1 line

drivers/md/dm.c +1 -0

+		boundary = ti->split_io - do_div(tmp, ti->split_io);

sector_div()?

What about 32-bit arch + LBD + large split_io (from raid?)
- Is a 32-bit restriction on split_io unreasonable nowadays?
- OR reasonable on 32bit/LBD?
- OR fallback to old code there?

Quoted 3 lines · Alasdair G Kergon

Quoted 1 line · Mike Snitzer

Required to support a target's use of a non power of 2 blocksize.

For which targets?

striped and thin-pool for starters.

Quoted 1 line

(merge_bvec supported?)

Yes.

I cannot see why we'd need a split_io that is larger than 32 bits -- a 32bit split_io can support up to 2TB (2**32 * 512b sectors). Even on a LBD (raid) the stripe size (split_io) will not be so large.

But what I think what you're driving at is:

is there a benefit/reason to
maintain the old code for some target that won't ever use non power of 2 split_io (e.g. dm-raid at the moment)? I see no point for the duality in the code but I'm open to the idea if you have a specific reason in mind -- are you concerned about perf on more obscure/older hardware?

Quoted 8 lines · Mike Snitzer

Quoted 3 lines · Alasdair G Kergon

Quoted 1 line · Mike Snitzer

Required to support a target's use of a non power of 2 blocksize.

For which targets?

striped and thin-pool for starters.

Quoted 1 line

(merge_bvec supported?)

Yes.

But there's overlap between merge_bvec and split_io.
- Why does stripe_merge() have:

if (!q->merge_bvec_fn)
return max_size;

when it's already done the division?

- Couldn't that be changed to avoid split_io causing a split? (Except, as ever, across a table reload, which prevents us removing it completely.)

Quoted 3 lines

I cannot see why we'd need a split_io that is larger than 32 bits -- a 32bit split_io can support up to 2TB (2**32 * 512b sectors). Even on a LBD (raid) the stripe size (split_io) will not be so large.

But is that enforced in the raid code or not?

Quoted 1 line

But what I think what you're driving at is:

(I'm not convinced the proposed patch has been tested on 32-bit+LBD, attempting to divide by a 64-bit number etc.)

Quoted 5 lines

is there a benefit/reason to
maintain the old code for some target that won't ever use non power of 2 split_io (e.g. dm-raid at the moment)? I see no point for the duality in the code but I'm open to the idea if you have a specific reason in mind -- are you concerned about perf on more obscure/older hardware?

EITHER the 32 bit split_io *must* be enforced (after we've convinced ourselves 64 bits will never be required);
OR we keep it 64-bit and add some compat code.

Quoted 6 lines

Quoted 4 lines

I cannot see why we'd need a split_io that is larger than 32 bits -- a 32bit split_io can support up to 2TB (2**32 * 512b sectors). Even on a LBD (raid) the stripe size (split_io) will not be so large.

But is that enforced in the raid code or not?

No idea, need Jon to weigh in here. I'm hopeful we can impose 32bit within dm-raid and coordinate with Neil on getting the appropriate MD code (chunk_sectors) to reflect the reality that 32 bit is adequate.

Quoted 4 lines

Quoted 1 line

But what I think what you're driving at is:

(I'm not convinced the proposed patch has been tested on 32-bit+LBD, attempting to divide by a 64-bit number etc.)

Right, it wasn't tested on 32bit. It'll fail to build due to split_io being sector_t.

Quoted 6 lines

Quoted 5 lines

is there a benefit/reason to
maintain the old code for some target that won't ever use non power of 2 split_io (e.g. dm-raid at the moment)? I see no point for the duality in the code but I'm open to the idea if you have a specific reason in mind -- are you concerned about perf on more obscure/older hardware?

Quoted 3 lines

EITHER the 32 bit split_io *must* be enforced (after we've convinced ourselves 64 bits will never be required);
OR we keep it 64-bit and add some compat code.

Yeap, I'm hopeful we can go with the former.

Jon, what do you think?