Re: [PATCH] Fix race of "mdadm --add" and "mdadm --incremental"

2023年5月8日 09:30，Li Xiao Keng [off-list ref] 写道：

ping

On 2023/4/23 9:30, Li Xiao Keng wrote:

quoted

ping

On 2023/4/17 22:01, Li Xiao Keng wrote:

quoted

When we add a new disk to a raid, it may return -EBUSY.

The main process of --add:
1. dev_open
2. store_super1(st, di->fd) in write_init_super1
3. fsync(di->fd) in write_init_super1
4. close(di->fd)
5. ioctl(ADD_NEW_DISK)

However, there will be some udev(change) event after step4. Then
"/usr/sbin/mdadm --incremental ..." will be run, and the new disk
will be add to md device. After that, ioctl will return -EBUSY.

Hi Xiao Keng,

The above description of the race is not informative enough, I am aware exactly how the race is from, therefore I am not able to response for the fix.

Coly Li

quoted

quoted

Here we add map_lock before write_init_super in "mdadm --add"
to fix this race.

Signed-off-by: Li Xiao Keng <redacted>
Signed-off-by: Guanqin Miao <redacted>
---
Assemble.c |  5 ++++-
Manage.c   | 25 +++++++++++++++++--------
2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index 49804941..086890ed 100644
--- a/Assemble.c
+++ b/Assemble.c

@@ -1479,8 +1479,11 @@ try_again:

* to our list.  We flag them so that we don't try to re-add,
* but can remove if they turn out to not be wanted.
*/
- if (map_lock(&map))
+ if (map_lock(&map)) {
pr_err("failed to get exclusive lock on mapfile - continue anyway...\n");
+ return 1;
+ }
+
if (c->update == UOPT_UUID)
mp = NULL;
else

diff --git a/Manage.c b/Manage.c
index f54de7c6..6a101bae 100644
--- a/Manage.c
+++ b/Manage.c

@@ -703,6 +703,7 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,

struct supertype *dev_st;
int j;
mdu_disk_info_t disc;
+ struct map_ent *map = NULL;

if (!get_dev_size(tfd, dv->devname, &ldsize)) {
if (dv->disposition == 'M')

@@ -900,6 +901,10 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,

disc.raid_disk = 0;
}

+ if (map_lock(&map)) {
+ pr_err("failed to get exclusive lock on mapfile when add disk\n");
+ return -1;
+ }
if (array->not_persistent==0) {
int dfd;
if (dv->disposition == 'j')

@@ -911,9 +916,9 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,

dfd = dev_open(dv->devname, O_RDWR | O_EXCL|O_DIRECT);
if (tst->ss->add_to_super(tst, &disc, dfd,
 dv->devname, INVALID_SECTORS))
- return -1;
+ goto unlock;
if (tst->ss->write_init_super(tst))
- return -1;
+ goto unlock;
} else if (dv->disposition == 'A') {
/*  this had better be raid1.
* As we are "--re-add"ing we must find a spare slot

@@ -971,14 +976,14 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,

pr_err("add failed for %s: could not get exclusive access to container\n",
      dv->devname);
tst->ss->free_super(tst);
- return -1;
+ goto unlock;
}

/* Check if metadata handler is able to accept the drive */
if (!tst->ss->validate_geometry(tst, LEVEL_CONTAINER, 0, 1, NULL,
   0, 0, dv->devname, NULL, 0, 1)) {
close(container_fd);
- return -1;
+ goto unlock;
}

Kill(dv->devname, NULL, 0, -1, 0);

@@ -987,7 +992,7 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,
 dv->devname, INVALID_SECTORS)) {

close(dfd);
close(container_fd);
- return -1;
+ goto unlock;
}
if (!mdmon_running(tst->container_devnm))
tst->ss->sync_metadata(tst);

@@ -998,7 +1003,7 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,
      dv->devname);

close(container_fd);
tst->ss->free_super(tst);
- return -1;
+ goto unlock;
}
sra->array.level = LEVEL_CONTAINER;
/* Need to set data_offset and component_size */

@@ -1013,7 +1018,7 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,

pr_err("add new device to external metadata failed for %s\n", dv->devname);
close(container_fd);
sysfs_free(sra);
- return -1;
+ goto unlock;
}
ping_monitor(devnm);
sysfs_free(sra);

@@ -1027,7 +1032,7 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,

else
pr_err("add new device failed for %s as %d: %s\n",
      dv->devname, j, strerror(errno));
- return -1;
+ goto unlock;
}
if (dv->disposition == 'j') {
pr_err("Journal added successfully, making %s read-write\n", devname);

@@ -1038,7 +1043,11 @@ int Manage_add(int fd, int tfd, struct mddev_dev *dv,

}
if (verbose >= 0)
pr_err("added %s\n", dv->devname);
+ map_unlock(&map);
return 1;
+unlock:
+ map_unlock(&map);
+ return -1;
}

int Manage_remove(struct supertype *tst, int fd, struct mddev_dev *dv,