diff mbox series

[1/4] libceph: support op append

Message ID 1534399172-27610-2-git-send-email-dongsheng.yang@easystack.cn (mailing list archive)
State New, archived
Headers show
Series [1/4] libceph: support op append | expand

Commit Message

Dongsheng Yang Aug. 16, 2018, 5:59 a.m. UTC 
we need to send append operation when we want to support journaling in kernel client.

Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
---
 net/ceph/osd_client.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Alex Elder Aug. 16, 2018, 12:49 p.m. UTC | #1 
On 08/16/2018 12:59 AM, Dongsheng Yang wrote:
> we need to send append operation when we want to support journaling in kernel client.
> 
> Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>

A superficial comment is that you should avoid using long lines.
I'll point out one or two below.  I also offer a few other comments.

					-Alex

> ---
>  net/ceph/osd_client.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
> index 34b5334..851ff9c 100644
> --- a/net/ceph/osd_client.c
> +++ b/net/ceph/osd_client.c
> @@ -378,6 +378,7 @@ static void osd_req_op_data_release(struct ceph_osd_request *osd_req,
>  	case CEPH_OSD_OP_READ:
>  	case CEPH_OSD_OP_WRITE:
>  	case CEPH_OSD_OP_WRITEFULL:
> +	case CEPH_OSD_OP_APPEND:
>  		ceph_osd_data_release(&op->extent.osd_data);
>  		break;
>  	case CEPH_OSD_OP_CALL:
> @@ -712,13 +713,13 @@ void osd_req_op_extent_init(struct ceph_osd_request *osd_req,
>  
>  	BUG_ON(opcode != CEPH_OSD_OP_READ && opcode != CEPH_OSD_OP_WRITE &&
>  	       opcode != CEPH_OSD_OP_WRITEFULL && opcode != CEPH_OSD_OP_ZERO &&
> -	       opcode != CEPH_OSD_OP_TRUNCATE);
> +	       opcode != CEPH_OSD_OP_TRUNCATE && opcode != CEPH_OSD_OP_APPEND);

This is good.

>  
>  	op->extent.offset = offset;
>  	op->extent.length = length;
>  	op->extent.truncate_size = truncate_size;
>  	op->extent.truncate_seq = truncate_seq;
> -	if (opcode == CEPH_OSD_OP_WRITE || opcode == CEPH_OSD_OP_WRITEFULL)
> +	if (opcode == CEPH_OSD_OP_WRITE || opcode == CEPH_OSD_OP_WRITEFULL || opcode == CEPH_OSD_OP_APPEND)

This is not good.

Also, in this case since it makes such a simple check require a wrapped line,
you could create a little helper function to make the code more readable
(and it could be reused below).  (The following is probably *not* the right
name to use.)

static bool osd_req_write_data_op(u16 op)
{
	return opcode == CEPH_OSD_OP_WRITE ||
		opcode == CEPH_OSD_OP_WRITEFULL ||
		opcode == CEPH_OSD_OP_APPEND;
}


>  		payload_len += length;
>  
>  	op->indata_len = payload_len;
> @@ -740,7 +741,7 @@ void osd_req_op_extent_update(struct ceph_osd_request *osd_req,
>  	BUG_ON(length > previous);
>  
>  	op->extent.length = length;
> -	if (op->op == CEPH_OSD_OP_WRITE || op->op == CEPH_OSD_OP_WRITEFULL)
> +	if (op->op == CEPH_OSD_OP_WRITE || op->op == CEPH_OSD_OP_WRITEFULL || op->op == CEPH_OSD_OP_APPEND)

The above helper could be used here.

>  		op->indata_len -= previous - length;
>  }
>  EXPORT_SYMBOL(osd_req_op_extent_update);
> @@ -762,7 +763,7 @@ void osd_req_op_extent_dup_last(struct ceph_osd_request *osd_req,
>  	op->extent.offset += offset_inc;
>  	op->extent.length -= offset_inc;
>  
> -	if (op->op == CEPH_OSD_OP_WRITE || op->op == CEPH_OSD_OP_WRITEFULL)
> +	if (op->op == CEPH_OSD_OP_WRITE || op->op == CEPH_OSD_OP_WRITEFULL || op->op == CEPH_OSD_OP_APPEND)
>  		op->indata_len -= offset_inc;
>  }
>  EXPORT_SYMBOL(osd_req_op_extent_dup_last);
> @@ -913,6 +914,7 @@ static u32 osd_req_encode_op(struct ceph_osd_op *dst,
>  	case CEPH_OSD_OP_READ:
>  	case CEPH_OSD_OP_WRITE:
>  	case CEPH_OSD_OP_WRITEFULL:
> +	case CEPH_OSD_OP_APPEND:
>  	case CEPH_OSD_OP_ZERO:
>  	case CEPH_OSD_OP_TRUNCATE:
>  		dst->extent.offset = cpu_to_le64(src->extent.offset);
> @@ -998,7 +1000,7 @@ struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *osdc,
>  
>  	BUG_ON(opcode != CEPH_OSD_OP_READ && opcode != CEPH_OSD_OP_WRITE &&
>  	       opcode != CEPH_OSD_OP_ZERO && opcode != CEPH_OSD_OP_TRUNCATE &&
> -	       opcode != CEPH_OSD_OP_CREATE && opcode != CEPH_OSD_OP_DELETE);
> +	       opcode != CEPH_OSD_OP_CREATE && opcode != CEPH_OSD_OP_DELETE && opcode != CEPH_OSD_OP_APPEND);

Long line here too.

Also, Ilya I think some of these debug lines like this should probably start
to go away.  They're great for development but as things stabilize I think
they can be a bit excessive.  In this case it's a BUG_ON(); it could at least
be an assertion that could be compiled out for "production" builds.

>  	req = ceph_osdc_alloc_request(osdc, snapc, num_ops, use_mempool,
>  					GFP_NOFS);
> @@ -1872,6 +1874,7 @@ static void setup_request_data(struct ceph_osd_request *req,
>  		/* request */
>  		case CEPH_OSD_OP_WRITE:
>  		case CEPH_OSD_OP_WRITEFULL:
> +		case CEPH_OSD_OP_APPEND:
>  			WARN_ON(op->indata_len != op->extent.length);
>  			ceph_osdc_msg_data_add(msg, &op->extent.osd_data);
>  			break;
>
Dongsheng Yang Aug. 17, 2018, 7:42 a.m. UTC | #2 
On 08/16/2018 08:49 PM, Alex Elder wrote:
> On 08/16/2018 12:59 AM, Dongsheng Yang wrote:
>> we need to send append operation when we want to support journaling in kernel client.
>>
>> Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
> A superficial comment is that you should avoid using long lines.
> I'll point out one or two below.  I also offer a few other comments.
>
> 					-Alex

Hi Alex,
     Thanx for your comments, Yes, there are still some coding
style problems in this RFC patchset. Sorry for these, and
thanx a lot for your suggestion, that's great.
>
>> ---
>>   net/ceph/osd_client.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
>> index 34b5334..851ff9c 100644
>> --- a/net/ceph/osd_client.c
>> +++ b/net/ceph/osd_client.c
>> @@ -378,6 +378,7 @@ static void osd_req_op_data_release(struct ceph_osd_request *osd_req,
>>   	case CEPH_OSD_OP_READ:
>>   	case CEPH_OSD_OP_WRITE:
>>   	case CEPH_OSD_OP_WRITEFULL:
>> +	case CEPH_OSD_OP_APPEND:
>>   		ceph_osd_data_release(&op->extent.osd_data);
>>   		break;
>>   	case CEPH_OSD_OP_CALL:
>> @@ -712,13 +713,13 @@ void osd_req_op_extent_init(struct ceph_osd_request *osd_req,
>>   
>>   	BUG_ON(opcode != CEPH_OSD_OP_READ && opcode != CEPH_OSD_OP_WRITE &&
>>   	       opcode != CEPH_OSD_OP_WRITEFULL && opcode != CEPH_OSD_OP_ZERO &&
>> -	       opcode != CEPH_OSD_OP_TRUNCATE);
>> +	       opcode != CEPH_OSD_OP_TRUNCATE && opcode != CEPH_OSD_OP_APPEND);
> This is good.
>
>>   
>>   	op->extent.offset = offset;
>>   	op->extent.length = length;
>>   	op->extent.truncate_size = truncate_size;
>>   	op->extent.truncate_seq = truncate_seq;
>> -	if (opcode == CEPH_OSD_OP_WRITE || opcode == CEPH_OSD_OP_WRITEFULL)
>> +	if (opcode == CEPH_OSD_OP_WRITE || opcode == CEPH_OSD_OP_WRITEFULL || opcode == CEPH_OSD_OP_APPEND)
> This is not good.
>
> Also, in this case since it makes such a simple check require a wrapped line,
> you could create a little helper function to make the code more readable
> (and it could be reused below).  (The following is probably *not* the right
> name to use.)
>
> static bool osd_req_write_data_op(u16 op)
> {
> 	return opcode == CEPH_OSD_OP_WRITE ||
> 		opcode == CEPH_OSD_OP_WRITEFULL ||
> 		opcode == CEPH_OSD_OP_APPEND;
> }

Good idea.
>
>
>>   		payload_len += length;
>>   
>>   	op->indata_len = payload_len;
>> @@ -740,7 +741,7 @@ void osd_req_op_extent_update(struct ceph_osd_request *osd_req,
>>   	BUG_ON(length > previous);
>>   
>>   	op->extent.length = length;
>> -	if (op->op == CEPH_OSD_OP_WRITE || op->op == CEPH_OSD_OP_WRITEFULL)
>> +	if (op->op == CEPH_OSD_OP_WRITE || op->op == CEPH_OSD_OP_WRITEFULL || op->op == CEPH_OSD_OP_APPEND)
> The above helper could be used here.
>
>>   		op->indata_len -= previous - length;
>>   }
>>   EXPORT_SYMBOL(osd_req_op_extent_update);
>> @@ -762,7 +763,7 @@ void osd_req_op_extent_dup_last(struct ceph_osd_request *osd_req,
>>   	op->extent.offset += offset_inc;
>>   	op->extent.length -= offset_inc;
>>   
>> -	if (op->op == CEPH_OSD_OP_WRITE || op->op == CEPH_OSD_OP_WRITEFULL)
>> +	if (op->op == CEPH_OSD_OP_WRITE || op->op == CEPH_OSD_OP_WRITEFULL || op->op == CEPH_OSD_OP_APPEND)
>>   		op->indata_len -= offset_inc;
>>   }
>>   EXPORT_SYMBOL(osd_req_op_extent_dup_last);
>> @@ -913,6 +914,7 @@ static u32 osd_req_encode_op(struct ceph_osd_op *dst,
>>   	case CEPH_OSD_OP_READ:
>>   	case CEPH_OSD_OP_WRITE:
>>   	case CEPH_OSD_OP_WRITEFULL:
>> +	case CEPH_OSD_OP_APPEND:
>>   	case CEPH_OSD_OP_ZERO:
>>   	case CEPH_OSD_OP_TRUNCATE:
>>   		dst->extent.offset = cpu_to_le64(src->extent.offset);
>> @@ -998,7 +1000,7 @@ struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *osdc,
>>   
>>   	BUG_ON(opcode != CEPH_OSD_OP_READ && opcode != CEPH_OSD_OP_WRITE &&
>>   	       opcode != CEPH_OSD_OP_ZERO && opcode != CEPH_OSD_OP_TRUNCATE &&
>> -	       opcode != CEPH_OSD_OP_CREATE && opcode != CEPH_OSD_OP_DELETE);
>> +	       opcode != CEPH_OSD_OP_CREATE && opcode != CEPH_OSD_OP_DELETE && opcode != CEPH_OSD_OP_APPEND);
> Long line here too.

Yes, thanx
>
> Also, Ilya I think some of these debug lines like this should probably start
> to go away.  They're great for development but as things stabilize I think
> they can be a bit excessive.  In this case it's a BUG_ON(); it could at least
> be an assertion that could be compiled out for "production" builds.
>
>>   	req = ceph_osdc_alloc_request(osdc, snapc, num_ops, use_mempool,
>>   					GFP_NOFS);
>> @@ -1872,6 +1874,7 @@ static void setup_request_data(struct ceph_osd_request *req,
>>   		/* request */
>>   		case CEPH_OSD_OP_WRITE:
>>   		case CEPH_OSD_OP_WRITEFULL:
>> +		case CEPH_OSD_OP_APPEND:
>>   			WARN_ON(op->indata_len != op->extent.length);
>>   			ceph_osdc_msg_data_add(msg, &op->extent.osd_data);
>>   			break;
>>
>
diff mbox series

Patch

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 34b5334..851ff9c 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -378,6 +378,7 @@  static void osd_req_op_data_release(struct ceph_osd_request *osd_req,
 	case CEPH_OSD_OP_READ:
 	case CEPH_OSD_OP_WRITE:
 	case CEPH_OSD_OP_WRITEFULL:
+	case CEPH_OSD_OP_APPEND:
 		ceph_osd_data_release(&op->extent.osd_data);
 		break;
 	case CEPH_OSD_OP_CALL:
@@ -712,13 +713,13 @@  void osd_req_op_extent_init(struct ceph_osd_request *osd_req,
 
 	BUG_ON(opcode != CEPH_OSD_OP_READ && opcode != CEPH_OSD_OP_WRITE &&
 	       opcode != CEPH_OSD_OP_WRITEFULL && opcode != CEPH_OSD_OP_ZERO &&
-	       opcode != CEPH_OSD_OP_TRUNCATE);
+	       opcode != CEPH_OSD_OP_TRUNCATE && opcode != CEPH_OSD_OP_APPEND);
 
 	op->extent.offset = offset;
 	op->extent.length = length;
 	op->extent.truncate_size = truncate_size;
 	op->extent.truncate_seq = truncate_seq;
-	if (opcode == CEPH_OSD_OP_WRITE || opcode == CEPH_OSD_OP_WRITEFULL)
+	if (opcode == CEPH_OSD_OP_WRITE || opcode == CEPH_OSD_OP_WRITEFULL || opcode == CEPH_OSD_OP_APPEND)
 		payload_len += length;
 
 	op->indata_len = payload_len;
@@ -740,7 +741,7 @@  void osd_req_op_extent_update(struct ceph_osd_request *osd_req,
 	BUG_ON(length > previous);
 
 	op->extent.length = length;
-	if (op->op == CEPH_OSD_OP_WRITE || op->op == CEPH_OSD_OP_WRITEFULL)
+	if (op->op == CEPH_OSD_OP_WRITE || op->op == CEPH_OSD_OP_WRITEFULL || op->op == CEPH_OSD_OP_APPEND)
 		op->indata_len -= previous - length;
 }
 EXPORT_SYMBOL(osd_req_op_extent_update);
@@ -762,7 +763,7 @@  void osd_req_op_extent_dup_last(struct ceph_osd_request *osd_req,
 	op->extent.offset += offset_inc;
 	op->extent.length -= offset_inc;
 
-	if (op->op == CEPH_OSD_OP_WRITE || op->op == CEPH_OSD_OP_WRITEFULL)
+	if (op->op == CEPH_OSD_OP_WRITE || op->op == CEPH_OSD_OP_WRITEFULL || op->op == CEPH_OSD_OP_APPEND)
 		op->indata_len -= offset_inc;
 }
 EXPORT_SYMBOL(osd_req_op_extent_dup_last);
@@ -913,6 +914,7 @@  static u32 osd_req_encode_op(struct ceph_osd_op *dst,
 	case CEPH_OSD_OP_READ:
 	case CEPH_OSD_OP_WRITE:
 	case CEPH_OSD_OP_WRITEFULL:
+	case CEPH_OSD_OP_APPEND:
 	case CEPH_OSD_OP_ZERO:
 	case CEPH_OSD_OP_TRUNCATE:
 		dst->extent.offset = cpu_to_le64(src->extent.offset);
@@ -998,7 +1000,7 @@  struct ceph_osd_request *ceph_osdc_new_request(struct ceph_osd_client *osdc,
 
 	BUG_ON(opcode != CEPH_OSD_OP_READ && opcode != CEPH_OSD_OP_WRITE &&
 	       opcode != CEPH_OSD_OP_ZERO && opcode != CEPH_OSD_OP_TRUNCATE &&
-	       opcode != CEPH_OSD_OP_CREATE && opcode != CEPH_OSD_OP_DELETE);
+	       opcode != CEPH_OSD_OP_CREATE && opcode != CEPH_OSD_OP_DELETE && opcode != CEPH_OSD_OP_APPEND);
 
 	req = ceph_osdc_alloc_request(osdc, snapc, num_ops, use_mempool,
 					GFP_NOFS);
@@ -1872,6 +1874,7 @@  static void setup_request_data(struct ceph_osd_request *req,
 		/* request */
 		case CEPH_OSD_OP_WRITE:
 		case CEPH_OSD_OP_WRITEFULL:
+		case CEPH_OSD_OP_APPEND:
 			WARN_ON(op->indata_len != op->extent.length);
 			ceph_osdc_msg_data_add(msg, &op->extent.osd_data);
 			break;