diff mbox series

[v3,3/4] ceph: switch to WARN_ON and bubble up errnos to the callers

Message ID 1592832300-29109-4-git-send-email-xiubli@redhat.com (mailing list archive)
State New, archived
Headers show
Series ceph: periodically send perf metrics to ceph | expand

Commit Message

Xiubo Li June 22, 2020, 1:24 p.m. UTC 
From: Xiubo Li <xiubli@redhat.com>

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

Comments

Jeff Layton June 23, 2020, 6:02 p.m. UTC | #1 
On Mon, 2020-06-22 at 09:24 -0400, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/mds_client.c | 46 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index f996363..f29cb11 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -1168,7 +1168,7 @@ static struct ceph_msg *create_session_msg(u32 op, u64 seq)
>  
>  static const unsigned char feature_bits[] = CEPHFS_FEATURES_CLIENT_SUPPORTED;
>  #define FEATURE_BYTES(c) (DIV_ROUND_UP((size_t)feature_bits[c - 1] + 1, 64) * 8)
> -static void encode_supported_features(void **p, void *end)
> +static int encode_supported_features(void **p, void *end)
>  {
>  	static const size_t count = ARRAY_SIZE(feature_bits);
>  
> @@ -1176,16 +1176,22 @@ static void encode_supported_features(void **p, void *end)
>  		size_t i;
>  		size_t size = FEATURE_BYTES(count);
>  
> -		BUG_ON(*p + 4 + size > end);
> +		if (WARN_ON(*p + 4 + size > end))
> +			return -ERANGE;
> +

Nice cleanup.

Let's use WARN_ON_ONCE instead?

It's better not to spam the logs if this is happening all over the
place. Also, I'm not sure that ERANGE is the right error here, but I
can't think of anything better. At least it should be distinctive.

>  		ceph_encode_32(p, size);
>  		memset(*p, 0, size);
>  		for (i = 0; i < count; i++)
>  			((unsigned char*)(*p))[i / 8] |= BIT(feature_bits[i] % 8);
>  		*p += size;
>  	} else {
> -		BUG_ON(*p + 4 > end);
> +		if (WARN_ON(*p + 4 > end))
> +			return -ERANGE;
> +
>  		ceph_encode_32(p, 0);
>  	}
> +
> +	return 0;
>  }
>  
>  /*
> @@ -1203,6 +1209,7 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>  	struct ceph_mount_options *fsopt = mdsc->fsc->mount_options;
>  	size_t size, count;
>  	void *p, *end;
> +	int ret;
>  
>  	const char* metadata[][2] = {
>  		{"hostname", mdsc->nodename},
> @@ -1232,7 +1239,7 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>  			   GFP_NOFS, false);
>  	if (!msg) {
>  		pr_err("create_session_msg ENOMEM creating msg\n");
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  	}
>  	p = msg->front.iov_base;
>  	end = p + msg->front.iov_len;
> @@ -1269,7 +1276,13 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>  		p += val_len;
>  	}
>  
> -	encode_supported_features(&p, end);
> +	ret = encode_supported_features(&p, end);
> +	if (ret) {
> +		pr_err("encode_supported_features failed!\n");
> +		ceph_msg_put(msg);
> +		return ERR_PTR(ret);
> +	}
> +
>  	msg->front.iov_len = p - msg->front.iov_base;
>  	msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
>  
> @@ -1297,8 +1310,8 @@ static int __open_session(struct ceph_mds_client *mdsc,
>  
>  	/* send connect message */
>  	msg = create_session_open_msg(mdsc, session->s_seq);
> -	if (!msg)
> -		return -ENOMEM;
> +	if (IS_ERR(msg))
> +		return PTR_ERR(msg);
>  	ceph_con_send(&session->s_con, msg);
>  	return 0;
>  }
> @@ -1312,6 +1325,7 @@ static int __open_session(struct ceph_mds_client *mdsc,
>  __open_export_target_session(struct ceph_mds_client *mdsc, int target)
>  {
>  	struct ceph_mds_session *session;
> +	int ret;
>  
>  	session = __ceph_lookup_mds_session(mdsc, target);
>  	if (!session) {
> @@ -1320,8 +1334,11 @@ static int __open_session(struct ceph_mds_client *mdsc,
>  			return session;
>  	}
>  	if (session->s_state == CEPH_MDS_SESSION_NEW ||
> -	    session->s_state == CEPH_MDS_SESSION_CLOSING)
> -		__open_session(mdsc, session);
> +	    session->s_state == CEPH_MDS_SESSION_CLOSING) {
> +		ret = __open_session(mdsc, session);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
>  
>  	return session;
>  }
> @@ -2520,7 +2537,12 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc,
>  		ceph_encode_copy(&p, &ts, sizeof(ts));
>  	}
>  
> -	BUG_ON(p > end);
> +	if (WARN_ON(p > end)) {
> +		ceph_msg_put(msg);
> +		msg = ERR_PTR(-ERANGE);
> +		goto out_free2;
> +	}
> +
>  	msg->front.iov_len = p - msg->front.iov_base;
>  	msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
>  
> @@ -2756,7 +2778,9 @@ static void __do_request(struct ceph_mds_client *mdsc,
>  		}
>  		if (session->s_state == CEPH_MDS_SESSION_NEW ||
>  		    session->s_state == CEPH_MDS_SESSION_CLOSING) {
> -			__open_session(mdsc, session);
> +			err = __open_session(mdsc, session);
> +			if (err)
> +				goto out_session;
>  			/* retry the same mds later */
>  			if (random)
>  				req->r_resend_mds = mds;
Xiubo Li June 24, 2020, 12:34 a.m. UTC | #2 
On 2020/6/24 2:02, Jeff Layton wrote:
> On Mon, 2020-06-22 at 09:24 -0400, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>   fs/ceph/mds_client.c | 46 +++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
>> index f996363..f29cb11 100644
>> --- a/fs/ceph/mds_client.c
>> +++ b/fs/ceph/mds_client.c
>> @@ -1168,7 +1168,7 @@ static struct ceph_msg *create_session_msg(u32 op, u64 seq)
>>   
>>   static const unsigned char feature_bits[] = CEPHFS_FEATURES_CLIENT_SUPPORTED;
>>   #define FEATURE_BYTES(c) (DIV_ROUND_UP((size_t)feature_bits[c - 1] + 1, 64) * 8)
>> -static void encode_supported_features(void **p, void *end)
>> +static int encode_supported_features(void **p, void *end)
>>   {
>>   	static const size_t count = ARRAY_SIZE(feature_bits);
>>   
>> @@ -1176,16 +1176,22 @@ static void encode_supported_features(void **p, void *end)
>>   		size_t i;
>>   		size_t size = FEATURE_BYTES(count);
>>   
>> -		BUG_ON(*p + 4 + size > end);
>> +		if (WARN_ON(*p + 4 + size > end))
>> +			return -ERANGE;
>> +
> Nice cleanup.
>
> Let's use WARN_ON_ONCE instead?
>
> It's better not to spam the logs if this is happening all over the
> place. Also, I'm not sure that ERANGE is the right error here, but I
> can't think of anything better. At least it should be distinctive.

Yeah, it  makes sense and I will fix it.

Thanks.


>
>>   		ceph_encode_32(p, size);
>>   		memset(*p, 0, size);
>>   		for (i = 0; i < count; i++)
>>   			((unsigned char*)(*p))[i / 8] |= BIT(feature_bits[i] % 8);
>>   		*p += size;
>>   	} else {
>> -		BUG_ON(*p + 4 > end);
>> +		if (WARN_ON(*p + 4 > end))
>> +			return -ERANGE;
>> +
>>   		ceph_encode_32(p, 0);
>>   	}
>> +
>> +	return 0;
>>   }
>>   
>>   /*
>> @@ -1203,6 +1209,7 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>>   	struct ceph_mount_options *fsopt = mdsc->fsc->mount_options;
>>   	size_t size, count;
>>   	void *p, *end;
>> +	int ret;
>>   
>>   	const char* metadata[][2] = {
>>   		{"hostname", mdsc->nodename},
>> @@ -1232,7 +1239,7 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>>   			   GFP_NOFS, false);
>>   	if (!msg) {
>>   		pr_err("create_session_msg ENOMEM creating msg\n");
>> -		return NULL;
>> +		return ERR_PTR(-ENOMEM);
>>   	}
>>   	p = msg->front.iov_base;
>>   	end = p + msg->front.iov_len;
>> @@ -1269,7 +1276,13 @@ static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
>>   		p += val_len;
>>   	}
>>   
>> -	encode_supported_features(&p, end);
>> +	ret = encode_supported_features(&p, end);
>> +	if (ret) {
>> +		pr_err("encode_supported_features failed!\n");
>> +		ceph_msg_put(msg);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>>   	msg->front.iov_len = p - msg->front.iov_base;
>>   	msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
>>   
>> @@ -1297,8 +1310,8 @@ static int __open_session(struct ceph_mds_client *mdsc,
>>   
>>   	/* send connect message */
>>   	msg = create_session_open_msg(mdsc, session->s_seq);
>> -	if (!msg)
>> -		return -ENOMEM;
>> +	if (IS_ERR(msg))
>> +		return PTR_ERR(msg);
>>   	ceph_con_send(&session->s_con, msg);
>>   	return 0;
>>   }
>> @@ -1312,6 +1325,7 @@ static int __open_session(struct ceph_mds_client *mdsc,
>>   __open_export_target_session(struct ceph_mds_client *mdsc, int target)
>>   {
>>   	struct ceph_mds_session *session;
>> +	int ret;
>>   
>>   	session = __ceph_lookup_mds_session(mdsc, target);
>>   	if (!session) {
>> @@ -1320,8 +1334,11 @@ static int __open_session(struct ceph_mds_client *mdsc,
>>   			return session;
>>   	}
>>   	if (session->s_state == CEPH_MDS_SESSION_NEW ||
>> -	    session->s_state == CEPH_MDS_SESSION_CLOSING)
>> -		__open_session(mdsc, session);
>> +	    session->s_state == CEPH_MDS_SESSION_CLOSING) {
>> +		ret = __open_session(mdsc, session);
>> +		if (ret)
>> +			return ERR_PTR(ret);
>> +	}
>>   
>>   	return session;
>>   }
>> @@ -2520,7 +2537,12 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc,
>>   		ceph_encode_copy(&p, &ts, sizeof(ts));
>>   	}
>>   
>> -	BUG_ON(p > end);
>> +	if (WARN_ON(p > end)) {
>> +		ceph_msg_put(msg);
>> +		msg = ERR_PTR(-ERANGE);
>> +		goto out_free2;
>> +	}
>> +
>>   	msg->front.iov_len = p - msg->front.iov_base;
>>   	msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
>>   
>> @@ -2756,7 +2778,9 @@ static void __do_request(struct ceph_mds_client *mdsc,
>>   		}
>>   		if (session->s_state == CEPH_MDS_SESSION_NEW ||
>>   		    session->s_state == CEPH_MDS_SESSION_CLOSING) {
>> -			__open_session(mdsc, session);
>> +			err = __open_session(mdsc, session);
>> +			if (err)
>> +				goto out_session;
>>   			/* retry the same mds later */
>>   			if (random)
>>   				req->r_resend_mds = mds;
diff mbox series

Patch

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index f996363..f29cb11 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -1168,7 +1168,7 @@  static struct ceph_msg *create_session_msg(u32 op, u64 seq)
 
 static const unsigned char feature_bits[] = CEPHFS_FEATURES_CLIENT_SUPPORTED;
 #define FEATURE_BYTES(c) (DIV_ROUND_UP((size_t)feature_bits[c - 1] + 1, 64) * 8)
-static void encode_supported_features(void **p, void *end)
+static int encode_supported_features(void **p, void *end)
 {
 	static const size_t count = ARRAY_SIZE(feature_bits);
 
@@ -1176,16 +1176,22 @@  static void encode_supported_features(void **p, void *end)
 		size_t i;
 		size_t size = FEATURE_BYTES(count);
 
-		BUG_ON(*p + 4 + size > end);
+		if (WARN_ON(*p + 4 + size > end))
+			return -ERANGE;
+
 		ceph_encode_32(p, size);
 		memset(*p, 0, size);
 		for (i = 0; i < count; i++)
 			((unsigned char*)(*p))[i / 8] |= BIT(feature_bits[i] % 8);
 		*p += size;
 	} else {
-		BUG_ON(*p + 4 > end);
+		if (WARN_ON(*p + 4 > end))
+			return -ERANGE;
+
 		ceph_encode_32(p, 0);
 	}
+
+	return 0;
 }
 
 /*
@@ -1203,6 +1209,7 @@  static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
 	struct ceph_mount_options *fsopt = mdsc->fsc->mount_options;
 	size_t size, count;
 	void *p, *end;
+	int ret;
 
 	const char* metadata[][2] = {
 		{"hostname", mdsc->nodename},
@@ -1232,7 +1239,7 @@  static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
 			   GFP_NOFS, false);
 	if (!msg) {
 		pr_err("create_session_msg ENOMEM creating msg\n");
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 	}
 	p = msg->front.iov_base;
 	end = p + msg->front.iov_len;
@@ -1269,7 +1276,13 @@  static struct ceph_msg *create_session_open_msg(struct ceph_mds_client *mdsc, u6
 		p += val_len;
 	}
 
-	encode_supported_features(&p, end);
+	ret = encode_supported_features(&p, end);
+	if (ret) {
+		pr_err("encode_supported_features failed!\n");
+		ceph_msg_put(msg);
+		return ERR_PTR(ret);
+	}
+
 	msg->front.iov_len = p - msg->front.iov_base;
 	msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
 
@@ -1297,8 +1310,8 @@  static int __open_session(struct ceph_mds_client *mdsc,
 
 	/* send connect message */
 	msg = create_session_open_msg(mdsc, session->s_seq);
-	if (!msg)
-		return -ENOMEM;
+	if (IS_ERR(msg))
+		return PTR_ERR(msg);
 	ceph_con_send(&session->s_con, msg);
 	return 0;
 }
@@ -1312,6 +1325,7 @@  static int __open_session(struct ceph_mds_client *mdsc,
 __open_export_target_session(struct ceph_mds_client *mdsc, int target)
 {
 	struct ceph_mds_session *session;
+	int ret;
 
 	session = __ceph_lookup_mds_session(mdsc, target);
 	if (!session) {
@@ -1320,8 +1334,11 @@  static int __open_session(struct ceph_mds_client *mdsc,
 			return session;
 	}
 	if (session->s_state == CEPH_MDS_SESSION_NEW ||
-	    session->s_state == CEPH_MDS_SESSION_CLOSING)
-		__open_session(mdsc, session);
+	    session->s_state == CEPH_MDS_SESSION_CLOSING) {
+		ret = __open_session(mdsc, session);
+		if (ret)
+			return ERR_PTR(ret);
+	}
 
 	return session;
 }
@@ -2520,7 +2537,12 @@  static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc,
 		ceph_encode_copy(&p, &ts, sizeof(ts));
 	}
 
-	BUG_ON(p > end);
+	if (WARN_ON(p > end)) {
+		ceph_msg_put(msg);
+		msg = ERR_PTR(-ERANGE);
+		goto out_free2;
+	}
+
 	msg->front.iov_len = p - msg->front.iov_base;
 	msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
 
@@ -2756,7 +2778,9 @@  static void __do_request(struct ceph_mds_client *mdsc,
 		}
 		if (session->s_state == CEPH_MDS_SESSION_NEW ||
 		    session->s_state == CEPH_MDS_SESSION_CLOSING) {
-			__open_session(mdsc, session);
+			err = __open_session(mdsc, session);
+			if (err)
+				goto out_session;
 			/* retry the same mds later */
 			if (random)
 				req->r_resend_mds = mds;