On Mon, Aug 27, 2018 at 7:19 PM Jason Dillaman <jdillama@xxxxxxxxxx> wrote:
>
> On Sun, Aug 26, 2018 at 6:24 AM Ilya Dryomov <idryomov@xxxxxxxxx> wrote:
> >
> > If parent_get class method is not supported by the OSDs, fall back to
> > the legacy class method and assume that the parent is in the default
> > (i.e. "") namespace. The "use the child's image namespace" workaround
> > is no longer needed because creating images within namespaces will
> > require parent_get aware OSDs.
> >
> > Signed-off-by: Ilya Dryomov <idryomov@xxxxxxxxx>
> > ---
> > drivers/block/rbd.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++------
> > 1 file changed, 99 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> > index e86ed5ee7337..45e20af8efb6 100644
> > --- a/drivers/block/rbd.c
> > +++ b/drivers/block/rbd.c
> > @@ -4586,10 +4586,99 @@ static int rbd_dev_v2_features(struct rbd_device *rbd_dev)
> >
> > struct parent_image_spec {
> > u64 pool_id;
> > + const char *pool_ns;
> > const char *image_id;
> > u64 snap_id;
> > };
> >
> > +/*
> > + * The caller is responsible for @pis.
> > + */
> > +static int decode_parent_image_spec(void **p, void *end,
> > + struct parent_image_spec *pis)
> > +{
> > + u8 struct_v;
> > + u32 struct_len;
> > + int ret;
> > +
> > + ret = ceph_start_decoding(p, end, 1, "ParentImageSpec",
> > + &struct_v, &struct_len);
> > + if (ret)
> > + return ret;
> > +
> > + ceph_decode_64_safe(p, end, pis->pool_id, e_inval);
> > + pis->pool_ns = ceph_extract_encoded_string(p, end, NULL, GFP_KERNEL);
> > + if (IS_ERR(pis->pool_ns)) {
> > + ret = PTR_ERR(pis->pool_ns);
> > + pis->pool_ns = NULL;
> > + return ret;
> > + }
> > + pis->image_id = ceph_extract_encoded_string(p, end, NULL, GFP_KERNEL);
> > + if (IS_ERR(pis->image_id)) {
> > + ret = PTR_ERR(pis->image_id);
> > + pis->image_id = NULL;
> > + return ret;
> > + }
> > + ceph_decode_64_safe(p, end, pis->snap_id, e_inval);
> > + return 0;
> > +
> > +e_inval:
> > + return -EINVAL;
> > +}
> > +
> > +static int get_parent_info(struct rbd_device *rbd_dev,
> > + struct parent_image_spec *pis, u64 *overlap)
> > +{
> > + struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
> > + struct page *req_page, *reply_page;
> > + size_t reply_len = PAGE_SIZE;
> > + void *p, *end;
> > + int ret;
> > +
> > + req_page = alloc_page(GFP_KERNEL);
> > + if (!req_page)
> > + return -ENOMEM;
> > +
> > + reply_page = alloc_page(GFP_KERNEL);
> > + if (!reply_page) {
> > + __free_page(req_page);
> > + return -ENOMEM;
> > + }
> > +
> > + p = page_address(req_page);
> > + ceph_encode_64(&p, rbd_dev->spec->snap_id);
> > + ret = ceph_osdc_call(osdc, &rbd_dev->header_oid, &rbd_dev->header_oloc,
> > + "rbd", "parent_get", CEPH_OSD_FLAG_READ,
> > + req_page, sizeof(u64), reply_page, &reply_len);
> > + if (ret)
> > + goto out;
> > +
> > + p = page_address(reply_page);
> > + end = p + reply_len;
> > + ret = decode_parent_image_spec(&p, end, pis);
> > + if (ret)
> > + goto out;
> > +
> > + ret = ceph_osdc_call(osdc, &rbd_dev->header_oid, &rbd_dev->header_oloc,
> > + "rbd", "parent_overlap_get", CEPH_OSD_FLAG_READ,
> > + req_page, sizeof(u64), reply_page, &reply_len);
> > + if (ret)
> > + goto out;
> > +
> > + p = page_address(reply_page);
> > + end = p + reply_len;
> > + ceph_decode_64_safe(&p, end, *overlap, e_inval);
> > +
> > +out:
> > + __free_page(req_page);
> > + __free_page(reply_page);
> > + return ret;
> > +
> > +e_inval:
> > + ret = -EINVAL;
> > + goto out;
> > +}
> > +
> > /*
> > * The caller is responsible for @pis.
> > */
> > @@ -4653,11 +4742,13 @@ static int rbd_dev_v2_parent_info(struct rbd_device *rbd_dev)
> > if (!parent_spec)
> > return -ENOMEM;
> >
> > - ret = get_parent_info_legacy(rbd_dev, &pis, &overlap);
> > + ret = get_parent_info(rbd_dev, &pis, &overlap);
> > + if (ret == -EOPNOTSUPP)
>
> If, for some strange reason, "get_parent_info" failed after it
> allocated the "pis->image_id", is that a potential memory leak if
> "get_parent_info_legacy" just re-allocates it?
Technically yes, but get_parent_info_legacy() shouldn't be called in
that case. For get_parent_info() to get to allocating pis->image_id,
ceph_osdc_call(..., "parent_get", ...) must succeed and that is the
only thing that can realistically fail with -EOPNOTSUPP.
>
> > + ret = get_parent_info_legacy(rbd_dev, &pis, &overlap);
> > if (ret)
> > goto out_err;
> > - dout("%s pool_id %llu image_id %s snap_id %llu overlap %llu\n",
> > - __func__, pis.pool_id, pis.image_id, pis.snap_id,
> > + dout("%s pool_id %llu pool_ns %s image_id %s snap_id %llu overlap %llu\n",
> > + __func__, pis.pool_id, pis.pool_ns, pis.image_id, pis.snap_id,
>
> Is it OK to pass a potential null pointer pool_ns string here?
Yes, it emits "(null)" or something like that.
Thanks,
Ilya
