[PATCH 0/3] libceph: focus calc_layout() on filling in the osd op

[PATCH 0/3] libceph: focus calc_layout() on filling in the osd op

[Alex Elder]

This series refactors the code involved with identifying the
details of the name, offset, and length of an object involved
with an osd request based on a file layout.  It makes the focus
of calc_layout() be filling in an osd op structure based on the
file layout it is provided.  The caller (ceph_osdc_new_request())
is then responsible for filling in fields related to the request,
such as the name of the target object.

					-Alex

[PATCH 1/3] libceph: pass object number back to calc_layout() caller
[PATCH 2/3] libceph: format target object name in caller
[PATCH 3/3] libceph: don't pass request to calc_layout()

[PATCH 1/3] libceph: pass object number back to calc_layout() caller

[Alex Elder]

Have calc_layout() pass the computed object number back to its
caller.  (This is a small step to simplify review.)

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

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 5daced2..fcc783b 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -67,16 +67,15 @@ static int calc_layout(struct ceph_vino vino,
 		       struct ceph_file_layout *layout,
 		       u64 off, u64 *plen,
 		       struct ceph_osd_request *req,
-		       struct ceph_osd_req_op *op)
+		       struct ceph_osd_req_op *op, u64 *bno)
 {
 	u64 orig_len = *plen;
-	u64 bno = 0;
 	u64 objoff = 0;
 	u64 objlen = 0;
 	int r;

 	/* object extent? */
-	r = ceph_calc_file_object_mapping(layout, off, orig_len, &bno,
+	r = ceph_calc_file_object_mapping(layout, off, orig_len, bno,
 					  &objoff, &objlen);
 	if (r < 0)
 		return r;
@@ -104,9 +103,9 @@ static int calc_layout(struct ceph_vino vino,
 		op->payload_len = *plen;

 	dout("calc_layout bno=%llx %llu~%llu (%d pages)\n",
-	     bno, objoff, objlen, req->r_num_pages);
+	     *bno, objoff, objlen, req->r_num_pages);

-	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, *bno);
 	req->r_oid_len = strlen(req->r_oid);

 	return 0;
@@ -416,6 +415,7 @@ 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;
 	int r;

 	memset(&ops, 0, sizeof ops);
@@ -436,11 +436,12 @@ struct ceph_osd_request
*ceph_osdc_new_request(struct ceph_osd_client *osdc,
 	req->r_flags = flags;

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

 	/* in case it differs from natural (file) alignment that
-- 
1.7.9.5

[PATCH 2/3] libceph: format target object name in caller

[Alex Elder]

Move the formatting of the object name (oid) to use for an object
request into the caller of calc_layout().  This makes the "vino"
parameter no longer necessary, so get rid of it.

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

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index fcc783b..4f90c24 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -63,9 +63,7 @@ static int op_has_extent(int op)
  *
  * fill osd op in request message.
  */
-static int calc_layout(struct ceph_vino vino,
-		       struct ceph_file_layout *layout,
-		       u64 off, u64 *plen,
+static int calc_layout(struct ceph_file_layout *layout, u64 off, u64 *plen,
 		       struct ceph_osd_request *req,
 		       struct ceph_osd_req_op *op, u64 *bno)
 {
@@ -105,9 +103,6 @@ static int calc_layout(struct ceph_vino vino,
 	dout("calc_layout bno=%llx %llu~%llu (%d pages)\n",
 	     *bno, objoff, objlen, req->r_num_pages);

-	snprintf(req->r_oid, sizeof(req->r_oid), "%llx.%08llx", vino.ino, *bno);
-	req->r_oid_len = strlen(req->r_oid);
-
 	return 0;
 }

@@ -436,7 +431,7 @@ struct ceph_osd_request
*ceph_osdc_new_request(struct ceph_osd_client *osdc,
 	req->r_flags = flags;

 	/* calculate max write size */
-	r = calc_layout(vino, layout, off, plen, req, ops, &bno);
+	r = calc_layout(layout, off, plen, req, ops, &bno);
 	if (r < 0) {
 		ceph_osdc_put_request(req);
 		return ERR_PTR(r);
@@ -444,6 +439,9 @@ struct ceph_osd_request
*ceph_osdc_new_request(struct ceph_osd_client *osdc,

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

+	snprintf(req->r_oid, sizeof(req->r_oid), "%llx.%08llx", vino.ino, bno);
+	req->r_oid_len = strlen(req->r_oid);
+
 	/* in case it differs from natural (file) alignment that
 	   calc_layout filled in for us */
 	req->r_num_pages = calc_pages_for(page_align, *plen);
-- 
1.7.9.5

[PATCH 3/3] libceph: don't pass request to calc_layout()

[Alex Elder]

The only remaining reason to pass the osd request to calc_layout()
is to fill in its r_num_pages and r_page_alignment fields.  Once it
fills those in, it doesn't do anything more with them.

We can therefore move those assignments into the caller, and get rid
of the "req" parameter entirely.

Note, however, that the only caller is ceph_osdc_new_request(),
and that immediately overwrites those fields with values based on
its passed-in page offset.  So the assignment inside calc_layout()
was redundant anyway.

This resolves:
    http://tracker.ceph.com/issues/4262

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

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index 4f90c24..980911e 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -64,7 +64,6 @@ 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_request *req,
 		       struct ceph_osd_req_op *op, u64 *bno)
 {
 	u64 orig_len = *plen;
@@ -95,13 +94,10 @@ static int calc_layout(struct ceph_file_layout
*layout, u64 off, u64 *plen,
 				op->extent.truncate_size = osize;
 		}
 	}
-	req->r_num_pages = calc_pages_for(off, *plen);
-	req->r_page_alignment = off & ~PAGE_MASK;
 	if (op->op == CEPH_OSD_OP_WRITE)
 		op->payload_len = *plen;

-	dout("calc_layout bno=%llx %llu~%llu (%d pages)\n",
-	     *bno, objoff, objlen, req->r_num_pages);
+	dout("calc_layout bno=%llx %llu~%llu\n", *bno, objoff, objlen);

 	return 0;
 }
@@ -431,7 +427,7 @@ 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, req, ops, &bno);
+	r = calc_layout(layout, off, plen, ops, &bno);
 	if (r < 0) {
 		ceph_osdc_put_request(req);
 		return ERR_PTR(r);
@@ -442,8 +438,8 @@ struct ceph_osd_request
*ceph_osdc_new_request(struct ceph_osd_client *osdc,
 	snprintf(req->r_oid, sizeof(req->r_oid), "%llx.%08llx", vino.ino, bno);
 	req->r_oid_len = strlen(req->r_oid);

-	/* in case it differs from natural (file) alignment that
-	   calc_layout filled in for us */
+	/* The alignment may differ from the natural (file) alignment */
+
 	req->r_num_pages = calc_pages_for(page_align, *plen);
 	req->r_page_alignment = page_align;

-- 
1.7.9.5

Re: [PATCH 0/3] libceph: focus calc_layout() on filling in the osd op

[Josh Durgin]

On 02/25/2013 03:09 PM, Alex Elder wrote:
> This series refactors the code involved with identifying the
> details of the name, offset, and length of an object involved
> with an osd request based on a file layout.  It makes the focus
> of calc_layout() be filling in an osd op structure based on the
> file layout it is provided.  The caller (ceph_osdc_new_request())
> is then responsible for filling in fields related to the request,
> such as the name of the target object.
>
> 					-Alex
>
> [PATCH 1/3] libceph: pass object number back to calc_layout() caller
> [PATCH 2/3] libceph: format target object name in caller
> [PATCH 3/3] libceph: don't pass request to calc_layout()

These all look good.
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>