On Tue, 12 Sep 2023 15:18:33 +0800
Li Xiao Keng [off-list ref] wrote:

On 2023/9/12 0:00, Mariusz Tkaczyk wrote:

quoted

On Thu, 7 Sep 2023 19:37:44 +0800
Li Xiao Keng [off-list ref] wrote:
  

quoted

There is a raid1 with sda and sdb. And we add sdc to this raid,
it may return -EBUSY.

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

Step 2 and 3 will add sdc to metadata of raid1. There will be
udev(change of sdc) event after step4. Then "/usr/sbin/mdadm
--incremental --export $devnode --offroot $env{DEVLINKS}"
will be run, and the sdc will be added to the raid1. Then
step 5 will return -EBUSY because it checks if device isn't
claimed in md_import_device()->lock_rdev()->blkdev_get_by_dev()  
->blkdev_get().    

It will be confusing for users because sdc is added first time.
The "incremental" will get map_lock before add sdc to raid1.
So we add map_lock before write_init_super in "mdadm --add"
to fix the race of "add" and "incremental".

Signed-off-by: Li Xiao Keng <redacted>
Signed-off-by: Guanqin Miao <redacted>  

Hello Li Xiao Keng,
Thanks for the patch, I'm trying to get my head around this.
As I understand, you added a mapfile locking and you are failing when the
log is already claimed, but you prefer to return -1 and print error instead
of returning EBUSY, right?  

No, I do not prefer to return -1 and print error instead of returning EBUSY.
At first, I had the same idea as Martin, thinking that map_lock() would fail
when it is locked by someone else.Actually it is not fail but block. So now I
just print error and go ahead if map_lock() fails, like map_lock() elsewhere.

Ok, thanks now it have more sense!

quoted

If yes, it has nothing to do with fixing. It is just a obscure hack to make
it, let say more reliable. We can either get EBUSY and know that is
expected. I see no difference, your solution is same confusing as current
implementation.

Also, you used map_lock(). I remember that initially I proposed - my bad.
We also rejected udev locking, as a buggy. In fact- I don't see ready
synchronization method to avoid this race in mdadm right now.

The best way, I see is:
1. Write metadata.
2. Give udev chance to process the device.
3. Add it manually if udev failed (or D_NO_UDEV).  

Is there any command to only write metadata?  Or change implementation of
--add ?

I think that we should change implementation for --add.

quoted

If it does not work for you, we need to add synchronization, I know that
there is pidfile used for mdmon, perhaps we can reuse it? We don't need
something fancy, just something to keep all synchronised and block
incremental to led --add finish the action.

We cannot use map_lock() and map_unlock() in this context. It claims lock on
mapfile which describes every array in your system (base details, there is
no info about members), see /var/run/mdadm. Add is just a tiny action to
add disk and we are not going to update the map. Taking this lock will make
--add more vulnerable, for example it will fail because another array is
assembled or created in the same time (I know that it will be hard to
achieve but it is another race - we don't want it).  

When I test this patch, I find map_lock(）in --incremental is block but not
fail. So I think it will not fail when another array is assembled.

Oh, it seems that flock() is waiting for the lock to be acquired, it is
blocking function. Now I read the documentation and I confirmed it.

quoted

And we cannot allow for partially done action, you added this lock after
writing metadata (according to description), we are not taking this lock to
complete the action, just to hide race. Lock should be taken before
writing metadata, but we know that it will not hide an issue.  

Here I add map_lock() before writing metadata and ulock it until finish. Do
you mean it should be before attempt_re_add()?

Indeed you wrote "before write_init_super in "mdadm --add". I was
against taking a map_lock() but now, when I understand that other processes will
wait I see this as a simplest way to keep all synchronized. 

After your clarification- I don't have an objections to take this. Taking
map_lock is not the best approach because we lock all mddevices but I don't see
a strong need to have something better. --add is a user action, low risk of
race with other --add or --assemble/--incremental.

I was against it because I thought that it is not blocking0, i.e. we are failing
if lock cannot be obtained. 

Reviewed-by: Mariusz Tkaczyk <redacted>

Thanks,
Mariusz