Re: [PATCH -next] md: simplify md_seq_ops

[Yu Kuai]

Hi,

在 2023/09/23 5:22, Song Liu 写道:
> On Mon, Sep 11, 2023 at 6:02 PM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
> >
> [...]
> > > > +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.
>
> Existing code holds pers_lock can seq_printf() in md_seq_show(). Do we see
> issues with this?

before this patch, in each loop:
- hold lock, get mddev, drop lock
- md_seq_show

and after this patch:
- hold lock in start, drop lock in stop
- lock is always held in each loop

And mariusz is concerned that lock time is increased and may cause some
performance regression.

We've discussed in slack, and decided to keep this behaviour. I'll
update this in v2.

Thanks,
Kuai

> Hi Kuai,
>
> This patch doesn't apply cleanly to md-next now. Please rebase and send v2.
>
> Thanks,
> Song
> .