ceph: allocate the accurate extra bytes for the session features
diff mbox series

Message ID 20200103025950.27659-1-xiubli@redhat.com
State New
Headers show
Series
  • ceph: allocate the accurate extra bytes for the session features
Related show

Commit Message

Xiubo Li Jan. 3, 2020, 2:59 a.m. UTC
From: Xiubo Li <xiubli@redhat.com>

The totally bytes maybe potentially larger than 8.

Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/mds_client.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Jeff Layton Jan. 6, 2020, 3:23 p.m. UTC | #1
On Thu, 2020-01-02 at 21:59 -0500, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> The totally bytes maybe potentially larger than 8.
> 
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/mds_client.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index ad9573b7e115..aa49e8557599 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -1067,20 +1067,20 @@ static struct ceph_msg *create_session_msg(u32 op, u64 seq)
>  	return msg;
>  }
>  
> +static const unsigned char feature_bits[] = CEPHFS_FEATURES_CLIENT_SUPPORTED;
>  static void encode_supported_features(void **p, void *end)
>  {
> -	static const unsigned char bits[] = CEPHFS_FEATURES_CLIENT_SUPPORTED;
> -	static const size_t count = ARRAY_SIZE(bits);
> +	static const size_t count = ARRAY_SIZE(feature_bits);
>  
>  	if (count > 0) {
>  		size_t i;
> -		size_t size = ((size_t)bits[count - 1] + 64) / 64 * 8;
> +		size_t size = ((size_t)feature_bits[count - 1] + 64) / 64 * 8;

The bug looks real, though it's not really a problem until we get to
flag 65. Note too that this calculation relies on having the highest
feature bit value as the last element of the array. That's probably
worth a comment in mds_client.h so we don't screw that up later.

Also, I think this would be better expressed using DIV_ROUND_UP, and
since we have this calculation in two places, maybe do something like:

    #define FEATURE_WORDS  (DIV_ROUND_UP(feature_bits[count - 1], 64) * 8)

...and then plug that macro into both places.

>  		BUG_ON(*p + 4 + size > end);
>  		ceph_encode_32(p, size);
>  		memset(*p, 0, size);
>  		for (i = 0; i < count; i++)
> -			((unsigned char*)(*p))[i / 8] |= 1 << (bits[i] % 8);
> +			((unsigned char*)(*p))[i / 8] |= 1 << (feature_bits[i] % 8);
>  		*p += size;
>  	} else {
>  		BUG_ON(*p + 4 > end);
> @@ -1101,6 +1101,7 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>  	int metadata_key_count = 0;
>  	struct ceph_options *opt = mdsc->fsc->client->options;
>  	struct ceph_mount_options *fsopt = mdsc->fsc->mount_options;
> +	size_t size, count;
>  	void *p, *end;
>  
>  	const char* metadata[][2] = {
> @@ -1118,8 +1119,13 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>  			strlen(metadata[i][1]);
>  		metadata_key_count++;
>  	}
> +
>  	/* supported feature */
> -	extra_bytes += 4 + 8;
> +	size = 0;
> +	count = ARRAY_SIZE(feature_bits);
> +	if (count > 0)
> +		size = ((size_t)feature_bits[count - 1] + 64) / 64 * 8;
> +	extra_bytes += 4 + size;
>  
>  	/* Allocate the message */
>  	msg = ceph_msg_new(CEPH_MSG_CLIENT_SESSION, sizeof(*h) + extra_bytes,
> @@ -1139,7 +1145,7 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>  	 * Serialize client metadata into waiting buffer space, using
>  	 * the format that userspace expects for map<string, string>
>  	 *
> -	 * ClientSession messages with metadata are v2
> +	 * ClientSession messages with metadata are v3
>  	 */
>  	msg->hdr.version = cpu_to_le16(3);
>  	msg->hdr.compat_version = cpu_to_le16(1);
Xiubo Li Jan. 7, 2020, 2:49 a.m. UTC | #2
On 2020/1/6 23:23, Jeff Layton wrote:
> On Thu, 2020-01-02 at 21:59 -0500, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> The totally bytes maybe potentially larger than 8.
>>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/mds_client.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>> index ad9573b7e115..aa49e8557599 100644
>> --- a/fs/ceph/mds_client.c
>> +++ b/fs/ceph/mds_client.c
>> @@ -1067,20 +1067,20 @@ static struct ceph_msg *create_session_msg(u32 op, u64 seq)
>>   	return msg;
>>   }
>>   
>> +static const unsigned char feature_bits[] = CEPHFS_FEATURES_CLIENT_SUPPORTED;
>>   static void encode_supported_features(void **p, void *end)
>>   {
>> -	static const unsigned char bits[] = CEPHFS_FEATURES_CLIENT_SUPPORTED;
>> -	static const size_t count = ARRAY_SIZE(bits);
>> +	static const size_t count = ARRAY_SIZE(feature_bits);
>>   
>>   	if (count > 0) {
>>   		size_t i;
>> -		size_t size = ((size_t)bits[count - 1] + 64) / 64 * 8;
>> +		size_t size = ((size_t)feature_bits[count - 1] + 64) / 64 * 8;
> The bug looks real, though it's not really a problem until we get to
> flag 65. Note too that this calculation relies on having the highest
> feature bit value as the last element of the array. That's probably
> worth a comment in mds_client.h so we don't screw that up later.

Yeah, this makes sense.


>
> Also, I think this would be better expressed using DIV_ROUND_UP, and
> since we have this calculation in two places, maybe do something like:
>
>      #define FEATURE_WORDS  (DIV_ROUND_UP(feature_bits[count - 1], 64) * 8)
>
> ...and then plug that macro into both places.

Will address it.

Thanks.


>>   		BUG_ON(*p + 4 + size > end);
>>   		ceph_encode_32(p, size);
>>   		memset(*p, 0, size);
>>   		for (i = 0; i < count; i++)
>> -			((unsigned char*)(*p))[i / 8] |= 1 << (bits[i] % 8);
>> +			((unsigned char*)(*p))[i / 8] |= 1 << (feature_bits[i] % 8);
>>   		*p += size;
>>   	} else {
>>   		BUG_ON(*p + 4 > end);
>> @@ -1101,6 +1101,7 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>>   	int metadata_key_count = 0;
>>   	struct ceph_options *opt = mdsc->fsc->client->options;
>>   	struct ceph_mount_options *fsopt = mdsc->fsc->mount_options;
>> +	size_t size, count;
>>   	void *p, *end;
>>   
>>   	const char* metadata[][2] = {
>> @@ -1118,8 +1119,13 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>>   			strlen(metadata[i][1]);
>>   		metadata_key_count++;
>>   	}
>> +
>>   	/* supported feature */
>> -	extra_bytes += 4 + 8;
>> +	size = 0;
>> +	count = ARRAY_SIZE(feature_bits);
>> +	if (count > 0)
>> +		size = ((size_t)feature_bits[count - 1] + 64) / 64 * 8;
>> +	extra_bytes += 4 + size;
>>   
>>   	/* Allocate the message */
>>   	msg = ceph_msg_new(CEPH_MSG_CLIENT_SESSION, sizeof(*h) + extra_bytes,
>> @@ -1139,7 +1145,7 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>>   	 * Serialize client metadata into waiting buffer space, using
>>   	 * the format that userspace expects for map<string, string>
>>   	 *
>> -	 * ClientSession messages with metadata are v2
>> +	 * ClientSession messages with metadata are v3
>>   	 */
>>   	msg->hdr.version = cpu_to_le16(3);
>>   	msg->hdr.compat_version = cpu_to_le16(1);
Xiubo Li Jan. 8, 2020, 4:02 a.m. UTC | #3
On 2020/1/6 23:23, Jeff Layton wrote:
> On Thu, 2020-01-02 at 21:59 -0500, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> The totally bytes maybe potentially larger than 8.
>>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/mds_client.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>> index ad9573b7e115..aa49e8557599 100644
>> --- a/fs/ceph/mds_client.c
>> +++ b/fs/ceph/mds_client.c
>> @@ -1067,20 +1067,20 @@ static struct ceph_msg *create_session_msg(u32 op, u64 seq)
>>   	return msg;
>>   }
>>   
>> +static const unsigned char feature_bits[] = CEPHFS_FEATURES_CLIENT_SUPPORTED;
>>   static void encode_supported_features(void **p, void *end)
>>   {
>> -	static const unsigned char bits[] = CEPHFS_FEATURES_CLIENT_SUPPORTED;
>> -	static const size_t count = ARRAY_SIZE(bits);
>> +	static const size_t count = ARRAY_SIZE(feature_bits);
>>   
>>   	if (count > 0) {
>>   		size_t i;
>> -		size_t size = ((size_t)bits[count - 1] + 64) / 64 * 8;
>> +		size_t size = ((size_t)feature_bits[count - 1] + 64) / 64 * 8;
> The bug looks real, though it's not really a problem until we get to
> flag 65.

When the flag bit >= 64, it will be a problem.

flag bit in [0, 63] we need only one word size.

>   Note too that this calculation relies on having the highest
> feature bit value as the last element of the array. That's probably
> worth a comment in mds_client.h so we don't screw that up later.
>
> Also, I think this would be better expressed using DIV_ROUND_UP, and
> since we have this calculation in two places, maybe do something like:
>
>      #define FEATURE_WORDS  (DIV_ROUND_UP(feature_bits[count - 1], 64) * 8)

Here it should be (DIV_ROUND_UP(feature_bits[count - 1] + 1, 64) * 8).


>
> ...and then plug that macro into both places.
>
>>   		BUG_ON(*p + 4 + size > end);
>>   		ceph_encode_32(p, size);
>>   		memset(*p, 0, size);
>>   		for (i = 0; i < count; i++)
>> -			((unsigned char*)(*p))[i / 8] |= 1 << (bits[i] % 8);
>> +			((unsigned char*)(*p))[i / 8] |= 1 << (feature_bits[i] % 8);
>>   		*p += size;
>>   	} else {
>>   		BUG_ON(*p + 4 > end);
>> @@ -1101,6 +1101,7 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>>   	int metadata_key_count = 0;
>>   	struct ceph_options *opt = mdsc->fsc->client->options;
>>   	struct ceph_mount_options *fsopt = mdsc->fsc->mount_options;
>> +	size_t size, count;
>>   	void *p, *end;
>>   
>>   	const char* metadata[][2] = {
>> @@ -1118,8 +1119,13 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>>   			strlen(metadata[i][1]);
>>   		metadata_key_count++;
>>   	}
>> +
>>   	/* supported feature */
>> -	extra_bytes += 4 + 8;
>> +	size = 0;
>> +	count = ARRAY_SIZE(feature_bits);
>> +	if (count > 0)
>> +		size = ((size_t)feature_bits[count - 1] + 64) / 64 * 8;
>> +	extra_bytes += 4 + size;
>>   
>>   	/* Allocate the message */
>>   	msg = ceph_msg_new(CEPH_MSG_CLIENT_SESSION, sizeof(*h) + extra_bytes,
>> @@ -1139,7 +1145,7 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>>   	 * Serialize client metadata into waiting buffer space, using
>>   	 * the format that userspace expects for map<string, string>
>>   	 *
>> -	 * ClientSession messages with metadata are v2
>> +	 * ClientSession messages with metadata are v3
>>   	 */
>>   	msg->hdr.version = cpu_to_le16(3);
>>   	msg->hdr.compat_version = cpu_to_le16(1);

Patch
diff mbox series

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index ad9573b7e115..aa49e8557599 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -1067,20 +1067,20 @@  static struct ceph_msg *create_session_msg(u32 op, u64 seq)
 	return msg;
 }
 
+static const unsigned char feature_bits[] = CEPHFS_FEATURES_CLIENT_SUPPORTED;
 static void encode_supported_features(void **p, void *end)
 {
-	static const unsigned char bits[] = CEPHFS_FEATURES_CLIENT_SUPPORTED;
-	static const size_t count = ARRAY_SIZE(bits);
+	static const size_t count = ARRAY_SIZE(feature_bits);
 
 	if (count > 0) {
 		size_t i;
-		size_t size = ((size_t)bits[count - 1] + 64) / 64 * 8;
+		size_t size = ((size_t)feature_bits[count - 1] + 64) / 64 * 8;
 
 		BUG_ON(*p + 4 + size > end);
 		ceph_encode_32(p, size);
 		memset(*p, 0, size);
 		for (i = 0; i < count; i++)
-			((unsigned char*)(*p))[i / 8] |= 1 << (bits[i] % 8);
+			((unsigned char*)(*p))[i / 8] |= 1 << (feature_bits[i] % 8);
 		*p += size;
 	} else {
 		BUG_ON(*p + 4 > end);
@@ -1101,6 +1101,7 @@  static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
 	int metadata_key_count = 0;
 	struct ceph_options *opt = mdsc->fsc->client->options;
 	struct ceph_mount_options *fsopt = mdsc->fsc->mount_options;
+	size_t size, count;
 	void *p, *end;
 
 	const char* metadata[][2] = {
@@ -1118,8 +1119,13 @@  static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
 			strlen(metadata[i][1]);
 		metadata_key_count++;
 	}
+
 	/* supported feature */
-	extra_bytes += 4 + 8;
+	size = 0;
+	count = ARRAY_SIZE(feature_bits);
+	if (count > 0)
+		size = ((size_t)feature_bits[count - 1] + 64) / 64 * 8;
+	extra_bytes += 4 + size;
 
 	/* Allocate the message */
 	msg = ceph_msg_new(CEPH_MSG_CLIENT_SESSION, sizeof(*h) + extra_bytes,
@@ -1139,7 +1145,7 @@  static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
 	 * Serialize client metadata into waiting buffer space, using
 	 * the format that userspace expects for map<string, string>
 	 *
-	 * ClientSession messages with metadata are v2
+	 * ClientSession messages with metadata are v3
 	 */
 	msg->hdr.version = cpu_to_le16(3);
 	msg->hdr.compat_version = cpu_to_le16(1);