[6/9] rbd: don't establish watch for read-only mappings
diff mbox series

Message ID 20191118133816.3963-7-idryomov@gmail.com
State New
Headers show
Series
  • wip-krbd-readonly
Related show

Commit Message

Ilya Dryomov Nov. 18, 2019, 1:38 p.m. UTC
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@gmail.com>
---
 drivers/block/rbd.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

Comments

Dongsheng Yang Nov. 19, 2019, 8:37 a.m. UTC | #1
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@gmail.com>
> ---
>   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;
Ilya Dryomov Nov. 19, 2019, 11:42 a.m. UTC | #2
On Tue, Nov 19, 2019 at 9:38 AM Dongsheng Yang
<dongsheng.yang@easystack.cn> wrote:
>
>
>
> 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@gmail.com>
> > ---
> >   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"?

Just a mechanical transformation, I think.

There were two pr_infos before this patch, one for images and one for
snapshots.  Because we don't call rbd_register_watch() in the read-only
case anymore, we need a second pr_info for images.  One is "active" for
the normal case (need_watch), the other is "active" for the read-only
case (!need_watch).

Since only one ENOENT is expected, we could just "if (ret == -ENOENT)",
"&& !need_watch" isn't strictly needed.

> > +                     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?

I don't think we need an assert here.  This just wraps the pr_info that
has been there for years, no other change is made.

Thanks,

                Ilya
Dongsheng Yang Nov. 19, 2019, 12:06 p.m. UTC | #3
On 11/19/2019 07:42 PM, Ilya Dryomov wrote:
> On Tue, Nov 19, 2019 at 9:38 AM Dongsheng Yang
> <dongsheng.yang@easystack.cn> wrote:
>>
>>
>> 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@gmail.com>
>>> ---
>>>    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"?
> Just a mechanical transformation, I think.
>
> There were two pr_infos before this patch, one for images and one for
> snapshots.  Because we don't call rbd_register_watch() in the read-only
> case anymore, we need a second pr_info for images.  One is "active" for
> the normal case (need_watch), the other is "active" for the read-only
> case (!need_watch).
>
> Since only one ENOENT is expected, we could just "if (ret == -ENOENT)",
> "&& !need_watch" isn't strictly needed.

Okey, thanx
>
>>> +                     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?
> I don't think we need an assert here.  This just wraps the pr_info that
> has been there for years, no other change is made.

Okey, let's keep this logic.
>
> Thanks,
>
>                  Ilya
>
Dongsheng Yang Nov. 19, 2019, 12:12 p.m. UTC | #4
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@gmail.com>

Reviewed-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
> ---
>   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)
> +			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);
>   		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;

Patch
diff mbox series

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)
+			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);
 		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;