| Message ID | 20250304102551.2533767-1-nilay@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | block: fix lock order and remove redundant locking | expand |
On Tue, 04 Mar 2025 15:52:29 +0530, Nilay Shroff wrote: > After we modeled the freeze & enter queue as lock for supporting lockdep > under commit f1be1788a32e ("block: model freeze & enter queue as lock > for supporting lockdep"), we received numerous lockdep splats. And one > of those splats[1] reported the potential deadlock due to incorrect lock > ordering issue between q->sysfs-lock and q->q_usage_counter. So some of > the patches in this series are aimed to cut the dependency between q-> > sysfs-lock and q->q_usage_counter. > > [...] Applied, thanks! [1/7] block: acquire q->limits_lock while reading sysfs attributes commit: 6e51a1279cd60cb93e3379ff140d8fa6c39ecf20 [2/7] block: move q->sysfs_lock and queue-freeze under show/store method commit: b07a889e833555735ce72ca4a6d39f4c2ca725ba [3/7] block: remove q->sysfs_lock for attributes which don't need it commit: d23977fee1ee838316fb1b00945064a146460843 [4/7] block: introduce a dedicated lock for protecting queue elevator updates commit: 1bf70d08cc3b55abd1763e6dff5855cb8dd8318b [5/7] block: protect nr_requests update using q->elevator_lock commit: 3efe7571c3ae2b6481253a2616c2bb3fbadd503b [6/7] block: protect wbt_lat_usec using q->elevator_lock commit: 245618f8e45ff4f79327627b474b563da71c2c75 [7/7] block: protect read_ahead_kb using q->limits_lock commit: 5e40f4452dc9a3fb44d13bb6bc7032f3911a2675 Best regards,
Hi, After we modeled the freeze & enter queue as lock for supporting lockdep under commit f1be1788a32e ("block: model freeze & enter queue as lock for supporting lockdep"), we received numerous lockdep splats. And one of those splats[1] reported the potential deadlock due to incorrect lock ordering issue between q->sysfs-lock and q->q_usage_counter. So some of the patches in this series are aimed to cut the dependency between q-> sysfs-lock and q->q_usage_counter. This patchset contains seven patches in the series. The 1st patch helps acquire q->limits_lock instead of q->sysfs_lock while reading a set of attributes whose write method is protected with atomic limit update APIs or updates to these attributes could occur under atomic limit update APIs such as queue_limit_start_update() and queue_limits_ commit_update(). So all such attributes have been now grouped in queue_ attr_show under ->show_limit() method. The 2nd patch is preparation to further simplify and group sysfs attributes in subsequent patches. So in this patch we move acquire/release of q->sysfs _lock as well as queue freeze/unfreeze under each attributes' respective ->show()/->store() method. The 3rd patch removes the q->sysfs_lock for all sysfs attributes which don't need it. We identified all sysfs attributes which don't need any locking and removed use of q->sysfs_lock for these attributes. Subsequent patches address remaining attributes individually which require some form of locking other than q->limits_lock or q->sysfs_lock. The 4th patch introduce a new dedicated lock for elevator switch/update and thus eliminates the dependency of sched update on q->sysfs_lock. The 5th patch protects sysfs attribute nr_requests using q->elevator_lock instead of q->sysfs_lock as the update to q->nr_requests now happen under q->elevator_lock. Similarly, the 6th patch protects sysfs attribute wbt_lat_usec using q->elevator_lock instead of q->sysfs_lock as the update to wbt state and latency now happen under q->elevator_lock. The 7th patch protects read_ahead_kb using q->limits_lock instead of q->sysfs_lock as update to ->ra_pages is protected by ->limits_lock. The ->ra_pages is usually calculated from the queue limits by queue_ limits_commit_update. Please note that above changes were unit tested against blktests and quick xfstests with lockdep enabled. [1] https://lore.kernel.org/all/67637e70.050a0220.3157ee.000c.GAE@google.com/ --- Changes from v5: - Removed the stale left over wbt_lat_usec sysfs attribute from under blk_mq_queue_attrs[] causing duplicate sysfs entry warning (Reported by kernel test robot <oliver.sang@intel.com>) Link to report: https://lore.kernel.org/oe-lkp/202503041413.872983bd-lkp@intel.com/ Link to v5: https://lore.kernel.org/all/20250226124006.1593985-1-nilay@linux.ibm.com/ Changes from v4: - Misc updates to commit message and code comments (hch) Link to v4: https://lore.kernel.org/all/20250225133110.1441035-1-nilay@linux.ibm.com/ Changes from v3: - Make code comment a proper senteence. Also mention the fields protected by q->elevator_lock (hch) - Add a comment describing lock ordering followed for newly introduced ->elevator_lock (Hannes Reinecke) Link to v3: https://lore.kernel.org/all/20250224133102.1240146-1-nilay@linux.ibm.com/ Changes from v2: - Add a prep patch (second patch in the series) to first move q->sysfs_ lock and queue freeze from queue_attr_show/queue_attr_store to show/ store method of the respective attribute; then in subsequent patches we remove q->sysfs_lock from attributes that don't require any from of locking or replace q->sysfs_lock with appropriate lock for attributes that require some form of locking; this way we can get away with using ->show_nolock()/->store_nolock() methods (hch) - Few other misc updates to commit message and code comments (hch) - Rearrange the patchset to improve organization and clarity. Start with a patch that groups all attributes requiring protection under ->limits_ lock. Follow this with a preparatory patch, then introduce a patch that consolidates attributes that do not require any form of locking. Finally, address the remaining attributes individually in subsequent patches, which require some form of locking other than q->limits_lock or q->sysfs_lock. Link to v2: https://lore.kernel.org/all/20250218082908.265283-1-nilay@linux.ibm.com/ Changes from v1: - Audit all sysfs attributes in block layer and find attributes which don't need any locking as well as attributes which needs some form of locking; then remove locking from queue_attr_store/queue_attr_show and move it into the attributes that still need it in some form, followed by replacing it with the more suitable locks (hch) - Use dedicated lock for elevator switch/update (Ming Lei) - Re-arrange patchset to first segregate and group together all attributes which don't need locking followed by grouping attributes which need some form of locking. Link to v1: https://lore.kernel.org/all/20250205144506.663819-1-nilay@linux.ibm.com/ --- Nilay Shroff (7): block: acquire q->limits_lock while reading sysfs attributes block: move q->sysfs_lock and queue-freeze under show/store method block: remove q->sysfs_lock for attributes which don't need it block: introduce a dedicated lock for protecting queue elevator updates block: protect nr_requests update using q->elevator_lock block: protect wbt_lat_usec using q->elevator_lock block: protect read_ahead_kb using q->limits_lock block/blk-core.c | 1 + block/blk-iocost.c | 2 + block/blk-mq.c | 15 +- block/blk-settings.c | 2 +- block/blk-sysfs.c | 309 +++++++++++++++++++++++++++-------------- block/elevator.c | 43 ++++-- block/elevator.h | 2 - block/genhd.c | 9 +- include/linux/blkdev.h | 13 ++ 9 files changed, 259 insertions(+), 137 deletions(-)