On Mon, Oct 02 2017, Jes Sorensen wrote:
> On 09/19/2017 02:49 AM, NeilBrown wrote:
>> On Tue, Sep 19 2017, Xiao Ni wrote:
>>
>>> After mdadm -S /dev/md0, the device node /dev/md0 still exists. The Remove
>>> events are generated by md_free() -> del_gendisk() -> blk_unregister_queue().
>>> After calling close(mdfd) the Remove events is generated. We should give udev
>>> a little time to handle the event.
>>>
>>> I tried usleep(100*1000), but the problem still can be reproduced sometime.
>>> So I choose to sleep(1). Because after close(mdfd) it can be generated CHANGE
>>> events too. So it's ok to choose to sleep(1) to wait udev to handle CHANGE
>>> events.
>>
>> I really don't like this approach. The fact that 1 second works for you
>> is no guarantee that it will work for everybody.
>> We have a few sleeps in the code already, but I don't like them either.
>> Let's try not to add more.
>>
>> If there is some event that you want to wait for, wait for that event.
>> e.g. if you want to wait for /dev/md0 to disappear then write a loop:
>>
>> while /dev/md0 exists
>> usleep(1000)
>>
>> But I'm still not convinced that this is really needed. If it is, then
>> maybe some sort of kernel fix would be better.
>
> I agree completely - any case where we need a sleep() is a warning that
> there is probably a bigger problem that needs to be addressed.
We current call sleep:
In Assemble.c ... to wait for everything to have closed the device
so that the next open goes through a path in __blkdev_get()
which calls bd_set_size().
We would need some change in __blkdev_get() to remove the need for
this.
In Create.c to give udev a chance to ignore the Change event
caused by closing the device, before the remove the file which
is causing udev to ignore the events.
Possible we could system("udevadm settle") insteadm.
In Grow.c:start_reshape, waiting for MD_RECOVERY_RUNNING to be
clear (I think). Maybe kernel could be more clever about this.
In Grow.c:Grow_continue_command ... I think this is waiting for
the newly started reshape to be reflected in the metadata... not sure.
In Manage.c:Manage_stop() - avoiding races with transient use of the
array which might cause the array to refuse to go inactive.
We might be able to get the kernel to check is uses are transient,
and block new ones... not sure.
In Manage.c:Manage_stop() again, waiting for the "critical section"
of a reshape to pass. Could maybe teach kernel to let us poll
"sync_max".
In Manage.c:Manage_stop() third time, - waiting for 'sync_action' to
stablize. Maybe we can teach the kernel to provide more stable
values.
... fourth time , same as first time.
In Manage.c:Manage_remove() : wait for 'fail' to be completely
processed so that 'remove' can happen. Maybe we should poll() some
sysfs thing.
In managemon.c:replace_array() - wait for monitor thread (in mdmon)
to make progress. Probably no value in changing this.
In managemon.c:manage_member() - again, waiting for monitor thread
In managemon.c: handle_message - and again
In managemon.c:handle_message again - more waiting.
Maybe monitor could indicate progress to managemon somehow.
In mdmon.c:clone_monitor: wait for monitor thread to start up. As above.
In super-intel.c:get_super_block - avoid race with mdmon which might be
writing metadata while mdadm tries to read it. I wonder if advisory
locking could be used here. Do flock locks work on block devices?
In super-intel.c:load_super_imsm - as above
In super-intel.c:wait_for_reshape_imsm - wait for reshape to stablize.
Maybe similar to third time of Manage_stop.
In util.c:open_dev_excl() - avoid race with transient O_EXCL .. I
guess. This isn't well documented.
In util.c:wait_for() - wait for a device to appear in /dev. Maybe
"udevadm settle" is better...
In util.c:hot_remove_disk() - similar to Manage_remove() above
In util.c:sys_hot_remove_disk() - same as above.
I'm fairly sure we can improve the kernel so that several of these
can be removed, or replaced with select/poll. Others probably have to
stay. using "udevadm settle" might be a good idea, but we would want
to make sure we can reproduce the problem, then be sure it is fixed.
For the current issue, fixing the kernel is probably a good idea, but
creating a "wait_while()" - similar to wait_for(), but waits while a
device exists in /dev - is probably simplest.
NeilBrown
Attachment:
signature.asc
Description: PGP signature
