Received-SPF: pass (google.com: domain of linux-kernel+bounces-86032-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Subject: Re: [PATCH md-6.9 v3 02/11] md/raid1: factor out helpers to add rdev
 to conf
To: Yu Kuai <yukuai1@huaweicloud.com>, xni@redhat.com,
 paul.e.luse@linux.intel.com, song@kernel.org, shli@fb.com, neilb@suse.com
Cc: linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org,
 yi.zhang@huawei.com, yangerkun@huawei.com, "yukuai (C)" <yukuai3@huawei.com>
References: <20240228114333.527222-1-yukuai1@huaweicloud.com>
 <20240228114333.527222-3-yukuai1@huaweicloud.com>
From: Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <ca4adb05-2e99-2f83-3032-e7a2e03e94f3@huaweicloud.com>
Date: Thu, 29 Feb 2024 10:11:55 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240228114333.527222-3-yukuai1@huaweicloud.com>
Content-Type: text/plain; charset=gbk; format=flowed
Content-Transfer-Encoding: 8bit

Hi,

?? 2024/02/28 19:43, Yu Kuai д??:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> There are no functional changes, just make code cleaner and prepare to
> record disk non-rotational information while adding and removing rdev to
> conf
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>   drivers/md/raid1.c | 74 ++++++++++++++++++++++++++++------------------
>   1 file changed, 46 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index a145fe48b9ce..1940ff398c23 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1757,6 +1757,40 @@ static int raid1_spare_active(struct mddev *mddev)
>   	return count;
>   }
>   
> +static bool raid1_add_conf(struct r1conf *conf, struct md_rdev *rdev, int disk)
> +{
> +	struct raid1_info *info = conf->mirrors + disk;
> +
> +	if (info->rdev)
> +		return false;
> +
> +	rdev->raid_disk = disk;
> +	info->head_position = 0;
> +	info->seq_start = MaxSector;
> +	WRITE_ONCE(info->rdev, rdev);
> +
> +	return true;
> +}

I misread the code, for replacement, rdev->raid_disk is the same as the
original rdev, while this patch set it to "raid_disk + conf->raid_disks"
and following should be merged with this patch:

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 1940ff398c23..ff1e1aeaf336 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1757,10 +1757,14 @@ static int raid1_spare_active(struct mddev *mddev)
         return count;
  }

-static bool raid1_add_conf(struct r1conf *conf, struct md_rdev *rdev, 
int disk)
+static bool raid1_add_conf(struct r1conf *conf, struct md_rdev *rdev, 
int disk,
+                          bool replacement)
  {
         struct raid1_info *info = conf->mirrors + disk;

+       if (replacement)
+               info += conf->raid_disks;
+
         if (info->rdev)
                 return false;

@@ -1826,7 +1830,7 @@ static int raid1_add_disk(struct mddev *mddev, 
struct md_rdev *rdev)
                                 disk_stack_limits(mddev->gendisk, 
rdev->bdev,
                                                   rdev->data_offset << 9);

-                       raid1_add_conf(conf, rdev, mirror);
+                       raid1_add_conf(conf, rdev, mirror, false);
                         err = 0;
                         /* As all devices are equivalent, we don't need 
a full recovery
                          * if this was recently any drive of the array
@@ -1844,7 +1848,7 @@ static int raid1_add_disk(struct mddev *mddev, 
struct md_rdev *rdev)
                 /* Add this device as a replacement */
                 clear_bit(In_sync, &rdev->flags);
                 set_bit(Replacement, &rdev->flags);
-               raid1_add_conf(conf, rdev, repl_slot);
+               raid1_add_conf(conf, rdev, repl_slot, true);
                 err = 0;
                 conf->fullsync = 1;
         }
@@ -3017,18 +3021,16 @@ static struct r1conf *setup_conf(struct mddev 
*mddev)

         err = -EINVAL;
         spin_lock_init(&conf->device_lock);
+       conf->raid_disks = mddev->raid_disks;
         rdev_for_each(rdev, mddev) {
                 int disk_idx = rdev->raid_disk;
-               if (disk_idx >= mddev->raid_disks
+               if (disk_idx >= conf->raid_disks
                     || disk_idx < 0)
                         continue;
-               if (test_bit(Replacement, &rdev->flags))
-                       disk_idx += mddev->raid_disks;
-
-               if (!raid1_add_conf(conf, rdev, disk_idx))
+               if (!raid1_add_conf(conf, rdev, disk_idx,
+                                   test_bit(Replacement, &rdev->flags)))
                         goto abort;
         }
-       conf->raid_disks = mddev->raid_disks;
         conf->mddev = mddev;
         INIT_LIST_HEAD(&conf->retry_list);
         INIT_LIST_HEAD(&conf->bio_end_io_list);


I'll finish mdadm tests suite before v4.

Thanks,
Kuai
> +
> +static bool raid1_remove_conf(struct r1conf *conf, int disk)
> +{
> +	struct raid1_info *info = conf->mirrors + disk;
> +	struct md_rdev *rdev = info->rdev;
> +
> +	if (!rdev || test_bit(In_sync, &rdev->flags) ||
> +	    atomic_read(&rdev->nr_pending))
> +		return false;
> +
> +	/* Only remove non-faulty devices if recovery is not possible. */
> +	if (!test_bit(Faulty, &rdev->flags) &&
> +	    rdev->mddev->recovery_disabled != conf->recovery_disabled &&
> +	    rdev->mddev->degraded < conf->raid_disks)
> +		return false;
> +
> +	WRITE_ONCE(info->rdev, NULL);
> +	return true;
> +}
> +
>   static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev)
>   {
>   	struct r1conf *conf = mddev->private;
> @@ -1792,15 +1826,13 @@ static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev)
>   				disk_stack_limits(mddev->gendisk, rdev->bdev,
>   						  rdev->data_offset << 9);
>   
> -			p->head_position = 0;
> -			rdev->raid_disk = mirror;
> +			raid1_add_conf(conf, rdev, mirror);
>   			err = 0;
>   			/* As all devices are equivalent, we don't need a full recovery
>   			 * if this was recently any drive of the array
>   			 */
>   			if (rdev->saved_raid_disk < 0)
>   				conf->fullsync = 1;
> -			WRITE_ONCE(p->rdev, rdev);
>   			break;
>   		}
>   		if (test_bit(WantReplacement, &p->rdev->flags) &&
> @@ -1810,13 +1842,11 @@ static int raid1_add_disk(struct mddev *mddev, struct md_rdev *rdev)
>   
>   	if (err && repl_slot >= 0) {
>   		/* Add this device as a replacement */
> -		p = conf->mirrors + repl_slot;
>   		clear_bit(In_sync, &rdev->flags);
>   		set_bit(Replacement, &rdev->flags);
> -		rdev->raid_disk = repl_slot;
> +		raid1_add_conf(conf, rdev, repl_slot);
>   		err = 0;
>   		conf->fullsync = 1;
> -		WRITE_ONCE(p[conf->raid_disks].rdev, rdev);
>   	}
>   
>   	print_conf(conf);
> @@ -1833,27 +1863,20 @@ static int raid1_remove_disk(struct mddev *mddev, struct md_rdev *rdev)
>   	if (unlikely(number >= conf->raid_disks))
>   		goto abort;
>   
> -	if (rdev != p->rdev)
> -		p = conf->mirrors + conf->raid_disks + number;
> +	if (rdev != p->rdev) {
> +		number += conf->raid_disks;
> +		p = conf->mirrors + number;
> +	}
>   
>   	print_conf(conf);
>   	if (rdev == p->rdev) {
> -		if (test_bit(In_sync, &rdev->flags) ||
> -		    atomic_read(&rdev->nr_pending)) {
> -			err = -EBUSY;
> -			goto abort;
> -		}
> -		/* Only remove non-faulty devices if recovery
> -		 * is not possible.
> -		 */
> -		if (!test_bit(Faulty, &rdev->flags) &&
> -		    mddev->recovery_disabled != conf->recovery_disabled &&
> -		    mddev->degraded < conf->raid_disks) {
> +		if (!raid1_remove_conf(conf, number)) {
>   			err = -EBUSY;
>   			goto abort;
>   		}
> -		WRITE_ONCE(p->rdev, NULL);
> -		if (conf->mirrors[conf->raid_disks + number].rdev) {
> +
> +		if (number < conf->raid_disks &&
> +		    conf->mirrors[conf->raid_disks + number].rdev) {
>   			/* We just removed a device that is being replaced.
>   			 * Move down the replacement.  We drain all IO before
>   			 * doing this to avoid confusion.
> @@ -3000,15 +3023,10 @@ static struct r1conf *setup_conf(struct mddev *mddev)
>   		    || disk_idx < 0)
>   			continue;
>   		if (test_bit(Replacement, &rdev->flags))
> -			disk = conf->mirrors + mddev->raid_disks + disk_idx;
> -		else
> -			disk = conf->mirrors + disk_idx;
> +			disk_idx += mddev->raid_disks;
>   
> -		if (disk->rdev)
> +		if (!raid1_add_conf(conf, rdev, disk_idx))
>   			goto abort;
> -		disk->rdev = rdev;
> -		disk->head_position = 0;
> -		disk->seq_start = MaxSector;
>   	}
>   	conf->raid_disks = mddev->raid_disks;
>   	conf->mddev = mddev;
>