[8/9] rbd: don't query snapshot features
diff mbox series

Message ID 20191118133816.3963-9-idryomov@gmail.com
State New
Headers show
Series
  • wip-krbd-readonly
Related show

Commit Message

Ilya Dryomov Nov. 18, 2019, 1:38 p.m. UTC
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(-)

Comments

Dongsheng Yang Nov. 19, 2019, 8:38 a.m. UTC | #1
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,
Ilya Dryomov Nov. 19, 2019, 11:55 a.m. UTC | #2
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
Dongsheng Yang Nov. 19, 2019, 12:08 p.m. UTC | #3
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
>

Patch
diff mbox series

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,