Re: [PATCH] md-cluster: Only one thread should request DLM lock

[Neil Brown <neilb@suse.de>]

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

rgoldwyn@suse.de writes:

> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> If a DLM lock is in progress, requesting the same DLM lock will
> result in -EBUSY. Use a mutex to make sure only one thread requests
> for dlm_lock() function at a time.
>
> This will fix the error -EBUSY returned from DLM's
> validate_lock_args().

I can see that we only want one thread calling dlm_lock() with a given
'struct dlm_lock_resource' at a time, otherwise nasty things could
happen.

However if such a race is possible, then aren't there other possibly
complications.

Suppose two threads try to lock the same resource.
Presumably one will try to lock the resource, then the next one (when it
gets the mutex) will discover that it already has the resource, but will
think it has exclusive access - maybe?

Then both threads will eventually try to unlock, and the second one will
unlock something that doesn't have locked.

I'm not certain, but that doesn't sound entirely safe.

Which resources to we actually see races with?

Thanks,
NeilBrown


>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  drivers/md/md-cluster.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
> index 35ac2e8..9b977a2 100644
> --- a/drivers/md/md-cluster.c
> +++ b/drivers/md/md-cluster.c
> @@ -29,6 +29,7 @@ struct dlm_lock_resource {
>  	void (*bast)(void *arg, int mode); /* blocking AST function pointer*/
>  	struct mddev *mddev; /* pointing back to mddev. */
>  	int mode;
> +	struct mutex res_lock;
>  };
>  
>  struct suspend_info {
> @@ -102,14 +103,19 @@ static int dlm_lock_sync(struct dlm_lock_resource *res, int mode)
>  {
>  	int ret = 0;
>  
> +	mutex_lock(&res->res_lock);
> +
>  	ret = dlm_lock(res->ls, mode, &res->lksb,
>  			res->flags, res->name, strlen(res->name),
>  			0, sync_ast, res, res->bast);
> -	if (ret)
> +	if (ret) {
> +		mutex_unlock(&res->res_lock);
>  		return ret;
> +	}
>  	wait_for_completion(&res->completion);
>  	if (res->lksb.sb_status == 0)
>  		res->mode = mode;
> +	mutex_unlock(&res->res_lock);
>  	return res->lksb.sb_status;
>  }
>  
> @@ -134,6 +140,7 @@ static struct dlm_lock_resource *lockres_init(struct mddev *mddev,
>  	res->mode = DLM_LOCK_IV;
>  	namelen = strlen(name);
>  	res->name = kzalloc(namelen + 1, GFP_KERNEL);
> +	mutex_init(&res->res_lock);
>  	if (!res->name) {
>  		pr_err("md-cluster: Unable to allocate resource name for resource %s\n", name);
>  		goto out_err;
> -- 
> 1.8.5.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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