tc->block_shift is the shift to get from a sector to the block_size,
and it doesn't make any sense to apply that to the block size.
Without this I get overflows of the optimal I/O size queue limit
when using large block sizes in dm-thinp.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/drivers/md/dm-thin-prov.c
================================================== =================
--- linux-2.6.orig/drivers/md/dm-thin-prov.c 2011-04-27 15:30:22.798345522 +0200
+++ linux-2.6/drivers/md/dm-thin-prov.c 2011-04-27 15:30:49.084869781 +0200
@@ -641,7 +641,7 @@ thinp_io_hints(struct dm_target *ti, str
struct thinp_c *tc = ti->private;

blk_limits_io_min(limits, 0);
- blk_limits_io_opt(limits, tc->block_size << tc->block_shift);
+ blk_limits_io_opt(limits, tc->block_size);
}

static int thinp_iterate_devices(struct dm_target *ti,

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

On Wed, 2011-04-27 at 09:41 -0400, Christoph Hellwig wrote:
> tc->block_shift is the shift to get from a sector to the block_size,
> and it doesn't make any sense to apply that to the block size.
> Without this I get overflows of the optimal I/O size queue limit
> when using large block sizes in dm-thinp.

NACK.

>From looking at dm-raid and dm-stripe it appears that the
blk_limits_io_opt function is expecting the size to be in bytes, not
sectors.

static void raid_io_hints(struct dm_target *ti, struct queue_limits *limits)
{
struct raid_set *rs = ti->private;
unsigned chunk_size = rs->md.chunk_sectors << 9;
raid5_conf_t *conf = rs->md.private;

blk_limits_io_min(limits, chunk_size);
blk_limits_io_opt(limits, chunk_size * (conf->raid_disks - conf->max_degraded));
}

static void stripe_io_hints(struct dm_target *ti,
struct queue_limits *limits)
{
struct stripe_c *sc = ti->private;
unsigned chunk_size = (sc->chunk_mask + 1) << 9;

blk_limits_io_min(limits, chunk_size);
blk_limits_io_opt(limits, chunk_size * sc->stripes);
}


tc->block_size is in sectors (you are passing sectors on the target
line?).

What's probably happening here is we should be doing:

blk_limits_io_opt(limits, min(<some theoretical max>, tc->block_size << SECTOR_SHIFT));


- Joe

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

On Wed, Apr 27, 2011 at 06:25:25PM +0100, Joe Thornber wrote:
> tc->block_size is in sectors (you are passing sectors on the target
> line?).
>
> What's probably happening here is we should be doing:
>
> blk_limits_io_opt(limits, min(<some theoretical max>, tc->block_size << SECTOR_SHIFT));

Yes, that should do it. I don't even think we need the max, the optimum
I/O size is a 32-bit value and we'll reach the limit of the possible
block sizes much earlier.

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

On Thu, 2011-04-28 at 04:42 -0400, Christoph Hellwig wrote:
> On Wed, Apr 27, 2011 at 06:25:25PM +0100, Joe Thornber wrote:
> > tc->block_size is in sectors (you are passing sectors on the target
> > line?).
> >
> > What's probably happening here is we should be doing:
> >
> > blk_limits_io_opt(limits, min(<some theoretical max>, tc->block_size << SECTOR_SHIFT));
>
> Yes, that should do it. I don't even think we need the max, the optimum
> I/O size is a 32-bit value and we'll reach the limit of the possible
> block sizes much earlier.

I'm tempted to just say min(16M, tc->block_size << SECTOR_SHIFT). Does
this sound reasonable to you?


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

On Thu, Apr 28, 2011 at 09:47:33AM +0100, Joe Thornber wrote:
> I'm tempted to just say min(16M, tc->block_size << SECTOR_SHIFT). Does
> this sound reasonable to you?

If you have a good reason for the limit go for it. But please add a
comment explaining why the limit is there, so people running into it
later on won't be completely puzzled.

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