On Tue, 12 Sep 2023 15:18:33 +0800 Li Xiao Keng <lixiaokeng@xxxxxxxxxx> wrote: > On 2023/9/12 0:00, Mariusz Tkaczyk wrote: > > On Thu, 7 Sep 2023 19:37:44 +0800 > > Li Xiao Keng <lixiaokeng@xxxxxxxxxx> wrote: > > > >> 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 <lixiaokeng@xxxxxxxxxx> > >> Signed-off-by: Guanqin Miao <miaoguanqin@xxxxxxxxxx> > > > > 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! > > > > 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. > > > > 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. > > > > 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 <mariusz.tkaczyk@xxxxxxxxxxxxxxx> Thanks, Mariusz