[PATCH v2] expose dm-stripe target's topology I/O hints

[PATCH v2] expose dm-stripe target's topology I/O hints

[Mike Snitzer]

Add .io_hints to 'struct target_type' to allow the I/O hints portion of
the 'struct queue_limits' to be set by each target.  Expose dm-stripe
target's topology I/O hints.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 drivers/md/dm-stripe.c        |   11 +++++++++++
 drivers/md/dm-table.c         |    4 ++++
 include/linux/device-mapper.h |    4 ++++
 3 files changed, 19 insertions(+)

Index: linux-2.6/drivers/md/dm-stripe.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-stripe.c
+++ linux-2.6/drivers/md/dm-stripe.c
@@ -329,6 +329,16 @@ static int stripe_iterate_devices(struct
 	return ret;
 }
 
+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);
+	limits->io_opt = chunk_size * sc->stripes;
+}
+
 static struct target_type stripe_target = {
 	.name   = "striped",
 	.version = {1, 2, 0},
@@ -339,6 +349,7 @@ static struct target_type stripe_target 
 	.end_io = stripe_end_io,
 	.status = stripe_status,
 	.iterate_devices = stripe_iterate_devices,
+	.io_hints = stripe_io_hints,
 };
 
 int __init dm_stripe_init(void)
Index: linux-2.6/drivers/md/dm-table.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-table.c
+++ linux-2.6/drivers/md/dm-table.c
@@ -996,6 +996,10 @@ int dm_calculate_queue_limits(struct dm_
 		ti->type->iterate_devices(ti, dm_set_device_limits,
 					  &ti_limits);
 
+		/* Set I/O hints portion of queue limits */
+		if (ti->type->io_hints)
+			ti->type->io_hints(ti, &ti_limits);
+
 		/*
 		 * Check each device area is consistent with the target's
 		 * overall queue limits.
Index: linux-2.6/include/linux/device-mapper.h
===================================================================
--- linux-2.6.orig/include/linux/device-mapper.h
+++ linux-2.6/include/linux/device-mapper.h
@@ -91,6 +91,9 @@ typedef int (*dm_iterate_devices_fn) (st
 				      iterate_devices_callout_fn fn,
 				      void *data);
 
+typedef void (*dm_io_hints_fn) (struct dm_target *ti,
+				struct queue_limits *limits);
+
 /*
  * Returns:
  *    0: The target can handle the next I/O immediately.
@@ -151,6 +154,7 @@ struct target_type {
 	dm_merge_fn merge;
 	dm_busy_fn busy;
 	dm_iterate_devices_fn iterate_devices;
+	dm_io_hints_fn io_hints;
 
 	/* For internal device-mapper use. */
 	struct list_head list;

Re: [PATCH v2] expose dm-stripe target's topology I/O hints

[Alasdair G Kergon]

On Fri, Aug 21, 2009 at 10:17:18AM -0400, Mike Snitzer wrote:
> Add .io_hints to 'struct target_type' to allow the I/O hints portion of
> the 'struct queue_limits' to be set by each target.  Expose dm-stripe
> target's topology I/O hints.
 
Do you still think this should go into 2.6.31?
If so, please supply a revised patch header that explains the benefits
of having the patch (and reference the new stuff in 2.6.31 that it
uses).

> +	limits->io_opt = chunk_size * sc->stripes;

blk_queue_io_opt() ?

>  	.version = {1, 2, 0},

Ought to inc that too when adding a new fn.

> +		/* Set I/O hints portion of queue limits */

Where else are these called I/O hints or is this a new term to define
(or avoid)?

Alasdair

Re: [PATCH v2] expose dm-stripe target's topology I/O hints

[Mike Snitzer]

On Thu, Aug 27 2009 at  9:12am -0400,
Alasdair G Kergon <agk@redhat.com> wrote:

> On Fri, Aug 21, 2009 at 10:17:18AM -0400, Mike Snitzer wrote:
> > Add .io_hints to 'struct target_type' to allow the I/O hints portion of
> > the 'struct queue_limits' to be set by each target.  Expose dm-stripe
> > target's topology I/O hints.
>  
> Do you still think this should go into 2.6.31?

Yes, it is important.  Otherwise DM won't expose the proper I/O hints in
/sys for a striped device.  This is bad because then higher-level tools
won't be able to rely on that information to properly align ther data
(e.g. mkfs.extX -E).

> If so, please supply a revised patch header that explains the benefits
> of having the patch (and reference the new stuff in 2.6.31 that it
> uses).
> 
> > +	limits->io_opt = chunk_size * sc->stripes;
> 
> blk_queue_io_opt() ?

Martin didn't expose such an interface because all existing code (both
DM and MD) does not require anything beyond a simple assignment.  I
worked with Martin to establish the blk_queue_io_min() precisely because
DM needed to avoid duplicating the block layer's logic.  I think Martin
stopped short of adding a symmetric wrapper for setting io_opt because
he felt it was unnecessary.  But I could be mis-remembering.

> >  	.version = {1, 2, 0},
> 
> Ought to inc that too when adding a new fn.

The version was already bumped during the 2.6.31-rcX cycle when I
introduced .iterate_devices (af4874e03ed82f050d5872d8c39ce64bf16b5c38).
I should bump it again?

> > +		/* Set I/O hints portion of queue limits */
> 
> Where else are these called I/O hints or is this a new term to define
> (or avoid)?

"I/O hints" isn't formally defined in Linux code but this portion of the
I/O topology infrastructure (io_min and io_opt) is commonly referred to
as "I/O Hints".  Not a new term, it is common jargon associated with the
I/O topology support.  Martin's Linux Symposium 09 "I/O Topology" slides
refer to these as "I/O hints".  I latched on to that because it
.io_hints gives precise understanding of what portion of the
queue_limits are intended to change through this DM interface.

Mike

Re: [PATCH v2] expose dm-stripe target's topology I/O hints

[Mike Snitzer]

On Thu, Aug 27 2009 at 10:19am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Thu, Aug 27 2009 at  9:12am -0400,
> Alasdair G Kergon <agk@redhat.com> wrote:
> 
> > On Fri, Aug 21, 2009 at 10:17:18AM -0400, Mike Snitzer wrote:
> > > Add .io_hints to 'struct target_type' to allow the I/O hints portion of
> > > the 'struct queue_limits' to be set by each target.  Expose dm-stripe
> > > target's topology I/O hints.
> >  
> > Do you still think this should go into 2.6.31?
> 
> Yes, it is important.  Otherwise DM won't expose the proper I/O hints in
> /sys for a striped device.  This is bad because then higher-level tools
> won't be able to rely on that information to properly align ther data
> (e.g. mkfs.extX -E).
> 
> > If so, please supply a revised patch header that explains the benefits
> > of having the patch (and reference the new stuff in 2.6.31 that it
> > uses).
> > 
> > > +	limits->io_opt = chunk_size * sc->stripes;
> > 
> > blk_queue_io_opt() ?

Did you mean why not use blk_limits_io_opt()?
 
> Martin didn't expose such an interface because all existing code (both
> DM and MD) does not require anything beyond a simple assignment.  I
> worked with Martin to establish the blk_queue_io_min() precisely because
> DM needed to avoid duplicating the block layer's logic.  I think Martin
> stopped short of adding a symmetric wrapper for setting io_opt because
> he felt it was unnecessary.  But I could be mis-remembering.

To clarify further; I meant to say blk_limits_io_min() in my above
paragraph.  We don't have access to the queue (because target's don't
have a queue); we're working with 'struct queue_limits *limits'

Mike

Re: Re: [PATCH v2] expose dm-stripe target's topology I/O hints

[Martin K. Petersen]

>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:

Mike> Martin didn't expose such an interface because all existing code
Mike> (both DM and MD) does not require anything beyond a simple
Mike> assignment.  I worked with Martin to establish the
Mike> blk_queue_io_min() precisely because DM needed to avoid
Mike> duplicating the block layer's logic.  I think Martin stopped short
Mike> of adding a symmetric wrapper for setting io_opt because he felt
Mike> it was unnecessary.

Jens is a hardliner and refuses unused functions and wrappers in block.
Even when they are trivial/obvious/symmetric.

At the time I submitted the topology code there were no users of a
blk_limits_io_opt() call so I didn't wire it up.

If you need it now, then great.  Let's add it to be consistent.

-- 
Martin K. Petersen	Oracle Linux Engineering