Re: [PATCH v4] nvmet: add missing lock around nvmet_ns_changed in nvmet_ns_revalidate

[Niels Dossche <dossche.niels@gmail.com>]

On 3/14/22 08:17, Christoph Hellwig wrote:
> On Mon, Mar 14, 2022 at 12:41:15AM +0100, Niels Dossche wrote:
>> -	mutex_lock(&ns->subsys->lock);
>>  	if (!ns->enabled) {
>>  		pr_err("enable ns before revalidate.\n");
>> -		mutex_unlock(&ns->subsys->lock);
>>  		return -EINVAL;
>>  	}
>>  	nvmet_ns_revalidate(ns);
>> -	mutex_unlock(&ns->subsys->lock);
>>  	return count;
>>  }
> 
> And now we lose the atomicy vs the enabled check.  I think we'll need
> something like:
> 
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 6fb24746de069..46d0dab686dd6 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -511,7 +511,11 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
>  		goto done;
>  	}
>  
> -	nvmet_ns_revalidate(req->ns);
> +	if (nvmet_ns_revalidate(req->ns)) {
> +		mutex_lock(&req->ns->subsys->lock);
> +		nvmet_ns_changed(req->ns->subsys, req->ns->nsid);
> +		mutex_unlock(&req->ns->subsys->lock);
> +	}
>  
>  	/*
>  	 * nuse = ncap = nsze isn't always true, but we have no way to find
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 091a0ca16361c..06ba477fbf771 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -586,7 +586,8 @@ static ssize_t nvmet_ns_revalidate_size_store(struct config_item *item,
>  		mutex_unlock(&ns->subsys->lock);
>  		return -EINVAL;
>  	}
> -	nvmet_ns_revalidate(ns);
> +	if (nvmet_ns_revalidate(ns))
> +		nvmet_ns_changed(ns->subsys, ns->nsid);
>  	mutex_unlock(&ns->subsys->lock);
>  	return count;
>  }
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 5119c687de683..17cc1cda836cc 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -531,7 +531,7 @@ static void nvmet_p2pmem_ns_add_p2p(struct nvmet_ctrl *ctrl,
>  		ns->nsid);
>  }
>  
> -void nvmet_ns_revalidate(struct nvmet_ns *ns)
> +bool nvmet_ns_revalidate(struct nvmet_ns *ns)
>  {
>  	loff_t oldsize = ns->size;
>  
> @@ -540,8 +540,7 @@ void nvmet_ns_revalidate(struct nvmet_ns *ns)
>  	else
>  		nvmet_file_ns_revalidate(ns);
>  
> -	if (oldsize != ns->size)
> -		nvmet_ns_changed(ns->subsys, ns->nsid);
> +	return oldsize != ns->size;
>  }
>  
>  int nvmet_ns_enable(struct nvmet_ns *ns)
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index af193423c10bb..731f529e11a51 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -542,7 +542,7 @@ u16 nvmet_file_flush(struct nvmet_req *req);
>  void nvmet_ns_changed(struct nvmet_subsys *subsys, u32 nsid);
>  void nvmet_bdev_ns_revalidate(struct nvmet_ns *ns);
>  int nvmet_file_ns_revalidate(struct nvmet_ns *ns);
> -void nvmet_ns_revalidate(struct nvmet_ns *ns);
> +bool nvmet_ns_revalidate(struct nvmet_ns *ns);
>  u16 blk_to_nvme_status(struct nvmet_req *req, blk_status_t blk_sts);
>  
>  bool nvmet_bdev_zns_enable(struct nvmet_ns *ns);
> diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
> index 46bc30fe85d2b..56c20502b7ef3 100644
> --- a/drivers/nvme/target/zns.c
> +++ b/drivers/nvme/target/zns.c
> @@ -123,7 +123,11 @@ void nvmet_execute_identify_cns_cs_ns(struct nvmet_req *req)
>  		goto done;
>  	}
>  
> -	nvmet_ns_revalidate(req->ns);
> +	if (nvmet_ns_revalidate(req->ns)) {
> +		mutex_lock(&req->ns->subsys->lock);
> +		nvmet_ns_changed(req->ns->subsys, req->ns->nsid);
> +		mutex_unlock(&req->ns->subsys->lock);
> +	}
>  	zsze = (bdev_zone_sectors(req->ns->bdev) << 9) >>
>  					req->ns->blksize_shift;
>  	id_zns->lbafe[0].zsze = cpu_to_le64(zsze);

Thank you for your time, this seems like a nice solution.