[PATCH v2 0/2] md/raid5: fix PPL disk add and replacement removal

[PATCH v2 0/2] md/raid5: fix PPL disk add and replacement removal

[Yingjie Wang]

This is v2 of the two independent PPL disk-transition fixes. The first
patch is unchanged. The second patch also handles a slot that remains
assigned above the current array width after a RAID5 shrink.

When a newly added disk fails PPL setup, raid5_add_disk() can publish the
private disk pointer before the caller rolls back raid_disk. A later
removal can then free the disk while RAID5 still references it. Patch 1
prepares the PPL log before publishing the pointer and restores the
previous state on failure.

Patch 2 keeps the active PPL log attached when an unpromoted replacement
is removed. It also skips log_modify() when removing an active device from
a slot at or above the current raid_disks value. Such a slot has no PPL
child log after a shrink, but its device pointer still has to be cleared.

Changes in v2:

  * guard the active-device log update with number < conf->raid_disks;
  * add a shrink test that leaves an extra slot blocked, enables PPL, and
    compares the v1 and v2 removal results.

The v1 public thread is:
https://lore.kernel.org/r/cover.1794215128.git.x-professor@outlook.com/

Tested on x86_64 with KASAN, PROVE_LOCKING, DEBUG_ATOMIC_SLEEP, LOCKDEP,
MD, and RAID456 enabled:

  * per-commit W=1 builds for both commits;
  * failed PPL disk addition, which reaches DONE on the v2 tip;
  * unpromoted replacement removal, which reaches PASS and DONE on the
    v2 tip, with tracing observing `ppl_write_stripe()` and a successful
    return during a subsequent array write;
  * the shrink comparison, where the sent v1 remains at slot 3 after
    three remove attempts while v2 removes the slot on the first attempt;
  * empty-header `-EIO` injection while adding a spare, where `slot=0`
    returns an error, leaves the slot unassigned, restores `ppl_size`, and
    leaves the degraded array writable.

The runtime tests use native metadata 1.2. IMSM external metadata was not
run. The empty-header failure uses a dedicated kernel with
`CONFIG_FAIL_MAKE_REQUEST` enabled; the separate page-allocation `-ENOMEM`
injection was not run.

Yingjie Wang (2):
  md/raid5: roll back failed PPL disk additions
  md/raid5: preserve active PPL log when removing a replacement

 drivers/md/raid5-ppl.c | 11 ++++++++++-
 drivers/md/raid5.c     | 13 +++++++++----
 2 files changed, 19 insertions(+), 5 deletions(-)


base-commit: 47e1b18f38d74015cb9fd30b5ff2a1eb88e77d31
-- 
2.43.0

[PATCH v2 1/2] md/raid5: roll back failed PPL disk additions

[Yingjie Wang]

raid5_add_disk() publishes a new device in conf->disks[] before
preparing its PPL log. If PPL validation or the empty-header write
fails, the caller resets rdev->raid_disk, but the RAID5 private pointer
remains. A later removal can free the device while RAID5 still
references it.

Prepare the PPL log before publishing the RAID5 disk pointer and reset
rdev->raid_disk to -1 when preparation fails. If empty-header setup
fails after the PPL log pointer is published, restore the previous log
pointer and PPL size and do not initialize the child log.

Fixes: 6358c239d88c ("raid5-ppl: support disk hot add/remove with PPL")
Cc: stable@vger.kernel.org
Signed-off-by: Yingjie Wang <x-professor@outlook.com>
---
 drivers/md/raid5-ppl.c | 11 ++++++++++-
 drivers/md/raid5.c     |  9 +++++++--
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/md/raid5-ppl.c b/drivers/md/raid5-ppl.c
index 0530632f9d2a..2ce4e0e72282 100644
--- a/drivers/md/raid5-ppl.c
+++ b/drivers/md/raid5-ppl.c
@@ -1461,6 +1461,8 @@ int ppl_modify_log(struct r5conf *conf, struct md_rdev *rdev, bool add)
 {
 	struct ppl_conf *ppl_conf = conf->log_private;
 	struct ppl_log *log;
+	struct md_rdev *old_rdev;
+	unsigned int old_ppl_size;
 	int ret = 0;
 
 	if (!rdev)
@@ -1480,11 +1482,18 @@ int ppl_modify_log(struct r5conf *conf, struct md_rdev *rdev, bool add)
 
 	mutex_lock(&log->io_mutex);
 	if (add) {
+		old_rdev = log->rdev;
+		old_ppl_size = rdev->ppl.size;
 		ret = ppl_validate_rdev(rdev);
 		if (!ret) {
 			log->rdev = rdev;
 			ret = ppl_write_empty_header(log);
-			ppl_init_child_log(log, rdev);
+			if (ret) {
+				log->rdev = old_rdev;
+				rdev->ppl.size = old_ppl_size;
+			} else {
+				ppl_init_child_log(log, rdev);
+			}
 		}
 	} else {
 		log->rdev = NULL;
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index efb4b0d37cb0..455e36ddceee 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -8550,12 +8550,17 @@ static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
 		if (p->rdev == NULL) {
 			clear_bit(In_sync, &rdev->flags);
 			rdev->raid_disk = disk;
+
+			err = log_modify(conf, rdev, true);
+			if (err) {
+				rdev->raid_disk = -1;
+				goto out;
+			}
+
 			if (rdev->saved_raid_disk != disk)
 				conf->fullsync = 1;
 			WRITE_ONCE(p->rdev, rdev);
 
-			err = log_modify(conf, rdev, true);
-
 			goto out;
 		}
 	}
-- 
2.43.0

[PATCH v2 2/2] md/raid5: preserve active PPL log when removing a replacement

[Yingjie Wang]

raid5_remove_disk() selects either the active member or its
replacement, but then clears the slot PPL log regardless of which
device is being removed. Removing an unpromoted replacement therefore
sets log->rdev to NULL even though the active member remains in the
slot.

ppl_write_stripe() subsequently returns -EAGAIN for that slot, which
lets ops_run_io() dispatch writes without first recording them in PPL.

Only change the PPL log when the active rdev is removed from a slot
that still belongs to the current array width. A slot left beyond
raid_disks after a shrink has no child PPL log, so skip log_modify()
there and still clear the device pointer.

Fixes: 6358c239d88c ("raid5-ppl: support disk hot add/remove with PPL")
Cc: stable@vger.kernel.org
Signed-off-by: Yingjie Wang <x-professor@outlook.com>
---
 drivers/md/raid5.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 455e36ddceee..418803741c67 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -8474,12 +8474,12 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
 		err = -EBUSY;
 		goto abort;
 	}
-	WRITE_ONCE(*rdevp, NULL);
-	if (!err) {
+	if (rdev == p->rdev && number < conf->raid_disks) {
 		err = log_modify(conf, rdev, false);
 		if (err)
 			goto abort;
 	}
+	WRITE_ONCE(*rdevp, NULL);
 
 	tmp = p->replacement;
 	if (tmp) {
-- 
2.43.0