Re: [PATCH 1/3] MD: hold mddev lock for .quiesce in md_do_sync

[NeilBrown <neilb@suse.com>]

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

On Sun, Jul 31 2016, shli@kernel.org wrote:

> From: Shaohua Li <shli@fb.com>
>
> .quiesce is called with mddev lock hold at most places. There are few
> exceptions. Calling .quesce without the lock hold could create races. For
> example, the .quesce of raid1 can't be recursively. The purpose of the patches
> is to fix a race in raid5-cache. The raid5-cache .quesce will write md
> superblock and should be called with mddev lock hold.
>
> Cc: NeilBrown <neilb@suse.com>
> Signed-off-by: Shaohua Li <shli@fb.com>

Acked-by: NeilBrown <neilb@suse.com>

This should be safe but I'm not sure I really like it.
The raid1 quiesce could be changed so that it can be called recursively.
The raid5-cache situation would be harder to get right and maybe this is
the best solution... It's just that 'quiesce' should be a fairly
light-weight operation, just waiting for pending requests to flush.  It
shouldn't really *need* a lock.

NeilBrown


> ---
>  drivers/md/md.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 2c3ab6f..0550445 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7945,8 +7945,10 @@ void md_do_sync(struct md_thread *thread)
>  		 * region.
>  		 */
>  		if (mddev->bitmap) {
> +			mddev_lock_nointr(mddev);
>  			mddev->pers->quiesce(mddev, 1);
>  			mddev->pers->quiesce(mddev, 0);
> +			mddev_unlock(mddev);
>  		}
>  	}
>  
> -- 
> 2.7.4

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