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@xxxxxxxxxxxx>
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;
