| Message ID | 20191118133816.3963-9-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: > 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@gmail.com> Reviewed-by: Dongsheng Yang <dongsheng.yang@easystack.cn> 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(). Thanx > - if (ret) > - return ret; > - > - *snap_features = features; > - } > - return 0; > -} > - > static int rbd_dev_mapping_set(struct rbd_device *rbd_dev) > { > u64 snap_id = rbd_dev->spec->snap_id; > u64 size = 0; > - u64 features = 0; > int ret; > > ret = rbd_snap_size(rbd_dev, snap_id, &size); > - if (ret) > - return ret; > - ret = rbd_snap_features(rbd_dev, snap_id, &features); > if (ret) > return ret; > > rbd_dev->mapping.size = size; > - rbd_dev->mapping.features = features; > - > return 0; > } > > static void rbd_dev_mapping_clear(struct rbd_device *rbd_dev) > { > rbd_dev->mapping.size = 0; > - rbd_dev->mapping.features = 0; > } > > static void zero_bvec(struct bio_vec *bv) > @@ -5190,17 +5159,12 @@ static ssize_t rbd_size_show(struct device *dev, > (unsigned long long)rbd_dev->mapping.size); > } > > -/* > - * Note this shows the features for whatever's mapped, which is not > - * necessarily the base image. > - */ > static ssize_t rbd_features_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); > > - return sprintf(buf, "0x%016llx\n", > - (unsigned long long)rbd_dev->mapping.features); > + return sprintf(buf, "0x%016llx\n", rbd_dev->header.features); > } > > static ssize_t rbd_major_show(struct device *dev,
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: > > 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@gmail.com> > > Reviewed-by: Dongsheng Yang <dongsheng.yang@easystack.cn> > > 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
On 11/19/2019 07:55 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: >>> 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@gmail.com> >> Reviewed-by: Dongsheng Yang <dongsheng.yang@easystack.cn> >> >> 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). Okey, so there is a reason from future plan to keep it. No problem Thanx > > Thanks, > > Ilya >
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); - if (ret) - return ret; - - *snap_features = features; - } - return 0; -} - static int rbd_dev_mapping_set(struct rbd_device *rbd_dev) { u64 snap_id = rbd_dev->spec->snap_id; u64 size = 0; - u64 features = 0; int ret; ret = rbd_snap_size(rbd_dev, snap_id, &size); - if (ret) - return ret; - ret = rbd_snap_features(rbd_dev, snap_id, &features); if (ret) return ret; rbd_dev->mapping.size = size; - rbd_dev->mapping.features = features; - return 0; } static void rbd_dev_mapping_clear(struct rbd_device *rbd_dev) { rbd_dev->mapping.size = 0; - rbd_dev->mapping.features = 0; } static void zero_bvec(struct bio_vec *bv) @@ -5190,17 +5159,12 @@ static ssize_t rbd_size_show(struct device *dev, (unsigned long long)rbd_dev->mapping.size); } -/* - * Note this shows the features for whatever's mapped, which is not - * necessarily the base image. - */ static ssize_t rbd_features_show(struct device *dev, struct device_attribute *attr, char *buf) { struct rbd_device *rbd_dev = dev_to_rbd_dev(dev); - return sprintf(buf, "0x%016llx\n", - (unsigned long long)rbd_dev->mapping.features); + return sprintf(buf, "0x%016llx\n", rbd_dev->header.features); } static ssize_t rbd_major_show(struct device *dev,
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@gmail.com> --- drivers/block/rbd.c | 38 +------------------------------------- 1 file changed, 1 insertion(+), 37 deletions(-)