Mike Snitzer wrote:
On Fri, Sep 18 2009 at 11:38am -0400,
Jun'ichi Nomura <j-nomura@xxxxxxxxxxxxx> wrote:
Mike Snitzer wrote:
On Fri, Sep 18 2009 at 2:00am -0400,
Martin K. Petersen <martin.petersen@xxxxxxxxxx> wrote:
"Mike" == Mike Snitzer <snitzer@xxxxxxxxxx> writes:
blk_set_default_limits(limits);
+ limits->max_sectors = 0;
+ limits->max_hw_sectors = 0;
Mike> Seems like we may want some common variant in block even though
Mike> I'm not aware of other block drivers that would benefit...
Mike> But I'll defer to Martin and/or Jens on whether these helpers are
Mike> fine to stay in dm-table.c or should be worked into blk-settings.c
In the pre-topology days we set max_sectors to SAFE_MAX_SECTORS upon
creation of a queue. This is an old ATA-ism that's been around for a
ages.
Ideally we'd simply nuke it and drivers that really needed to lower the
bar would explicitly call blk_queue_max_sectors(). However, I'm afraid
to change the default because I'm sure there are legacy drivers lurking
somewhere that depend on it.
Seeing as blk_set_default_limits() is mostly aimed at stacking drivers I
think I'd prefer moving SAFE_MAX_SECTORS back to blk_queue_make_request
and then set max_sectors and max_hw_sectors to 0 in default_limits.
Would that work for you guys?
So you're referring to fact that this commit removed
blk_queue_max_sectors(q, SAFE_MAX_SECTORS) from blk_queue_make_request:
http://git.kernel.org/linus/e475bba2
I think I like your proposal. But, to clarify things further, are you
saying:
By moving SAFE_MAX_SECTORS back to blk_queue_make_request (after its
existing call to blk_set_default_limits right?) and having
blk_set_default_limits set max_sectors and max_hw_sectors to 0:
DM will be free to establish the proper limit stacking because the DM
limits are not derived from the queue's default limits? Because the DM
device limits are just stacked and copied to the queue, some background
for those following along:
DM's actual stacking of limits takes place when the DM table is
translated to the DM device's final queue (at table resume time), see:
http://git.kernel.org/linus/754c5fc7e
drivers/md/dm.c:dm_swap_table() calls dm_calculate_queue_limits() to
stack the limits.
drivers/md/dm.c:__bind() sets the DM device's queue_limits via
dm_table_set_restrictions()
drivers/md/dm-table.c:dm_table_set_restrictions() simply copies the
queue_limits established by DM's stacking with:
/*
* Copy table's
limits to the DM device's request_queue
*/
q->limits = *limits;
Now coming full circle:
AFAIK the only piece I'm missing is how/where your proposed changes will
account for the need to establish SAFE_MAX_SECTORS _after_ the stacking
of queue_limits: IFF max_sectors and max_hw_sectors are still 0 (like
Jun'ichi did in DM with the 2nd patch posted).
But I don't pretend to have this all sorted out in my head. I could
easily be missing some other piece(s) implicit in your proposal.
Maybe an RFC patch that illustrates your thinking would help further this
discussion?
I just sent out revised patchset:
[PATCH 1/2] dm: Set safe default max_sectors for targets with no
underlying device
https://www.redhat.com/archives/dm-devel/2009-September/msg00203.html
[PATCH 2/2] block: blk_set_default_limits sets 0 to max_sectors
https://www.redhat.com/archives/dm-devel/2009-September/msg00205.html
But I wonder better fix might be to provide blk_queue_copy_limits()
to replace this in dm-table.c:
q->limits = *limits;
where blk_queue_copy_limits() looks like this:
void blk_queue_copy_limits(struct request_queue *q, struct queue_limits
*lim)
{
q->limits = *limits;
/* fix-up bad values */
if (q->limits.max_sectors == 0 || q->limits.max_hw_sectors == 0)
blk_queue_max_sectors(q, SAFE_MAX_SECTORS);
}
so that block/blk-settings.c has full-control on default value
and dm don't need to care about the magic 'SAFE_MAX_SECTORS'.
Even better, I like that much better than your DM specific changes I
just commented on.
But rather than "fix-up bad values" I'd suggest a more helpful comment
block (like the one from your patch that I just commented on).
Thanks for the comments.
I re-posted the patchset. Please check them.
[PATCH 1/3] block: Add blk_queue_copy_limits()
https://www.redhat.com/archives/dm-devel/2009-September/msg00209.html
[PATCH 2/3] dm: Use blk_queue_copy_limits()
https://www.redhat.com/archives/dm-devel/2009-September/msg00210.html
[PATCH 3/3] block: blk_set_default_limits sets 0 to max_sectors
https://www.redhat.com/archives/dm-devel/2009-September/msg00211.html
You likely planned on cleaning the above up with a more robust comment
and I'm jumping the gun on being critical :)
I was falling asleep but woken up by your comment :)
Thanks,
--
Jun'ichi Nomura, NEC Corporation
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
