| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <b2754d8e-dfe7-ffff-66ac-052f366530e4@huaweicloud.com>
Date: Tue, 12 Sep 2023 09:02:19 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Mariusz Tkaczyk <mariusz.tkaczyk@...ux.intel.com>,
Yu Kuai <yukuai1@...weicloud.com>
Cc: song@...nel.org, linux-raid@...r.kernel.org,
linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
yangerkun@...wei.com, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH -next] md: simplify md_seq_ops
Hi,
在 2023/09/11 22:05, Mariusz Tkaczyk 写道:
> On Mon, 11 Sep 2023 14:50:10 +0800
> Yu Kuai <yukuai1@...weicloud.com> wrote:
>
>> From: Yu Kuai <yukuai3@...wei.com>
>>
>> Use seq_list_start/next/stop() directly. Move printing "Personalities"
>> to md_sep_start() and "unsed devices" to md_seq_stop().
>>
>> Cc: Mariusz Tkaczyk <mariusz.tkaczyk@...ux.intel.com>
>> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
>> ---
>> drivers/md/md.c | 124 ++++++++++++------------------------------------
>> 1 file changed, 31 insertions(+), 93 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 0fe7ab6e8ab9..9c1155042335 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -8192,105 +8192,14 @@ static int status_resync(struct seq_file *seq,
>> struct mddev *mddev) return 1;
>> }
>>
>> -static void *md_seq_start(struct seq_file *seq, loff_t *pos)
>> -{
>> - struct list_head *tmp;
>> - loff_t l = *pos;
>> - struct mddev *mddev;
>> -
>> - if (l == 0x10000) {
>> - ++*pos;
>> - return (void *)2;
>> - }
>> - if (l > 0x10000)
>> - return NULL;
>> - if (!l--)
>> - /* header */
>> - return (void*)1;
>> -
>> - spin_lock(&all_mddevs_lock);
>> - list_for_each(tmp,&all_mddevs)
>> - if (!l--) {
>> - mddev = list_entry(tmp, struct mddev, all_mddevs);
>> - if (!mddev_get(mddev))
>> - continue;
>> - spin_unlock(&all_mddevs_lock);
>> - return mddev;
>> - }
>> - spin_unlock(&all_mddevs_lock);
>> - if (!l--)
>> - return (void*)2;/* tail */
>> - return NULL;
>> -}
>> -
>> -static void *md_seq_next(struct seq_file *seq, void *v, loff_t *pos)
>> -{
>> - struct list_head *tmp;
>> - struct mddev *next_mddev, *mddev = v;
>> - struct mddev *to_put = NULL;
>> -
>> - ++*pos;
>> - if (v == (void*)2)
>> - return NULL;
>> -
>> - spin_lock(&all_mddevs_lock);
>> - if (v == (void*)1) {
>> - tmp = all_mddevs.next;
>> - } else {
>> - to_put = mddev;
>> - tmp = mddev->all_mddevs.next;
>> - }
>> -
>> - for (;;) {
>> - if (tmp == &all_mddevs) {
>> - next_mddev = (void*)2;
>> - *pos = 0x10000;
>> - break;
>> - }
>> - next_mddev = list_entry(tmp, struct mddev, all_mddevs);
>> - if (mddev_get(next_mddev))
>> - break;
>> - mddev = next_mddev;
>> - tmp = mddev->all_mddevs.next;
>> - }
>> - spin_unlock(&all_mddevs_lock);
>> -
>> - if (to_put)
>> - mddev_put(mddev);
>> - return next_mddev;
>> -
>> -}
>> -
>> -static void md_seq_stop(struct seq_file *seq, void *v)
>> -{
>> - struct mddev *mddev = v;
>> -
>> - if (mddev && v != (void*)1 && v != (void*)2)
>> - mddev_put(mddev);
>> -}
>> -
>> static int md_seq_show(struct seq_file *seq, void *v)
>> {
>> - struct mddev *mddev = v;
>> + struct mddev *mddev = list_entry(v, struct mddev, all_mddevs);
>> sector_t sectors;
>> struct md_rdev *rdev;
>>
>> - if (v == (void*)1) {
>> - struct md_personality *pers;
>> - seq_printf(seq, "Personalities : ");
>> - spin_lock(&pers_lock);
>> - list_for_each_entry(pers, &pers_list, list)
>> - seq_printf(seq, "[%s] ", pers->name);
>> -
>> - spin_unlock(&pers_lock);
>> - seq_printf(seq, "\n");
>> - seq->poll_event = atomic_read(&md_event_count);
>> - return 0;
>> - }
>> - if (v == (void*)2) {
>> - status_unused(seq);
>> + if (test_bit(MD_DELETED, &mddev->flags))
>> return 0;
>> - }
>>
>> spin_lock(&mddev->lock);
>> if (mddev->pers || mddev->raid_disks || !list_empty(&mddev->disks)) {
>> @@ -8366,6 +8275,35 @@ static int md_seq_show(struct seq_file *seq, void *v)
>> return 0;
>> }
>>
>> +static void *md_seq_start(struct seq_file *seq, loff_t *pos)
>> +{
>> + struct md_personality *pers;
>> +
>> + seq_puts(seq, "Personalities : ");
>> + spin_lock(&pers_lock);
>> + list_for_each_entry(pers, &pers_list, list)
>> + seq_printf(seq, "[%s] ", pers->name);
>> +
>> + spin_unlock(&pers_lock);
>> + seq_puts(seq, "\n");
>> + seq->poll_event = atomic_read(&md_event_count);
>> +
>> + spin_lock(&all_mddevs_lock);
>
> I would prefer to increase "active" instead holding lock when enumerating over
> the devices. the main reason is that parsing mdstat is implemented in mdadm, so
> it could kind of blocker action- for example mdmon follows mdstat so it is read
> frequently. The time of getting other actions done can highly increase because
> every open or sysfs_read/write requires this lock.
>
>> +
>> + return seq_list_start(&all_mddevs, *pos);
>> +}
>> +
>> +static void *md_seq_next(struct seq_file *seq, void *v, loff_t *pos)
>> +{
>> + return seq_list_next(v, &all_mddevs, pos);
>> +}
> Can it be so simple? Why previous versions takes care of holding "(void)*1" and
> "(void)*2" then? Could you elaborate?
"1" means printing "Personalities", which is now moved to md_seq_start,
and "2" means printing "unsed devices" which is now moved to
md_seq_stop. And now md_seq_next is only used to iterate the mddev list.
Thanks,
Kuai
>
>> +
>> +static void md_seq_stop(struct seq_file *seq, void *v)
>> +{
>> + status_unused(seq);
>> + spin_unlock(&all_mddevs_lock);
>> +}
>> +
>> static const struct seq_operations md_seq_ops = {
>> .start = md_seq_start,
>> .next = md_seq_next,
>
> Thanks,
> Mariusz
>
> .
>
Powered by blists - more mailing lists