md question re: max_hw_sectors_kb

PATCH Michael Reed 5 participants 9 messages May 3, 2011 – May 31, 2011 MD RAID

lore

Share & tools

lore

Michael Reed

author

2011-05-03 20:20 UTC #0 lore raw

Resending to linux-raid.

Hi Neil,

My name is Mike Reed. I work at SGI. I've been looking at a performance issue with md writes and have tracked it down to the md device not having a high enough initial max_hw_sectors_kb setting.

There is code in blk_queue_make_request() which lowers the default value from INT_MAX to BLK_SAFE_MAX_SECTORS, which is 255. This is generally lower than all the underlying devices with which I use md.

As md appears to be a stacking driver, i.e., it calls disk_stack_limits() for each member of a volume, it would seem reasonable for md to use the, INT_MAX setting for max_hw_sectors_kb instead of BLK_SAFE_MAX_SECTORS.

I have tried this, and have observed that md correctly limits the md device's max_sectors_kb to the value of the underlying devices in my mirror volume.

Is this the correct way to address this issue?

Applies to linux-2.6.39-rc4.

Signed-off-by Michael Reed <[email protected]>

md/md.c.new renamed from md/md.c +1 -0

@@ -4328,6 +4328,7 @@ static int md_alloc(dev_t dev, char *nam
 	mddev->queue->queuedata = mddev;
 
 	blk_queue_make_request(mddev->queue, md_make_request);
+	blk_queue_max_sectors(mddev->queue, INT_MAX);
 
 	disk = alloc_disk(1 << shift);
 	if (!disk) {

Martin K. Petersen

2011-05-04 17:58 UTC #1 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Michael" == Michael Reed <[email protected]> writes:

Michael> There is code in blk_queue_make_request() which lowers the Michael> default value from INT_MAX to BLK_SAFE_MAX_SECTORS, which is Michael> 255. This is generally lower than all the underlying devices Michael> with which I use md.

Yeah, the SAFE value is there to appease legacy low-level drivers.

Michael> As md appears to be a stacking driver, i.e., it calls Michael> disk_stack_limits() for each member of a volume, it would seem Michael> reasonable for md to use the, INT_MAX setting for Michael> max_hw_sectors_kb instead of BLK_SAFE_MAX_SECTORS.

Your fix is functionally correct. However, another case just popped up this week where we need to distinguish between stacking driver and LLD defaults. So I think we should try to handle this at the block layer instead of explicitly tweaking this knob in MD.

I'll get this fixed up and will CC: you on the patch.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Bernd Schubert

2011-05-04 18:08 UTC #2 lore raw

Quoted 15 lines · Martin K. Petersen

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Michael" == Michael Reed<[email protected]> writes:

Michael>  There is code in blk_queue_make_request() which lowers the
Michael>  default value from INT_MAX to BLK_SAFE_MAX_SECTORS, which is
Michael>  255.  This is generally lower than all the underlying devices
Michael>  with which I use md.

Yeah, the SAFE value is there to appease legacy low-level drivers.

Michael>  As md appears to be a stacking driver, i.e., it calls
Michael>  disk_stack_limits() for each member of a volume, it would seem
Michael>  reasonable for md to use the, INT_MAX setting for
Michael>  max_hw_sectors_kb instead of BLK_SAFE_MAX_SECTORS.

Quoted 7 lines

Your fix is functionally correct. However, another case just popped up this week where we need to distinguish between stacking driver and LLD defaults. So I think we should try to handle this at the block layer instead of explicitly tweaking this knob in MD.
I'll get this fixed up and will CC: you on the patch.

Could you please CC me or linux-raid as well? I have a similar patch for raid1 and while I'm not working on Lustre anymore, I know that Lustre also always has a another kernel patch for raid5...

Thanks,
Bernd

NeilBrown

2011-05-09 23:52 UTC #3 lore raw

In reply to Martin K. Petersen

Quoted 15 lines · On Wed, 04 May 2011 13:58:05 -0400 "Martin K. Petersen"…

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Michael" == Michael Reed <[email protected]> writes:
Michael> There is code in blk_queue_make_request() which lowers the Michael> default value from INT_MAX to BLK_SAFE_MAX_SECTORS, which is Michael> 255. This is generally lower than all the underlying devices Michael> with which I use md.
Yeah, the SAFE value is there to appease legacy low-level drivers.
Michael> As md appears to be a stacking driver, i.e., it calls Michael> disk_stack_limits() for each member of a volume, it would seem Michael> reasonable for md to use the, INT_MAX setting for Michael> max_hw_sectors_kb instead of BLK_SAFE_MAX_SECTORS.

Quoted 7 lines

Your fix is functionally correct. However, another case just popped up this week where we need to distinguish between stacking driver and LLD defaults. So I think we should try to handle this at the block layer instead of explicitly tweaking this knob in MD.
I'll get this fixed up and will CC: you on the patch.

What case is this?

The is another problem that I am aware of with this patch - maybe it is the same was what you are thinking of - maybe not.

If you have FS -> DM -> MD, then any change that MD makes to max_hw_sectors_kb will not be visible to the FS. So adding and activating a hot spare with smaller max_hw_sectors_kb cause cause it to receive requests that are too big.

With the current default of BLK_SAFE_MAX_SECTORS, that only seems to affect a few USB devices. If we raise the default we could see problems happening more often.

So we really need a propery resolution to this problem first. i.e. A way for 'dm' to notice when 'md' changes its parameters - or in general any stacking deivce to find out when an underlying device changes in any way.

I would implement this by having blkdev_get{,_by_path,_by_dev} take an extra arg which is a pointer to a struct of functions. In the first instance there would be just one which tells the claimer that something in queue.limits has changed. Later we could add other calls to help with size changes.

So when md adds a new device, it call disk_stack_limits to updates its limits, then if the bdev for the mddev is claimed with a non-NULL operations pointer, it calls the 'limits_have_changed' function.

Thoughts?

NeilBrown

Martin K. Petersen

2011-05-12 03:51 UTC #4 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Neil" == NeilBrown <[email protected]> writes:

Neil,

Quoted 3 lines

Quoted 3 lines
Your fix is functionally correct. However, another case just popped up this week where we need to distinguish between stacking driver and LLD defaults.

Neil> What case is this?

This particular case involved the need to set different defaults for discard depending on whether it was a stacking or a low level driver.

Neil> If you have FS -> DM -> MD, then any change that MD makes to Neil> max_hw_sectors_kb will not be visible to the FS. So adding and Neil> activating a hot spare with smaller max_hw_sectors_kb cause cause Neil> it to receive requests that are too big.

Yeah, this issue pops up occasionally. Alasdair and I were discussing it just a couple of weeks ago.

Neil> So we really need a propery resolution to this problem first. Neil> i.e. A way for 'dm' to notice when 'md' changes its parameters - Neil> or in general any stacking deivce to find out when an underlying Neil> device changes in any way.

Neil> I would implement this by having blkdev_get{,_by_path,_by_dev} Neil> take an extra arg which is a pointer to a struct of functions. In Neil> the first instance there would be just one which tells the claimer Neil> that something in queue.limits has changed. Later we could add Neil> other calls to help with size changes.

I agree we need a way to propagate queue limit and capacity changes up the stack. I'll put in on my todo list.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

[email protected]

2011-05-31 03:06 UTC #5 lore raw

Hi Mike,

Thank you for the patch. I've tested it though on 2.6.38 and do not see any significant change in md RAID 5 write performance compared to the unpatched kernel. Can you clarify how you may have achieved a demonstrable improvement?

-Tommy

Quoted 37 lines · Martin K. Petersen

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Neil" == NeilBrown <[email protected]> writes:
Neil,
Quoted 3 lines
Quoted 3 lines
Your fix is functionally correct. However, another case just popped up this week where we need to distinguish between stacking driver and LLD defaults.
Neil> What case is this?
This particular case involved the need to set different defaults for discard depending on whether it was a stacking or a low level driver.
Neil> If you have FS -> DM -> MD, then any change that MD makes to Neil> max_hw_sectors_kb will not be visible to the FS. So adding and Neil> activating a hot spare with smaller max_hw_sectors_kb cause cause Neil> it to receive requests that are too big.
Yeah, this issue pops up occasionally. Alasdair and I were discussing it just a couple of weeks ago.
Neil> So we really need a propery resolution to this problem first. Neil> i.e. A way for 'dm' to notice when 'md' changes its parameters - Neil> or in general any stacking deivce to find out when an underlying Neil> device changes in any way.
Neil> I would implement this by having blkdev_get{,_by_path,_by_dev} Neil> take an extra arg which is a pointer to a struct of functions. In Neil> the first instance there would be just one which tells the claimer Neil> that something in queue.limits has changed. Later we could add Neil> other calls to help with size changes.
I agree we need a way to propagate queue limit and capacity changes up the stack. I'll put in on my todo list.

Quoted 7 lines

--
Martin K. Petersen Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

NeilBrown

2011-05-06 04:24 UTC #6 lore raw

In reply to Michael Reed

Quoted 31 lines · On Tue, 03 May 2011 15:20:46 -0500 Michael Reed <[email protected]> wrote:

Resending to linux-raid.
Hi Neil,
My name is Mike Reed. I work at SGI. I've been looking at a performance issue with md writes and have tracked it down to the md device not having a high enough initial max_hw_sectors_kb setting.
There is code in blk_queue_make_request() which lowers the default value from INT_MAX to BLK_SAFE_MAX_SECTORS, which is 255. This is generally lower than all the underlying devices with which I use md.
As md appears to be a stacking driver, i.e., it calls disk_stack_limits() for each member of a volume, it would seem reasonable for md to use the, INT_MAX setting for max_hw_sectors_kb instead of BLK_SAFE_MAX_SECTORS.
I have tried this, and have observed that md correctly limits the md device's max_sectors_kb to the value of the underlying devices in my mirror volume.
Is this the correct way to address this issue?
Applies to linux-2.6.39-rc4.
Signed-off-by Michael Reed <[email protected]>

md/md.c.new static int md_alloc(dev_t dev, char *nam · line 4334

Quoted 7 lines

md/md.c.new renamed from md/md.c +1 -0

@@ -4328,6 +4328,7 @@ static int md_alloc(dev_t dev, char *nam
 	mddev->queue->queuedata = mddev;
 
 	blk_queue_make_request(mddev->queue, md_make_request);
+	blk_queue_max_sectors(mddev->queue, INT_MAX);
 
 	disk = alloc_disk(1 << shift);
 	if (!disk) {

Hi Mike,
sorry for not responding to this earlier - it seemed to keep falling through cracks for some reason :-(

Yes, I completely agree with your analysis and think you patch is correct and useful.
I will queue it up for the next merge window.

thanks,
NeilBrown

NeilBrown

2011-05-06 04:40 UTC #7 lore raw

Quoted 45 lines · On Fri, 6 May 2011 14:24:34 +1000 NeilBrown <[email protected]> wrote:

Quoted 31 lines · On Tue, 03 May 2011 15:20:46 -0500 Michael Reed <[email protected]> wrote:
Resending to linux-raid.
Hi Neil,
My name is Mike Reed. I work at SGI. I've been looking at a performance issue with md writes and have tracked it down to the md device not having a high enough initial max_hw_sectors_kb setting.
There is code in blk_queue_make_request() which lowers the default value from INT_MAX to BLK_SAFE_MAX_SECTORS, which is 255. This is generally lower than all the underlying devices with which I use md.
As md appears to be a stacking driver, i.e., it calls disk_stack_limits() for each member of a volume, it would seem reasonable for md to use the, INT_MAX setting for max_hw_sectors_kb instead of BLK_SAFE_MAX_SECTORS.
I have tried this, and have observed that md correctly limits the md device's max_sectors_kb to the value of the underlying devices in my mirror volume.
Is this the correct way to address this issue?
Applies to linux-2.6.39-rc4.
Signed-off-by Michael Reed <[email protected]>
md/md.c.new static int md_alloc(dev_t dev, char *nam · line 4334
Quoted 7 lines
md/md.c.new renamed from md/md.c +1 -0
@@ -4328,6 +4328,7 @@ static int md_alloc(dev_t dev, char *nam
 	mddev->queue->queuedata = mddev;
 
 	blk_queue_make_request(mddev->queue, md_make_request);
+	blk_queue_max_sectors(mddev->queue, INT_MAX);
 
 	disk = alloc_disk(1 << shift);
 	if (!disk) {
Hi Mike,
sorry for not responding to this earlier - it seemed to keep falling through cracks for some reason :-(

Quoted 6 lines

Yes, I completely agree with your analysis and think you patch is correct and useful.
I will queue it up for the next merge window.
thanks,
NeilBrown

...except of course that it should be

     blk_queue_max_hw_sectors(......)
                   ^^

because blk_queue_max_sectors was renamed back in 2.6.34 :-)

NeilBrown

Michael Reed

author

2011-05-09 16:02 UTC #8 lore raw

Quoted 54 lines · NeilBrown

Quoted 45 lines · On Fri, 6 May 2011 14:24:34 +1000 NeilBrown<[email protected]> wrote:
Quoted 31 lines · On Tue, 03 May 2011 15:20:46 -0500 Michael Reed<[email protected]> wrote:
Resending to linux-raid.
Hi Neil,
My name is Mike Reed. I work at SGI. I've been looking at a performance issue with md writes and have tracked it down to the md device not having a high enough initial max_hw_sectors_kb setting.
There is code in blk_queue_make_request() which lowers the default value from INT_MAX to BLK_SAFE_MAX_SECTORS, which is 255. This is generally lower than all the underlying devices with which I use md.
As md appears to be a stacking driver, i.e., it calls disk_stack_limits() for each member of a volume, it would seem reasonable for md to use the, INT_MAX setting for max_hw_sectors_kb instead of BLK_SAFE_MAX_SECTORS.
I have tried this, and have observed that md correctly limits the md device's max_sectors_kb to the value of the underlying devices in my mirror volume.
Is this the correct way to address this issue?
Applies to linux-2.6.39-rc4.
Signed-off-by Michael Reed<[email protected]>
md/md.c.new static int md_alloc(dev_t dev, char *nam · line 4334
Quoted 7 lines
md/md.c.new renamed from md/md.c +1 -0
@@ -4328,6 +4328,7 @@ static int md_alloc(dev_t dev, char *nam
  	mddev->queue->queuedata = mddev;
 
  	blk_queue_make_request(mddev->queue, md_make_request);
+	blk_queue_max_sectors(mddev->queue, INT_MAX);
 
  	disk = alloc_disk(1<<  shift);
  	if (!disk) {
Hi Mike,
sorry for not responding to this earlier - it seemed to keep falling through cracks for some reason :-(
Quoted 6 lines
Yes, I completely agree with your analysis and think you patch is correct and useful.
I will queue it up for the next merge window.
thanks,
NeilBrown

Quoted 7 lines

...except of course that it should be
      blk_queue_max_hw_sectors(......)
                    ^^
because blk_queue_max_sectors was renamed back in 2.6.34 :-)
NeilBrown

Thank you for catching that! I've attached a corrected patch for the record.

Signed-off-by Michael Reed<[email protected]>

md/md.c.new renamed from md/md.c +1 -0

@@ -4328,6 +4328,7 @@ static int md_alloc(dev_t dev, char *nam
 	mddev->queue->queuedata = mddev;
 
 	blk_queue_make_request(mddev->queue, md_make_request);
+	blk_queue_max_hw_sectors(mddev->queue, INT_MAX);
 
 	disk = alloc_disk(1 << shift);
 	if (!disk) {