* [PATCH v2] md: scope memalloc_noio to allocation critical sections
From: Chen Cheng @ 2026-07-18 8:42 UTC (permalink / raw)
To: linux-raid, yukuai; +Cc: chencheng, chencheng, linux-kernel
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:
call flags = memalloc_noio_save() directly in mdraid, where:
1) after array suspend succeed;
2) before allocating memory
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>
---
rfc -> v1:
- dm-raid side just bypass because suspend and resume not in the
same task context..
v1 -> v2:
- just call flags = memalloc_noio_save() directly in mdraid
(suggest by Yu Kuai)
---
drivers/md/md-bitmap.c | 3 +++
drivers/md/md.c | 52 ++++++++++++++++++++++++++++--------------
drivers/md/md.h | 1 -
drivers/md/raid5.c | 14 ++++++++----
4 files changed, 47 insertions(+), 23 deletions(-)
diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index 3e40cb45d473..6d495cdf3fb2 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -2621,14 +2621,16 @@ location_show(struct mddev *mddev, char *page)
static ssize_t
location_store(struct mddev *mddev, const char *buf, size_t len)
{
int rv;
+ unsigned int noio_flags;
rv = mddev_suspend_and_lock(mddev);
if (rv)
return rv;
+ noio_flags = memalloc_noio_save();
if (mddev->pers) {
if (mddev->recovery || mddev->sync_thread) {
rv = -EBUSY;
goto out;
@@ -2711,10 +2713,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:
+ memalloc_noio_restore(noio_flags);
mddev_unlock_and_resume(mddev);
if (rv)
return rv;
return len;
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 096bb64e87bd..25e06f088dc1 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -231,35 +231,35 @@ static int rdev_need_serial(struct md_rdev *rdev)
* 2. rdev is NULL, means we want to enable serialization for all rdevs.
*/
void mddev_create_serial_pool(struct mddev *mddev, struct md_rdev *rdev)
{
int ret = 0;
+ unsigned int noio_flags;
if (rdev && !rdev_need_serial(rdev) &&
!test_bit(CollisionCheck, &rdev->flags))
return;
+ noio_flags = memalloc_noio_save();
if (!rdev)
ret = rdevs_init_serial(mddev);
else
ret = rdev_init_serial(rdev);
if (ret)
- return;
+ goto out;
if (mddev->serial_info_pool == NULL) {
- /*
- * already in memalloc noio context by
- * mddev_suspend()
- */
mddev->serial_info_pool =
mempool_create_kmalloc_pool(NR_SERIAL_INFOS,
sizeof(struct serial_info));
if (!mddev->serial_info_pool) {
rdevs_uninit_serial(mddev);
pr_err("can't alloc memory pool for serialization\n");
}
}
+out:
+ memalloc_noio_restore(noio_flags);
}
/*
* Free resource from rdev(s), and destroy serial_info_pool under conditions:
* 1. rdev is the last device flaged with CollisionCheck.
@@ -514,13 +514,10 @@ int mddev_suspend(struct mddev *mddev, bool interruptible)
* allow new reshape to start while waiting for io to be done to
* 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();
-
mutex_unlock(&mddev->suspend_mutex);
return 0;
}
EXPORT_SYMBOL_GPL(mddev_suspend);
@@ -533,13 +530,10 @@ static void __mddev_resume(struct mddev *mddev, bool recovery_needed)
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);
@@ -4045,10 +4039,11 @@ static ssize_t
level_store(struct mddev *mddev, const char *buf, size_t len)
{
char clevel[16];
ssize_t rv;
size_t slen = len;
+ unsigned int noio_flags;
struct md_personality *pers, *oldpers;
long level;
void *priv, *oldpriv;
struct md_rdev *rdev;
@@ -4056,10 +4051,11 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
return -EINVAL;
rv = mddev_suspend_and_lock(mddev);
if (rv)
return rv;
+ noio_flags = memalloc_noio_save();
if (mddev->pers == NULL) {
memcpy(mddev->clevel, buf, slen);
if (mddev->clevel[slen-1] == '\n')
slen--;
@@ -4231,10 +4227,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:
+ memalloc_noio_restore(noio_flags);
mddev_unlock_and_resume(mddev);
return rv;
}
static struct md_sysfs_entry md_level =
@@ -4410,19 +4407,21 @@ 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);
if (err)
return err;
+ noio_flags = memalloc_noio_save();
if (mddev->pers) {
if (n != mddev->raid_disks)
err = update_raid_disks(mddev, n);
} else if (mddev->reshape_position != MaxSector) {
struct md_rdev *rdev;
@@ -4442,10 +4441,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:
+ memalloc_noio_restore(noio_flags);
mddev_unlock_and_resume(mddev);
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 +4822,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);
@@ -4837,10 +4838,11 @@ new_dev_store(struct mddev *mddev, const char *buf, size_t len)
return -EOVERFLOW;
err = mddev_suspend_and_lock(mddev);
if (err)
return err;
+ noio_flags = memalloc_noio_save();
if (mddev->persistent) {
rdev = md_import_device(dev, mddev->major_version,
mddev->minor_version);
if (!IS_ERR(rdev) && !list_empty(&mddev->disks)) {
struct md_rdev *rdev0
@@ -4855,17 +4857,19 @@ 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)) {
+ memalloc_noio_restore(noio_flags);
mddev_unlock_and_resume(mddev);
return PTR_ERR(rdev);
}
err = bind_rdev_to_array(rdev, mddev);
out:
if (err)
export_rdev(rdev);
+ memalloc_noio_restore(noio_flags);
mddev_unlock_and_resume(mddev);
if (!err)
md_new_event();
return err ? err : len;
}
@@ -8322,12 +8326,14 @@ 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;
+ bool suspend;
err = md_ioctl_valid(cmd);
if (err)
return err;
@@ -8373,17 +8379,19 @@ 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) :
- mddev_lock(mddev);
+ suspend = md_ioctl_need_suspend(cmd);
+ err = suspend ? mddev_suspend_and_lock(mddev) : mddev_lock(mddev);
if (err) {
pr_debug("md: ioctl lock interrupted, reason %d, cmd %d\n",
err, cmd);
goto out;
}
+ if (suspend)
+ noio_flags = memalloc_noio_save();
if (cmd == SET_ARRAY_INFO) {
err = __md_set_array_info(mddev, argp);
goto unlock;
}
@@ -8504,12 +8512,16 @@ 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) :
- mddev_unlock(mddev);
+ if (suspend) {
+ memalloc_noio_restore(noio_flags);
+ mddev_unlock_and_resume(mddev);
+ } else {
+ mddev_unlock(mddev);
+ }
out:
if (cmd == STOP_ARRAY_RO || (err && cmd == STOP_ARRAY))
clear_bit(MD_CLOSING, &mddev->flags);
return err;
@@ -10173,20 +10185,22 @@ 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);
+ noio_flags = memalloc_noio_save();
}
mddev_lock_nointr(mddev);
if (!md_is_rdwr(mddev)) {
/*
@@ -10229,12 +10243,14 @@ static void md_start_sync(struct work_struct *ws)
* md_start_sync was triggered by MD_RECOVERY_NEEDED, so we should
* 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)
+ if (suspend) {
+ memalloc_noio_restore(noio_flags);
__mddev_resume(mddev, false);
+ }
md_wakeup_thread(mddev->sync_thread);
sysfs_notify_dirent_safe(mddev->sysfs_action);
md_new_event();
return;
@@ -10249,12 +10265,14 @@ static void md_start_sync(struct work_struct *ws)
* md_start_sync was triggered by MD_RECOVERY_NEEDED, so we should
* 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)
+ if (suspend) {
+ memalloc_noio_restore(noio_flags);
__mddev_resume(mddev, false);
+ }
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..76488cd9e81e 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.
*/
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 65ae7d8930fc..863dcb396d8b 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2454,15 +2454,10 @@ static int scribble_alloc(struct raid5_percpu *percpu,
sizeof(struct page *) * (num + 2) +
sizeof(addr_conv_t) * (num + 2) +
sizeof(unsigned int) * (num + 2);
void *scribble;
- /*
- * If here is in raid array suspend context, it is in memalloc noio
- * context as well, there is no potential recursive memory reclaim
- * I/Os with the GFP_KERNEL flag.
- */
scribble = kvmalloc_array(cnt, obj_size, GFP_KERNEL);
if (!scribble)
return -ENOMEM;
kvfree(percpu->scribble);
@@ -2473,18 +2468,20 @@ static int scribble_alloc(struct raid5_percpu *percpu,
}
static int resize_chunks(struct r5conf *conf, int new_disks, int new_sectors)
{
unsigned long cpu;
+ unsigned int noio_flags;
int err = 0;
/* Never shrink. */
if (conf->scribble_disks >= new_disks &&
conf->scribble_sectors >= new_sectors)
return 0;
raid5_quiesce(conf->mddev, true);
+ noio_flags = memalloc_noio_save();
cpus_read_lock();
for_each_present_cpu(cpu) {
struct raid5_percpu *percpu;
@@ -2494,10 +2491,11 @@ static int resize_chunks(struct r5conf *conf, int new_disks, int new_sectors)
if (err)
break;
}
cpus_read_unlock();
+ memalloc_noio_restore(noio_flags);
raid5_quiesce(conf->mddev, false);
if (!err) {
conf->scribble_disks = new_disks;
conf->scribble_sectors = new_sectors;
@@ -6993,10 +6991,11 @@ raid5_show_stripe_size(struct mddev *mddev, char *page)
static ssize_t
raid5_store_stripe_size(struct mddev *mddev, const char *page, size_t len)
{
struct r5conf *conf;
unsigned long new;
+ unsigned int noio_flags = 0;
int err;
int size;
if (len >= PAGE_SIZE)
return -EINVAL;
@@ -7033,10 +7032,11 @@ raid5_store_stripe_size(struct mddev *mddev, const char *page, size_t len)
mddev->reshape_position != MaxSector || mddev->sysfs_active) {
err = -EBUSY;
goto out_unlock;
}
+ noio_flags = memalloc_noio_save();
mutex_lock(&conf->cache_size_mutex);
size = conf->max_nr_stripes;
shrink_stripes(conf);
@@ -7049,10 +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:
+ memalloc_noio_restore(noio_flags);
mddev_unlock_and_resume(mddev);
return err ?: len;
}
static struct md_sysfs_entry
@@ -8940,10 +8941,11 @@ 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);
if (err)
return err;
@@ -8951,10 +8953,11 @@ static int raid5_change_consistency_policy(struct mddev *mddev, const char *buf)
if (!conf) {
mddev_unlock_and_resume(mddev);
return -ENODEV;
}
+ noio_flags = memalloc_noio_save();
if (strncmp(buf, "ppl", 3) == 0) {
/* ppl only works with RAID 5 */
if (!raid5_has_ppl(conf) && conf->level == 5) {
err = log_init(conf, NULL, true);
if (!err) {
@@ -8990,10 +8993,11 @@ static int raid5_change_consistency_policy(struct mddev *mddev, const char *buf)
}
if (!err)
md_update_sb(mddev, 1);
+ memalloc_noio_restore(noio_flags);
mddev_unlock_and_resume(mddev);
return err;
}
--
2.54.0
^ permalink raw reply related
* [PATCH] md/bitmap: resume array on backlog_store() error path
From: Chen Cheng @ 2026-07-18 3:42 UTC (permalink / raw)
To: linux-raid, yukuai; +Cc: chencheng, chencheng, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
backlog_store() suspends the array before checking whether a write-mostly
device exists. If no such device exists, the error path only unlocks
reconfig_mutex and leaves the array suspended, blocking subsequent I/O.
Use mddev_unlock_and_resume() to release both states.
Fixes: 58226942ad3d ("md: use new apis to suspend array before mddev_create/destroy_serial_pool")
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/md-bitmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index 028b9ca8ce52..3e40cb45d473 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -2854,11 +2854,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);
return -EINVAL;
}
mddev->bitmap_info.max_write_behind = backlog;
if (!backlog && mddev->serial_info_pool) {
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] md/bitmap: resume array on backlog_store() error path
From: sashiko-bot @ 2026-07-18 3:56 UTC (permalink / raw)
To: Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260718034236.4119093-1-chencheng@fnnas.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Silent allocation failures in `mddev_create_serial_pool()` lead to NULL pointer dereferences and silent data corruption.
- [High] Cross-ioctl and error path usage of `mddev_suspend()` in `dm-raid` leaks `PF_MEMALLOC_NOIO` task flag.
--
--- Patch [1]: [PATCH] md/bitmap: resume array on backlog_store() error path ---
Hi,
Thank you for the patch. I noticed a few pre-existing issues in the related
code paths that might be worth addressing.
commit 5425190104cdbeefa8ae0eda1f825b462b6535ca
Author: Chen Cheng <chencheng@fnnas.com>
md/bitmap: resume array on backlog_store() error path
backlog_store() suspends the array before checking whether a write-mostly
device exists. If no such device exists, the error path only unlocks
reconfig_mutex and leaves the array suspended, blocking subsequent I/O.
Use mddev_unlock_and_resume() to release both states.
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 0f02e2956398d..c90fa1e106e26 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -2862,7 +2862,7 @@ 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);
[Severity: High]
This isn't a bug introduced by this patch, but since this patch fixes an error
path missing a resume, does this same missing resume problem exist in
dm-raid.c?
In drivers/md/dm-raid.c:raid_postsuspend(), the code suspends the mddev and
acquires the PF_MEMALLOC_NOIO flag, but returns to userspace without resuming:
mddev_suspend(&rs->md, false);
If a user-space process issues a DM_DEV_SUSPEND ioctl, this leaves the flag
set. When DM_DEV_RESUME is later issued, potentially by a different task,
raid_resume() calls mddev_resume(), which applies the saved flags to the
new task.
Also, in raid_ctr(), if setup fails, the error path skips the restore
completely and permanently leaks the flag:
bad_unlock:
md_stop(&rs->md);
mddev_unlock(&rs->md);
Could this permanently leak the PF_MEMALLOC_NOIO task flag and cause
memory reclaim deadlocks?
[Severity: Critical]
This is a pre-existing issue, but further down in backlog_store(), is it
possible for mddev_create_serial_pool() to fail silently?
drivers/md/md-bitmap.c:backlog_store() {
...
} else if (backlog && !mddev->serial_info_pool) {
/* serial_info_pool is needed since backlog is not zero */
rdev_for_each(rdev, mddev)
mddev_create_serial_pool(mddev, rdev);
}
...
}
Since mddev_create_serial_pool() returns void, it hides memory allocation
failures. If it fails for a disk in this loop, that device is left without
initialization. Does this silently bypass write-behind serialization for that
disk, leading to overlapping writes and silent data corruption?
Similarly, if MD_SERIALIZE_POLICY is active, check_and_add_serial() in
drivers/md/raid1.c will unconditionally dereference rdev->serial:
int idx = sector_to_idx(r1_bio->sector);
struct serial_in_rdev *serial = &rdev->serial[idx];
struct serial_info *head_si;
spin_lock_irqsave(&serial->serial_lock, flags);
Can this cause a NULL pointer dereference for devices that failed
initialization?
> return -EINVAL;
> }
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260718034236.4119093-1-chencheng@fnnas.com?part=1
^ permalink raw reply
* Re: Subject: RFC: Read repair for md RAID1 after mirror read failures
From: G.W. Kant - Hunenet B.V. @ 2026-07-17 19:15 UTC (permalink / raw)
To: Roger Heflin
Cc: Abd-Alrhman Masalkhi, linux-raid@vger.kernel.org, yukuai@fygo.io
In-Reply-To: <CAAMCDefN+a_mMWByAMPEkCo6sT2iyi-b8J2et5LsJpxZtZBzQQ@mail.gmail.com>
On 7/17/26 8:11 PM, Roger Heflin wrote:
> The drives are said to do refreshes when it finds a weak sector when
> doing a long media test.
>
> The manufactures do not seem to document any disk internals, they
> never have. All manufactures claim their magic firmware/processes
> give them a competitive advantage and so don't disclose what goes on.
> Pretty much all of the hardware is handled this way, with poor
> external documentation.
>
> Smart seems to only disclose the common settings/errors, and don't
> seem to allow anything except setting changes, and to run defined
> tests (short, long and similar).
>
> I have been told by a WD engineer that their internal tools have the
> ability to read out a number of other parameters that aren't exposed
> to SMART, and may also be able to do other things. Those tools are
> not externally available and the when I had access to one of those
> tools (for a few hours so I could get it into a high security site
> with the WD engineer) I had to agree to not keep it.
>
> I watched one of the tools be used on some disks by a WD engineer that
> went onsite (but I don't remember if I saw any non-SMART like data on
> the screen--it was 20 years ago) to check for errors and determine if
> WD would RMA/replace (without media return) around 1000 disks.
>
> I have been reading smart data for as long as smart has been around,
> and outside of maybe at most 10 parameters, most of the rest of the
> parms are rather useless because the manufacturer does not supply a
> decoder ring to make any sense of what said parameter is really
> measuring.
>
> And whatever the manufactures document would be what was documented
> before the disk firmware was completely finished, and may or may not
> have really worked in the given firmware. I have ran into disks that
> would refuse to relocate most bad sectors (even though older disk by
> the same company did this just fine), and a number of other weird disk
> actions indicating that the firmware does not always even implement
> the features that they are supposed to correctly in all disks.
>
> Basically if the smart tools team actually knows anything about the
> disk internals, they are almost certainly under an NDA and cannot tell
> you (likely this is not the case), and if they aren't under an NDA
> they were not told anything.
>
> And contrary to what other experts claim (that disks regularly will
> return bad data--ie 1 in 1e15 sectors), I have never seen the disk
> itself return bad data(I have seen raid controllers deliver corrupted
> data many times from both hardware failures and defective raid
> controller firmware), if the sector is read successfully it is the
> right data simply because the disks ECC correction uses so many bits
> that the chance of a corrupted sector being corrupted and still
> passing ECC is so low as to not be worried about.
Roger,
Thanks for sharing this. I like your observation:
"...if the sector is read successfully it is the right data..."
It made me look at the problem from an information-theory perspective.
To me there are actually two cascaded communication channels.
The first is the time channel: from the moment the data is written to
the magnetic medium until the drive firmware has reconstructed the
sector using its ECC. If the ECC decoder succeeds, then, as you point
out, the probability that the returned sector is still incorrect is
extraordinarily small. In other words, the information has survived
storage in time.
Only after that does the second channel begin: transport of that sector
through the SATA/SAS link, controller, DMA, memory, RAID layer and
finally to the filesystem. This is the space channel. Corruption here is
a different class of error. As you mention, RAID controllers or other
hardware can introduce corruption after the disk has already
reconstructed the correct sector.
Viewed this way, the disk ECC and a filesystem checksum (such as Btrfs
or ZFS) are not competing mechanisms—they protect two different
communication channels.
Disk ECC protects the temporal storage channel.
Btrfs checksums provide end-to-end protection over the transport (space)
channel.
During my recent investigation into a Btrfs device removal with hundreds
of ATA UNC errors, this distinction became quite useful. The Linux
storage stack (MD RAID1 in particular) managed to recover the sector
from the disk, and once the drive delivered it successfully, the Btrfs
checksum verified that the data had also survived the remainder of the
path through the system.
I thought you might appreciate this way of looking at it.
^ permalink raw reply
* Re: Subject: RFC: Read repair for md RAID1 after mirror read failures
From: Roger Heflin @ 2026-07-17 18:11 UTC (permalink / raw)
To: G.W. Kant - Hunenet B.V.
Cc: Abd-Alrhman Masalkhi, linux-raid@vger.kernel.org, yukuai@fygo.io
In-Reply-To: <fd1f940e-4382-4a8a-9d16-e70a708da3cb@hunenet.nl>
The drives are said to do refreshes when it finds a weak sector when
doing a long media test.
The manufactures do not seem to document any disk internals, they
never have. All manufactures claim their magic firmware/processes
give them a competitive advantage and so don't disclose what goes on.
Pretty much all of the hardware is handled this way, with poor
external documentation.
Smart seems to only disclose the common settings/errors, and don't
seem to allow anything except setting changes, and to run defined
tests (short, long and similar).
I have been told by a WD engineer that their internal tools have the
ability to read out a number of other parameters that aren't exposed
to SMART, and may also be able to do other things. Those tools are
not externally available and the when I had access to one of those
tools (for a few hours so I could get it into a high security site
with the WD engineer) I had to agree to not keep it.
I watched one of the tools be used on some disks by a WD engineer that
went onsite (but I don't remember if I saw any non-SMART like data on
the screen--it was 20 years ago) to check for errors and determine if
WD would RMA/replace (without media return) around 1000 disks.
I have been reading smart data for as long as smart has been around,
and outside of maybe at most 10 parameters, most of the rest of the
parms are rather useless because the manufacturer does not supply a
decoder ring to make any sense of what said parameter is really
measuring.
And whatever the manufactures document would be what was documented
before the disk firmware was completely finished, and may or may not
have really worked in the given firmware. I have ran into disks that
would refuse to relocate most bad sectors (even though older disk by
the same company did this just fine), and a number of other weird disk
actions indicating that the firmware does not always even implement
the features that they are supposed to correctly in all disks.
Basically if the smart tools team actually knows anything about the
disk internals, they are almost certainly under an NDA and cannot tell
you (likely this is not the case), and if they aren't under an NDA
they were not told anything.
And contrary to what other experts claim (that disks regularly will
return bad data--ie 1 in 1e15 sectors), I have never seen the disk
itself return bad data(I have seen raid controllers deliver corrupted
data many times from both hardware failures and defective raid
controller firmware), if the sector is read successfully it is the
right data simply because the disks ECC correction uses so many bits
that the chance of a corrupted sector being corrupted and still
passing ECC is so low as to not be worried about.
On Fri, Jul 17, 2026 at 10:48 AM G.W. Kant - Hunenet B.V.
<g.w.kant@hunenet.nl> wrote:
>
> On 7/17/26 12:55 PM, Roger Heflin wrote:
> > On Fri, Jul 17, 2026 at 1:42 AM G.W. Kant - Hunenet B.V.
> > <g.w.kant@hunenet.nl> wrote:
> >>
> >> On 7/15/26 8:04 PM, Abd-Alrhman Masalkhi wrote:
> >>> On Wed, Jul 15, 2026 at 19:41 +0200, Abd-Alrhman Masalkhi wrote:
> >>>> Hi Dion,
> >>>>
> >>>
> >>> I'm not entirely sure about the media refresh part on modern drives...,
> >>> I'll have to look into that first.
> >>>
> >>
> >> Maybe, we could ask the smartmontools people about this. They should
> >> know this. But I have never read about a refresh action in their
> >> documentation.
> >>
> >> Best regards,
> >>
> >> Dion
> >
> > I am not sure why you believe the smartmontools people would have ANY
> > idea about the refresh action. Smartmontools simply exposes the
> > internal drive counters that the drive manufacturer exposes and/or
> > documents.
> >
> > The refresh action is 100% entirely controlled by the drive's firmware
> > and has nothing to do with the smartmontools, and the smart counters
> > may not show any of the internal ECC forced rewrites.
> >
> > As far as I can tell in general the drive does what it wants to
> > (refresh or not, relocation bad sectors or not) and there is NO way
> > that I have ever found to force the firmware do do something else.
>
> When you refer to "the refresh action", are you referring to a
> documented firmware mechanism that periodically rewrites successfully
> readable sectors, or are you using the term more generally for any
> internal media maintenance the drive may perform?
>
> I have not yet found documentation describing such a refresh mechanism
> for commodity HDDs. That is why I mentioned the smartmontools
> developers: not because I expect smartmontools to implement or control
> such a mechanism, but because they are familiar with the capabilities
> that modern drives expose through SMART and related standards.
>
> I'm trying to understand what is publicly known about this.
>
> Best regards,
>
> Dion Kant
>
^ permalink raw reply
* Re: Subject: RFC: Read repair for md RAID1 after mirror read failures
From: G.W. Kant - Hunenet B.V. @ 2026-07-17 15:48 UTC (permalink / raw)
To: Roger Heflin
Cc: Abd-Alrhman Masalkhi, linux-raid@vger.kernel.org, yukuai@fygo.io
In-Reply-To: <CAAMCDeefBEP0c4oW3iX4_cbZBLDuyfuAtCREtFgiH-qS4ZaiXQ@mail.gmail.com>
On 7/17/26 12:55 PM, Roger Heflin wrote:
> On Fri, Jul 17, 2026 at 1:42 AM G.W. Kant - Hunenet B.V.
> <g.w.kant@hunenet.nl> wrote:
>>
>> On 7/15/26 8:04 PM, Abd-Alrhman Masalkhi wrote:
>>> On Wed, Jul 15, 2026 at 19:41 +0200, Abd-Alrhman Masalkhi wrote:
>>>> Hi Dion,
>>>>
>>>
>>> I'm not entirely sure about the media refresh part on modern drives...,
>>> I'll have to look into that first.
>>>
>>
>> Maybe, we could ask the smartmontools people about this. They should
>> know this. But I have never read about a refresh action in their
>> documentation.
>>
>> Best regards,
>>
>> Dion
>
> I am not sure why you believe the smartmontools people would have ANY
> idea about the refresh action. Smartmontools simply exposes the
> internal drive counters that the drive manufacturer exposes and/or
> documents.
>
> The refresh action is 100% entirely controlled by the drive's firmware
> and has nothing to do with the smartmontools, and the smart counters
> may not show any of the internal ECC forced rewrites.
>
> As far as I can tell in general the drive does what it wants to
> (refresh or not, relocation bad sectors or not) and there is NO way
> that I have ever found to force the firmware do do something else.
When you refer to "the refresh action", are you referring to a
documented firmware mechanism that periodically rewrites successfully
readable sectors, or are you using the term more generally for any
internal media maintenance the drive may perform?
I have not yet found documentation describing such a refresh mechanism
for commodity HDDs. That is why I mentioned the smartmontools
developers: not because I expect smartmontools to implement or control
such a mechanism, but because they are familiar with the capabilities
that modern drives expose through SMART and related standards.
I'm trying to understand what is publicly known about this.
Best regards,
Dion Kant
^ permalink raw reply
* Re: Subject: RFC: Read repair for md RAID1 after mirror read failures
From: Roger Heflin @ 2026-07-17 10:55 UTC (permalink / raw)
To: G.W. Kant - Hunenet B.V.
Cc: Abd-Alrhman Masalkhi, linux-raid@vger.kernel.org, yukuai@fygo.io
In-Reply-To: <59fa3f9f-eda1-4f8a-917d-8f6612639a1b@hunenet.nl>
On Fri, Jul 17, 2026 at 1:42 AM G.W. Kant - Hunenet B.V.
<g.w.kant@hunenet.nl> wrote:
>
> On 7/15/26 8:04 PM, Abd-Alrhman Masalkhi wrote:
> > On Wed, Jul 15, 2026 at 19:41 +0200, Abd-Alrhman Masalkhi wrote:
> >> Hi Dion,
> >>
> >
> > I'm not entirely sure about the media refresh part on modern drives...,
> > I'll have to look into that first.
> >
>
> Maybe, we could ask the smartmontools people about this. They should
> know this. But I have never read about a refresh action in their
> documentation.
>
> Best regards,
>
> Dion
I am not sure why you believe the smartmontools people would have ANY
idea about the refresh action. Smartmontools simply exposes the
internal drive counters that the drive manufacturer exposes and/or
documents.
The refresh action is 100% entirely controlled by the drive's firmware
and has nothing to do with the smartmontools, and the smart counters
may not show any of the internal ECC forced rewrites.
As far as I can tell in general the drive does what it wants to
(refresh or not, relocation bad sectors or not) and there is NO way
that I have ever found to force the firmware do do something else.
^ permalink raw reply
* Re: [PATCH] md: fix soft lockup during resync when sync is repeatedly skipped
From: sashiko-bot @ 2026-07-17 6:51 UTC (permalink / raw)
To: Yunye Zhao; +Cc: yukuai, linux-raid
In-Reply-To: <20260717062743.128189-1-yunye.zhao@linux.alibaba.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Repeatedly skipped I/O blocks cause an event storm, calling md_new_event() millions of times in a tight loop.
--
--- Patch [1]: [PATCH] md: fix soft lockup during resync when sync is repeatedly skipped ---
commit 02c063c34357030eaa8514f57d5e37328d410c0b
Author: Yunye Zhao <yunye.zhao@linux.alibaba.com>
md: fix soft lockup during resync when sync is repeatedly skipped
The md_do_sync() main loop advances io_sectors only when I/O is actually
issued. When regions are skipped, io_sectors never increases and the loop
takes the continue branch, eventually triggering a soft lockup on
non-preemptive kernels. This patch adds cond_resched() to this continue
path to prevent the soft lockup.
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d1465bcd86c81..e7411b0334901 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9881,9 +9881,10 @@ void md_do_sync(struct md_thread *thread)
> */
> md_new_event();
>
> - if (last_check + window > io_sectors || j == max_sectors)
> + if (last_check + window > io_sectors || j == max_sectors) {
> + cond_resched();
> continue;
[Severity: High]
This is a pre-existing issue, but can this loop in md_do_sync() cause an event
storm when I/O is repeatedly skipped?
During array recovery or resync with a sparse bitmap, io_sectors remains 0
when regions are skipped. Because last_check starts at 0, the check
(last_check + window > io_sectors) evaluates to true, causing the loop to
continue.
Since last_check is only updated after this continue statement, it remains 0
permanently across skipped regions. As a result, the preceding check for
(last_check == 0) remains true, calling md_new_event() on every skipped
iteration. This repeatedly wakes up user-space processes polling /proc/mdstat
and causes lock contention.
> -
> + }
> last_check = io_sectors;
> repeat:
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717062743.128189-1-yunye.zhao@linux.alibaba.com?part=1
^ permalink raw reply
* [PATCH] md: fix soft lockup during resync when sync is repeatedly skipped
From: Yunye Zhao @ 2026-07-17 6:27 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, Joseph Qi, linux-raid, linux-kernel, Yunye Zhao
md_do_sync()'s main loop advances io_sectors only when I/O is actually
issued (skipped == 0). When sync_request() keeps returning skipped == 1,
io_sectors never increases, the "last_check + window > io_sectors" test
stays true, and every iteration takes the continue branch:
sectors = mddev->pers->sync_request(mddev, j, max_sectors, &skipped);
...
if (!skipped)
io_sectors += sectors;
j += sectors;
...
if (last_check + window > io_sectors || j == max_sectors)
continue;
During recovery or resync of a large array with a sparse bitmap, many
regions that need no syncing are skipped:
raid10_sync_request()
md_bitmap_start_sync() -> must_sync = false (no bitmap page)
/* every mirror skipped */
biolist == NULL -> *skipped = 1; return max_sync;
j then traverses the whole skipped range while io_sectors stays
unchanged. On a non-preemptive kernel the resync thread (mdX_resync)
hogs the CPU for a long time and eventually triggers a soft lockup:
watchdog: BUG: soft lockup - CPU#149 stuck for 313s! [mdX_resync]
md_bitmap_start_sync+0x6f/0xe0
raid10_sync_request+0x2c9/0x1530 [raid10]
md_do_sync+0x810/0x1030
md_thread+0xa7/0x150
Add cond_resched() to this continue path.
Signed-off-by: Yunye Zhao <yunye.zhao@linux.alibaba.com>
---
drivers/md/md.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d1465bcd86c8..e7411b033490 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9881,9 +9881,10 @@ void md_do_sync(struct md_thread *thread)
*/
md_new_event();
- if (last_check + window > io_sectors || j == max_sectors)
+ if (last_check + window > io_sectors || j == max_sectors) {
+ cond_resched();
continue;
-
+ }
last_check = io_sectors;
repeat:
if (time_after_eq(jiffies, mark[last_mark] + SYNC_MARK_STEP )) {
--
2.19.1.6.gb485710b
^ permalink raw reply related
* Re: Subject: RFC: Read repair for md RAID1 after mirror read failures
From: G.W. Kant - Hunenet B.V. @ 2026-07-17 6:42 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, linux-raid@vger.kernel.org, yukuai@fygo.io
In-Reply-To: <m2ech4dt8k.fsf@f8410a22-cd7b-4baf-acf5-82353315d5e5.fritz.box>
On 7/15/26 8:04 PM, Abd-Alrhman Masalkhi wrote:
> On Wed, Jul 15, 2026 at 19:41 +0200, Abd-Alrhman Masalkhi wrote:
>> Hi Dion,
>>
>> On Wed, Jul 15, 2026 at 14:13 +0000, G. W. Kant wrote:
>>> Hi Abd,
>>>
>>> On 7/15/26 7:27 AM, Abd-Alrhman Masalkhi wrote:
>>>>> With today's 18–24 TB HDDs and backup/archive workloads, where data may
>>>>> remain unchanged for years, latent media degradation seems increasingly
>>>>> relevant. A successful read from the alternate mirror may be one of the
>>>>> last opportunities to refresh such a sector before it becomes
>>>>> permanently unreadable.
>>>>>
>>>> And Check/Repair is the right defense for cold archival data on large
>>>> drives.
>>>
>>>
>>> I have been looking into your suggestion that periodic check/repair is
>>> the appropriate defense for cold archival data on large drives.
>>>
>>> Before I start reading the resync code, I would like to make sure I
>>> understand what repair actually does.
>>>
>>> Suppose all mirrors read successfully and contain identical data. In
>>> other words, there are no read errors and no mismatches.
>>>
>>> Does repair simply verify the data and move on, or does it also rewrite
>>> each successfully read block to the corresponding LBA on every mirror?
>>>
>>> The distinction is important for what I have in mind.
>>>
>>> My concern is not recovery from an existing read error—that is already
>>> handled by fix_read_error()—but rather the long-term aging of cold
>>> magnetic recordings.
>>>
>>> If a sector is still readable today but is approaching the point where
>>> the drive's ECC will eventually no longer be able to reconstruct it,
>>> then a successful rewrite would effectively establish a new "write time"
>>> for that sector. If the drive can no longer reliably store the data at
>>> that location, the rewrite would also give the firmware an opportunity
>>> to remap the sector.
>>>
>>> So my question is essentially:
>>>
>>> Does repair perform such a rewrite of successfully read blocks, or does
>>> it only rewrite blocks when a read error or mismatch has already been
>>> detected?
>>>
>> Okay, I see what you're getting at now. No, check/repair won't rewrite
>> sectors that are read successfully and match across all mirrors. It only
>> rewrites data when it encounters a mismatch or needs to recover from a
>> read error.
>>
>> I think a full-array media refresh is a reasonable idea. but to my
>> knowledge, modern drives already perform background media scans that
>> refresh or remap marginal sectors, which covers part of this...
>>
>
> I'm not entirely sure about the media refresh part on modern drives...,
> I'll have to look into that first.
>
Maybe, we could ask the smartmontools people about this. They should
know this. But I have never read about a refresh action in their
documentation.
Best regards,
Dion
^ permalink raw reply
* Re: [PATCHv2] md: clear MD_CLOSING when array_state_store() bails on interrupted lock
From: sashiko-bot @ 2026-07-17 5:25 UTC (permalink / raw)
To: Jack Wang; +Cc: yukuai, linux-raid
In-Reply-To: <20260717051233.383373-1-jinpu.wang@ionos.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Calling `restart_array()` with the `mddev->lock` spinlock held can sleep because it clears the disk read-only flag, which triggers a `GFP_KERNEL` memory allocation for a uevent.
--
--- Patch [1]: [PATCHv2] md: clear MD_CLOSING when array_state_store() bails on interrupted lock ---
commit 3ae9b81fbeee45b412994652c2c3b3ac1b35d177
Author: Jack Wang <jinpu.wang@cloud.ionos.com>
md: clear MD_CLOSING when array_state_store() bails on interrupted lock
This commit ensures that MD_CLOSING is correctly cleared if
mutex_lock_interruptible() fails while writing to array_state. It introduces
a common exit label and a boolean flag to track whether the flag was actually
set by the current thread.
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d1465bcd86c81..7b7a4d9259079 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
[ ... ]
> @@ -4696,7 +4698,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
> }
> err = mddev_lock(mddev);
> if (err)
> - return err;
> + goto out_clear_closing;
[Severity: High]
This is a pre-existing issue, but just above this code in array_state_store(),
there is a fast path that can sleep while holding a spinlock.
If a privileged user sets the md device to read-only using
blockdev --setro /dev/md0 and then writes 'active' to
/sys/block/md0/md/array_state, the code acquires the mddev->lock spinlock
and calls restart_array():
drivers/md/md.c:array_state_store() {
...
spin_lock(&mddev->lock);
if (st == active) {
restart_array(mddev);
...
}
restart_array() unconditionally calls set_disk_ro() to ensure the disk is
read-write:
drivers/md/md.c:restart_array() {
...
set_disk_ro(disk, 0);
...
}
Because the disk was set to read-only, set_disk_ro() clears the flag and
triggers a uevent:
block/genhd.c:set_disk_ro() {
...
set_disk_ro_uevent(disk, read_only);
...
}
This calls into kobject_uevent_env(), which performs an unconditional
GFP_KERNEL allocation:
lib/kobject_uevent.c:kobject_uevent_env() {
...
devpath = kobject_get_path(kobj, GFP_KERNEL);
...
}
Can this sequence cause a sleep-in-atomic bug and potentially lead to a
deadlock or panic?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717051233.383373-1-jinpu.wang@ionos.com?part=1
^ permalink raw reply
* [PATCHv2] md: clear MD_CLOSING when array_state_store() bails on interrupted lock
From: Jack Wang @ 2026-07-17 5:12 UTC (permalink / raw)
To: Song Liu, Yu Kuai; +Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Jack Wang
From: Jack Wang <jinpu.wang@cloud.ionos.com>
Writing "clear", "readonly", "inactive" or "read_auto" to array_state
calls mddev_set_closing_and_sync_blockdev(), which sets MD_CLOSING so the
array cannot be reopened while it is being torn down. The flag is only
cleared again at the tail of array_state_store() (for readonly, read_auto,
inactive, or the failed-clear case).
Between setting the flag and reaching that tail there is an early return:
err = mddev_lock(mddev);
if (err)
return err;
mddev_lock() is mutex_lock_interruptible() on reconfig_mutex. If the
writing task is signalled while blocked there - easy to hit when the mutex
is held for a long time by a running resync/recovery or reshape - it
returns -EINTR and array_state_store() returns with MD_CLOSING still set.
do_md_stop()/md_set_readonly() never ran, so the array keeps working
internally, but every subsequent md_open() now returns -ENODEV for all
callers: the device still shows up in /proc/mdstat and sysfs is fully
populated, yet it cannot be opened and cannot be recovered without a
reboot (the remaining clear paths either need the device open or re-enter
test_and_set_bit(MD_CLOSING) and bail with -EBUSY before the clear).
Route the interrupted-lock exit through a common label so the flag is
released. Guard the clear with a set_closing flag that records whether
this write actually set MD_CLOSING: when mddev->pers is already NULL (for
example a concurrent STOP_ARRAY is mid-teardown) this write skips the
mddev_set_closing_and_sync_blockdev() call above, so it must not clear a
MD_CLOSING that the other thread owns - doing so could let the array be
reopened while it is being destroyed. This also tightens the pre-existing
tail clear, which had the same unconditional behaviour.
mddev_unlock() is correctly skipped on the goto path since the lock was
never acquired.
Fixes: 99b902ac1725 ("md: sync blockdev before stopping raid or setting readonly")
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
---
drivers/md/md.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d1465bcd86c8..7b7a4d925907 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4649,6 +4649,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
{
int err = 0;
enum array_state st = match_word(buf, array_states);
+ bool set_closing = false;
/* No lock dependent actions */
switch (st) {
@@ -4668,6 +4669,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
err = mddev_set_closing_and_sync_blockdev(mddev, 0);
if (err)
return err;
+ set_closing = true;
break;
default:
break;
@@ -4696,7 +4698,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
}
err = mddev_lock(mddev);
if (err)
- return err;
+ goto out_clear_closing;
switch (st) {
case inactive:
@@ -4769,8 +4771,17 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
}
mddev_unlock(mddev);
- if (st == readonly || st == read_auto || st == inactive ||
- (err && st == clear))
+out_clear_closing:
+ /*
+ * Only clear MD_CLOSING if this write actually set it. Otherwise a
+ * concurrent teardown (e.g. STOP_ARRAY) may own the flag - this write
+ * would have skipped setting it above when mddev->pers was already
+ * NULL - and clearing it here would let the array be reopened while it
+ * is being destroyed.
+ */
+ if (set_closing &&
+ (st == readonly || st == read_auto || st == inactive ||
+ (err && st == clear)))
clear_bit(MD_CLOSING, &mddev->flags);
return err ?: len;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] md: clear MD_CLOSING when array_state_store() bails on interrupted lock
From: sashiko-bot @ 2026-07-17 4:29 UTC (permalink / raw)
To: Jack Wang; +Cc: linux-raid, yukuai
In-Reply-To: <20260717041746.363740-1-jinpu.wang@ionos.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional clearing of MD_CLOSING on the error path can clear a flag owned by a concurrent thread, exposing the array to use-after-free races during teardown.
--
--- Patch [1]: [PATCH] md: clear MD_CLOSING when array_state_store() bails on interrupted lock ---
commit 919b922dd46da90c5c7e1a2b46106db8f5e8ffbf
Author: Jack Wang <jinpu.wang@cloud.ionos.com>
md: clear MD_CLOSING when array_state_store() bails on interrupted lock
This commit addresses a bug where an interrupted lock in array_state_store()
leaves MD_CLOSING set, preventing the array from being reopened. It modifies
the error path to jump to the existing cleanup logic to clear the flag.
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d1465bcd86c81..c4de2053593a0 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -4696,7 +4696,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
> }
> err = mddev_lock(mddev);
> if (err)
> - return err;
> + goto out_clear_closing;
[Severity: High]
Does this unconditionally clear MD_CLOSING even if this thread didn't set it?
If Thread A initiates array teardown (e.g., via md_ioctl(STOP_ARRAY)), it sets
MD_CLOSING, takes reconfig_mutex, and sets mddev->pers = NULL.
If Thread B concurrently writes "inactive" to array_state, it skips setting
MD_CLOSING because mddev->pers is NULL:
drivers/md/md.c:array_state_store() {
...
case inactive:
...
if (!mddev->pers || !md_is_rdwr(mddev))
break;
err = mddev_set_closing_and_sync_blockdev(mddev, 0);
...
}
Thread B then calls mddev_lock(), which blocks because Thread A holds the
reconfig_mutex. If Thread B receives a signal, mddev_lock() returns -EINTR,
and we jump to out_clear_closing:
>
> switch (st) {
> case inactive:
[ ... ]
> @@ -4769,6 +4769,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
> }
> mddev_unlock(mddev);
>
> +out_clear_closing:
> if (st == readonly || st == read_auto || st == inactive ||
> (err && st == clear))
> clear_bit(MD_CLOSING, &mddev->flags);
Because st == inactive, Thread B blindly clears MD_CLOSING, even though it
never set it.
Could this allow a third thread to successfully open the device (since
md_open() only checks MD_CLOSING) while Thread A is actively destroying the
array data structures, leading to a use-after-free?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260717041746.363740-1-jinpu.wang@ionos.com?part=1
^ permalink raw reply
* [PATCH] md: clear MD_CLOSING when array_state_store() bails on interrupted lock
From: Jack Wang @ 2026-07-17 4:17 UTC (permalink / raw)
To: Song Liu, Yu Kuai; +Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Jack Wang
From: Jack Wang <jinpu.wang@cloud.ionos.com>
Writing "clear", "readonly", "inactive" or "read_auto" to array_state
calls mddev_set_closing_and_sync_blockdev(), which sets MD_CLOSING so the
array cannot be reopened while it is being torn down. The flag is only
cleared again at the tail of array_state_store() (for readonly, read_auto,
inactive, or the failed-clear case).
Between setting the flag and reaching that tail there is an early return:
err = mddev_lock(mddev);
if (err)
return err;
mddev_lock() is mutex_lock_interruptible() on reconfig_mutex. If the
writing task is signalled while blocked there - easy to hit when the mutex
is held for a long time by a running resync/recovery or reshape - it
returns -EINTR and array_state_store() returns with MD_CLOSING still set.
do_md_stop()/md_set_readonly() never ran, so the array keeps working
internally, but every subsequent md_open() now returns -ENODEV for all
callers: the device still shows up in /proc/mdstat and sysfs is fully
populated, yet it cannot be opened and cannot be recovered without a
reboot (the remaining clear paths either need the device open or re-enter
test_and_set_bit(MD_CLOSING) and bail with -EBUSY before the clear).
Route the interrupted-lock exit through the existing MD_CLOSING clearing
logic instead of returning directly, mirroring the goto-out pattern
md_ioctl() already uses for the same flag. mddev_unlock() is correctly
skipped since the lock was never acquired.
Fixes: 99b902ac1725 ("md: sync blockdev before stopping raid or setting readonly")
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
---
drivers/md/md.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d1465bcd86c8..c4de2053593a 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4696,7 +4696,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
}
err = mddev_lock(mddev);
if (err)
- return err;
+ goto out_clear_closing;
switch (st) {
case inactive:
@@ -4769,6 +4769,7 @@ array_state_store(struct mddev *mddev, const char *buf, size_t len)
}
mddev_unlock(mddev);
+out_clear_closing:
if (st == readonly || st == read_auto || st == inactive ||
(err && st == clear))
clear_bit(MD_CLOSING, &mddev->flags);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v5 4/5] dm crypt: batch a bio segment's sectors via dun()
From: kernel test robot @ 2026-07-16 13:34 UTC (permalink / raw)
To: Leonid Ravich
Cc: oe-lkp, lkp, linux-raid, dm-devel, linux-crypto, linux-block,
linux-kernel, herbert, davem, ebiggers, snitzer, mpatocka, axboe,
oliver.sang
In-Reply-To: <20260630083431.2772-5-lravich@amazon.com>
Hello,
kernel test robot noticed "RIP:alg_test.cold" on:
commit: 89bd605959ed9145ac2906d1cc4357b71eee9d3b ("[PATCH v5 4/5] dm crypt: batch a bio segment's sectors via dun()")
url: https://github.com/intel-lab-lkp/linux/commits/Leonid-Ravich/crypto-skcipher-add-per-request-data_unit_size/20260701-185738
patch link: https://lore.kernel.org/all/20260630083431.2772-5-lravich@amazon.com/
patch subject: [PATCH v5 4/5] dm crypt: batch a bio segment's sectors via dun()
in testcase: kunit
version:
with following parameters:
group: group-00
config: x86_64-rhel-9.4-kunit
compiler: gcc-14
test machine: 10 threads 1 sockets Intel(R) Core Ultra Series 3 @ 3.20GHz (Panther Lake) with 16G memory
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202607161535.5e935cc3-lkp@intel.com
kern :warn : [ 136.442332] [ T2675] ------------[ cut here ]------------
kern :warn : [ 136.442345] [ T2675] alg: self-tests for cbc(aes) using cbc-aes-aesni failed (rc=-110)
kern :warn : [ 136.442348] [ T2675] WARNING: crypto/testmgr.c:6087 at alg_test.cold+0x9c/0xb1, CPU#5: cryptomgr_test/2675
kern :warn : [ 136.442364] [ T2675] Modules linked in: dun
kern :warn : [ 136.442373] [ T2675] CPU: 5 UID: 0 PID: 2675 Comm: cryptomgr_test Not tainted 7.1.0-rc3+ #1 PREEMPT(lazy)
kern :warn : [ 136.442382] [ T2675] Hardware name: Intel Corporation Panther Lake Client Platform/PTL-UH LP5 T3 RVP1, BIOS PTLPFWI1.R00.3332.D01.2508190102 08/19/2025
kern :warn : [ 136.442389] [ T2675] RIP: 0010:alg_test.cold (testmgr.c:6087 (discriminator 1))
kern :warn : [ 136.442395] [ T2675] Code: 89 de 48 c7 c7 e0 44 93 84 e8 9b c2 fb ff 41 83 fc fe 0f 84 b1 e8 43 01 48 8d 3d 7a 80 85 05 48 8b 54 24 08 44 89 e1 48 89 de <67> 48 0f b9 3a e9 95 e8 43 01 48 8b 14 24 b8 ff ff 37 00 48 c1 e0
All code
========
0: 89 de mov %ebx,%esi
2: 48 c7 c7 e0 44 93 84 mov $0xffffffff849344e0,%rdi
9: e8 9b c2 fb ff call 0xfffffffffffbc2a9
e: 41 83 fc fe cmp $0xfffffffe,%r12d
12: 0f 84 b1 e8 43 01 je 0x143e8c9
18: 48 8d 3d 7a 80 85 05 lea 0x585807a(%rip),%rdi # 0x5858099
1f: 48 8b 54 24 08 mov 0x8(%rsp),%rdx
24: 44 89 e1 mov %r12d,%ecx
27: 48 89 de mov %rbx,%rsi
2a:* 67 48 0f b9 3a ud1 (%edx),%rdi <-- trapping instruction
2f: e9 95 e8 43 01 jmp 0x143e8c9
34: 48 8b 14 24 mov (%rsp),%rdx
38: b8 ff ff 37 00 mov $0x37ffff,%eax
3d: 48 rex.W
3e: c1 .byte 0xc1
3f: e0 .byte 0xe0
Code starting with the faulting instruction
===========================================
0: 67 48 0f b9 3a ud1 (%edx),%rdi
5: e9 95 e8 43 01 jmp 0x143e89f
a: 48 8b 14 24 mov (%rsp),%rdx
e: b8 ff ff 37 00 mov $0x37ffff,%eax
13: 48 rex.W
14: c1 .byte 0xc1
15: e0 .byte 0xe0
kern :warn : [ 136.442404] [ T2675] RSP: 0018:ffffc9000219fce0 EFLAGS: 00010293
kern :warn : [ 136.442411] [ T2675] RAX: 0000000000000041 RBX: ffff888474e11480 RCX: 00000000ffffff92
kern :warn : [ 136.442416] [ T2675] RDX: ffff888474e11400 RSI: ffff888474e11480 RDI: ffffffff86abc5a0
kern :warn : [ 136.442421] [ T2675] RBP: 1ffff92000433fa5 R08: 0000000000000000 R09: fffffbfff0b2b848
kern :warn : [ 136.442426] [ T2675] R10: 0000000000000003 R11: fefefefefefefeff R12: 00000000ffffff92
kern :warn : [ 136.442431] [ T2675] R13: dffffc0000000000 R14: 0000000000000069 R15: 0000000000000027
kern :warn : [ 136.442436] [ T2675] FS: 0000000000000000(0000) GS:ffff88848572b000(0000) knlGS:0000000000000000
kern :warn : [ 136.442442] [ T2675] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kern :warn : [ 136.442446] [ T2675] CR2: 00007fc9e48120ac CR3: 000000047c074001 CR4: 0000000108770ef0
kern :warn : [ 136.442451] [ T2675] PKRU: 55555554
kern :warn : [ 136.442455] [ T2675] Call Trace:
kern :warn : [ 136.442458] [ T2675] <TASK>
kern :warn : [ 136.442464] [ T2675] ? __pfx_alg_test (linux/crypto.h:449)
kern :warn : [ 136.442470] [ T2675] ? kasan_save_track (kasan/common.c:78)
kern :warn : [ 136.442477] [ T2675] ? _raw_spin_lock_irqsave (linux/instrumented.h:55 linux/atomic/atomic-instrumented.h:1301 asm-generic/qspinlock.h:111 linux/spinlock.h:187 linux/spinlock_api_smp.h:133 locking/spinlock.c:166)
kern :warn : [ 136.442483] [ T2675] ? __pfx__raw_spin_lock_irqsave (locking/spinlock.c:273)
kern :warn : [ 136.442489] [ T2675] ? __pfx_cryptomgr_test (usercopy_64.c:?)
kern :warn : [ 136.442496] [ T2675] ? kthread_affine_node (kthread.c:377)
kern :warn : [ 136.442501] [ T2675] ? __kthread_parkme (linux/instrumented.h:82 asm-generic/bitops/instrumented-non-atomic.h:141 kthread.c:272)
kern :warn : [ 136.442508] [ T2675] ? __pfx_cryptomgr_test (usercopy_64.c:?)
kern :warn : [ 136.442513] [ T2675] cryptomgr_test (algboss.c:179)
kern :warn : [ 136.442519] [ T2675] kthread (kthread.c:436)
kern :warn : [ 136.442523] [ T2675] ? recalc_sigpending (linux/instrumented.h:97 asm-generic/bitops/instrumented-atomic.h:41 linux/thread_info.h:109 signal.c:181)
kern :warn : [ 136.442530] [ T2675] ? __pfx_kthread (linux/list.h:381 (discriminator 2))
kern :warn : [ 136.442534] [ T2675] ret_from_fork (x86/kernel/process.c:158)
kern :warn : [ 136.442541] [ T2675] ? __pfx_ret_from_fork (x86/include/asm/entry-common.h:54)
kern :warn : [ 136.442547] [ T2675] ? switch_fpu (linux/instrumented.h:82 asm-generic/bitops/instrumented-non-atomic.h:141 linux/thread_info.h:133 linux/sched.h:2069 x86/include/asm/fpu/sched.h:34)
kern :warn : [ 136.442553] [ T2675] ? __switch_to (x86/kernel/process_64.c:403 x86/kernel/process_64.c:663)
kern :warn : [ 136.442558] [ T2675] ? __pfx_kthread (linux/list.h:381 (discriminator 2))
kern :warn : [ 136.442563] [ T2675] ret_from_fork_asm (x86/entry/entry_64.S:245)
kern :warn : [ 136.442570] [ T2675] </TASK>
kern :warn : [ 136.442573] [ T2675] ---[ end trace 0000000000000000 ]---
kern :err : [ 136.442580] [ T2675] alg: Unexpected test result for cbc-aes-aesni: -110
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20260716/202607161535.5e935cc3-lkp@intel.com
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v4 7/7] md/raid10: simplify read request error handling
From: yu kuai @ 2026-07-16 9:52 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, magiclinan, xiao, axboe, vverma,
john.g.garry, martin.petersen, linux-kernel, yu kuai
Cc: linux-raid
In-Reply-To: <20260710101521.1714-8-abd.masalkhi@gmail.com>
Hi,
在 2026/7/10 18:15, Abd-Alrhman Masalkhi 写道:
> 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 v4:
> - No changes.
> - Link to v3: https://lore.kernel.org/linux-raid/20260708101341.473750-8-abd.masalkhi@gmail.com/
>
> Changes in v3:
> - No changes.
> - Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-8-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(-)
I don't think patch 6 and 7 are actually simplifications, and 7 in particular introduces
a fragile dependency I'd rather not take as-is. The diff lines also said this and I'd
rather not take as-is.
>
> 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.
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v4 5/7] md/raid10: replace wait loop with wait_event_idle()
From: yu kuai @ 2026-07-16 9:25 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, magiclinan, xiao, axboe, vverma,
john.g.garry, martin.petersen, linux-kernel, yu kuai
Cc: linux-raid
In-Reply-To: <20260710101521.1714-6-abd.masalkhi@gmail.com>
在 2026/7/10 18:15, Abd-Alrhman Masalkhi 写道:
> 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 v4:
> - No changes.
> - Link to v3:https://lore.kernel.org/linux-raid/20260708101341.473750-6-abd.masalkhi@gmail.com/
>
> Changes in v3:
> - No changes.
> - Link to v2:https://lore.kernel.org/linux-raid/20260628142420.1051027-6-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(-)
Reviewed-by: Yu Kuai <yukuai@fygo.io>
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v4 4/7] md/raid10: remove unnecessary barrier around bio_submit_split_bioset()
From: yu kuai @ 2026-07-16 9:24 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, magiclinan, xiao, axboe, vverma,
john.g.garry, martin.petersen, linux-kernel, yu kuai
Cc: linux-raid
In-Reply-To: <20260710101521.1714-5-abd.masalkhi@gmail.com>
在 2026/7/10 18:15, Abd-Alrhman Masalkhi 写道:
> raid10_write_request() drops the barrier before calling
> bio_submit_split_bioset() and reacquires it afterwards. This is no
> longer necessary because the split bio cannot re-enter
> raid10_write_request() while the barrier is held.
>
> The allow_barrier()/wait_barrier() pair was introduced by commit
> e820d55cb99d ("md: fix raid10 hang issue caused by barrier") when
> submit_flushes() called md_handle_request() directly, allowing re-entry
> into raid10_write_request(). Since v5.2, submit_flushes() has instead
> gone through submit_bio(), eliminating that recursion. submit_flushes()
> was later removed entirely by commit b75197e86e6d ("md: Remove flush
> handling").
>
> Currently, raid10_write_request() is only entered from the bio
> submission path, so the split bio submitted by bio_submit_split_bioset()
> cannot recurse back into wait_barrier().
>
> Remove the redundant allow_barrier()/wait_barrier() pair around
> bio_submit_split_bioset().
>
> Signed-off-by: Abd-Alrhman Masalkhi<abd.masalkhi@gmail.com>
> ---
> Changes in v4:
> - No changes.
> - Link to v3:https://lore.kernel.org/linux-raid/20260708101341.473750-5-abd.masalkhi@gmail.com/
>
> Changes in v3:
> - No changes.
> - Link to v2:https://lore.kernel.org/linux-raid/20260628142420.1051027-5-abd.masalkhi@gmail.com/
>
> Changes in v2:
> - Expand the commit message to explain why the
> allow_barrier()/wait_barrier() pair is no longer needed.
> - Link to v1:https://lore.kernel.org/linux-raid/20260623072456.333437-5-abd.masalkhi@gmail.com/
> ---
> drivers/md/raid10.c | 2 --
> 1 file changed, 2 deletions(-)
Reviewed-by: Yu Kuai <yukuai@fygo.io>
--
Thanks,
Kuai
^ permalink raw reply
* Re: Subject: RFC: Read repair for md RAID1 after mirror read failures
From: Abd-Alrhman Masalkhi @ 2026-07-15 18:04 UTC (permalink / raw)
To: G.W. Kant - Hunenet B.V., linux-raid@vger.kernel.org,
yukuai@fygo.io
In-Reply-To: <m2jyqwduba.fsf@f8410a22-cd7b-4baf-acf5-82353315d5e5.fritz.box>
On Wed, Jul 15, 2026 at 19:41 +0200, Abd-Alrhman Masalkhi wrote:
> Hi Dion,
>
> On Wed, Jul 15, 2026 at 14:13 +0000, G. W. Kant wrote:
>> Hi Abd,
>>
>> On 7/15/26 7:27 AM, Abd-Alrhman Masalkhi wrote:
>>>> With today's 18–24 TB HDDs and backup/archive workloads, where data may
>>>> remain unchanged for years, latent media degradation seems increasingly
>>>> relevant. A successful read from the alternate mirror may be one of the
>>>> last opportunities to refresh such a sector before it becomes
>>>> permanently unreadable.
>>>>
>>> And Check/Repair is the right defense for cold archival data on large
>>> drives.
>>
>>
>> I have been looking into your suggestion that periodic check/repair is
>> the appropriate defense for cold archival data on large drives.
>>
>> Before I start reading the resync code, I would like to make sure I
>> understand what repair actually does.
>>
>> Suppose all mirrors read successfully and contain identical data. In
>> other words, there are no read errors and no mismatches.
>>
>> Does repair simply verify the data and move on, or does it also rewrite
>> each successfully read block to the corresponding LBA on every mirror?
>>
>> The distinction is important for what I have in mind.
>>
>> My concern is not recovery from an existing read error—that is already
>> handled by fix_read_error()—but rather the long-term aging of cold
>> magnetic recordings.
>>
>> If a sector is still readable today but is approaching the point where
>> the drive's ECC will eventually no longer be able to reconstruct it,
>> then a successful rewrite would effectively establish a new "write time"
>> for that sector. If the drive can no longer reliably store the data at
>> that location, the rewrite would also give the firmware an opportunity
>> to remap the sector.
>>
>> So my question is essentially:
>>
>> Does repair perform such a rewrite of successfully read blocks, or does
>> it only rewrite blocks when a read error or mismatch has already been
>> detected?
>>
> Okay, I see what you're getting at now. No, check/repair won't rewrite
> sectors that are read successfully and match across all mirrors. It only
> rewrites data when it encounters a mismatch or needs to recover from a
> read error.
>
> I think a full-array media refresh is a reasonable idea. but to my
> knowlage, modern drives already perform background media scans that
> refresh or remap marginal sectors, which covers part of this...
>
I'm not entirely sure about the media refresh part on modern drives...,
I'll have to look into that first.
>> Regards,
>>
>> Dion
>
> --
> Best Regards,
> Abd-Alrhman
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: Subject: RFC: Read repair for md RAID1 after mirror read failures
From: Abd-Alrhman Masalkhi @ 2026-07-15 17:53 UTC (permalink / raw)
To: Yu Kuai, G.W. Kant - Hunenet B.V., linux-raid@vger.kernel.org,
yu kuai
In-Reply-To: <2718fd27-9453-4dea-9d1d-13cfecef4e6c@fnnas.com>
Hi Kuai,
On Wed, Jul 15, 2026 at 15:27 +0800, Yu Kuai wrote:
> Hi,
>
> 在 2026/7/15 15:10, G.W. Kant - Hunenet B.V. 写道:
>> On 7/15/26 7:27 AM, Abd-Alrhman Masalkhi wrote:
>>>> In other words, the first successfully recovered read request could
>>>> automatically become a repair opportunity. The repair could even be
>>>> scheduled asynchronously, so the successful read is returned immediately
>>>> while the rewrite is performed in the background. Unlike a periodic
>>>> resync, this repair would be driven by an actual read failure, making it
>>>> targeted rather than rewriting the entire mirror.
>>>>
>>> Yes, md has had this for a long time. Look at fix_read_error() in
>>> raid1.c. It is called from handle_read_error() on any failed read. It
>>> reads from a healthy mirror and rewrites the bad region on the failing
>>> device, giving the drive a chance to rewrite or remap the sector. If the
>>> rewrite fails, it records a bad block. md does this synchronously under
>>> a frozen array, so it is not a missing feature.
>>>
>>> The likely reason you didn't see it is that your array was already
>>> degraded, so there was no healthy in-array copy for fix_read_error() to
>>> recover from. In your case, you were likely able to retrieve the data
>>> due to btrfs level redundancy, and md can't repair across arrays.
>>>
>>>> With today's 18–24 TB HDDs and backup/archive workloads, where data may
>>>> remain unchanged for years, latent media degradation seems increasingly
>>>> relevant. A successful read from the alternate mirror may be one of the
>>>> last opportunities to refresh such a sector before it becomes
>>>> permanently unreadable.
>>>>
>>> And Check/Repair is the right defense for cold archival data on large
>>> drives.
>> Thank you. fix_read_error() was exactly the piece I was looking for. I
>> had missed that md already performs read repair when another healthy
>> mirror is available.
>>
>> My situation was indeed different because the array had already lost one
>> member, so the successful read originated from Btrfs redundancy across
>> another md RAID1 rather than from the surviving md mirror.
>>
>> One question remains, though. check/repair periodically reads all
>> sectors, but as far as I understand it, successfully read sectors are
>> not rewritten. From a media aging perspective this is a different
>> problem than recovering from a read error. Have there ever been
>> discussions about a true media refresh pass that rewrites successfully
>> read sectors to refresh long-lived magnetic recordings?
>
> Perhaps this is what you looking for:
>
> [RFC v2 0/5] md/raid1: introduce a new sync action to repair badblocks -
> Zheng Qixing
> <https://lore.kernel.org/all/20260203061259.609206-1-zhengqixing@huaweicloud.com/>
>
> However, AFAIK, Qixing no longer work in this area and there will not be
> new version.
>
It seems interesting. Do you think it's worth carrying it forward?
>>
>> Best regards,
>> Dion Kant
>
> --
> Thanks,
> Kuai
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: Subject: RFC: Read repair for md RAID1 after mirror read failures
From: Abd-Alrhman Masalkhi @ 2026-07-15 17:41 UTC (permalink / raw)
To: G.W. Kant - Hunenet B.V., linux-raid@vger.kernel.org,
yukuai@fygo.io
In-Reply-To: <cff97a09-ee38-4785-9a45-1fd562fbaf5e@hunenet.nl>
Hi Dion,
On Wed, Jul 15, 2026 at 14:13 +0000, G. W. Kant wrote:
> Hi Abd,
>
> On 7/15/26 7:27 AM, Abd-Alrhman Masalkhi wrote:
>>> With today's 18–24 TB HDDs and backup/archive workloads, where data may
>>> remain unchanged for years, latent media degradation seems increasingly
>>> relevant. A successful read from the alternate mirror may be one of the
>>> last opportunities to refresh such a sector before it becomes
>>> permanently unreadable.
>>>
>> And Check/Repair is the right defense for cold archival data on large
>> drives.
>
>
> I have been looking into your suggestion that periodic check/repair is
> the appropriate defense for cold archival data on large drives.
>
> Before I start reading the resync code, I would like to make sure I
> understand what repair actually does.
>
> Suppose all mirrors read successfully and contain identical data. In
> other words, there are no read errors and no mismatches.
>
> Does repair simply verify the data and move on, or does it also rewrite
> each successfully read block to the corresponding LBA on every mirror?
>
> The distinction is important for what I have in mind.
>
> My concern is not recovery from an existing read error—that is already
> handled by fix_read_error()—but rather the long-term aging of cold
> magnetic recordings.
>
> If a sector is still readable today but is approaching the point where
> the drive's ECC will eventually no longer be able to reconstruct it,
> then a successful rewrite would effectively establish a new "write time"
> for that sector. If the drive can no longer reliably store the data at
> that location, the rewrite would also give the firmware an opportunity
> to remap the sector.
>
> So my question is essentially:
>
> Does repair perform such a rewrite of successfully read blocks, or does
> it only rewrite blocks when a read error or mismatch has already been
> detected?
>
Okay, I see what you're getting at now. No, check/repair won't rewrite
sectors that are read successfully and match across all mirrors. It only
rewrites data when it encounters a mismatch or needs to recover from a
read error.
I think a full-array media refresh is a reasonable idea. but to my
knowlage, modern drives already perform background media scans that
refresh or remap marginal sectors, which covers part of this...
> Regards,
>
> Dion
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: Subject: RFC: Read repair for md RAID1 after mirror read failures
From: Dragan Milivojević @ 2026-07-15 17:01 UTC (permalink / raw)
To: John Stoffel
Cc: G.W. Kant - Hunenet B.V., Abd-Alrhman Masalkhi,
linux-raid@vger.kernel.org, yukuai@fygo.io
In-Reply-To: <27223.46342.543928.404779@quad.stoffel.home>
> It's not a simple problem. For long term archiving, I would try to
> use something with RAID6 or more, and ideally have a filesystem which
> also checksums data. This is what ZFS does, and which bcachefs does
> as well, but since they're both out of kernel, I don't use either with
> my current home linux setup.
>
> But for long term storage, that could be the way to go. but then you
> need to periodically scrub your data, which means reading it all,
> checking the checksums and re-writing and fixing corruption.
>
> > My concern is not recovery from an existing read error—that is already
> > handled by fix_read_error()—but rather the long-term aging of cold
> > magnetic recordings.
>
> You really need three or more disks with multiple parity and checksums
> for long term storage.
dm-integrity ...
^ permalink raw reply
* Re: Subject: RFC: Read repair for md RAID1 after mirror read failures
From: John Stoffel @ 2026-07-15 16:27 UTC (permalink / raw)
To: G.W. Kant - Hunenet B.V.
Cc: Abd-Alrhman Masalkhi, linux-raid@vger.kernel.org, yukuai@fygo.io
In-Reply-To: <cff97a09-ee38-4785-9a45-1fd562fbaf5e@hunenet.nl>
>>>>> "G" == "G W Kant <- Hunenet B.V." <g.w.kant@hunenet.nl>> writes:
> Hi Abd,
> On 7/15/26 7:27 AM, Abd-Alrhman Masalkhi wrote:
>>> With today's 18–24 TB HDDs and backup/archive workloads, where data may
>>> remain unchanged for years, latent media degradation seems increasingly
>>> relevant. A successful read from the alternate mirror may be one of the
>>> last opportunities to refresh such a sector before it becomes
>>> permanently unreadable.
>>>
>> And Check/Repair is the right defense for cold archival data on large
>> drives.
> I have been looking into your suggestion that periodic check/repair is
> the appropriate defense for cold archival data on large drives.
> Before I start reading the resync code, I would like to make sure I
> understand what repair actually does.
> Suppose all mirrors read successfully and contain identical data. In
> other words, there are no read errors and no mismatches.
> Does repair simply verify the data and move on, or does it also rewrite
> each successfully read block to the corresponding LBA on every mirror?
> The distinction is important for what I have in mind.
So you have to also think of other situations. You have a RAID1
array, and one block gets corrupted, but is still readable. How do
you tell which block is the proper one? You need checksums or parity
or more copies to check against for consistency.
It's not a simple problem. For long term archiving, I would try to
use something with RAID6 or more, and ideally have a filesystem which
also checksums data. This is what ZFS does, and which bcachefs does
as well, but since they're both out of kernel, I don't use either with
my current home linux setup.
But for long term storage, that could be the way to go. but then you
need to periodically scrub your data, which means reading it all,
checking the checksums and re-writing and fixing corruption.
> My concern is not recovery from an existing read error—that is already
> handled by fix_read_error()—but rather the long-term aging of cold
> magnetic recordings.
You really need three or more disks with multiple parity and checksums
for long term storage.
> If a sector is still readable today but is approaching the point where
> the drive's ECC will eventually no longer be able to reconstruct it,
> then a successful rewrite would effectively establish a new "write time"
> for that sector. If the drive can no longer reliably store the data at
> that location, the rewrite would also give the firmware an opportunity
> to remap the sector.
> So my question is essentially:
> Does repair perform such a rewrite of successfully read blocks, or does
> it only rewrite blocks when a read error or mismatch has already been
> detected?
> Regards,
> Dion
^ permalink raw reply
* Re: [PATCH 4/8] xor/kunit: add a benchmark
From: sashiko-bot @ 2026-07-15 15:05 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: yukuai, linux-raid
In-Reply-To: <20260715144825.95432-5-hch@lst.de>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unbounded preemption-disabled region in `xor_benchmark` causes softlockup panics and RCU stall warnings on slower architectures.
--
--- Patch [4]: [PATCH 4/8] xor/kunit: add a benchmark ---
commit f403ad2f80a6eb7a690f9b948152ad30281caf15
Author: Christoph Hellwig <hch@lst.de>
xor/kunit: add a benchmark
This commit adds a benchmark to the XOR KUnit test suite to evaluate
performance across varying block sizes and numbers of disks. The benchmark
reports runtime speeds in GB/s.
> diff --git a/lib/raid/xor/tests/xor_kunit.c b/lib/raid/xor/tests/xor_kunit.c
> --- a/lib/raid/xor/tests/xor_kunit.c
> +++ b/lib/raid/xor/tests/xor_kunit.c
[ ... ]
> +static void xor_benchmark(struct kunit *test)
> +{
[ ... ]
> + for (j = 0; j < ARRAY_SIZE(len_to_test); j++) {
> + unsigned int len = len_to_test[j];
> + const unsigned long num_iters = 1000;
> +
> + KUNIT_ASSERT_GT(test, len, 0);
> + KUNIT_ASSERT_LE(test, len, XOR_KUNIT_MAX_BYTES);
> +
> + preempt_disable();
> + t = ktime_get_ns();
> + for (l = 0; l < num_iters; l++)
> + xor_gen(test_dest, test_buffers, nr, len);
> + t = max(ktime_get_ns() - t, 1);
> + preempt_enable();
[Severity: High]
Will this preemption-disabled region cause softlockup panics or RCU stalls
on slower architectures?
In xor_benchmark(), preempt_disable() is called before processing 1,000
iterations of up to 32 blocks of 16KB each. On a slow emulated architecture
without hardware acceleration, this loop will synchronously process over
500 MB of memory.
If the memory bandwidth is low enough, could this tight loop exceed the
default 20-second softlockup and 21-second RCU stall timeouts, resulting
in a kernel panic during CI testing?
Would it be safer to place the preemption-disabled block inside the
iteration loop and accumulate the time, or dynamically scale the number
of iterations based on a time threshold so the scheduler has a chance to run?
> +
> + speed[j] = div64_u64((u64)len * num_iters * nr, t);
> + }
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715144825.95432-1-hch@lst.de?part=4
^ permalink raw reply
* Re: [PATCH 2/8] xor: improve the runtime selection benchmark
From: sashiko-bot @ 2026-07-15 15:04 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-raid, yukuai
In-Reply-To: <20260715144825.95432-3-hch@lst.de>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Replacing clock-tick synchronization with a fixed number of loop iterations causes zero-duration time deltas on coarse clocks, leading to artificial throughput inflation, integer overflow, and random algorithm selection.
--
--- Patch [2]: [PATCH 2/8] xor: improve the runtime selection benchmark ---
commit 2205984bf337f5c013e14971e819bb6346e19853
Author: Christoph Hellwig <hch@lst.de>
xor: improve the runtime selection benchmark
This commit modifies the XOR block benchmark to use ktime_get_ns for timing
and switches to 4 + 1 disks for a more realistic load. It also reports the
throughput on the data disks instead of the parity disk.
> diff --git a/lib/raid/xor/xor-core.c b/lib/raid/xor/xor-core.c
> index 50931fbf0324e..f9d61b096b117 100644
> --- a/lib/raid/xor/xor-core.c
> +++ b/lib/raid/xor/xor-core.c
[ ... ]
> -#define BENCH_SIZE 4096
> +#define BENCH_SIZE SZ_4K
> +#define NR_SRCS 4
> #define REPS 800U
>
> -static void __init
> -do_xor_speed(struct xor_block_template *tmpl, void *b1, void *b2)
> +static void __init do_xor_speed(struct xor_block_template *tmpl, void *dest,
> + void *srcs[NR_SRCS])
> {
> - int speed;
> - unsigned long reps;
> - ktime_t min, start, t0;
> - void *srcs[1] = { b2 };
> + u64 t;
> + int i;
>
> preempt_disable();
> -
> - reps = 0;
> - t0 = ktime_get();
> - /* delay start until time has advanced */
> - while ((start = ktime_get()) == t0)
> - cpu_relax();
> - do {
> + t = ktime_get_ns();
> + for (i = 0; i < REPS; i++) {
> mb(); /* prevent loop optimization */
> - tmpl->xor_gen(b1, srcs, 1, BENCH_SIZE);
> + tmpl->xor_gen(dest, srcs, NR_SRCS, BENCH_SIZE);
> mb();
> - } while (reps++ < REPS || (t0 = ktime_get()) == start);
> - min = ktime_sub(t0, start);
> -
> + }
> + t = max(ktime_get_ns() - t, 1);
[Severity: High]
Does removing the clock-tick synchronization create an issue with low
resolution clocks?
If a high-resolution clocksource is not yet available during early boot,
the 800 loop iterations might complete within a single clock tick. If that
happens, the elapsed time t evaluates to 0 and gets clamped to 1.
> preempt_enable();
>
> - // bytes/ns == GB/s, multiply by 1000 to get MB/s [not MiB/s]
> - speed = (1000 * reps * BENCH_SIZE) / (unsigned int)ktime_to_ns(min);
> - tmpl->speed = speed;
> + /* bytes/ns == GB/s, multiply by 1000 to get MB/s [not MiB/s] */
> + tmpl->speed = div64_u64((u64)BENCH_SIZE * REPS * NR_SRCS * 1000, t);
[Severity: High]
Can this division silently overflow the 32-bit signed integer tmpl->speed?
If t is clamped to 1 ns, it yields a quotient of approximately 13.1 billion.
This overflows the 32-bit integer and could result in negative or
astronomically high throughput values.
This might break the calibration logic and cause the kernel to select a
suboptimal XOR algorithm.
>
> - pr_info(" %-16s: %5d MB/sec\n", tmpl->name, speed);
> + pr_info(" %-16s: %5d MB/sec\n", tmpl->name, tmpl->speed);
> }
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260715144825.95432-1-hch@lst.de?part=2
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox