On Tue, Nov 19, 2019 at 9:38 AM Dongsheng Yang
<dongsheng.yang@xxxxxxxxxxxx> 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@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"?
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
