Repository navigation
md: don't wait for q->limits_lock while md holds back I/O - #1270
blktests-ci-kpd[bot] wants to merge 8 commits into
Conversation
|
Upstream branch: 28924df |
4ddd216 to
00cc4ca
Compare
|
Upstream branch: 893e117 |
fbe586b to
496c3db
Compare
|
Upstream branch: 893e117 |
496c3db to
ea8d4ab
Compare
00cc4ca to
7efd8cd
Compare
|
Upstream branch: 50d05c7 |
ea8d4ab to
ed8a026
Compare
7efd8cd to
a0aeca9
Compare
|
Upstream branch: 5225b8e |
ed8a026 to
c77a8c5
Compare
a0aeca9 to
772381e
Compare
|
Upstream branch: 2f0c1cf |
2 similar comments
|
Upstream branch: 2f0c1cf |
|
Upstream branch: 2f0c1cf |
c77a8c5 to
572ea3c
Compare
772381e to
0224dee
Compare
|
Upstream branch: 2f0c1cf |
572ea3c to
01edfda
Compare
0224dee to
f14340f
Compare
|
Upstream branch: 5878583 |
01edfda to
b4f03e4
Compare
f14340f to
0e174bc
Compare
|
Upstream branch: ce1e022 |
bfd3ea7 to
aad14bb
Compare
bbd3af0 to
a9c0b46
Compare
|
Upstream branch: e767a4e |
aad14bb to
0b54051
Compare
a9c0b46 to
72d0f5e
Compare
|
Upstream branch: a74306e |
0b54051 to
e9e4927
Compare
72d0f5e to
d980ad5
Compare
|
Upstream branch: None |
e9e4927 to
b4ee610
Compare
d980ad5 to
83b99cc
Compare
|
Upstream branch: 22430ae |
b4ee610 to
6d0eaab
Compare
83b99cc to
9081535
Compare
Adding a leg stacks its queue limits, which mddev_stack_new_rdev() does by taking q->limits_lock itself. Callers holding reconfig_mutex or a suspended array cannot allow that, and must own the update instead. Give ->hot_add_disk(), remove_and_add_spares() and md_choose_sync_action() a struct queue_limits argument with three states: an update to stack into, NULL to let the personality take the lock as before, or MDDEV_STACK_SKIP to add the leg without touching the limits, for callers that can do neither. mddev_stack_rdev_into() stacks into a caller-owned update without the lock. Every caller still passes NULL and nothing passes the sentinel yet, so there is no functional change; the users follow. Assisted-by: LLM Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
check_sb_changes() activates a spare another node added, reached from md_reload_sb() -> process_metadata_update() with reconfig_mutex held. Stacking the device's limits there waits for q->limits_lock under that mutex, which deadlocks: the lock's holder waits for the queue to drain, and that I/O can be waiting for a superblock update needing reconfig_mutex. The device is already a member, so its limits are stacked. Add it with MDDEV_STACK_SKIP and leave them alone. Assisted-by: LLM Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
state_store() and slot_store() can add a leg back to the array, which stacks its limits, and q->limits_lock has to be taken before the array is locked and suspended. Give the rdev sysfs store callback a struct queue_limits argument. rdev_attr_store() passes NULL, so there is no functional change; the user follows. Assisted-by: LLM Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
mddev_update_io_opt() runs from end_reshape() in the sync thread, and md_reap_sync_thread() waits for that thread with reconfig_mutex held. Taking q->limits_lock there hangs a finishing reshape whenever the lock's holder waits for I/O that only md_check_recovery() can let complete, and no ordering avoids it: the sync thread is what lets that I/O finish. Hand the update to a work item, which holds neither reconfig_mutex nor the suspend and so takes q->limits_lock in the order the rest of md uses, before suspending. __md_stop() flushes it, as it suspends the array. Also give the function a queue_limits argument, so a caller that already owns an update has it changed in place; the users of that path follow. Assisted-by: LLM Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
Writing a queue limits attribute while a spare is re-added deadlocks the
array:
udev-worker queue_attr_store() holds q->limits_lock, waits in
blk_mq_freeze_queue() for q_usage_counter to drain
fio holds a q_usage_counter reference, parked in
md_handle_request()'s is_suspended() loop
mdadm suspended the array, waits for reconfig_mutex
md_start_sync holds reconfig_mutex, waits for q->limits_lock
Blocking on q->limits_lock while holding reconfig_mutex, or with the
array suspended, is waiting for normal I/O, which mddev_suspend()
already warns about with lockdep_assert_not_held(). So q->limits_lock
has to nest outside both.
Take the update before the array is locked and suspended, and pass it
down so the personality stacks into it:
- md_start_sync(), at both suspend points
- md_ioctl() for ADD_NEW_DISK and HOT_REMOVE_DISK
- rdev_attr_store(), for slot and for state "remove"/"re-add"
- raid5 skip_copy_store(), which took the lock while suspended
They are converted together because a mix of the two orders is an ABBA.
All of them commit while the array is still quiesced.
Two callers still take the lock inside reconfig_mutex with the array
suspended: ->start_reshape() from action_store(), which suspends before
flushing sync_work so the update cannot be held across it, and
raid*_run() from level_store(), which a later patch converts.
Verified with a raid1 of two ram devices, fio in flight and a loop
writing queue/max_sectors_kb: 20 fail/remove/add cycles complete, where
the same test wedges the array before the change.
Fixes: c99f66e ("block: fix queue freeze vs limits lock order in sysfs store methods")
Assisted-by: LLM
Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
raid*_run() -> queue_limits_set() takes q->limits_lock with reconfig_mutex held, the order the previous patches inverted. With lockdep on, creating an array and then adding a leg reports it: -> #1 (&q->limits_lock): -> #0 (&mddev->reconfig_mutex): queue_limits_set md_ioctl <- ADD_NEW_DISK raid1_run do_md_run md_ioctl <- RUN_ARRAY The earlier patch left this for level_store() alone; RUN_ARRAY reaches it too, so every array creation records the wrong order. Give ->run() a queue_limits argument and take the update at the entry points that start an array: md_ioctl() for RUN_ARRAY, level_store(), autorun_devices(), md_setup_drive(), and array_state_store() for readonly, read_auto and active -- but only while mddev->pers is NULL, as with the array running those states go to md_set_readonly(), which waits in stop_sync_thread() for the work that takes the same lock. dm-raid passes NULL: with no gendisk the personalities return before touching any limits. Assisted-by: LLM Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
Opening a leg takes disk->open_mutex, and the scsi disk probe path nests q->limits_lock inside it (sd_open() -> sd_revalidate_disk()). md opens legs under reconfig_mutex, which this series makes q->limits_lock nest outside, closing a cycle. Booting with lockdep on an md root reports it during assembly: -> #2 (&q->limits_lock): sd_revalidate_disk / sd_open -> #1 (&disk->open_mutex): md_import_device md_add_new_disk md_ioctl <- ADD_NEW_DISK -> #0 (&mddev->reconfig_mutex): md_ioctl <- RUN_ARRAY Move every open out from under the lock. md_import_new_disk() mirrors md_add_new_disk()'s branch selection so all three of its branches take a pre-opened leg, and hot_add_disk(), new_dev_store() and md_setup_drive() open before they lock as well. The mddev fields the open depends on are read without reconfig_mutex, so each caller rechecks them once the array is locked and rejects the add with -EBUSY if the branch or the superblock format would have changed. md_autostart_arrays() needs no change: it opens under detected_devices_mutex. Assisted-by: LLM Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
bd_link_disk_holder() takes the leg's disk->open_mutex, and bind_rdev_to_array() calls it with reconfig_mutex held, so the dependency the previous patch removed from md_import_device() is still there by another route: -> #2 (&q->limits_lock): sd_revalidate_disk / sd_open -> #1 (&disk->open_mutex): bd_link_disk_holder bind_rdev_to_array md_add_new_disk md_ioctl <- ADD_NEW_DISK -> #0 (&mddev->reconfig_mutex): md_ioctl <- RUN_ARRAY bd_unlink_disk_holder() only takes blk_holder_mutex, which is why the release side needs no change and made the link side easy to miss. Link the holder where the leg is opened, before the array is locked, and record it in a new HolderLinked flag so the release side knows whether there is a link to drop. A failed link is not fatal, as before. A leg that is linked but not yet bound is released through md_export_rdev(), which drops the link first. A leg is now linked before it is known to be acceptable, so a leg the array goes on to reject shows up in its slaves directory until the error path releases it. md then no longer takes disk->open_mutex under reconfig_mutex: of the functions that take it, md reaches bdev_open() and bd_link_disk_holder() from the paths above, bdev_release() and bdev_fput() only through fput(), which defers to task work, del_gendisk() only from mddev teardown, and never sync_bdevs(). Assisted-by: LLM Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
|
Upstream branch: 69f80fe |
6d0eaab to
b36c904
Compare
|
Upstream branch: 69f80fe |
1 similar comment
|
Upstream branch: 69f80fe |
|
Github failed to update this PR after force push. Close it. |
Pull request for series with
subject: md: don't wait for q->limits_lock while md holds back I/O
version: 1
url: https://patchwork.kernel.org/series/1159795/