| Message ID | 1250864238-4093-1-git-send-email-snitzer@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Alasdair Kergon |
| Headers | show |
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
>>>>> "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.
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;
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(+) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel