[PATCH v4 0/2] block, md: suspend md around queue limits updates

[PATCH v4 0/2] block, md: suspend md around queue limits updates

[Jack Wang]

From: Jack Wang <jinpu.wang@cloud.ionos.com>

Writing to a queue limits attribute of an md array while a spare is
being re-added deadlocks the array.  I reported this earlier here:

  https://lore.kernel.org/linux-raid/CAMGffE=heGA3y8FjQ0Sm1jj-kd-=H9Y54WozKASSEZhc9UNKjA@mail.gmail.com/

Four tasks, one 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

Since commit c99f66e4084a ("block: fix queue freeze vs limits lock
order in sysfs store methods") the sysfs store holds q->limits_lock
across the freeze.  A bio parked in md_handle_request() while the array
is suspended still holds a q_usage_counter reference, so that freeze
never completes.

v3 froze the queue in mddev_suspend().  Review showed this deadlocks
against raid1d/raid10d resubmitting a split bio and against raid456 I/O
waiting on an interrupted reshape.  Following Yu Kuai's suggestion, v4
lets a bio based disk suspend itself instead of freezing its queue:

 - Patch 1 adds optional ->suspend() and ->resume() to
   block_device_operations.  queue_attr_store() and the integrity
   flag_store() call ->suspend() before taking q->limits_lock, commit the
   limits without a freeze, then call ->resume().
 - Patch 2 implements them for md with mddev_suspend() and
   mddev_resume().

The order is suspend, then q->limits_lock, as in mddev_update_io_opt().
A suspend already held by another task only nests.

Known limitation: md_submit_bio() reads the limits in
bio_split_to_limits() before taking active_io, so a bio already there
when the suspend completes can see a mix of old and new limits.
mddev_update_io_opt() has the same window.

Christoph, does a ->suspend()/->resume() pair in block_device_operations
fit what you had in mind?

v3: https://lore.kernel.org/linux-raid/20261102154847.198472-1-jinpu.wang@ionos.com/
v2: https://lore.kernel.org/linux-raid/e2c36f31-d198-40a7-a18d-156e1fd6fbb2@fygo.io/
v1: https://lore.kernel.org/linux-raid/20260909063029.GC29874@lst.de/T/#t

Changes since v3:
 - Drop the freeze in mddev_suspend(); add ->suspend()/->resume() to
   block_device_operations, as suggested by Yu Kuai.
 - Also cover the integrity flag_store() path.
 - Rebased on v7.4-rc1.

Testing: the reproducer below, 20 fail/remove/add cycles with fio in
flight and a loop writing queue/max_sectors_kb, on a 6.12 based kernel
with lockdep enabled.  No lockdep reports, no hung tasks.  That kernel
had the queue_attr_store() change only; the flag_store() change and the
rebase to v7.4-rc1 are build tested only.

A blktests case for this deadlock was sent separately:

  https://lore.kernel.org/all/20260910100916.1638378-1-jinpu.wang@ionos.com/

The reproducer:

  mdadm -C /dev/md111 --force -e 1.2 --assume-clean -l 1 \
        --bitmap=internal -n 2 /dev/ram0 /dev/ram1

  fio --direct=1 --rw=randrw --ioengine=libaio --iodepth=32 --numjobs=4 \
      --time_based=1 --runtime=180 --filename=/dev/md111 --name=repro &

  while :; do
      echo 128 > /sys/block/md111/queue/max_sectors_kb 2>/dev/null
  done &

  for i in $(seq 20); do
      mdadm /dev/md111 --fail /dev/ram0
      mdadm /dev/md111 --remove /dev/ram0
      mdadm /dev/md111 --add /dev/ram0
      mdadm --wait /dev/md111
  done

Jack Wang (2):
  block: suspend bio based disks around queue limits updates
  md: suspend the array instead of freezing the queue for limits updates

 block/blk-integrity.c  | 10 ++++++++--
 block/blk-sysfs.c      | 19 +++++++++++++------
 block/blk.h            | 20 ++++++++++++++++++++
 drivers/md/md.c        | 12 ++++++++++++
 include/linux/blkdev.h |  7 +++++++
 5 files changed, 60 insertions(+), 8 deletions(-)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4

[PATCH v4 1/2] block: suspend bio based disks around queue limits updates

[Jack Wang]

queue_attr_store() freezes the queue under q->limits_lock. For md this can
deadlock: a bio parked while the array is suspended still holds a
q_usage_counter reference, so the freeze waits for it, while the suspender
waits for reconfig_mutex held by md_start_sync(), which waits for
q->limits_lock.

Add optional ->suspend() and ->resume() block_device_operations. When
present, queue_attr_store() and the integrity flag_store() suspend the disk
before taking q->limits_lock and commit the limits without freezing the
queue. Other disks behave as before.

Fixes: c99f66e4084a ("block: fix queue freeze vs limits lock order in sysfs store methods")
Suggested-by: Yu Kuai <yukuai@fygo.io>
Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
---
 block/blk-integrity.c  | 10 ++++++++--
 block/blk-sysfs.c      | 19 +++++++++++++------
 block/blk.h            | 20 ++++++++++++++++++++
 include/linux/blkdev.h |  7 +++++++
 4 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 964eebbee14d..b449e7c04eea 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -226,7 +226,8 @@ EXPORT_SYMBOL_GPL(blk_integrity_profile_name);
 static ssize_t flag_store(struct device *dev, const char *page, size_t count,
 		unsigned char flag)
 {
-	struct request_queue *q = dev_to_disk(dev)->queue;
+	struct gendisk *disk = dev_to_disk(dev);
+	struct request_queue *q = disk->queue;
 	struct queue_limits lim;
 	unsigned long val;
 	int err;
@@ -235,6 +236,10 @@ static ssize_t flag_store(struct device *dev, const char *page, size_t count,
 	if (err)
 		return err;
 
+	err = blk_disk_suspend(disk);
+	if (err)
+		return err;
+
 	/* note that the flags are inverted vs the values in the sysfs files */
 	lim = queue_limits_start_update(q);
 	if (val)
@@ -242,7 +247,8 @@ static ssize_t flag_store(struct device *dev, const char *page, size_t count,
 	else
 		lim.integrity.flags |= flag;
 
-	err = queue_limits_commit_update_frozen(q, &lim);
+	err = blk_disk_commit_limits(disk, &lim);
+	blk_disk_resume(disk);
 	if (err)
 		return err;
 	return count;
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 520972676ab4..3cbe36d04699 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -887,20 +887,27 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
 		return -EIO;
 
 	if (entry->store_limit) {
+		struct queue_limits lim;
 		ssize_t res;
 
-		struct queue_limits lim = queue_limits_start_update(q);
+		res = blk_disk_suspend(disk);
+		if (res)
+			return res;
+
+		lim = queue_limits_start_update(q);
 
 		res = entry->store_limit(disk, page, length, &lim);
 		if (res < 0) {
 			queue_limits_cancel_update(q);
-			return res;
+			goto out_resume;
 		}
 
-		res = queue_limits_commit_update_frozen(q, &lim);
-		if (res)
-			return res;
-		return length;
+		res = blk_disk_commit_limits(disk, &lim);
+		if (!res)
+			res = length;
+out_resume:
+		blk_disk_resume(disk);
+		return res;
 	}
 
 	return entry->store(disk, page, length);
diff --git a/block/blk.h b/block/blk.h
index ed2172472622..1c214f0bc178 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -661,6 +661,26 @@ void drop_partition(struct block_device *part);
 
 void bdev_set_nr_sectors(struct block_device *bdev, sector_t sectors);
 
+/* Suspend before queue_limits_start_update(), never under q->limits_lock. */
+static inline int blk_disk_suspend(struct gendisk *disk)
+{
+	return disk->fops->suspend ? disk->fops->suspend(disk) : 0;
+}
+
+static inline void blk_disk_resume(struct gendisk *disk)
+{
+	if (disk->fops->suspend)
+		disk->fops->resume(disk);
+}
+
+static inline int blk_disk_commit_limits(struct gendisk *disk,
+					 struct queue_limits *lim)
+{
+	if (disk->fops->suspend)
+		return queue_limits_commit_update(disk->queue, lim);
+	return queue_limits_commit_update_frozen(disk->queue, lim);
+}
+
 struct gendisk *__alloc_disk_node(struct request_queue *q, int node_id,
 		struct lock_class_key *lkclass);
 struct request_queue *blk_alloc_queue(struct queue_limits *lim, int node_id);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 0c2010230632..0011b7c0740f 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1601,6 +1601,13 @@ struct block_device_operations {
 			enum blk_unique_id id_type);
 	int (*init_dma_buf_io_ctx)(struct block_device *,
 				   struct dma_buf_io_ctx *);
+	/*
+	 * Optional, for bio based drivers that cannot be frozen under
+	 * q->limits_lock: stop I/O around a sysfs limits update.  Called
+	 * before q->limits_lock is taken, with no queue frozen.
+	 */
+	int (*suspend)(struct gendisk *disk);
+	void (*resume)(struct gendisk *disk);
 	struct module *owner;
 	const struct pr_ops *pr_ops;
 
-- 
2.43.0

Re: [PATCH v4 1/2] block: suspend bio based disks around queue limits updates

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] block: missing __releases() annotation on
  blk_disk_commit_limits()
--

commit 191cb84f3b159c80a0741f7887d437ec63eb1187
Author: Jack Wang <jinpu.wang@cloud.ionos.com>

block: suspend bio based disks around queue limits updates

This commit adds optional ->suspend() and ->resume() block_device_operations
callbacks and uses them in queue_attr_store() and flag_store() to suspend the
disk before taking q->limits_lock and commit queue limits without freezing
the queue.

> diff --git a/block/blk.h b/block/blk.h
> index ed21724726223..1c214f0bc1780 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -661,6 +661,26 @@ void drop_partition(struct block_device *part);
>  
>  void bdev_set_nr_sectors(struct block_device *bdev, sector_t sectors);
>  
> +/* Suspend before queue_limits_start_update(), never under q->limits_lock. */
> +static inline int blk_disk_suspend(struct gendisk *disk)
> +{
> +	return disk->fops->suspend ? disk->fops->suspend(disk) : 0;
> +}
> +
> +static inline void blk_disk_resume(struct gendisk *disk)
> +{
> +	if (disk->fops->suspend)
> +		disk->fops->resume(disk);
> +}
> +
> +static inline int blk_disk_commit_limits(struct gendisk *disk,
> +					 struct queue_limits *lim)
> +{
> +	if (disk->fops->suspend)
> +		return queue_limits_commit_update(disk->queue, lim);
> +	return queue_limits_commit_update_frozen(disk->queue, lim);
> +}

[Severity: Low]
Should blk_disk_commit_limits() have a __releases(&disk->queue->limits_lock)
annotation?

block/Makefile enables CONTEXT_ANALYSIS := y, and both
queue_limits_commit_update() and queue_limits_commit_update_frozen() are
annotated with __releases(&q->limits_lock) to pair with
queue_limits_start_update().  Without the annotation on
blk_disk_commit_limits(), building with Clang and
CONFIG_WARN_CONTEXT_ANALYSIS=y triggers -Wthread-safety warnings in
blk_disk_commit_limits() and its callers flag_store() and queue_attr_store().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112134700.372702-1-jinpu.wang@cloud.ionos.com?part=1

[PATCH v4 2/2] md: suspend the array instead of freezing the queue for limits updates

[Jack Wang]

Provide ->suspend() and ->resume() for md_fops using mddev_suspend() and
mddev_resume(), so sysfs limits updates no longer freeze the queue.

mddev_suspend() only waits for active_io, but md_submit_bio() reads the
limits in bio_split_to_limits() before taking it. A bio already there may
see a mix of old and new limits, as with mddev_update_io_opt().

Fixes: c99f66e4084a ("block: fix queue freeze vs limits lock order in sysfs store methods")
Suggested-by: Yu Kuai <yukuai@fygo.io>
Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
---
 drivers/md/md.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 03e35386a802..c9fa4e99569a 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8669,6 +8669,16 @@ static unsigned int md_check_events(struct gendisk *disk, unsigned int clearing)
 	return ret;
 }
 
+static int md_suspend_disk(struct gendisk *disk)
+{
+	return mddev_suspend(disk->private_data, true);
+}
+
+static void md_resume_disk(struct gendisk *disk)
+{
+	mddev_resume(disk->private_data);
+}
+
 static void md_free_disk(struct gendisk *disk)
 {
 	struct mddev *mddev = disk->private_data;
@@ -8689,6 +8699,8 @@ const struct block_device_operations md_fops =
 	.getgeo		= md_getgeo,
 	.check_events	= md_check_events,
 	.set_read_only	= md_set_read_only,
+	.suspend	= md_suspend_disk,
+	.resume		= md_resume_disk,
 	.free_disk	= md_free_disk,
 };
 
-- 
2.43.0