diff mbox series

[md-6.9,v3,02/11] md/raid1: factor out helpers to add rdev to conf

Message ID 20240228114333.527222-3-yukuai1@huaweicloud.com (mailing list archive)
State Superseded, archived
Headers show
Series md/raid1: refactor read_balance() and some minor fix | expand

Commit Message

Yu Kuai Feb. 28, 2024, 11:43 a.m. UTC 
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(-)

Comments

Yu Kuai Feb. 29, 2024, 2:11 a.m. UTC | #1 
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;
>
diff mbox series

Patch

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;
+}
+
+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;