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:
> > Since infernalis, ceph.git commit 281f87f9ee52 ("cls_rbd: get_features
> > on snapshots returns HEAD image features"), querying and checking that
> > is pointless. Userspace support for manipulating image features after
> > image creation came also in infernalis, so a snapshot with a different
> > set of features wasn't ever possible.
> >
> > Signed-off-by: Ilya Dryomov <idryomov@xxxxxxxxx>
>
> Reviewed-by: Dongsheng Yang <dongsheng.yang@xxxxxxxxxxxx>
>
> Just one small nit below.
> > ---
> > drivers/block/rbd.c | 38 +-------------------------------------
> > 1 file changed, 1 insertion(+), 37 deletions(-)
> >
> > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> > index aba60e37b058..935b66808e40 100644
> > --- a/drivers/block/rbd.c
> > +++ b/drivers/block/rbd.c
> > @@ -377,7 +377,6 @@ struct rbd_client_id {
> >
> > struct rbd_mapping {
> > u64 size;
> > - u64 features;
> > };
> >
> > /*
> > @@ -644,8 +643,6 @@ static const char *rbd_dev_v2_snap_name(struct rbd_device *rbd_dev,
> > u64 snap_id);
> > static int _rbd_dev_v2_snap_size(struct rbd_device *rbd_dev, u64 snap_id,
> > u8 *order, u64 *snap_size);
> > -static int _rbd_dev_v2_snap_features(struct rbd_device *rbd_dev, u64 snap_id,
> > - u64 *snap_features);
> > static int rbd_dev_v2_get_flags(struct rbd_device *rbd_dev);
> >
> > static void rbd_obj_handle_request(struct rbd_obj_request *obj_req, int result);
> > @@ -1303,51 +1300,23 @@ static int rbd_snap_size(struct rbd_device *rbd_dev, u64 snap_id,
> > return 0;
> > }
> >
> > -static int rbd_snap_features(struct rbd_device *rbd_dev, u64 snap_id,
> > - u64 *snap_features)
> > -{
> > - rbd_assert(rbd_image_format_valid(rbd_dev->image_format));
> > - if (snap_id == CEPH_NOSNAP) {
> > - *snap_features = rbd_dev->header.features;
> > - } else if (rbd_dev->image_format == 1) {
> > - *snap_features = 0; /* No features for format 1 */
> > - } else {
> > - u64 features = 0;
> > - int ret;
> > -
> > - ret = _rbd_dev_v2_snap_features(rbd_dev, snap_id, &features);
>
> Just nit:
>
> _rbd_dev_v2_snap_features has only one caller now. we can implement it directly in rbd_dev_v2_features().
I kept both to minimize code churn and also because I actually expect
rbd_dev_v2_features() to be removed in the future. We need to get away
from using rbd_dev as a global variable (and thus functions that take
just rbd_dev and both read from and write to it).
Thanks,
Ilya
