| Message ID | 20191118133816.3963-7-idryomov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | wip-krbd-readonly | expand |
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;
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
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 >
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;
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;
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(-)