[PATCH 0/4] libceph: use op formatter for ceph_osdc_new_request()

[PATCH 0/4] libceph: use op formatter for ceph_osdc_new_request()

[Alex Elder]

The only place outside rbd that allocates an osd request
and then initializes the array of osd ops inside it is
ceph_osdc_new_request().  This series rearranges a little
code so that the new op formatting functions (osd_req_op_init()
and osd_req_op_extent_init()) can be used there.

					-Alex

[PATCH 1/4] libceph: pass offset and length out of calc_layout()
[PATCH 2/4] libceph: don't update op in calc_layout()
[PATCH 3/4] libceph: clean up ceph_osd_new_request()
[PATCH 4/4] libceph: use osd_req_op_extent_init()

[PATCH 1/4] libceph: pass offset and length out of calc_layout()

[Alex Elder]

The purpose of calc_layout() is to determine, given a file offset
and length and a layout describing the placement of file data across
objects, where in "object space" that data resides.

Specifically, it determines which object should hold the first part
of the specified range of file data, and the offset and length of
data within that object.  The length will not exceed the bounds
of the object, and the caller is informed of that maximum length.

Add two parameters to calc_layout() to allow the object-relative
offset and length to be passed back to the caller.

This is the first steps toward having ceph_osdc_new_request() build
its osd op structure using osd_req_op_extent_init().

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/osd_client.c |   32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 02ed728..f782aca 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -64,32 +64,31 @@ static int op_has_extent(int op)
  * fill osd op in request message.
  */
 static int calc_layout(struct ceph_file_layout *layout, u64 off, u64 *plen,
-		       struct ceph_osd_req_op *op, u64 *bno)
+		       struct ceph_osd_req_op *op, u64 *objnum,
+		       u64 *objoff, u64 *objlen)
 {
 	u64 orig_len = *plen;
-	u64 objoff = 0;
-	u64 objlen = 0;
 	int r;

 	/* object extent? */
-	r = ceph_calc_file_object_mapping(layout, off, orig_len, bno,
-					  &objoff, &objlen);
+	r = ceph_calc_file_object_mapping(layout, off, orig_len, objnum,
+					  objoff, objlen);
 	if (r < 0)
 		return r;
-	if (objlen < orig_len) {
-		*plen = objlen;
+	if (*objlen < orig_len) {
+		*plen = *objlen;
 		dout(" skipping last %llu, final file extent %llu~%llu\n",
 		     orig_len - *plen, off, *plen);
 	}

 	if (op_has_extent(op->op)) {
 		u32 osize = le32_to_cpu(layout->fl_object_size);
-		op->extent.offset = objoff;
-		op->extent.length = objlen;
-		if (op->extent.truncate_size <= off - objoff) {
+		op->extent.offset = *objoff;
+		op->extent.length = *objlen;
+		if (op->extent.truncate_size <= off - *objoff) {
 			op->extent.truncate_size = 0;
 		} else {
-			op->extent.truncate_size -= off - objoff;
+			op->extent.truncate_size -= off - *objoff;
 			if (op->extent.truncate_size > osize)
 				op->extent.truncate_size = osize;
 		}
@@ -97,7 +96,7 @@ static int calc_layout(struct ceph_file_layout
*layout, u64 off, u64 *plen,
 	if (op->op == CEPH_OSD_OP_WRITE)
 		op->payload_len = *plen;

-	dout("calc_layout bno=%llx %llu~%llu\n", *bno, objoff, objlen);
+	dout("calc_layout objnum=%llx %llu~%llu\n", *objnum, *objoff, *objlen);

 	return 0;
 }
@@ -572,7 +571,9 @@ struct ceph_osd_request
*ceph_osdc_new_request(struct ceph_osd_client *osdc,
 	struct ceph_osd_req_op ops[2];
 	struct ceph_osd_request *req;
 	unsigned int num_op = 1;
-	u64 bno = 0;
+	u64 objnum = 0;
+	u64 objoff = 0;
+	u64 objlen = 0;
 	int r;

 	memset(&ops, 0, sizeof ops);
@@ -593,14 +594,15 @@ struct ceph_osd_request
*ceph_osdc_new_request(struct ceph_osd_client *osdc,
 	req->r_flags = flags;

 	/* calculate max write size */
-	r = calc_layout(layout, off, plen, ops, &bno);
+	r = calc_layout(layout, off, plen, ops, &objnum, &objoff, &objlen);
 	if (r < 0) {
 		ceph_osdc_put_request(req);
 		return ERR_PTR(r);
 	}
 	req->r_file_layout = *layout;  /* keep a copy */

-	snprintf(req->r_oid, sizeof(req->r_oid), "%llx.%08llx", vino.ino, bno);
+	snprintf(req->r_oid, sizeof(req->r_oid), "%llx.%08llx",
+		vino.ino, objnum);
 	req->r_oid_len = strlen(req->r_oid);

 	ceph_osdc_build_request(req, off, num_op, ops,
-- 
1.7.9.5

[PATCH 2/4] libceph: don't update op in calc_layout()

[Alex Elder]

The ceph_osdc_new_request() an array of osd operations is built up
and filled in partially within that function and partially in the
called function calc_layout().  Move the latter part back out to
ceph_osdc_new_request() so it's all done in one place.  This makes
it unnecessary to pass the op pointer to calc_layout(), so get rid
of that parameter.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/osd_client.c |   36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index f782aca..0eb417b 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -64,8 +64,7 @@ static int op_has_extent(int op)
  * fill osd op in request message.
  */
 static int calc_layout(struct ceph_file_layout *layout, u64 off, u64 *plen,
-		       struct ceph_osd_req_op *op, u64 *objnum,
-		       u64 *objoff, u64 *objlen)
+			u64 *objnum, u64 *objoff, u64 *objlen)
 {
 	u64 orig_len = *plen;
 	int r;
@@ -81,21 +80,6 @@ static int calc_layout(struct ceph_file_layout
*layout, u64 off, u64 *plen,
 		     orig_len - *plen, off, *plen);
 	}

-	if (op_has_extent(op->op)) {
-		u32 osize = le32_to_cpu(layout->fl_object_size);
-		op->extent.offset = *objoff;
-		op->extent.length = *objlen;
-		if (op->extent.truncate_size <= off - *objoff) {
-			op->extent.truncate_size = 0;
-		} else {
-			op->extent.truncate_size -= off - *objoff;
-			if (op->extent.truncate_size > osize)
-				op->extent.truncate_size = osize;
-		}
-	}
-	if (op->op == CEPH_OSD_OP_WRITE)
-		op->payload_len = *plen;
-
 	dout("calc_layout objnum=%llx %llu~%llu\n", *objnum, *objoff, *objlen);

 	return 0;
@@ -594,11 +578,27 @@ struct ceph_osd_request
*ceph_osdc_new_request(struct ceph_osd_client *osdc,
 	req->r_flags = flags;

 	/* calculate max write size */
-	r = calc_layout(layout, off, plen, ops, &objnum, &objoff, &objlen);
+	r = calc_layout(layout, off, plen, &objnum, &objoff, &objlen);
 	if (r < 0) {
 		ceph_osdc_put_request(req);
 		return ERR_PTR(r);
 	}
+
+	if (op_has_extent(ops[0].op)) {
+		u32 osize = le32_to_cpu(layout->fl_object_size);
+		ops[0].extent.offset = objoff;
+		ops[0].extent.length = objlen;
+		if (ops[0].extent.truncate_size <= off - objoff) {
+			ops[0].extent.truncate_size = 0;
+		} else {
+			ops[0].extent.truncate_size -= off - objoff;
+			if (ops[0].extent.truncate_size > osize)
+				ops[0].extent.truncate_size = osize;
+		}
+	}
+	if (ops[0].op == CEPH_OSD_OP_WRITE)
+		ops[0].payload_len = *plen;
+
 	req->r_file_layout = *layout;  /* keep a copy */

 	snprintf(req->r_oid, sizeof(req->r_oid), "%llx.%08llx",
-- 
1.7.9.5

[PATCH 3/4] libceph: clean up ceph_osd_new_request()

[Alex Elder]

All callers of ceph_osd_new_request() pass either CEPH_OSD_OP_READ
or CEPH_OSD_OP_WRITE as the opcode value.  The function assumes it
by filling in the extent fields in the ops array it builds.  So just
assert that is the case, and don't bother calling op_has_extent()
before filling in the first osd operation in the array.

Define some local variables to gather the information to fill into
the first op, and then fill in the op array all in one place.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/osd_client.c |   50
++++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 0eb417b..7136060 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -32,12 +32,6 @@ static void __unregister_linger_request(struct
ceph_osd_client *osdc,
 static void __send_request(struct ceph_osd_client *osdc,
 			   struct ceph_osd_request *req);

-static int op_has_extent(int op)
-{
-	return (op == CEPH_OSD_OP_READ ||
-		op == CEPH_OSD_OP_WRITE);
-}
-
 /*
  * Implement client access to distributed object storage cluster.
  *
@@ -554,22 +548,15 @@ struct ceph_osd_request
*ceph_osdc_new_request(struct ceph_osd_client *osdc,
 {
 	struct ceph_osd_req_op ops[2];
 	struct ceph_osd_request *req;
-	unsigned int num_op = 1;
+	unsigned int num_op = do_sync ? 2 : 1;
 	u64 objnum = 0;
 	u64 objoff = 0;
 	u64 objlen = 0;
+	u32 object_size;
+	u64 object_base;
 	int r;

-	memset(&ops, 0, sizeof ops);
-
-	ops[0].op = opcode;
-	ops[0].extent.truncate_seq = truncate_seq;
-	ops[0].extent.truncate_size = truncate_size;
-
-	if (do_sync) {
-		ops[1].op = CEPH_OSD_OP_STARTSYNC;
-		num_op++;
-	}
+	BUG_ON(opcode != CEPH_OSD_OP_READ && opcode != CEPH_OSD_OP_WRITE);

 	req = ceph_osdc_alloc_request(osdc, snapc, num_op, use_mempool,
 					GFP_NOFS);
@@ -584,21 +571,28 @@ struct ceph_osd_request
*ceph_osdc_new_request(struct ceph_osd_client *osdc,
 		return ERR_PTR(r);
 	}

-	if (op_has_extent(ops[0].op)) {
-		u32 osize = le32_to_cpu(layout->fl_object_size);
-		ops[0].extent.offset = objoff;
-		ops[0].extent.length = objlen;
-		if (ops[0].extent.truncate_size <= off - objoff) {
-			ops[0].extent.truncate_size = 0;
-		} else {
-			ops[0].extent.truncate_size -= off - objoff;
-			if (ops[0].extent.truncate_size > osize)
-				ops[0].extent.truncate_size = osize;
-		}
+	object_size = le32_to_cpu(layout->fl_object_size);
+	object_base = off - objoff;
+	if (truncate_size <= object_base) {
+		truncate_size = 0;
+	} else {
+		truncate_size -= object_base;
+		if (truncate_size > object_size)
+			truncate_size = object_size;
 	}
+
+	memset(&ops, 0, sizeof ops);
+	ops[0].op = opcode;
+	ops[0].extent.offset = objoff;
+	ops[0].extent.length = objlen;
+	ops[0].extent.truncate_size = truncate_size;
+	ops[0].extent.truncate_seq = truncate_seq;
 	if (ops[0].op == CEPH_OSD_OP_WRITE)
 		ops[0].payload_len = *plen;

+	if (do_sync)
+		ops[1].op = CEPH_OSD_OP_STARTSYNC;
+
 	req->r_file_layout = *layout;  /* keep a copy */

 	snprintf(req->r_oid, sizeof(req->r_oid), "%llx.%08llx",
-- 
1.7.9.5

[PATCH 4/4] libceph: use osd_req_op_extent_init()

[Alex Elder]

Use osd_req_op_extent_init() in ceph_osdc_new_request() to
initialize the one or two ops built in that function.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/osd_client.c |   13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 7136060..55f7c9a 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -581,17 +581,10 @@ struct ceph_osd_request
*ceph_osdc_new_request(struct ceph_osd_client *osdc,
 			truncate_size = object_size;
 	}

-	memset(&ops, 0, sizeof ops);
-	ops[0].op = opcode;
-	ops[0].extent.offset = objoff;
-	ops[0].extent.length = objlen;
-	ops[0].extent.truncate_size = truncate_size;
-	ops[0].extent.truncate_seq = truncate_seq;
-	if (ops[0].op == CEPH_OSD_OP_WRITE)
-		ops[0].payload_len = *plen;
-
+	osd_req_op_extent_init(&ops[0], opcode, objoff, objlen,
+				truncate_size, truncate_seq);
 	if (do_sync)
-		ops[1].op = CEPH_OSD_OP_STARTSYNC;
+		osd_req_op_init(&ops[1], CEPH_OSD_OP_STARTSYNC);

 	req->r_file_layout = *layout;  /* keep a copy */

-- 
1.7.9.5

Re: [PATCH 0/4] libceph: use op formatter for ceph_osdc_new_request()

[Josh Durgin]

On 03/29/2013 02:30 PM, Alex Elder wrote:
> The only place outside rbd that allocates an osd request
> and then initializes the array of osd ops inside it is
> ceph_osdc_new_request().  This series rearranges a little
> code so that the new op formatting functions (osd_req_op_init()
> and osd_req_op_extent_init()) can be used there.
>
> 					-Alex
>
> [PATCH 1/4] libceph: pass offset and length out of calc_layout()
> [PATCH 2/4] libceph: don't update op in calc_layout()
> [PATCH 3/4] libceph: clean up ceph_osd_new_request()
> [PATCH 4/4] libceph: use osd_req_op_extent_init()

These all look good.

Reviewed-by: Josh Durgin <josh.durgin@inktank.com>