Re: [PATCH 6/9] rbd: don't establish watch for read-only mappings

[Dongsheng Yang <dongsheng.yang@xxxxxxxxxxxx>]

On 11/18/2019 09:38 PM, Ilya Dryomov wrote:
> With exclusive lock out of the way, watch is the only thing left that
> prevents a read-only mapping from being used with read-only OSD caps.
>
> Signed-off-by: Ilya Dryomov <idryomov@xxxxxxxxx>
> ---
>   drivers/block/rbd.c | 41 +++++++++++++++++++++++++++--------------
>   1 file changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index aaa359561356..bfff195e8e23 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -6985,6 +6985,24 @@ static int rbd_dev_header_name(struct rbd_device *rbd_dev)
>   	return ret;
>   }
>   
> +static void rbd_print_dne(struct rbd_device *rbd_dev, bool is_snap)
> +{
> +	if (!is_snap) {
> +		pr_info("image %s/%s%s%s does not exist\n",
> +			rbd_dev->spec->pool_name,
> +			rbd_dev->spec->pool_ns ?: "",
> +			rbd_dev->spec->pool_ns ? "/" : "",
> +			rbd_dev->spec->image_name);
> +	} else {
> +		pr_info("snap %s/%s%s%s@%s does not exist\n",
> +			rbd_dev->spec->pool_name,
> +			rbd_dev->spec->pool_ns ?: "",
> +			rbd_dev->spec->pool_ns ? "/" : "",
> +			rbd_dev->spec->image_name,
> +			rbd_dev->spec->snap_name);
> +	}
> +}
> +
>   static void rbd_dev_image_release(struct rbd_device *rbd_dev)
>   {
>   	rbd_dev_unprobe(rbd_dev);
> @@ -7003,6 +7021,7 @@ static void rbd_dev_image_release(struct rbd_device *rbd_dev)
>    */
>   static int rbd_dev_image_probe(struct rbd_device *rbd_dev, int depth)
>   {
> +	bool need_watch = !depth && !rbd_is_ro(rbd_dev);
>   	int ret;
>   
>   	/*
> @@ -7019,22 +7038,21 @@ static int rbd_dev_image_probe(struct rbd_device *rbd_dev, int depth)
>   	if (ret)
>   		goto err_out_format;
>   
> -	if (!depth) {
> +	if (need_watch) {
>   		ret = rbd_register_watch(rbd_dev);
>   		if (ret) {
>   			if (ret == -ENOENT)
> -				pr_info("image %s/%s%s%s does not exist\n",
> -					rbd_dev->spec->pool_name,
> -					rbd_dev->spec->pool_ns ?: "",
> -					rbd_dev->spec->pool_ns ? "/" : "",
> -					rbd_dev->spec->image_name);
> +				rbd_print_dne(rbd_dev, false);
>   			goto err_out_format;
>   		}
>   	}
>   
>   	ret = rbd_dev_header_info(rbd_dev);
> -	if (ret)
> +	if (ret) {
> +		if (ret == -ENOENT && !need_watch)

It's not just "if (ret == -ENOENT)" here, could you explain it more 
about why we need "&& !need_watch"?
> +			rbd_print_dne(rbd_dev, false);
>   		goto err_out_watch;
> +	}
>   
>   	/*
>   	 * If this image is the one being mapped, we have pool name and
> @@ -7048,12 +7066,7 @@ static int rbd_dev_image_probe(struct rbd_device *rbd_dev, int depth)
>   		ret = rbd_spec_fill_names(rbd_dev);
>   	if (ret) {
>   		if (ret == -ENOENT)
> -			pr_info("snap %s/%s%s%s@%s does not exist\n",
> -				rbd_dev->spec->pool_name,
> -				rbd_dev->spec->pool_ns ?: "",
> -				rbd_dev->spec->pool_ns ? "/" : "",
> -				rbd_dev->spec->image_name,
> -				rbd_dev->spec->snap_name);
> +			rbd_print_dne(rbd_dev, true);

is_snap here is always true? IIUC, as we have a watcher for non-snap 
mapping, the rbd_spec_fill_snap_id()
would not be fail with -ENOENT. Is that the reason? If so, can we add an 
rbd_assert(depth); and add
a comment about why we use is_snap == true here?

Thanx
>   		goto err_out_probe;
>   	}
>   
> @@ -7085,7 +7098,7 @@ static int rbd_dev_image_probe(struct rbd_device *rbd_dev, int depth)
>   err_out_probe:
>   	rbd_dev_unprobe(rbd_dev);
>   err_out_watch:
> -	if (!depth)
> +	if (need_watch)
>   		rbd_unregister_watch(rbd_dev);
>   err_out_format:
>   	rbd_dev->image_format = 0;