<div dir="ltr"><div>NACK, see details below.</div><div><br></div><div>On Wed, Dec 8, 2021 at 3:24 PM Guoqing Jiang <<a href="mailto:guoqing.jiang@linux.dev" target="_blank">guoqing.jiang@linux.dev</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On 12/1/21 1:27 AM, Paul Menzel wrote:<br>
><br>
>>>>>>> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c<br>
>>>>>>> index cab12b2..0c4cbba 100644<br>
>>>>>>> --- a/drivers/md/dm-raid.c<br>
>>>>>>> +++ b/drivers/md/dm-raid.c<br>
>>>>>>> @@ -3668,7 +3668,7 @@ static int raid_message(struct dm_target <br>
>>>>>>> *ti, unsigned int argc, char **argv,<br>
>>>>>>> if (!strcasecmp(argv[0], "idle") || !strcasecmp(argv[0], <br>
>>>>>>> "frozen")) {<br>
>>>>>>> if (mddev->sync_thread) {<br>
>>>>>>> set_bit(MD_RECOVERY_INTR, <br>
>>>>>>> &mddev->recovery);<br>
>>>>>>> - md_reap_sync_thread(mddev);<br>
>>>>>>> + md_reap_sync_thread(mddev, false);<br>
>>>>><br>
>>>>> I think we can add mddev_lock() and mddev_unlock() here and then <br>
>>>>> we don't<br>
>>>>> need the extra parameter?<br>
>>>><br>
>>>> I thought it too, but I would prefer get the input from DM people <br>
>>>> first.<br>
>>>><br>
>>>> @ Mike or Alasdair<br>
>>><br>
>>> Hi Mike and Alasdair,<br>
>>><br>
>>> Could you please comment on this option: adding mddev_lock() and <br>
>>> mddev_unlock()<br>
>>> to raid_message() around md_reap_sync_thread()?<br>
<br>
Add Heinz and Jonathan, could you comment about this? Thanks.<br>
<br>
>><br>
>> The issue is unfortunately still unresolved (at least Linux 5.10.82). <br>
>> How can we move forward?<br>
<br>
If it is not applicable to change dm-raid, another alternative could be <br>
like this<br>
<br>
--- a/drivers/md/md.c<br>
+++ b/drivers/md/md.c<br>
@@ -9409,8 +9409,12 @@ void md_reap_sync_thread(struct mdev *mddev)<br>
sector_t old_dev_sectors = mddev->dev_sectors;<br>
bool is_reshaped = false;<br>
<br>
+ if (mddev_is_locked(mddev))<br>
+ mddev_unlock(mddev);<br>
/* resync has finished, collect result */<br>
md_unregister_thread(&mddev->sync_thread);<br>
+ if (mddev_is_locked(mddev))<br>
+ mddev_lock(mddev);<br>
if (!test_bit(MD_RECOVERY_INTR, &mddev->recovery) &&<br>
!test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery) &&<br>
mddev->degraded != mddev->raid_disks) {<br>
diff --git a/drivers/md/md.h b/drivers/md/md.h<br>
index 53ea7a6961de..96a88b7681d6 100644<br>
--- a/drivers/md/md.h<br>
+++ b/drivers/md/md.h<br>
@@ -549,6 +549,11 @@ static inline int mddev_trylock(struct mddev *mddev)<br>
}<br>
extern void mddev_unlock(struct mddev *mddev);<br>
<br>
+static inline int mddev_is_locked(struct mddev *mddev)<br>
+{<br>
+ return mutex_is_locked(&mddev->reconfig_mutex);<br>
+}<br>
+<br>
<br></blockquote><div><br></div><div>Patch is bogus relative to the proposed mddev_unlock/mddev_lock logic in md.c around the<br>md_unregister_thread() as it's failing to lock again if it was holding the mutex before as it again<br>calls mddev_locked() after having the mutex unlocked just before the md_unregister_thread() call.</div><div><br></div><div>If that patch to md.c holds up in further analysis, it has to keep the mddev_is_locked() result<br>and unlock/lock conditionally based on its result.</div><div><br></div><div>Thanks,<br></div><div>Heinz </div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
BTW, it is holiday season, so people are probably on vacation.<br>
<br>
Thanks,<br>
Guoqing<br>
<br>
</blockquote></div></div>