Re: [MDADM PATCH 2/2] Give enough time to udev to handle events

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]

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.

NeilBrown


>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  Create.c |  2 +-
>  mdadm.c  | 16 +++++++++++++++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/Create.c b/Create.c
> index 239545f..24e852e 100644
> --- a/Create.c
> +++ b/Create.c
> @@ -1054,7 +1054,7 @@ int Create(struct supertype *st, char *mddev,
>  	/* Give udev a moment to process the Change event caused
>  	 * by the close.
>  	 */
> -	usleep(100*1000);
> +	sleep(1);
>  	udev_unblock();
>  	return 0;
>  
> diff --git a/mdadm.c b/mdadm.c
> index 7cdcdba..2905dea 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -1734,8 +1734,14 @@ int main(int argc, char *argv[])
>  		autodetect();
>  		break;
>  	}
> -	if (mdfd > 0)
> +
> +	if (mdfd > 0) {
>  		close(mdfd);
> +		/* Give udev a moment to process the udev event caused
> +		 * by the close.
> +		 */
> +		sleep(1);
> +	}
>  	exit(rv);
>  }
>  
> @@ -1897,6 +1903,10 @@ static int stop_scan(int verbose)
>  				else
>  					progress = 1;
>  				close(mdfd);
> +				/* Give udev a moment to process the Remove event caused
> +				 * by the close.
> +				 */
> +				sleep(1);
>  			}
>  
>  			put_md_name(name);
> @@ -1997,6 +2007,10 @@ static int misc_list(struct mddev_dev *devlist,
>  				break;
>  			}
>  			close(mdfd);
> +			/* Give udev a moment to process the udev event caused
> +			 * by the close.
> +			 */
> +			sleep(1);
>  		} else
>  			rv |= 1;
>  	}
> -- 
> 2.7.4

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]