rbd: header read/refresh improvements

PATCH In review Douglas Fuller 4 participants 14 messages Apr 24, 2015 – May 7, 2015

Subsystems (3)

Block layer Ceph common code (libceph) RADOS block device (RBD)

lore

Share & tools

lore

Douglas Fuller

author

2015-04-24 13:22 UTC #0 lore raw

v2 cover rbd: header read/refresh improvements

Support multiple class op calls in one ceph_msg and consolidate rbd header read and refresh processes to use this feature to reduce the number of ceph_msgs sent for that process. Refresh features on header refresh and begin returning EIO if features have changed since mapping.

v2: Edit history and address comments from Mike Christie.

Douglas Fuller (3):
ceph: support multiple class method calls in one ceph_msg
rbd: combine object method calls in header refresh using fewer
ceph_msgs
rbd: re-read features during header refresh and detect changes.

 drivers/block/rbd.c             | 512 +++++++++++++++++++++++++++++-----------
 include/linux/ceph/osd_client.h |   3 +-
 net/ceph/messenger.c            |   4 +
 net/ceph/osd_client.c           |  90 ++++++-
 4 files changed, 462 insertions(+), 147 deletions(-)

Signature · 1 line

1.9.3

Douglas Fuller

author

2015-04-24 13:22 UTC #1 lore raw

PATCH v2 1/3 ceph: support multiple class method calls in one ceph_msg

Messages are received from the wire directly into destination buffers. A short read could result in corruption of the following destination buffers. Allocate a single message buffer for all class method calls and split them at the osd_client level. This only applies to ceph_msgs containing multiple op call and may break support for ceph_msgs containing a mix of class method calls that return data and other ops.

Signed-off-by Douglas Fuller <[email protected]>

---
 include/linux/ceph/osd_client.h |  1 +
 net/ceph/messenger.c            |  4 ++
 net/ceph/osd_client.c           | 90 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 93 insertions(+), 2 deletions(-)

include/linux/ceph/osd_client.h +1 -0

index 61b19c4..65fcf80 100644
@@ -99,6 +99,7 @@ struct ceph_osd_req_op {
 			struct ceph_osd_data request_info;
 			struct ceph_osd_data request_data;
 			struct ceph_osd_data response_data;
+			struct ceph_osd_data chain_data;
 			__u8 class_len;
 			__u8 method_len;
 			__u8 argc;

net/ceph/messenger.c +4 -0

index 967080a..ec04be4 100644
@@ -907,6 +907,10 @@ static void ceph_msg_data_pages_cursor_init(struct ceph_msg_data_cursor *cursor,
 	BUG_ON(!data->pages);
 	BUG_ON(!data->length);
 
+	/*
+	 * bug here if a short read occurs and length < data->length; see
+	 * http://tracker.ceph.com/issues/11424
+	 */
 	cursor->resid = min(length, data->length);
 	page_count = calc_pages_for(data->alignment, (u64)data->length);
 	cursor->page_offset = data->alignment & ~PAGE_MASK;

net/ceph/osd_client.c +88 -2

108 lines not on this page · raw message

Signature · 1 line

1.9.3

Douglas Fuller

author

2015-04-24 13:22 UTC #2 lore raw

PATCH v2 2/3 rbd: combine object method calls in header refresh using fewer ceph_msgs

In reply to Douglas Fuller

Signed-off-by Douglas Fuller <[email protected]>

---
 drivers/block/rbd.c             | 331 ++++++++++++++++++++++++++++++++++++----
 include/linux/ceph/osd_client.h |   2 +-
 2 files changed, 306 insertions(+), 27 deletions(-)

drivers/block/rbd.c +305 ~~-26~~

index 8125233..9184b43 100644
@@ -4667,20 +4667,23 @@ static int rbd_dev_v2_header_info(struct rbd_device *rbd_dev)
 	bool first_time = rbd_dev->header.object_prefix == NULL;
 	int ret;
 
-	ret = rbd_dev_v2_image_size(rbd_dev);
-	if (ret)
-		return ret;
+	if (!first_time)
+	{
+		ret = rbd_dev_v2_image_size(rbd_dev);
+		if (ret)
+			return ret;
 
-	if (first_time) {
+		ret = rbd_dev_v2_snap_context(rbd_dev);
+		dout("rbd_dev_v2_snap_context returned %d\n", ret);
+		return ret;
+	}
+	else
+	{
 		ret = rbd_dev_v2_header_onetime(rbd_dev);
 		if (ret)
 			return ret;
 	}
-
-	ret = rbd_dev_v2_snap_context(rbd_dev);
-	dout("rbd_dev_v2_snap_context returned %d\n", ret);
-
-	return ret;
+	return ret; /* XXX change logic? */
 }
 
 static int rbd_dev_header_info(struct rbd_device *rbd_dev)

@@ -5091,36 +5094,312 @@ static void rbd_dev_unprobe(struct rbd_device *rbd_dev)
 	memset(header, 0, sizeof (*header));
 }
 
+static struct ceph_osd_request * rbd_obj_header_req_alloc(
+		struct rbd_device *rbd_dev,
+		int num_ops)
+{
+	struct ceph_osd_request *osd_req;
+	struct page ***replies;
+	
+	replies = kmalloc(CEPH_OSD_MAX_OP * sizeof(*replies), GFP_KERNEL);
+	if (!replies)
+		return NULL;
+
+	osd_req = ceph_osdc_alloc_request(&rbd_dev->rbd_client->client->osdc,
+		NULL, num_ops, false, GFP_KERNEL);
+	if (!osd_req) {
+		kfree(replies);
+		return NULL;
+	}
+
+	osd_req->r_flags = CEPH_OSD_FLAG_READ;
+	osd_req->r_base_oloc.pool = ceph_file_layout_pg_pool(rbd_dev->layout);
+	osd_req->r_priv = replies;
+	ceph_oid_set_name(&osd_req->r_base_oid, rbd_dev->header_name);
+
+	return osd_req;
+}
+
+static int rbd_obj_header_method_add(struct ceph_osd_request *osd_req,
+		int which,
+		const char *class_name,
+		const char *method_name,
+		const void *outbound,
+		size_t outbound_size,
+		size_t inbound_size)
+{
+	u32 page_count;
+	struct page ***replies = osd_req->r_priv;
+
+	BUG_ON(which >= osd_req->r_num_ops);
+
+	osd_req_op_cls_init(osd_req, which, CEPH_OSD_OP_CALL,
+		class_name, method_name);
+
+	if (outbound_size) {
+		struct ceph_pagelist *pagelist;
+		pagelist = kmalloc(sizeof(*pagelist), GFP_NOFS);
+		/* XXX is this ever freed? */
+		if (!pagelist)
+			return -ENOMEM;
+
+		ceph_pagelist_init(pagelist);
+		ceph_pagelist_append(pagelist, outbound, outbound_size);
+		osd_req_op_cls_request_data_pagelist(osd_req, which, pagelist);
+	}
+
+	page_count = (u32)calc_pages_for(0, inbound_size);
+	replies[which] = ceph_alloc_page_vector(page_count, GFP_KERNEL);
+	if (IS_ERR(replies[which]))
+		return PTR_ERR(replies[which]);
+	osd_req_op_cls_response_data_pages(osd_req, which, replies[which],
+		inbound_size, 0, false, false);
+
+	return 0;
+}
+
+static int rbd_obj_header_req_exec(struct rbd_device * rbd_dev,
+		struct ceph_osd_request *osd_req)
+{
+	int ret;
+
+	ceph_osdc_build_request(osd_req, 0, NULL, cpu_to_le64(CEPH_NOSNAP), NULL);
+	ret = ceph_osdc_start_request(&rbd_dev->rbd_client->client->osdc,
+		osd_req, false);
+	if (ret) {
+		ceph_osdc_put_request(osd_req);
+		return ret;
+	}
+
+	ret = ceph_osdc_wait_request(&rbd_dev->rbd_client->client->osdc, osd_req);
+	if (ret < 0) {
+		ceph_osdc_cancel_request(osd_req);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int rbd_obj_header_method_retrieve(struct ceph_osd_request *osd_req,
+		int which, void *buf, size_t length)
+{
+	struct page ***replies = (struct page ***)osd_req->r_priv;
+	size_t reply_length = osd_req->r_reply_op_len[which];
+
+	BUG_ON(reply_length > length);
+
+	ceph_copy_from_page_vector(replies[which], buf, 0, reply_length);
+	ceph_release_page_vector(replies[which], calc_pages_for(0, reply_length));
+
+	return reply_length;
+}
+
+static void rbd_obj_header_req_destroy(struct ceph_osd_request *osd_req)
+{
+	if (osd_req) {
+		kfree(osd_req->r_priv);
+		ceph_osdc_put_request(osd_req);
+	}
+}
+
+static int __extract_prefix(struct rbd_device *rbd_dev,
+		struct ceph_osd_request *osd_req,
+		int which)
+{
+	void *prefix_buf;
+	void * p;
+	size_t prefix_buflen;
+	int ret;
+
+	prefix_buf = kzalloc(RBD_OBJ_PREFIX_LEN_MAX, GFP_KERNEL);
+	if (!prefix_buf)
+		return -ENOMEM;
+
+	prefix_buflen = rbd_obj_header_method_retrieve(osd_req, which,
+		prefix_buf, RBD_OBJ_PREFIX_LEN_MAX);
+	p = prefix_buf;
+	rbd_dev->header.object_prefix = ceph_extract_encoded_string(&p,
+		p + prefix_buflen, NULL, GFP_KERNEL);
+
+	if (IS_ERR(rbd_dev->header.object_prefix)) {
+		ret = PTR_ERR(rbd_dev->header.object_prefix);
+		rbd_dev->header.object_prefix = NULL;
+		goto out_err;
+	}
+
+	ret = 0;
+
+out_err:
+	kfree(prefix_buf);
+	return ret;
+}
+
+static int __extract_features(struct rbd_device *rbd_dev,
+		struct ceph_osd_request *osd_req,
+		int which)
+{
+	int ret;
+	struct {
+		__le64 features;
+		__le64 incompat;
+	} __attribute__((packed)) features_buf = { 0 };
+	u64 incompat;
+
+	rbd_obj_header_method_retrieve(osd_req, which,
+		&features_buf, sizeof(features_buf));
+	incompat = le64_to_cpu(features_buf.incompat);
+	if (incompat & ~RBD_FEATURES_SUPPORTED) {
+		ret = -ENXIO;
+		goto out_err;
+	}
+	rbd_dev->header.features = le64_to_cpu(features_buf.features);
+
+	ret = 0;
+
+out_err:
+	return ret;
+}
+
+static int __extract_snapcontext(struct rbd_device *rbd_dev,
+		struct ceph_osd_request *osd_req,
+		int which)
+{
+	void *snapc_buf;
+	size_t snapc_max;
+	size_t snapc_buflen;
+
+	void *p;
+	void *q;
+
+	struct ceph_snap_context * snapc;
+
+	u32 seq;
+	u32 snap_count;
+
+	int i;
+	int ret;
+
+	snapc_max = sizeof(__le64) + sizeof(__le32) +
+		RBD_MAX_SNAP_COUNT * sizeof(__le64);
+
+	snapc_buf = kzalloc(snapc_max, GFP_KERNEL);
+	if (!snapc_buf) {
+		ret = -ENOMEM;
+		goto out_err;
+	}
+
+	snapc_buflen = rbd_obj_header_method_retrieve(osd_req, which,
+		snapc_buf, snapc_max);
+
+	p = snapc_buf;
+	q = snapc_buf + snapc_buflen;
+	ret = -ERANGE;
+	ceph_decode_64_safe(&p, q, seq, out_err);
+	ceph_decode_32_safe(&p, q, snap_count, out_err);
+
+	if (snap_count > (SIZE_MAX - sizeof(struct ceph_snap_context)) /
+		sizeof(u64)) {
+		ret = -EINVAL;
+		goto out_err;
+	}
+	if (!ceph_has_room(&p, q, snap_count * sizeof(__le64)))
+		goto out_err;
+
+	snapc = ceph_create_snap_context(snap_count, GFP_KERNEL);
+	if (!snapc) {
+		ret = -ENOMEM;
+	}
+
+	snapc->seq = seq;
+	for (i=0; i < snap_count; i++)
+		snapc->snaps[i] = ceph_decode_64(&p);
+	ceph_put_snap_context(rbd_dev->header.snapc);
+	rbd_dev->header.snapc = snapc;
+
+	ret = 0;
+out_err:
+	kfree(snapc_buf);
+	return ret;
+}
+
+static int __extract_size(struct rbd_device *rbd_dev,
+		struct ceph_osd_request *osd_req,
+		int which)
+{
+	struct {
+		u8 order;
+		__le64 size;
+	} __attribute((packed)) size_buf = { 0 };
+
+	rbd_obj_header_method_retrieve(osd_req, which, &size_buf, sizeof(size_buf));
+
+	rbd_dev->header.obj_order = size_buf.order;
+	rbd_dev->header.image_size = le64_to_cpu(size_buf.size);
+
+	return 0;
+}
+
 static int rbd_dev_v2_header_onetime(struct rbd_device *rbd_dev)
 {
 	int ret;
 
-	ret = rbd_dev_v2_object_prefix(rbd_dev);
+	size_t snapc_max;
+	__le64 snapid = cpu_to_le64(CEPH_NOSNAP);
+	struct ceph_osd_request *osd_req;
+
+	snapc_max = sizeof(__le64) + sizeof(__le32) +
+		RBD_MAX_SNAP_COUNT * sizeof(__le64);
+
+	osd_req = rbd_obj_header_req_alloc(rbd_dev, 4);
+
+	if (!osd_req) {
+		ret = -ENOMEM;
+		goto out_err;
+	}
+
+	ret = rbd_obj_header_method_add(osd_req, 0,
+		"rbd", "get_object_prefix",
+		NULL, 0, RBD_OBJ_PREFIX_LEN_MAX);
 	if (ret)
 		goto out_err;
 
-	/*
-	 * Get the and check features for the image.  Currently the
-	 * features are assumed to never change.
-	 */
-	ret = rbd_dev_v2_features(rbd_dev);
+	ret = rbd_obj_header_method_add(osd_req, 1,
+		"rbd", "get_features",
+		&snapid, sizeof(snapid),
+		sizeof(struct{__le64 features;__le64 incompat;} __attribute__((packed))));
 	if (ret)
 		goto out_err;
 
-	/* If the image supports fancy striping, get its parameters */
+	ret = rbd_obj_header_method_add(osd_req, 2,
+		"rbd", "get_snapcontext",
+		NULL, 0, snapc_max);
+	if (ret)
+		goto out_err;
 
-	if (rbd_dev->header.features & RBD_FEATURE_STRIPINGV2) {
-		ret = rbd_dev_v2_striping_info(rbd_dev);
-		if (ret < 0)
-			goto out_err;
-	}
-	/* No support for crypto and compression type format 2 images */
+	ret = rbd_obj_header_method_add(osd_req, 3,
+		"rbd", "get_size",
+		&snapid, sizeof(snapid),
+		sizeof(struct{u8 order; __le64 size;}__attribute((packed))));
+	if (ret)
+		goto out_err;
+
+	ret = rbd_obj_header_req_exec(rbd_dev, osd_req);
+	if (ret)
+		goto out_err;
+
+	if ((ret = __extract_prefix(rbd_dev, osd_req, 0)))
+		goto out_err;
+	if ((ret = __extract_features(rbd_dev, osd_req, 1)))
+		goto out_err;
+	if ((ret = __extract_snapcontext(rbd_dev, osd_req, 2)))
+		goto out_err;
+	if ((ret = __extract_size(rbd_dev, osd_req, 3)))
+		goto out_err;
+
+	ret = 0;
 
-	return 0;
 out_err:
-	rbd_dev->header.features = 0;
-	kfree(rbd_dev->header.object_prefix);
-	rbd_dev->header.object_prefix = NULL;
+	rbd_obj_header_req_destroy(osd_req);
 
 	return ret;
 }

include/linux/ceph/osd_client.h +1 -1

index 65fcf80..71c946d 100644
@@ -43,7 +43,7 @@ struct ceph_osd {
 };
 
 
-#define CEPH_OSD_MAX_OP	3
+#define CEPH_OSD_MAX_OP	4
 
 enum ceph_osd_data_type {
 	CEPH_OSD_DATA_TYPE_NONE = 0,

Signature · 1 line

1.9.3

Douglas Fuller

author

2015-04-24 13:22 UTC #3 lore raw

PATCH v2 3/3 rbd: re-read features during header refresh and detect changes.

In reply to Douglas Fuller

From: Douglas Fuller <[email protected]>

Add a new header item to track when features have changed since mapping and return EIO on I/O operations. Clean up and consolidate code path for header read and update. Remove unused code.

Signed-off-by Douglas Fuller <[email protected]>

---
 drivers/block/rbd.c | 207 +++++++++++++++++++---------------------------------
 1 file changed, 76 insertions(+), 131 deletions(-)

drivers/block/rbd.c +76 ~~-131~~

index 9184b43..d67dcb0 100644
@@ -320,9 +320,16 @@ struct rbd_img_request {
 #define for_each_obj_request_safe(ireq, oreq, n) \
 	list_for_each_entry_safe_reverse(oreq, n, &(ireq)->obj_requests, links)
 
+enum rbd_mapping_state {
+	MAP_STATE_MAPPED,
+	MAP_STATE_DETACHED,
+};
+		
+
 struct rbd_mapping {
 	u64                     size;
 	u64                     features;
+	enum rbd_mapping_state	state;
 	bool			read_only;
 };

@@ -527,7 +534,7 @@ static void rbd_img_parent_read(struct rbd_obj_request *obj_request);
 static void rbd_dev_remove_parent(struct rbd_device *rbd_dev);
 
 static int rbd_dev_refresh(struct rbd_device *rbd_dev);
-static int rbd_dev_v2_header_onetime(struct rbd_device *rbd_dev);
+static int rbd_dev_v2_header_data(struct rbd_device *rbd_dev);
 static int rbd_dev_header_info(struct rbd_device *rbd_dev);
 static int rbd_dev_v2_parent_info(struct rbd_device *rbd_dev);
 static const char *rbd_dev_v2_snap_name(struct rbd_device *rbd_dev,

@@ -3336,6 +3343,14 @@ static void rbd_queue_workfn(struct work_struct *work)
 	else
 		op_type = OBJ_OP_READ;
 
+	/* If a configuration change was detected, issue EIO. */
+
+	if (rbd_dev->mapping.state == MAP_STATE_DETACHED)
+	{
+		result = -EIO;
+		goto err_rq;
+	}
+
 	/* Ignore/skip any zero-length requests */
 
 	if (!length) {

@@ -3670,12 +3685,23 @@ static void rbd_dev_update_size(struct rbd_device *rbd_dev)
 static int rbd_dev_refresh(struct rbd_device *rbd_dev)
 {
 	u64 mapping_size;
+	u64 features;
 	int ret;
 
 	down_write(&rbd_dev->header_rwsem);
 	mapping_size = rbd_dev->mapping.size;
+	features = rbd_dev->mapping.features;
 
 	ret = rbd_dev_header_info(rbd_dev);
+
+	if (ret == -ENXIO || (ret && features != rbd_dev->mapping.features))
+	{
+		rbd_dev->mapping.read_only = 1;
+		rbd_dev->mapping.state = MAP_STATE_DETACHED;
+		rbd_warn(rbd_dev,
+			"detected feature change in mapped device, setting read-only");
+	}
+
 	if (ret)
 		goto out;

@@ -3698,6 +3724,9 @@ static int rbd_dev_refresh(struct rbd_device *rbd_dev)
 
 out:
 	up_write(&rbd_dev->header_rwsem);
+
+	if (rbd_dev->mapping.read_only)
+		set_disk_ro(rbd_dev->disk, 1);
 	if (!ret && mapping_size != rbd_dev->mapping.size)
 		rbd_dev_update_size(rbd_dev);

@@ -4111,47 +4140,6 @@ static int _rbd_dev_v2_snap_size(struct rbd_device *rbd_dev, u64 snap_id,
 	return 0;
 }
 
-static int rbd_dev_v2_image_size(struct rbd_device *rbd_dev)
-{
-	return _rbd_dev_v2_snap_size(rbd_dev, CEPH_NOSNAP,
-					&rbd_dev->header.obj_order,
-					&rbd_dev->header.image_size);
-}
-
-static int rbd_dev_v2_object_prefix(struct rbd_device *rbd_dev)
-{
-	void *reply_buf;
-	int ret;
-	void *p;
-
-	reply_buf = kzalloc(RBD_OBJ_PREFIX_LEN_MAX, GFP_KERNEL);
-	if (!reply_buf)
-		return -ENOMEM;
-
-	ret = rbd_obj_method_sync(rbd_dev, rbd_dev->header_name,
-				"rbd", "get_object_prefix", NULL, 0,
-				reply_buf, RBD_OBJ_PREFIX_LEN_MAX);
-	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
-	if (ret < 0)
-		goto out;
-
-	p = reply_buf;
-	rbd_dev->header.object_prefix = ceph_extract_encoded_string(&p,
-						p + ret, NULL, GFP_NOIO);
-	ret = 0;
-
-	if (IS_ERR(rbd_dev->header.object_prefix)) {
-		ret = PTR_ERR(rbd_dev->header.object_prefix);
-		rbd_dev->header.object_prefix = NULL;
-	} else {
-		dout("  object_prefix = %s\n", rbd_dev->header.object_prefix);
-	}
-out:
-	kfree(reply_buf);
-
-	return ret;
-}
-
 static int _rbd_dev_v2_snap_features(struct rbd_device *rbd_dev, u64 snap_id,
 		u64 *snap_features)
 {

@@ -4187,12 +4175,6 @@ static int _rbd_dev_v2_snap_features(struct rbd_device *rbd_dev, u64 snap_id,
 	return 0;
 }
 
-static int rbd_dev_v2_features(struct rbd_device *rbd_dev)
-{
-	return _rbd_dev_v2_snap_features(rbd_dev, CEPH_NOSNAP,
-						&rbd_dev->header.features);
-}
-
 static int rbd_dev_v2_parent_info(struct rbd_device *rbd_dev)
 {
 	struct rbd_spec *parent_spec;

@@ -4549,78 +4531,6 @@ out_err:
 	return ret;
 }
 
-static int rbd_dev_v2_snap_context(struct rbd_device *rbd_dev)
-{
-	size_t size;
-	int ret;
-	void *reply_buf;
-	void *p;
-	void *end;
-	u64 seq;
-	u32 snap_count;
-	struct ceph_snap_context *snapc;
-	u32 i;
-
-	/*
-	 * We'll need room for the seq value (maximum snapshot id),
-	 * snapshot count, and array of that many snapshot ids.
-	 * For now we have a fixed upper limit on the number we're
-	 * prepared to receive.
-	 */
-	size = sizeof (__le64) + sizeof (__le32) +
-			RBD_MAX_SNAP_COUNT * sizeof (__le64);
-	reply_buf = kzalloc(size, GFP_KERNEL);
-	if (!reply_buf)
-		return -ENOMEM;
-
-	ret = rbd_obj_method_sync(rbd_dev, rbd_dev->header_name,
-				"rbd", "get_snapcontext", NULL, 0,
-				reply_buf, size);
-	dout("%s: rbd_obj_method_sync returned %d\n", __func__, ret);
-	if (ret < 0)
-		goto out;
-
-	p = reply_buf;
-	end = reply_buf + ret;
-	ret = -ERANGE;
-	ceph_decode_64_safe(&p, end, seq, out);
-	ceph_decode_32_safe(&p, end, snap_count, out);
-
-	/*
-	 * Make sure the reported number of snapshot ids wouldn't go
-	 * beyond the end of our buffer.  But before checking that,
-	 * make sure the computed size of the snapshot context we
-	 * allocate is representable in a size_t.
-	 */
-	if (snap_count > (SIZE_MAX - sizeof (struct ceph_snap_context))
-				 / sizeof (u64)) {
-		ret = -EINVAL;
-		goto out;
-	}
-	if (!ceph_has_room(&p, end, snap_count * sizeof (__le64)))
-		goto out;
-	ret = 0;
-
-	snapc = ceph_create_snap_context(snap_count, GFP_KERNEL);
-	if (!snapc) {
-		ret = -ENOMEM;
-		goto out;
-	}
-	snapc->seq = seq;
-	for (i = 0; i < snap_count; i++)
-		snapc->snaps[i] = ceph_decode_64(&p);
-
-	ceph_put_snap_context(rbd_dev->header.snapc);
-	rbd_dev->header.snapc = snapc;
-
-	dout("  snap context seq = %llu, snap_count = %u\n",
-		(unsigned long long)seq, (unsigned int)snap_count);
-out:
-	kfree(reply_buf);
-
-	return ret;
-}
-
 static const char *rbd_dev_v2_snap_name(struct rbd_device *rbd_dev,
 					u64 snap_id)
 {

@@ -4662,6 +4572,16 @@ out:
 	return snap_name;
 }
 
+static int rbd_dev_v2_mutable_metadata(struct rbd_device *rbd_dev)
+{
+	return rbd_dev_v2_header_data(rbd_dev);
+}
+
+static int rbd_dev_v2_immutable_metadata(struct rbd_device *rbd_dev)
+{
+	return rbd_dev_v2_header_data(rbd_dev);
+}
+
 static int rbd_dev_v2_header_info(struct rbd_device *rbd_dev)
 {
 	bool first_time = rbd_dev->header.object_prefix == NULL;

@@ -4669,27 +4589,39 @@ static int rbd_dev_v2_header_info(struct rbd_device *rbd_dev)
 
 	if (!first_time)
 	{
-		ret = rbd_dev_v2_image_size(rbd_dev);
+		ret = rbd_dev_v2_mutable_metadata(rbd_dev);
 		if (ret)
-			return ret;
-
-		ret = rbd_dev_v2_snap_context(rbd_dev);
-		dout("rbd_dev_v2_snap_context returned %d\n", ret);
-		return ret;
+			goto out_err;
 	}
 	else
 	{
-		ret = rbd_dev_v2_header_onetime(rbd_dev);
+		ret = rbd_dev_v2_immutable_metadata(rbd_dev);
 		if (ret)
-			return ret;
+			goto out_err;
 	}
-	return ret; /* XXX change logic? */
+
+	if (rbd_dev->header.features & RBD_FEATURE_STRIPINGV2)
+	{
+		ret = rbd_dev_v2_striping_info(rbd_dev);
+		if (ret)
+			goto out_err;
+	}
+
+	ret = 0;
+out_err:
+	rbd_dev->header.features = 0;
+	kfree(rbd_dev->header.object_prefix);
+	rbd_dev->header.object_prefix = NULL;
+
+	return ret;
 }
 
 static int rbd_dev_header_info(struct rbd_device *rbd_dev)
 {
 	rbd_assert(rbd_image_format_valid(rbd_dev->image_format));
 
+	/* XXX need to fix 11418 here too? */
+
 	if (rbd_dev->image_format == 1)
 		return rbd_dev_v1_header_info(rbd_dev);

@@ -5139,7 +5071,6 @@ static int rbd_obj_header_method_add(struct ceph_osd_request *osd_req,
 	if (outbound_size) {
 		struct ceph_pagelist *pagelist;
 		pagelist = kmalloc(sizeof(*pagelist), GFP_NOFS);
-		/* XXX is this ever freed? */
 		if (!pagelist)
 			return -ENOMEM;

@@ -5279,6 +5210,12 @@ static int __extract_snapcontext(struct rbd_device *rbd_dev,
 	int i;
 	int ret;
 
+	/*
+	 * We'll need room for the seq value (maximum snapshot id),
+	 * snapshot count, and array of that many snapshot ids.
+	 * For now we have a fixed upper limit on the number we're
+	 * prepared to receive.
+	 */
 	snapc_max = sizeof(__le64) + sizeof(__le32) +
 		RBD_MAX_SNAP_COUNT * sizeof(__le64);

@@ -5297,6 +5234,12 @@ static int __extract_snapcontext(struct rbd_device *rbd_dev,
 	ceph_decode_64_safe(&p, q, seq, out_err);
 	ceph_decode_32_safe(&p, q, snap_count, out_err);
 
+	/*
+	 * Make sure the reported number of snapshot ids wouldn't go
+	 * beyond the end of our buffer.  But before checking that,
+	 * make sure the computed size of the snapshot context we
+	 * allocate is representable in a size_t.
+	 */
 	if (snap_count > (SIZE_MAX - sizeof(struct ceph_snap_context)) /
 		sizeof(u64)) {
 		ret = -EINVAL;

@@ -5339,7 +5282,7 @@ static int __extract_size(struct rbd_device *rbd_dev,
 	return 0;
 }
 
-static int rbd_dev_v2_header_onetime(struct rbd_device *rbd_dev)
+static int rbd_dev_v2_header_data(struct rbd_device *rbd_dev)
 {
 	int ret;

@@ -5712,6 +5655,8 @@ static ssize_t do_rbd_add(struct bus_type *bus,
 		read_only = true;
 	rbd_dev->mapping.read_only = read_only;
 
+	rbd_dev->mapping.state = MAP_STATE_MAPPED;
+
 	rc = rbd_dev_device_setup(rbd_dev);
 	if (rc) {
 		/*

Signature · 1 line

1.9.3

Alex Elder

2015-04-26 06:29 UTC #4 lore raw

In reply to Douglas Fuller

Quoted 4 lines · Douglas Fuller

Support multiple class op calls in one ceph_msg and consolidate rbd header read and refresh processes to use this feature to reduce the number of ceph_msgs sent for that process. Refresh features on header refresh and begin returning EIO if features have changed since mapping.

I think I understand the motivation for what you're doing here. You ultimately want to pack more ops into a message, which would be really great. Ideally you could pack some arbitrary number (much larger than 3), but it seems to me there'll be some restructuring required to get there.

I am having a hard time understanding your intended implementation though. I've looked mainly at your first patch, and scanned the other two. Let's see if I have it right.

You are adding another hunk of data to a class request, and using it as an intermediate buffer for receiving data.

Currently a class request has three "hunks":
- request_info, a pagelist data item, which contains the class name and method name, and perhaps someday, an argument list (?).
- request_data, which can contain any number of data items of any type, and constitutes outgoing data for a write request.
- response_data, also any number of any type of data items, which constitutes space into which incoming data is placed.

You are adding a fourth:
- chain_data, which is a page vector big enough to hold all incoming response data.

When building up an OSD request, we call osd_req_encode_op() for every op in the request. For a class op, that records the length of the class and method names in the op structure, and then appends those names to the outbound request_data. Following that, the "real" request data (if any) provided with the class op is appended to request_data.

Next (before your patches) whatever buffer space was provided (as a page array, pagelist, or bio) to receive response data is appended to response_data. Prior to this, the class op will have been set up with adequate buffers to directly receive the expected data.

What your first patch seems to do is circumvent this last step, and instead of using the prearranged buffers, you allocate another page vector, big enough to hold the combined size of all response_data data items, and store that in the new chain_data field of an OSD request class op. (Note that this could be in *any* of the ops in an OSD request, though I suspect it's always the first.)

When a response arrives, if the *first* op in the request array is CEPH_OSD_OP_CALL, you "split" the data, which will have been deposited in the "chain_data" page vector. That process involves allocating another buffer sufficient to hold the entirety of the received data. You copy the received data into that buffer and release the chain_data page vector. And then you copy the data from this new buffer back into the original response array, and then free the buffer.

This sounds pretty expensive. For every class operation you are copying the received data two extra times.

It's possible I'm not understanding what you're doing here and that's why I'm writing this now. Before I provide any more specific commentary on your patches, I want to be sure I understand what you're trying to do.

If my understanding *is* correct, I would say you're going about this the wrong way, and I may have some suggestions for a better approach.

Will you please correct me where I'm wrong above, and maybe give a little better high-level explanation of what you're trying to do? I see in patch 1 you mention a problem with short reads, and there may be a simpler fix than that (to begin with). But beyond that, if you're trying to incorporate more ops in a message, there may be better ways to go about that as well.

Thanks.

-Alex

Quoted 14 lines

v2: Edit history and address comments from Mike Christie.
Douglas Fuller (3):
ceph: support multiple class method calls in one ceph_msg
rbd: combine object method calls in header refresh using fewer
ceph_msgs
rbd: re-read features during header refresh and detect changes.
  drivers/block/rbd.c             | 512 +++++++++++++++++++++++++++++-----------
  include/linux/ceph/osd_client.h |   3 +-
  net/ceph/messenger.c            |   4 +
  net/ceph/osd_client.c           |  90 ++++++-
  4 files changed, 462 insertions(+), 147 deletions(-)

Ilya Dryomov

maintainer

2015-04-26 08:44 UTC #5 lore raw

Quoted 81 lines · Alex Elder

Quoted 5 lines · Douglas Fuller
Support multiple class op calls in one ceph_msg and consolidate rbd header read and refresh processes to use this feature to reduce the number of ceph_msgs sent for that process. Refresh features on header refresh and begin returning EIO if features have changed since mapping.
I think I understand the motivation for what you're doing here. You ultimately want to pack more ops into a message, which would be really great. Ideally you could pack some arbitrary number (much larger than 3), but it seems to me there'll be some restructuring required to get there.
I am having a hard time understanding your intended implementation though. I've looked mainly at your first patch, and scanned the other two. Let's see if I have it right.
You are adding another hunk of data to a class request, and using it as an intermediate buffer for receiving data.
Currently a class request has three "hunks":
- request_info, a pagelist data item, which contains the class name and method name, and perhaps someday, an argument list (?).
- request_data, which can contain any number of data items of any type, and constitutes outgoing data for a write request.
- response_data, also any number of any type of data items, which constitutes space into which incoming data is placed.
You are adding a fourth:
- chain_data, which is a page vector big enough to hold all incoming response data.
When building up an OSD request, we call osd_req_encode_op() for every op in the request. For a class op, that records the length of the class and method names in the op structure, and then appends those names to the outbound request_data. Following that, the "real" request data (if any) provided with the class op is appended to request_data.
Next (before your patches) whatever buffer space was provided (as a page array, pagelist, or bio) to receive response data is appended to response_data. Prior to this, the class op will have been set up with adequate buffers to directly receive the expected data.
What your first patch seems to do is circumvent this last step, and instead of using the prearranged buffers, you allocate another page vector, big enough to hold the combined size of all response_data data items, and store that in the new chain_data field of an OSD request class op. (Note that this could be in *any* of the ops in an OSD request, though I suspect it's always the first.)
When a response arrives, if the *first* op in the request array is CEPH_OSD_OP_CALL, you "split" the data, which will have been deposited in the "chain_data" page vector. That process involves allocating another buffer sufficient to hold the entirety of the received data. You copy the received data into that buffer and release the chain_data page vector. And then you copy the data from this new buffer back into the original response array, and then free the buffer.
This sounds pretty expensive. For every class operation you are copying the received data two extra times.
It's possible I'm not understanding what you're doing here and that's why I'm writing this now. Before I provide any more specific commentary on your patches, I want to be sure I understand what you're trying to do.
If my understanding *is* correct, I would say you're going about this the wrong way, and I may have some suggestions for a better approach.

Quoted 7 lines

Will you please correct me where I'm wrong above, and maybe give a little better high-level explanation of what you're trying to do? I see in patch 1 you mention a problem with short reads, and there may be a simpler fix than that (to begin with). But beyond that, if you're trying to incorporate more ops in a message, there may be better ways to go about that as well.

Yeah, the only objective of this was to pack more call ops into an osd_request in order to reduce the number of network RTs during rbd map and refresh. The fundamental problem the first patch is trying to work around is the first ceph_msg_data item consuming all reply buffers instead of just its own. We have to preallocate response buffers and if the preallocated response buffer for the first op is large enough (it always is) it will consume the entire ceph_msg, along with replies to other ops.

My understanding is that the first patch is supposed to be a specific workarond for just that - I think it's noted somewhere that it will break on reads combined with call ops or similar.

I too have my efficiency concerns and I raised them in one of the standups but the argument was that this is only going to be used for header init/refresh, not copyup, so the overhead is negligible. I'm still not sold on the copying everything twice though and was going to try to see if there is a simple way to special case this even more and make the diffstat smaller.

Thanks,

Ilya

Douglas Fuller

author

2015-04-26 14:28 UTC #6 lore raw

Alex,

I think you are correct in both your understanding and your impression of the approach.

Quoted 5 lines · Ilya Dryomov

Quoted 2 lines · Alex Elder
Quoted 1 line · Douglas Fuller

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
Support multiple class op calls in one ceph_msg and consolidate rbd header read and refresh processes to use this feature to reduce the number of ceph_msgs sent for that process. Refresh features on header refresh and begin returning EIO if features have changed since mapping.
This sounds pretty expensive. For every class operation you are copying the received data two extra times.

I’d really like a solution to this, but I don’t think one is available given the constraints.

Quoted 17 lines

Quoted 7 lines
Will you please correct me where I'm wrong above, and maybe give a little better high-level explanation of what you're trying to do? I see in patch 1 you mention a problem with short reads, and there may be a simpler fix than that (to begin with). But beyond that, if you're trying to incorporate more ops in a message, there may be better ways to go about that as well.
Yeah, the only objective of this was to pack more call ops into an osd_request in order to reduce the number of network RTs during rbd map and refresh. The fundamental problem the first patch is trying to work around is the first ceph_msg_data item consuming all reply buffers instead of just its own. We have to preallocate response buffers and if the preallocated response buffer for the first op is large enough (it always is) it will consume the entire ceph_msg, along with replies to other ops.

Quoted 3 lines

My understanding is that the first patch is supposed to be a specific workarond for just that - I think it's noted somewhere that it will break on reads combined with call ops or similar.

That’s correct. This patch only works around that short response issue in this specific corner case. It does not fix cases where call ops returning data could be arbitrarily combined with reads or writes (and apparently they never have been, because that would not work). It should not introduce additional cases of breakage, though.

I was told that the preferred way to proceed for now was to avoid changing the osd_client interface and to handle this case in osd_client instead of the messaging layer. Given those constraints, it was agreed in the standups and on #ceph-devel that it should be done this way.

We can’t know the actual response sizes until they are decoded in osd_client. Therefore, we can’t copy them directly to the user buffers off the wire. That costs us one extra copy. The other copy is required because pagevec.c does not provide an interface for copying arbitrary data between two page vectors.

Quoted 6 lines

I too have my efficiency concerns and I raised them in one of the standups but the argument was that this is only going to be used for header init/refresh, not copyup, so the overhead is negligible. I'm still not sold on the copying everything twice though and was going to try to see if there is a simple way to special case this even more and make the diffstat smaller.

You and I agreed in that particular standup discussion; I don’t like it, either. I would prefer a general-case solution that avoids introducing so much extra overhead, especially for such a small amount of data (really just a few bytes).

If there’s a way to handle this with a better method, I’m all ears. I had thought of taking advantage of the fact that the sum total of all this data will never exceed a single page, but that seems to me to require working around even more functionality for what is, essentially, a corner case.

-Doug

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alex Elder

2015-04-26 14:39 UTC #7 lore raw

Quoted 4 lines · Douglas Fuller

Alex,
I think you are correct in both your understanding and your impression of the approach.

OK, good to hear.

Quoted 13 lines

Quoted 13 lines
On Apr 26, 2015, at 4:44 AM, Ilya Dryomov <[email protected]> wrote:
Quoted 8 lines · Alex Elder
Quoted 6 lines · Douglas Fuller
Support multiple class op calls in one ceph_msg and consolidate rbd header read and refresh processes to use this feature to reduce the number of ceph_msgs sent for that process. Refresh features on header refresh and begin returning EIO if features have changed since mapping.

Quoted 5 lines

Quoted 2 lines
Quoted 2 lines
This sounds pretty expensive. For every class operation you are copying the received data two extra times.
I’d really like a solution to this, but I don’t think one is available given the constraints.

Now that I know I understand it, I will at least provide some review comments on the patches. I'll also think about it a little. My instinct says there should be no need to make the copies, but the devil will be in the details.

Quoted 20 lines

Quoted 16 lines
Quoted 6 lines
Will you please correct me where I'm wrong above, and maybe give a little better high-level explanation of what you're trying to do? I see in patch 1 you mention a problem with short reads, and there may be a simpler fix than that (to begin with). But beyond that, if you're trying to incorporate more ops in a message, there may be better ways to go about that as well.
Yeah, the only objective of this was to pack more call ops into an osd_request in order to reduce the number of network RTs during rbd map and refresh. The fundamental problem the first patch is trying to work around is the first ceph_msg_data item consuming all reply buffers instead of just its own. We have to preallocate response buffers and if the preallocated response buffer for the first op is large enough (it always is) it will consume the entire ceph_msg, along with replies to other ops.
Quoted 3 lines
My understanding is that the first patch is supposed to be a specific workarond for just that - I think it's noted somewhere that it will break on reads combined with call ops or similar.

Quoted 5 lines

That’s correct. This patch only works around that short response issue in this specific corner case. It does not fix cases where call ops returning data could be arbitrarily combined with reads or writes (and apparently they never have been, because that would not work). It should not introduce additional cases of breakage, though.

I knew when we added support for copyup that we were kind of kludging things to allow *just one* more op in a message. Then the discard support added another, but in both of these cases it was not done in a nice, general way. The "right" way is to generalize it, but if that was easy it would have been done by now.

Quoted 5 lines

I was told that the preferred way to proceed for now was to avoid changing the osd_client interface and to handle this case in osd_client instead of the messaging layer. Given those constraints, it was agreed in the standups and on #ceph-devel that it should be done this way.

Sorry, I haven't been keeping up with all the activity on the mailing list. I pay closest attention to patches to the kernel code.

Quoted 5 lines

We can’t know the actual response sizes until they are decoded in osd_client. Therefore, we can’t copy them directly to the user buffers off the wire. That costs us one extra copy. The other copy is required because pagevec.c does not provide an interface for copying arbitrary data between two page vectors.

That could probably be changed, but that's a separate issue (and somewhat out of your hands as far as getting it done when you need it).

Quoted 12 lines

Quoted 6 lines
I too have my efficiency concerns and I raised them in one of the standups but the argument was that this is only going to be used for header init/refresh, not copyup, so the overhead is negligible. I'm still not sold on the copying everything twice though and was going to try to see if there is a simple way to special case this even more and make the diffstat smaller.
You and I agreed in that particular standup discussion; I don’t like it, either. I would prefer a general-case solution that avoids introducing so much extra overhead, especially for such a small amount of data (really just a few bytes).

Quoted 5 lines

If there’s a way to handle this with a better method, I’m all ears. I had thought of taking advantage of the fact that the sum total of all this data will never exceed a single page, but that seems to me to require working around even more functionality for what is, essentially, a corner case.

OK, well I'll go look at that code path again to see if I can come up with any bright ideas.

Thanks for the explanation.

-Alex

Quoted 3 lines

-Doug

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Douglas Fuller

author

2015-04-26 15:07 UTC #8 lore raw

Quoted 8 lines

Quoted 3 lines
I’d really like a solution to this, but I don’t think one is available given the constraints.
Now that I know I understand it, I will at least provide some review comments on the patches. I'll also think about it a little. My instinct says there should be no need to make the copies, but the devil will be in the details.

Yeah, it seems like it should be simpler. To fix it, I think we would have to either commit a layering violation (decode the call ops’ response sizes in the message receive flow) or change the interface to osd_client (don’t provide preallocated pagevecs for call response data even though that’s what’s done for other types of ops). I’m the new guy, though, so it’s certainly possible I’m missing something.

Quoted 9 lines

Quoted 5 lines
I was told that the preferred way to proceed for now was to avoid changing the osd_client interface and to handle this case in osd_client instead of the messaging layer. Given those constraints, it was agreed in the standups and on #ceph-devel that it should be done this way.
Sorry, I haven't been keeping up with all the activity on the mailing list. I pay closest attention to patches to the kernel code.

Well, this was worked out mainly in the standups and on IRC. There wasn’t a mailing list discussion on it. Having said that, the attention to the patch is certainly appreciated.

Quoted 3 lines

OK, well I'll go look at that code path again to see if I can
come up with any bright ideas.

Thanks very much.

-Doug--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sage Weil

2015-04-27 04:15 UTC #9 lore raw

Quoted 16 lines · Douglas Fuller

Quoted 7 lines
Quoted 2 lines
I?d really like a solution to this, but I don?t think one is available given the constraints.
Now that I know I understand it, I will at least provide some review comments on the patches. I'll also think about it a little. My instinct says there should be no need to make the copies, but the devil will be in the details.
Yeah, it seems like it should be simpler. To fix it, I think we would have to either commit a layering violation (decode the call ops? response sizes in the message receive flow) or change the interface to osd_client (don?t provide preallocated pagevecs for call response data even though that?s what?s done for other types of ops). I?m the new guy, though, so it?s certainly possible I?m missing something.
Quoted 6 lines
Quoted 5 lines
I was told that the preferred way to proceed for now was to avoid changing the osd_client interface and to handle this case in osd_client instead of the messaging layer. Given those constraints, it was agreed in the standups and on #ceph-devel that it should be done this way.

Quoted 5 lines

Quoted 3 lines
Sorry, I haven't been keeping up with all the activity on the mailing list. I pay closest attention to patches to the kernel code.
Well, this was worked out mainly in the standups and on IRC. There wasn?t a mailing list discussion on it. Having said that, the attention to the patch is certainly appreciated.

Two basic approaches come to mind:

1) add an up-call between the front and data portions. Right now we have a get_reply between header and front that is used to get the buffers for front (and data). We could add a second that comes after front to prepare the data buffers. This would basically change teh sequence from

- read header
- get/allocate ceph_msg, along with front, middle, and data buffers
- read front, middle, data
- dispatch message
- decode
- process

- read header
- get/allocate ceph_msg, along with front, maybe middle, and data buffers
- read front, middle
- call get_data hook
- decode front, prepare/allocate data buffers
- read data
- dispatch message
- process

I think that's a viable approach, but it means a fair bit of refactoring to separate the decoding from the processing (at least for osd_reply)--right now those two steps are combined in all of the handle_* functions.

2) Introduce a protocol change so that the section boundaries are described in a generic way. Even more work!

Given the only uesrs we have in mind have very small data payloads, copying seems simplest to me, but it's up to you guys. Maybe there is something much simpler I'm missing? Somehow we have to do the front parsing before reading data... and probably we want to avoid parsing it twice...

sage

Douglas Fuller

author

2015-04-27 04:35 UTC #10 lore raw

Quoted 31 lines · Sage Weil

Two basic approaches come to mind:
1) add an up-call between the front and data portions. Right now we have a get_reply between header and front that is used to get the buffers for front (and data). We could add a second that comes after front to prepare the data buffers. This would basically change teh sequence from
- read header
- get/allocate ceph_msg, along with front, middle, and data buffers
- read front, middle, data
- dispatch message
- decode
- process
to
- read header
- get/allocate ceph_msg, along with front, maybe middle, and data buffers
- read front, middle
- call get_data hook
- decode front, prepare/allocate data buffers
- read data
- dispatch message
- process

Quoted 4 lines

I think that's a viable approach, but it means a fair bit of refactoring to separate the decoding from the processing (at least for osd_reply)--right now those two steps are combined in all of the handle_* functions.

In the kernel client, the data buffers are allocated before the message is even sent. The messaging code reads the reply data directly into those preallocated buffers. If we add a call up to decode the message size and allocate the data buffers when we process the reply, we have to change the osd_client interface.

Quoted 2 lines

2) Introduce a protocol change so that the section boundaries are described in a generic way. Even more work!

We could also pad the relevant op replies so that each has a fixed length. It would be a protocol change, but a smaller one, I assume.

Quoted 5 lines

Given the only uesrs we have in mind have very small data payloads, copying seems simplest to me, but it's up to you guys. Maybe there is something much simpler I'm missing? Somehow we have to do the front parsing before reading data... and probably we want to avoid parsing it twice...

This solution just feels hacky and inefficient, so I think there is a desire to feel like we at least tried to come up something simpler and more efficient before proceeding.

-Doug

Alex Elder

2015-04-27 04:40 UTC #11 lore raw

Quoted 3 lines · Douglas Fuller

This solution just feels hacky and inefficient, so I think there is a desire to feel like we at least tried to come up something simpler and more efficient before proceeding.

Right. And somehow more fitting with the existing code, if that's possible. There are a few things in this proposal that are just special case code, and it would be nice to avoid that.

I'm sorry I've been too busy today to get back to this. I spent a lot of time on Saturday getting ramped back up on this code so I could provide a decent review...

I'm going to at least look at the code (error on a read op) before going to bed, so I at least know what the problem is. I'll try to have something to say (even if it's "I've got nothing") in the next day or two.

-Alex

Alex Elder

2015-05-07 15:29 UTC #12 lore raw

Quoted 14 lines · Alex Elder

Quoted 3 lines · Douglas Fuller
This solution just feels hacky and inefficient, so I think there is a desire to feel like we at least tried to come up something simpler and more efficient before proceeding.
Right. And somehow more fitting with the existing code, if that's possible. There are a few things in this proposal that are just special case code, and it would be nice to avoid that.
I'm sorry I've been too busy today to get back to this. I spent a lot of time on Saturday getting ramped back up on this code so I could provide a decent review...

Quoted 6 lines

I'm going to at least look at the code (error on a read op) before going to bed, so I at least know what the problem is. I'll try to have something to say (even if it's "I've got nothing") in the next day or two.
-Alex

It's been a week and a half since I sent this. I don't expect to be able to contribute anything for at least a few more days.

If adding this functionality is something you need quickly, I don't want you to be waiting for me.

If you can wait, I will gladly review it and offer my suggestions for a better way (if I can think of one...).

But if not, you should just proceed with what you have if it solves the problem.

I just don't want to keep you waiting.

-Alex

Douglas Fuller

author

2015-05-07 16:48 UTC #13 lore raw

Alex,

Thanks for circling back.

We should prioritize correctness and accuracy over promptness here; this isn’t a critical feature and I’d like my first submission to receive a thorough review.

Thanks,
—Doug

Quoted 34 lines · Alex Elder

Quoted 14 lines · Alex Elder
Quoted 3 lines · Douglas Fuller
This solution just feels hacky and inefficient, so I think there is a desire to feel like we at least tried to come up something simpler and more efficient before proceeding.
Right. And somehow more fitting with the existing code, if that's possible. There are a few things in this proposal that are just special case code, and it would be nice to avoid that.
I'm sorry I've been too busy today to get back to this. I spent a lot of time on Saturday getting ramped back up on this code so I could provide a decent review...
Quoted 6 lines
I'm going to at least look at the code (error on a read op) before going to bed, so I at least know what the problem is. I'll try to have something to say (even if it's "I've got nothing") in the next day or two.
-Alex
It's been a week and a half since I sent this. I don't expect to be able to contribute anything for at least a few more days.
If adding this functionality is something you need quickly, I don't want you to be waiting for me.
If you can wait, I will gladly review it and offer my suggestions for a better way (if I can think of one...).

Quoted 6 lines

But if not, you should just proceed with what you have if it solves the problem.
I just don't want to keep you waiting.
-Alex

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html