by Yu Kuai

[permalink] [raw]

Subject: Re: [PATCH md-6.10 3/9] md: add new helpers for sync_action

Hi,

在 2024/05/20 19:51, Su Yue 写道:
>
> On Thu 09 May 2024 at 09:18, Yu Kuai <[email protected]> wrote:
>
>> From: Yu Kuai <[email protected]>
>>
>> The new helpers will get current sync_action of the array, will be used
>> in later patches to make code cleaner.
>>
>> Signed-off-by: Yu Kuai <[email protected]>
>> ---
>> drivers/md/md.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
>> drivers/md/md.h | 3 +++
>> 2 files changed, 67 insertions(+)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 00bbafcd27bb..48ec35342d1b 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -69,6 +69,16 @@
>> #include "md-bitmap.h"
>> #include "md-cluster.h"
>>
>> +static char *action_name[NR_SYNC_ACTIONS] = {
>>
>
> Th array will not be modified, so:
>
> static const char * const action_names[NR_SYNC_ACTIONS]

Yes, this make sense.
>
>> +    [ACTION_RESYNC]        = "resync",
>> +    [ACTION_RECOVER]    = "recover",
>> +    [ACTION_CHECK]        = "check",
>> +    [ACTION_REPAIR]        = "repair",
>> +    [ACTION_RESHAPE]    = "reshape",
>> +    [ACTION_FROZEN]        = "frozen",
>> +    [ACTION_IDLE]        = "idle",
>> +};
>> +
>> /* pers_list is a list of registered personalities protected by
>> pers_lock. */
>> static LIST_HEAD(pers_list);
>> static DEFINE_SPINLOCK(pers_lock);
>> @@ -4867,6 +4877,60 @@ metadata_store(struct mddev *mddev, const char
>> *buf, size_t len)
>> static struct md_sysfs_entry md_metadata =
>> __ATTR_PREALLOC(metadata_version, S_IRUGO|S_IWUSR, metadata_show,
>> metadata_store);
>>
>> +enum sync_action md_sync_action(struct mddev *mddev)
>> +{
>> +    unsigned long recovery = mddev->recovery;
>> +
>> +    /*
>> +     * frozen has the highest priority, means running sync_thread
>> will be
>> +     * stopped immediately, and no new sync_thread can start.
>> +     */
>> +    if (test_bit(MD_RECOVERY_FROZEN, &recovery))
>> +        return ACTION_FROZEN;
>> +
>> +    /*
>> +     * idle means no sync_thread is running, and no new sync_thread is
>> +     * requested.
>> +     */
>> +    if (!test_bit(MD_RECOVERY_RUNNING, &recovery) &&
>> +        (!md_is_rdwr(mddev) || !test_bit(MD_RECOVERY_NEEDED,
>> &recovery)))
>> +        return ACTION_IDLE;
> My brain was lost sometimes looking into nested conditions of md code...
> I agree with Xiao Ni's suggestion that more comments about the array
> state should be added.

Okay, perhaps you're refering md_is_rdwr()? which is supposed to be
related to "no new sync_thread is requestd", perhaps following is
better:

/* only read-write array can start sync_thread */
if (!(md_is_rdwr(mddev))
return ACTION_IDLE;

/* sync_thread is not running, and no new sync_thread is requested */
if (!test_bit(MD_RECOVERY_RUNNING, &recovery) &&
!test_bit(MD_RECOVERY_NEEDED, &recovery)
return ACTION_IDLE;

>
>> +    if (test_bit(MD_RECOVERY_RESHAPE, &recovery) ||
>> +        mddev->reshape_position != MaxSector)
>> +        return ACTION_RESHAPE;
>> +
>> +    if (test_bit(MD_RECOVERY_RECOVER, &recovery))
>> +        return ACTION_RECOVER;
>> +
>>
> In action_show, MD_RECOVERY_SYNC is tested first then MD_RECOVERY_RECOVER.
> After looking through the logic of MD_RECOVERY_RECOVER clear/set_bit, the
> change is fine to me. However, better to follow old pattern unless there
> have resons.

It's just because MD_RECOVERY_SYNC is more complicated, and I move it to
the last, just programming habits. :)
>
>
>> +    if (test_bit(MD_RECOVERY_SYNC, &recovery)) {
>> +        if (test_bit(MD_RECOVERY_CHECK, &recovery))
>> +            return ACTION_CHECK;
>> +        if (test_bit(MD_RECOVERY_REQUESTED, &recovery))
>> +            return ACTION_REPAIR;
>> +        return ACTION_RESYNC;
>> +    }
>> +
>> +    return ACTION_IDLE;
>> +}
>> +
>> +enum sync_action md_sync_action_by_name(char *page)
>> +{
>> +    enum sync_action action;
>> +
>> +    for (action = 0; action < NR_SYNC_ACTIONS; ++action) {
>> +        if (cmd_match(page, action_name[action]))
>> +            return action;
>> +    }
>> +
>> +    return NR_SYNC_ACTIONS;
>> +}
>> +
>> +char *md_sync_action_name(enum sync_action action)
>>
>
> And 'const char *'

Yes

Thanks,
Kuai

>
> --
> Su
>
>> +{
>> +    return action_name[action];
>> +}
>> +
>> static ssize_t
>> action_show(struct mddev *mddev, char *page)
>> {
>> diff --git a/drivers/md/md.h b/drivers/md/md.h
>> index 2edad966f90a..72ca7a796df5 100644
>> --- a/drivers/md/md.h
>> +++ b/drivers/md/md.h
>> @@ -864,6 +864,9 @@ extern void md_unregister_thread(struct mddev
>> *mddev, struct md_thread __rcu **t
>> extern void md_wakeup_thread(struct md_thread __rcu *thread);
>> extern void md_check_recovery(struct mddev *mddev);
>> extern void md_reap_sync_thread(struct mddev *mddev);
>> +extern enum sync_action md_sync_action(struct mddev *mddev);
>> +extern enum sync_action md_sync_action_by_name(char *page);
>> +extern char *md_sync_action_name(enum sync_action action);
>> extern bool md_write_start(struct mddev *mddev, struct bio *bi);
>> extern void md_write_inc(struct mddev *mddev, struct bio *bi);
>> extern void md_write_end(struct mddev *mddev);
> .
>

2024-05-21 03:03:55

Hi,

在 2024/05/21 11:21, Xiao Ni 写道:
> Hi Kuai
>
> I've tested 07reshape5intr with the latest upstream kernel 15 times
> without failure. So it's better to have a try with 07reshape5intr with
> your patch set.

I just discussed with Xiao on slack, for conclusion here:

The test 07reshape5intr will add a new disk to array, then start
reshape:

mdadm /dev/md0 --add /dev/xxx
mdadm --grow /dev/md0 -n 3

However, the grow will fail:
mdadm: Failed to initiate reshape!

Root cause is that in kernel, action_store() will return -EBUSY
if MD_RECOVERY_RUNNING is set:

// mdadm add
add_bound_rdev
set_bit(MD_RECOVERY_NEEDED, &mddev->recovery);

// daemon thread
md_check_recovery
set_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
// do nothing
// mdadm grow
action_store
if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery))
return -EBUSY
clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery)

This is a long term problem, and we need new synchronization in kernel
to make sure the grow won't fail.

Thanks,
Kuai

>
> Regards
> Xiao
>
>
>
>
> On Tue, May 21, 2024 at 11:02 AM Oliver Sang <[email protected]> wrote:
>>
>> hi, Yu Kuai,
>>
>> On Tue, May 21, 2024 at 10:20:54AM +0800, Yu Kuai wrote:
>>> Hi,
>>>
>>> 在 2024/05/20 23:01, kernel test robot 写道:
>>>>
>>>>
>>>> Hello,
>>>>
>>>> kernel test robot noticed "mdadm-selftests.07reshape5intr.fail" on:
>>>>
>>>> commit: 18effaab5f57ef44763e537c782f905e06f6c4f5 ("[PATCH md-6.10 5/9] md: replace sysfs api sync_action with new helpers")
>>>> url: https://github.com/intel-lab-lkp/linux/commits/Yu-Kuai/md-rearrange-recovery_flage/20240509-093248
>>>> base: https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git for-next
>>>> patch link: https://lore.kernel.org/all/[email protected]/
>>>> patch subject: [PATCH md-6.10 5/9] md: replace sysfs api sync_action with new helpers
>>>>
>>>> in testcase: mdadm-selftests
>>>> version: mdadm-selftests-x86_64-5f41845-1_20240412
>>>> with following parameters:
>>>>
>>>> disk: 1HDD
>>>> test_prefix: 07reshape5intr
>>>>
>>>>
>>>>
>>>> compiler: gcc-13
>>>> test machine: 8 threads 1 sockets Intel(R) Core(TM) i7-4790T CPU @ 2.70GHz (Haswell) with 16G memory
>>>>
>>>> (please refer to attached dmesg/kmsg for entire log/backtrace)
>>>>
>>>>
>>>>
>>>>
>>>> If you fix the issue in a separate patch/commit (i.e. not just a new version of
>>>> the same patch/commit), kindly add following tags
>>>> | Reported-by: kernel test robot <[email protected]>
>>>> | Closes: https://lore.kernel.org/oe-lkp/[email protected]
>>>>
>>>> 2024-05-14 21:36:26 mkdir -p /var/tmp
>>>> 2024-05-14 21:36:26 mke2fs -t ext3 -b 4096 -J size=4 -q /dev/sda1
>>>> 2024-05-14 21:36:57 mount -t ext3 /dev/sda1 /var/tmp
>>>> sed -e 's/{DEFAULT_METADATA}/1.2/g' \
>>>> -e 's,{MAP_PATH},/run/mdadm/map,g' mdadm.8.in > mdadm.8
>>>> /usr/bin/install -D -m 644 mdadm.8 /usr/share/man/man8/mdadm.8
>>>> /usr/bin/install -D -m 644 mdmon.8 /usr/share/man/man8/mdmon.8
>>>> /usr/bin/install -D -m 644 md.4 /usr/share/man/man4/md.4
>>>> /usr/bin/install -D -m 644 mdadm.conf.5 /usr/share/man/man5/mdadm.conf.5
>>>> /usr/bin/install -D -m 644 udev-md-raid-creating.rules /lib/udev/rules.d/01-md-raid-creating.rules
>>>> /usr/bin/install -D -m 644 udev-md-raid-arrays.rules /lib/udev/rules.d/63-md-raid-arrays.rules
>>>> /usr/bin/install -D -m 644 udev-md-raid-assembly.rules /lib/udev/rules.d/64-md-raid-assembly.rules
>>>> /usr/bin/install -D -m 644 udev-md-clustered-confirm-device.rules /lib/udev/rules.d/69-md-clustered-confirm-device.rules
>>>> /usr/bin/install -D -m 755 mdadm /sbin/mdadm
>>>> /usr/bin/install -D -m 755 mdmon /sbin/mdmon
>>>> Testing on linux-6.9.0-rc2-00012-g18effaab5f57 kernel
>>>> /lkp/benchmarks/mdadm-selftests/tests/07reshape5intr... FAILED - see /var/tmp/07reshape5intr.log and /var/tmp/fail07reshape5intr.log for detail
>>> [root@fedora mdadm]# ./test --dev=loop --tests=07reshape5intr
>>> test: skipping tests for multipath, which is removed in upstream 6.8+
>>> kernels
>>> test: skipping tests for linear, which is removed in upstream 6.8+ kernels
>>> Testing on linux-6.9.0-rc2-00023-gf092583596a2 kernel
>>> /root/mdadm/tests/07reshape5intr... FAILED - see /var/tmp/07reshape5intr.log
>>> and /var/tmp/fail07reshape5intr.log for details
>>> (KNOWN BROKEN TEST: always fails)
>>>
>>> So, since this test is marked BROKEN.
>>>
>>> Please share the whole log, and is it possible to share the two logs?
>>
>>
>> we only captured one log as attached log-18effaab5f.
>> also attached parent log FYI.
>>
>>
>>>
>>> Thanks,
>>> Kuai
>>>
>>>>
>>>>
>>>>
>>>> The kernel config and materials to reproduce are available at:
>>>> https://download.01.org/0day-ci/archive/20240520/[email protected]
>>>>
>>>>
>>>>
>>>
>
>
> .
>