On 2023/9/6 3:08, Martin Wilck wrote:

On Wed, 2023-09-06 at 00:17 +0800, Coly Li wrote:

quoted

Hi Xiao Keng,

Thanks for the updated version, I add my comments inline.

On Tue, Sep 05, 2023 at 08:02:06PM +0800, Li Xiao Keng wrote:

quoted

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

Where is above -EBUSY from? do you mean mdadm command returns
-EBUSY or it is returned by some specific function in mdadm
source code.

Because the new disk is added to the raid by "mdadm --incremental",
the "mdadm --add" will return the err.

quoted

quoted

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.

Dose returning -EBUSY hurt anything? Or only returns -EBUSY and
other stuffs all work as expected?

IIUC, it does not. The manual --add command will fail. Li Xiao Keng has
described the problem in earlier emails.

Yes！ The disk is add to the raid, but the manual --add command will fail.
We will decide the next action based on the return value.

quoted

quoted

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

I am not familiar this part of code, but I see ignoring the failure
from map_lock() in Assemble() is on purpose by Neil. Therefore I
just guess simply return from Assemble when map_lock() fails might
not be what you wanted.

quoted

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;

Especially when the error message noticed "continue anyway" but a
return 1
followed, the behavior might be still confusing.

Now as you're saying it, I recall I had the same comment last time ;-)

I'm very sorry for this stupid mistake. I I find I send v1 patch but not
v2. I will send patch v2 to instead of it.

-	if (map_lock(&map))
-		pr_err("failed to get exclusive lock on mapfile - continue anyway...\n");
+	if (map_lock(&map)) {
+		pr_err("failed to get exclusive lock on mapfile when assemble raid.\n");
+		return 1;
+	}

I might add that "return 1" is dangerous, as it pretends that
Manage_add() was successful and actually added a device, which is not
the case. In the special case that Li Xiao Keng wants to fix, it's true
(sort of) because the asynchronous "mdadm -I" will have added the
device already. But there could be other races where Assemble_map()
can't obtain the lock and still the device will not be added later.

Do I missunstandings
"AFAICS it would only help if the code snipped above did not only
pr_err() but exit if it can't get an exclusive lock." ?

Anyway, map_lock is a blocking function. If it can't get the lock, it blocks.
If map_lock() return error, Assemble() return 1. When -add unlock it,
Assemble() will go ahead but not return at map_lock().

Do you think it should remain as it is? Like:

	if (map_lock(&map))
		pr_err("failed to get exclusive lock on mapfile - continue anyway...\n");

quoted

quoted

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