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