Re: [PATCH 19/20] rbd: support for object-map and fast-diff

[Dongsheng Yang <dongsheng.yang@xxxxxxxxxxxx>]

On 02/07/2019 19:55, Jason Dillaman wrote:
> On Tue, Jul 2, 2019 at 2:16 AM Dongsheng Yang
> <dongsheng.yang@xxxxxxxxxxxx> wrote:
> > Hi Jason,
> >
> > On 07/02/2019 12:09 AM, Jason Dillaman wrote:
> > > On Tue, Jun 25, 2019 at 10:42 AM Ilya Dryomov <idryomov@xxxxxxxxx> wrote:
> > > > Speed up reads, discards and zeroouts through RBD_OBJ_FLAG_MAY_EXIST
> > > > and RBD_OBJ_FLAG_NOOP_FOR_NONEXISTENT based on object map.
> > > >
> > > > Invalid object maps are not trusted, but still updated.  Note that we
> > > > never iterate, resize or invalidate object maps.  If object-map feature
> > > > is enabled but object map fails to load, we just fail the requester
> > > > (either "rbd map" or I/O, by way of post-acquire action).
> > ... ...
> > > > +}
> > > > +
> > > > +static int rbd_object_map_open(struct rbd_device *rbd_dev)
> > > > +{
> > > > +       int ret;
> > > > +
> > > > +       ret = rbd_object_map_lock(rbd_dev);
> > > Only lock/unlock if rbd_dev->spec.snap_id == CEPH_NOSNAP?
> > Hmm, IIUC, rbd_object_map_open() is called in post exclusive-lock
> > acquired, when
> > rbd_dev->spec.snap_id != CEPH_NOSNAP, we don't need to acquire
> > exclusive-lock I think.
> Userspace opens the object-map for snapshots (and therefore parent
> images) are well. It doesn't require the exclusive-lock since the
> images should be immutable at the snapshot.


Okey, when this version of implementation is trying to map a snapshot, 
it will call

do_rbd_add()

 -> rbd_dev_image_probe()

        -> rbd_object_map_load()

code as below: (in rbd_dev_image_probe())

    if (rbd_dev->spec->snap_id != CEPH_NOSNAP &&
        (rbd_dev->header.features & RBD_FEATURE_OBJECT_MAP)) {
        ret = rbd_object_map_load(rbd_dev);
        if (ret)
            goto err_out_probe;
    }

That means, when we are opening object-map for snapshot, it will call 
rbd_object_map_load() directly,

instead of rbd_object_map_open() which will do object-map lock.

> > But maybe we can add an assert in this function to make it clear.
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       ret = rbd_object_map_load(rbd_dev);
> > > > +       if (ret) {
> > > > +               rbd_object_map_unlock(rbd_dev);
> > > > +               return ret;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static void rbd_object_map_close(struct rbd_device *rbd_dev)
> > > > +{
> > > > +       rbd_object_map_free(rbd_dev);
> > > > +       rbd_object_map_unlock(rbd_dev);
> > > > +}
> > > > +
> > > > +/*
> > > > + * This function needs snap_id (or more precisely just something to
> > > > + * distinguish between HEAD and snapshot object maps), new_state and
> > > > + * current_state that were passed to rbd_object_map_update().
> > > > + *
> > > > + * To avoid allocating and stashing a context we piggyback on the OSD
> > > > + * request.  A HEAD update has two ops (assert_locked).  For new_state
> > > > + * and current_state we decode our own object_map_update op, encoded in
> > > > + * rbd_cls_object_map_update().
> > > Decoding the OSD request seems a little awkward. Since you would only
> > > update the in-memory state for the HEAD revision, could you just stash
> > > these fields in the "rbd_object_request" struct? Then in
> > > "rbd_object_map_update", set the callback to either a
> > > "rbd_object_map_snapshot_callback" callback or
> > > "rbd_object_map_head_callback".
> > Good idea, even we don't need two callback, because the
> > rbd_object_map_update_finish() will not update snapshot:
> The deep-flatten feature requires updating object-map snapshots for
> the child image during copy-up. There just isn't an in-memory version
> of the object-map for that case, if that is what you are refering to.


I mean, the current callback in this patch:

rbd_object_map_callback() is smart enough to know need to update 
in-memory object-map or not.

code as below:

rbd_object_map_callback()

   ->rbd_object_map_update_finish()


In rbd_object_map_update_finish:

    /*
     * Nothing to do for a snapshot object map.
     */
    if (osd_req->r_num_ops == 1)
        return 0;


it will check the r_num_ops to know whether we need to update in-memory 
object-map. because

when we are updating a HEAD, we will send two ops in this request 
(assert_locked + object_map_update).


So we don't need to provide two callbacks like 
"rbd_object_map_head_callback" and "rbd_object_map_snapshot_callback" ,

but just the current "rbd_object_map_callback" can handle the two cases.


Thanx

Yang

> > +    /*
> > +     * Nothing to do for a snapshot object map.
> > +     */
> > +    if (osd_req->r_num_ops == 1)
> > +        return 0;
> > > > + */
> > > > +static int rbd_object_map_update_finish(struct rbd_obj_request *obj_req,
> > > > +                                       struct ceph_osd_request *osd_req)
> > > > +{
> > > > +       struct rbd_device *rbd_dev = obj_req->img_request->rbd_dev;
> > > > +       struct ceph_osd_data *osd_data;
> > > > +       u64 objno;
> > > > +       u8 state, new_state, current_state;
> > > > +       bool has_current_state;
> > > > +       void *p;
> > ... ...
> > > > +/*
> > > > + * Return:
> > > > + *   0 - object map update sent
> > > > + *   1 - object map update isn't needed
> > > > + *  <0 - error
> > > > + */
> > > > +static int rbd_obj_write_post_object_map(struct rbd_obj_request *obj_req)
> > > > +{
> > > > +       struct rbd_device *rbd_dev = obj_req->img_request->rbd_dev;
> > > > +       u8 current_state = OBJECT_PENDING;
> > > > +
> > > > +       if (!(rbd_dev->header.features & RBD_FEATURE_OBJECT_MAP))
> > > > +               return 1;
> > > > +
> > > > +       if (!(obj_req->flags & RBD_OBJ_FLAG_DELETION))
> > > > +               return 1;
> > > > +
> > > > +       return rbd_object_map_update(obj_req, CEPH_NOSNAP, OBJECT_NONEXISTENT,
> > > > +                                    &current_state);
> > > > +}
> > > > +
> > > >    static bool rbd_obj_advance_write(struct rbd_obj_request *obj_req, int *result)
> > > >    {
> > > >           struct rbd_device *rbd_dev = obj_req->img_request->rbd_dev;
> > > > @@ -2805,6 +3419,24 @@ static bool rbd_obj_advance_write(struct rbd_obj_request *obj_req, int *result)
> > > >           case RBD_OBJ_WRITE_START:
> > > >                   rbd_assert(!*result);
> > > >
> > > > +               if (rbd_obj_write_is_noop(obj_req))
> > > > +                       return true;
> > > Does this properly handle the case where it has a parent overlap? If
> > > the child object doesn't exist, we would still want to perform the
> > > copyup (if required), correct?
> > Good point. I found the zeroout case is handled, but discard not.
> >
> > zeroout will check  (!obj_req->num_img_extents) before setting NOOP
> > flag. but discard check it after setting.
> > Thanx
> > Yang
> > > > +               ret = rbd_obj_write_pre_object_map(obj_req);
> > > > +               if (ret < 0) {
> > > > +                       *result = ret;
> > > > +                       return true;
> > > > +               }
> > > > +               obj_req->write_state = RBD_OBJ_WRITE_PRE_OBJECT_MAP;
> > > > +               if (ret > 0)
> > > > +                       goto again;
> > > > +               return false;
> > > > +       case RBD_OBJ_WRITE_PRE_OBJECT_MAP:
> > > > +               if (*result) {
> > > > +                       rbd_warn(rbd_dev, "pre object map update failed: %d",
> > > > +                                *result);
> > > > +                       return true;
> > > > +               }
> > > >                   ret = rbd_obj_write_object(obj_req);
> > > >                   if (ret) {
> > > >                           *result = ret;
> > > > @@ -2837,8 +3469,23 @@ static bool rbd_obj_advance_write(struct rbd_obj_request *obj_req, int *result)
> > > >                           return false;
> > > >                   /* fall through */
> > > >           case RBD_OBJ_WRITE_COPYUP:
> > > > -               if (*result)
> > > > +               if (*result) {
> > > >                           rbd_warn(rbd_dev, "copyup failed: %d", *result);
> > > > +                       return true;
> > > > +               }
> > > > +               ret = rbd_obj_write_post_object_map(obj_req);
> > > > +               if (ret < 0) {
> > > > +                       *result = ret;
> > > > +                       return true;
> > > > +               }
> > > > +               obj_req->write_state = RBD_OBJ_WRITE_POST_OBJECT_MAP;
> > > > +               if (ret > 0)
> > > > +                       goto again;
> > > > +               return false;
> > > > +       case RBD_OBJ_WRITE_POST_OBJECT_MAP:
> > > > +               if (*result)
> > > > +                       rbd_warn(rbd_dev, "post object map update failed: %d",
> > > > +                                *result);
> > > >                   return true;
> > > >           default:
> > > >                   BUG();
> > > > @@ -2892,7 +3539,8 @@ static bool need_exclusive_lock(struct rbd_img_request *img_req)
> > > >                   return false;
> > > >
> > > >           rbd_assert(!test_bit(IMG_REQ_CHILD, &img_req->flags));
> > > > -       if (rbd_dev->opts->lock_on_read)
> > > > +       if (rbd_dev->opts->lock_on_read ||
> > > > +           (rbd_dev->header.features & RBD_FEATURE_OBJECT_MAP))
> > > >                   return true;
> > > >
> > > >           return rbd_img_is_write(img_req);
> > > > @@ -3427,7 +4075,7 @@ static int rbd_try_lock(struct rbd_device *rbd_dev)
> > > >                   if (ret)
> > > >                           goto out; /* request lock or error */
> > > >
> > > > -               rbd_warn(rbd_dev, "%s%llu seems dead, breaking lock",
> > > > +               rbd_warn(rbd_dev, "breaking header lock owned by %s%llu",
> > > >                            ENTITY_NAME(lockers[0].id.name));
> > > >
> > > >                   ret = ceph_monc_blacklist_add(&client->monc,
> > > > @@ -3454,6 +4102,19 @@ static int rbd_try_lock(struct rbd_device *rbd_dev)
> > > >           return ret;
> > > >    }
> > > >
> > > > +static int rbd_post_acquire_action(struct rbd_device *rbd_dev)
> > > > +{
> > > > +       int ret;
> > > > +
> > > > +       if (rbd_dev->header.features & RBD_FEATURE_OBJECT_MAP) {
> > > > +               ret = rbd_object_map_open(rbd_dev);
> > > > +               if (ret)
> > > > +                       return ret;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > >    /*
> > > >     * Return:
> > > >     *   0 - lock acquired
> > > > @@ -3497,6 +4158,17 @@ static int rbd_try_acquire_lock(struct rbd_device *rbd_dev)
> > > >           rbd_assert(rbd_dev->lock_state == RBD_LOCK_STATE_LOCKED);
> > > >           rbd_assert(list_empty(&rbd_dev->running_list));
> > > >
> > > > +       ret = rbd_post_acquire_action(rbd_dev);
> > > > +       if (ret) {
> > > > +               rbd_warn(rbd_dev, "post-acquire action failed: %d", ret);
> > > > +               /*
> > > > +                * Can't stay in RBD_LOCK_STATE_LOCKED because
> > > > +                * rbd_lock_add_request() would let the request through,
> > > > +                * assuming that e.g. object map is locked and loaded.
> > > > +                */
> > > > +               rbd_unlock(rbd_dev);
> > > > +       }
> > > > +
> > > >    out:
> > > >           wake_requests(rbd_dev, ret);
> > > >           up_write(&rbd_dev->lock_rwsem);
> > > > @@ -3570,10 +4242,17 @@ static bool rbd_quiesce_lock(struct rbd_device *rbd_dev)
> > > >           return true;
> > > >    }
> > > >
> > > > +static void rbd_pre_release_action(struct rbd_device *rbd_dev)
> > > > +{
> > > > +       if (rbd_dev->header.features & RBD_FEATURE_OBJECT_MAP)
> > > > +               rbd_object_map_close(rbd_dev);
> > > > +}
> > > > +
> > > >    static void __rbd_release_lock(struct rbd_device *rbd_dev)
> > > >    {
> > > >           rbd_assert(list_empty(&rbd_dev->running_list));
> > > >
> > > > +       rbd_pre_release_action(rbd_dev);
> > > >           rbd_unlock(rbd_dev);
> > > >    }
> > > >
> > > > @@ -4860,6 +5539,8 @@ static struct rbd_device *__rbd_dev_create(struct rbd_client *rbdc,
> > > >           init_completion(&rbd_dev->acquire_wait);
> > > >           init_completion(&rbd_dev->releasing_wait);
> > > >
> > > > +       spin_lock_init(&rbd_dev->object_map_lock);
> > > > +
> > > >           rbd_dev->dev.bus = &rbd_bus_type;
> > > >           rbd_dev->dev.type = &rbd_device_type;
> > > >           rbd_dev->dev.parent = &rbd_root_dev;
> > > > @@ -5041,6 +5722,32 @@ static int rbd_dev_v2_features(struct rbd_device *rbd_dev)
> > > >                                                   &rbd_dev->header.features);
> > > >    }
> > > >
> > > > +/*
> > > > + * These are generic image flags, but since they are used only for
> > > > + * object map, store them in rbd_dev->object_map_flags.
> > > > + *
> > > > + * For the same reason, this function is called only on object map
> > > > + * (re)load and not on header refresh.
> > > > + */
> > > > +static int rbd_dev_v2_get_flags(struct rbd_device *rbd_dev)
> > > > +{
> > > > +       __le64 snapid = cpu_to_le64(rbd_dev->spec->snap_id);
> > > > +       __le64 flags;
> > > > +       int ret;
> > > > +
> > > > +       ret = rbd_obj_method_sync(rbd_dev, &rbd_dev->header_oid,
> > > > +                                 &rbd_dev->header_oloc, "get_flags",
> > > > +                                 &snapid, sizeof(snapid),
> > > > +                                 &flags, sizeof(flags));
> > > > +       if (ret < 0)
> > > > +               return ret;
> > > > +       if (ret < sizeof(flags))
> > > > +               return -EBADMSG;
> > > > +
> > > > +       rbd_dev->object_map_flags = le64_to_cpu(flags);
> > > > +       return 0;
> > > > +}
> > > > +
> > > >    struct parent_image_info {
> > > >           u64             pool_id;
> > > >           const char      *pool_ns;
> > > > @@ -6014,6 +6721,7 @@ static void rbd_dev_unprobe(struct rbd_device *rbd_dev)
> > > >           struct rbd_image_header *header;
> > > >
> > > >           rbd_dev_parent_put(rbd_dev);
> > > > +       rbd_object_map_free(rbd_dev);
> > > >           rbd_dev_mapping_clear(rbd_dev);
> > > >
> > > >           /* Free dynamic fields from the header, then zero it out */
> > > > @@ -6263,6 +6971,13 @@ static int rbd_dev_image_probe(struct rbd_device *rbd_dev, int depth)
> > > >           if (ret)
> > > >                   goto err_out_probe;
> > > >
> > > > +       if (rbd_dev->spec->snap_id != CEPH_NOSNAP &&
> > > > +           (rbd_dev->header.features & RBD_FEATURE_OBJECT_MAP)) {
> > > > +               ret = rbd_object_map_load(rbd_dev);
> > > > +               if (ret)
> > > > +                       goto err_out_probe;
> > > > +       }
> > > > +
> > > >           if (rbd_dev->header.features & RBD_FEATURE_LAYERING) {
> > > >                   ret = rbd_dev_v2_parent_info(rbd_dev);
> > > >                   if (ret)
> > > > diff --git a/drivers/block/rbd_types.h b/drivers/block/rbd_types.h
> > > > index 62ff50d3e7a6..ac98ab6ccd3b 100644
> > > > --- a/drivers/block/rbd_types.h
> > > > +++ b/drivers/block/rbd_types.h
> > > > @@ -18,6 +18,7 @@
> > > >    /* For format version 2, rbd image 'foo' consists of objects
> > > >     *   rbd_id.foo                - id of image
> > > >     *   rbd_header.<id>   - image metadata
> > > > + *   rbd_object_map.<id> - optional image object map
> > > >     *   rbd_data.<id>.0000000000000000
> > > >     *   rbd_data.<id>.0000000000000001
> > > >     *   ...               - data
> > > > @@ -25,6 +26,7 @@
> > > >     */
> > > >
> > > >    #define RBD_HEADER_PREFIX      "rbd_header."
> > > > +#define RBD_OBJECT_MAP_PREFIX  "rbd_object_map."
> > > >    #define RBD_ID_PREFIX          "rbd_id."
> > > >    #define RBD_V2_DATA_FORMAT     "%s.%016llx"
> > > >
> > > > @@ -39,6 +41,14 @@ enum rbd_notify_op {
> > > >           RBD_NOTIFY_OP_HEADER_UPDATE      = 3,
> > > >    };
> > > >
> > > > +#define OBJECT_NONEXISTENT     0
> > > > +#define OBJECT_EXISTS          1
> > > > +#define OBJECT_PENDING         2
> > > > +#define OBJECT_EXISTS_CLEAN    3
> > > > +
> > > > +#define RBD_FLAG_OBJECT_MAP_INVALID    (1ULL << 0)
> > > > +#define RBD_FLAG_FAST_DIFF_INVALID     (1ULL << 1)
> > > > +
> > > >    /*
> > > >     * For format version 1, rbd image 'foo' consists of objects
> > > >     *   foo.rbd           - image metadata
> > > > diff --git a/include/linux/ceph/cls_lock_client.h b/include/linux/ceph/cls_lock_client.h
> > > > index bea6c77d2093..17bc7584d1fe 100644
> > > > --- a/include/linux/ceph/cls_lock_client.h
> > > > +++ b/include/linux/ceph/cls_lock_client.h
> > > > @@ -52,4 +52,7 @@ int ceph_cls_lock_info(struct ceph_osd_client *osdc,
> > > >                          char *lock_name, u8 *type, char **tag,
> > > >                          struct ceph_locker **lockers, u32 *num_lockers);
> > > >
> > > > +int ceph_cls_assert_locked(struct ceph_osd_request *req, int which,
> > > > +                          char *lock_name, u8 type, char *cookie, char *tag);
> > > > +
> > > >    #endif
> > > > diff --git a/include/linux/ceph/striper.h b/include/linux/ceph/striper.h
> > > > index cbd0d24b7148..3486636c0e6e 100644
> > > > --- a/include/linux/ceph/striper.h
> > > > +++ b/include/linux/ceph/striper.h
> > > > @@ -66,4 +66,6 @@ int ceph_extent_to_file(struct ceph_file_layout *l,
> > > >                           struct ceph_file_extent **file_extents,
> > > >                           u32 *num_file_extents);
> > > >
> > > > +u64 ceph_get_num_objects(struct ceph_file_layout *l, u64 size);
> > > > +
> > > >    #endif
> > > > diff --git a/net/ceph/cls_lock_client.c b/net/ceph/cls_lock_client.c
> > > > index 56bbfe01e3ac..99cce6f3ec48 100644
> > > > --- a/net/ceph/cls_lock_client.c
> > > > +++ b/net/ceph/cls_lock_client.c
> > > > @@ -6,6 +6,7 @@
> > > >
> > > >    #include <linux/ceph/cls_lock_client.h>
> > > >    #include <linux/ceph/decode.h>
> > > > +#include <linux/ceph/libceph.h>
> > > >
> > > >    /**
> > > >     * ceph_cls_lock - grab rados lock for object
> > > > @@ -375,3 +376,47 @@ int ceph_cls_lock_info(struct ceph_osd_client *osdc,
> > > >           return ret;
> > > >    }
> > > >    EXPORT_SYMBOL(ceph_cls_lock_info);
> > > > +
> > > > +int ceph_cls_assert_locked(struct ceph_osd_request *req, int which,
> > > > +                          char *lock_name, u8 type, char *cookie, char *tag)
> > > > +{
> > > > +       int assert_op_buf_size;
> > > > +       int name_len = strlen(lock_name);
> > > > +       int cookie_len = strlen(cookie);
> > > > +       int tag_len = strlen(tag);
> > > > +       struct page **pages;
> > > > +       void *p, *end;
> > > > +       int ret;
> > > > +
> > > > +       assert_op_buf_size = name_len + sizeof(__le32) +
> > > > +                            cookie_len + sizeof(__le32) +
> > > > +                            tag_len + sizeof(__le32) +
> > > > +                            sizeof(u8) + CEPH_ENCODING_START_BLK_LEN;
> > > > +       if (assert_op_buf_size > PAGE_SIZE)
> > > > +               return -E2BIG;
> > > > +
> > > > +       ret = osd_req_op_cls_init(req, which, "lock", "assert_locked");
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       pages = ceph_alloc_page_vector(1, GFP_NOIO);
> > > > +       if (IS_ERR(pages))
> > > > +               return PTR_ERR(pages);
> > > > +
> > > > +       p = page_address(pages[0]);
> > > > +       end = p + assert_op_buf_size;
> > > > +
> > > > +       /* encode cls_lock_assert_op struct */
> > > > +       ceph_start_encoding(&p, 1, 1,
> > > > +                           assert_op_buf_size - CEPH_ENCODING_START_BLK_LEN);
> > > > +       ceph_encode_string(&p, end, lock_name, name_len);
> > > > +       ceph_encode_8(&p, type);
> > > > +       ceph_encode_string(&p, end, cookie, cookie_len);
> > > > +       ceph_encode_string(&p, end, tag, tag_len);
> > > > +       WARN_ON(p != end);
> > > > +
> > > > +       osd_req_op_cls_request_data_pages(req, which, pages, assert_op_buf_size,
> > > > +                                         0, false, true);
> > > > +       return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(ceph_cls_assert_locked);
> > > > diff --git a/net/ceph/striper.c b/net/ceph/striper.c
> > > > index c36462dc86b7..3b3fa75d1189 100644
> > > > --- a/net/ceph/striper.c
> > > > +++ b/net/ceph/striper.c
> > > > @@ -259,3 +259,20 @@ int ceph_extent_to_file(struct ceph_file_layout *l,
> > > >           return 0;
> > > >    }
> > > >    EXPORT_SYMBOL(ceph_extent_to_file);
> > > > +
> > > > +u64 ceph_get_num_objects(struct ceph_file_layout *l, u64 size)
> > > > +{
> > > > +       u64 period = (u64)l->stripe_count * l->object_size;
> > > > +       u64 num_periods = DIV64_U64_ROUND_UP(size, period);
> > > > +       u64 remainder_bytes;
> > > > +       u64 remainder_objs = 0;
> > > > +
> > > > +       div64_u64_rem(size, period, &remainder_bytes);
> > > > +       if (remainder_bytes > 0 &&
> > > > +           remainder_bytes < (u64)l->stripe_count * l->stripe_unit)
> > > > +               remainder_objs = l->stripe_count -
> > > > +                           DIV_ROUND_UP_ULL(remainder_bytes, l->stripe_unit);
> > > > +
> > > > +       return num_periods * l->stripe_count - remainder_objs;
> > > > +}
> > > > +EXPORT_SYMBOL(ceph_get_num_objects);
> > > > --
> > > > 2.19.2
> > > Nit: might have been nice to break this one commit into several smaller commits.