On Fri, 19 Sep 2025, Heinz Mauelshagen wrote:
> Xiao, > > This aspect of the patch corrects an asymmetry in device-mapper > suspend/resume handling where MD_RDWR/MD_RDONLY > flag management is unbalanced (resume sets read-write, but post_suspend > does't properly set read-only). Not setting > MD_RDONLY would e.g. cause md_submit_bio() to not error any writes when it > should. > > Although no failures have been attributed to this inconsistency, the fix > ensures correct state transitions for completeness. > However, we could additionally catch writes to a read-only array in the > target's map function. OK, thanks. I staged the patch and added this explanation to the patch header. Mikulas > - lvmguy > > On Fri, Sep 19, 2025 at 3:20 AM Xiao Ni <[email protected]> wrote: > Hi Heinz > > On Thu, Sep 18, 2025 at 9:55 PM Heinz Mauelshagen <[email protected]> > wrote: > > > > The dm-raid code was using hardcoded integer values to represent the > read-only/read-write state > > of RAID arrays instead of the proper enumeration constants defined in > the md_ro_state enumerator type. > > > > Changes: > > > > - Replace hardcoded integers with the appropriate md_ro_state > enumerator values > > > > - Add the missing MD_RDONLY setting in the post_suspend function > > It should be the most important change in this patch. Mind adding the > reproducer in the commit message? And explain why you say missing it? > > Best Regards > Xiao > > > > This improves code clarity and maintainability by using the defined > enumeration constants > > rather than magic numbers, ensuring the code properly conforms to the > established API interface. > > > > Signed-off-by: Heinz Mauelshagen <[email protected]> > > --- > > drivers/md/dm-raid.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c > > index 79ea85d18e24..6cdfd7ca998c 100644 > > --- a/drivers/md/dm-raid.c > > +++ b/drivers/md/dm-raid.c > > @@ -3247,7 +3247,7 @@ static int raid_ctr(struct dm_target *ti, > unsigned int argc, char **argv) > > rs_reset_inconclusive_reshape(rs); > > > > /* Start raid set read-only and assumed clean to change in > raid_resume() */ > > - rs->md.ro = 1; > > + rs->md.ro = MD_RDONLY; > > rs->md.in_sync = 1; > > > > /* Has to be held on running the array */ > > @@ -3385,7 +3385,7 @@ static enum sync_state > decipher_sync_action(struct mddev *mddev, unsigned long r > > /* The MD sync thread can be done with io or be interrupted > but still be running */ > > if (!test_bit(MD_RECOVERY_DONE, &recovery) && > > (test_bit(MD_RECOVERY_RUNNING, &recovery) || > > - (!mddev->ro && test_bit(MD_RECOVERY_NEEDED, > &recovery)))) { > > + (md_is_rdwr(mddev) && test_bit(MD_RECOVERY_NEEDED, > &recovery)))) { > > if (test_bit(MD_RECOVERY_RESHAPE, &recovery)) > > return st_reshape; > > > > @@ -3775,11 +3775,11 @@ static int raid_message(struct dm_target *ti, > unsigned int argc, char **argv, > > } else > > return -EINVAL; > > } > > - if (mddev->ro == 2) { > > + if (mddev->ro == MD_AUTO_READ) { > > /* A write to sync_action is enough to justify > > * canceling read-auto mode > > */ > > - mddev->ro = 0; > > + mddev->ro = MD_RDWR; > > if (!mddev->suspended) > > md_wakeup_thread(mddev->sync_thread); > > } > > @@ -3858,6 +3858,7 @@ static void raid_postsuspend(struct dm_target > *ti) > > */ > > md_stop_writes(&rs->md); > > mddev_suspend(&rs->md, false); > > + rs->md.ro = MD_RDONLY; > > } > > } > > > > @@ -3968,7 +3969,7 @@ static void rs_update_sbs(struct raid_set *rs) > > int ro = mddev->ro; > > > > set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags); > > - mddev->ro = 0; > > + mddev->ro = MD_RDWR; > > md_update_sb(mddev, 1); > > mddev->ro = ro; > > } > > @@ -4125,7 +4126,7 @@ static void raid_resume(struct dm_target *ti) > > > WARN_ON_ONCE(rcu_dereference_protected(mddev->sync_thread, > > > lockdep_is_held(&mddev->reconfig_mutex))); > > clear_bit(RT_FLAG_RS_FROZEN, &rs->runtime_flags); > > - mddev->ro = 0; > > + mddev->ro = MD_RDWR; > > mddev->in_sync = 0; > > md_unfrozen_sync_thread(mddev); > > mddev_unlock_and_resume(mddev); > > -- > > 2.51.0 > > > > > > >
