Re: [PATCH 2/3] MD: hold mddev lock to change bitmap location

[NeilBrown <neilb@suse.com>]

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

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

> From: Shaohua Li <shli@fb.com>
>
> Changing the location changes a lot of things. Holding the lock to avoid race.
> This makes the .quiesce called with mddev lock hold too.
>
> Cc: NeilBrown <neilb@suse.com>
> Signed-off-by: Shaohua Li <shli@fb.com>

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

Yes, I think this is justified.  As you say, location_store is a fairly
significant change.

Thanks,
NeilBrown

> ---
>  drivers/md/bitmap.c | 47 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> index 6fff794..13041ee 100644
> --- a/drivers/md/bitmap.c
> +++ b/drivers/md/bitmap.c
> @@ -2183,19 +2183,29 @@ location_show(struct mddev *mddev, char *page)
>  static ssize_t
>  location_store(struct mddev *mddev, const char *buf, size_t len)
>  {
> +	int rv;
>  
> +	rv = mddev_lock(mddev);
> +	if (rv)
> +		return rv;
>  	if (mddev->pers) {
> -		if (!mddev->pers->quiesce)
> -			return -EBUSY;
> -		if (mddev->recovery || mddev->sync_thread)
> -			return -EBUSY;
> +		if (!mddev->pers->quiesce) {
> +			rv = -EBUSY;
> +			goto out;
> +		}
> +		if (mddev->recovery || mddev->sync_thread) {
> +			rv = -EBUSY;
> +			goto out;
> +		}
>  	}
>  
>  	if (mddev->bitmap || mddev->bitmap_info.file ||
>  	    mddev->bitmap_info.offset) {
>  		/* bitmap already configured.  Only option is to clear it */
> -		if (strncmp(buf, "none", 4) != 0)
> -			return -EBUSY;
> +		if (strncmp(buf, "none", 4) != 0) {
> +			rv = -EBUSY;
> +			goto out;
> +		}
>  		if (mddev->pers) {
>  			mddev->pers->quiesce(mddev, 1);
>  			bitmap_destroy(mddev);
> @@ -2214,21 +2224,25 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
>  			/* nothing to be done */;
>  		else if (strncmp(buf, "file:", 5) == 0) {
>  			/* Not supported yet */
> -			return -EINVAL;
> +			rv = -EINVAL;
> +			goto out;
>  		} else {
> -			int rv;
>  			if (buf[0] == '+')
>  				rv = kstrtoll(buf+1, 10, &offset);
>  			else
>  				rv = kstrtoll(buf, 10, &offset);
>  			if (rv)
> -				return rv;
> -			if (offset == 0)
> -				return -EINVAL;
> +				goto out;
> +			if (offset == 0) {
> +				rv = -EINVAL;
> +				goto out;
> +			}
>  			if (mddev->bitmap_info.external == 0 &&
>  			    mddev->major_version == 0 &&
> -			    offset != mddev->bitmap_info.default_offset)
> -				return -EINVAL;
> +			    offset != mddev->bitmap_info.default_offset) {
> +				rv = -EINVAL;
> +				goto out;
> +			}
>  			mddev->bitmap_info.offset = offset;
>  			if (mddev->pers) {
>  				struct bitmap *bitmap;
> @@ -2245,7 +2259,7 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
>  				mddev->pers->quiesce(mddev, 0);
>  				if (rv) {
>  					bitmap_destroy(mddev);
> -					return rv;
> +					goto out;
>  				}
>  			}
>  		}
> @@ -2257,6 +2271,11 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
>  		set_bit(MD_CHANGE_DEVS, &mddev->flags);
>  		md_wakeup_thread(mddev->thread);
>  	}
> +	rv = 0;
> +out:
> +	mddev_unlock(mddev);
> +	if (rv)
> +		return rv;
>  	return len;
>  }
>  
> -- 
> 2.7.4

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