On Sat, Jul 29, 2023 at 03:37:41PM +0800, Yu Kuai wrote: > Hi, > > 在 2023/07/29 14:16, Xueshi Hu 写道: > > On Sat, Jul 29, 2023 at 11:51:27AM +0800, Yu Kuai wrote: > > > Hi, > > > > > > 在 2023/07/29 11:36, Yu Kuai 写道: > > > > Hi, > > > > > > > > 在 2023/07/29 11:29, Xueshi Hu 写道: > > > > > > > > I think this is wrong, you should at least keep following: > > > > > > > > > > > > > > > > set_bit(MD_RECOVERY_RECOVER, &mddev->recovery); > > > > > > > > set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); > > > > > > > > md_wakeup_thread(mddev->thread); > > > > > > > > > > > > > > > I fail to comprehend the rationale behind the kernel's need to invoke > > > > > > > raid1d() repeatedly despite the absence of any modifications. > > > > > > > It appears that raid1d() is responsible for invoking > > > > > > > md_check_recovery() > > > > > > > and resubmit the failed io. > > > > > > > > > > > > No, the point here is to set MD_RECOVERY_NEEDED and MD_RECOVERY_RECOVER, > > > > > > so that md_check_recovery() can start to reshape. > > > > > I apologize, but I am still unable to comprehend your idea. > > > > > If mmdev->delta_disks == 0 , what is the purpose of invoking > > > > > md_check_recovery() again? > > > > > > > > Sounds like you think raid1_reshape() can only be called from > > > > md_check_recovery(), please take a look at other callers. > > Thank you for the quick reply and patience. > > > > Of course, I have checked all the caller of md_personality::check_reshape. > > > > - layout_store > > - action_store > > - chunk_size_store > > - md_ioctl > > __md_set_array_info > > update_array_info > > - md_check_recovery > > - md_ioctl > > __md_set_array_info > > update_array_info > > update_raid_disks > > - process_metadata_update > > md_reload_sb > > check_sb_changes > > update_raid_disks > > - raid_disks_store > > update_raid_disks > > > > There are three categories of callers except md_check_recovery(). > > 1. write to sysfs > > 2. ioctl > > 3. revice instructions from md cluster peer > > > > Using "echo 4 > /sys/devices/virtual/block/md10/md/raid_disks" as an > > example, if the mddev::raid_disks is already 4, I don't think > > raid1_reshape() should set MD_RECOVERY_RECOVER and MD_RECOVERY_NEEDED > > bit, then wake up the mddev::thread to call md_check_recovery(). > > > > Is there any counterexample to demonstrate the issues that may arise if > > md_check_recovery() is not called out of raid1_reshape() ? > > > > > > > > And even if raid1_reshape() is called from md_check_recovery(), > > > which means reshape is interupted, then MD_RECOVERY_RECOVER > > > and MD_RECOVERY_RECOVER need to be set, so that reshape can > > > continue. > > > > raid1 only register md_personality::check_reshape, all the work related > > with reshape are handle in md_personality::check_reshape. > > What's the meaning of "reshape can continue" ? In another word, what's > > the additional work need to do if mddev::raid_disks doesn't change ? > > Okay, I missed that raid1 doesn't have 'start_reshape', reshape here > really means recovery, synchronize data to new disks. Never mind what > I said "reshape can continue", it's not possible for raid1. > > Then the problem is the same from 'recovery' point of view: > if the 'recovery' is interrupted, before this patch, even if raid_disks > is the same, raid1_reshape() will still set the flag and then > md_check_recovery() will try to continue the recovery. I'd like to > keep this behaviour untill it can be sure that no user will be > affected. But md_check_recovery() will never call raid1_reshape(). md_personality::check_reshape() is called in md_check_recovery() when the reshape is in process. But, raid1 is speicial as mddev::reshape_position always equals to MaxSector in raid1. By the way, the concern in V2 patch[1] is unnecessary out of the same reason. [1]: https://lore.kernel.org/linux-raid/ff93bc7a-5ae2-7a85-91c9-9662d3c5a442@xxxxxxxxxxxxxxx/#t > > Thanks, > Kuai > > > > > Thanks, > > Hu > > > > > > > > Thanks, > > > Kuai > > > > > > > > Thanks, > > > > Kuai > > > > > > > > . > > > > > > > > > . > > >
