Re: [Update PATCH V3] md: don't unregister sync_thread with reconfig_mutex held

Hi all

I've tried the test and got the same result.  8b48ec23cc51(md: don't
unregister sync_thread with reconfig_mutex held)
introduces this problem.

My environment:
kernel: latest md-next branch
mdadm: Logan's tree bugfixes2 branch
(https://github.com/lsgunth/mdadm/) because Logan fixed some
errors for the test cases. I'm also trying to fix other failures based
on the tree.

Case1:
mdadm -CR /dev/md0 -amd -l5 -c 1024 -n2 --assume-clean /dev/loop1 /dev/loop2
mkfs.xfs /dev/md0
mount /dev/md0 /mnt/
mdadm /dev/md0 -a /dev/loop0
mdadm /dev/md0 --grow -n3
cat /proc/mdstat
umount  /mnt/
mdadm --stop /dev/md0
mdadm -A /dev/md0 /dev/loop[0-2]
cat /proc/mdstat (reshape doesn't happen)
mount /dev/md0 /mnt/  (fails here)
mount: /mnt: wrong fs type, bad option, bad superblock on /dev/md0,
missing codepage or helper program, or other error.

Case2:
I commented some codes, built the md module and insmod the new module
        //if (reconfig_mutex_held)
        //      mddev_unlock(mddev);
        /* resync has finished, collect result */
        md_unregister_thread(&mddev->sync_thread);
        //if (reconfig_mutex_held)
        //      mddev_lock_nointr(mddev);

The test Case1 can pass

Need to look more to find the root cause. It looks like some
information in superblock doesn't sync to member devices.
So after assemble, it can't do reshape again. Need more time to look at this.

Best Regards
Xiao

On Sat, May 21, 2022 at 2:27 AM Logan Gunthorpe [off-list ref] wrote:

Hi,

On 2022-05-10 00:44, Song Liu wrote:

quoted

On Mon, May 9, 2022 at 1:09 AM Guoqing Jiang [off-list ref] wrote:

quoted

On 5/9/22 2:37 PM, Song Liu wrote:

quoted

On Fri, May 6, 2022 at 4:37 AM Guoqing Jiang[off-list ref]  wrote:

quoted

From: Guoqing Jiang<redacted>

Unregister sync_thread doesn't need to hold reconfig_mutex since it
doesn't reconfigure array.

And it could cause deadlock problem for raid5 as follows:

1. process A tried to reap sync thread with reconfig_mutex held after echo
    idle to sync_action.
2. raid5 sync thread was blocked if there were too many active stripes.
3. SB_CHANGE_PENDING was set (because of write IO comes from upper layer)
    which causes the number of active stripes can't be decreased.
4. SB_CHANGE_PENDING can't be cleared since md_check_recovery was not able
    to hold reconfig_mutex.

More details in the link:
https://lore.kernel.org/linux-raid/5ed54ffc-ce82-bf66-4eff-390cb23bc1ac@molgen.mpg.de/T/#t (local)

Let's call unregister thread between mddev_unlock and mddev_lock_nointr
(thanks for the report from kernel test robot[off-list ref]) if the
reconfig_mutex is held, and mddev_is_locked is introduced accordingly.

mddev_is_locked() feels really hacky to me. It cannot tell whether
mddev is locked
by current thread. So technically, we can unlock reconfigure_mutex for
other thread
by accident, no?

I can switch back to V2 if you think that is the correct way to do though no
one comment about the change in dm-raid.

I guess v2 is the best at the moment. I pushed a slightly modified v2 to
md-next.

I noticed a clear regression with mdadm tests with this patch in md-next
(7e6ba434cc6080).

Before the patch, tests 07reshape5intr and 07revert-grow would fail
fairly infrequently (about 1 in 4 runs for the former and 1 in 30 runs
for the latter).

After this patch, both tests always fail.

I don't have time to dig into why this is, but it would be nice if
someone can at least fix the regression. It is hard to make any progress
on these tests if we are continuing to further break them.

Logan