[PATCH] rbd: support reading parent page data for writes

[PATCH] rbd: support reading parent page data for writes

[Alex Elder]

Currently, rbd_img_obj_parent_read_full() assumes the incoming
object request contains bio data.  But if a layered image is part of
a multi-layer stack of images it will result in read requests of
page data to parent images.

This is handling the same kind of issue as was resolved by this
commit:
    5b2ab72d  rbd: support reading parent page data

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

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 51c45e7..b9221a0 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -2160,7 +2160,8 @@ rbd_img_obj_parent_read_full_callback(struct
rbd_img_request *img_request)
 	struct page **pages;
 	int result;
 	u64 obj_size;
-	u64 xferred;
+	u64 offset;
+	u64 length;

 	rbd_assert(img_request_child_test(img_request));

@@ -2172,10 +2173,10 @@ rbd_img_obj_parent_read_full_callback(struct
rbd_img_request *img_request)

 	orig_request = img_request->obj_request;
 	rbd_assert(orig_request != NULL);
-	rbd_assert(orig_request->type == OBJ_REQUEST_BIO);
+	rbd_assert(obj_request_type_valid(orig_request->type));
 	result = img_request->result;
 	obj_size = img_request->length;
-	xferred = img_request->xferred;
+	rbd_assert(obj_size == img_request->xferred);
 	rbd_img_request_put(img_request);

 	rbd_assert(orig_request->img_request);
@@ -2204,11 +2205,17 @@ rbd_img_obj_parent_read_full_callback(struct
rbd_img_request *img_request)

 	/* Then the original write request op */

+	offset = orig_request->offset;
+	length = orig_request->length;
 	osd_req_op_extent_init(osd_req, 1, CEPH_OSD_OP_WRITE,
-					orig_request->offset,
-					orig_request->length, 0, 0);
-	osd_req_op_extent_osd_data_bio(osd_req, 1, orig_request->bio_list,
-					orig_request->length);
+					offset, length, 0, 0);
+	if (orig_request->type == OBJ_REQUEST_BIO)
+		osd_req_op_extent_osd_data_bio(osd_req, 1,
+					orig_request->bio_list, length);
+	else
+		osd_req_op_extent_osd_data_pages(osd_req, 1,
+					orig_request->pages, length,
+					offset & ~PAGE_MASK, false, false);

 	rbd_osd_req_format_write(orig_request);

-- 
1.7.9.5

Re: [PATCH] rbd: support reading parent page data for writes

[Josh Durgin]

On 05/11/2013 10:37 AM, Alex Elder wrote:
> Currently, rbd_img_obj_parent_read_full() assumes the incoming
> object request contains bio data.  But if a layered image is part of
> a multi-layer stack of images it will result in read requests of
> page data to parent images.
>
> This is handling the same kind of issue as was resolved by this
> commit:
>      5b2ab72d  rbd: support reading parent page data
>
> This resolves:
>      http://tracker.ceph.com/issues/5027
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 51c45e7..b9221a0 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -2160,7 +2160,8 @@ rbd_img_obj_parent_read_full_callback(struct
> rbd_img_request *img_request)
>   	struct page **pages;
>   	int result;
>   	u64 obj_size;
> -	u64 xferred;
> +	u64 offset;
> +	u64 length;
>
>   	rbd_assert(img_request_child_test(img_request));
>
> @@ -2172,10 +2173,10 @@ rbd_img_obj_parent_read_full_callback(struct
> rbd_img_request *img_request)
>
>   	orig_request = img_request->obj_request;
>   	rbd_assert(orig_request != NULL);
> -	rbd_assert(orig_request->type == OBJ_REQUEST_BIO);
> +	rbd_assert(obj_request_type_valid(orig_request->type));
>   	result = img_request->result;
>   	obj_size = img_request->length;

Naming this obj_size is a bit confusing now, since a parent read that
would cross the overlap point will be shortened to less than an object
size.

The rest looks good.

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

> -	xferred = img_request->xferred;
> +	rbd_assert(obj_size == img_request->xferred);
>   	rbd_img_request_put(img_request);
>
>   	rbd_assert(orig_request->img_request);
> @@ -2204,11 +2205,17 @@ rbd_img_obj_parent_read_full_callback(struct
> rbd_img_request *img_request)
>
>   	/* Then the original write request op */
>
> +	offset = orig_request->offset;
> +	length = orig_request->length;
>   	osd_req_op_extent_init(osd_req, 1, CEPH_OSD_OP_WRITE,
> -					orig_request->offset,
> -					orig_request->length, 0, 0);
> -	osd_req_op_extent_osd_data_bio(osd_req, 1, orig_request->bio_list,
> -					orig_request->length);
> +					offset, length, 0, 0);
> +	if (orig_request->type == OBJ_REQUEST_BIO)
> +		osd_req_op_extent_osd_data_bio(osd_req, 1,
> +					orig_request->bio_list, length);
> +	else
> +		osd_req_op_extent_osd_data_pages(osd_req, 1,
> +					orig_request->pages, length,
> +					offset & ~PAGE_MASK, false, false);
>
>   	rbd_osd_req_format_write(orig_request);
>

[PATCH] rbd: fix parent request size assumption

[Alex Elder]

I have inserted this patch into a series I posted over the
weekend to address an issue that Josh's comment raised in
the review for:
    [PATCH] rbd: support reading parent page data for writes
This patch, along with the updated series, is available in the
"review/wip-flatten-1" branch of the ceph-client git repository.





The code that reads object data from the parent for a copyup on
write request currently assumes that the size of that request is the
size of a "full" object from the original target image.

That is not necessarily the case.  The parent overlap could reduce
the request size below that.  To fix that assumption we need to
record the number of pages in the copyup_pages array, for both an
image request and an object request.  Rename a local variable in
rbd_img_obj_parent_read_full_callback() to reflect we're recording
the length of the parent read request, not the size of the target
object.

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

Signed-off-by: Alex Elder <elder@inktank.com>
---
 drivers/block/rbd.c |   31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index 51c45e7..597b9bb 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -224,6 +224,7 @@ struct rbd_obj_request {
 		};
 	};
 	struct page		**copyup_pages;
+	u32			copyup_page_count;

 	struct ceph_osd_request	*osd_req;

@@ -256,6 +257,7 @@ struct rbd_img_request {
 		struct rbd_obj_request	*obj_request;	/* obj req initiator */
 	};
 	struct page		**copyup_pages;
+	u32			copyup_page_count;
 	spinlock_t		completion_lock;/* protects next_completion */
 	u32			next_completion;
 	rbd_img_callback_t	callback;
@@ -2119,7 +2121,7 @@ rbd_img_obj_copyup_callback(struct rbd_obj_request
*obj_request)
 {
 	struct rbd_img_request *img_request;
 	struct rbd_device *rbd_dev;
-	u64 length;
+	struct page **pages;
 	u32 page_count;

 	rbd_assert(obj_request->type == OBJ_REQUEST_BIO);
@@ -2129,12 +2131,14 @@ rbd_img_obj_copyup_callback(struct
rbd_obj_request *obj_request)

 	rbd_dev = img_request->rbd_dev;
 	rbd_assert(rbd_dev);
-	length = (u64)1 << rbd_dev->header.obj_order;
-	page_count = (u32)calc_pages_for(0, length);

-	rbd_assert(obj_request->copyup_pages);
-	ceph_release_page_vector(obj_request->copyup_pages, page_count);
+	pages = obj_request->copyup_pages;
+	rbd_assert(pages != NULL);
 	obj_request->copyup_pages = NULL;
+	page_count = obj_request->copyup_page_count;
+	rbd_assert(page_count);
+	obj_request->copyup_page_count = 0;
+	ceph_release_page_vector(pages, page_count);

 	/*
 	 * We want the transfer count to reflect the size of the
@@ -2158,9 +2162,9 @@ rbd_img_obj_parent_read_full_callback(struct
rbd_img_request *img_request)
 	struct ceph_osd_client *osdc;
 	struct rbd_device *rbd_dev;
 	struct page **pages;
+	u32 page_count;
 	int result;
-	u64 obj_size;
-	u64 xferred;
+	u64 parent_length;

 	rbd_assert(img_request_child_test(img_request));

@@ -2169,19 +2173,21 @@ rbd_img_obj_parent_read_full_callback(struct
rbd_img_request *img_request)
 	pages = img_request->copyup_pages;
 	rbd_assert(pages != NULL);
 	img_request->copyup_pages = NULL;
+	page_count = img_request->copyup_page_count;
+	rbd_assert(page_count);
+	img_request->copyup_page_count = 0;

 	orig_request = img_request->obj_request;
 	rbd_assert(orig_request != NULL);
 	rbd_assert(orig_request->type == OBJ_REQUEST_BIO);
 	result = img_request->result;
-	obj_size = img_request->length;
-	xferred = img_request->xferred;
+	parent_length = img_request->length;
+	rbd_assert(parent_length == img_request->xferred);
 	rbd_img_request_put(img_request);

 	rbd_assert(orig_request->img_request);
 	rbd_dev = orig_request->img_request->rbd_dev;
 	rbd_assert(rbd_dev);
-	rbd_assert(obj_size == (u64)1 << rbd_dev->header.obj_order);

 	if (result)
 		goto out_err;
@@ -2195,11 +2201,12 @@ rbd_img_obj_parent_read_full_callback(struct
rbd_img_request *img_request)
 		goto out_err;
 	orig_request->osd_req = osd_req;
 	orig_request->copyup_pages = pages;
+	orig_request->copyup_page_count = page_count;

 	/* Initialize the copyup op */

 	osd_req_op_cls_init(osd_req, 0, CEPH_OSD_OP_CALL, "rbd", "copyup");
-	osd_req_op_cls_request_data_pages(osd_req, 0, pages, obj_size, 0,
+	osd_req_op_cls_request_data_pages(osd_req, 0, pages, parent_length, 0,
 						false, false);

 	/* Then the original write request op */
@@ -2312,6 +2319,7 @@ static int rbd_img_obj_parent_read_full(struct
rbd_obj_request *obj_request)
 	if (result)
 		goto out_err;
 	parent_request->copyup_pages = pages;
+	parent_request->copyup_page_count = page_count;

 	parent_request->callback = rbd_img_obj_parent_read_full_callback;
 	result = rbd_img_request_submit(parent_request);
@@ -2319,6 +2327,7 @@ static int rbd_img_obj_parent_read_full(struct
rbd_obj_request *obj_request)
 		return 0;

 	parent_request->copyup_pages = NULL;
+	parent_request->copyup_page_count = 0;
 	parent_request->obj_request = NULL;
 	rbd_obj_request_put(obj_request);
 out_err:
-- 
1.7.9.5

Re: [PATCH] rbd: fix parent request size assumption

[Josh Durgin]

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

On 05/13/2013 10:58 AM, Alex Elder wrote:
> I have inserted this patch into a series I posted over the
> weekend to address an issue that Josh's comment raised in
> the review for:
>      [PATCH] rbd: support reading parent page data for writes
> This patch, along with the updated series, is available in the
> "review/wip-flatten-1" branch of the ceph-client git repository.
>
>
>
>
>
> The code that reads object data from the parent for a copyup on
> write request currently assumes that the size of that request is the
> size of a "full" object from the original target image.
>
> That is not necessarily the case.  The parent overlap could reduce
> the request size below that.  To fix that assumption we need to
> record the number of pages in the copyup_pages array, for both an
> image request and an object request.  Rename a local variable in
> rbd_img_obj_parent_read_full_callback() to reflect we're recording
> the length of the parent read request, not the size of the target
> object.
>
> This resolves:
>      http://tracker.ceph.com/issues/5038
>
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>   drivers/block/rbd.c |   31 ++++++++++++++++++++-----------
>   1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 51c45e7..597b9bb 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -224,6 +224,7 @@ struct rbd_obj_request {
>   		};
>   	};
>   	struct page		**copyup_pages;
> +	u32			copyup_page_count;
>
>   	struct ceph_osd_request	*osd_req;
>
> @@ -256,6 +257,7 @@ struct rbd_img_request {
>   		struct rbd_obj_request	*obj_request;	/* obj req initiator */
>   	};
>   	struct page		**copyup_pages;
> +	u32			copyup_page_count;
>   	spinlock_t		completion_lock;/* protects next_completion */
>   	u32			next_completion;
>   	rbd_img_callback_t	callback;
> @@ -2119,7 +2121,7 @@ rbd_img_obj_copyup_callback(struct rbd_obj_request
> *obj_request)
>   {
>   	struct rbd_img_request *img_request;
>   	struct rbd_device *rbd_dev;
> -	u64 length;
> +	struct page **pages;
>   	u32 page_count;
>
>   	rbd_assert(obj_request->type == OBJ_REQUEST_BIO);
> @@ -2129,12 +2131,14 @@ rbd_img_obj_copyup_callback(struct
> rbd_obj_request *obj_request)
>
>   	rbd_dev = img_request->rbd_dev;
>   	rbd_assert(rbd_dev);
> -	length = (u64)1 << rbd_dev->header.obj_order;
> -	page_count = (u32)calc_pages_for(0, length);
>
> -	rbd_assert(obj_request->copyup_pages);
> -	ceph_release_page_vector(obj_request->copyup_pages, page_count);
> +	pages = obj_request->copyup_pages;
> +	rbd_assert(pages != NULL);
>   	obj_request->copyup_pages = NULL;
> +	page_count = obj_request->copyup_page_count;
> +	rbd_assert(page_count);
> +	obj_request->copyup_page_count = 0;
> +	ceph_release_page_vector(pages, page_count);
>
>   	/*
>   	 * We want the transfer count to reflect the size of the
> @@ -2158,9 +2162,9 @@ rbd_img_obj_parent_read_full_callback(struct
> rbd_img_request *img_request)
>   	struct ceph_osd_client *osdc;
>   	struct rbd_device *rbd_dev;
>   	struct page **pages;
> +	u32 page_count;
>   	int result;
> -	u64 obj_size;
> -	u64 xferred;
> +	u64 parent_length;
>
>   	rbd_assert(img_request_child_test(img_request));
>
> @@ -2169,19 +2173,21 @@ rbd_img_obj_parent_read_full_callback(struct
> rbd_img_request *img_request)
>   	pages = img_request->copyup_pages;
>   	rbd_assert(pages != NULL);
>   	img_request->copyup_pages = NULL;
> +	page_count = img_request->copyup_page_count;
> +	rbd_assert(page_count);
> +	img_request->copyup_page_count = 0;
>
>   	orig_request = img_request->obj_request;
>   	rbd_assert(orig_request != NULL);
>   	rbd_assert(orig_request->type == OBJ_REQUEST_BIO);
>   	result = img_request->result;
> -	obj_size = img_request->length;
> -	xferred = img_request->xferred;
> +	parent_length = img_request->length;
> +	rbd_assert(parent_length == img_request->xferred);
>   	rbd_img_request_put(img_request);
>
>   	rbd_assert(orig_request->img_request);
>   	rbd_dev = orig_request->img_request->rbd_dev;
>   	rbd_assert(rbd_dev);
> -	rbd_assert(obj_size == (u64)1 << rbd_dev->header.obj_order);
>
>   	if (result)
>   		goto out_err;
> @@ -2195,11 +2201,12 @@ rbd_img_obj_parent_read_full_callback(struct
> rbd_img_request *img_request)
>   		goto out_err;
>   	orig_request->osd_req = osd_req;
>   	orig_request->copyup_pages = pages;
> +	orig_request->copyup_page_count = page_count;
>
>   	/* Initialize the copyup op */
>
>   	osd_req_op_cls_init(osd_req, 0, CEPH_OSD_OP_CALL, "rbd", "copyup");
> -	osd_req_op_cls_request_data_pages(osd_req, 0, pages, obj_size, 0,
> +	osd_req_op_cls_request_data_pages(osd_req, 0, pages, parent_length, 0,
>   						false, false);
>
>   	/* Then the original write request op */
> @@ -2312,6 +2319,7 @@ static int rbd_img_obj_parent_read_full(struct
> rbd_obj_request *obj_request)
>   	if (result)
>   		goto out_err;
>   	parent_request->copyup_pages = pages;
> +	parent_request->copyup_page_count = page_count;
>
>   	parent_request->callback = rbd_img_obj_parent_read_full_callback;
>   	result = rbd_img_request_submit(parent_request);
> @@ -2319,6 +2327,7 @@ static int rbd_img_obj_parent_read_full(struct
> rbd_obj_request *obj_request)
>   		return 0;
>
>   	parent_request->copyup_pages = NULL;
> +	parent_request->copyup_page_count = 0;
>   	parent_request->obj_request = NULL;
>   	rbd_obj_request_put(obj_request);
>   out_err:
>