* Re: Request for assistance
From: o1bigtenor @ 2016-07-06 18:28 UTC (permalink / raw)
To: Wols Lists; +Cc: Adam Goryachev, Linux-RAID
In-Reply-To: <577CFEC4.7040704@youngman.org.uk>
On Wed, Jul 6, 2016 at 7:51 AM, Wols Lists <antlists@youngman.org.uk> wrote:
> On 06/07/16 13:14, o1bigtenor wrote:
>> On Tue, Jul 5, 2016 at 8:55 PM, Adam Goryachev
>> <mailinglists@websitemanagers.com.au> wrote:
>>> On 06/07/16 10:13, o1bigtenor wrote:
>>>>
>>>> Greetings
>>>>
>>>> Running a Raid 10 array with 4 - 3 TB drives. Have a UPS but this area
>>>> gets significant lightning and also brownout (rural power) events.
>>>>
>> snip
snip
>>
>> So my array is back up - - - thank you very much for your assistance!!!
>>
> But why did they drop ... are you using desktop drives? I use Seagate
> Barracudas - NOT a particularly good idea. You should be using WD Red,
> Seagate NAS, or similar.
Sorry - - - this system is 4 1 TB WD Red drives
>
> "smartctl -x /dev/sdx" will give you an idea of what's going on. Search
> the list for "timeout error" for an idea of the grief you'll get if
> you're using desktop drives ...
>
> If smartctl says smart is disabled, enable it. When I do, my drive comes
> back (using the -x option again) saying "SCT Error Recovery not
> supported". This is a no-no for a decent raid drive. I think the other
> acronyms are ETL or TLS - either way you can control how the drive
> reports an error back to the OS. Which is why you need proper raid
> drives (the manufacturers have downgraded the firmware on desktop drives :-(
>
> You need to fix the WHY or it could easily happen again. And this could
> well be why ... (if you've had a problem on a desktop drive, it WILL
> happen again, and data loss is quite likely ... even if you recover the
> bulk of the drive).
My best understanding as to the why is - - dirty power - - - fixing that means
going off-grid. Expensive and not happening any time soon although I would
really like that.
As I do not understand the error messages in smartctl I add the following
(maybe someone would explain what they mean) :
smartctl -x /dev/sdf
smartctl 6.4 2014-10-07 r4002 [x86_64-linux-4.1.0-2-amd64] (local build)
Copyright (C) 2002-14, Bruce Allen, Christian Franke, www.smartmontools.org
=== START OF INFORMATION SECTION ===
Model Family: Western Digital Red (AF)
Device Model: WDC WD10EFRX-68FYTN0
Serial Number: WD-WCC4J4XV62F4
LU WWN Device Id: 5 0014ee 20cd9d7d1
Firmware Version: 82.00A82
User Capacity: 1,000,204,886,016 bytes [1.00 TB]
Sector Sizes: 512 bytes logical, 4096 bytes physical
Rotation Rate: 5400 rpm
Device is: In smartctl database [for details use: -P show]
ATA Version is: ACS-2 (minor revision not indicated)
SATA Version is: SATA 3.0, 6.0 Gb/s (current: 6.0 Gb/s)
Local Time is: Wed Jul 6 13:21:25 2016 CDT
SMART support is: Available - device has SMART capability.
SMART support is: Enabled
AAM feature is: Unavailable
APM feature is: Unavailable
Rd look-ahead is: Enabled
Write cache is: Enabled
ATA Security is: Disabled, NOT FROZEN [SEC1]
Wt Cache Reorder: Enabled
=== START OF READ SMART DATA SECTION ===
SMART overall-health self-assessment test result: PASSED
General SMART Values:
Offline data collection status: (0x00) Offline data collection activity
was never started.
Auto Offline Data Collection: Disabled.
Self-test execution status: ( 0) The previous self-test routine completed
without error or no self-test has ever
been run.
Total time to complete Offline
data collection: (13320) seconds.
Offline data collection
capabilities: (0x7b) SMART execute Offline immediate.
Auto Offline data collection on/off support.
Suspend Offline collection upon new
command.
Offline surface scan supported.
Self-test supported.
Conveyance Self-test supported.
Selective Self-test supported.
SMART capabilities: (0x0003) Saves SMART data before entering
power-saving mode.
Supports SMART auto save timer.
Error logging capability: (0x01) Error logging supported.
General Purpose Logging supported.
Short self-test routine
recommended polling time: ( 2) minutes.
Extended self-test routine
recommended polling time: ( 152) minutes.
Conveyance self-test routine
recommended polling time: ( 5) minutes.
SCT capabilities: (0x303d) SCT Status supported.
SCT Error Recovery Control supported.
SCT Feature Control supported.
SCT Data Table supported.
SMART Attributes Data Structure revision number: 16
Vendor Specific SMART Attributes with Thresholds:
ID# ATTRIBUTE_NAME FLAGS VALUE WORST THRESH FAIL RAW_VALUE
1 Raw_Read_Error_Rate POSR-K 200 200 051 - 0
3 Spin_Up_Time POS--K 139 139 021 - 4050
4 Start_Stop_Count -O--CK 100 100 000 - 23
5 Reallocated_Sector_Ct PO--CK 200 200 140 - 0
7 Seek_Error_Rate -OSR-K 200 200 000 - 0
9 Power_On_Hours -O--CK 100 099 000 - 423
10 Spin_Retry_Count -O--CK 100 253 000 - 0
11 Calibration_Retry_Count -O--CK 100 253 000 - 0
12 Power_Cycle_Count -O--CK 100 100 000 - 6
192 Power-Off_Retract_Count -O--CK 200 200 000 - 1
193 Load_Cycle_Count -O--CK 198 198 000 - 8922
194 Temperature_Celsius -O---K 115 107 000 - 28
196 Reallocated_Event_Count -O--CK 200 200 000 - 0
197 Current_Pending_Sector -O--CK 200 200 000 - 0
198 Offline_Uncorrectable ----CK 100 253 000 - 0
199 UDMA_CRC_Error_Count -O--CK 200 200 000 - 0
200 Multi_Zone_Error_Rate ---R-- 100 253 000 - 0
||||||_ K auto-keep
|||||__ C event count
||||___ R error rate
|||____ S speed/performance
||_____ O updated online
|______ P prefailure warning
General Purpose Log Directory Version 1
SMART Log Directory Version 1 [multi-sector log support]
Address Access R/W Size Description
0x00 GPL,SL R/O 1 Log Directory
0x01 SL R/O 1 Summary SMART error log
0x02 SL R/O 5 Comprehensive SMART error log
0x03 GPL R/O 6 Ext. Comprehensive SMART error log
0x04 GPL,SL R/O 8 Device Statistics log
0x06 SL R/O 1 SMART self-test log
0x07 GPL R/O 1 Extended self-test log
0x09 SL R/W 1 Selective self-test log
0x10 GPL R/O 1 SATA NCQ Queued Error log
0x11 GPL R/O 1 SATA Phy Event Counters log
0x21 GPL R/O 1 Write stream error log
0x22 GPL R/O 1 Read stream error log
0x80-0x9f GPL,SL R/W 16 Host vendor specific log
0xa0-0xa7 GPL,SL VS 16 Device vendor specific log
0xa8-0xb7 GPL,SL VS 1 Device vendor specific log
0xbd GPL,SL VS 1 Device vendor specific log
0xc0 GPL,SL VS 1 Device vendor specific log
0xc1 GPL VS 93 Device vendor specific log
0xe0 GPL,SL R/W 1 SCT Command/Status
0xe1 GPL,SL R/W 1 SCT Data Transfer
SMART Extended Comprehensive Error Log Version: 1 (6 sectors)
Device Error Count: 1
CR = Command Register
FEATR = Features Register
COUNT = Count (was: Sector Count) Register
LBA_48 = Upper bytes of LBA High/Mid/Low Registers ] ATA-8
LH = LBA High (was: Cylinder High) Register ] LBA
LM = LBA Mid (was: Cylinder Low) Register ] Register
LL = LBA Low (was: Sector Number) Register ]
DV = Device (was: Device/Head) Register
DC = Device Control Register
ER = Error register
ST = Status register
Powered_Up_Time is measured from power on, and printed as
DDd+hh:mm:SS.sss where DD=days, hh=hours, mm=minutes,
SS=sec, and sss=millisec. It "wraps" after 49.710 days.
Error 1 [0] occurred at disk power-on lifetime: 395 hours (16 days + 11 hours)
When the command that caused the error occurred, the device was
active or idle.
After command completion occurred, registers were:
ER -- ST COUNT LBA_48 LH LM LL DV DC
-- -- -- == -- == == == -- -- -- -- --
10 -- 51 00 00 00 00 18 11 28 00 40 00 Error: IDNF at LBA =
0x18112800 = 403777536
Commands leading to the command that caused the error were:
CR FEATR COUNT LBA_48 LH LM LL DV DC Powered_Up_Time Command/Feature_Name
-- == -- == -- == == == -- -- -- -- -- --------------- --------------------
61 51 78 00 e0 00 00 18 06 38 00 40 08 5d+03:01:34.882 WRITE FPDMA QUEUED
61 50 00 00 d8 00 00 18 05 e8 00 40 08 5d+03:01:34.882 WRITE FPDMA QUEUED
61 50 00 00 d0 00 00 18 05 98 00 40 08 5d+03:01:34.882 WRITE FPDMA QUEUED
61 50 00 00 c8 00 00 18 05 48 00 40 08 5d+03:01:34.882 WRITE FPDMA QUEUED
61 50 00 00 c0 00 00 18 04 f8 00 40 08 5d+03:01:34.882 WRITE FPDMA QUEUED
SMART Extended Self-test Log Version: 1 (1 sectors)
No self-tests have been logged. [To run self-tests, use: smartctl -t]
SMART Selective self-test log data structure revision number 1
SPAN MIN_LBA MAX_LBA CURRENT_TEST_STATUS
1 0 0 Not_testing
2 0 0 Not_testing
3 0 0 Not_testing
4 0 0 Not_testing
5 0 0 Not_testing
Selective self-test flags (0x0):
After scanning selected spans, do NOT read-scan remainder of disk.
If Selective self-test is pending on power-up, resume after 0 minute delay.
SCT Status Version: 3
SCT Version (vendor specific): 258 (0x0102)
SCT Support Level: 1
Device State: Active (0)
Current Temperature: 28 Celsius
Power Cycle Min/Max Temperature: 21/28 Celsius
Lifetime Min/Max Temperature: 20/36 Celsius
Under/Over Temperature Limit Count: 0/0
Vendor specific:
00 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
SCT Temperature History Version: 2
Temperature Sampling Period: 1 minute
Temperature Logging Interval: 1 minute
Min/Max recommended Temperature: 0/60 Celsius
Min/Max Temperature Limit: -41/85 Celsius
Temperature History Size (Index): 478 (237)
Index Estimated Time Temperature Celsius
238 2016-07-06 05:24 26 *******
... ..( 34 skipped). .. *******
273 2016-07-06 05:59 26 *******
274 2016-07-06 06:00 27 ********
... ..( 8 skipped). .. ********
283 2016-07-06 06:09 27 ********
284 2016-07-06 06:10 26 *******
... ..( 3 skipped). .. *******
288 2016-07-06 06:14 26 *******
289 2016-07-06 06:15 27 ********
... ..( 42 skipped). .. ********
332 2016-07-06 06:58 27 ********
333 2016-07-06 06:59 28 *********
... ..( 18 skipped). .. *********
352 2016-07-06 07:18 28 *********
353 2016-07-06 07:19 29 **********
... ..( 3 skipped). .. **********
357 2016-07-06 07:23 29 **********
358 2016-07-06 07:24 28 *********
... ..( 29 skipped). .. *********
388 2016-07-06 07:54 28 *********
389 2016-07-06 07:55 29 **********
390 2016-07-06 07:56 28 *********
391 2016-07-06 07:57 28 *********
392 2016-07-06 07:58 29 **********
393 2016-07-06 07:59 28 *********
394 2016-07-06 08:00 28 *********
395 2016-07-06 08:01 29 **********
... ..( 4 skipped). .. **********
400 2016-07-06 08:06 29 **********
401 2016-07-06 08:07 ? -
402 2016-07-06 08:08 21 **
403 2016-07-06 08:09 21 **
404 2016-07-06 08:10 21 **
405 2016-07-06 08:11 22 ***
406 2016-07-06 08:12 22 ***
407 2016-07-06 08:13 22 ***
408 2016-07-06 08:14 24 *****
409 2016-07-06 08:15 24 *****
410 2016-07-06 08:16 23 ****
411 2016-07-06 08:17 23 ****
412 2016-07-06 08:18 23 ****
413 2016-07-06 08:19 24 *****
... ..( 2 skipped). .. *****
416 2016-07-06 08:22 24 *****
417 2016-07-06 08:23 25 ******
... ..( 3 skipped). .. ******
421 2016-07-06 08:27 25 ******
422 2016-07-06 08:28 26 *******
... ..( 60 skipped). .. *******
5 2016-07-06 09:29 26 *******
6 2016-07-06 09:30 27 ********
... ..(106 skipped). .. ********
113 2016-07-06 11:17 27 ********
114 2016-07-06 11:18 26 *******
... ..(113 skipped). .. *******
228 2016-07-06 13:12 26 *******
229 2016-07-06 13:13 27 ********
... ..( 4 skipped). .. ********
234 2016-07-06 13:18 27 ********
235 2016-07-06 13:19 26 *******
236 2016-07-06 13:20 26 *******
237 2016-07-06 13:21 26 *******
SCT Error Recovery Control:
Read: 70 (7.0 seconds)
Write: 70 (7.0 seconds)
Device Statistics (GP Log 0x04)
Page Offset Size Value Description
1 ===== = = == General Statistics (rev 2) ==
1 0x008 4 6 Lifetime Power-On Resets
1 0x010 4 423 Power-on Hours
1 0x018 6 2044877667 Logical Sectors Written
1 0x020 6 2397939 Number of Write Commands
1 0x028 6 1961443492 Logical Sectors Read
1 0x030 6 9792433 Number of Read Commands
3 ===== = = == Rotating Media Statistics (rev 1) ==
3 0x008 4 2800 Spindle Motor Power-on Hours
3 0x010 4 1582 Head Flying Hours
3 0x018 4 8924 Head Load Events
3 0x020 4 200~ Number of Reallocated Logical Sectors
3 0x028 4 0 Read Recovery Attempts
3 0x030 4 0 Number of Mechanical Start Failures
4 ===== = = == General Errors Statistics (rev 1) ==
4 0x008 4 1 Number of Reported Uncorrectable Errors
4 0x010 4 0 Resets Between Cmd Acceptance and Completion
5 ===== = = == Temperature Statistics (rev 1) ==
5 0x008 1 28 Current Temperature
5 0x010 1 27 Average Short Term Temperature
5 0x018 1 26 Average Long Term Temperature
5 0x020 1 36 Highest Temperature
5 0x028 1 20 Lowest Temperature
5 0x030 1 33 Highest Average Short Term Temperature
5 0x038 1 22 Lowest Average Short Term Temperature
5 0x040 1 27 Highest Average Long Term Temperature
5 0x048 1 25 Lowest Average Long Term Temperature
5 0x050 4 0 Time in Over-Temperature
5 0x058 1 60 Specified Maximum Operating Temperature
5 0x060 4 0 Time in Under-Temperature
5 0x068 1 0 Specified Minimum Operating Temperature
6 ===== = = == Transport Statistics (rev 1) ==
6 0x008 4 96 Number of Hardware Resets
6 0x010 4 45 Number of ASR Events
6 0x018 4 0 Number of Interface CRC Errors
|_ ~ normalized value
SATA Phy Event Counters (GP Log 0x11)
ID Size Value Description
0x0001 2 0 Command failed due to ICRC error
0x0002 2 0 R_ERR response for data FIS
0x0003 2 0 R_ERR response for device-to-host data FIS
0x0004 2 0 R_ERR response for host-to-device data FIS
0x0005 2 0 R_ERR response for non-data FIS
0x0006 2 0 R_ERR response for device-to-host non-data FIS
0x0007 2 0 R_ERR response for host-to-device non-data FIS
0x0008 2 0 Device-to-host non-data FIS retries
0x0009 2 8 Transition from drive PhyRdy to drive PhyNRdy
0x000a 2 14 Device-to-host register FISes sent due to a COMRESET
0x000b 2 0 CRC errors within host-to-device FIS
0x000f 2 0 R_ERR response for host-to-device data FIS, CRC
0x0012 2 0 R_ERR response for host-to-device non-data FIS, CRC
0x8000 4 24888 Vendor specific
^ permalink raw reply
* Re: Request for assistance
From: Wols Lists @ 2016-07-06 21:31 UTC (permalink / raw)
To: o1bigtenor; +Cc: Adam Goryachev, Linux-RAID
In-Reply-To: <CAPpdf5-bdvWwp3pSTFgoL++avt-8=3VvzL-Psk2qR7rHRZUBtQ@mail.gmail.com>
On 06/07/16 19:28, o1bigtenor wrote:
> SCT Error Recovery Control:
> Read: 70 (7.0 seconds)
> Write: 70 (7.0 seconds)
As soon as you said WD Red, that said the drives are good. The SCT says
the drives will wait at most 7 seconds before returning a problem, so
that's what you want (My Barracudas can't do that - a problem waiting to
happen).
I'll let someone who knows more comment on the rest of the output, but
that SCT stuff tells us your problem is not the usual one of someone
using the wrong drives.
Cheers,
Wol
^ permalink raw reply
* Re: Request for assistance
From: Brad Campbell @ 2016-07-07 2:05 UTC (permalink / raw)
To: o1bigtenor; +Cc: Linux-RAID
In-Reply-To: <CAPpdf5-bdvWwp3pSTFgoL++avt-8=3VvzL-Psk2qR7rHRZUBtQ@mail.gmail.com>
On 07/07/16 02:28, o1bigtenor wrote:
> My best understanding as to the why is - - dirty power - - - fixing that means
> going off-grid. Expensive and not happening any time soon although I would
> really like that.
>
Get a UPS.
Get a UPS.
Get a UPS.
Get a UPS.
I've got some nice full on-line double conversion units, but they are
noisy and less efficient. In my experience, a second hand APC SmartUPS
will sort enough of the most revolting power to keep things running
smoothly, and they are CHEAP. Despite owning several expensive UPS
units, all my stuff is behind a couple of second hand SmartUPS.
My last purchase saw me pick up 5 decent line interactive UPS units for
about $25 each as a job lot. New batteries for one were less than $100
(same brand as the UPS comes with) from the local wholesaler. I get 4-5
years out of a set of batteries.
If you had to budget for time, one blip on a RAID and the associated
recovery pays for the UPS. Cheap insurance.
Regards,
Brad
^ permalink raw reply
* Re: Request for assistance
From: o1bigtenor @ 2016-07-07 3:28 UTC (permalink / raw)
To: Brad Campbell; +Cc: Linux-RAID
In-Reply-To: <1ec7a4d3-a214-942a-4bae-2d57f3167202@fnarfbargle.com>
On Wed, Jul 6, 2016 at 9:05 PM, Brad Campbell <lists2009@fnarfbargle.com> wrote:
> On 07/07/16 02:28, o1bigtenor wrote:
>
>> My best understanding as to the why is - - dirty power - - - fixing that
>> means
>> going off-grid. Expensive and not happening any time soon although I would
>> really like that.
>>
>
> Get a UPS.
> Get a UPS.
> Get a UPS.
> Get a UPS.
Hmmmmmmmmmmm - - - got one. Working on getting a bigger one setup as
maybe the first one isn't big enough. Have also found out that voltage
spikes destroy surge protectors and not necessarily all at once - - that
they die with each 'use'. It is frustrating to have such 'dirty' power. Even
better is that the CSA standards for voltage are so sloppy that electronics
die early (and often) when you are in rural country.
>
> I've got some nice full on-line double conversion units, but they are noisy
> and less efficient. In my experience, a second hand APC SmartUPS will sort
> enough of the most revolting power to keep things running smoothly, and they
> are CHEAP. Despite owning several expensive UPS units, all my stuff is
> behind a couple of second hand SmartUPS.
>
> My last purchase saw me pick up 5 decent line interactive UPS units for
> about $25 each as a job lot. New batteries for one were less than $100 (same
> brand as the UPS comes with) from the local wholesaler. I get 4-5 years out
> of a set of batteries.
>
> If you had to budget for time, one blip on a RAID and the associated
> recovery pays for the UPS. Cheap insurance.
>
Working on more. Haven't found too many of those 'reasonable' upses
though.
Regards
Dee
^ permalink raw reply
* Re: [dm-devel] [RFC] block: fix blk_queue_split() resource exhaustion
From: NeilBrown @ 2016-07-07 5:35 UTC (permalink / raw)
To: linux-block
Cc: Jens Axboe, linux-raid, linux-kernel, Martin K. Petersen,
Mike Snitzer, Peter Zijlstra, Jiri Kosina, Ming Lei, linux-bcache,
Zheng Liu, Keith Busch, Takashi Iwai, dm-devel, Ingo Molnar,
Kirill A. Shutemov, Lars Ellenberg, Shaohua Li, Kent Overstreet,
Alasdair Kergon, Roland Kammerer
In-Reply-To: <1466583730-28595-1-git-send-email-lars.ellenberg@linbit.com>
[-- Attachment #1: Type: text/plain, Size: 3959 bytes --]
On Wed, Jun 22 2016, Lars Ellenberg wrote:
> For a long time, generic_make_request() converts recursion into
> iteration by queuing recursive arguments on current->bio_list.
>
> This is convenient for stacking drivers,
> the top-most driver would take the originally submitted bio,
> and re-submit a re-mapped version of it, or one or more clones,
> or one or more new allocated bios to its backend(s). Which
> are then simply processed in turn, and each can again queue
> more "backend-bios" until we reach the bottom of the driver stack,
> and actually dispatch to the real backend device.
>
> Any stacking driver ->make_request_fn() could expect that,
> once it returns, any backend-bios it submitted via recursive calls
> to generic_make_request() would now be processed and dispatched, before
> the current task would call into this driver again.
>
> This is changed by commit
> 54efd50 block: make generic_make_request handle arbitrarily sized bios
>
> Drivers may call blk_queue_split() inside their ->make_request_fn(),
> which may split the current bio into a front-part to be dealt with
> immediately, and a remainder-part, which may need to be split even
> further. That remainder-part will simply also be pushed to
> current->bio_list, and would end up being head-of-queue, in front
> of any backend-bios the current make_request_fn() might submit during
> processing of the fron-part.
>
> Which means the current task would immediately end up back in the same
> make_request_fn() of the same driver again, before any of its backend
> bios have even been processed.
>
> This can lead to resource starvation deadlock.
> Drivers could avoid this by learning to not need blk_queue_split(),
> or by submitting their backend bios in a different context (dedicated
> kernel thread, work_queue context, ...). Or by playing funny re-ordering
> games with entries on current->bio_list.
>
> Instead, I suggest to distinguish between recursive calls to
> generic_make_request(), and pushing back the remainder part in
> blk_queue_split(), by pointing current->bio_lists to a
> struct recursion_to_iteration_bio_lists {
> struct bio_list recursion;
> struct bio_list remainder;
> }
>
> To have all bios targeted to drivers lower in the stack processed before
> processing the next piece of a bio targeted at the higher levels,
> as long as queued bios resulting from recursion are available,
> they will continue to be processed in FIFO order.
> Pushed back bio-parts resulting from blk_queue_split() will be processed
> in LIFO order, one-by-one, whenever the recursion list becomes empty.
I really like this change. It seems to precisely address the problem.
The "problem" being that requests for "this" device are potentially
mixed up with requests from underlying devices.
However I'm not sure it is quite general enough.
The "remainder" list is a stack of requests aimed at "this" level or
higher, and I think it will always exactly fit that description.
The "recursion" list needs to be a queue of requests aimed at the next
level down, and that doesn't quiet work, because once you start acting
on the first entry in that list, all the rest become "this" level.
I think you can address this by always calling ->make_request_fn with an
empty "recursion", then after the call completes, splice the "recursion"
list that resulted (if any) on top of the "remainder" stack.
This way, the "remainder" stack is always "requests for lower-level
devices before request for upper level devices" and the "recursion"
queue is always "requests for devices below the current level".
I also really *don't* like the idea of punting to a separate thread - it
seems to be just delaying the problem.
Can you try move the bio_list_init(->recursion) call to just before
the ->make_request_fn() call, and adding
bio_list_merge_head(->remainder, ->recursion)
just after?
(or something like that) and confirm it makes sense, and works?
Thanks!
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [RFC] block: fix blk_queue_split() resource exhaustion
From: Lars Ellenberg @ 2016-07-07 8:03 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, Keith Busch, Linux Kernel Mailing List,
Martin K. Petersen, Mike Snitzer, Peter Zijlstra, Jiri Kosina,
NeilBrown, Zheng Liu, linux-block, Takashi Iwai,
open list:DEVICE-MAPPER (LVM), Ingo Molnar, Kirill A. Shutemov,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT,
open list:BCACHE (BLOCK LAYER CACHE), Shaohua Li, Kent Overstreet
In-Reply-To: <CACVXFVPO1mwf9+MKosv_xY8Wz2KeShVVnefffgVr0wr=Tdo57Q@mail.gmail.com>
On Wed, Jul 06, 2016 at 11:57:51PM +0800, Ming Lei wrote:
> > ==== my suggestion
> >
> > generic_make_request(bio_orig)
> > NULL in-flight=0
> > bio_orig empty in-flight=0
> > qA->make_request_fn(bio_orig)
> > blk_queue_split()
> > result:
> > bio_s, and bio_r stuffed away to head of remainder list.
> > in-flight=1
> > bio_c = bio_clone(bio_s)
> > generic_make_request(bio_c to qB)
> > bio_c
> > <-return
> > bio_c
> > bio_list_pop()
> > empty
> > qB->make_request_fn(bio_c)
> > (Assume it does not clone, but only remap.
> > But it may also be a striping layer,
> > and queue more than one bio here.)
> > generic_make_request(bio_c to qC)
> > bio_c
> > <-return
> > bio_list_pop()
> > empty
> > qC->make_request_fn(bio_c)
> > generic_make_request(bio_c to qD)
> > bio_c
> > <-return
> > bio_list_pop()
> > empty
> > qD->make_request_fn(bio_c)
> > dispatches to hardware
> > <-return
> > empty
> > bio_list_pop()
> > NULL, great, lets pop from remainder list
> > qA->make_request_fn(bio_r) in-flight=?
> >
> > May block, but only until completion of bio_c.
> > Which may already have happened.
> >
> > *makes progress*
>
> I admit your solution is smart, but it isn't easy to prove it as correct
> in theory. But if the traversal can be mapped into pre-order traversal
> of the above binary tree, it may be correct.
What are you talking about.
There is no tree.
There is a single fifo.
And I suggest to make that one fifo, and one lifo instead.
|<------ original bio ----->|
|piece|----remainder--------|
|piece| is then processed, just as it was before,
all recursive submissions turned into iterative processing,
in the exact order they have been called recursively.
Until all deeper level submissions have been fully processed.
If deeper levels are again calling bio_queue_split, their
respective remainder are queued in front of the "top level"
remainder.
And only then, the remainders are processed,
just as if they did come in as "original bio", see above.
So if it did make progress before,
it will make progress now.
Lars
^ permalink raw reply
* Re: [dm-devel] [RFC] block: fix blk_queue_split() resource exhaustion
From: Lars Ellenberg @ 2016-07-07 8:16 UTC (permalink / raw)
To: NeilBrown
Cc: linux-block, Jens Axboe, linux-raid, linux-kernel,
Martin K. Petersen, Mike Snitzer, Peter Zijlstra, Jiri Kosina,
Ming Lei, linux-bcache, Zheng Liu, Keith Busch, Takashi Iwai,
dm-devel, Ingo Molnar, Kirill A. Shutemov, Shaohua Li,
Kent Overstreet, Alasdair Kergon, Roland Kammerer
In-Reply-To: <871t36ggcr.fsf@notabene.neil.brown.name>
On Thu, Jul 07, 2016 at 03:35:48PM +1000, NeilBrown wrote:
> On Wed, Jun 22 2016, Lars Ellenberg wrote:
>
> > For a long time, generic_make_request() converts recursion into
> > iteration by queuing recursive arguments on current->bio_list.
> >
> > This is convenient for stacking drivers,
> > the top-most driver would take the originally submitted bio,
> > and re-submit a re-mapped version of it, or one or more clones,
> > or one or more new allocated bios to its backend(s). Which
> > are then simply processed in turn, and each can again queue
> > more "backend-bios" until we reach the bottom of the driver stack,
> > and actually dispatch to the real backend device.
> >
> > Any stacking driver ->make_request_fn() could expect that,
> > once it returns, any backend-bios it submitted via recursive calls
> > to generic_make_request() would now be processed and dispatched, before
> > the current task would call into this driver again.
> >
> > This is changed by commit
> > 54efd50 block: make generic_make_request handle arbitrarily sized bios
> >
> > Drivers may call blk_queue_split() inside their ->make_request_fn(),
> > which may split the current bio into a front-part to be dealt with
> > immediately, and a remainder-part, which may need to be split even
> > further. That remainder-part will simply also be pushed to
> > current->bio_list, and would end up being head-of-queue, in front
> > of any backend-bios the current make_request_fn() might submit during
> > processing of the fron-part.
> >
> > Which means the current task would immediately end up back in the same
> > make_request_fn() of the same driver again, before any of its backend
> > bios have even been processed.
> >
> > This can lead to resource starvation deadlock.
> > Drivers could avoid this by learning to not need blk_queue_split(),
> > or by submitting their backend bios in a different context (dedicated
> > kernel thread, work_queue context, ...). Or by playing funny re-ordering
> > games with entries on current->bio_list.
> >
> > Instead, I suggest to distinguish between recursive calls to
> > generic_make_request(), and pushing back the remainder part in
> > blk_queue_split(), by pointing current->bio_lists to a
> > struct recursion_to_iteration_bio_lists {
> > struct bio_list recursion;
> > struct bio_list remainder;
> > }
> >
> > To have all bios targeted to drivers lower in the stack processed before
> > processing the next piece of a bio targeted at the higher levels,
> > as long as queued bios resulting from recursion are available,
> > they will continue to be processed in FIFO order.
> > Pushed back bio-parts resulting from blk_queue_split() will be processed
> > in LIFO order, one-by-one, whenever the recursion list becomes empty.
>
> I really like this change. It seems to precisely address the problem.
> The "problem" being that requests for "this" device are potentially
> mixed up with requests from underlying devices.
> However I'm not sure it is quite general enough.
>
> The "remainder" list is a stack of requests aimed at "this" level or
> higher, and I think it will always exactly fit that description.
> The "recursion" list needs to be a queue of requests aimed at the next
> level down, and that doesn't quiet work, because once you start acting
> on the first entry in that list, all the rest become "this" level.
Uhm, well,
that's how it has been since you introduced this back in 2007, d89d879.
And it worked.
> I think you can address this by always calling ->make_request_fn with an
> empty "recursion", then after the call completes, splice the "recursion"
> list that resulted (if any) on top of the "remainder" stack.
>
> This way, the "remainder" stack is always "requests for lower-level
> devices before request for upper level devices" and the "recursion"
> queue is always "requests for devices below the current level".
Yes, I guess that would work as well,
but may need "empirical proof" to check for performance regressions.
> I also really *don't* like the idea of punting to a separate thread - it
> seems to be just delaying the problem.
>
> Can you try move the bio_list_init(->recursion) call to just before
> the ->make_request_fn() call, and adding
> bio_list_merge_head(->remainder, ->recursion)
> just after?
> (or something like that) and confirm it makes sense, and works?
Sure, will do.
I'd suggest this would be a patch on its own though, on top of this one.
Because it would change the order in which stacked bios are processed
wrt the way it used to be since 2007 (my suggestion as is does not).
Which may change performance metrics.
It may even improve some of them,
or maybe it does nothing, but we don't know.
Thanks,
Lars
^ permalink raw reply
* The subarray is loaded container by load_container
From: Xiao Ni @ 2016-07-07 9:13 UTC (permalink / raw)
To: linux-raid
Cc: Jes Sorensen, tomasz.majchrzak, aleksey.obitotskiy,
pawel.baldysiak, artur.paszkiewicz
In-Reply-To: <1516157514.3880097.1467881964185.JavaMail.zimbra@redhat.com>
Hi all
We encountered one problem at booting time:
Run MD devices: mdadm: array /dev/md/OSVOLUME0 now has 2 devices (0 new) [FAILED]
mdadm -IRs returns 1 at the booting time. In fact the array is already running. The
command mdadm -IRs shouldn't return 1 if all the arrays are running, right?
In IncrementalScan function, it scans all the raid in /run/mdadm/map. It contains the
subarray too. The return value is 1 from function load_container. So the return value
of IncrementalScan rv is set to 1. If all arrays are running already, I think mdadm -IRs
should return 0, not 1.
Should we checks earlier whether it's a container or not before calling load_container?
Best Regards
Xiao
^ permalink raw reply
* Re: The subarray is loaded container by load_container
From: Artur Paszkiewicz @ 2016-07-07 10:16 UTC (permalink / raw)
To: Xiao Ni, linux-raid
Cc: Jes Sorensen, tomasz.majchrzak, aleksey.obitotskiy,
pawel.baldysiak
In-Reply-To: <1782686607.3885369.1467882826743.JavaMail.zimbra@redhat.com>
On 07/07/2016 11:13 AM, Xiao Ni wrote:
> Hi all
>
> We encountered one problem at booting time:
>
> Run MD devices: mdadm: array /dev/md/OSVOLUME0 now has 2 devices (0 new) [FAILED]
>
> mdadm -IRs returns 1 at the booting time. In fact the array is already running. The
> command mdadm -IRs shouldn't return 1 if all the arrays are running, right?
>
> In IncrementalScan function, it scans all the raid in /run/mdadm/map. It contains the
> subarray too. The return value is 1 from function load_container. So the return value
> of IncrementalScan rv is set to 1. If all arrays are running already, I think mdadm -IRs
> should return 0, not 1.
>
> Should we checks earlier whether it's a container or not before calling load_container?
>
> Best Regards
> Xiao
>
Hi Xiao,
I think you're right that mdadm -IRs should return 0 in this case.
IncrementalScan should not try loading a container from a member array,
because that will always fail. Can you check if this fixes the problem
for you?
diff --git a/Incremental.c b/Incremental.c
index ba97b00..cc01d41 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -1347,8 +1347,12 @@ restart:
if (devnm && strcmp(devnm, me->devnm) != 0)
continue;
- if (devnm && me->metadata[0] == '/') {
+ if (me->metadata[0] == '/') {
char *sl;
+
+ if (!devnm)
+ continue;
+
/* member array, need to work on container */
strncpy(container, me->metadata+1, 32);
container[31] = 0;
Thanks,
Artur
^ permalink raw reply related
* Re: [dm-devel] [RFC] block: fix blk_queue_split() resource exhaustion
From: Lars Ellenberg @ 2016-07-07 12:39 UTC (permalink / raw)
To: NeilBrown
Cc: linux-block, Jens Axboe, linux-raid, linux-kernel,
Martin K. Petersen, Mike Snitzer, Peter Zijlstra, Jiri Kosina,
Ming Lei, linux-bcache, Zheng Liu, Keith Busch, Takashi Iwai,
dm-devel, Ingo Molnar, Kirill A. Shutemov, Shaohua Li,
Kent Overstreet, Alasdair Kergon, Roland Kammerer
In-Reply-To: <20160707081616.GH13335@soda.linbit>
[-- Attachment #1: Type: text/plain, Size: 2969 bytes --]
On Thu, Jul 07, 2016 at 10:16:16AM +0200, Lars Ellenberg wrote:
> > > Instead, I suggest to distinguish between recursive calls to
> > > generic_make_request(), and pushing back the remainder part in
> > > blk_queue_split(), by pointing current->bio_lists to a
> > > struct recursion_to_iteration_bio_lists {
> > > struct bio_list recursion;
> > > struct bio_list remainder;
> > > }
> > >
> > > To have all bios targeted to drivers lower in the stack processed before
> > > processing the next piece of a bio targeted at the higher levels,
> > > as long as queued bios resulting from recursion are available,
> > > they will continue to be processed in FIFO order.
> > > Pushed back bio-parts resulting from blk_queue_split() will be processed
> > > in LIFO order, one-by-one, whenever the recursion list becomes empty.
> >
> > I really like this change. It seems to precisely address the problem.
> > The "problem" being that requests for "this" device are potentially
> > mixed up with requests from underlying devices.
> > However I'm not sure it is quite general enough.
> >
> > The "remainder" list is a stack of requests aimed at "this" level or
> > higher, and I think it will always exactly fit that description.
> > The "recursion" list needs to be a queue of requests aimed at the next
> > level down, and that doesn't quiet work, because once you start acting
> > on the first entry in that list, all the rest become "this" level.
>
> Uhm, well,
> that's how it has been since you introduced this back in 2007, d89d879.
> And it worked.
>
> > I think you can address this by always calling ->make_request_fn with an
> > empty "recursion", then after the call completes, splice the "recursion"
> > list that resulted (if any) on top of the "remainder" stack.
> >
> > This way, the "remainder" stack is always "requests for lower-level
> > devices before request for upper level devices" and the "recursion"
> > queue is always "requests for devices below the current level".
>
> Yes, I guess that would work as well,
> but may need "empirical proof" to check for performance regressions.
>
> > I also really *don't* like the idea of punting to a separate thread - it
> > seems to be just delaying the problem.
> >
> > Can you try move the bio_list_init(->recursion) call to just before
> > the ->make_request_fn() call, and adding
> > bio_list_merge_head(->remainder, ->recursion)
> > just after?
> > (or something like that) and confirm it makes sense, and works?
>
> Sure, will do.
Attached,
on top of the patch of my initial post.
Also fixes the issue for me.
> I'd suggest this would be a patch on its own though, on top of this one.
> Because it would change the order in which stacked bios are processed
> wrt the way it used to be since 2007 (my suggestion as is does not).
>
> Which may change performance metrics.
> It may even improve some of them,
> or maybe it does nothing, but we don't know.
>
> Thanks,
>
> Lars
>
[-- Attachment #2: 0001-block-generic_make_request-recursive-bios-process-de.patch --]
[-- Type: text/x-diff, Size: 5459 bytes --]
From 73254eae63786aca0af10e42e5b41465c90d8da8 Mon Sep 17 00:00:00 2001
From: Lars Ellenberg <lars.ellenberg@linbit.com>
Date: Thu, 7 Jul 2016 11:03:30 +0200
Subject: [PATCH] block: generic_make_request() recursive bios: process deepest
levels first
By providing each q->make_request_fn() with an empty "recursion"
bio_list, then merging any recursively submitted bios to the
head of the "remainder" list, we can make the recursion-to-iteration
logic in generic_make_request() process deepest level bios first.
---
As suggested by Neil Brown while discussing
[RFC] block: fix blk_queue_split() resource exhaustion
https://lkml.org/lkml/2016/7/7/27
Stack: qA -> qB -> qC -> qD
=== Without this patch:
generic_make_request(bio_orig to qA)
recursion: empty, remainder: empty
qA->make_request_fn(bio_orig)
potential call to bio_queue_split()
result: bio_S, bio_R
recursion: empty, remainder: bio_R
bio_S
generic_make_request(bio_S to qB)
recursion: bio_S, remainder: bio_R
<- return
pop: recursion: empty, remainder: bio_R
qB->make_request_fn(bio_S)
remap, maybe many clones because of striping
generic_make_request(clones to qC)
recursion: bio_C1, bio_C2, bio_C3
remainder: bio_R
<- return
pop: recursion: bio_C2, bio_C3,
remainder: bio_R
qC->make_request_fn(bio_C1)
remap, ...
generic_make_request(clones to qD)
recursion: bio_C2, bio_C3, bio_D1_1, bio_D1_2
remainder: bio_R
<- return
pop: recursion: bio_C3, bio_D1_1, bio_D1_2
remainder: bio_R
qC->make_request_fn(bio_C2)
recursion: bio_C3, bio_D1_1, bio_D1_2, bio_D2_1, bio_D2_2
remainder: bio_R
<- return
pop: recursion: bio_D1_1, bio_D1_2, bio_D2_1, bio_D2_2
remainder: bio_R
qC->make_request_fn(bio_C3)
...
=== With this patch:
generic_make_request(bio_orig to qA)
recursion: empty, remainder: empty
qA->make_request_fn(bio_orig)
potential call to bio_queue_split()
result: bio_S, bio_R
recursion: empty, remainder: bio_R
bio_S
generic_make_request(bio_S to qB)
recursion: bio_S, remainder: bio_R
<- return
merge_head:
recursion: empty, remainder: bio_S, bio_R
pop: recursion: empty, remainder: bio_R
qB->make_request_fn(bio_S)
remap, maybe many clones because of striping
generic_make_request(clones to qC)
recursion: bio_C1, bio_C2, bio_C3
remainder: bio_R
<- return
merge_head:
recursion: empty
remainder: bio_C1, bio_C2, bio_C3, bio_R
pop: remainder: bio_C2, bio_C3, bio_R
qC->make_request_fn(bio_C1)
remap, ...
generic_make_request(clones to qD)
recursion: bio_D1_1, bio_D1_2
remainder: bio_C2, bio_C3, bio_R
<- return
merge_head:
recursion: empty
remainder: bio_D1_1, bio_D1_2, bio_C2, bio_C3, bio_R
pop
qC->make_request_fn(bio_D1_1)
remainder: bio_D1_2, bio_C2, bio_C3, bio_R
...
---
block/bio.c | 17 ++++++++++++++---
block/blk-core.c | 10 +++++-----
2 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/block/bio.c b/block/bio.c
index 2ffcea0..92733ce 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -366,13 +366,17 @@ static void punt_bios_to_rescuer(struct bio_set *bs)
*/
bio_list_init(&punt);
- bio_list_init(&nopunt);
+ bio_list_init(&nopunt);
while ((bio = bio_list_pop(¤t->bio_lists->recursion)))
bio_list_add(bio->bi_pool == bs ? &punt : &nopunt, bio);
-
current->bio_lists->recursion = nopunt;
+ bio_list_init(&nopunt);
+ while ((bio = bio_list_pop(¤t->bio_lists->remainder)))
+ bio_list_add(bio->bi_pool == bs ? &punt : &nopunt, bio);
+ current->bio_lists->remainder = nopunt;
+
spin_lock(&bs->rescue_lock);
bio_list_merge(&bs->rescue_list, &punt);
spin_unlock(&bs->rescue_lock);
@@ -380,6 +384,13 @@ static void punt_bios_to_rescuer(struct bio_set *bs)
queue_work(bs->rescue_workqueue, &bs->rescue_work);
}
+static bool current_has_pending_bios(void)
+{
+ return current->bio_lists &&
+ (!bio_list_empty(¤t->bio_lists->recursion) ||
+ !bio_list_empty(¤t->bio_lists->remainder));
+}
+
/**
* bio_alloc_bioset - allocate a bio for I/O
* @gfp_mask: the GFP_ mask given to the slab allocator
@@ -459,7 +470,7 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
* workqueue before we retry with the original gfp_flags.
*/
- if (current->bio_lists && !bio_list_empty(¤t->bio_lists->recursion))
+ if (current_has_pending_bios())
gfp_mask &= ~__GFP_DIRECT_RECLAIM;
p = mempool_alloc(bs->bio_pool, gfp_mask);
diff --git a/block/blk-core.c b/block/blk-core.c
index f03ff4c..675131b 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2070,22 +2070,22 @@ blk_qc_t generic_make_request(struct bio *bio)
* bio_list, and call into ->make_request() again.
*/
BUG_ON(bio->bi_next);
- bio_list_init(&bio_lists_on_stack.recursion);
bio_list_init(&bio_lists_on_stack.remainder);
current->bio_lists = &bio_lists_on_stack;
do {
struct request_queue *q = bdev_get_queue(bio->bi_bdev);
if (likely(blk_queue_enter(q, false) == 0)) {
+ bio_list_init(&bio_lists_on_stack.recursion);
ret = q->make_request_fn(q, bio);
-
blk_queue_exit(q);
+ bio_list_merge_head(&bio_lists_on_stack.remainder,
+ &bio_lists_on_stack.recursion);
+ /* XXX bio_list_init(&bio_lists_on_stack.recursion); */
} else {
bio_io_error(bio);
}
- bio = bio_list_pop(¤t->bio_lists->recursion);
- if (!bio)
- bio = bio_list_pop(¤t->bio_lists->remainder);
+ bio = bio_list_pop(¤t->bio_lists->remainder);
} while (bio);
current->bio_lists = NULL; /* deactivate */
--
1.9.1
^ permalink raw reply related
* Re: block: fix blk_queue_split() resource exhaustion
From: Mike Snitzer @ 2016-07-07 12:45 UTC (permalink / raw)
To: NeilBrown
Cc: Lars Ellenberg, linux-block, Jens Axboe, linux-raid, linux-kernel,
Martin K. Petersen, Peter Zijlstra, Jiri Kosina, Ming Lei,
linux-bcache, Zheng Liu, Keith Busch, Takashi Iwai, dm-devel,
Ingo Molnar, Kirill A. Shutemov, Shaohua Li, Kent Overstreet,
Alasdair Kergon, Roland Kammerer
In-Reply-To: <871t36ggcr.fsf@notabene.neil.brown.name>
On Thu, Jul 07 2016 at 1:35am -0400,
NeilBrown <neilb@suse.com> wrote:
> On Wed, Jun 22 2016, Lars Ellenberg wrote:
>
> > For a long time, generic_make_request() converts recursion into
> > iteration by queuing recursive arguments on current->bio_list.
> >
> > This is convenient for stacking drivers,
> > the top-most driver would take the originally submitted bio,
> > and re-submit a re-mapped version of it, or one or more clones,
> > or one or more new allocated bios to its backend(s). Which
> > are then simply processed in turn, and each can again queue
> > more "backend-bios" until we reach the bottom of the driver stack,
> > and actually dispatch to the real backend device.
> >
> > Any stacking driver ->make_request_fn() could expect that,
> > once it returns, any backend-bios it submitted via recursive calls
> > to generic_make_request() would now be processed and dispatched, before
> > the current task would call into this driver again.
> >
> > This is changed by commit
> > 54efd50 block: make generic_make_request handle arbitrarily sized bios
> >
> > Drivers may call blk_queue_split() inside their ->make_request_fn(),
> > which may split the current bio into a front-part to be dealt with
> > immediately, and a remainder-part, which may need to be split even
> > further. That remainder-part will simply also be pushed to
> > current->bio_list, and would end up being head-of-queue, in front
> > of any backend-bios the current make_request_fn() might submit during
> > processing of the fron-part.
> >
> > Which means the current task would immediately end up back in the same
> > make_request_fn() of the same driver again, before any of its backend
> > bios have even been processed.
> >
> > This can lead to resource starvation deadlock.
> > Drivers could avoid this by learning to not need blk_queue_split(),
> > or by submitting their backend bios in a different context (dedicated
> > kernel thread, work_queue context, ...). Or by playing funny re-ordering
> > games with entries on current->bio_list.
> >
> > Instead, I suggest to distinguish between recursive calls to
> > generic_make_request(), and pushing back the remainder part in
> > blk_queue_split(), by pointing current->bio_lists to a
> > struct recursion_to_iteration_bio_lists {
> > struct bio_list recursion;
> > struct bio_list remainder;
> > }
> >
> > To have all bios targeted to drivers lower in the stack processed before
> > processing the next piece of a bio targeted at the higher levels,
> > as long as queued bios resulting from recursion are available,
> > they will continue to be processed in FIFO order.
> > Pushed back bio-parts resulting from blk_queue_split() will be processed
> > in LIFO order, one-by-one, whenever the recursion list becomes empty.
>
> I really like this change. It seems to precisely address the problem.
> The "problem" being that requests for "this" device are potentially
> mixed up with requests from underlying devices.
> However I'm not sure it is quite general enough.
>
> The "remainder" list is a stack of requests aimed at "this" level or
> higher, and I think it will always exactly fit that description.
> The "recursion" list needs to be a queue of requests aimed at the next
> level down, and that doesn't quiet work, because once you start acting
> on the first entry in that list, all the rest become "this" level.
>
> I think you can address this by always calling ->make_request_fn with an
> empty "recursion", then after the call completes, splice the "recursion"
> list that resulted (if any) on top of the "remainder" stack.
>
> This way, the "remainder" stack is always "requests for lower-level
> devices before request for upper level devices" and the "recursion"
> queue is always "requests for devices below the current level".
>
> I also really *don't* like the idea of punting to a separate thread
Hi Neil,
Was this concern about "punting to a separate thread" in reference to
the line of work from Mikulas at the top of this 'wip' branch?
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=wip
> - it seems to be just delaying the problem.
Have you looked at this closely? Not seeing how you can say that given
that on schedule the bios on current->bio_list are flushed.
The incremental work to delay the offload of queued bios is just meant
to preserve existing bio submission order unless there is reason to
believe a deadlock exists.
I would agree that this timer based approach is rather "gross" to some
degree _but_ it beats deadlocks! This code needs fixing. And the fix
cannot be constrained to bio_queue_split() because DM isn't even using
it.
Mike
^ permalink raw reply
* Re: block: fix blk_queue_split() resource exhaustion
From: Mike Snitzer @ 2016-07-07 12:47 UTC (permalink / raw)
To: Lars Ellenberg
Cc: NeilBrown, linux-block, Jens Axboe, linux-raid, linux-kernel,
Martin K. Petersen, Peter Zijlstra, Jiri Kosina, Ming Lei,
linux-bcache, Zheng Liu, Keith Busch, Takashi Iwai, dm-devel,
Ingo Molnar, Kirill A. Shutemov, Shaohua Li, Kent Overstreet,
Alasdair Kergon, Roland Kammerer
In-Reply-To: <20160707123937.GK13335@soda.linbit>
On Thu, Jul 07 2016 at 8:39am -0400,
Lars Ellenberg <lars.ellenberg@linbit.com> wrote:
> On Thu, Jul 07, 2016 at 10:16:16AM +0200, Lars Ellenberg wrote:
> > > > Instead, I suggest to distinguish between recursive calls to
> > > > generic_make_request(), and pushing back the remainder part in
> > > > blk_queue_split(), by pointing current->bio_lists to a
> > > > struct recursion_to_iteration_bio_lists {
> > > > struct bio_list recursion;
> > > > struct bio_list remainder;
> > > > }
> > > >
> > > > To have all bios targeted to drivers lower in the stack processed before
> > > > processing the next piece of a bio targeted at the higher levels,
> > > > as long as queued bios resulting from recursion are available,
> > > > they will continue to be processed in FIFO order.
> > > > Pushed back bio-parts resulting from blk_queue_split() will be processed
> > > > in LIFO order, one-by-one, whenever the recursion list becomes empty.
> > >
> > > I really like this change. It seems to precisely address the problem.
> > > The "problem" being that requests for "this" device are potentially
> > > mixed up with requests from underlying devices.
> > > However I'm not sure it is quite general enough.
> > >
> > > The "remainder" list is a stack of requests aimed at "this" level or
> > > higher, and I think it will always exactly fit that description.
> > > The "recursion" list needs to be a queue of requests aimed at the next
> > > level down, and that doesn't quiet work, because once you start acting
> > > on the first entry in that list, all the rest become "this" level.
> >
> > Uhm, well,
> > that's how it has been since you introduced this back in 2007, d89d879.
> > And it worked.
> >
> > > I think you can address this by always calling ->make_request_fn with an
> > > empty "recursion", then after the call completes, splice the "recursion"
> > > list that resulted (if any) on top of the "remainder" stack.
> > >
> > > This way, the "remainder" stack is always "requests for lower-level
> > > devices before request for upper level devices" and the "recursion"
> > > queue is always "requests for devices below the current level".
> >
> > Yes, I guess that would work as well,
> > but may need "empirical proof" to check for performance regressions.
> >
> > > I also really *don't* like the idea of punting to a separate thread - it
> > > seems to be just delaying the problem.
> > >
> > > Can you try move the bio_list_init(->recursion) call to just before
> > > the ->make_request_fn() call, and adding
> > > bio_list_merge_head(->remainder, ->recursion)
> > > just after?
> > > (or something like that) and confirm it makes sense, and works?
> >
> > Sure, will do.
>
> Attached,
> on top of the patch of my initial post.
> Also fixes the issue for me.
>
> > I'd suggest this would be a patch on its own though, on top of this one.
> > Because it would change the order in which stacked bios are processed
> > wrt the way it used to be since 2007 (my suggestion as is does not).
> >
> > Which may change performance metrics.
> > It may even improve some of them,
> > or maybe it does nothing, but we don't know.
> >
> > Thanks,
> >
> > Lars
> >
> From 73254eae63786aca0af10e42e5b41465c90d8da8 Mon Sep 17 00:00:00 2001
> From: Lars Ellenberg <lars.ellenberg@linbit.com>
> Date: Thu, 7 Jul 2016 11:03:30 +0200
> Subject: [PATCH] block: generic_make_request() recursive bios: process deepest
> levels first
>
> By providing each q->make_request_fn() with an empty "recursion"
> bio_list, then merging any recursively submitted bios to the
> head of the "remainder" list, we can make the recursion-to-iteration
> logic in generic_make_request() process deepest level bios first.
>
> ---
>
> As suggested by Neil Brown while discussing
> [RFC] block: fix blk_queue_split() resource exhaustion
> https://lkml.org/lkml/2016/7/7/27
Will look closer at this today, thanks!
^ permalink raw reply
* Re: [RFC] block: fix blk_queue_split() resource exhaustion
From: Ming Lei @ 2016-07-07 13:14 UTC (permalink / raw)
To: Lars Ellenberg
Cc: linux-block, Roland Kammerer, Jens Axboe, NeilBrown,
Kent Overstreet, Shaohua Li, Alasdair Kergon, Mike Snitzer,
open list:DEVICE-MAPPER (LVM), Ingo Molnar, Peter Zijlstra,
Takashi Iwai, Jiri Kosina, Zheng Liu, Keith Busch,
Martin K. Petersen, Kirill A. Shutemov, Linux Kernel Mailing List,
open list:BCACHE (BLOCK LAYER CACHE),
open list:SOFTWARE RAID (Multiple Disks) SUPPORT
In-Reply-To: <20160707080328.GG13335@soda.linbit>
On Thu, Jul 7, 2016 at 4:03 PM, Lars Ellenberg
<lars.ellenberg@linbit.com> wrote:
> On Wed, Jul 06, 2016 at 11:57:51PM +0800, Ming Lei wrote:
>> > ==== my suggestion
>> >
>> > generic_make_request(bio_orig)
>> > NULL in-flight=0
>> > bio_orig empty in-flight=0
>> > qA->make_request_fn(bio_orig)
>> > blk_queue_split()
>> > result:
>> > bio_s, and bio_r stuffed away to head of remainder list.
>> > in-flight=1
>> > bio_c = bio_clone(bio_s)
>> > generic_make_request(bio_c to qB)
>> > bio_c
>> > <-return
>> > bio_c
>> > bio_list_pop()
>> > empty
>> > qB->make_request_fn(bio_c)
>> > (Assume it does not clone, but only remap.
>> > But it may also be a striping layer,
>> > and queue more than one bio here.)
>> > generic_make_request(bio_c to qC)
>> > bio_c
>> > <-return
>> > bio_list_pop()
>> > empty
>> > qC->make_request_fn(bio_c)
>> > generic_make_request(bio_c to qD)
>> > bio_c
>> > <-return
>> > bio_list_pop()
>> > empty
>> > qD->make_request_fn(bio_c)
>> > dispatches to hardware
>> > <-return
>> > empty
>> > bio_list_pop()
>> > NULL, great, lets pop from remainder list
>> > qA->make_request_fn(bio_r) in-flight=?
>> >
>> > May block, but only until completion of bio_c.
>> > Which may already have happened.
>> >
>> > *makes progress*
>>
>> I admit your solution is smart, but it isn't easy to prove it as correct
>> in theory. But if the traversal can be mapped into pre-order traversal
>> of the above binary tree, it may be correct.
>
> What are you talking about.
> There is no tree.
> There is a single fifo.
> And I suggest to make that one fifo, and one lifo instead.
The implementation may use fifo(queue) or lifo(stack), but the traversal
order actually is pre-order on one binary tree if we think the splitted bio and
the cloned bio as left child, and the remainder bio as right child.
The big benifit of modeling the algorithem as tree is that the implementation
can be proved easily.
Your patch is very similar with non-recursive pre-order algorithem.
And the one line change approach is just the typical pre-order
recursive algorithem on binary tree.
Let's abstract the one line change into following pseudocode:
bio_list_add_head(current->bio_list, bio);
while (!bio_list_empty(current->bio_list)) {
bio = bio_list_pop(current->bio_list);
q->make_request_fn(q, bio); //visit the node
}
q->make_request_fn(q, bio)
-> blk_queue_split(split, bio)
-> generic_make_request(remainder)
->bio_list_add_head(current->bio_list, bio);
-> push(split) & pop(split) & visit the split bio
->generic_make_request(cloned_bio)
->bio_list_add_head(current->bio_list, bio);
If you compare the above with the non-recursive pre-order traversal
implementation[1], it is basically same.
[1] http://algorithmsandme.in/2015/03/preorder-traversal-of-tree-without-recursion/
The main difference between oneline change and this patch is the submit
order in the following situation:
q->make_request_fn(bio)
->generic_make_request(cloned_bio0)
->generic_make_request(cloned_bio1)
->generic_make_request(cloned_bio2)
But the three BIOs are often submitted into different queues, so the order
may not make a big difference. Even they are submitted to same queue
(disk), the order change may not be a issue too because there is still
I/O scheduler.
And do we have stacked driver which submits several BIOs to same queue
in .make_request_fn()?
If there isn't such case, I suggest the oneline change, together with
the pre-order traversal comment since it is very simple.
Thanks,
>
> |<------ original bio ----->|
> |piece|----remainder--------|
>
> |piece| is then processed, just as it was before,
> all recursive submissions turned into iterative processing,
> in the exact order they have been called recursively.
> Until all deeper level submissions have been fully processed.
>
> If deeper levels are again calling bio_queue_split, their
> respective remainder are queued in front of the "top level"
> remainder.
>
> And only then, the remainders are processed,
> just as if they did come in as "original bio", see above.
>
> So if it did make progress before,
> it will make progress now.
>
> Lars
>
^ permalink raw reply
* Re: block: fix blk_queue_split() resource exhaustion
From: Mike Snitzer @ 2016-07-07 14:36 UTC (permalink / raw)
To: Lars Ellenberg
Cc: linux-block, Roland Kammerer, Jens Axboe, NeilBrown,
Kent Overstreet, Shaohua Li, Alasdair Kergon, dm-devel,
Ingo Molnar, Peter Zijlstra, Takashi Iwai, Jiri Kosina, Zheng Liu,
Ming Lei, Keith Busch, Martin K. Petersen, Kirill A. Shutemov,
linux-kernel, linux-bcache, linux-raid
In-Reply-To: <1466583730-28595-1-git-send-email-lars.ellenberg@linbit.com>
On Wed, Jun 22 2016 at 4:22am -0400,
Lars Ellenberg <lars.ellenberg@linbit.com> wrote:
> For a long time, generic_make_request() converts recursion into
> iteration by queuing recursive arguments on current->bio_list.
>
> This is convenient for stacking drivers,
> the top-most driver would take the originally submitted bio,
> and re-submit a re-mapped version of it, or one or more clones,
> or one or more new allocated bios to its backend(s). Which
> are then simply processed in turn, and each can again queue
> more "backend-bios" until we reach the bottom of the driver stack,
> and actually dispatch to the real backend device.
>
> Any stacking driver ->make_request_fn() could expect that,
> once it returns, any backend-bios it submitted via recursive calls
> to generic_make_request() would now be processed and dispatched, before
> the current task would call into this driver again.
>
> This is changed by commit
> 54efd50 block: make generic_make_request handle arbitrarily sized bios
>
> Drivers may call blk_queue_split() inside their ->make_request_fn(),
> which may split the current bio into a front-part to be dealt with
> immediately, and a remainder-part, which may need to be split even
> further. That remainder-part will simply also be pushed to
> current->bio_list, and would end up being head-of-queue, in front
> of any backend-bios the current make_request_fn() might submit during
> processing of the fron-part.
>
> Which means the current task would immediately end up back in the same
> make_request_fn() of the same driver again, before any of its backend
> bios have even been processed.
>
> This can lead to resource starvation deadlock.
> Drivers could avoid this by learning to not need blk_queue_split(),
> or by submitting their backend bios in a different context (dedicated
> kernel thread, work_queue context, ...). Or by playing funny re-ordering
> games with entries on current->bio_list.
>
> Instead, I suggest to distinguish between recursive calls to
> generic_make_request(), and pushing back the remainder part in
> blk_queue_split(), by pointing current->bio_lists to a
> struct recursion_to_iteration_bio_lists {
> struct bio_list recursion;
> struct bio_list remainder;
> }
>
> To have all bios targeted to drivers lower in the stack processed before
> processing the next piece of a bio targeted at the higher levels,
> as long as queued bios resulting from recursion are available,
> they will continue to be processed in FIFO order.
> Pushed back bio-parts resulting from blk_queue_split() will be processed
> in LIFO order, one-by-one, whenever the recursion list becomes empty.
>
> Signed-off-by: Lars Ellenberg <lars.ellenberg@linbit.com>
> Signed-off-by: Roland Kammerer <roland.kammerer@linbit.com>
I've rebased this patch against Jens' for-4.8/core (resolved conflict in
blk-merge.c) and pushed the result to this wip2 branch, feel free to use
it to resubmit for inclusion if/when that is the way forward, see:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=wip2&id=36cee4b1ddef0a46562045b421792a847c570b6b
^ permalink raw reply
* Re: [dm-devel] [RFC] block: fix blk_queue_split() resource exhaustion
From: NeilBrown @ 2016-07-07 22:07 UTC (permalink / raw)
To: Lars Ellenberg
Cc: linux-block, Jens Axboe, linux-raid, linux-kernel,
Martin K. Petersen, Mike Snitzer, Peter Zijlstra, Jiri Kosina,
Ming Lei, linux-bcache, Zheng Liu, Keith Busch, Takashi Iwai,
dm-devel, Ingo Molnar, Kirill A. Shutemov, Shaohua Li,
Kent Overstreet, Alasdair Kergon, Roland Kammerer
In-Reply-To: <20160707081616.GH13335@soda.linbit>
[-- Attachment #1: Type: text/plain, Size: 7065 bytes --]
On Thu, Jul 07 2016, Lars Ellenberg wrote:
> On Thu, Jul 07, 2016 at 03:35:48PM +1000, NeilBrown wrote:
>> On Wed, Jun 22 2016, Lars Ellenberg wrote:
>>
>> > For a long time, generic_make_request() converts recursion into
>> > iteration by queuing recursive arguments on current->bio_list.
>> >
>> > This is convenient for stacking drivers,
>> > the top-most driver would take the originally submitted bio,
>> > and re-submit a re-mapped version of it, or one or more clones,
>> > or one or more new allocated bios to its backend(s). Which
>> > are then simply processed in turn, and each can again queue
>> > more "backend-bios" until we reach the bottom of the driver stack,
>> > and actually dispatch to the real backend device.
>> >
>> > Any stacking driver ->make_request_fn() could expect that,
>> > once it returns, any backend-bios it submitted via recursive calls
>> > to generic_make_request() would now be processed and dispatched, before
>> > the current task would call into this driver again.
>> >
>> > This is changed by commit
>> > 54efd50 block: make generic_make_request handle arbitrarily sized bios
>> >
>> > Drivers may call blk_queue_split() inside their ->make_request_fn(),
>> > which may split the current bio into a front-part to be dealt with
>> > immediately, and a remainder-part, which may need to be split even
>> > further. That remainder-part will simply also be pushed to
>> > current->bio_list, and would end up being head-of-queue, in front
>> > of any backend-bios the current make_request_fn() might submit during
>> > processing of the fron-part.
>> >
>> > Which means the current task would immediately end up back in the same
>> > make_request_fn() of the same driver again, before any of its backend
>> > bios have even been processed.
>> >
>> > This can lead to resource starvation deadlock.
>> > Drivers could avoid this by learning to not need blk_queue_split(),
>> > or by submitting their backend bios in a different context (dedicated
>> > kernel thread, work_queue context, ...). Or by playing funny re-ordering
>> > games with entries on current->bio_list.
>> >
>> > Instead, I suggest to distinguish between recursive calls to
>> > generic_make_request(), and pushing back the remainder part in
>> > blk_queue_split(), by pointing current->bio_lists to a
>> > struct recursion_to_iteration_bio_lists {
>> > struct bio_list recursion;
>> > struct bio_list remainder;
>> > }
>> >
>> > To have all bios targeted to drivers lower in the stack processed before
>> > processing the next piece of a bio targeted at the higher levels,
>> > as long as queued bios resulting from recursion are available,
>> > they will continue to be processed in FIFO order.
>> > Pushed back bio-parts resulting from blk_queue_split() will be processed
>> > in LIFO order, one-by-one, whenever the recursion list becomes empty.
>>
>> I really like this change. It seems to precisely address the problem.
>> The "problem" being that requests for "this" device are potentially
>> mixed up with requests from underlying devices.
>> However I'm not sure it is quite general enough.
>>
>> The "remainder" list is a stack of requests aimed at "this" level or
>> higher, and I think it will always exactly fit that description.
>> The "recursion" list needs to be a queue of requests aimed at the next
>> level down, and that doesn't quiet work, because once you start acting
>> on the first entry in that list, all the rest become "this" level.
>
> Uhm, well,
> that's how it has been since you introduced this back in 2007, d89d879.
> And it worked.
Just because it "worked", doesn't mean it was "right" :-)
>
>> I think you can address this by always calling ->make_request_fn with an
>> empty "recursion", then after the call completes, splice the "recursion"
>> list that resulted (if any) on top of the "remainder" stack.
>>
>> This way, the "remainder" stack is always "requests for lower-level
>> devices before request for upper level devices" and the "recursion"
>> queue is always "requests for devices below the current level".
>
> Yes, I guess that would work as well,
> but may need "empirical proof" to check for performance regressions.
>
>> I also really *don't* like the idea of punting to a separate thread - it
>> seems to be just delaying the problem.
>>
>> Can you try move the bio_list_init(->recursion) call to just before
>> the ->make_request_fn() call, and adding
>> bio_list_merge_head(->remainder, ->recursion)
>> just after?
>> (or something like that) and confirm it makes sense, and works?
>
> Sure, will do.
> I'd suggest this would be a patch on its own though, on top of this one.
> Because it would change the order in which stacked bios are processed
> wrt the way it used to be since 2007 (my suggestion as is does not).
Yes, because I think the original order is wrong, as you have helped me
to see.
Before I introduced the recursion limiting, requests were handled as an
in-order tree walk. The code I wrote tried to preserve that but didn't
for several reasons. I think we need to restore the original in-order
walk because it makes the most sense.
So after processing a particular bio, we should then process all the
'child' bios - bios send to underlying devices. Then the 'sibling'
bios, that were split off, and then any remaining parents and ancestors.
You patch created the right structures for doing this, my proposal took
it a step closer, but now after more careful analysis I don't think it
is quite right.
With my previous proposal (and you latest patch - thanks!) requests for
"this" level are stacked, but they should be queued.
If a make_request_fn only ever submits one request for this level and
zero or more lower levels, then the difference between a queue and a
stack is irrelevant. If it submited more that one, a stack would cause
them to be handled in the reverse order.
To make the patch "perfect", and maybe even more elegant we could treat
->remainder and ->recursion more alike.
i.e.:
- generic make request has a private "stack" of requests.
- before calling ->make_request_fn(), both ->remainder and ->recursion
are initialised
- after ->make_request_fn(), ->remainder are spliced in to top of
'stack', then ->recursion is spliced onto that.
- If stack is not empty, the top request is popped and we loop to top.
This reliably follows in-order execution, and handles siblings correctly
(in submitted order) if/when a request splits off multiple siblings.
>
> Which may change performance metrics.
> It may even improve some of them,
> or maybe it does nothing, but we don't know.
I think that as long a requests are submitted in the order they are
created at each level there is no reason to expect performance
regressions.
All we are doing is changing the ordering between requests generated at
different levels, and I think we are restoring a more natural order.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: block: fix blk_queue_split() resource exhaustion
From: NeilBrown @ 2016-07-07 22:40 UTC (permalink / raw)
To: Mike Snitzer
Cc: Lars Ellenberg, linux-block, Jens Axboe, linux-raid, linux-kernel,
Martin K. Petersen, Peter Zijlstra, Jiri Kosina, Ming Lei,
linux-bcache, Zheng Liu, Keith Busch, Takashi Iwai, dm-devel,
Ingo Molnar, Kirill A. Shutemov, Shaohua Li, Kent Overstreet,
Alasdair Kergon, Roland Kammerer
In-Reply-To: <20160707124505.GB2737@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 6787 bytes --]
On Thu, Jul 07 2016, Mike Snitzer wrote:
> On Thu, Jul 07 2016 at 1:35am -0400,
> NeilBrown <neilb@suse.com> wrote:
>
>> On Wed, Jun 22 2016, Lars Ellenberg wrote:
>>
>> > For a long time, generic_make_request() converts recursion into
>> > iteration by queuing recursive arguments on current->bio_list.
>> >
>> > This is convenient for stacking drivers,
>> > the top-most driver would take the originally submitted bio,
>> > and re-submit a re-mapped version of it, or one or more clones,
>> > or one or more new allocated bios to its backend(s). Which
>> > are then simply processed in turn, and each can again queue
>> > more "backend-bios" until we reach the bottom of the driver stack,
>> > and actually dispatch to the real backend device.
>> >
>> > Any stacking driver ->make_request_fn() could expect that,
>> > once it returns, any backend-bios it submitted via recursive calls
>> > to generic_make_request() would now be processed and dispatched, before
>> > the current task would call into this driver again.
>> >
>> > This is changed by commit
>> > 54efd50 block: make generic_make_request handle arbitrarily sized bios
>> >
>> > Drivers may call blk_queue_split() inside their ->make_request_fn(),
>> > which may split the current bio into a front-part to be dealt with
>> > immediately, and a remainder-part, which may need to be split even
>> > further. That remainder-part will simply also be pushed to
>> > current->bio_list, and would end up being head-of-queue, in front
>> > of any backend-bios the current make_request_fn() might submit during
>> > processing of the fron-part.
>> >
>> > Which means the current task would immediately end up back in the same
>> > make_request_fn() of the same driver again, before any of its backend
>> > bios have even been processed.
>> >
>> > This can lead to resource starvation deadlock.
>> > Drivers could avoid this by learning to not need blk_queue_split(),
>> > or by submitting their backend bios in a different context (dedicated
>> > kernel thread, work_queue context, ...). Or by playing funny re-ordering
>> > games with entries on current->bio_list.
>> >
>> > Instead, I suggest to distinguish between recursive calls to
>> > generic_make_request(), and pushing back the remainder part in
>> > blk_queue_split(), by pointing current->bio_lists to a
>> > struct recursion_to_iteration_bio_lists {
>> > struct bio_list recursion;
>> > struct bio_list remainder;
>> > }
>> >
>> > To have all bios targeted to drivers lower in the stack processed before
>> > processing the next piece of a bio targeted at the higher levels,
>> > as long as queued bios resulting from recursion are available,
>> > they will continue to be processed in FIFO order.
>> > Pushed back bio-parts resulting from blk_queue_split() will be processed
>> > in LIFO order, one-by-one, whenever the recursion list becomes empty.
>>
>> I really like this change. It seems to precisely address the problem.
>> The "problem" being that requests for "this" device are potentially
>> mixed up with requests from underlying devices.
>> However I'm not sure it is quite general enough.
>>
>> The "remainder" list is a stack of requests aimed at "this" level or
>> higher, and I think it will always exactly fit that description.
>> The "recursion" list needs to be a queue of requests aimed at the next
>> level down, and that doesn't quiet work, because once you start acting
>> on the first entry in that list, all the rest become "this" level.
>>
>> I think you can address this by always calling ->make_request_fn with an
>> empty "recursion", then after the call completes, splice the "recursion"
>> list that resulted (if any) on top of the "remainder" stack.
>>
>> This way, the "remainder" stack is always "requests for lower-level
>> devices before request for upper level devices" and the "recursion"
>> queue is always "requests for devices below the current level".
>>
>> I also really *don't* like the idea of punting to a separate thread
>
> Hi Neil,
>
> Was this concern about "punting to a separate thread" in reference to
> the line of work from Mikulas at the top of this 'wip' branch?
> http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=wip
A little bit in response to
https://patchwork.kernel.org/patch/7398411/
which you linked upthread, but more in response to the per-blocked
workqueue threads we now have. The (Commit df2cb6daa4) really seemed
like a lazy solution.
I may will be that the current proposal makes these threads redundant by
completely handling the first section split of a request before looking
to see if there is any more to split.
For this to work, each split would need to be a binary split.
i.e.
If request is too big:
1/ split into A that isn't too big and B
2/ add B to the "remainder" queue
3/ generate sub requests for A and submit them to "recursion" queue
Then 'A' would be completely submitted before B was started, so it would
be safe for B to wait for any resources (like something from a mempool)
that A might be using.
>
>> - it seems to be just delaying the problem.
>
> Have you looked at this closely? Not seeing how you can say that given
> that on schedule the bios on current->bio_list are flushed.
I fully accept that it is similar to a current solution. I just don't
like either.
In the sequence of steps given in
https://patchwork.kernel.org/patch/7398411/
step 6 would now not happen until after the bio mentioned in step 4 has
been completely submitted, and so after step5 has had a chance to
release the lock.
>
> The incremental work to delay the offload of queued bios is just meant
> to preserve existing bio submission order unless there is reason to
> believe a deadlock exists.
>
> I would agree that this timer based approach is rather "gross" to some
> degree _but_ it beats deadlocks! This code needs fixing. And the fix
> cannot be constrained to bio_queue_split() because DM isn't even using
> it.
Yes, the 1-second timer isn't the most elegant thing ever. Certainly
better than a deadlock. But I think we can do better.
Certainly DM needs fixing, as well as md (and maybe others). The fix
should be fairly easy though.
For md/raid0, the loop in raid0_make_request would be discarded
and the
} while (split != bio);
at the end would be replaced by
if (split != bio)
generic_queue_request(bio);
though probably with a better function name.
generic_queue_request() would add the bio to the ->remainder list.
Presumably DM could do something similar, but I'm not familiar enough
with the code to say precisely what.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [PATCH RFC] md/raid1: fix deadlock between freeze_array() and wait_barrier().
From: NeilBrown @ 2016-07-07 23:41 UTC (permalink / raw)
To: Alexander Lyakas, 马建朋, linux-raid; +Cc: Jes Sorensen
In-Reply-To: <CAGRgLy6RcG0_DYbd89LK0fWSJkMgqmDYn8ug7pYn+4uRqDRGqg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]
On Mon, Jun 27 2016, Alexander Lyakas wrote:
> When we call wait_barrier, we might have some bios waiting
> in current->bio_list, which prevents the array_freeze call to
> complete. Those can only be internal READs, which have already
> passed the wait_barrier call (thus incrementing nr_pending), but
> still were not submitted to the lower level, due to generic_make_request
> logic to avoid recursive calls. In such case, we have a deadlock:
> - array_frozen is already set to 1, so wait_barrier unconditionally waits, so
> - internal READ bios will not be submitted, thus freeze_array will
> never completes
>
> This problem was originally fixed in commit:
> d6b42dc md/raid1,raid10: avoid deadlock during resync/recovery.
>
> But then it was broken in commit:
> b364e3d raid1: Add a field array_frozen to indicate whether raid in
> freeze state.
Thanks for the great analysis.
I think this primarily a problem in generic_make_request(). It queues
requests in the *wrong* order.
Please try the patch from
https://lkml.org/lkml/2016/7/7/428
and see if it helps. If two requests for a raid1 are in the
generic_make_request queue, this patch causes the sub-requests created
by the first to be handled before the second is attempted.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* re: dm: raid456 basic support
From: NeilBrown @ 2016-07-07 23:57 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-raid
In-Reply-To: <20160617091405.GA25609@mwanda>
[-- Attachment #1: Type: text/plain, Size: 2246 bytes --]
On Fri, Jun 17 2016, Dan Carpenter wrote:
> [ No idea why it's only just now complaining about issues from 2011... ]
>
> Hello NeilBrown,
Hello ... sorry for the delay in replying.
>
> The patch 9d09e663d550: "dm: raid456 basic support" from Jan 13,
> 2011, leads to the following static checker warning:
>
> drivers/md/dm-raid.c:1217 parse_raid_params()
> warn: no lower bound on 'value'
>
> drivers/md/dm-raid.c
> 1211 return -EINVAL;
> 1212 }
> 1213 if (!value || (value > MAX_SCHEDULE_TIMEOUT)) {
>
> value is an int. MAX_SCHEDULE_TIMEOUT is LONG_MAX. Should it be
> INT_MAX? What about negatives?
% $ git show 9d09e663d550 | grep 'value;' | head -n1
+ unsigned long value;
I think value is unsigned long.
It is set on two occasions with:
strict_strtoul(argv[0], 10, &value)
and we bail out if that fails.
The first time we assign it to an int ({new_,}chunk_sectors) without
range checking, which is bad.
We cast it to an int for calling raid5_set_cache_size() without first
range checking, which is bad.
Might either of these be the cause of the rather peculiar warning?
The following patch (against mainline) should fix those issues.
Do they silence your warning?
Thanks,
NeilBrown
diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
index 52532745a50f..670d237a26a9 100644
--- a/drivers/md/dm-raid.c
+++ b/drivers/md/dm-raid.c
@@ -520,6 +520,9 @@ static int parse_raid_params(struct raid_set *rs, char **argv,
} else if (value < 8) {
rs->ti->error = "Chunk size value is too small";
return -EINVAL;
+ } else if (value > INT_MAX) {
+ rs->ti->error = "Chunk size value is too large";
+ return -EINVAL;
}
rs->md.new_chunk_sectors = rs->md.chunk_sectors = value;
@@ -650,7 +653,8 @@ static int parse_raid_params(struct raid_set *rs, char **argv,
rs->ti->error = "Inappropriate argument: stripe_cache";
return -EINVAL;
}
- if (raid5_set_cache_size(&rs->md, (int)value)) {
+ if (value > INT_MAX ||
+ raid5_set_cache_size(&rs->md, (int)value)) {
rs->ti->error = "Bad stripe_cache size";
return -EINVAL;
}
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply related
* Re: [dm-devel] [RFC] block: fix blk_queue_split() resource exhaustion
From: Lars Ellenberg @ 2016-07-08 8:02 UTC (permalink / raw)
To: NeilBrown
Cc: linux-block, Jens Axboe, linux-raid, linux-kernel,
Martin K. Petersen, Mike Snitzer, Peter Zijlstra, Jiri Kosina,
Ming Lei, linux-bcache, Zheng Liu, Keith Busch, Takashi Iwai,
dm-devel, Ingo Molnar, Kirill A. Shutemov, Shaohua Li,
Kent Overstreet, Alasdair Kergon, Roland Kammerer
In-Reply-To: <87vb0hf6fb.fsf@notabene.neil.brown.name>
On Fri, Jul 08, 2016 at 08:07:52AM +1000, NeilBrown wrote:
> Before I introduced the recursion limiting, requests were handled as an
> in-order tree walk. The code I wrote tried to preserve that but didn't
> for several reasons. I think we need to restore the original in-order
> walk because it makes the most sense.
> So after processing a particular bio, we should then process all the
> 'child' bios - bios send to underlying devices. Then the 'sibling'
> bios, that were split off, and then any remaining parents and ancestors.
>
> You patch created the right structures for doing this, my proposal took
> it a step closer, but now after more careful analysis I don't think it
> is quite right.
> With my previous proposal (and you latest patch - thanks!) requests for
> "this" level are stacked, but they should be queued.
> If a make_request_fn only ever submits one request for this level and
> zero or more lower levels, then the difference between a queue and a
> stack is irrelevant. If it submited more that one, a stack would cause
> them to be handled in the reverse order.
We have a device stack.
q_this_level->make_request_fn() cannot possibly submit anything
on "this_level", or it would create a device loop, I think.
So we start with the initial, "top most" call to generic_make_request().
That is one single bio. All queues are empty.
This bio is then passed on to its destination queue make_request_fn().
Which may chose to split it (via blk_queue_split, or like dm does, or
else). If it, like blk_queue_split() does, splits it into
"piece-I-can-handle-now" and "remainder", both still targeted at the
top most (current) queue, I think the "remainder" should just be pushed
back, which will make it look as if upper layers did
generic_make_request("piece-I-can-handle-now");
generic_make_request("remainder");
Which I do, by using bio_list_add_head(remainder, bio); (*head*).
I don't see any other way for a make_request_fn(bio(l=x)) to generate
"sibling" bios to the same level (l=x) as its own argument.
This same q(l=x)->make_request_fn(bio(l=x)) may now call
generic_make_request() for zero or more "child" bios (l=x+1),
which are queued in order: bio_list_add(recursion, bio); (*tail*).
Then, once l=x returns, the queue generated by it is spliced
in front of the "remainder" (*head*).
All bios are processed in the order they have been queued,
by peeling off of the head.
After all "child" bios of level l>=x+1 have been processed,
the next bio to be processed will be the "pushed back" remainder.
All "Natural order".
> To make the patch "perfect", and maybe even more elegant we could treat
> ->remainder and ->recursion more alike.
> i.e.:
> - generic make request has a private "stack" of requests.
> - before calling ->make_request_fn(), both ->remainder and ->recursion
> are initialised
> - after ->make_request_fn(), ->remainder are spliced in to top of
> 'stack', then ->recursion is spliced onto that.
> - If stack is not empty, the top request is popped and we loop to top.
>
> This reliably follows in-order execution, and handles siblings correctly
> (in submitted order) if/when a request splits off multiple siblings.
The only splitting that creates siblings on the current level
is blk_queue_split(), which splits the current bio into
"front piece" and "remainder", already processed in this order.
Anything else creating "siblings" is not creating siblings for the
current layer, but for the next deeper layer, which are queue on
"recursion" and also processed in the order they have been generated.
> I think that as long a requests are submitted in the order they are
> created at each level there is no reason to expect performance
> regressions.
> All we are doing is changing the ordering between requests generated at
> different levels, and I think we are restoring a more natural order.
I believe both patches combined are doing exactly this already.
I could rename .remainder to .todo or .incoming, though.
.incoming = [ bio(l=0) ]
.recursion = []
split
.incoming = [ bio(l=0,now_1), bio(l=0,remainder_1) ]
.recursion = []
process head of .incoming
.incoming = [ bio(l=0,remainder_1) ]
.recursion = [ bio(l=1,a), bio(l=1,b), bio(l=1,c), ... ]
merge_head
.incoming = [ bio(l=1,a), bio(l=1,b), bio(l=1,c), ...,
bio(l=0,remainder_1) ]
.recursion = []
process head of .incoming, potentially split first
.incoming = [ bio(l=1,a,now), bio(l=1,a,remainder), bio(l=1,b), bio(l=1,c), ...,
bio(l=0,remainder_1) ]
...
.incoming = [ bio(l=1,a,remainder), bio(l=1,b), bio(l=1,c), ...,
bio(l=0,remainder_1) ]
.recursion = [ bio(l=2,aa), bio(l=2,ab), ... ]
merge_head
.incoming = [ bio(l=2,aa), bio(l=2,ab), ...,
bio(l=1,a,remainder), bio(l=1,b), bio(l=1,c), ...,
bio(l=0,remainder_1) ]
.recursion = []
...
process away ... until back at l=0
.incoming = [ bio(l=0,remainder_1) ]
.recursion = []
potentially split further
.incoming = [ bio(l=0,now_2), bio(l=0,remainder_2) ]
.recursion = []
rinse, repeat.
Thanks,
Lars Ellenberg
^ permalink raw reply
* Re: [dm-devel] [RFC] block: fix blk_queue_split() resource exhaustion
From: NeilBrown @ 2016-07-08 9:39 UTC (permalink / raw)
To: Lars Ellenberg
Cc: Jens Axboe, linux-raid, Martin K. Petersen, Mike Snitzer,
Peter Zijlstra, Jiri Kosina, Ming Lei, linux-kernel, Zheng Liu,
linux-block, Takashi Iwai, linux-bcache, Ingo Molnar,
Alasdair Kergon, Keith Busch, dm-devel, Shaohua Li,
Kent Overstreet, Kirill A. Shutemov, Roland Kammerer
In-Reply-To: <20160708080219.GT13335@soda.linbit>
[-- Attachment #1: Type: text/plain, Size: 6287 bytes --]
On Fri, Jul 08 2016, Lars Ellenberg wrote:
> On Fri, Jul 08, 2016 at 08:07:52AM +1000, NeilBrown wrote:
>> Before I introduced the recursion limiting, requests were handled as an
>> in-order tree walk. The code I wrote tried to preserve that but didn't
>> for several reasons. I think we need to restore the original in-order
>> walk because it makes the most sense.
>> So after processing a particular bio, we should then process all the
>> 'child' bios - bios send to underlying devices. Then the 'sibling'
>> bios, that were split off, and then any remaining parents and ancestors.
>>
>> You patch created the right structures for doing this, my proposal took
>> it a step closer, but now after more careful analysis I don't think it
>> is quite right.
>> With my previous proposal (and you latest patch - thanks!) requests for
>> "this" level are stacked, but they should be queued.
>> If a make_request_fn only ever submits one request for this level and
>> zero or more lower levels, then the difference between a queue and a
>> stack is irrelevant. If it submited more that one, a stack would cause
>> them to be handled in the reverse order.
>
> We have a device stack.
> q_this_level->make_request_fn() cannot possibly submit anything
> on "this_level", or it would create a device loop, I think.
>
> So we start with the initial, "top most" call to generic_make_request().
> That is one single bio. All queues are empty.
>
> This bio is then passed on to its destination queue make_request_fn().
>
> Which may chose to split it (via blk_queue_split, or like dm does, or
> else). If it, like blk_queue_split() does, splits it into
> "piece-I-can-handle-now" and "remainder", both still targeted at the
> top most (current) queue, I think the "remainder" should just be pushed
> back, which will make it look as if upper layers did
> generic_make_request("piece-I-can-handle-now");
> generic_make_request("remainder");
> Which I do, by using bio_list_add_head(remainder, bio); (*head*).
This is exactly what I mean by "submitting a request at 'this' level".
Maybe that is a poor way to express it. Within a make_request_fn, you
cannot "submit" a request, you can only "queue" a request.
generic_make_request hides that by doing something different for a call
From a make_request_fn that for a call from anywhere else.
The important thing is to have 2 queues to queue to.
>
> I don't see any other way for a make_request_fn(bio(l=x)) to generate
> "sibling" bios to the same level (l=x) as its own argument.
Yes, it only comes from splitting.
>
> This same q(l=x)->make_request_fn(bio(l=x)) may now call
> generic_make_request() for zero or more "child" bios (l=x+1),
> which are queued in order: bio_list_add(recursion, bio); (*tail*).
> Then, once l=x returns, the queue generated by it is spliced
> in front of the "remainder" (*head*).
> All bios are processed in the order they have been queued,
> by peeling off of the head.
>
> After all "child" bios of level l>=x+1 have been processed,
> the next bio to be processed will be the "pushed back" remainder.
>
> All "Natural order".
>
>> To make the patch "perfect", and maybe even more elegant we could treat
>> ->remainder and ->recursion more alike.
>> i.e.:
>> - generic make request has a private "stack" of requests.
>> - before calling ->make_request_fn(), both ->remainder and ->recursion
>> are initialised
>> - after ->make_request_fn(), ->remainder are spliced in to top of
>> 'stack', then ->recursion is spliced onto that.
>> - If stack is not empty, the top request is popped and we loop to top.
>>
>> This reliably follows in-order execution, and handles siblings correctly
>> (in submitted order) if/when a request splits off multiple siblings.
>
> The only splitting that creates siblings on the current level
> is blk_queue_split(), which splits the current bio into
> "front piece" and "remainder", already processed in this order.
Yes.
I imagine that a driver *could* split a bio into three parts with a
single allocation, but I cannot actually see any point in doing it. So
I was over-complicating things.
>
> Anything else creating "siblings" is not creating siblings for the
> current layer, but for the next deeper layer, which are queue on
> "recursion" and also processed in the order they have been generated.
>
>> I think that as long a requests are submitted in the order they are
>> created at each level there is no reason to expect performance
>> regressions.
>> All we are doing is changing the ordering between requests generated at
>> different levels, and I think we are restoring a more natural order.
>
> I believe both patches combined are doing exactly this already.
> I could rename .remainder to .todo or .incoming, though.
:-) neither "remainder" or "recursion" seem like brilliant names to me,
but I don't have anything better to suggest. Naming is hard!
As long as a comment explains the name clearly I could cope with X and Y.
>
> .incoming = [ bio(l=0) ]
> .recursion = []
>
> split
>
> .incoming = [ bio(l=0,now_1), bio(l=0,remainder_1) ]
> .recursion = []
>
> process head of .incoming
>
> .incoming = [ bio(l=0,remainder_1) ]
> .recursion = [ bio(l=1,a), bio(l=1,b), bio(l=1,c), ... ]
>
> merge_head
>
> .incoming = [ bio(l=1,a), bio(l=1,b), bio(l=1,c), ...,
> bio(l=0,remainder_1) ]
> .recursion = []
>
> process head of .incoming, potentially split first
>
> .incoming = [ bio(l=1,a,now), bio(l=1,a,remainder), bio(l=1,b), bio(l=1,c), ...,
> bio(l=0,remainder_1) ]
> ...
> .incoming = [ bio(l=1,a,remainder), bio(l=1,b), bio(l=1,c), ...,
> bio(l=0,remainder_1) ]
> .recursion = [ bio(l=2,aa), bio(l=2,ab), ... ]
>
> merge_head
>
> .incoming = [ bio(l=2,aa), bio(l=2,ab), ...,
> bio(l=1,a,remainder), bio(l=1,b), bio(l=1,c), ...,
> bio(l=0,remainder_1) ]
> .recursion = []
>
> ...
>
> process away ... until back at l=0
>
> .incoming = [ bio(l=0,remainder_1) ]
> .recursion = []
>
> potentially split further
> .incoming = [ bio(l=0,now_2), bio(l=0,remainder_2) ]
> .recursion = []
>
> rinse, repeat.
I think we just might be in violent agreement.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply
* Re: [dm-devel] [RFC] block: fix blk_queue_split() resource exhaustion
From: Ming Lei @ 2016-07-08 11:08 UTC (permalink / raw)
To: NeilBrown
Cc: Lars Ellenberg, linux-block, Jens Axboe,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT,
Linux Kernel Mailing List, Martin K. Petersen, Mike Snitzer,
Peter Zijlstra, Jiri Kosina, open list:BCACHE (BLOCK LAYER CACHE),
Zheng Liu, Keith Busch, Takashi Iwai,
open list:DEVICE-MAPPER (LVM), Ingo Molnar, Kirill A. Shutemov,
Shaohua Li, Kent Overstreet, Alasdair Kergon, Roland Kammerer
In-Reply-To: <87vb0hf6fb.fsf@notabene.neil.brown.name>
On Fri, Jul 8, 2016 at 6:07 AM, NeilBrown <neilb@suse.com> wrote:
> On Thu, Jul 07 2016, Lars Ellenberg wrote:
>
>> On Thu, Jul 07, 2016 at 03:35:48PM +1000, NeilBrown wrote:
>>> On Wed, Jun 22 2016, Lars Ellenberg wrote:
>>>
>>> > For a long time, generic_make_request() converts recursion into
>>> > iteration by queuing recursive arguments on current->bio_list.
>>> >
>>> > This is convenient for stacking drivers,
>>> > the top-most driver would take the originally submitted bio,
>>> > and re-submit a re-mapped version of it, or one or more clones,
>>> > or one or more new allocated bios to its backend(s). Which
>>> > are then simply processed in turn, and each can again queue
>>> > more "backend-bios" until we reach the bottom of the driver stack,
>>> > and actually dispatch to the real backend device.
>>> >
>>> > Any stacking driver ->make_request_fn() could expect that,
>>> > once it returns, any backend-bios it submitted via recursive calls
>>> > to generic_make_request() would now be processed and dispatched, before
>>> > the current task would call into this driver again.
>>> >
>>> > This is changed by commit
>>> > 54efd50 block: make generic_make_request handle arbitrarily sized bios
>>> >
>>> > Drivers may call blk_queue_split() inside their ->make_request_fn(),
>>> > which may split the current bio into a front-part to be dealt with
>>> > immediately, and a remainder-part, which may need to be split even
>>> > further. That remainder-part will simply also be pushed to
>>> > current->bio_list, and would end up being head-of-queue, in front
>>> > of any backend-bios the current make_request_fn() might submit during
>>> > processing of the fron-part.
>>> >
>>> > Which means the current task would immediately end up back in the same
>>> > make_request_fn() of the same driver again, before any of its backend
>>> > bios have even been processed.
>>> >
>>> > This can lead to resource starvation deadlock.
>>> > Drivers could avoid this by learning to not need blk_queue_split(),
>>> > or by submitting their backend bios in a different context (dedicated
>>> > kernel thread, work_queue context, ...). Or by playing funny re-ordering
>>> > games with entries on current->bio_list.
>>> >
>>> > Instead, I suggest to distinguish between recursive calls to
>>> > generic_make_request(), and pushing back the remainder part in
>>> > blk_queue_split(), by pointing current->bio_lists to a
>>> > struct recursion_to_iteration_bio_lists {
>>> > struct bio_list recursion;
>>> > struct bio_list remainder;
>>> > }
>>> >
>>> > To have all bios targeted to drivers lower in the stack processed before
>>> > processing the next piece of a bio targeted at the higher levels,
>>> > as long as queued bios resulting from recursion are available,
>>> > they will continue to be processed in FIFO order.
>>> > Pushed back bio-parts resulting from blk_queue_split() will be processed
>>> > in LIFO order, one-by-one, whenever the recursion list becomes empty.
>>>
>>> I really like this change. It seems to precisely address the problem.
>>> The "problem" being that requests for "this" device are potentially
>>> mixed up with requests from underlying devices.
>>> However I'm not sure it is quite general enough.
>>>
>>> The "remainder" list is a stack of requests aimed at "this" level or
>>> higher, and I think it will always exactly fit that description.
>>> The "recursion" list needs to be a queue of requests aimed at the next
>>> level down, and that doesn't quiet work, because once you start acting
>>> on the first entry in that list, all the rest become "this" level.
>>
>> Uhm, well,
>> that's how it has been since you introduced this back in 2007, d89d879.
>> And it worked.
>
> Just because it "worked", doesn't mean it was "right" :-)
>
>>
>>> I think you can address this by always calling ->make_request_fn with an
>>> empty "recursion", then after the call completes, splice the "recursion"
>>> list that resulted (if any) on top of the "remainder" stack.
>>>
>>> This way, the "remainder" stack is always "requests for lower-level
>>> devices before request for upper level devices" and the "recursion"
>>> queue is always "requests for devices below the current level".
>>
>> Yes, I guess that would work as well,
>> but may need "empirical proof" to check for performance regressions.
>>
>>> I also really *don't* like the idea of punting to a separate thread - it
>>> seems to be just delaying the problem.
>>>
>>> Can you try move the bio_list_init(->recursion) call to just before
>>> the ->make_request_fn() call, and adding
>>> bio_list_merge_head(->remainder, ->recursion)
>>> just after?
>>> (or something like that) and confirm it makes sense, and works?
>>
>> Sure, will do.
>> I'd suggest this would be a patch on its own though, on top of this one.
>> Because it would change the order in which stacked bios are processed
>> wrt the way it used to be since 2007 (my suggestion as is does not).
>
> Yes, because I think the original order is wrong, as you have helped me
> to see.
> Before I introduced the recursion limiting, requests were handled as an
> in-order tree walk. The code I wrote tried to preserve that but didn't
> for several reasons. I think we need to restore the original in-order
> walk because it makes the most sense.
> So after processing a particular bio, we should then process all the
> 'child' bios - bios send to underlying devices. Then the 'sibling'
> bios, that were split off, and then any remaining parents and ancestors.
IMHO, that is just what the oneline patch is doing, isn't it?
| diff --git a/block/blk-core.c b/block/blk-core.c
| index 2475b1c7..a5623f6 100644
| --- a/block/blk-core.c
| +++ b/block/blk-core.c
| @@ -2048,7 +2048,7 @@ blk_qc_t generic_make_request(struct bio *bio)
| * should be added at the tail
| */
| if (current->bio_list) {
| - bio_list_add(current->bio_list, bio);
| + bio_list_add_head(current->bio_list, bio);
| goto out;
| }
Thanks,
>
> You patch created the right structures for doing this, my proposal took
> it a step closer, but now after more careful analysis I don't think it
> is quite right.
> With my previous proposal (and you latest patch - thanks!) requests for
> "this" level are stacked, but they should be queued.
> If a make_request_fn only ever submits one request for this level and
> zero or more lower levels, then the difference between a queue and a
> stack is irrelevant. If it submited more that one, a stack would cause
> them to be handled in the reverse order.
>
> To make the patch "perfect", and maybe even more elegant we could treat
> ->remainder and ->recursion more alike.
> i.e.:
> - generic make request has a private "stack" of requests.
> - before calling ->make_request_fn(), both ->remainder and ->recursion
> are initialised
> - after ->make_request_fn(), ->remainder are spliced in to top of
> 'stack', then ->recursion is spliced onto that.
> - If stack is not empty, the top request is popped and we loop to top.
>
> This reliably follows in-order execution, and handles siblings correctly
> (in submitted order) if/when a request splits off multiple siblings.
>
>>
>> Which may change performance metrics.
>> It may even improve some of them,
>> or maybe it does nothing, but we don't know.
>
> I think that as long a requests are submitted in the order they are
> created at each level there is no reason to expect performance
> regressions.
> All we are doing is changing the ordering between requests generated at
> different levels, and I think we are restoring a more natural order.
>
> Thanks,
> NeilBrown
^ permalink raw reply
* Re: [dm-devel] [RFC] block: fix blk_queue_split() resource exhaustion
From: Lars Ellenberg @ 2016-07-08 12:52 UTC (permalink / raw)
To: Ming Lei
Cc: NeilBrown, linux-block, Jens Axboe,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT,
Linux Kernel Mailing List, Martin K. Petersen, Mike Snitzer,
Peter Zijlstra, Jiri Kosina, open list:BCACHE (BLOCK LAYER CACHE),
Zheng Liu, Keith Busch, Takashi Iwai,
open list:DEVICE-MAPPER (LVM), Ingo Molnar, Kirill A. Shutemov,
Shaohua Li, Kent Overstreet, Alasdair Kergon, Roland Kammerer
In-Reply-To: <CACVXFVOpY5f4o-hKc+HFsap=a5YY95SGQ-4n33mWPxm=P+gS-g@mail.gmail.com>
On Fri, Jul 08, 2016 at 07:08:32PM +0800, Ming Lei wrote:
> > So after processing a particular bio, we should then process all the
> > 'child' bios - bios send to underlying devices. Then the 'sibling'
> > bios, that were split off, and then any remaining parents and ancestors.
>
> IMHO, that is just what the oneline patch is doing, isn't it?
>
> | diff --git a/block/blk-core.c b/block/blk-core.c
> | index 2475b1c7..a5623f6 100644
> | --- a/block/blk-core.c
> | +++ b/block/blk-core.c
> | @@ -2048,7 +2048,7 @@ blk_qc_t generic_make_request(struct bio *bio)
> | * should be added at the tail
> | */
> | if (current->bio_list) {
> | - bio_list_add(current->bio_list, bio);
> | + bio_list_add_head(current->bio_list, bio);
> | goto out;
> | }
Almost, but not quite.
As explained earlier, this will re-order.
It will still process bios in "deepest level first" order,
but it will process "sibling" bios in reverse submission order.
Think "very large bio" submitted to a stripe set
with small stripe width/stripe unit size.
So I'd expect this to be a performance hit in some scenarios,
unless the stack at some deeper level does back-merging in its elevator.
(If some driver is not able to merge stuff because of "reverse submission
order" this can easily mean saturating IOPS of the physical device with
small requests, throttling bandwidth to a minimum.)
That's why I mentioned it as "potential easy fix for the deadlock",
but did not suggest it as the proper way to fix this.
If however the powers that be decide that this was a non-issue,
we could use it this way.
Lars
^ permalink raw reply
* Re: [dm-devel] [RFC] block: fix blk_queue_split() resource exhaustion
From: Lars Ellenberg @ 2016-07-08 13:00 UTC (permalink / raw)
To: NeilBrown
Cc: Jens Axboe, linux-raid, Martin K. Petersen, Mike Snitzer,
Peter Zijlstra, Jiri Kosina, Ming Lei, linux-kernel, Zheng Liu,
linux-block, Takashi Iwai, linux-bcache, Ingo Molnar,
Alasdair Kergon, Keith Busch, dm-devel, Shaohua Li,
Kent Overstreet, Kirill A. Shutemov, Roland Kammerer
In-Reply-To: <877fcwfoyv.fsf@notabene.neil.brown.name>
On Fri, Jul 08, 2016 at 07:39:36PM +1000, NeilBrown wrote:
> >> To make the patch "perfect", and maybe even more elegant we could treat
> >> ->remainder and ->recursion more alike.
> >> i.e.:
> >> - generic make request has a private "stack" of requests.
> >> - before calling ->make_request_fn(), both ->remainder and ->recursion
> >> are initialised
> >> - after ->make_request_fn(), ->remainder are spliced in to top of
> >> 'stack', then ->recursion is spliced onto that.
> >> - If stack is not empty, the top request is popped and we loop to top.
> >>
> >> This reliably follows in-order execution, and handles siblings correctly
> >> (in submitted order) if/when a request splits off multiple siblings.
> >
> > The only splitting that creates siblings on the current level
> > is blk_queue_split(), which splits the current bio into
> > "front piece" and "remainder", already processed in this order.
>
> Yes.
> I imagine that a driver *could* split a bio into three parts with a
> single allocation, but I cannot actually see any point in doing it. So
> I was over-complicating things.
>
> >
> > Anything else creating "siblings" is not creating siblings for the
> > current layer, but for the next deeper layer, which are queue on
> > "recursion" and also processed in the order they have been generated.
> >
> >> I think that as long a requests are submitted in the order they are
> >> created at each level there is no reason to expect performance
> >> regressions.
> >> All we are doing is changing the ordering between requests generated at
> >> different levels, and I think we are restoring a more natural order.
> >
> > I believe both patches combined are doing exactly this already.
> > I could rename .remainder to .todo or .incoming, though.
>
> :-) neither "remainder" or "recursion" seem like brilliant names to me,
> but I don't have anything better to suggest. Naming is hard!
> As long as a comment explains the name clearly I could cope with X and Y.
...
> I think we just might be in violent agreement.
I thought so, too :-)
Should I merge both patches,
rename to ".queue" and ".tmp",
and submit for inclusion?
Lars Ellenberg
^ permalink raw reply
* Re: block: fix blk_queue_split() resource exhaustion
From: Mike Snitzer @ 2016-07-08 13:05 UTC (permalink / raw)
To: Lars Ellenberg
Cc: Ming Lei, NeilBrown, linux-block, Jens Axboe,
open list:SOFTWARE RAID (Multiple Disks) SUPPORT,
Linux Kernel Mailing List, Martin K. Petersen, Peter Zijlstra,
Jiri Kosina, open list:BCACHE (BLOCK LAYER CACHE), Zheng Liu,
Keith Busch, Takashi Iwai, open list:DEVICE-MAPPER (LVM),
Ingo Molnar, Kirill A. Shutemov, Shaohua Li, Kent Overstreet,
Alasdair Kergon, Roland Kammerer
In-Reply-To: <20160708125225.GV13335@soda.linbit>
On Fri, Jul 08 2016 at 8:52am -0400,
Lars Ellenberg <lars.ellenberg@linbit.com> wrote:
> On Fri, Jul 08, 2016 at 07:08:32PM +0800, Ming Lei wrote:
> > > So after processing a particular bio, we should then process all the
> > > 'child' bios - bios send to underlying devices. Then the 'sibling'
> > > bios, that were split off, and then any remaining parents and ancestors.
> >
> > IMHO, that is just what the oneline patch is doing, isn't it?
> >
> > | diff --git a/block/blk-core.c b/block/blk-core.c
> > | index 2475b1c7..a5623f6 100644
> > | --- a/block/blk-core.c
> > | +++ b/block/blk-core.c
> > | @@ -2048,7 +2048,7 @@ blk_qc_t generic_make_request(struct bio *bio)
> > | * should be added at the tail
> > | */
> > | if (current->bio_list) {
> > | - bio_list_add(current->bio_list, bio);
> > | + bio_list_add_head(current->bio_list, bio);
> > | goto out;
> > | }
>
> Almost, but not quite.
>
> As explained earlier, this will re-order.
> It will still process bios in "deepest level first" order,
> but it will process "sibling" bios in reverse submission order.
>
> Think "very large bio" submitted to a stripe set
> with small stripe width/stripe unit size.
>
> So I'd expect this to be a performance hit in some scenarios,
> unless the stack at some deeper level does back-merging in its elevator.
> (If some driver is not able to merge stuff because of "reverse submission
> order" this can easily mean saturating IOPS of the physical device with
> small requests, throttling bandwidth to a minimum.)
>
> That's why I mentioned it as "potential easy fix for the deadlock",
> but did not suggest it as the proper way to fix this.
>
> If however the powers that be decide that this was a non-issue,
> we could use it this way.
No, we cannot do this. With blk-mq it doesn't have any of the more
elaborate IO scheduling that request_fn request_queues have. We should
not be knowingly mangling the order of IO with the thought that some
other layer will fix it up.
I think it best for you to rebase your work (against jens' for-4.8/core)
into a single coherent patch and resubmit for 4.8 inclusion. I really
don't see a huge benefit to keeping neilb's suggestion split out -- but
if you or others do that's fine.
The only concern I have relative to DM is: DM doesn't use
blk_queue_split, so will it need to open-code setting
recursion/remainder in order to ensure forward progress? neilb seemed
to think the rework in generic_make_request would "just work" for the
dm-snapshot deadlock case though so maybe this isn't a valid
concern... unfortunately we don't have a quick reproducer for that
dm-snapshot issue so it'll take a bit to prove.
Mike
^ permalink raw reply
* Dear Beneficiary,
From: Sarah Michael @ 2016-07-08 13:49 UTC (permalink / raw)
Dear Beneficiary,
I'm happy to inform you that your email address has been selected
among those that have compensation payment of ($2.5 Million USD), The
INTERNATIONAL MONETARY FUND contacted us for the release of your money
a couple of hours ago due to your allocated security code. We are
unable to complete a transfer directed at an email address, so we
require some more information in order to complete this transfer, But
for security purpose the money has been loaded in a prepaid Visa Debit
Card on your behalf, You Withdraw cash at ATMs and make purchases
anywhere debit cards are accepted, including retail stores,grocery
stores, restaurants and pharmacies, You are advised that a maximum
withdrawal value of $10,000.00 USD is permitted daily.
Therefore arrangements has been perfectly concluded to effect the
delivery of your Visa Debit Card to your doorstep as soon as
youre-confirm the information's below.
1) Your full name:
2) Mobile number:
3) Delivery address:
4) Zip code:
I am waiting to hear from you soon,
With Best Regards,
Ms:Sarah Michael.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox