* Re: [PATCH] md: avoid modifying spares while the array is not suspended
From: yu kuai @ 2026-07-05 16:16 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, magiclinan, xiao, yukuai
Cc: linux-raid, linux-kernel, sashiko-bot
In-Reply-To: <20260630075640.1081634-1-abd.masalkhi@gmail.com>
Hi,
在 2026/6/30 15:56, Abd-Alrhman Masalkhi 写道:
> remove_spares() and remove_and_add_spares() modify the array's rdev
> configuration. These operations are only safe after the array has been
> suspended.
>
> Today, md_start_sync() can call md_choose_sync_action() even when the
> array has not been suspended. As a result, md_choose_sync_action() may
> remove or replace rdevs while normal I/O is still accessing them.
>
> The race can occur as follows:
>
> raid10d Worker Normal IO
> ____________ _______________________ ______________________
>
> raid10_write_request()
> wait_blocked_dev()
> set Blocked
> set Faulty
> Skip Faulty rdev
> rrdev->nr_pending++
> .repl_bio = bio
> removeable_rdev = false .
> array not suspended .
> lock mddev goto err_handle
> lock mddev (wait)
> .
> update sb .
> clear Blocked .
> .
> unlock mddev .
> lock mddev (acquires)
> remove_spares()
> removeable_rdev = true
>
> raid10_remove_disk()
> rdev = replacement
> replacement = NULL
> rdev_dec_pending(NULL)
> unlock mddev (NULL)->nr_pending--
>
> In this case, rdev_dec_pending() is called with a NULL pointer,
> resulting in a NULL pointer dereference when attempting to decrement
> nr_pending.
>
> Fix this by ensuring that remove_spares() and remove_and_add_spares()
> are only called after the array has been suspended, preventing
> concurrent rdev configuration changes while normal I/O is in progress.
>
> Fixes: bc08041b32ab ("md: suspend array in md_start_sync() if array need reconfiguration")
> Reported-by: sashiko-bot <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
> drivers/md/md.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 66a41d482e59..c85ebb59535b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -10116,7 +10116,8 @@ static int remove_and_add_spares(struct mddev *mddev,
> return spares;
> }
>
> -static bool md_choose_sync_action(struct mddev *mddev, int *spares)
> +static bool md_choose_sync_action(struct mddev *mddev, int *spares,
> + bool array_suspended)
> {
> /* Check if reshape is in progress first. */
> if (mddev->reshape_position != MaxSector) {
> @@ -10132,7 +10133,9 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
>
> /* Check if resync is in progress. */
> if (mddev->resync_offset < MaxSector) {
> - remove_spares(mddev, NULL);
> + if (array_suspended)
> + remove_spares(mddev, NULL);
The problem looks real, however, I think this will cause a change that user will be awared,
if there are really spares that can be removed from conf, but array is not suspended here,
user will still expect rdev will be removed from conf automatically.
In md_start_sync, if suspend is false, can we check again after mddev_lock? If suspend is
supposed to be true, we can release the lock and retry with suspend = true.
> +
> set_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> clear_bit(MD_RECOVERY_LAZY_RECOVER, &mddev->recovery);
> @@ -10144,7 +10147,11 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
> * also removed and re-added, to allow the personality to fail the
> * re-add.
> */
> - *spares = remove_and_add_spares(mddev, NULL);
> + if (array_suspended)
> + *spares = remove_and_add_spares(mddev, NULL);
> + else
> + *spares = 0;
> +
> if (*spares || test_bit(MD_RECOVERY_LAZY_RECOVER, &mddev->recovery)) {
> clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> @@ -10189,11 +10196,12 @@ static void md_start_sync(struct work_struct *ws)
> * As we only add devices that are already in-sync, we can
> * activate the spares immediately.
> */
> - remove_and_add_spares(mddev, NULL);
> + if (suspend)
> + remove_and_add_spares(mddev, NULL);
> goto not_running;
> }
>
> - if (!md_choose_sync_action(mddev, &spares))
> + if (!md_choose_sync_action(mddev, &spares, suspend))
> goto not_running;
>
> if (!mddev->pers->sync_request)
--
Thanks,
Kuai
^ permalink raw reply
* Re: [RFC PATCH] md,dm: caller-owned memalloc_noio token in mddev_suspend()/resume()
From: yu kuai @ 2026-07-05 13:10 UTC (permalink / raw)
To: Chen Cheng, linux-raid, dm-devel, yukuai; +Cc: linux-kernel
In-Reply-To: <20260624101353.2678126-1-chencheng@fnnas.com>
Hi,
在 2026/6/24 18:13, Chen Cheng 写道:
> From: Chen Cheng <chencheng@fnnas.com>
>
> Save token as mddev-scoped in mddev->noio_flags cause PF_MEMALLOC_NOIO
> leak into task A, while task B restores a token that it never saved.
>
> scenario:
>
> task A mddev task B
> ====== ======= ============
> write suspend_lo
> mddev_suspend()
> suspended == 0
> drain active_io
> suspended = 1
> A: noio_A = memalloc_noio_save()
> A returns with PF_MEMALLOC_NOIO set
>
> write suspend_hi
> mddev_suspend()
> suspended == 1
> suspended = 2
> B returns
> (no save)
>
> mddev_resume()
> suspended = 1
> not last resume
> A returns
> A still has PF_MEMALLOC_NOIO <-- leaked
>
> mddev_resume()
> suspended = 0
> memalloc_noio_restore(noio_A)
> (restores A's token in B)
>
> Fixed by:
> - return each caller's noio_flags from mddev_suspend()
> - pass that token back into mddev_resume()
> - update the suspend-and-lock helpers to carry the token
> - store the token in struct raid_set for dm-raid paths where suspend
> and resume are paired across callbacks
>
> Validation:
> repeatedly updates the array's suspend_lo and suspend_hi sysfs from many
> concurrent userspace workers. That makes multiple tasks to call
> mddev_suspend()/mddev_resume() concurrently.
>
> Each worker:
> - reads its initial /proc/self/stat flags and verifies that PF_MEMALLOC_NOIO is not already
> set
> - writes 0 to either suspend_lo or suspend_hi
> - immediately reads its own task flags again
> - reports success if flags & PF_MEMALLOC_NOIO is true after the write returns
>
> Link: https://github.com/chencheng-fnnas/reproducer/blob/main/repro-md-noio-token-leak.sh
>
> Fixes: 78f57ef9d50a ("md: use memalloc scope APIs in mddev_suspend()/mddev_resume()")
>
> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
> ---
> drivers/md/dm-raid.c | 7 ++--
> drivers/md/md-autodetect.c | 5 ++-
> drivers/md/md-bitmap.c | 12 +++---
> drivers/md/md.c | 85 ++++++++++++++++++++++----------------
> drivers/md/md.h | 23 ++++++-----
> drivers/md/raid5-cache.c | 11 +++--
> drivers/md/raid5.c | 25 ++++++-----
> 7 files changed, 97 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> index 8f5a5e1342a9..d89207e3722a 100644
> --- a/drivers/md/dm-raid.c
> +++ b/drivers/md/dm-raid.c
> @@ -239,10 +239,11 @@ struct raid_set {
> int raid_disks;
> int delta_disks;
> int data_offset;
> int raid10_copies;
> int requested_bitmap_chunk_sectors;
> + unsigned int suspend_noio_flags;
>
> struct mddev md;
> struct raid_type *raid_type;
>
> sector_t array_sectors;
> @@ -3251,11 +3252,11 @@ static int raid_ctr(struct dm_target *ti, unsigned int argc, char **argv)
> /* Start raid set read-only and assumed clean to change in raid_resume() */
> rs->md.ro = MD_RDONLY;
> rs->md.in_sync = 1;
>
> /* Has to be held on running the array */
> - mddev_suspend_and_lock_nointr(&rs->md);
> + mddev_suspend_and_lock_nointr(&rs->md, &rs->suspend_noio_flags);
>
> /* Keep array frozen until resume. */
> md_frozen_sync_thread(&rs->md);
>
> r = md_run(&rs->md);
> @@ -3863,11 +3864,11 @@ static void raid_postsuspend(struct dm_target *ti)
> /*
> * sync_thread must be stopped during suspend, and writes have
> * to be stopped before suspending to avoid deadlocks.
> */
> md_stop_writes(&rs->md);
> - mddev_suspend(&rs->md, false);
> + mddev_suspend(&rs->md, false, &rs->suspend_noio_flags);
> rs->md.ro = MD_RDONLY;
> }
> clear_bit(MD_DM_SUSPENDING, &mddev->flags);
>
> }
> @@ -4141,11 +4142,11 @@ static void raid_resume(struct dm_target *ti)
> lockdep_is_held(&mddev->reconfig_mutex)));
> clear_bit(RT_FLAG_RS_FROZEN, &rs->runtime_flags);
> mddev->ro = MD_RDWR;
> mddev->in_sync = 0;
> md_unfrozen_sync_thread(mddev);
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, rs->suspend_noio_flags);
> }
> }
For mdraid, changes looks fine. However, for dm-raid, for example:
dmsetup suspend ...
In this case, array will be suspended while task returned to user. Take a look at
the commit to introduce memalloc_noio_save(), it's supposed to be called for mdraid
arrays to allocate memory while array is suspended, I don't see why dm-raid need it.
So I'll suggest just bypass the memalloc_noio_save() for dm-raid where mddev_suspend()
is not paired with mddev_resume() in the same task context.
>
> static struct target_type raid_target = {
> .name = "raid",
> diff --git a/drivers/md/md-autodetect.c b/drivers/md/md-autodetect.c
> index 4b80165afd23..58e062cd0580 100644
> --- a/drivers/md/md-autodetect.c
> +++ b/drivers/md/md-autodetect.c
> @@ -126,10 +126,11 @@ static void __init md_setup_drive(struct md_setup_args *args)
> dev_t devices[MD_SB_DISKS + 1], mdev;
> struct mdu_array_info_s ainfo = { };
> struct mddev *mddev;
> int err = 0, i;
> char name[16];
> + unsigned int noio_flags;
>
> if (args->partitioned) {
> mdev = MKDEV(mdp_major, args->minor << MdpMinorShift);
> sprintf(name, "md_d%d", args->minor);
> } else {
> @@ -173,11 +174,11 @@ static void __init md_setup_drive(struct md_setup_args *args)
> if (IS_ERR(mddev)) {
> pr_err("md: md_alloc failed - cannot start array %s\n", name);
> return;
> }
>
> - err = mddev_suspend_and_lock(mddev);
> + err = mddev_suspend_and_lock(mddev, &noio_flags);
> if (err) {
> pr_err("md: failed to lock array %s\n", name);
> goto out_mddev_put;
> }
>
> @@ -219,11 +220,11 @@ static void __init md_setup_drive(struct md_setup_args *args)
> if (!err)
> err = do_md_run(mddev);
> if (err)
> pr_warn("md: starting %s failed\n", name);
> out_unlock:
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> out_mddev_put:
> mddev_put(mddev);
> }
>
> static int __init raid_setup(char *str)
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 028b9ca8ce52..74b7f569a3f4 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -2620,13 +2620,14 @@ location_show(struct mddev *mddev, char *page)
> }
>
> static ssize_t
> location_store(struct mddev *mddev, const char *buf, size_t len)
> {
> + unsigned int noio_flags;
> int rv;
>
> - rv = mddev_suspend_and_lock(mddev);
> + rv = mddev_suspend_and_lock(mddev, &noio_flags);
> if (rv)
> return rv;
>
> if (mddev->pers) {
> if (mddev->recovery || mddev->sync_thread) {
> @@ -2711,11 +2712,11 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
> set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
> md_wakeup_thread(mddev->thread);
> }
> rv = 0;
> out:
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> if (rv)
> return rv;
> return len;
>
> merge_err:
> @@ -2831,17 +2832,18 @@ backlog_store(struct mddev *mddev, const char *buf, size_t len)
> {
> unsigned long backlog;
> unsigned long old_mwb = mddev->bitmap_info.max_write_behind;
> struct md_rdev *rdev;
> bool has_write_mostly = false;
> + unsigned int noio_flags;
> int rv = kstrtoul(buf, 10, &backlog);
> if (rv)
> return rv;
> if (backlog > COUNTER_MAX)
> return -EINVAL;
>
> - rv = mddev_suspend_and_lock(mddev);
> + rv = mddev_suspend_and_lock(mddev, &noio_flags);
> if (rv)
> return rv;
>
> /*
> * Without write mostly device, it doesn't make sense to set
> @@ -2854,11 +2856,11 @@ backlog_store(struct mddev *mddev, const char *buf, size_t len)
> }
> }
> if (!has_write_mostly) {
> pr_warn_ratelimited("%s: can't set backlog, no write mostly device available\n",
> mdname(mddev));
> - mddev_unlock(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> return -EINVAL;
> }
>
> mddev->bitmap_info.max_write_behind = backlog;
> if (!backlog && mddev->serial_info_pool) {
> @@ -2871,11 +2873,11 @@ backlog_store(struct mddev *mddev, const char *buf, size_t len)
> mddev_create_serial_pool(mddev, rdev);
> }
> if (old_mwb != backlog)
> bitmap_update_sb(mddev->bitmap);
>
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> return len;
> }
>
> static struct md_sysfs_entry bitmap_backlog =
> __ATTR(backlog, S_IRUGO|S_IWUSR, backlog_show, backlog_store);
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 1377c407614c..86d938dee50a 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -459,11 +459,12 @@ static void md_submit_bio(struct bio *bio)
>
> /*
> * Make sure no new requests are submitted to the device, and any requests that
> * have been submitted are completely handled.
> */
> -int mddev_suspend(struct mddev *mddev, bool interruptible)
> +int mddev_suspend(struct mddev *mddev, bool interruptible,
> + unsigned int *noio_flags)
> {
> int err = 0;
>
> /*
> * hold reconfig_mutex to wait for normal io will deadlock, because
> @@ -478,10 +479,11 @@ int mddev_suspend(struct mddev *mddev, bool interruptible)
> mutex_lock(&mddev->suspend_mutex);
> if (err)
> return err;
>
> if (mddev->suspended) {
> + *noio_flags = memalloc_noio_save();
> WRITE_ONCE(mddev->suspended, mddev->suspended + 1);
> mutex_unlock(&mddev->suspend_mutex);
> return 0;
> }
>
> @@ -515,31 +517,30 @@ int mddev_suspend(struct mddev *mddev, bool interruptible)
> * prevent deadlock.
> */
> WRITE_ONCE(mddev->suspended, mddev->suspended + 1);
>
> /* restrict memory reclaim I/O during raid array is suspend */
> - mddev->noio_flag = memalloc_noio_save();
> + *noio_flags = memalloc_noio_save();
>
> mutex_unlock(&mddev->suspend_mutex);
> return 0;
> }
> EXPORT_SYMBOL_GPL(mddev_suspend);
>
> -static void __mddev_resume(struct mddev *mddev, bool recovery_needed)
> +static void __mddev_resume(struct mddev *mddev, bool recovery_needed,
> + unsigned int noio_flags)
> {
> lockdep_assert_not_held(&mddev->reconfig_mutex);
>
> mutex_lock(&mddev->suspend_mutex);
> + memalloc_noio_restore(noio_flags);
> WRITE_ONCE(mddev->suspended, mddev->suspended - 1);
> if (mddev->suspended) {
> mutex_unlock(&mddev->suspend_mutex);
> return;
> }
>
> - /* entred the memalloc scope from mddev_suspend() */
> - memalloc_noio_restore(mddev->noio_flag);
> -
> percpu_ref_resurrect(&mddev->active_io);
> wake_up(&mddev->sb_wait);
>
> if (recovery_needed)
> set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
> @@ -547,13 +548,13 @@ static void __mddev_resume(struct mddev *mddev, bool recovery_needed)
> md_wakeup_thread(mddev->sync_thread); /* possibly kick off a reshape */
>
> mutex_unlock(&mddev->suspend_mutex);
> }
>
> -void mddev_resume(struct mddev *mddev)
> +void mddev_resume(struct mddev *mddev, unsigned int noio_flags)
> {
> - return __mddev_resume(mddev, true);
> + return __mddev_resume(mddev, true, noio_flags);
> }
> EXPORT_SYMBOL_GPL(mddev_resume);
>
> /* sync bdev before setting device to readonly or stopping raid*/
> static int mddev_set_closing_and_sync_blockdev(struct mddev *mddev, int opener_num)
> @@ -3737,10 +3738,11 @@ rdev_attr_store(struct kobject *kobj, struct attribute *attr,
> {
> struct rdev_sysfs_entry *entry = container_of(attr, struct rdev_sysfs_entry, attr);
> struct md_rdev *rdev = container_of(kobj, struct md_rdev, kobj);
> struct kernfs_node *kn = NULL;
> bool suspend = false;
> + unsigned int noio_flags = 0;
> ssize_t rv;
> struct mddev *mddev = READ_ONCE(rdev->mddev);
>
> if (!entry->store)
> return -EIO;
> @@ -3756,17 +3758,17 @@ rdev_attr_store(struct kobject *kobj, struct attribute *attr,
> cmd_match(page, "writemostly") ||
> cmd_match(page, "-writemostly"))
> suspend = true;
> }
>
> - rv = suspend ? mddev_suspend_and_lock(mddev) : mddev_lock(mddev);
> + rv = suspend ? mddev_suspend_and_lock(mddev, &noio_flags) : mddev_lock(mddev);
> if (!rv) {
> if (rdev->mddev == NULL)
> rv = -ENODEV;
> else
> rv = entry->store(rdev, page, length);
> - suspend ? mddev_unlock_and_resume(mddev) : mddev_unlock(mddev);
> + suspend ? mddev_unlock_and_resume(mddev, noio_flags) : mddev_unlock(mddev);
> }
>
> if (kn)
> sysfs_unbreak_active_protection(kn);
>
> @@ -4049,15 +4051,16 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
> size_t slen = len;
> struct md_personality *pers, *oldpers;
> long level;
> void *priv, *oldpriv;
> struct md_rdev *rdev;
> + unsigned int noio_flags;
>
> if (slen == 0 || slen >= sizeof(clevel))
> return -EINVAL;
>
> - rv = mddev_suspend_and_lock(mddev);
> + rv = mddev_suspend_and_lock(mddev, &noio_flags);
> if (rv)
> return rv;
>
> if (mddev->pers == NULL) {
> memcpy(mddev->clevel, buf, slen);
> @@ -4231,11 +4234,11 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
> md_update_sb(mddev, 1);
> sysfs_notify_dirent_safe(mddev->sysfs_level);
> md_new_event();
> rv = len;
> out_unlock:
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> return rv;
> }
>
> static struct md_sysfs_entry md_level =
> __ATTR(level, S_IRUGO|S_IWUSR, level_show, level_store);
> @@ -4410,17 +4413,18 @@ static int update_raid_disks(struct mddev *mddev, int raid_disks);
>
> static ssize_t
> raid_disks_store(struct mddev *mddev, const char *buf, size_t len)
> {
> unsigned int n;
> + unsigned int noio_flags;
> int err;
>
> err = kstrtouint(buf, 10, &n);
> if (err < 0)
> return err;
>
> - err = mddev_suspend_and_lock(mddev);
> + err = mddev_suspend_and_lock(mddev, &noio_flags);
> if (err)
> return err;
> if (mddev->pers) {
> if (n != mddev->raid_disks)
> err = update_raid_disks(mddev, n);
> @@ -4442,11 +4446,11 @@ raid_disks_store(struct mddev *mddev, const char *buf, size_t len)
> mddev->raid_disks = n;
> mddev->reshape_backwards = (mddev->delta_disks < 0);
> } else
> mddev->raid_disks = n;
> out_unlock:
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> return err ? err : len;
> }
> static struct md_sysfs_entry md_raid_disks =
> __ATTR(raid_disks, S_IRUGO|S_IWUSR, raid_disks_show, raid_disks_store);
>
> @@ -4822,10 +4826,11 @@ new_dev_store(struct mddev *mddev, const char *buf, size_t len)
> char *e;
> int major = simple_strtoul(buf, &e, 10);
> int minor;
> dev_t dev;
> struct md_rdev *rdev;
> + unsigned int noio_flags;
> int err;
>
> if (!*buf || *e != ':' || !e[1] || e[1] == '\n')
> return -EINVAL;
> minor = simple_strtoul(e+1, &e, 10);
> @@ -4834,11 +4839,11 @@ new_dev_store(struct mddev *mddev, const char *buf, size_t len)
> dev = MKDEV(major, minor);
> if (major != MAJOR(dev) ||
> minor != MINOR(dev))
> return -EOVERFLOW;
>
> - err = mddev_suspend_and_lock(mddev);
> + err = mddev_suspend_and_lock(mddev, &noio_flags);
> if (err)
> return err;
> if (mddev->persistent) {
> rdev = md_import_device(dev, mddev->major_version,
> mddev->minor_version);
> @@ -4855,18 +4860,18 @@ new_dev_store(struct mddev *mddev, const char *buf, size_t len)
> rdev = md_import_device(dev, -2, -1);
> else
> rdev = md_import_device(dev, -1, -1);
>
> if (IS_ERR(rdev)) {
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> return PTR_ERR(rdev);
> }
> err = bind_rdev_to_array(rdev, mddev);
> out:
> if (err)
> export_rdev(rdev);
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> if (!err)
> md_new_event();
> return err ? err : len;
> }
>
> @@ -5257,28 +5262,29 @@ static int mddev_start_reshape(struct mddev *mddev)
> static ssize_t
> action_store(struct mddev *mddev, const char *page, size_t len)
> {
> int ret;
> enum sync_action action;
> + unsigned int noio_flags = 0;
>
> if (!mddev->pers || !mddev->pers->sync_request)
> return -EINVAL;
>
> action = md_sync_action_by_name(page);
> retry:
> if (work_busy(&mddev->sync_work))
> flush_work(&mddev->sync_work);
>
> ret = (action == ACTION_RESHAPE) ?
> - mddev_suspend_and_lock(mddev) :
> + mddev_suspend_and_lock(mddev, &noio_flags) :
> mddev_lock(mddev);
> if (ret)
> return ret;
>
> if (work_busy(&mddev->sync_work)) {
> if (action == ACTION_RESHAPE)
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> else
> mddev_unlock(mddev);
> goto retry;
> }
>
> @@ -5349,11 +5355,11 @@ action_store(struct mddev *mddev, const char *page, size_t len)
> sysfs_notify_dirent_safe(mddev->sysfs_action);
> ret = len;
>
> out:
> if (action == ACTION_RESHAPE)
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> else
> mddev_unlock(mddev);
> return ret;
> }
>
> @@ -5640,24 +5646,25 @@ suspend_lo_show(struct mddev *mddev, char *page)
>
> static ssize_t
> suspend_lo_store(struct mddev *mddev, const char *buf, size_t len)
> {
> unsigned long long new;
> + unsigned int noio_flags;
> int err;
>
> err = kstrtoull(buf, 10, &new);
> if (err < 0)
> return err;
> if (new != (sector_t)new)
> return -EINVAL;
>
> - err = mddev_suspend(mddev, true);
> + err = mddev_suspend(mddev, true, &noio_flags);
> if (err)
> return err;
>
> WRITE_ONCE(mddev->suspend_lo, new);
> - mddev_resume(mddev);
> + mddev_resume(mddev, noio_flags);
>
> return len;
> }
> static struct md_sysfs_entry md_suspend_lo =
> __ATTR(suspend_lo, S_IRUGO|S_IWUSR, suspend_lo_show, suspend_lo_store);
> @@ -5671,24 +5678,25 @@ suspend_hi_show(struct mddev *mddev, char *page)
>
> static ssize_t
> suspend_hi_store(struct mddev *mddev, const char *buf, size_t len)
> {
> unsigned long long new;
> + unsigned int noio_flags;
> int err;
>
> err = kstrtoull(buf, 10, &new);
> if (err < 0)
> return err;
> if (new != (sector_t)new)
> return -EINVAL;
>
> - err = mddev_suspend(mddev, true);
> + err = mddev_suspend(mddev, true, &noio_flags);
> if (err)
> return err;
>
> WRITE_ONCE(mddev->suspend_hi, new);
> - mddev_resume(mddev);
> + mddev_resume(mddev, noio_flags);
>
> return len;
> }
> static struct md_sysfs_entry md_suspend_hi =
> __ATTR(suspend_hi, S_IRUGO|S_IWUSR, suspend_hi_show, suspend_hi_store);
> @@ -5928,19 +5936,20 @@ static ssize_t serialize_policy_show(struct mddev *mddev, char *page)
> static ssize_t
> serialize_policy_store(struct mddev *mddev, const char *buf, size_t len)
> {
> int err;
> bool value;
> + unsigned int noio_flags;
>
> err = kstrtobool(buf, &value);
> if (err)
> return err;
>
> if (value == test_bit(MD_SERIALIZE_POLICY, &mddev->flags))
> return len;
>
> - err = mddev_suspend_and_lock(mddev);
> + err = mddev_suspend_and_lock(mddev, &noio_flags);
> if (err)
> return err;
> if (mddev->pers == NULL || (mddev->pers->head.id != ID_RAID1)) {
> pr_err("md: serialize_policy is only effective for raid1\n");
> err = -EINVAL;
> @@ -5953,11 +5962,11 @@ serialize_policy_store(struct mddev *mddev, const char *buf, size_t len)
> } else {
> mddev_destroy_serial_pool(mddev, NULL);
> clear_bit(MD_SERIALIZE_POLICY, &mddev->flags);
> }
> unlock:
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> return err ?: len;
> }
>
> static struct md_sysfs_entry md_serialize_policy =
> __ATTR(serialize_policy, S_IRUGO | S_IWUSR, serialize_policy_show,
> @@ -6263,21 +6272,22 @@ EXPORT_SYMBOL_GPL(mddev_stack_new_rdev);
>
> /* update the optimal I/O size after a reshape */
> void mddev_update_io_opt(struct mddev *mddev, unsigned int nr_stripes)
> {
> struct queue_limits lim;
> + unsigned int noio_flags;
>
> if (mddev_is_dm(mddev))
> return;
>
> /* don't bother updating io_opt if we can't suspend the array */
> - if (mddev_suspend(mddev, false) < 0)
> + if (mddev_suspend(mddev, false, &noio_flags) < 0)
> return;
> lim = queue_limits_start_update(mddev->gendisk->queue);
> lim.io_opt = lim.io_min * nr_stripes;
> queue_limits_commit_update(mddev->gendisk->queue, &lim);
> - mddev_resume(mddev);
> + mddev_resume(mddev, noio_flags);
> }
> EXPORT_SYMBOL_GPL(mddev_update_io_opt);
>
> static void mddev_delayed_delete(struct work_struct *ws)
> {
> @@ -7255,10 +7265,11 @@ static void autorun_array(struct mddev *mddev)
> */
> static void autorun_devices(int part)
> {
> struct md_rdev *rdev0, *rdev, *tmp;
> struct mddev *mddev;
> + unsigned int noio_flags;
>
> pr_info("md: autorun ...\n");
> while (!list_empty(&pending_raid_disks)) {
> int unit;
> dev_t dev;
> @@ -7295,27 +7306,27 @@ static void autorun_devices(int part)
>
> mddev = md_alloc(dev, NULL);
> if (IS_ERR(mddev))
> break;
>
> - if (mddev_suspend_and_lock(mddev))
> + if (mddev_suspend_and_lock(mddev, &noio_flags))
> pr_warn("md: %s locked, cannot run\n", mdname(mddev));
> else if (mddev->raid_disks || mddev->major_version
> || !list_empty(&mddev->disks)) {
> pr_warn("md: %s already running, cannot run %pg\n",
> mdname(mddev), rdev0->bdev);
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> } else {
> pr_debug("md: created %s\n", mdname(mddev));
> mddev->persistent = 1;
> rdev_for_each_list(rdev, tmp, &candidates) {
> list_del_init(&rdev->same_set);
> if (bind_rdev_to_array(rdev, mddev))
> export_rdev(rdev);
> }
> autorun_array(mddev);
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> }
> /* on success, candidates will be empty, on error
> * it won't...
> */
> rdev_for_each_list(rdev, tmp, &candidates) {
> @@ -8329,10 +8340,11 @@ static int __md_set_array_info(struct mddev *mddev, void __user *argp)
>
> static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
> unsigned int cmd, unsigned long arg)
> {
> int err = 0;
> + unsigned int noio_flags = 0;
> void __user *argp = (void __user *)arg;
> struct mddev *mddev = NULL;
>
> err = md_ioctl_valid(cmd);
> if (err)
> @@ -8380,11 +8392,11 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
> }
>
> if (!md_is_rdwr(mddev))
> flush_work(&mddev->sync_work);
>
> - err = md_ioctl_need_suspend(cmd) ? mddev_suspend_and_lock(mddev) :
> + err = md_ioctl_need_suspend(cmd) ? mddev_suspend_and_lock(mddev, &noio_flags) :
> mddev_lock(mddev);
> if (err) {
> pr_debug("md: ioctl lock interrupted, reason %d, cmd %d\n",
> err, cmd);
> goto out;
> @@ -8511,11 +8523,11 @@ static int md_ioctl(struct block_device *bdev, blk_mode_t mode,
> unlock:
> if (mddev->hold_active == UNTIL_IOCTL &&
> err != -EINVAL)
> mddev->hold_active = 0;
>
> - md_ioctl_need_suspend(cmd) ? mddev_unlock_and_resume(mddev) :
> + md_ioctl_need_suspend(cmd) ? mddev_unlock_and_resume(mddev, noio_flags) :
> mddev_unlock(mddev);
>
> out:
> if (cmd == STOP_ARRAY_RO || (err && cmd == STOP_ARRAY))
> clear_bit(MD_CLOSING, &mddev->flags);
> @@ -10180,20 +10192,21 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
> static void md_start_sync(struct work_struct *ws)
> {
> struct mddev *mddev = container_of(ws, struct mddev, sync_work);
> int spares = 0;
> bool suspend = false;
> + unsigned int noio_flags = 0;
> char *name;
>
> /*
> * If reshape is still in progress, spares won't be added or removed
> * from conf until reshape is done.
> */
> if (mddev->reshape_position == MaxSector &&
> md_spares_need_change(mddev)) {
> suspend = true;
> - mddev_suspend(mddev, false);
> + mddev_suspend(mddev, false, &noio_flags);
> }
>
> mddev_lock_nointr(mddev);
> if (!md_is_rdwr(mddev)) {
> /*
> @@ -10237,11 +10250,11 @@ static void md_start_sync(struct work_struct *ws)
> * not set it again. Otherwise, we may cause issue like this one:
> * https://bugzilla.kernel.org/show_bug.cgi?id=218200
> * Therefore, use __mddev_resume(mddev, false).
> */
> if (suspend)
> - __mddev_resume(mddev, false);
> + __mddev_resume(mddev, false, noio_flags);
> md_wakeup_thread(mddev->sync_thread);
> sysfs_notify_dirent_safe(mddev->sysfs_action);
> md_new_event();
> return;
>
> @@ -10257,11 +10270,11 @@ static void md_start_sync(struct work_struct *ws)
> * not set it again. Otherwise, we may cause issue like this one:
> * https://bugzilla.kernel.org/show_bug.cgi?id=218200
> * Therefore, use __mddev_resume(mddev, false).
> */
> if (suspend)
> - __mddev_resume(mddev, false);
> + __mddev_resume(mddev, false, noio_flags);
>
> wake_up(&resync_wait);
> if (test_and_clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery) &&
> mddev->sysfs_action)
> sysfs_notify_dirent_safe(mddev->sysfs_action);
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index d8daf0f75cbb..3337cd21eb30 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -619,11 +619,10 @@ struct mddev {
> mempool_t *serial_info_pool;
> void (*sync_super)(struct mddev *mddev, struct md_rdev *rdev);
> struct md_cluster_info *cluster_info;
> struct md_cluster_operations *cluster_ops;
> unsigned int good_device_nr; /* good device num within cluster raid */
> - unsigned int noio_flag; /* for memalloc scope API */
>
> /*
> * Temporarily store rdev that will be finally removed when
> * reconfig_mutex is unlocked, protected by reconfig_mutex.
> */
> @@ -953,12 +952,13 @@ extern void md_stop(struct mddev *mddev);
> extern void md_stop_writes(struct mddev *mddev);
> extern int md_rdev_init(struct md_rdev *rdev);
> extern void md_rdev_clear(struct md_rdev *rdev);
>
> extern bool md_handle_request(struct mddev *mddev, struct bio *bio);
> -extern int mddev_suspend(struct mddev *mddev, bool interruptible);
> -extern void mddev_resume(struct mddev *mddev);
> +extern int mddev_suspend(struct mddev *mddev, bool interruptible,
> + unsigned int *noio_flags);
> +extern void mddev_resume(struct mddev *mddev, unsigned int noio_flags);
> extern void md_idle_sync_thread(struct mddev *mddev);
> extern void md_frozen_sync_thread(struct mddev *mddev);
> extern void md_unfrozen_sync_thread(struct mddev *mddev);
>
> extern void md_update_sb(struct mddev *mddev, int force);
> @@ -999,35 +999,38 @@ static inline void mddev_check_write_zeroes(struct mddev *mddev, struct bio *bio
> if (bio_op(bio) == REQ_OP_WRITE_ZEROES &&
> !bio->bi_bdev->bd_disk->queue->limits.max_write_zeroes_sectors)
> mddev->gendisk->queue->limits.max_write_zeroes_sectors = 0;
> }
>
> -static inline int mddev_suspend_and_lock(struct mddev *mddev)
> +static inline int mddev_suspend_and_lock(struct mddev *mddev,
> + unsigned int *noio_flags)
> {
> int ret;
>
> - ret = mddev_suspend(mddev, true);
> + ret = mddev_suspend(mddev, true, noio_flags);
> if (ret)
> return ret;
>
> ret = mddev_lock(mddev);
> if (ret)
> - mddev_resume(mddev);
> + mddev_resume(mddev, *noio_flags);
>
> return ret;
> }
>
> -static inline void mddev_suspend_and_lock_nointr(struct mddev *mddev)
> +static inline void mddev_suspend_and_lock_nointr(struct mddev *mddev,
> + unsigned int *noio_flags)
> {
> - mddev_suspend(mddev, false);
> + mddev_suspend(mddev, false, noio_flags);
> mddev_lock_nointr(mddev);
> }
>
> -static inline void mddev_unlock_and_resume(struct mddev *mddev)
> +static inline void mddev_unlock_and_resume(struct mddev *mddev,
> + unsigned int noio_flags)
> {
> mddev_unlock(mddev);
> - mddev_resume(mddev);
> + mddev_resume(mddev, noio_flags);
> }
>
> struct mdu_array_info_s;
> struct mdu_disk_info_s;
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 7b7546bfa21f..6f8e3a624456 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -693,13 +693,15 @@ static void r5c_disable_writeback_async(struct work_struct *work)
> !READ_ONCE(conf->log) ||
> !test_bit(MD_SB_CHANGE_PENDING, &mddev->sb_flags));
>
> log = READ_ONCE(conf->log);
> if (log) {
> - mddev_suspend(mddev, false);
> + unsigned int noio_flags;
> +
> + mddev_suspend(mddev, false, &noio_flags);
> log->r5c_journal_mode = R5C_JOURNAL_MODE_WRITE_THROUGH;
> - mddev_resume(mddev);
> + mddev_resume(mddev, noio_flags);
> }
> }
>
> static void r5l_submit_current_io(struct r5l_log *log)
> {
> @@ -2603,10 +2605,11 @@ EXPORT_SYMBOL(r5c_journal_mode_set);
> static ssize_t r5c_journal_mode_store(struct mddev *mddev,
> const char *page, size_t length)
> {
> int mode = ARRAY_SIZE(r5c_journal_mode_str);
> size_t len = length;
> + unsigned int noio_flags;
> int ret;
>
> if (len < 2)
> return -EINVAL;
>
> @@ -2615,15 +2618,15 @@ static ssize_t r5c_journal_mode_store(struct mddev *mddev,
>
> while (mode--)
> if (strlen(r5c_journal_mode_str[mode]) == len &&
> !strncmp(page, r5c_journal_mode_str[mode], len))
> break;
> - ret = mddev_suspend_and_lock(mddev);
> + ret = mddev_suspend_and_lock(mddev, &noio_flags);
> if (ret)
> return ret;
> ret = r5c_journal_mode_set(mddev, mode);
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> return ret ?: length;
> }
>
> struct md_sysfs_entry
> r5c_journal_mode = __ATTR(journal_mode, 0644,
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 65ae7d8930fc..6062c4b62cc8 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -6992,11 +6992,11 @@ raid5_show_stripe_size(struct mddev *mddev, char *page)
> #if PAGE_SIZE != DEFAULT_STRIPE_SIZE
> static ssize_t
> raid5_store_stripe_size(struct mddev *mddev, const char *page, size_t len)
> {
> struct r5conf *conf;
> - unsigned long new;
> + unsigned long new, noio_flags;
> int err;
> int size;
>
> if (len >= PAGE_SIZE)
> return -EINVAL;
> @@ -7011,11 +7011,11 @@ raid5_store_stripe_size(struct mddev *mddev, const char *page, size_t len)
> if (new % DEFAULT_STRIPE_SIZE != 0 ||
> new > PAGE_SIZE || new == 0 ||
> new != roundup_pow_of_two(new))
> return -EINVAL;
>
> - err = mddev_suspend_and_lock(mddev);
> + err = mddev_suspend_and_lock(mddev, &noio_flags);
> if (err)
> return err;
>
> conf = mddev->private;
> if (!conf) {
> @@ -7049,11 +7049,11 @@ raid5_store_stripe_size(struct mddev *mddev, const char *page, size_t len)
> err = -ENOMEM;
> }
> mutex_unlock(&conf->cache_size_mutex);
>
> out_unlock:
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> return err ?: len;
> }
>
> static struct md_sysfs_entry
> raid5_stripe_size = __ATTR(stripe_size, 0644,
> @@ -7127,19 +7127,20 @@ raid5_show_skip_copy(struct mddev *mddev, char *page)
> static ssize_t
> raid5_store_skip_copy(struct mddev *mddev, const char *page, size_t len)
> {
> struct r5conf *conf;
> unsigned long new;
> + unsigned int noio_flags;
> int err;
>
> if (len >= PAGE_SIZE)
> return -EINVAL;
> if (kstrtoul(page, 10, &new))
> return -EINVAL;
> new = !!new;
>
> - err = mddev_suspend_and_lock(mddev);
> + err = mddev_suspend_and_lock(mddev, &noio_flags);
> if (err)
> return err;
> conf = mddev->private;
> if (!conf)
> err = -ENODEV;
> @@ -7152,11 +7153,11 @@ raid5_store_skip_copy(struct mddev *mddev, const char *page, size_t len)
> lim.features |= BLK_FEAT_STABLE_WRITES;
> else
> lim.features &= ~BLK_FEAT_STABLE_WRITES;
> err = queue_limits_commit_update(q, &lim);
> }
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> return err ?: len;
> }
>
> static struct md_sysfs_entry
> raid5_skip_copy = __ATTR(skip_copy, S_IRUGO | S_IWUSR,
> @@ -7195,10 +7196,11 @@ static int alloc_thread_groups(struct r5conf *conf, int cnt,
> static ssize_t
> raid5_store_group_thread_cnt(struct mddev *mddev, const char *page, size_t len)
> {
> struct r5conf *conf;
> unsigned int new;
> + unsigned int noio_flags;
> int err;
> struct r5worker_group *new_groups, *old_groups;
> int group_cnt;
>
> if (len >= PAGE_SIZE)
> @@ -7207,16 +7209,16 @@ raid5_store_group_thread_cnt(struct mddev *mddev, const char *page, size_t len)
> return -EINVAL;
> /* 8192 should be big enough */
> if (new > 8192)
> return -EINVAL;
>
> - err = mddev_suspend_and_lock(mddev);
> + err = mddev_suspend_and_lock(mddev, &noio_flags);
> if (err)
> return err;
> conf = mddev->private;
> if (!conf) {
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> return -ENODEV;
> }
> raid5_quiesce(mddev, true);
>
> if (new != conf->worker_cnt_per_group) {
> @@ -7237,11 +7239,11 @@ raid5_store_group_thread_cnt(struct mddev *mddev, const char *page, size_t len)
> kfree(old_groups);
> }
> }
>
> raid5_quiesce(mddev, false);
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
>
> return err ?: len;
> }
>
> static struct md_sysfs_entry
> @@ -8940,18 +8942,19 @@ static void *raid6_takeover(struct mddev *mddev)
> }
>
> static int raid5_change_consistency_policy(struct mddev *mddev, const char *buf)
> {
> struct r5conf *conf;
> + unsigned int noio_flags;
> int err;
>
> - err = mddev_suspend_and_lock(mddev);
> + err = mddev_suspend_and_lock(mddev, &noio_flags);
> if (err)
> return err;
> conf = mddev->private;
> if (!conf) {
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
> return -ENODEV;
> }
>
> if (strncmp(buf, "ppl", 3) == 0) {
> /* ppl only works with RAID 5 */
> @@ -8990,11 +8993,11 @@ static int raid5_change_consistency_policy(struct mddev *mddev, const char *buf)
> }
>
> if (!err)
> md_update_sb(mddev, 1);
>
> - mddev_unlock_and_resume(mddev);
> + mddev_unlock_and_resume(mddev, noio_flags);
>
> return err;
> }
>
> static int raid5_start(struct mddev *mddev)
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v2] md: use READ_ONCE() for lockless reads of sb_flags
From: yu kuai @ 2026-07-05 10:55 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: linux-kernel
In-Reply-To: <20260623111617.2500313-1-chencheng@fnnas.com>
Hi,
在 2026/6/23 19:16, Chen Cheng 写道:
> From: Chen Cheng <chencheng@fnnas.com>
>
> sb_flags is checked without a lock in md, raid1, raid5, and raid10.
> KCSAN reports these reads as data races.
>
> The write side uses atomic bit ops.
> The read side still has plain loads in a few places.
>
> Use READ_ONCE() for the lockless reads of sb_flags.
>
>
> v1 -> v2:
> - Add lock-free read paths for other array levels.
>
> KCSAN reports #1:
> ======================================
>
> BUG: KCSAN: data-race in md_check_recovery / md_write_start
>
> write (marked) to 0xffff8e39f897f030 of 8 bytes by task 248146 on cpu 8:
> md_write_start+0x5dd/0x910
> raid10_make_request+0x9b/0x1080
> md_handle_request+0x4a2/0xa40
> [........]
>
> read to 0xffff8e39f897f030 of 8 bytes by task 250445 on cpu 11:
> md_check_recovery+0x574/0x900
> raid10d+0xb7/0x2950
> [........]
>
> KCSAN reports #2:
> ======================================
> BUG: KCSAN: data-race in md_check_recovery / md_write_start
>
> write (marked) to 0xffff8e39e953f030 of 8 bytes by task 540091 on cpu 11:
> md_write_start+0x5dd/0x910
> raid1_make_request+0x141/0x1990
> [........]
>
> read to 0xffff8e39e953f030 of 8 bytes by task 580822 on cpu 0:
> md_check_recovery+0x574/0x900
> raid1d+0xcc/0x3840
> [........]
>
> value changed: 0x0000000000000002 -> 0x0000000000000006
>
> KCSAN reports #3:
> ======================================
> BUG: KCSAN: data-race in md_check_recovery / md_do_sync.cold
>
> write (marked) to 0xffff8e39e9404030 of 8 bytes by task 492473 on cpu 6:
> md_do_sync.cold+0x3f6/0x1686
> [........]
>
> read to 0xffff8e39e9404030 of 8 bytes by task 492402 on cpu 3:
> md_check_recovery+0x16d/0x900
> raid1d+0xcc/0x3840
> [........]
>
> value changed: 0x0000000000000000 -> 0x0000000000000002
>
> KCSAN reports #4:
> ======================================
> BUG: KCSAN: data-race in md_do_sync.cold / raid5d
>
> write (marked) to 0xffff8e39c35cb030 of 8 bytes by task 192196 on cpu 10:
> md_do_sync.cold+0x3f6/0x1686
> md_thread+0x15a/0x2d0
> [........]
>
> read to 0xffff8e39c35cb030 of 8 bytes by task 190759 on cpu 5:
> raid5d+0x7f9/0xba0
> md_thread+0x15a/0x2d0
> [........]
>
> value changed: 0x0000000000000000 -> 0x0000000000000002
>
> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
> ---
> drivers/md/md.c | 8 ++++----
> drivers/md/raid1.c | 2 +-
> drivers/md/raid10.c | 4 ++--
> drivers/md/raid5.c | 7 ++++---
> 4 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 096bb64e87bd..c5c50640b684 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -6830,11 +6830,11 @@ int md_run(struct mddev *mddev)
> * via sysfs - until a lack of spares is confirmed.
> */
> set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);
>
> - if (mddev->sb_flags)
> + if (READ_ONCE(mddev->sb_flags))
> md_update_sb(mddev, 0);
>
> if (IS_ENABLED(CONFIG_MD_BITMAP) && !mddev->bitmap_info.file &&
> !mddev->bitmap_info.offset)
> md_bitmap_set_none(mddev);
> @@ -7024,11 +7024,11 @@ static void __md_stop_writes(struct mddev *mddev)
> mddev->bitmap_ops->flush(mddev);
> }
>
> if (md_is_rdwr(mddev) &&
> ((!mddev->in_sync && !mddev_is_clustered(mddev)) ||
> - mddev->sb_flags)) {
> + READ_ONCE(mddev->sb_flags))) {
> /* mark array as shutdown cleanly */
> if (!mddev_is_clustered(mddev))
> mddev->in_sync = 1;
> md_update_sb(mddev, 1);
> }
> @@ -10294,11 +10294,11 @@ static bool md_should_do_recovery(struct mddev *mddev)
> /*
> * MD_SB_CHANGE_PENDING indicates that the array is switching from clean to
> * active, and no action is needed for now.
> * All other MD_SB_* flags require to update the superblock.
> */
> - if (mddev->sb_flags & ~ (1<<MD_SB_CHANGE_PENDING))
> + if (READ_ONCE(mddev->sb_flags) & ~ (1<<MD_SB_CHANGE_PENDING))
Since this line is involved, please also fix checkpatch warning as well, like what
you did below.
> return true;
>
> /*
> * If the array is not using external metadata and there has been no data
> * written for some time, then the array's status needs to be set to
> @@ -10423,11 +10423,11 @@ void md_check_recovery(struct mddev *mddev)
> spin_lock(&mddev->lock);
> set_in_sync(mddev);
> spin_unlock(&mddev->lock);
> }
>
> - if (mddev->sb_flags)
> + if (READ_ONCE(mddev->sb_flags))
> md_update_sb(mddev, 0);
>
> /*
> * Never start a new sync thread if MD_RECOVERY_RUNNING is
> * still set.
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 29b58583e381..bd6808656edb 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2738,11 +2738,11 @@ static void raid1d(struct md_thread *thread)
> handle_read_error(conf, r1_bio);
> else
> WARN_ON_ONCE(1);
>
> cond_resched();
> - if (mddev->sb_flags & ~(1<<MD_SB_CHANGE_PENDING))
> + if (READ_ONCE(mddev->sb_flags) & ~(1 << MD_SB_CHANGE_PENDING))
> md_check_recovery(mddev);
> }
> blk_finish_plug(&plug);
> }
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index adaf9e432e25..3ffa5a19964d 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -3030,11 +3030,11 @@ static void raid10d(struct md_thread *thread)
> handle_read_error(mddev, r10_bio);
> else
> WARN_ON_ONCE(1);
>
> cond_resched();
> - if (mddev->sb_flags & ~(1<<MD_SB_CHANGE_PENDING))
> + if (READ_ONCE(mddev->sb_flags) & ~(1 << MD_SB_CHANGE_PENDING))
> md_check_recovery(mddev);
> }
> blk_finish_plug(&plug);
> }
>
> @@ -4698,11 +4698,11 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
> else
> mddev->curr_resync_completed = conf->reshape_progress;
> conf->reshape_checkpoint = jiffies;
> set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
> md_wakeup_thread(mddev->thread);
> - wait_event(mddev->sb_wait, mddev->sb_flags == 0 ||
> + wait_event(mddev->sb_wait, READ_ONCE(mddev->sb_flags) == 0 ||
> test_bit(MD_RECOVERY_INTR, &mddev->recovery));
> if (test_bit(MD_RECOVERY_INTR, &mddev->recovery)) {
> allow_barrier(conf);
> return sectors_done;
> }
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index ded6a69f7795..cb58b4353995 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -1219,11 +1219,11 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
> break;
>
> if (bad < 0) {
> set_bit(BlockedBadBlocks, &rdev->flags);
> if (!conf->mddev->external &&
> - conf->mddev->sb_flags) {
> + READ_ONCE(conf->mddev->sb_flags)) {
> /* It is very unlikely, but we might
> * still need to write out the
> * bad block log - better give it
> * a chance*/
> md_check_recovery(conf->mddev);
> @@ -6469,11 +6469,11 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr, int *sk
> rdev->recovery_offset = sector_nr;
>
> conf->reshape_checkpoint = jiffies;
> set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
> md_wakeup_thread(mddev->thread);
> - wait_event(mddev->sb_wait, mddev->sb_flags == 0 ||
> + wait_event(mddev->sb_wait, READ_ONCE(mddev->sb_flags) == 0 ||
> test_bit(MD_RECOVERY_INTR, &mddev->recovery));
> if (test_bit(MD_RECOVERY_INTR, &mddev->recovery))
> return 0;
> spin_lock_irq(&conf->device_lock);
> conf->reshape_safe = mddev->reshape_position;
> @@ -6913,11 +6913,12 @@ static void raid5d(struct md_thread *thread)
> conf->temp_inactive_list);
> if (!batch_size && !released)
> break;
> handled += batch_size;
>
> - if (mddev->sb_flags & ~(1 << MD_SB_CHANGE_PENDING)) {
> + if (READ_ONCE(mddev->sb_flags) &
> + ~(1 << MD_SB_CHANGE_PENDING)) {
> spin_unlock_irq(&conf->device_lock);
> md_check_recovery(mddev);
> spin_lock_irq(&conf->device_lock);
> }
> }
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid1: protect sequential read hints for read balance
From: yu kuai @ 2026-07-05 9:31 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: linux-kernel
In-Reply-To: <20260623075940.2476255-1-chencheng@fnnas.com>
在 2026/6/23 15:59, Chen Cheng 写道:
> The patch just suppress KCSAN noise. No functional change.
>
> KCSAN reports a race, point to update_read_sectors() update next_seq_sect vs.
> read next_seq_sect.
>
> Protect next_seq_sect and seq_start with READ_ONCE/WRITE_ONCE, otherwise,
> read balance see stale sequential-read hints.
>
> KCSAN report:
> ==============
> BUG: KCSAN: data-race in raid1_read_request / raid1_read_request
>
> write to 0xffff8e3a2d6736d0 of 8 bytes by task 593784 on cpu 10:
> raid1_read_request+0xe5a/0x19f0
> raid1_make_request+0xdf/0x1990
> md_handle_request+0x4a2/0xa40
> [...]
>
> read to 0xffff8e3a2d6736d0 of 8 bytes by task 593776 on cpu 11:
> raid1_read_request+0xe3f/0x19f0
> raid1_make_request+0xdf/0x1990
> md_handle_request+0x4a2/0xa40
> [...]
>
> value changed: 0x0000000000356368 -> 0x0000000000356370
>
> Signed-off-by: Chen Cheng<chencheng@fnnas.com>
> ---
> drivers/md/raid1.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
Reviewed-by: Yu Kuai <yukuai@fygo.io>
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v2] md/raid5-ppl: fix use-after-free in ppl_do_flush()
From: yu kuai @ 2026-07-05 9:02 UTC (permalink / raw)
To: Sajal Gupta, linux-raid, song
Cc: tomasz.majchrzak, linux-kernel, error27, skhan, me,
linux-kernel-mentees, yukuai
In-Reply-To: <20260622142146.56637-1-sajal2005gupta@gmail.com>
在 2026/6/22 22:06, Sajal Gupta 写道:
> The loop in ppl_do_flush() continues iterating after calling
> ppl_io_unit_finished(), touching io->pending_flushes and leading to a
> use-after-free.
>
> Add a break statement to stop the loop once io is freed.
>
> Fixes: 1532d9e87e8b ("raid5-ppl: PPL support for disks with write-back cache enabled")
> Reported-by: Dan Carpenter<error27@gmail.com>
> Closes:https://lore.kernel.org/all/ajJF2wKYWRk4GGCK@stanley.mountain/
> Signed-off-by: Sajal Gupta<sajal2005gupta@gmail.com>
> ---
> Compile tested only.
>
> Changes in v2:
> - drop the refcount_t conversion
>
> v1:https://lore.kernel.org/all/20260622080656.22786-1-sajal2005gupta@gmail.com/
>
> drivers/md/raid5-ppl.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Looks correct.
Reviewed-by: Yu Kuai <yukuai@fygo.io>
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v2] md/raid5: protect bitmap batch counters aka seq_flush/seq_write consistency
From: yu kuai @ 2026-07-05 8:50 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: chenchneg33, linux-kernel
In-Reply-To: <20260617142839.882378-1-chencheng@fnnas.com>
在 2026/6/17 22:28, Chen Cheng 写道:
> From: Chen Cheng<chencheng@fnnas.com>
>
> kcsan detect race :
> - raid5d() closes the current bitmap batch by updating
> conf->seq_flush under conf->device_lock.
> - __add_stripe_bio() read conf->seq_flush without that
> lock when assigning sh->bm_seq.
>
> so, protect seq_flush/seq_write consistency for multiple CPUs by
> READ_ONCE()/WRITE_ONCE() under the path without held device_lock.
>
> re-explain the stripe batch sequence number update flow:
> 1. sh->bm_seq declare which batch number the stripe belongs to
> when perform bitmap-related write.
> ==> bm_seq = seq_flush+1
>
> 2. stripe be handled,
> * if sh->bm_seq - conf->seq_write > 0, means the
> batch stripes **newer than** the last written
> batch, it cannot proceed yet, queued on bitmap_list.
> * otherwise , has already proceed.
>
> 3. raid5d() `++seq_flush` to closes the current batch, means
> * no more stripes join that old batch
> * just-closed batch ready to write-out to disk
>
> 4. raid5d() calls bitmap hooks unplug() or writeout, then,
> `++seq_write` to the same as bm_seq.
>
> - seq_flush - for producer, to close batches.
> - seq_write - for consumer, the checkpoint number.
>
> the report:
> ====================================
> BUG: KCSAN: data-race in __add_stripe_bio / raid5d
>
> write to 0xffff88ba5625d470 of 4 bytes by task 82401 on cpu 0:
> raid5d+0x1d9/0xba0
> [.....]
>
> read to 0xffff88ba5625d470 of 4 bytes by task 82421 on cpu 8:
> __add_stripe_bio+0x332/0x400
> raid5_make_request+0x6ac/0x2930
> md_handle_request+0x4a2/0xa40
> md_submit_bio+0x109/0x1a0
> __submit_bio+0x2ec/0x390
> [.....]
>
> v1 -> v2:
> - remove WRITE_ONCE(conf->seq_write) in held device_lock path.
> - remove READ_ONCE(conf->seq_flush) in held device_lock path.
>
> Signed-off-by: Chen Cheng<chencheng@fnnas.com>
> ---
> drivers/md/raid5.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
Reviewed-by: Yu Kuai <yukuai@fygo.io>
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v6 3/3] md/raid10: free r10bio before ending master_bio in raid_end_bio_io() and raid_end_discard_bio()
From: yu kuai @ 2026-07-05 7:48 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: linux-kernel
In-Reply-To: <20260623123840.2521340-4-chencheng@fnnas.com>
Hi,
在 2026/6/23 20:38, Chen Cheng 写道:
> From: Chen Cheng <chencheng@fnnas.com>
>
> origin flow:
>
> bio_endio(master_bio); /* may drop active_io to zero */
> allow_barrier(conf);
> free_r10bio(r10_bio); /* reads conf->geo, returns to pool */
>
> one scenario is:
>
> CPU A (softirq, raid_end_bio_io) CPU B (action_store) --> reshape
> ================================ ===============================
> bio_endio(master_bio)
> md_end_clone_io
> percpu_ref_put -> 0
> wait_event wakeup, and,
> mddev_suspend return
> raid10_start_reshape:
> setup_geo(&conf->geo, new)
> ...
> mempool_destroy(old_pool)
> conf->r10bio_pool = new_pool
> allow_barrier(conf)
> free_r10bio(r10_bio)
> put_all_bios:
> for (i=0; i<conf->geo.raid_disks; i++)
> ==> old obj, new geo, OOB
> mempool_free(r10_bio, conf->r10bio_pool)
> ==> old-geometry obj freed into new pool
>
> so .. fix by reorder the flow:
>
> free_r10bio(r10_bio)
> allow_barrier(conf)
> bio_endio(master_io)
>
> raid_end_discard_bio() is exactly the same.
>
> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
> ---
> drivers/md/raid10.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index d740744a9746..e44a9b6e95c7 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -330,24 +330,25 @@ static void reschedule_retry(struct r10bio *r10_bio)
> */
> static void raid_end_bio_io(struct r10bio *r10_bio)
> {
> struct bio *bio = r10_bio->master_bio;
> struct r10conf *conf = r10_bio->mddev->private;
> + unsigned long state = r10_bio->state;
>
> - if (!test_and_set_bit(R10BIO_Returned, &r10_bio->state)) {
> - if (!test_bit(R10BIO_Uptodate, &r10_bio->state))
> - bio->bi_status = BLK_STS_IOERR;
> - bio_endio(bio);
> - }
> + free_r10bio(r10_bio);
>
> /*
> * Wake up any possible resync thread that waits for the device
> * to go idle.
> */
> allow_barrier(conf);
>
> - free_r10bio(r10_bio);
> + if (!test_and_set_bit(R10BIO_Returned, &state)) {
> + if (!test_bit(R10BIO_Uptodate, &state))
> + bio->bi_status = BLK_STS_IOERR;
> + bio_endio(bio);
> + }
Why is this moved to the end? I feel it's not necessary.
> }
>
> /*
> * Update disk head position estimator based on IRQ completion info.
> */
> @@ -1580,13 +1581,15 @@ static void raid_end_discard_bio(struct r10bio *r10bio)
> if (!test_bit(R10BIO_Discard, &r10bio->state)) {
> first_r10bio = (struct r10bio *)r10bio->master_bio;
> free_r10bio(r10bio);
> r10bio = first_r10bio;
> } else {
> + struct bio *master_bio = r10bio->master_bio;
> +
> md_write_end(r10bio->mddev);
> - bio_endio(r10bio->master_bio);
> free_r10bio(r10bio);
> + bio_endio(master_bio);
> break;
> }
> }
> }
>
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v6 1/3] md: suspend array when sync_action=reshape
From: yu kuai @ 2026-07-05 7:43 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: linux-kernel
In-Reply-To: <20260623123840.2521340-2-chencheng@fnnas.com>
Hi，
在 2026/6/23 20:38, Chen Cheng 写道:
> From: Chen Cheng <chencheng@fnnas.com>
>
> raid10 needs to resize/swap r10bio_pool when reshape changes
> raid_disks, and, don't let new requests keep allocating r10bio
> objects from the old pool while that transition is in progress.
>
> suspend and lock array before mddev_start_reshape(), and resume
> it on exit.
>
> Other sync_action ops are unchanged.
>
> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
> ---
> drivers/md/md.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 096bb64e87bd..1377c407614c 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5261,25 +5261,29 @@ action_store(struct mddev *mddev, const char *page, size_t len)
> enum sync_action action;
>
> if (!mddev->pers || !mddev->pers->sync_request)
> return -EINVAL;
>
> + action = md_sync_action_by_name(page);
> retry:
> if (work_busy(&mddev->sync_work))
> flush_work(&mddev->sync_work);
>
> - ret = mddev_lock(mddev);
> + ret = (action == ACTION_RESHAPE) ?
> + mddev_suspend_and_lock(mddev) :
> + mddev_lock(mddev);
I already suggestted in previous verions that suspend for retry is too
heavy, please suspend before retry.
> if (ret)
> return ret;
>
> if (work_busy(&mddev->sync_work)) {
> - mddev_unlock(mddev);
> + if (action == ACTION_RESHAPE)
> + mddev_unlock_and_resume(mddev);
> + else
> + mddev_unlock(mddev);
> goto retry;
> }
>
> - action = md_sync_action_by_name(page);
> -
> /* TODO: mdadm rely on "idle" to start sync_thread. */
> if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
> switch (action) {
> case ACTION_FROZEN:
> md_frozen_sync_thread(mddev);
> @@ -5344,11 +5348,14 @@ action_store(struct mddev *mddev, const char *page, size_t len)
> md_wakeup_thread(mddev->thread);
> sysfs_notify_dirent_safe(mddev->sysfs_action);
> ret = len;
>
> out:
> - mddev_unlock(mddev);
> + if (action == ACTION_RESHAPE)
> + mddev_unlock_and_resume(mddev);
> + else
> + mddev_unlock(mddev);
> return ret;
> }
>
> static struct md_sysfs_entry md_scan_mode =
> __ATTR_PREALLOC(sync_action, S_IRUGO|S_IWUSR, action_show, action_store);
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v2 2/7] md/raid1: advertise atomic write limits and handle runtime constraints
From: John Garry @ 2026-07-03 8:28 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, yukuai, magiclinan, xiao, axboe,
vverma, martin.petersen, linux-kernel
Cc: linux-raid
In-Reply-To: <m2se64zak9.fsf@gmail.com>
On 30/06/2026 09:39, Abd-Alrhman Masalkhi wrote:
>
> Hi John,
>
> On Mon, Jun 29, 2026 at 15:48 +0100, John Garry wrote:
>> On 28/06/2026 15:24, Abd-Alrhman Masalkhi wrote:
>>> Atomic writes in RAID1 must fit within a single barrier unit. Advertise
>>> this restriction through the queue limits by setting
>>> atomic_write_hw_unit_max to BARRIER_UNIT_SECTOR_SIZE so that bios which
>>> would cross a barrier-unit boundary are rejected by the block layer
>>> before reaching MD.
>>>
>>> A bio that passes block-layer validation may still become unserviceable
>>> within RAID1 due to bad blocks or write-behind constraints. In the former
>>> case, complete the bio with EIO. In the latter case, disable
>>> write-behind rather than failing the bio with EIO.
>>>
>>> Fixes: f2a38abf5f1c ("md/raid1: Atomic write support")
>>> Fixes: a4c55c902670 ("md/raid1: simplify raid1_write_request() error handling")
>>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>>> ---
>>> Changes in v2:
>>> - Drop the early atomic write split check from raid1_write_request().
>>> - Advertise the atomic write size limit via queue limits.
>>> - Disable write-behind instead of failing atomic writes when the
>>> BIO_MAX_VECS limit is encountered.
>>> - Link to v1: https://urldefense.com/v3/__https://lore.kernel.org/linux-raid/20260623072456.333437-3-abd.masalkhi@gmail.com/__;!!ACWV5N9M2RV99hQ!LbMSGSClRi0PNBqQti5ZNWGDVjDd34-7saYEAwNyBNjpNTjEA7veqM5RHG8KB1QiscarW4UaIefjm19ywSImtIgh$
>>> ---
>>> drivers/md/raid1.c | 36 +++++++++++++++++++-----------------
>>> 1 file changed, 19 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>>> index afe2ca96ad8c..f322048ab3c2 100644
>>> --- a/drivers/md/raid1.c
>>> +++ b/drivers/md/raid1.c
>>> @@ -1522,6 +1522,7 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>>> int first_clone;
>>> bool write_behind = false;
>>> bool nowait = bio->bi_opf & REQ_NOWAIT;
>>> + bool atomic = bio->bi_opf & REQ_ATOMIC;
>>> bool is_discard = op_is_discard(bio->bi_opf);
>>> sector_t sector = bio->bi_iter.bi_sector;
>>>
>>> @@ -1603,20 +1604,6 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>>> }
>>> if (is_bad) {
>>> int good_sectors;
>>> -
>>> - /*
>>> - * We cannot atomically write this, so just
>>> - * error in that case. It could be possible to
>>> - * atomically write other mirrors, but the
>>> - * complexity of supporting that is not worth
>>> - * the benefit.
>>> - */
>>> - if (bio->bi_opf & REQ_ATOMIC) {
>>> - bio->bi_status = BLK_STS_NOTSUPP;
>>> - bio_endio(bio);
>>> - goto err_dec_pending;
>>> - }
>>> -
>>> good_sectors = first_bad - sector;
>>> if (good_sectors < max_sectors)
>>> max_sectors = good_sectors;
>>> @@ -1633,10 +1620,24 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>>> * at a time and thus needs a new bio that can fit the whole payload
>>> * this bio in page sized chunks.
>>> */
>>> - if (write_behind && mddev->bitmap)
>>> - max_sectors = min_t(int, max_sectors,
>>> - BIO_MAX_VECS * (PAGE_SIZE >> 9));
>>> + if (write_behind && mddev->bitmap) {
>>> + if (atomic && max_sectors > BIO_MAX_VECS * (PAGE_SIZE >> 9))
>>
>> where does BIO_MAX_VECS * (PAGE_SIZE >> 9) even come from?
>>
>
> BIO_MAX_VECS * (PAGE_SIZE >> 9) defines the maximum size supported by
> write-behind. The write-behind copy (alloc_behind_master_bio) uses a
> single bio, which can hold at most BIO_MAX_VECS pages, making this the
> largest payload it can carry. With a 4 KiB PAGE_SIZE, that corresponds
> to 256 pages, or 1 MiB (2048 sectors).
> This patch changes the behavior for atomic writes that exceed this
> limit. Instead of failing the write with -EIO when the number of sectors
> must be reduced, it disables write-behind and proceeds with the atomic
> write.
>
>>> + /*
>>> + * Atomic writes cannot be split, so disable
>>> + * write-behind.
>>> + */
>>> + write_behind = false;
It's a bit poor to have write_behind initially = false, then allow it to
be set = true, and then later be set = false. Can this be improved?
>>> + else
>>> + max_sectors = min_t(int, max_sectors,
>>> + BIO_MAX_VECS * (PAGE_SIZE >> 9));
>>> + }
>>> +
>>> if (max_sectors < bio_sectors(bio)) {
>>> + if (atomic) {
>>> + bio_io_error(bio);
>>> + goto err_dec_pending;
>>> + }
>>> +
>>> bio = bio_submit_split_bioset(bio, max_sectors,
>>> &conf->bio_split);
>>> if (!bio)
>>> @@ -3229,6 +3230,7 @@ static int raid1_set_limits(struct mddev *mddev)
>>> lim.max_write_zeroes_sectors = 0;
>>> lim.max_hw_wzeroes_unmap_sectors = 0;
>>> lim.logical_block_size = mddev->logical_block_size;
>>> + lim.atomic_write_hw_unit_max = BARRIER_UNIT_SECTOR_SIZE;
>>
>> This BARRIER_UNIT_SECTOR_SIZE is a bit like chunk sectors, no? I am just
>> wondering if we just should set it to chunk sectors =
>> BARRIER_UNIT_SECTOR_SIZE
>>
>> I assume that it affects more than Reads and writes, e.g. discard also.
>>
>
> BARRIER_UNIT_SECTOR_SIZE is the resync barrier-bucket, not the layout
> chunk size. Unless I'm missing something, using
> atomic_write_hw_unit_max seems more appropriate than using the chunk
> size. That way, the limit only applies to atomic writes instead of
> affecting other operations such.
well it seems to be that everything in the driver is split over
BARRIER_UNIT_SECTOR_SIZE, so is in effect a chunk size.
Note that atomic_write_hw_unit_max is going to be small always compared
to BARRIER_UNIT_SECTOR_SIZE.
However, can you check the blk stacking code to make sure that this does
as you want? As I remember, for stacking we take the atomic write limits
of the first bottom device and then stack the other bottom devices and I
don't think that setting atomic_write_hw_unit_max in this way has an
impact - see blk_stack_atomic_writes_limits()
>
>>> lim.features |= BLK_FEAT_ATOMIC_WRITES;
>>> lim.features |= BLK_FEAT_PCI_P2PDMA;
>>> err = mddev_stack_rdev_limits(mddev, &lim, MDDEV_STACK_INTEGRITY);
>>
>
^ permalink raw reply
* Re: [PATCH 1/8] x86/fpu: Check for missing AVX and AVX-512 xstate bits
From: Borislav Petkov @ 2026-07-01 0:49 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Eric Biggers, linux-raid, yukuai, LKML
In-Reply-To: <20260626050038.2AD661F000E9@smtp.kernel.org>
On Fri, Jun 26, 2026 at 05:00:37AM +0000, sashiko-bot@kernel.org wrote:
> [Severity: Critical]
> This is a pre-existing issue, but Memory Protection Keys (PKU) isolation
> can be bypassed because we don't verify X86_FEATURE_PKU against the
> presence of XFEATURE_PKRU in xstate here.
>
> If a hypervisor reports X86_FEATURE_PKU in CPUID but masks out the
> XFEATURE_PKRU xstate component, the kernel's setup_pku() enables CR4.PKE
> and forces X86_FEATURE_OSPKE. Because X86_FEATURE_OSPKE is artificially
> forced, the feature is not cleared.
>
> CR4.PKE=1 permits userspace to use WRPKRU, but because XCR0.PKRU=0,
> xsaves/xrstors silently ignore the PKRU register during task context
> switches, leading to state leakage across tasks.
>
> Should we also validate PKU against missing xstate components in this
> function to prevent isolation bypass?
I *think* Sashiko means here our preexisting fpu__init_system_xstate() here
but I'm not sure...
> [Severity: High]
> Does this logic get bypassed if XSAVE or FPU support is missing?
>
> Earlier in fpu__init_system_xstate(), there are early returns if
> X86_FEATURE_FPU or X86_FEATURE_XSAVE are missing:
>
> if (!boot_cpu_has(X86_FEATURE_XSAVE)) {
> pr_info("x86/fpu: x87 FPU will use %s\n",
> boot_cpu_has(X86_FEATURE_FXSR) ? "FXSAVE" : "FSAVE");
> return;
> }
>
> If a hypervisor natively enumerates AVX=1 but XSAVE=0 (such as QEMU
> with -cpu host,-xsave), check_cpufeature_deps() only warns about unmet
Yah, we don't care. We won't support non-sensical guest configs.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH] md: avoid modifying spares while the array is not suspended
From: sashiko-bot @ 2026-06-30 10:18 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: linux-raid, yukuai
In-Reply-To: <20260630075640.1081634-1-abd.masalkhi@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] The patch prevents the recovery thread from starting or resuming for existing rebuilding spares, silently abandoning recovery and leaving the array permanently degraded.
Pre-existing issues:
- [High] The patch addresses a TOCTOU race for `state_store` but misses other vulnerable paths like `slot_store` and cluster updates, which still call `remove_and_add_spares()` without array suspension.
--
commit 16cf64f20d25bc671dc52422ec0a44672674e524
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
md: avoid modifying spares while the array is not suspended
This commit ensures that remove_spares() and remove_and_add_spares() are
only called after the array has been suspended. This prevents a race
condition where normal I/O could trigger a NULL pointer dereference when
rdevs are removed or replaced concurrently.
Closes: https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d1465bcd86c81..f555eb5aca5d8 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -10151,7 +10154,11 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
> * also removed and re-added, to allow the personality to fail the
> * re-add.
> */
> - *spares = remove_and_add_spares(mddev, NULL);
> + if (array_suspended)
> + *spares = remove_and_add_spares(mddev, NULL);
> + else
> + *spares = 0;
> +
[Severity: High]
Does this else branch prevent the recovery thread from starting or resuming
for existing rebuilding spares?
In md_start_sync(), suspend is determined by checking
md_spares_need_change(mddev). For an existing recovering spare receiving
normal I/O (where nr_pending > 0), both rdev_removeable() and
rdev_addable() can evaluate to false. This causes md_spares_need_change()
to return false, leaving suspend as false.
When md_choose_sync_action() is called with array_suspended == false, this
new else branch forces *spares = 0. By completely bypassing the counting of
existing recovering spares, md_choose_sync_action() will return false and
skip starting the sync thread.
Since nr_pending dropping to zero on a non-faulty spare doesn't trigger a
new MD_RECOVERY_NEEDED event, could this silently abandon recovery and
leave the array permanently degraded?
[Severity: High]
This is a pre-existing issue, but does restricting remove_and_add_spares()
calls to suspended arrays in md_choose_sync_action() leave other
vulnerable paths exposed?
The commit addresses a TOCTOU race by ensuring remove_and_add_spares() is
only called when the array is suspended, preventing concurrent I/O from
encountering a cleared replacement pointer.
However, slot_store() can still call remove_and_add_spares() without an
array suspension when a user writes "none" (slot == -1):
drivers/md/md.c:slot_store() {
...
clear_bit(Blocked, &rdev->flags);
remove_and_add_spares(rdev->mddev, rdev);
if (rdev->raid_disk >= 0)
return -EBUSY;
...
}
Similarly, the MD cluster component updates the array configuration in
check_sb_changes() by calling remove_and_add_spares() while the array is
not suspended.
Could normal I/O still increment nr_pending on a replacement drive just
before remove_spares() removes the faulty drive and clears the replacement
pointer in these paths, reproducing the exact TOCTOU crash this patch
aims to fix?
> @@ -10196,11 +10203,12 @@ static void md_start_sync(struct work_struct *ws)
> * As we only add devices that are already in-sync, we can
> * activate the spares immediately.
> */
> - remove_and_add_spares(mddev, NULL);
> + if (suspend)
> + remove_and_add_spares(mddev, NULL);
> goto not_running;
> }
>
> - if (!md_choose_sync_action(mddev, &spares))
> + if (!md_choose_sync_action(mddev, &spares, suspend))
> goto not_running;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260630075640.1081634-1-abd.masalkhi@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH v2 2/7] md/raid1: advertise atomic write limits and handle runtime constraints
From: Abd-Alrhman Masalkhi @ 2026-06-30 8:39 UTC (permalink / raw)
To: John Garry, song, yukuai, magiclinan, xiao, axboe, vverma,
martin.petersen, linux-kernel
Cc: linux-raid
In-Reply-To: <7cbaaca3-4eb5-434d-a13f-f9574c9f977b@oracle.com>
Hi John,
On Mon, Jun 29, 2026 at 15:48 +0100, John Garry wrote:
> On 28/06/2026 15:24, Abd-Alrhman Masalkhi wrote:
>> Atomic writes in RAID1 must fit within a single barrier unit. Advertise
>> this restriction through the queue limits by setting
>> atomic_write_hw_unit_max to BARRIER_UNIT_SECTOR_SIZE so that bios which
>> would cross a barrier-unit boundary are rejected by the block layer
>> before reaching MD.
>>
>> A bio that passes block-layer validation may still become unserviceable
>> within RAID1 due to bad blocks or write-behind constraints. In the former
>> case, complete the bio with EIO. In the latter case, disable
>> write-behind rather than failing the bio with EIO.
>>
>> Fixes: f2a38abf5f1c ("md/raid1: Atomic write support")
>> Fixes: a4c55c902670 ("md/raid1: simplify raid1_write_request() error handling")
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> Changes in v2:
>> - Drop the early atomic write split check from raid1_write_request().
>> - Advertise the atomic write size limit via queue limits.
>> - Disable write-behind instead of failing atomic writes when the
>> BIO_MAX_VECS limit is encountered.
>> - Link to v1: https://urldefense.com/v3/__https://lore.kernel.org/linux-raid/20260623072456.333437-3-abd.masalkhi@gmail.com/__;!!ACWV5N9M2RV99hQ!LbMSGSClRi0PNBqQti5ZNWGDVjDd34-7saYEAwNyBNjpNTjEA7veqM5RHG8KB1QiscarW4UaIefjm19ywSImtIgh$
>> ---
>> drivers/md/raid1.c | 36 +++++++++++++++++++-----------------
>> 1 file changed, 19 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index afe2ca96ad8c..f322048ab3c2 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -1522,6 +1522,7 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>> int first_clone;
>> bool write_behind = false;
>> bool nowait = bio->bi_opf & REQ_NOWAIT;
>> + bool atomic = bio->bi_opf & REQ_ATOMIC;
>> bool is_discard = op_is_discard(bio->bi_opf);
>> sector_t sector = bio->bi_iter.bi_sector;
>>
>> @@ -1603,20 +1604,6 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>> }
>> if (is_bad) {
>> int good_sectors;
>> -
>> - /*
>> - * We cannot atomically write this, so just
>> - * error in that case. It could be possible to
>> - * atomically write other mirrors, but the
>> - * complexity of supporting that is not worth
>> - * the benefit.
>> - */
>> - if (bio->bi_opf & REQ_ATOMIC) {
>> - bio->bi_status = BLK_STS_NOTSUPP;
>> - bio_endio(bio);
>> - goto err_dec_pending;
>> - }
>> -
>> good_sectors = first_bad - sector;
>> if (good_sectors < max_sectors)
>> max_sectors = good_sectors;
>> @@ -1633,10 +1620,24 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>> * at a time and thus needs a new bio that can fit the whole payload
>> * this bio in page sized chunks.
>> */
>> - if (write_behind && mddev->bitmap)
>> - max_sectors = min_t(int, max_sectors,
>> - BIO_MAX_VECS * (PAGE_SIZE >> 9));
>> + if (write_behind && mddev->bitmap) {
>> + if (atomic && max_sectors > BIO_MAX_VECS * (PAGE_SIZE >> 9))
>
> where does BIO_MAX_VECS * (PAGE_SIZE >> 9) even come from?
>
BIO_MAX_VECS * (PAGE_SIZE >> 9) defines the maximum size supported by
write-behind. The write-behind copy (alloc_behind_master_bio) uses a
single bio, which can hold at most BIO_MAX_VECS pages, making this the
largest payload it can carry. With a 4 KiB PAGE_SIZE, that corresponds
to 256 pages, or 1 MiB (2048 sectors).
This patch changes the behavior for atomic writes that exceed this
limit. Instead of failing the write with -EIO when the number of sectors
must be reduced, it disables write-behind and proceeds with the atomic
write.
>> + /*
>> + * Atomic writes cannot be split, so disable
>> + * write-behind.
>> + */
>> + write_behind = false;
>> + else
>> + max_sectors = min_t(int, max_sectors,
>> + BIO_MAX_VECS * (PAGE_SIZE >> 9));
>> + }
>> +
>> if (max_sectors < bio_sectors(bio)) {
>> + if (atomic) {
>> + bio_io_error(bio);
>> + goto err_dec_pending;
>> + }
>> +
>> bio = bio_submit_split_bioset(bio, max_sectors,
>> &conf->bio_split);
>> if (!bio)
>> @@ -3229,6 +3230,7 @@ static int raid1_set_limits(struct mddev *mddev)
>> lim.max_write_zeroes_sectors = 0;
>> lim.max_hw_wzeroes_unmap_sectors = 0;
>> lim.logical_block_size = mddev->logical_block_size;
>> + lim.atomic_write_hw_unit_max = BARRIER_UNIT_SECTOR_SIZE;
>
> This BARRIER_UNIT_SECTOR_SIZE is a bit like chunk sectors, no? I am just
> wondering if we just should set it to chunk sectors =
> BARRIER_UNIT_SECTOR_SIZE
>
> I assume that it affects more than Reads and writes, e.g. discard also.
>
BARRIER_UNIT_SECTOR_SIZE is the resync barrier-bucket, not the layout
chunk size. Unless I'm missing something, using
atomic_write_hw_unit_max seems more appropriate than using the chunk
size. That way, the limit only applies to atomic writes instead of
affecting other operations such.
>> lim.features |= BLK_FEAT_ATOMIC_WRITES;
>> lim.features |= BLK_FEAT_PCI_P2PDMA;
>> err = mddev_stack_rdev_limits(mddev, &lim, MDDEV_STACK_INTEGRITY);
>
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* [PATCH] md: avoid modifying spares while the array is not suspended
From: Abd-Alrhman Masalkhi @ 2026-06-30 7:56 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao
Cc: linux-raid, linux-kernel, Abd-Alrhman Masalkhi, sashiko-bot
remove_spares() and remove_and_add_spares() modify the array's rdev
configuration. These operations are only safe after the array has been
suspended.
Today, md_start_sync() can call md_choose_sync_action() even when the
array has not been suspended. As a result, md_choose_sync_action() may
remove or replace rdevs while normal I/O is still accessing them.
The race can occur as follows:
raid10d Worker Normal IO
____________ _______________________ ______________________
raid10_write_request()
wait_blocked_dev()
set Blocked
set Faulty
Skip Faulty rdev
rrdev->nr_pending++
.repl_bio = bio
removeable_rdev = false .
array not suspended .
lock mddev goto err_handle
lock mddev (wait)
.
update sb .
clear Blocked .
.
unlock mddev .
lock mddev (acquires)
remove_spares()
removeable_rdev = true
raid10_remove_disk()
rdev = replacement
replacement = NULL
rdev_dec_pending(NULL)
unlock mddev (NULL)->nr_pending--
In this case, rdev_dec_pending() is called with a NULL pointer,
resulting in a NULL pointer dereference when attempting to decrement
nr_pending.
Fix this by ensuring that remove_spares() and remove_and_add_spares()
are only called after the array has been suspended, preventing
concurrent rdev configuration changes while normal I/O is in progress.
Fixes: bc08041b32ab ("md: suspend array in md_start_sync() if array need reconfiguration")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
drivers/md/md.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 66a41d482e59..c85ebb59535b 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -10116,7 +10116,8 @@ static int remove_and_add_spares(struct mddev *mddev,
return spares;
}
-static bool md_choose_sync_action(struct mddev *mddev, int *spares)
+static bool md_choose_sync_action(struct mddev *mddev, int *spares,
+ bool array_suspended)
{
/* Check if reshape is in progress first. */
if (mddev->reshape_position != MaxSector) {
@@ -10132,7 +10133,9 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
/* Check if resync is in progress. */
if (mddev->resync_offset < MaxSector) {
- remove_spares(mddev, NULL);
+ if (array_suspended)
+ remove_spares(mddev, NULL);
+
set_bit(MD_RECOVERY_SYNC, &mddev->recovery);
clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
clear_bit(MD_RECOVERY_LAZY_RECOVER, &mddev->recovery);
@@ -10144,7 +10147,11 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
* also removed and re-added, to allow the personality to fail the
* re-add.
*/
- *spares = remove_and_add_spares(mddev, NULL);
+ if (array_suspended)
+ *spares = remove_and_add_spares(mddev, NULL);
+ else
+ *spares = 0;
+
if (*spares || test_bit(MD_RECOVERY_LAZY_RECOVER, &mddev->recovery)) {
clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
@@ -10189,11 +10196,12 @@ static void md_start_sync(struct work_struct *ws)
* As we only add devices that are already in-sync, we can
* activate the spares immediately.
*/
- remove_and_add_spares(mddev, NULL);
+ if (suspend)
+ remove_and_add_spares(mddev, NULL);
goto not_running;
}
- if (!md_choose_sync_action(mddev, &spares))
+ if (!md_choose_sync_action(mddev, &spares, suspend))
goto not_running;
if (!mddev->pers->sync_request)
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2 2/7] md/raid1: advertise atomic write limits and handle runtime constraints
From: John Garry @ 2026-06-29 14:48 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, yukuai, magiclinan, xiao, axboe,
vverma, martin.petersen, linux-kernel
Cc: linux-raid
In-Reply-To: <20260628142420.1051027-3-abd.masalkhi@gmail.com>
On 28/06/2026 15:24, Abd-Alrhman Masalkhi wrote:
> Atomic writes in RAID1 must fit within a single barrier unit. Advertise
> this restriction through the queue limits by setting
> atomic_write_hw_unit_max to BARRIER_UNIT_SECTOR_SIZE so that bios which
> would cross a barrier-unit boundary are rejected by the block layer
> before reaching MD.
>
> A bio that passes block-layer validation may still become unserviceable
> within RAID1 due to bad blocks or write-behind constraints. In the former
> case, complete the bio with EIO. In the latter case, disable
> write-behind rather than failing the bio with EIO.
>
> Fixes: f2a38abf5f1c ("md/raid1: Atomic write support")
> Fixes: a4c55c902670 ("md/raid1: simplify raid1_write_request() error handling")
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
> Changes in v2:
> - Drop the early atomic write split check from raid1_write_request().
> - Advertise the atomic write size limit via queue limits.
> - Disable write-behind instead of failing atomic writes when the
> BIO_MAX_VECS limit is encountered.
> - Link to v1: https://urldefense.com/v3/__https://lore.kernel.org/linux-raid/20260623072456.333437-3-abd.masalkhi@gmail.com/__;!!ACWV5N9M2RV99hQ!LbMSGSClRi0PNBqQti5ZNWGDVjDd34-7saYEAwNyBNjpNTjEA7veqM5RHG8KB1QiscarW4UaIefjm19ywSImtIgh$
> ---
> drivers/md/raid1.c | 36 +++++++++++++++++++-----------------
> 1 file changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index afe2ca96ad8c..f322048ab3c2 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1522,6 +1522,7 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
> int first_clone;
> bool write_behind = false;
> bool nowait = bio->bi_opf & REQ_NOWAIT;
> + bool atomic = bio->bi_opf & REQ_ATOMIC;
> bool is_discard = op_is_discard(bio->bi_opf);
> sector_t sector = bio->bi_iter.bi_sector;
>
> @@ -1603,20 +1604,6 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
> }
> if (is_bad) {
> int good_sectors;
> -
> - /*
> - * We cannot atomically write this, so just
> - * error in that case. It could be possible to
> - * atomically write other mirrors, but the
> - * complexity of supporting that is not worth
> - * the benefit.
> - */
> - if (bio->bi_opf & REQ_ATOMIC) {
> - bio->bi_status = BLK_STS_NOTSUPP;
> - bio_endio(bio);
> - goto err_dec_pending;
> - }
> -
> good_sectors = first_bad - sector;
> if (good_sectors < max_sectors)
> max_sectors = good_sectors;
> @@ -1633,10 +1620,24 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
> * at a time and thus needs a new bio that can fit the whole payload
> * this bio in page sized chunks.
> */
> - if (write_behind && mddev->bitmap)
> - max_sectors = min_t(int, max_sectors,
> - BIO_MAX_VECS * (PAGE_SIZE >> 9));
> + if (write_behind && mddev->bitmap) {
> + if (atomic && max_sectors > BIO_MAX_VECS * (PAGE_SIZE >> 9))
where does BIO_MAX_VECS * (PAGE_SIZE >> 9) even come from?
> + /*
> + * Atomic writes cannot be split, so disable
> + * write-behind.
> + */
> + write_behind = false;
> + else
> + max_sectors = min_t(int, max_sectors,
> + BIO_MAX_VECS * (PAGE_SIZE >> 9));
> + }
> +
> if (max_sectors < bio_sectors(bio)) {
> + if (atomic) {
> + bio_io_error(bio);
> + goto err_dec_pending;
> + }
> +
> bio = bio_submit_split_bioset(bio, max_sectors,
> &conf->bio_split);
> if (!bio)
> @@ -3229,6 +3230,7 @@ static int raid1_set_limits(struct mddev *mddev)
> lim.max_write_zeroes_sectors = 0;
> lim.max_hw_wzeroes_unmap_sectors = 0;
> lim.logical_block_size = mddev->logical_block_size;
> + lim.atomic_write_hw_unit_max = BARRIER_UNIT_SECTOR_SIZE;
This BARRIER_UNIT_SECTOR_SIZE is a bit like chunk sectors, no? I am just
wondering if we just should set it to chunk sectors =
BARRIER_UNIT_SECTOR_SIZE
I assume that it affects more than Reads and writes, e.g. discard also.
> lim.features |= BLK_FEAT_ATOMIC_WRITES;
> lim.features |= BLK_FEAT_PCI_P2PDMA;
> err = mddev_stack_rdev_limits(mddev, &lim, MDDEV_STACK_INTEGRITY);
^ permalink raw reply
* Re: cleanup the RAID6 P/Q library v3
From: Christoph Hellwig @ 2026-06-29 12:48 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Christoph Hellwig, Andrew Morton, Catalin Marinas, Will Deacon,
Ard Biesheuvel, Huacai Chen, WANG Xuerui, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy (CS GROUP),
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, Herbert Xu,
Dan Williams, Chris Mason, David Sterba, Arnd Bergmann, Song Liu,
Yu Kuai, Li Nan, linux-kernel, linux-arm-kernel, loongarch,
linuxppc-dev, linux-riscv, linux-s390, linux-crypto, linux-btrfs,
linux-arch, linux-raid
In-Reply-To: <8684FEC2-AD56-4BDA-874F-1D3D7170F880@zytor.com>
On Mon, Jun 29, 2026 at 05:34:41AM -0700, H. Peter Anvin wrote:
> The real issue though is that the benchmarking isn't really helpful, either.
>
> It has to do with power across the whole chip, so it perversely enough depends on what *else* is going on...
Yes. And preferences might matter if this is just a file server or
something running a real workload as well. But I don't really have a
good answer to that.
^ permalink raw reply
* Re: cleanup the RAID6 P/Q library v3
From: H. Peter Anvin @ 2026-06-29 12:34 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Andrew Morton, Catalin Marinas, Will Deacon, Ard Biesheuvel,
Huacai Chen, WANG Xuerui, Madhavan Srinivasan, Michael Ellerman,
Nicholas Piggin, Christophe Leroy (CS GROUP), Paul Walmsley,
Palmer Dabbelt, Albert Ou, Alexandre Ghiti, Heiko Carstens,
Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
Sven Schnelle, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, Herbert Xu, Dan Williams, Chris Mason,
David Sterba, Arnd Bergmann, Song Liu, Yu Kuai, Li Nan,
linux-kernel, linux-arm-kernel, loongarch, linuxppc-dev,
linux-riscv, linux-s390, linux-crypto, linux-btrfs, linux-arch,
linux-raid
In-Reply-To: <20260629122152.GA21344@lst.de>
On June 29, 2026 5:21:52 AM PDT, Christoph Hellwig <hch@lst.de> wrote:
>On Fri, Jun 26, 2026 at 05:52:18PM -0700, H. Peter Anvin wrote:
>> On 2026-05-19 01:24, Christoph Hellwig wrote:
>> >
>> > raid6: rework registration of optimized algorithms
>> >
>> > - avx2 instead of avx512 is probably the right thing for no
>> > benchmarking, but if it was intentional (it wasn't), that should
>> > be document. So I'll just switch back to the previous version to
>> > keep the state of the art
>>
>> It is unlikely to be the right thing *going forward*, though.
>>
>> The very unfortunate performance inversion is likely model-specific. It is one
>> of those things where you largely would have to have a list of quirks :(
>
>Note that the default is still to benchmark the implementations.
>This is just about the case where the user explicitly disabled that
>benchmark-based selection.
>
The real issue though is that the benchmarking isn't really helpful, either.
It has to do with power across the whole chip, so it perversely enough depends on what *else* is going on...
^ permalink raw reply
* Re: cleanup the RAID6 P/Q library v3
From: Christoph Hellwig @ 2026-06-29 12:21 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Christoph Hellwig, Andrew Morton, Catalin Marinas, Will Deacon,
Ard Biesheuvel, Huacai Chen, WANG Xuerui, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy (CS GROUP),
Paul Walmsley, Palmer Dabbelt, Albert Ou, Alexandre Ghiti,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, Herbert Xu,
Dan Williams, Chris Mason, David Sterba, Arnd Bergmann, Song Liu,
Yu Kuai, Li Nan, linux-kernel, linux-arm-kernel, loongarch,
linuxppc-dev, linux-riscv, linux-s390, linux-crypto, linux-btrfs,
linux-arch, linux-raid
In-Reply-To: <7d8efc7c-65e3-49f8-9381-910de7affef2@zytor.com>
On Fri, Jun 26, 2026 at 05:52:18PM -0700, H. Peter Anvin wrote:
> On 2026-05-19 01:24, Christoph Hellwig wrote:
> >
> > raid6: rework registration of optimized algorithms
> >
> > - avx2 instead of avx512 is probably the right thing for no
> > benchmarking, but if it was intentional (it wasn't), that should
> > be document. So I'll just switch back to the previous version to
> > keep the state of the art
>
> It is unlikely to be the right thing *going forward*, though.
>
> The very unfortunate performance inversion is likely model-specific. It is one
> of those things where you largely would have to have a list of quirks :(
Note that the default is still to benchmark the implementations.
This is just about the case where the user explicitly disabled that
benchmark-based selection.
^ permalink raw reply
* Re: [PATCH v2 3/7] md/raid10: consistently fail atomic writes that require splitting
From: Abd-Alrhman Masalkhi @ 2026-06-28 21:35 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-raid, yukuai
In-Reply-To: <20260628143620.64A6C1F000E9@smtp.kernel.org>
Hi Kuai,
On Sun, Jun 28, 2026 at 14:36 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] The `err_dec_pending` (formerly `err_handle`) cleanup path in `raid10_write_request` can trigger a NULL pointer dereference when a replacement device is promoted concurrently.
> --
>
> commit bc9d81fdb2c33f8c55baad27ab1eedf427ae25ed
> Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>
> md/raid10: consistently fail atomic writes that require splitting
>
> RAID10 handles one badblock path explicitly by failing atomic writes with
> EIO, but another path can also reduce the writable range and force the bio
> through bio_submit_split_bioset(), completing it with EINVAL. This commit
> centralizes the check to fail all atomic writes requiring splitting with
> EIO.
>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index bd322eccdc3ff..3480fc7907f01 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
> [ ... ]
>> @@ -1493,6 +1484,9 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
>> r10_bio->sectors = max_sectors;
>>
>> if (r10_bio->sectors < bio_sectors(bio)) {
>> + if (atomic)
>> + goto err_handle;
>> +
>
> [Severity: High]
> This is a pre-existing issue, but can this jump to the error handler cause a
> null pointer dereference if a replacement device is concurrently promoted?
>
> In the error handler path, the code fetches the replacement device without
> checking for null or falling back to the primary slot:
>
> err_handle:
> for (k = 0; k < i; k++) {
> int d = r10_bio->devs[k].devnum;
> struct md_rdev *rrdev = conf->mirrors[d].replacement;
> ...
> if (r10_bio->devs[k].repl_bio) {
> rdev_dec_pending(rrdev, mddev);
> r10_bio->devs[k].repl_bio = NULL;
> }
> }
>
> If raid10_remove_disk() runs concurrently, it can promote the replacement to
> the primary slot and clear conf->mirrors[d].replacement:
>
> raid10_remove_disk() {
> ...
> WRITE_ONCE(*rdevp, NULL);
> if (p->replacement) {
> WRITE_ONCE(p->rdev, p->replacement);
> clear_bit(Replacement, &p->replacement->flags);
> WRITE_ONCE(p->replacement, NULL);
> }
> ...
> }
>
At first look this looks unreachable, because raid10_remove_disk()
should only run while the array is suspended, which would drain the
in-flight write before any promotion. But that assumption does not hold
on the md_start_sync() path. The suspend there is gated on a lock-free
check of md_spares_need_change() taken once at function entry; we only
suspend if it returns true. We then call remove_spares()
unconditionally, regardless of that earlier decision, and
remove_spares() re-evaluates rdev_removeable() independently under the
lock.
So the suspend decision and the actual removal are sampled at different
times with nothing held across them. A primary that was In_sync at the
entry check (suspend skipped) but fails afterward becomes removeable by
the time remove_spares() runs, and raid10_remove_disk() then promotes
the replacement and clears the slot with no suspend in effect. Meanwhile
an in-flight raid10_write_request() that referenced only the replacement
(because the primary was already Faulty when it ran) can be sitting in
err_handle, and it reads conf->mirrors[d].replacement as NULL.
It seems real. I'll submit a fix addressing this issue.
> If this concurrent promotion happens, will rrdev be null here? It looks like
> other paths such as raid10_end_write_request() safely handle this exact
> race by falling back to rdev when replacement is null, but this cleanup
> path does not.
>
>> allow_barrier(conf);
>> bio = bio_submit_split_bioset(bio, r10_bio->sectors,
>> &conf->bio_split);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: [PATCH v2 1/7] md/raid10: fix r10bio leak in raid10_write_request() error paths
From: sashiko-bot @ 2026-06-28 14:39 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: yukuai, linux-raid
In-Reply-To: <20260628142420.1051027-2-abd.masalkhi@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Execution paths in raid10_write_request and raid10_read_request unconditionally block on REQ_NOWAIT bios.
- [High] Unconditional use of GFP_NOIO for r10_bio allocation can block, violating REQ_NOWAIT semantics.
--
commit d4f532acb9d5fd7506dfbbb4a12a97248c3fb429
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
md/raid10: fix r10bio leak in raid10_write_request() error paths
When raid10_write_request() fails because REQ_NOWAIT is set, the
allocated r10_bio is not freed before returning, resulting in a memory
leak. Free r10_bio before returning from the REQ_NOWAIT error paths.
Link: https://lore.kernel.org/linux-raid/20260613184042.BCEC01F000E9@smtp.kernel.org/
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 0a3cfdd3f5df8..bd322eccdc3ff 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1365,6 +1365,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
> /* Bail out if REQ_NOWAIT is set for the bio */
> if (bio->bi_opf & REQ_NOWAIT) {
> bio_wouldblock_error(bio);
> + free_r10bio(r10_bio);
> return false;
> }
[ ... ]
> @@ -1398,6 +1399,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
> if (bio->bi_opf & REQ_NOWAIT) {
> allow_barrier(conf);
> bio_wouldblock_error(bio);
> + free_r10bio(r10_bio);
> return false;
> }
[ ... ]
[Severity: High]
This is a pre-existing issue, but does wait_blocked_dev() still block
unconditionally later in raid10_write_request() even when REQ_NOWAIT is set?
raid10_write_request() {
...
r10_bio->read_slot = -1;
raid10_find_phys(conf, r10_bio);
wait_blocked_dev(mddev, r10_bio);
...
}
Inside wait_blocked_dev(), the code calls md_wait_for_blocked_rdev() and
wait_barrier(), both of which can block indefinitely. Can this cause
unintended latency spikes or thread starvation for userspace applications
issuing async I/O?
Similarly, in raid10_read_request(), when a bio is split across a chunk
boundary, it calls wait_barrier(conf, false) without checking the
parent bio's REQ_NOWAIT flag.
[Severity: High]
This is another pre-existing issue, but do the mempool_alloc() calls in
__make_request() and raid10_handle_discard() bypass REQ_NOWAIT semantics?
__make_request() {
...
r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
...
}
If the mempool is depleted and memory is low, this GFP_NOIO allocation can
block. Should this use GFP_NOWAIT for REQ_NOWAIT bios and return
BLK_STS_AGAIN when the pool is exhausted to preserve the non-blocking
contract?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH v2 2/7] md/raid1: advertise atomic write limits and handle runtime constraints
From: sashiko-bot @ 2026-06-28 14:38 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: linux-raid, yukuai
In-Reply-To: <20260628142420.1051027-3-abd.masalkhi@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Atomic write limits are improperly configured and completely overwritten during limit stacking.
--
commit c2837eff05b0ac2b80653d924f40f1bef7bad6d3
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
md/raid1: advertise atomic write limits and handle runtime constraints
Atomic writes in RAID1 must fit within a single barrier unit. Advertise
this restriction through the queue limits by setting
atomic_write_hw_unit_max to BARRIER_UNIT_SECTOR_SIZE so that bios which
would cross a barrier-unit boundary are rejected by the block layer
before reaching MD.
A bio that passes block-layer validation may still become unserviceable
within RAID1 due to bad blocks or write-behind constraints. In the former
case, complete the bio with EIO. In the latter case, disable
write-behind rather than failing the bio with EIO.
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1633,10 +1620,24 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
[ ... ]
> if (max_sectors < bio_sectors(bio)) {
> + if (atomic) {
> + bio_io_error(bio);
> + goto err_dec_pending;
> + }
> +
[Severity: High]
Will this result in atomic writes dynamically failing with unexpected I/O
errors instead of being properly rejected early by the block layer?
Because the limits configured in raid1_set_limits() appear to get overwritten
during stacking, boundary-crossing bios will bypass block layer validation and
trigger an error dynamically here.
> bio = bio_submit_split_bioset(bio, max_sectors,
> &conf->bio_split);
[ ... ]
> @@ -3229,6 +3230,7 @@ static int raid1_set_limits(struct mddev *mddev)
> lim.max_write_zeroes_sectors = 0;
> lim.max_hw_wzeroes_unmap_sectors = 0;
> lim.logical_block_size = mddev->logical_block_size;
> + lim.atomic_write_hw_unit_max = BARRIER_UNIT_SECTOR_SIZE;
> + lim.features |= BLK_FEAT_ATOMIC_WRITES;
> + lim.features |= BLK_FEAT_PCI_P2PDMA;
> err = mddev_stack_rdev_limits(mddev, &lim, MDDEV_STACK_INTEGRITY);
[Severity: High]
Is BARRIER_UNIT_SECTOR_SIZE being assigned in sectors while the
atomic_write_hw_unit_max limit expects bytes? If so, this might artificially
cap atomic writes to mismatched units.
Additionally, will these custom RAID1 atomic limits be completely overwritten
when stacking?
Because atomic_write_hw_max is left unmodified from its default of UINT_MAX,
when mddev_stack_rdev_limits() is called:
mddev_stack_rdev_limits() ->
queue_limits_stack_bdev() ->
blk_stack_atomic_writes_limits() ->
blk_stack_atomic_writes_head()
The stacking code will see UINT_MAX and unconditionally overwrite the newly
configured RAID1 limit with the first underlying device's limits.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=2
^ permalink raw reply
* Re: [PATCH v2 3/7] md/raid10: consistently fail atomic writes that require splitting
From: sashiko-bot @ 2026-06-28 14:36 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: linux-raid, yukuai
In-Reply-To: <20260628142420.1051027-4-abd.masalkhi@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] The `err_dec_pending` (formerly `err_handle`) cleanup path in `raid10_write_request` can trigger a NULL pointer dereference when a replacement device is promoted concurrently.
--
commit bc9d81fdb2c33f8c55baad27ab1eedf427ae25ed
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
md/raid10: consistently fail atomic writes that require splitting
RAID10 handles one badblock path explicitly by failing atomic writes with
EIO, but another path can also reduce the writable range and force the bio
through bio_submit_split_bioset(), completing it with EINVAL. This commit
centralizes the check to fail all atomic writes requiring splitting with
EIO.
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index bd322eccdc3ff..3480fc7907f01 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
[ ... ]
> @@ -1493,6 +1484,9 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
> r10_bio->sectors = max_sectors;
>
> if (r10_bio->sectors < bio_sectors(bio)) {
> + if (atomic)
> + goto err_handle;
> +
[Severity: High]
This is a pre-existing issue, but can this jump to the error handler cause a
null pointer dereference if a replacement device is concurrently promoted?
In the error handler path, the code fetches the replacement device without
checking for null or falling back to the primary slot:
err_handle:
for (k = 0; k < i; k++) {
int d = r10_bio->devs[k].devnum;
struct md_rdev *rrdev = conf->mirrors[d].replacement;
...
if (r10_bio->devs[k].repl_bio) {
rdev_dec_pending(rrdev, mddev);
r10_bio->devs[k].repl_bio = NULL;
}
}
If raid10_remove_disk() runs concurrently, it can promote the replacement to
the primary slot and clear conf->mirrors[d].replacement:
raid10_remove_disk() {
...
WRITE_ONCE(*rdevp, NULL);
if (p->replacement) {
WRITE_ONCE(p->rdev, p->replacement);
clear_bit(Replacement, &p->replacement->flags);
WRITE_ONCE(p->replacement, NULL);
}
...
}
If this concurrent promotion happens, will rrdev be null here? It looks like
other paths such as raid10_end_write_request() safely handle this exact
race by falling back to rdev when replacement is null, but this cleanup
path does not.
> allow_barrier(conf);
> bio = bio_submit_split_bioset(bio, r10_bio->sectors,
> &conf->bio_split);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
^ permalink raw reply
* [PATCH] md: remove REQ_NOWAIT support
From: Abd-Alrhman Masalkhi @ 2026-06-28 14:27 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, vverma, axboe, linux-raid,
linux-kernel
Cc: Abd-Alrhman Masalkhi
REQ_NOWAIT support in md is fundamentally incomplete. While reads can
avoid some blocking paths, write requests can still encounter cases
where one mirror succeeds while another returns -EAGAIN. At that point
md cannot distinguish queue pressure from a real device failure, so it
can neither record a bad block nor safely retry the write without
REQ_NOWAIT, leaving mirrors with divergent data.
Rather than continue advertising REQ_NOWAIT support that cannot be
implemented correctly, remove it from md and its raid personalities.
This simplifies the waiting paths and avoids exposing incorrect
non-blocking semantics.
Fixes: bf2c411bb1cf ("md: raid456 add nowait support")
Fixes: c9aa889b035f ("md: raid10 add nowait support")
Fixes: 5aa705039c4f ("md: raid1 add nowait support")
Fixes: f51d46d0e7cb ("md: add support for REQ_NOWAIT")
Suggested-by: Yu Kuai <yukuai@fygo.io>
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
drivers/md/md-bitmap.c | 9 +---
drivers/md/md-bitmap.h | 2 +-
drivers/md/md-llbitmap.c | 10 +----
drivers/md/md.c | 15 ++-----
drivers/md/raid1-10.c | 8 ++--
drivers/md/raid1.c | 91 +++++++++-------------------------------
drivers/md/raid10.c | 80 +++++++++--------------------------
drivers/md/raid5.c | 13 ------
8 files changed, 51 insertions(+), 177 deletions(-)
diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index 0f02e2956398..7d778fe1c47c 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -2064,23 +2064,18 @@ static void bitmap_end_behind_write(struct mddev *mddev)
bitmap->mddev->bitmap_info.max_write_behind);
}
-static bool bitmap_wait_behind_writes(struct mddev *mddev, bool nowait)
+static void bitmap_wait_behind_writes(struct mddev *mddev)
{
struct bitmap *bitmap = mddev->bitmap;
/* wait for behind writes to complete */
if (bitmap && atomic_read(&bitmap->behind_writes) > 0) {
- if (nowait)
- return false;
-
pr_debug("md:%s: behind writes in progress - waiting to stop.\n",
mdname(mddev));
/* need to kick something here to make sure I/O goes? */
wait_event(bitmap->behind_wait,
atomic_read(&bitmap->behind_writes) == 0);
}
-
- return true;
}
static void bitmap_destroy(struct mddev *mddev)
@@ -2090,7 +2085,7 @@ static void bitmap_destroy(struct mddev *mddev)
if (!bitmap) /* there was no bitmap */
return;
- bitmap_wait_behind_writes(mddev, false);
+ bitmap_wait_behind_writes(mddev);
if (!test_bit(MD_SERIALIZE_POLICY, &mddev->flags))
mddev_destroy_serial_pool(mddev, NULL);
diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h
index f46674bdfeb9..214f623c7e79 100644
--- a/drivers/md/md-bitmap.h
+++ b/drivers/md/md-bitmap.h
@@ -98,7 +98,7 @@ struct bitmap_operations {
void (*start_behind_write)(struct mddev *mddev);
void (*end_behind_write)(struct mddev *mddev);
- bool (*wait_behind_writes)(struct mddev *mddev, bool nowait);
+ void (*wait_behind_writes)(struct mddev *mddev);
md_bitmap_fn *start_write;
md_bitmap_fn *end_write;
diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
index 5a4e2abaa757..2a2b38c663c3 100644
--- a/drivers/md/md-llbitmap.c
+++ b/drivers/md/md-llbitmap.c
@@ -1574,19 +1574,13 @@ static void llbitmap_end_behind_write(struct mddev *mddev)
wake_up(&llbitmap->behind_wait);
}
-static bool llbitmap_wait_behind_writes(struct mddev *mddev, bool nowait)
+static void llbitmap_wait_behind_writes(struct mddev *mddev)
{
struct llbitmap *llbitmap = mddev->bitmap;
- if (llbitmap && atomic_read(&llbitmap->behind_writes) > 0) {
- if (nowait)
- return false;
-
+ if (llbitmap && atomic_read(&llbitmap->behind_writes) > 0)
wait_event(llbitmap->behind_wait,
atomic_read(&llbitmap->behind_writes) == 0);
- }
-
- return true;
}
static ssize_t bits_show(struct mddev *mddev, char *page)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d1465bcd86c8..66a41d482e59 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -402,14 +402,9 @@ bool md_handle_request(struct mddev *mddev, struct bio *bio)
*/
percpu_ref_get(&mddev->active_io);
} else {
- if (is_suspended(mddev, bio)) {
- /* Bail out if REQ_NOWAIT is set for the bio */
- if (bio->bi_opf & REQ_NOWAIT) {
- bio_wouldblock_error(bio);
- return true;
- }
+ if (is_suspended(mddev, bio))
wait_event(mddev->sb_wait, !is_suspended(mddev, bio));
- }
+
if (!percpu_ref_tryget_live(&mddev->active_io))
goto check_suspended;
}
@@ -6283,7 +6278,7 @@ void md_init_stacking_limits(struct queue_limits *lim)
{
blk_set_stacking_limits(lim);
lim->features = BLK_FEAT_WRITE_CACHE | BLK_FEAT_FUA |
- BLK_FEAT_IO_STAT | BLK_FEAT_NOWAIT;
+ BLK_FEAT_IO_STAT;
}
EXPORT_SYMBOL_GPL(md_init_stacking_limits);
@@ -6631,7 +6626,6 @@ int md_run(struct mddev *mddev)
int err;
struct md_rdev *rdev;
struct md_personality *pers;
- bool nowait = true;
if (list_empty(&mddev->disks))
/* cannot run an array with no devices.. */
@@ -6702,7 +6696,6 @@ int md_run(struct mddev *mddev)
}
}
sysfs_notify_dirent_safe(rdev->sysfs_state);
- nowait = nowait && bdev_nowait(rdev->bdev);
}
pers = get_pers(mddev->level, mddev->clevel);
@@ -7050,7 +7043,7 @@ EXPORT_SYMBOL_GPL(md_stop_writes);
static void mddev_detach(struct mddev *mddev)
{
if (md_bitmap_enabled(mddev, false))
- mddev->bitmap_ops->wait_behind_writes(mddev, false);
+ mddev->bitmap_ops->wait_behind_writes(mddev);
if (mddev->pers && mddev->pers->quiesce && !is_md_suspended(mddev)) {
mddev->pers->quiesce(mddev, 1);
mddev->pers->quiesce(mddev, 0);
diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
index 56a56a4da4f8..3b0e230692ba 100644
--- a/drivers/md/raid1-10.c
+++ b/drivers/md/raid1-10.c
@@ -290,9 +290,8 @@ static inline bool raid1_should_read_first(struct mddev *mddev,
}
/*
- * bio with REQ_RAHEAD or REQ_NOWAIT can fail at anytime, before such IO is
- * submitted to the underlying disks, hence don't record badblocks or retry
- * in this case.
+ * bio with REQ_RAHEAD can fail at anytime, before such IO is submitted to the
+ * underlying disks, hence don't record badblocks or retry in this case.
*
* BLK_STS_INVAL means the bio was not valid for the underlying device. This
* is a user error, not a device failure, so retrying or recording bad blocks
@@ -300,6 +299,5 @@ static inline bool raid1_should_read_first(struct mddev *mddev,
*/
static inline bool raid1_should_handle_error(struct bio *bio)
{
- return !(bio->bi_opf & (REQ_RAHEAD | REQ_NOWAIT)) &&
- bio->bi_status != BLK_STS_INVAL;
+ return !(bio->bi_opf & REQ_RAHEAD) && bio->bi_status != BLK_STS_INVAL;
}
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index f322048ab3c2..a217c9e2d504 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1051,10 +1051,8 @@ static void lower_barrier(struct r1conf *conf, sector_t sector_nr)
wake_up(&conf->wait_barrier);
}
-static bool _wait_barrier(struct r1conf *conf, int idx, bool nowait)
+static void _wait_barrier(struct r1conf *conf, int idx)
{
- bool ret = true;
-
/*
* We need to increase conf->nr_pending[idx] very early here,
* then raise_barrier() can be blocked when it waits for
@@ -1085,7 +1083,7 @@ static bool _wait_barrier(struct r1conf *conf, int idx, bool nowait)
*/
if (!READ_ONCE(conf->array_frozen) &&
!atomic_read(&conf->barrier[idx]))
- return ret;
+ return;
/*
* After holding conf->resync_lock, conf->nr_pending[idx]
@@ -1104,26 +1102,18 @@ static bool _wait_barrier(struct r1conf *conf, int idx, bool nowait)
wake_up_barrier(conf);
/* Wait for the barrier in same barrier unit bucket to drop. */
- /* Return false when nowait flag is set */
- if (nowait) {
- ret = false;
- } else {
- wait_event_lock_irq(conf->wait_barrier,
- !conf->array_frozen &&
- !atomic_read(&conf->barrier[idx]),
- conf->resync_lock);
- atomic_inc(&conf->nr_pending[idx]);
- }
+ wait_event_lock_irq(conf->wait_barrier, !conf->array_frozen &&
+ !atomic_read(&conf->barrier[idx]),
+ conf->resync_lock);
+ atomic_inc(&conf->nr_pending[idx]);
atomic_dec(&conf->nr_waiting[idx]);
spin_unlock_irq(&conf->resync_lock);
- return ret;
}
-static bool wait_read_barrier(struct r1conf *conf, sector_t sector_nr, bool nowait)
+static void wait_read_barrier(struct r1conf *conf, sector_t sector_nr)
{
int idx = sector_to_idx(sector_nr);
- bool ret = true;
/*
* Very similar to _wait_barrier(). The difference is, for read
@@ -1135,7 +1125,7 @@ static bool wait_read_barrier(struct r1conf *conf, sector_t sector_nr, bool nowa
atomic_inc(&conf->nr_pending[idx]);
if (!READ_ONCE(conf->array_frozen))
- return ret;
+ return;
spin_lock_irq(&conf->resync_lock);
atomic_inc(&conf->nr_waiting[idx]);
@@ -1147,27 +1137,19 @@ static bool wait_read_barrier(struct r1conf *conf, sector_t sector_nr, bool nowa
wake_up_barrier(conf);
/* Wait for array to be unfrozen */
- /* Return false when nowait flag is set */
- if (nowait) {
- /* Return false when nowait flag is set */
- ret = false;
- } else {
- wait_event_lock_irq(conf->wait_barrier,
- !conf->array_frozen,
- conf->resync_lock);
- atomic_inc(&conf->nr_pending[idx]);
- }
+ wait_event_lock_irq(conf->wait_barrier, !conf->array_frozen,
+ conf->resync_lock);
+ atomic_inc(&conf->nr_pending[idx]);
atomic_dec(&conf->nr_waiting[idx]);
spin_unlock_irq(&conf->resync_lock);
- return ret;
}
-static bool wait_barrier(struct r1conf *conf, sector_t sector_nr, bool nowait)
+static void wait_barrier(struct r1conf *conf, sector_t sector_nr)
{
int idx = sector_to_idx(sector_nr);
- return _wait_barrier(conf, idx, nowait);
+ _wait_barrier(conf, idx);
}
static void _allow_barrier(struct r1conf *conf, int idx)
@@ -1342,7 +1324,6 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
int max_sectors;
int rdisk;
bool r1bio_existed = !!r1_bio;
- bool nowait = bio->bi_opf & REQ_NOWAIT;
/*
* An md cloned bio indicates we are in the error path.
@@ -1362,16 +1343,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
* Still need barrier for READ in case that whole
* array is frozen.
*/
- if (!wait_read_barrier(conf, bio->bi_iter.bi_sector, nowait)) {
- bio_wouldblock_error(bio);
-
- if (r1bio_existed) {
- set_bit(R1BIO_Returned, &r1_bio->state);
- raid_end_bio_io(r1_bio);
- }
-
- return;
- }
+ wait_read_barrier(conf, bio->bi_iter.bi_sector);
if (!r1_bio)
r1_bio = alloc_r1bio(mddev, bio);
@@ -1409,11 +1381,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
* over-take any writes that are 'behind'
*/
mddev_add_trace_msg(mddev, "raid1 wait behind writes");
- if (!mddev->bitmap_ops->wait_behind_writes(mddev, nowait)) {
- bio_wouldblock_error(bio);
- set_bit(R1BIO_Returned, &r1_bio->state);
- goto err_handle;
- }
+ mddev->bitmap_ops->wait_behind_writes(mddev);
}
if (max_sectors < bio_sectors(bio)) {
@@ -1435,7 +1403,6 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
}
read_bio = bio_alloc_clone(mirror->rdev->bdev, bio, gfp,
&mddev->bio_set);
- read_bio->bi_opf &= ~REQ_NOWAIT;
r1_bio->bios[rdisk] = read_bio;
read_bio->bi_iter.bi_sector = r1_bio->sector +
@@ -1454,7 +1421,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
raid_end_bio_io(r1_bio);
}
-static bool wait_blocked_rdev(struct mddev *mddev, struct bio *bio)
+static void wait_blocked_rdev(struct mddev *mddev, struct bio *bio)
{
struct r1conf *conf = mddev->private;
int disks = conf->raid_disks * 2;
@@ -1474,9 +1441,6 @@ static bool wait_blocked_rdev(struct mddev *mddev, struct bio *bio)
set_bit(BlockedBadBlocks, &rdev->flags);
if (rdev_blocked(rdev)) {
- if (bio->bi_opf & REQ_NOWAIT)
- return false;
-
mddev_add_trace_msg(rdev->mddev, "raid1 wait rdev %d blocked",
rdev->raid_disk);
atomic_inc(&rdev->nr_pending);
@@ -1484,8 +1448,6 @@ static bool wait_blocked_rdev(struct mddev *mddev, struct bio *bio)
goto retry;
}
}
-
- return true;
}
static void raid1_start_write_behind(struct mddev *mddev, struct r1bio *r1_bio,
@@ -1521,7 +1483,6 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
unsigned long flags;
int first_clone;
bool write_behind = false;
- bool nowait = bio->bi_opf & REQ_NOWAIT;
bool atomic = bio->bi_opf & REQ_ATOMIC;
bool is_discard = op_is_discard(bio->bi_opf);
sector_t sector = bio->bi_iter.bi_sector;
@@ -1529,11 +1490,6 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
if (mddev_is_clustered(mddev) &&
mddev->cluster_ops->area_resyncing(mddev, WRITE, sector,
bio_end_sector(bio))) {
-
- if (nowait) {
- bio_wouldblock_error(bio);
- return false;
- }
wait_event_idle(conf->wait_barrier,
!mddev->cluster_ops->area_resyncing(mddev, WRITE,
sector,
@@ -1545,15 +1501,9 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
* thread has put up a bar for new requests.
* Continue immediately if no resync is active currently.
*/
- if (!wait_barrier(conf, sector, nowait)) {
- bio_wouldblock_error(bio);
- return false;
- }
+ wait_barrier(conf, sector);
- if (!wait_blocked_rdev(mddev, bio)) {
- bio_wouldblock_error(bio);
- goto err_allow_barrier;
- }
+ wait_blocked_rdev(mddev, bio);
r1_bio = alloc_r1bio(mddev, bio);
r1_bio->sectors = max_sectors;
@@ -1682,7 +1632,6 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
wait_for_serialization(rdev, r1_bio);
}
- mbio->bi_opf &= ~REQ_NOWAIT;
r1_bio->bios[i] = mbio;
mbio->bi_iter.bi_sector = sector + rdev->data_offset;
@@ -1721,8 +1670,6 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
}
free_r1bio(r1_bio);
-
-err_allow_barrier:
allow_barrier(conf, sector);
return false;
@@ -1851,7 +1798,7 @@ static void close_sync(struct r1conf *conf)
int idx;
for (idx = 0; idx < BARRIER_BUCKETS_NR; idx++) {
- _wait_barrier(conf, idx, false);
+ _wait_barrier(conf, idx);
_allow_barrier(conf, idx);
}
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 01162c483644..c26507a79fbc 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1002,32 +1002,22 @@ static bool wait_barrier_nolock(struct r10conf *conf)
return false;
}
-static bool wait_barrier(struct r10conf *conf, bool nowait)
+static void wait_barrier(struct r10conf *conf)
{
- bool ret = true;
-
if (wait_barrier_nolock(conf))
- return true;
+ return;
write_seqlock_irq(&conf->resync_lock);
if (conf->barrier) {
- /* Return false when nowait flag is set */
- if (nowait) {
- ret = false;
- } else {
- conf->nr_waiting++;
- mddev_add_trace_msg(conf->mddev, "raid10 wait barrier");
- wait_event_barrier(conf, stop_waiting_barrier(conf));
- conf->nr_waiting--;
- }
+ conf->nr_waiting++;
+ mddev_add_trace_msg(conf->mddev, "raid10 wait barrier");
+ wait_event_barrier(conf, stop_waiting_barrier(conf));
+ conf->nr_waiting--;
if (!conf->nr_waiting)
wake_up(&conf->wait_barrier);
}
- /* Only increment nr_pending when we wait */
- if (ret)
- atomic_inc(&conf->nr_pending);
+ atomic_inc(&conf->nr_pending);
write_sequnlock_irq(&conf->resync_lock);
- return ret;
}
static void allow_barrier(struct r10conf *conf)
@@ -1119,28 +1109,22 @@ static void raid10_unplug(struct blk_plug_cb *cb, bool from_schedule)
* currently.
* 2. If IO spans the reshape position. Need to wait for reshape to pass.
*/
-static bool regular_request_wait(struct mddev *mddev, struct r10conf *conf,
+static void regular_request_wait(struct mddev *mddev, struct r10conf *conf,
struct bio *bio, sector_t sectors)
{
- /* Bail out if REQ_NOWAIT is set for the bio */
- if (!wait_barrier(conf, bio->bi_opf & REQ_NOWAIT))
- return false;
+ wait_barrier(conf);
while (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
bio->bi_iter.bi_sector < conf->reshape_progress &&
bio->bi_iter.bi_sector + sectors > conf->reshape_progress) {
allow_barrier(conf);
- if (bio->bi_opf & REQ_NOWAIT)
- return false;
-
mddev_add_trace_msg(conf->mddev, "raid10 wait reshape");
wait_event(conf->wait_barrier,
conf->reshape_progress <= bio->bi_iter.bi_sector ||
conf->reshape_progress >= bio->bi_iter.bi_sector +
sectors);
- wait_barrier(conf, false);
+ wait_barrier(conf);
}
- return true;
}
static bool raid10_read_request(struct mddev *mddev, struct bio *bio,
@@ -1189,10 +1173,7 @@ static bool raid10_read_request(struct mddev *mddev, struct bio *bio,
}
}
- if (!regular_request_wait(mddev, conf, bio, r10_bio->sectors)) {
- bio_wouldblock_error(bio);
- return false;
- }
+ regular_request_wait(mddev, conf, bio, r10_bio->sectors);
rdev = read_balance(conf, r10_bio, &max_sectors);
if (!rdev) {
@@ -1213,7 +1194,7 @@ static bool raid10_read_request(struct mddev *mddev, struct bio *bio,
allow_barrier(conf);
bio = bio_submit_split_bioset(bio, max_sectors,
&conf->bio_split);
- wait_barrier(conf, false);
+ wait_barrier(conf);
if (!bio)
goto err_dec_pending;
@@ -1227,8 +1208,6 @@ static bool raid10_read_request(struct mddev *mddev, struct bio *bio,
r10_bio->master_bio = bio;
}
read_bio = bio_alloc_clone(rdev->bdev, bio, gfp, &mddev->bio_set);
- read_bio->bi_opf &= ~REQ_NOWAIT;
-
r10_bio->devs[slot].bio = read_bio;
r10_bio->devs[slot].rdev = rdev;
@@ -1267,7 +1246,6 @@ static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
conf->mirrors[devnum].rdev;
mbio = bio_alloc_clone(rdev->bdev, bio, GFP_NOIO, &mddev->bio_set);
- mbio->bi_opf &= ~REQ_NOWAIT;
if (replacement)
r10_bio->devs[n_copy].repl_bio = mbio;
else
@@ -1346,7 +1324,7 @@ static void wait_blocked_dev(struct mddev *mddev, struct r10bio *r10_bio)
"raid10 %s wait rdev %d blocked",
__func__, blocked_rdev->raid_disk);
md_wait_for_blocked_rdev(blocked_rdev, mddev);
- wait_barrier(conf, false);
+ wait_barrier(conf);
goto retry_wait;
}
}
@@ -1357,29 +1335,19 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
struct r10conf *conf = mddev->private;
int i, k;
int max_sectors = r10_bio->sectors;
- bool nowait = bio->bi_opf & REQ_NOWAIT;
bool atomic = bio->bi_opf & REQ_ATOMIC;
if ((mddev_is_clustered(mddev) &&
mddev->cluster_ops->area_resyncing(mddev, WRITE,
bio->bi_iter.bi_sector,
bio_end_sector(bio)))) {
- /* Bail out if REQ_NOWAIT is set for the bio */
- if (nowait) {
- bio_wouldblock_error(bio);
- return false;
- }
-
wait_event_idle(conf->wait_barrier,
!mddev->cluster_ops->area_resyncing(mddev, WRITE,
bio->bi_iter.bi_sector,
bio_end_sector(bio)));
}
- if (!regular_request_wait(mddev, conf, bio, max_sectors)) {
- bio_wouldblock_error(bio);
- return false;
- }
+ regular_request_wait(mddev, conf, bio, max_sectors);
if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
(mddev->reshape_backwards
@@ -1392,10 +1360,6 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
set_mask_bits(&mddev->sb_flags, 0,
BIT(MD_SB_CHANGE_DEVS) | BIT(MD_SB_CHANGE_PENDING));
md_wakeup_thread(mddev->thread);
- if (nowait) {
- bio_wouldblock_error(bio);
- goto err_allow_barrier;
- }
mddev_add_trace_msg(conf->mddev,
"raid10 wait reshape metadata");
wait_event(mddev->sb_wait,
@@ -1626,11 +1590,7 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery))
return -EAGAIN;
- if (!wait_barrier(conf, bio->bi_opf & REQ_NOWAIT)) {
- bio_wouldblock_error(bio);
- md_write_end(mddev);
- return 0;
- }
+ wait_barrier(conf);
/*
* Check reshape again to avoid reshape happens after checking
@@ -1681,7 +1641,7 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
allow_barrier(conf);
/* Resend the fist split part */
submit_bio_noacct(split);
- wait_barrier(conf, false);
+ wait_barrier(conf);
}
div_u64_rem(bio_end, stripe_size, &remainder);
if (remainder) {
@@ -1701,7 +1661,7 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
/* Resend the second split part */
submit_bio_noacct(bio);
bio = split;
- wait_barrier(conf, false);
+ wait_barrier(conf);
}
bio_start = bio->bi_iter.bi_sector;
@@ -1859,7 +1819,7 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
end_disk_offset += geo->stride;
atomic_inc(&first_r10bio->remaining);
raid_end_discard_bio(r10_bio);
- wait_barrier(conf, false);
+ wait_barrier(conf);
goto retry_discard;
}
@@ -2059,7 +2019,7 @@ static void print_conf(struct r10conf *conf)
static void close_sync(struct r10conf *conf)
{
- wait_barrier(conf, false);
+ wait_barrier(conf);
allow_barrier(conf);
mempool_exit(&conf->r10buf_pool);
@@ -4694,7 +4654,7 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
if (need_flush ||
time_after(jiffies, conf->reshape_checkpoint + 10*HZ)) {
/* Need to update reshape_position in metadata */
- wait_barrier(conf, false);
+ wait_barrier(conf);
mddev->reshape_position = conf->reshape_progress;
if (mddev->reshape_backwards)
mddev->curr_resync_completed = raid10_size(mddev, 0, 0)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 0c5c9fb0606e..5171e221696a 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5718,10 +5718,6 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
struct bio *orig_bi = bi;
int stripe_sectors;
- /* We need to handle this when io_uring supports discard/trim */
- if (WARN_ON_ONCE(bi->bi_opf & REQ_NOWAIT))
- return;
-
if (mddev->reshape_position != MaxSector)
/* Skip discard while reshape is happening */
return;
@@ -6191,15 +6187,6 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
pr_debug("raid456: %s, logical %llu to %llu\n", __func__,
bi->bi_iter.bi_sector, ctx->last_sector);
- /* Bail out if conflicts with reshape and REQ_NOWAIT is set */
- if ((bi->bi_opf & REQ_NOWAIT) &&
- get_reshape_loc(mddev, conf, logical_sector) == LOC_INSIDE_RESHAPE) {
- bio_wouldblock_error(bi);
- if (rw == WRITE)
- md_write_end(mddev);
- mempool_free(ctx, conf->ctx_pool);
- return true;
- }
md_account_bio(mddev, &bi);
/*
--
2.43.0
^ permalink raw reply related
* [PATCH v2 7/7] md/raid10: simplify read request error handling
From: Abd-Alrhman Masalkhi @ 2026-06-28 14:24 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid
In-Reply-To: <20260628142420.1051027-1-abd.masalkhi@gmail.com>
raid10_read_request() currently handles bio completion, barrier
handling, and r10_bio lifetime management in several different error
paths. This results in duplicated cleanup logic and increases the risk
of introducing bugs in future modifications.
Make raid10_read_request() return a status to its callers, consolidate
the read error paths, and free r10_bio from a single location in the
callers. Since the callers allocate r10_bio, they should also be
responsible for freeing it when the request fails.
This makes the read path follow the same ownership model as the write
path and simplifies the error handling flow.
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v2:
- Fix a compilation error (bi -> bio).
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-8-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 45 +++++++++++++++++++++++++--------------------
1 file changed, 25 insertions(+), 20 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index d94c1f28a6f6..01162c483644 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1143,7 +1143,7 @@ static bool regular_request_wait(struct mddev *mddev, struct r10conf *conf,
return true;
}
-static void raid10_read_request(struct mddev *mddev, struct bio *bio,
+static bool raid10_read_request(struct mddev *mddev, struct bio *bio,
struct r10bio *r10_bio)
{
struct r10conf *conf = mddev->private;
@@ -1191,8 +1191,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
if (!regular_request_wait(mddev, conf, bio, r10_bio->sectors)) {
bio_wouldblock_error(bio);
- free_r10bio(r10_bio);
- return;
+ return false;
}
rdev = read_balance(conf, r10_bio, &max_sectors);
@@ -1202,8 +1201,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
mdname(mddev), b,
(unsigned long long)r10_bio->sector);
}
- raid_end_bio_io(r10_bio);
- return;
+ bio_io_error(bio);
+ goto err_allow_barrier;
}
if (err_rdev)
pr_err_ratelimited("md/raid10:%s: %pg: redirecting sector %llu to another mirror\n",
@@ -1215,10 +1214,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
bio = bio_submit_split_bioset(bio, max_sectors,
&conf->bio_split);
wait_barrier(conf, false);
- if (!bio) {
- set_bit(R10BIO_Returned, &r10_bio->state);
- goto err_handle;
- }
+ if (!bio)
+ goto err_dec_pending;
r10_bio->master_bio = bio;
r10_bio->sectors = max_sectors;
@@ -1244,10 +1241,16 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
read_bio->bi_private = r10_bio;
mddev_trace_remap(mddev, read_bio, r10_bio->sector);
submit_bio_noacct(read_bio);
- return;
-err_handle:
+
+ return true;
+
+err_dec_pending:
atomic_dec(&rdev->nr_pending);
- raid_end_bio_io(r10_bio);
+
+err_allow_barrier:
+ allow_barrier(conf);
+
+ return false;
}
static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
@@ -1538,14 +1541,13 @@ static bool __make_request(struct mddev *mddev, struct bio *bio, int sectors)
memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) *
conf->geo.raid_disks);
- ret = true;
if (bio_data_dir(bio) == READ)
- raid10_read_request(mddev, bio, r10_bio);
- else {
+ ret = raid10_read_request(mddev, bio, r10_bio);
+ else
ret = raid10_write_request(mddev, bio, r10_bio);
- if (!ret)
- free_r10bio(r10_bio);
- }
+
+ if (!ret)
+ free_r10bio(r10_bio);
return ret;
}
@@ -1875,6 +1877,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
sector_t chunk_mask = (conf->geo.chunk_mask & conf->prev.chunk_mask);
int chunk_sects = chunk_mask + 1;
int sectors = bio_sectors(bio);
+ bool write = bio_data_dir(bio) == WRITE;
if (unlikely(bio->bi_opf & REQ_PREFLUSH)
&& md_flush_request(mddev, bio))
@@ -1898,7 +1901,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
sectors = chunk_sects -
(bio->bi_iter.bi_sector &
(chunk_sects - 1));
- if (!__make_request(mddev, bio, sectors))
+ if (!__make_request(mddev, bio, sectors) && write)
md_write_end(mddev);
/* In case raid10d snuck in to freeze_array */
@@ -2866,7 +2869,9 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
rdev_dec_pending(rdev, mddev);
r10_bio->state = 0;
- raid10_read_request(mddev, r10_bio->master_bio, r10_bio);
+ if (!raid10_read_request(mddev, r10_bio->master_bio, r10_bio))
+ free_r10bio(r10_bio);
+
/*
* allow_barrier after re-submit to ensure no sync io
* can be issued while regular io pending.
--
2.43.0
^ permalink raw reply related
* [PATCH v2 6/7] md/raid10: simplify write request error handling
From: Abd-Alrhman Masalkhi @ 2026-06-28 14:24 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid
In-Reply-To: <20260628142420.1051027-1-abd.masalkhi@gmail.com>
raid10_write_request() currently handles bio completion, barrier
handling, and r10_bio lifetime management in several different error
paths. This results in duplicated cleanup logic and increases the risk
of introducing bugs in future modifications.
Move bio_wouldblock_error() handling to the callers of
regular_request_wait(), consolidate the write error paths, and free
r10_bio from a single location in __make_request() when
raid10_write_request() fails.
It remove redundant local copies of r10_bio->sectors and use a single
max_sectors variable throughout the function.
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v2:
- No changes.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-7-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 58 ++++++++++++++++++++++-----------------------
1 file changed, 28 insertions(+), 30 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 57813f249578..d94c1f28a6f6 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1123,18 +1123,16 @@ static bool regular_request_wait(struct mddev *mddev, struct r10conf *conf,
struct bio *bio, sector_t sectors)
{
/* Bail out if REQ_NOWAIT is set for the bio */
- if (!wait_barrier(conf, bio->bi_opf & REQ_NOWAIT)) {
- bio_wouldblock_error(bio);
+ if (!wait_barrier(conf, bio->bi_opf & REQ_NOWAIT))
return false;
- }
+
while (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
bio->bi_iter.bi_sector < conf->reshape_progress &&
bio->bi_iter.bi_sector + sectors > conf->reshape_progress) {
allow_barrier(conf);
- if (bio->bi_opf & REQ_NOWAIT) {
- bio_wouldblock_error(bio);
+ if (bio->bi_opf & REQ_NOWAIT)
return false;
- }
+
mddev_add_trace_msg(conf->mddev, "raid10 wait reshape");
wait_event(conf->wait_barrier,
conf->reshape_progress <= bio->bi_iter.bi_sector ||
@@ -1192,6 +1190,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
}
if (!regular_request_wait(mddev, conf, bio, r10_bio->sectors)) {
+ bio_wouldblock_error(bio);
free_r10bio(r10_bio);
return;
}
@@ -1354,8 +1353,8 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
{
struct r10conf *conf = mddev->private;
int i, k;
- sector_t sectors;
- int max_sectors;
+ int max_sectors = r10_bio->sectors;
+ bool nowait = bio->bi_opf & REQ_NOWAIT;
bool atomic = bio->bi_opf & REQ_ATOMIC;
if ((mddev_is_clustered(mddev) &&
@@ -1363,9 +1362,8 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
bio->bi_iter.bi_sector,
bio_end_sector(bio)))) {
/* Bail out if REQ_NOWAIT is set for the bio */
- if (bio->bi_opf & REQ_NOWAIT) {
+ if (nowait) {
bio_wouldblock_error(bio);
- free_r10bio(r10_bio);
return false;
}
@@ -1375,28 +1373,25 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
bio_end_sector(bio)));
}
- sectors = r10_bio->sectors;
- if (!regular_request_wait(mddev, conf, bio, sectors)) {
- free_r10bio(r10_bio);
+ if (!regular_request_wait(mddev, conf, bio, max_sectors)) {
+ bio_wouldblock_error(bio);
return false;
}
if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
(mddev->reshape_backwards
? (bio->bi_iter.bi_sector < conf->reshape_safe &&
- bio->bi_iter.bi_sector + sectors > conf->reshape_progress)
- : (bio->bi_iter.bi_sector + sectors > conf->reshape_safe &&
+ bio->bi_iter.bi_sector + max_sectors > conf->reshape_progress)
+ : (bio->bi_iter.bi_sector + max_sectors > conf->reshape_safe &&
bio->bi_iter.bi_sector < conf->reshape_progress))) {
/* Need to update reshape_position in metadata */
mddev->reshape_position = conf->reshape_progress;
set_mask_bits(&mddev->sb_flags, 0,
BIT(MD_SB_CHANGE_DEVS) | BIT(MD_SB_CHANGE_PENDING));
md_wakeup_thread(mddev->thread);
- if (bio->bi_opf & REQ_NOWAIT) {
- allow_barrier(conf);
+ if (nowait) {
bio_wouldblock_error(bio);
- free_r10bio(r10_bio);
- return false;
+ goto err_allow_barrier;
}
mddev_add_trace_msg(conf->mddev,
"raid10 wait reshape metadata");
@@ -1421,8 +1416,6 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
wait_blocked_dev(mddev, r10_bio);
- max_sectors = r10_bio->sectors;
-
for (i = 0; i < conf->copies; i++) {
int d = r10_bio->devs[i].devnum;
struct md_rdev *rdev, *rrdev;
@@ -1479,15 +1472,15 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
r10_bio->sectors = max_sectors;
if (r10_bio->sectors < bio_sectors(bio)) {
- if (atomic)
- goto err_handle;
+ if (atomic) {
+ bio_io_error(bio);
+ goto err_dec_pending;
+ }
bio = bio_submit_split_bioset(bio, r10_bio->sectors,
&conf->bio_split);
- if (!bio) {
- set_bit(R10BIO_Returned, &r10_bio->state);
- goto err_handle;
- }
+ if (!bio)
+ goto err_dec_pending;
r10_bio->master_bio = bio;
}
@@ -1505,7 +1498,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
one_write_done(r10_bio);
return true;
-err_handle:
+err_dec_pending:
for (k = 0; k < i; k++) {
int d = r10_bio->devs[k].devnum;
struct md_rdev *rdev = conf->mirrors[d].rdev;
@@ -1521,7 +1514,9 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
}
}
- raid_end_bio_io(r10_bio);
+err_allow_barrier:
+ allow_barrier(conf);
+
return false;
}
@@ -1546,8 +1541,11 @@ static bool __make_request(struct mddev *mddev, struct bio *bio, int sectors)
ret = true;
if (bio_data_dir(bio) == READ)
raid10_read_request(mddev, bio, r10_bio);
- else
+ else {
ret = raid10_write_request(mddev, bio, r10_bio);
+ if (!ret)
+ free_r10bio(r10_bio);
+ }
return ret;
}
--
2.43.0
^ permalink raw reply related
* [PATCH v2 5/7] md/raid10: replace wait loop with wait_event_idle()
From: Abd-Alrhman Masalkhi @ 2026-06-28 14:24 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid
In-Reply-To: <20260628142420.1051027-1-abd.masalkhi@gmail.com>
The wait loop is equivalent to wait_event_idle() and can be simplified
by usaing it for improving readability.
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v2:
- No changes.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-6-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 2574f60dd771..57813f249578 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1362,22 +1362,17 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
mddev->cluster_ops->area_resyncing(mddev, WRITE,
bio->bi_iter.bi_sector,
bio_end_sector(bio)))) {
- DEFINE_WAIT(w);
/* Bail out if REQ_NOWAIT is set for the bio */
if (bio->bi_opf & REQ_NOWAIT) {
bio_wouldblock_error(bio);
free_r10bio(r10_bio);
return false;
}
- for (;;) {
- prepare_to_wait(&conf->wait_barrier,
- &w, TASK_IDLE);
- if (!mddev->cluster_ops->area_resyncing(mddev, WRITE,
- bio->bi_iter.bi_sector, bio_end_sector(bio)))
- break;
- schedule();
- }
- finish_wait(&conf->wait_barrier, &w);
+
+ wait_event_idle(conf->wait_barrier,
+ !mddev->cluster_ops->area_resyncing(mddev, WRITE,
+ bio->bi_iter.bi_sector,
+ bio_end_sector(bio)));
}
sectors = r10_bio->sectors;
--
2.43.0
^ permalink raw reply related
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