Date: Tue, 9 Jan 2024 15:48:25 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Yu Kuai <yukuai1@...weicloud.com>, Song Liu <song@...nel.org>
Cc: mariusz.tkaczyk@...ux.intel.com, xni@...hat.com,
 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 v3 2/2] md: simplify md_seq_ops

Hi,

在 2024/01/09 9:21, Yu Kuai 写道:
> Hi,
> 
> 在 2024/01/09 7:38, Song Liu 写道:
>> On Tue, Sep 26, 2023 at 11:19 PM Yu Kuai <yukuai1@...weicloud.com> wrote:
>>>
>>> From: Yu Kuai <yukuai3@...wei.com>
>>>
>>> Before this patch, the implementation is hacky and hard to understand:
>>>
>>> 1) md_seq_start set pos to 1;
>>> 2) md_seq_show found pos is 1, then print Personalities;
>>> 3) md_seq_next found pos is 1, then it update pos to the first mddev;
>>> 4) md_seq_show found pos is not 1 or 2, show mddev;
>>> 5) md_seq_next found pos is not 1 or 2, update pos to next mddev;
>>> 6) loop 4-5 until the last mddev, then md_seq_next update pos to 2;
>>> 7) md_seq_show found pos is 2, then print unused devices;
>>> 8) md_seq_next found pos is 2, stop;
>>>
>>> This patch remove the magic value and use seq_list_start/next/stop()
>>> directly, and move printing "Personalities" to md_seq_start(),
>>> "unsed devices" to md_seq_stop():
>>>
>>> 1) md_seq_start print Personalities, and then set pos to first mddev;
>>> 2) md_seq_show show mddev;
>>> 3) md_seq_next update pos to next mddev;
>>> 4) loop 2-3 until the last mddev;
>>> 5) md_seq_stop print unsed devices;
>>>
>>> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
>>
>> Just realized this introduced a behavior change:
>>
>> When there is not md devices, before this patch, we have
>>
>> [root@...50-1 ~]# cat /proc/mdstat
>> Personalities : [raid0] [raid1] [raid10] [raid6] [raid5] [raid4]
>> unused devices: <none>
>>
>> After this patch, "cat /proc/mdstat" returns nothing. This causes
>> some confusion for users who want to read "Personalities" line,
>> for example, the mdadm test suite reads it.
>>
>> I haven't figured out the best fix yet.
> 
> Yes, that's a problem. And after reviewing seq_read_iter() in detail, I
> realize that I also can't use seq_printf() in m->op->start() directly,
> because if seq buffer overflowed, md_seq_start() can be called more than
> once.
> 
> I'll fix these problems soon.
How about following patch(already tested)?

Thanks,
Kuai

diff --git a/drivers/md/md.c b/drivers/md/md.c
index e351e6c51cc7..289d3d89e73d 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8135,6 +8135,19 @@ static void status_unused(struct seq_file *seq)
         seq_printf(seq, "\n");
  }

+static void status_personalities(struct seq_file *seq)
+{
+       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");
+}
+
  static int status_resync(struct seq_file *seq, struct mddev *mddev)
  {
         sector_t max_sectors, resync, res;
@@ -8273,43 +8286,53 @@ static int status_resync(struct seq_file *seq, 
struct mddev *mddev)
         return 1;
  }

+#define MDDEV_NONE (void *)1
+
  static void *md_seq_start(struct seq_file *seq, loff_t *pos)
         __acquires(&all_mddevs_lock)
  {
-       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);

-       return seq_list_start(&all_mddevs, *pos);
+       if (!list_empty(&all_mddevs))
+               return seq_list_start(&all_mddevs, *pos);
+       else if (*pos == 0)
+               return MDDEV_NONE;
+       else
+               return NULL;
  }

  static void *md_seq_next(struct seq_file *seq, void *v, loff_t *pos)
  {
+       if (v == MDDEV_NONE) {
+               ++*pos;
+               return NULL;
+       }
+
         return seq_list_next(v, &all_mddevs, pos);
  }

  static void md_seq_stop(struct seq_file *seq, void *v)
         __releases(&all_mddevs_lock)
  {
-       status_unused(seq);
         spin_unlock(&all_mddevs_lock);
  }
  static int md_seq_show(struct seq_file *seq, void *v)
  {
-       struct mddev *mddev = list_entry(v, struct mddev, all_mddevs);
+       struct mddev *mddev;
         sector_t sectors;
         struct md_rdev *rdev;

+       if (v == MDDEV_NONE) {
+               status_personalities(seq);
+               status_unused(seq);
+               return 0;
+       }
+
+       mddev = list_entry(v, struct mddev, all_mddevs);
+       if (mddev == list_first_entry(&all_mddevs, struct mddev, 
all_mddevs))
+               status_personalities(seq);
         if (!mddev_get(mddev))
                 return 0;

@@ -8385,6 +8408,10 @@ static int md_seq_show(struct seq_file *seq, void *v)
         }
         spin_unlock(&mddev->lock);
         spin_lock(&all_mddevs_lock);
+
+       if (mddev == list_last_entry(&all_mddevs, struct mddev, all_mddevs))
+               status_unused(seq);
+
         if (atomic_dec_and_test(&mddev->active))
                 __mddev_put(mddev);




> 
> Thanks,
> Kuai
> 
>>
>> Thanks,
>> Song
>>
>> .
>>
> 
> .
> 

Powered by blists - more mailing lists