[PATCHSET 0/4] libosd: scatter gather commands and stuff for 2.6.37

[PATCHSET 0/4] libosd: scatter gather commands and stuff for 2.6.37

[Boaz Harrosh]

Hi James.

Please submit the following patches hopefully for the 2.6.37 merge window.
2 first patches are just bug fixes. The Last two patches enable the
scatter gather OSD operations. These will only be used in filesystem code
at the 2.6.38 Kernel but it could be nice to have them in before hand, so
to avoid trees dependencies.

The original scatter gather work was done by John Chandy. I have reworked
most of it, to better fit with the rest of the library, and the design
decisions made there. I have also removed some fixture support that was
originally available, for lack of any users.

I must say that John's original work had some merit and robustness to it
but higher complexity and slower execution in the way the library is used
today. If we want his system we should enable it for all segments not
only the continuation segment. To get people up to speed. John's work
enabled a bio-list for the continuation segment. But the rest of the
code assumes a single allocation, single bio_map_kern for each segment.
So to match the other segments handling I converted the continuation 
segment to a flat single-allocation as well.

The patches are:
[PATCH 1/4] libosd: Fix bug in attr_page handling
[PATCH 2/4] libosd: Free resources in reverse order of allocation

	These two should go in regardless

[PATCH 3/4] libosd: Support for scatter gather write/read commands

	Please submit this patch as well I will need it in the RAID5
	support for exofs. (Read without XOR blocks)

[PATCH 4/4] libosd: write/read_sg_kern API

	This patch is currently only used in the out-of-tree osd_ktest.ko
	module. I can keep it in that tree if needed until Kernel users
	come up. Please advise.

Thanks
Boaz

[PATCH 1/4] libosd: Fix bug in attr_page handling

[Boaz Harrosh]

The _osd_req_finalize_attr_page was off by a mile, when trying to
append the enc_get_attr segment instead of the proper set_attr segment.

Also properly support when we don't have any attribute to set while
getting a full page. And when clearing an attribute by setting it's
size to zero.

Reported-by: John Chandy <john.chandy@uconn.edu>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/osd/osd_initiator.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index e88bbdd..771ab12 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1218,17 +1218,18 @@ int osd_req_add_get_attr_page(struct osd_request *or,
 	or->get_attr.buff = attar_page;
 	or->get_attr.total_bytes = max_page_len;
 
-	or->set_attr.buff = set_one_attr->val_ptr;
-	or->set_attr.total_bytes = set_one_attr->len;
-
 	cdbh->attrs_page.get_attr_page = cpu_to_be32(page_id);
 	cdbh->attrs_page.get_attr_alloc_length = cpu_to_be32(max_page_len);
-	/* ocdb->attrs_page.get_attr_offset; */
+
+	if (!set_one_attr || !set_one_attr->attr_page)
+		return 0; /* The set is optional */
+
+	or->set_attr.buff = set_one_attr->val_ptr;
+	or->set_attr.total_bytes = set_one_attr->len;
 
 	cdbh->attrs_page.set_attr_page = cpu_to_be32(set_one_attr->attr_page);
 	cdbh->attrs_page.set_attr_id = cpu_to_be32(set_one_attr->attr_id);
 	cdbh->attrs_page.set_attr_length = cpu_to_be32(set_one_attr->len);
-	/* ocdb->attrs_page.set_attr_offset; */
 	return 0;
 }
 EXPORT_SYMBOL(osd_req_add_get_attr_page);
@@ -1248,11 +1249,14 @@ static int _osd_req_finalize_attr_page(struct osd_request *or)
 	if (ret)
 		return ret;
 
+	if (or->set_attr.total_bytes == 0)
+		return 0;
+
 	/* set one value */
 	cdbh->attrs_page.set_attr_offset =
 		osd_req_encode_offset(or, or->out.total_bytes, &out_padding);
 
-	ret = _req_append_segment(or, out_padding, &or->enc_get_attr, NULL,
+	ret = _req_append_segment(or, out_padding, &or->set_attr, NULL,
 				  &or->out);
 	return ret;
 }
-- 
1.7.2.3

[PATCH 2/4] libosd: Free resources in reverse order of allocation

[Boaz Harrosh]

At osd_end_request first free the request that might
point to pages, then free these pages. In reverse order
of allocation. For now it's just anal neatness. When we'll
use mempools It'll also pay in performance.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/osd/osd_initiator.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index 771ab12..acbdcb6 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -452,10 +452,6 @@ void osd_end_request(struct osd_request *or)
 {
 	struct request *rq = or->request;
 
-	_osd_free_seg(or, &or->set_attr);
-	_osd_free_seg(or, &or->enc_get_attr);
-	_osd_free_seg(or, &or->get_attr);
-
 	if (rq) {
 		if (rq->next_rq) {
 			_put_request(rq->next_rq);
@@ -464,6 +460,11 @@ void osd_end_request(struct osd_request *or)
 
 		_put_request(rq);
 	}
+
+	_osd_free_seg(or, &or->get_attr);
+	_osd_free_seg(or, &or->enc_get_attr);
+	_osd_free_seg(or, &or->set_attr);
+
 	_osd_request_free(or);
 }
 EXPORT_SYMBOL(osd_end_request);
-- 
1.7.2.3

[PATCH 3/4] libosd: Support for scatter gather write/read commands

[Boaz Harrosh]

This patch adds the Scatter-Gather (sg) API to libosd.
Scatter-gather enables a write/read of multiple none-contiguous
areas of an object, in a single call. The extents may overlap
and/or be in any order.

The Scatter-Gather list is sent to the target in what is called
a "cdb continuation segment". This is yet another possible segment
in the osd-out-buffer. It is unlike all other segments in that it
sits before the actual "data" segment (which until now was always
first), and that it is signed by itself and not part of the data
buffer. This is because the cdb-continuation-segment is considered
a spill-over of the CDB data, and is therefor signed under
OSD_SEC_CAPKEY and higher.

TODO: A new osd_finalize_request_ex version should be supplied so
the @caps received on the network also contains a size parameter
and can be spilled over into the "cdb continuation segment".

Thanks to John Chandy <john.chandy@uconn.edu> for the original
code, and investigations. And the implementation of SG support
in the osd-target.

Original-coded-by: John Chandy <john.chandy@uconn.edu>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/osd/osd_initiator.c |  148 ++++++++++++++++++++++++++++++++++++--
 include/scsi/osd_initiator.h     |    9 ++-
 include/scsi/osd_protocol.h      |   42 +++++++++++
 include/scsi/osd_types.h         |    5 ++
 4 files changed, 198 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index acbdcb6..f5b5735 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -464,6 +464,7 @@ void osd_end_request(struct osd_request *or)
 	_osd_free_seg(or, &or->get_attr);
 	_osd_free_seg(or, &or->enc_get_attr);
 	_osd_free_seg(or, &or->set_attr);
+	_osd_free_seg(or, &or->cdb_cont);
 
 	_osd_request_free(or);
 }
@@ -548,6 +549,12 @@ static int _osd_realloc_seg(struct osd_request *or,
 	return 0;
 }
 
+static int _alloc_cdb_cont(struct osd_request *or, unsigned total_bytes)
+{
+	OSD_DEBUG("total_bytes=%d\n", total_bytes);
+	return _osd_realloc_seg(or, &or->cdb_cont, total_bytes);
+}
+
 static int _alloc_set_attr_list(struct osd_request *or,
 	const struct osd_attr *oa, unsigned nelem, unsigned add_bytes)
 {
@@ -886,6 +893,128 @@ int osd_req_read_kern(struct osd_request *or,
 }
 EXPORT_SYMBOL(osd_req_read_kern);
 
+static int _add_sg_continuation_descriptor(struct osd_request *or,
+	const struct osd_sg_entry *sglist, unsigned numentries, u64 *len)
+{
+	struct osd_sg_continuation_descriptor *oscd;
+	u32 oscd_size;
+	unsigned i;
+	int ret;
+
+	oscd_size = sizeof(*oscd) + numentries * sizeof(oscd->entries[0]);
+
+	if (!or->cdb_cont.total_bytes) {
+		/* First time, jump over the header, we will write to:
+		 *	cdb_cont.buff + cdb_cont.total_bytes
+		 */
+		or->cdb_cont.total_bytes =
+				sizeof(struct osd_continuation_segment_header);
+	}
+
+	ret = _alloc_cdb_cont(or, or->cdb_cont.total_bytes + oscd_size);
+	if (unlikely(ret))
+		return ret;
+
+	oscd = or->cdb_cont.buff + or->cdb_cont.total_bytes;
+	oscd->hdr.type = cpu_to_be16(SCATTER_GATHER_LIST);
+	oscd->hdr.pad_length = 0;
+	oscd->hdr.length = cpu_to_be32(oscd_size - sizeof(*oscd));
+
+	*len = 0;
+	/* copy the sg entries and convert to network byte order */
+	for (i = 0; i < numentries; i++) {
+		oscd->entries[i].offset = cpu_to_be64(sglist[i].offset);
+		oscd->entries[i].len    = cpu_to_be64(sglist[i].len);
+		*len += sglist[i].len;
+	}
+
+	or->cdb_cont.total_bytes += oscd_size;
+	OSD_DEBUG("total_bytes=%d oscd_size=%d numentries=%d\n",
+		  or->cdb_cont.total_bytes, oscd_size, numentries);
+	return 0;
+}
+
+static int _osd_req_finalize_cdb_cont(struct osd_request *or, const u8 *cap_key)
+{
+	struct request_queue *req_q = osd_request_queue(or->osd_dev);
+	struct bio *bio;
+	struct osd_cdb_head *cdbh = osd_cdb_head(&or->cdb);
+	struct osd_continuation_segment_header *cont_seg_hdr;
+
+	if (!or->cdb_cont.total_bytes)
+		return 0;
+
+	cont_seg_hdr = or->cdb_cont.buff;
+	cont_seg_hdr->format = CDB_CONTINUATION_FORMAT_V2;
+	cont_seg_hdr->service_action = cdbh->varlen_cdb.service_action;
+
+	/* create a bio for continuation segment */
+	bio = bio_map_kern(req_q, or->cdb_cont.buff, or->cdb_cont.total_bytes,
+			   GFP_KERNEL);
+	if (unlikely(!bio))
+		return -ENOMEM;
+
+	bio->bi_rw |= REQ_WRITE;
+
+	/* integrity check the continuation before the bio is linked
+	 * with the other data segments since the continuation
+	 * integrity is separate from the other data segments.
+	 */
+	osd_sec_sign_data(cont_seg_hdr->integrity_check, bio, cap_key);
+
+	cdbh->v2.cdb_continuation_length = cpu_to_be32(or->cdb_cont.total_bytes);
+
+	/* we can't use _req_append_segment, because we need to link in the
+	 * continuation bio to the head of the bio list - the
+	 * continuation segment (if it exists) is always the first segment in
+	 * the out data buffer.
+	 */
+	bio->bi_next = or->out.bio;
+	or->out.bio = bio;
+	or->out.total_bytes += or->cdb_cont.total_bytes;
+
+	return 0;
+}
+
+/* osd_req_write_sg: Takes a @bio that points to the data out buffer and an
+ * @sglist that has the scatter gather entries. Scatter-gather enables a write
+ * of multiple none-contiguous areas of an object, in a single call. The extents
+ * may overlap and/or be in any order. The only constrain is that:
+ *	total_bytes(sglist) >= total_bytes(bio)
+ */
+int osd_req_write_sg(struct osd_request *or,
+	const struct osd_obj_id *obj, struct bio *bio,
+	const struct osd_sg_entry *sglist, unsigned numentries)
+{
+	u64 len;
+	int ret = _add_sg_continuation_descriptor(or, sglist, numentries, &len);
+
+	if (ret)
+		return ret;
+	osd_req_write(or, obj, 0, bio, len);
+
+	return 0;
+}
+EXPORT_SYMBOL(osd_req_write_sg);
+
+/* osd_req_read_sg: Read multiple extents of an object into @bio
+ * See osd_req_write_sg
+ */
+int osd_req_read_sg(struct osd_request *or,
+	const struct osd_obj_id *obj, struct bio *bio,
+	const struct osd_sg_entry *sglist, unsigned numentries)
+{
+	u64 len;
+	int ret = _add_sg_continuation_descriptor(or, sglist, numentries, &len);
+
+	if (ret)
+		return ret;
+	osd_req_read(or, obj, 0, bio, len);
+
+	return 0;
+}
+EXPORT_SYMBOL(osd_req_read_sg);
+
 void osd_req_get_attributes(struct osd_request *or,
 	const struct osd_obj_id *obj)
 {
@@ -1281,7 +1410,8 @@ static inline void osd_sec_parms_set_in_offset(bool is_v1,
 }
 
 static int _osd_req_finalize_data_integrity(struct osd_request *or,
-	bool has_in, bool has_out, u64 out_data_bytes, const u8 *cap_key)
+	bool has_in, bool has_out, struct bio *out_data_bio, u64 out_data_bytes,
+	const u8 *cap_key)
 {
 	struct osd_security_parameters *sec_parms = _osd_req_sec_params(or);
 	int ret;
@@ -1312,7 +1442,7 @@ static int _osd_req_finalize_data_integrity(struct osd_request *or,
 		or->out.last_seg = NULL;
 
 		/* they are now all chained to request sign them all together */
-		osd_sec_sign_data(&or->out_data_integ, or->out.req->bio,
+		osd_sec_sign_data(&or->out_data_integ, out_data_bio,
 				  cap_key);
 	}
 
@@ -1408,6 +1538,8 @@ int osd_finalize_request(struct osd_request *or,
 {
 	struct osd_cdb_head *cdbh = osd_cdb_head(&or->cdb);
 	bool has_in, has_out;
+	 /* Save for data_integrity without the cdb_continuation */
+	struct bio *out_data_bio = or->out.bio;
 	u64 out_data_bytes = or->out.total_bytes;
 	int ret;
 
@@ -1423,9 +1555,14 @@ int osd_finalize_request(struct osd_request *or,
 	osd_set_caps(&or->cdb, cap);
 
 	has_in = or->in.bio || or->get_attr.total_bytes;
-	has_out = or->out.bio || or->set_attr.total_bytes ||
-		or->enc_get_attr.total_bytes;
+	has_out = or->out.bio || or->cdb_cont.total_bytes ||
+		or->set_attr.total_bytes || or->enc_get_attr.total_bytes;
 
+	ret = _osd_req_finalize_cdb_cont(or, cap_key);
+	if (ret) {
+		OSD_DEBUG("_osd_req_finalize_cdb_cont failed\n");
+		return ret;
+	}
 	ret = _init_blk_request(or, has_in, has_out);
 	if (ret) {
 		OSD_DEBUG("_init_blk_request failed\n");
@@ -1463,7 +1600,8 @@ int osd_finalize_request(struct osd_request *or,
 	}
 
 	ret = _osd_req_finalize_data_integrity(or, has_in, has_out,
-					       out_data_bytes, cap_key);
+					       out_data_bio, out_data_bytes,
+					       cap_key);
 	if (ret)
 		return ret;
 
diff --git a/include/scsi/osd_initiator.h b/include/scsi/osd_initiator.h
index a8f3701..169a5dc 100644
--- a/include/scsi/osd_initiator.h
+++ b/include/scsi/osd_initiator.h
@@ -137,7 +137,7 @@ struct osd_request {
 		void *buff;
 		unsigned alloc_size; /* 0 here means: don't call kfree */
 		unsigned total_bytes;
-	} set_attr, enc_get_attr, get_attr;
+	} cdb_cont, set_attr, enc_get_attr, get_attr;
 
 	struct _osd_io_info {
 		struct bio *bio;
@@ -448,6 +448,13 @@ void osd_req_read(struct osd_request *or,
 int osd_req_read_kern(struct osd_request *or,
 	const struct osd_obj_id *obj, u64 offset, void *buff, u64 len);
 
+/* Scatter/Gather write/read commands */
+int osd_req_write_sg(struct osd_request *or,
+	const struct osd_obj_id *obj, struct bio *bio,
+	const struct osd_sg_entry *sglist, unsigned numentries);
+int osd_req_read_sg(struct osd_request *or,
+	const struct osd_obj_id *obj, struct bio *bio,
+	const struct osd_sg_entry *sglist, unsigned numentries);
 /*
  * Root/Partition/Collection/Object Attributes commands
  */
diff --git a/include/scsi/osd_protocol.h b/include/scsi/osd_protocol.h
index 6856612..a6026da 100644
--- a/include/scsi/osd_protocol.h
+++ b/include/scsi/osd_protocol.h
@@ -631,4 +631,46 @@ static inline void osd_sec_set_caps(struct osd_capability_head *cap,
 	put_unaligned_le16(bit_mask, &cap->permissions_bit_mask);
 }
 
+/* osd2r05a sec 5.3: CDB continuation segment formats */
+enum osd_continuation_segment_format {
+	CDB_CONTINUATION_FORMAT_V2 = 0x01,
+};
+
+struct osd_continuation_segment_header {
+	u8	format;
+	u8	reserved1;
+	__be16	service_action;
+	__be32	reserved2;
+	u8	integrity_check[OSDv2_CRYPTO_KEYID_SIZE];
+} __packed;
+
+/* osd2r05a sec 5.4.1: CDB continuation descriptors */
+enum osd_continuation_descriptor_type {
+	NO_MORE_DESCRIPTORS = 0x0000,
+	SCATTER_GATHER_LIST = 0x0001,
+	QUERY_LIST = 0x0002,
+	USER_OBJECT = 0x0003,
+	COPY_USER_OBJECT_SOURCE = 0x0101,
+	EXTENSION_CAPABILITIES = 0xFFEE
+};
+
+struct osd_continuation_descriptor_header {
+	__be16	type;
+	u8	reserved;
+	u8	pad_length;
+	__be32	length;
+} __packed;
+
+
+/* osd2r05a sec 5.4.2: Scatter/gather list */
+struct osd_sg_list_entry {
+	__be64 offset;
+	__be64 len;
+};
+
+struct osd_sg_continuation_descriptor {
+	struct osd_continuation_descriptor_header hdr;
+	struct osd_sg_list_entry entries[];
+};
+
 #endif /* ndef __OSD_PROTOCOL_H__ */
diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
index 3f5e88c..bd0be7e 100644
--- a/include/scsi/osd_types.h
+++ b/include/scsi/osd_types.h
@@ -37,4 +37,9 @@ struct osd_attr {
 	void *val_ptr;		/* in network order */
 };
 
+struct osd_sg_entry {
+	u64 offset;
+	u64 len;
+};
+
 #endif /* ndef __OSD_TYPES_H__ */
-- 
1.7.2.3

[PATCH 4/4] libosd: write/read_sg_kern API

[Boaz Harrosh]

From: John A. Chandy <john.chandy@uconn.edu>

This is a trivial addition to the SG API that can receive kernel
pointers. It is only used by the out-of-tree test module. So
it's immediate need is questionable. For maintenance ease it might
just get in, as it's very small.

John.
do you need this in the Kernel, or is it only for osd_ktest.ko?

Signed-off-by: John A. Chandy <john.chandy@uconn.edu>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/osd/osd_initiator.c |   71 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index f5b5735..0433ea6 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1015,6 +1015,77 @@ int osd_req_read_sg(struct osd_request *or,
 }
 EXPORT_SYMBOL(osd_req_read_sg);
 
+/* SG-list write/read Kern API
+ *
+ * osd_req_{write,read}_sg_kern takes an array of @buff pointers and an array
+ * of sg_entries. @numentries indicates how many pointers and sg_entries there
+ * are.  By requiring an array of buff pointers. This allows a caller to do a
+ * single write/read and scatter into multiple buffers.
+ * NOTE: Each buffer + len should not cross a page boundary.
+ */
+static struct bio *_create_sg_bios(struct osd_request *or,
+	void **buff, const struct osd_sg_entry *sglist, unsigned numentries)
+{
+	struct request_queue *q = osd_request_queue(or->osd_dev);
+	struct bio *bio;
+	unsigned i;
+
+	bio = bio_kmalloc(GFP_KERNEL, numentries);
+	if (unlikely(!bio)) {
+		OSD_DEBUG("Faild to allocate BIO size=%u\n", numentries);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	for (i = 0; i < numentries; i++) {
+		unsigned offset = offset_in_page(buff[i]);
+		struct page *page = virt_to_page(buff[i]);
+		unsigned len = sglist[i].len;
+		unsigned added_len;
+
+		BUG_ON(offset + len > PAGE_SIZE);
+		added_len = bio_add_pc_page(q, bio, page, len, offset);
+		if (unlikely(len != added_len)) {
+			OSD_DEBUG("bio_add_pc_page len(%d) != added_len(%d)\n",
+				  len, added_len);
+			bio_put(bio);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
+
+	return bio;
+}
+
+int osd_req_write_sg_kern(struct osd_request *or,
+	const struct osd_obj_id *obj, void **buff,
+	const struct osd_sg_entry *sglist, unsigned numentries)
+{
+	struct bio *bio = _create_sg_bios(or, buff, sglist, numentries);
+	if (IS_ERR(bio))
+		return PTR_ERR(bio);
+
+	bio->bi_rw |= REQ_WRITE;
+	osd_req_write_sg(or, obj, bio, sglist, numentries);
+
+	return 0;
+}
+EXPORT_SYMBOL(osd_req_write_sg_kern);
+
+int osd_req_read_sg_kern(struct osd_request *or,
+	const struct osd_obj_id *obj, void **buff,
+	const struct osd_sg_entry *sglist, unsigned numentries)
+{
+	struct bio *bio = _create_sg_bios(or, buff, sglist, numentries);
+	if (IS_ERR(bio))
+		return PTR_ERR(bio);
+
+	osd_req_read_sg(or, obj, bio, sglist, numentries);
+
+	return 0;
+}
+EXPORT_SYMBOL(osd_req_read_sg_kern);
+
+
+
 void osd_req_get_attributes(struct osd_request *or,
 	const struct osd_obj_id *obj)
 {
-- 
1.7.2.3

Re: [PATCH 4/4] libosd: write/read_sg_kern API

[Boaz Harrosh]

On 10/19/2010 02:22 PM, Boaz Harrosh wrote:
> From: John A. Chandy <john.chandy@uconn.edu>
> 
> This is a trivial addition to the SG API that can receive kernel
> pointers. It is only used by the out-of-tree test module. So
> it's immediate need is questionable. For maintenance ease it might
> just get in, as it's very small.
> 
> John.
> do you need this in the Kernel, or is it only for osd_ktest.ko?
> 
> Signed-off-by: John A. Chandy <john.chandy@uconn.edu>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  drivers/scsi/osd/osd_initiator.c |   71 ++++++++++++++++++++++++++++++++++++++

I forgot the header declarations, Resending version 2

Boaz


>  1 files changed, 71 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index f5b5735..0433ea6 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -1015,6 +1015,77 @@ int osd_req_read_sg(struct osd_request *or,
>  }
>  EXPORT_SYMBOL(osd_req_read_sg);
>  
> +/* SG-list write/read Kern API
> + *
> + * osd_req_{write,read}_sg_kern takes an array of @buff pointers and an array
> + * of sg_entries. @numentries indicates how many pointers and sg_entries there
> + * are.  By requiring an array of buff pointers. This allows a caller to do a
> + * single write/read and scatter into multiple buffers.
> + * NOTE: Each buffer + len should not cross a page boundary.
> + */
> +static struct bio *_create_sg_bios(struct osd_request *or,
> +	void **buff, const struct osd_sg_entry *sglist, unsigned numentries)
> +{
> +	struct request_queue *q = osd_request_queue(or->osd_dev);
> +	struct bio *bio;
> +	unsigned i;
> +
> +	bio = bio_kmalloc(GFP_KERNEL, numentries);
> +	if (unlikely(!bio)) {
> +		OSD_DEBUG("Faild to allocate BIO size=%u\n", numentries);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	for (i = 0; i < numentries; i++) {
> +		unsigned offset = offset_in_page(buff[i]);
> +		struct page *page = virt_to_page(buff[i]);
> +		unsigned len = sglist[i].len;
> +		unsigned added_len;
> +
> +		BUG_ON(offset + len > PAGE_SIZE);
> +		added_len = bio_add_pc_page(q, bio, page, len, offset);
> +		if (unlikely(len != added_len)) {
> +			OSD_DEBUG("bio_add_pc_page len(%d) != added_len(%d)\n",
> +				  len, added_len);
> +			bio_put(bio);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +	}
> +
> +	return bio;
> +}
> +
> +int osd_req_write_sg_kern(struct osd_request *or,
> +	const struct osd_obj_id *obj, void **buff,
> +	const struct osd_sg_entry *sglist, unsigned numentries)
> +{
> +	struct bio *bio = _create_sg_bios(or, buff, sglist, numentries);
> +	if (IS_ERR(bio))
> +		return PTR_ERR(bio);
> +
> +	bio->bi_rw |= REQ_WRITE;
> +	osd_req_write_sg(or, obj, bio, sglist, numentries);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(osd_req_write_sg_kern);
> +
> +int osd_req_read_sg_kern(struct osd_request *or,
> +	const struct osd_obj_id *obj, void **buff,
> +	const struct osd_sg_entry *sglist, unsigned numentries)
> +{
> +	struct bio *bio = _create_sg_bios(or, buff, sglist, numentries);
> +	if (IS_ERR(bio))
> +		return PTR_ERR(bio);
> +
> +	osd_req_read_sg(or, obj, bio, sglist, numentries);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(osd_req_read_sg_kern);
> +
> +
> +
>  void osd_req_get_attributes(struct osd_request *or,
>  	const struct osd_obj_id *obj)
>  {

[PATCH 4/4 ver2] libosd: write/read_sg_kern API

[Boaz Harrosh]

This is a trivial addition to the SG API that can receive kernel
pointers. It is only used by the out-of-tree test module. So
it's immediate need is questionable. For maintenance ease it might
just get in, as it's very small.

John.
do you need this in the Kernel, or is it only for osd_ktest.ko?

Signed-off-by: John A. Chandy <john.chandy@uconn.edu>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/osd/osd_initiator.c |   71 ++++++++++++++++++++++++++++++++++++++
 include/scsi/osd_initiator.h     |    7 ++++
 2 files changed, 78 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index f5b5735..0433ea6 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -1015,6 +1015,77 @@ int osd_req_read_sg(struct osd_request *or,
 }
 EXPORT_SYMBOL(osd_req_read_sg);
 
+/* SG-list write/read Kern API
+ *
+ * osd_req_{write,read}_sg_kern takes an array of @buff pointers and an array
+ * of sg_entries. @numentries indicates how many pointers and sg_entries there
+ * are.  By requiring an array of buff pointers. This allows a caller to do a
+ * single write/read and scatter into multiple buffers.
+ * NOTE: Each buffer + len should not cross a page boundary.
+ */
+static struct bio *_create_sg_bios(struct osd_request *or,
+	void **buff, const struct osd_sg_entry *sglist, unsigned numentries)
+{
+	struct request_queue *q = osd_request_queue(or->osd_dev);
+	struct bio *bio;
+	unsigned i;
+
+	bio = bio_kmalloc(GFP_KERNEL, numentries);
+	if (unlikely(!bio)) {
+		OSD_DEBUG("Faild to allocate BIO size=%u\n", numentries);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	for (i = 0; i < numentries; i++) {
+		unsigned offset = offset_in_page(buff[i]);
+		struct page *page = virt_to_page(buff[i]);
+		unsigned len = sglist[i].len;
+		unsigned added_len;
+
+		BUG_ON(offset + len > PAGE_SIZE);
+		added_len = bio_add_pc_page(q, bio, page, len, offset);
+		if (unlikely(len != added_len)) {
+			OSD_DEBUG("bio_add_pc_page len(%d) != added_len(%d)\n",
+				  len, added_len);
+			bio_put(bio);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
+
+	return bio;
+}
+
+int osd_req_write_sg_kern(struct osd_request *or,
+	const struct osd_obj_id *obj, void **buff,
+	const struct osd_sg_entry *sglist, unsigned numentries)
+{
+	struct bio *bio = _create_sg_bios(or, buff, sglist, numentries);
+	if (IS_ERR(bio))
+		return PTR_ERR(bio);
+
+	bio->bi_rw |= REQ_WRITE;
+	osd_req_write_sg(or, obj, bio, sglist, numentries);
+
+	return 0;
+}
+EXPORT_SYMBOL(osd_req_write_sg_kern);
+
+int osd_req_read_sg_kern(struct osd_request *or,
+	const struct osd_obj_id *obj, void **buff,
+	const struct osd_sg_entry *sglist, unsigned numentries)
+{
+	struct bio *bio = _create_sg_bios(or, buff, sglist, numentries);
+	if (IS_ERR(bio))
+		return PTR_ERR(bio);
+
+	osd_req_read_sg(or, obj, bio, sglist, numentries);
+
+	return 0;
+}
+EXPORT_SYMBOL(osd_req_read_sg_kern);
+
+
+
 void osd_req_get_attributes(struct osd_request *or,
 	const struct osd_obj_id *obj)
 {
diff --git a/include/scsi/osd_initiator.h b/include/scsi/osd_initiator.h
index 169a5dc..53a9e88 100644
--- a/include/scsi/osd_initiator.h
+++ b/include/scsi/osd_initiator.h
@@ -455,6 +455,13 @@ int osd_req_write_sg(struct osd_request *or,
 int osd_req_read_sg(struct osd_request *or,
 	const struct osd_obj_id *obj, struct bio *bio,
 	const struct osd_sg_entry *sglist, unsigned numentries);
+int osd_req_write_sg_kern(struct osd_request *or,
+	const struct osd_obj_id *obj, void **buff,
+	const struct osd_sg_entry *sglist, unsigned numentries);
+int osd_req_read_sg_kern(struct osd_request *or,
+	const struct osd_obj_id *obj, void **buff,
+	const struct osd_sg_entry *sglist, unsigned numentries);
+
 /*
  * Root/Partition/Collection/Object Attributes commands
  */
-- 
1.7.2.3

Re: [PATCH 4/4 ver2] libosd: write/read_sg_kern API

[Vladislav Bolkhovitin]

Boaz Harrosh, on 10/19/2010 06:13 PM wrote:
> This is a trivial addition to the SG API that can receive kernel
> pointers. It is only used by the out-of-tree test module. So
> it's immediate need is questionable. For maintenance ease it might
> just get in, as it's very small.

This patch doesn't handle queue's restrictions (DMA, etc.).

Here is a complete patch for reading/writing SG lists from the kernel:
http://lkml.org/lkml/2010/9/14/201

Vlad

Re: [PATCH 4/4 ver2] libosd: write/read_sg_kern API

[Boaz Harrosh]

On 10/22/2010 07:32 PM, Vladislav Bolkhovitin wrote:
> Boaz Harrosh, on 10/19/2010 06:13 PM wrote:
>> This is a trivial addition to the SG API that can receive kernel
>> pointers. It is only used by the out-of-tree test module. So
>> it's immediate need is questionable. For maintenance ease it might
>> just get in, as it's very small.
> 
> This patch doesn't handle queue's restrictions (DMA, etc.).
> 

Please for give me. I know that the name is misleading. The
Scatter-List here is the osd scatter-list. Which describes
extents on the object, not of the memory. The proper name would
be extents API. The reason we named it SG is because this is the
way the OSD standard names it. We keep very close with the STD
naming convention, so it ended up this way.

> Here is a complete patch for reading/writing SG lists from the kernel:
> http://lkml.org/lkml/2010/9/14/201
> 
> Vlad

Boaz

Re: [PATCH 4/4 ver2] libosd: write/read_sg_kern API

[Vladislav Bolkhovitin]

Boaz Harrosh, on 10/24/2010 01:55 PM wrote:
> On 10/22/2010 07:32 PM, Vladislav Bolkhovitin wrote:
>> Boaz Harrosh, on 10/19/2010 06:13 PM wrote:
>>> This is a trivial addition to the SG API that can receive kernel
>>> pointers. It is only used by the out-of-tree test module. So
>>> it's immediate need is questionable. For maintenance ease it might
>>> just get in, as it's very small.
>>
>> This patch doesn't handle queue's restrictions (DMA, etc.).
>>
> 
> Please for give me. I know that the name is misleading. The
> Scatter-List here is the osd scatter-list. Which describes
> extents on the object, not of the memory. The proper name would
> be extents API. The reason we named it SG is because this is the
> way the OSD standard names it. We keep very close with the STD
> naming convention, so it ended up this way.

OK, but the queue's restrictions aren't handled by it anyway.

Vlad

Re: [PATCH 4/4 ver2] libosd: write/read_sg_kern API

[Boaz Harrosh]

On 10/25/2010 08:50 PM, Vladislav Bolkhovitin wrote:
> Boaz Harrosh, on 10/24/2010 01:55 PM wrote:
>> On 10/22/2010 07:32 PM, Vladislav Bolkhovitin wrote:
>>> Boaz Harrosh, on 10/19/2010 06:13 PM wrote:
>>>> This is a trivial addition to the SG API that can receive kernel
>>>> pointers. It is only used by the out-of-tree test module. So
>>>> it's immediate need is questionable. For maintenance ease it might
>>>> just get in, as it's very small.
>>>
>>> This patch doesn't handle queue's restrictions (DMA, etc.).
>>>
>>
>> Please for give me. I know that the name is misleading. The
>> Scatter-List here is the osd scatter-list. Which describes
>> extents on the object, not of the memory. The proper name would
>> be extents API. The reason we named it SG is because this is the
>> way the OSD standard names it. We keep very close with the STD
>> naming convention, so it ended up this way.
> 
> OK, but the queue's restrictions aren't handled by it anyway.
> 
> Vlad

Yes they are!

add_pc_page does the limits. blk_make_request() receives a bio-chain
and does bouncing.

But all this does not matter, because osd is a char-device not
block. Meaning osd objects can be written at char alignment
(offset and length). The way we work with multi segmented "out"
and "in" buffers, anything not char aligned like iscsi or fb will
not work very good any way. (Not very good I mean, everything
will bounce)

The libosd at the API level and internally works with bio-chains
the block-request is only made much later. blk_make_request(bio-chain)
should make everything work. If not it should be fixed.

Boaz

Re: [PATCH 4/4] libosd: write/read_sg_kern API

[John Chandy]

I don't need these, but I thought that they might be helpful routines.  With the normal write_sg/read_sg routines if the data on the caller side is scattered, the caller has to copy the scattered data into/from a single bio buffer.  With the _kern API the caller could instead let these routines set up the bios from a pointer list without a copy.

John.

On Oct 19, 2010, at 8:22 AM, Boaz Harrosh wrote:

> From: John A. Chandy <john.chandy@uconn.edu>
> 
> This is a trivial addition to the SG API that can receive kernel
> pointers. It is only used by the out-of-tree test module. So
> it's immediate need is questionable. For maintenance ease it might
> just get in, as it's very small.
> 
> John.
> do you need this in the Kernel, or is it only for osd_ktest.ko?
> 
> Signed-off-by: John A. Chandy <john.chandy@uconn.edu>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
> drivers/scsi/osd/osd_initiator.c |   71 ++++++++++++++++++++++++++++++++++++++
> 1 files changed, 71 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index f5b5735..0433ea6 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -1015,6 +1015,77 @@ int osd_req_read_sg(struct osd_request *or,
> }
> EXPORT_SYMBOL(osd_req_read_sg);
> 
> +/* SG-list write/read Kern API
> + *
> + * osd_req_{write,read}_sg_kern takes an array of @buff pointers and an array
> + * of sg_entries. @numentries indicates how many pointers and sg_entries there
> + * are.  By requiring an array of buff pointers. This allows a caller to do a
> + * single write/read and scatter into multiple buffers.
> + * NOTE: Each buffer + len should not cross a page boundary.
> + */
> +static struct bio *_create_sg_bios(struct osd_request *or,
> +	void **buff, const struct osd_sg_entry *sglist, unsigned numentries)
> +{
> +	struct request_queue *q = osd_request_queue(or->osd_dev);
> +	struct bio *bio;
> +	unsigned i;
> +
> +	bio = bio_kmalloc(GFP_KERNEL, numentries);
> +	if (unlikely(!bio)) {
> +		OSD_DEBUG("Faild to allocate BIO size=%u\n", numentries);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	for (i = 0; i < numentries; i++) {
> +		unsigned offset = offset_in_page(buff[i]);
> +		struct page *page = virt_to_page(buff[i]);
> +		unsigned len = sglist[i].len;
> +		unsigned added_len;
> +
> +		BUG_ON(offset + len > PAGE_SIZE);
> +		added_len = bio_add_pc_page(q, bio, page, len, offset);
> +		if (unlikely(len != added_len)) {
> +			OSD_DEBUG("bio_add_pc_page len(%d) != added_len(%d)\n",
> +				  len, added_len);
> +			bio_put(bio);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +	}
> +
> +	return bio;
> +}
> +
> +int osd_req_write_sg_kern(struct osd_request *or,
> +	const struct osd_obj_id *obj, void **buff,
> +	const struct osd_sg_entry *sglist, unsigned numentries)
> +{
> +	struct bio *bio = _create_sg_bios(or, buff, sglist, numentries);
> +	if (IS_ERR(bio))
> +		return PTR_ERR(bio);
> +
> +	bio->bi_rw |= REQ_WRITE;
> +	osd_req_write_sg(or, obj, bio, sglist, numentries);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(osd_req_write_sg_kern);
> +
> +int osd_req_read_sg_kern(struct osd_request *or,
> +	const struct osd_obj_id *obj, void **buff,
> +	const struct osd_sg_entry *sglist, unsigned numentries)
> +{
> +	struct bio *bio = _create_sg_bios(or, buff, sglist, numentries);
> +	if (IS_ERR(bio))
> +		return PTR_ERR(bio);
> +
> +	osd_req_read_sg(or, obj, bio, sglist, numentries);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(osd_req_read_sg_kern);
> +
> +
> +
> void osd_req_get_attributes(struct osd_request *or,
> 	const struct osd_obj_id *obj)
> {
> -- 
> 1.7.2.3
> 

Re: [PATCH 4/4] libosd: write/read_sg_kern API

[Boaz Harrosh]

On 10/20/2010 04:28 PM, John Chandy wrote:
> I don't need these, but I thought that they might be helpful routines.
> With the normal write_sg/read_sg routines if the data on the caller 
> side is scattered, the caller has to copy the scattered data into/from a 
> single bio buffer.  With the _kern API the caller could instead let these 
> routines set up the bios from a pointer list without a copy.
> 
> John.
> 

Hi John.

I agree it is a very convenient API.
Please note that the same could be done with a bio. Look how I changed the
implementation of _create_sg_bios() (I forgot to drop the s at the end) I
only allocate one bio and map all buffers to it. No copy.
It is however useful, but has no in-tree users. I'll keep it in the
out-of-tree git, so it will be available for user-mode libosd users.
If it gets to be a maintenance problem I'll try to push it again.
(I'll publish the new open-osd tree soon)

If you still have your original setup, it could be nice if you can test
with newest code. The osd-target code is your original submission, and
with this patchset I use it in exofs when reading RAID5 to jump over
the XOR units. It works nice.

Thanks for everything
Boaz

> On Oct 19, 2010, at 8:22 AM, Boaz Harrosh wrote:
> 
>> From: John A. Chandy <john.chandy@uconn.edu>
>>
>> This is a trivial addition to the SG API that can receive kernel
>> pointers. It is only used by the out-of-tree test module. So
>> it's immediate need is questionable. For maintenance ease it might
>> just get in, as it's very small.
>>
>> John.
>> do you need this in the Kernel, or is it only for osd_ktest.ko?
>>
>> Signed-off-by: John A. Chandy <john.chandy@uconn.edu>
>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>> ---
>> drivers/scsi/osd/osd_initiator.c |   71 ++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 71 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
>> index f5b5735..0433ea6 100644
>> --- a/drivers/scsi/osd/osd_initiator.c
>> +++ b/drivers/scsi/osd/osd_initiator.c
>> @@ -1015,6 +1015,77 @@ int osd_req_read_sg(struct osd_request *or,
>> }
>> EXPORT_SYMBOL(osd_req_read_sg);
>>
>> +/* SG-list write/read Kern API
>> + *
>> + * osd_req_{write,read}_sg_kern takes an array of @buff pointers and an array
>> + * of sg_entries. @numentries indicates how many pointers and sg_entries there
>> + * are.  By requiring an array of buff pointers. This allows a caller to do a
>> + * single write/read and scatter into multiple buffers.
>> + * NOTE: Each buffer + len should not cross a page boundary.
>> + */
>> +static struct bio *_create_sg_bios(struct osd_request *or,
>> +	void **buff, const struct osd_sg_entry *sglist, unsigned numentries)
>> +{
>> +	struct request_queue *q = osd_request_queue(or->osd_dev);
>> +	struct bio *bio;
>> +	unsigned i;
>> +
>> +	bio = bio_kmalloc(GFP_KERNEL, numentries);
>> +	if (unlikely(!bio)) {
>> +		OSD_DEBUG("Faild to allocate BIO size=%u\n", numentries);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	for (i = 0; i < numentries; i++) {
>> +		unsigned offset = offset_in_page(buff[i]);
>> +		struct page *page = virt_to_page(buff[i]);
>> +		unsigned len = sglist[i].len;
>> +		unsigned added_len;
>> +
>> +		BUG_ON(offset + len > PAGE_SIZE);
>> +		added_len = bio_add_pc_page(q, bio, page, len, offset);
>> +		if (unlikely(len != added_len)) {
>> +			OSD_DEBUG("bio_add_pc_page len(%d) != added_len(%d)\n",
>> +				  len, added_len);
>> +			bio_put(bio);
>> +			return ERR_PTR(-ENOMEM);
>> +		}
>> +	}
>> +
>> +	return bio;
>> +}
>> +
>> +int osd_req_write_sg_kern(struct osd_request *or,
>> +	const struct osd_obj_id *obj, void **buff,
>> +	const struct osd_sg_entry *sglist, unsigned numentries)
>> +{
>> +	struct bio *bio = _create_sg_bios(or, buff, sglist, numentries);
>> +	if (IS_ERR(bio))
>> +		return PTR_ERR(bio);
>> +
>> +	bio->bi_rw |= REQ_WRITE;
>> +	osd_req_write_sg(or, obj, bio, sglist, numentries);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(osd_req_write_sg_kern);
>> +
>> +int osd_req_read_sg_kern(struct osd_request *or,
>> +	const struct osd_obj_id *obj, void **buff,
>> +	const struct osd_sg_entry *sglist, unsigned numentries)
>> +{
>> +	struct bio *bio = _create_sg_bios(or, buff, sglist, numentries);
>> +	if (IS_ERR(bio))
>> +		return PTR_ERR(bio);
>> +
>> +	osd_req_read_sg(or, obj, bio, sglist, numentries);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(osd_req_read_sg_kern);
>> +
>> +
>> +
>> void osd_req_get_attributes(struct osd_request *or,
>> 	const struct osd_obj_id *obj)
>> {
>> -- 
>> 1.7.2.3
>>
> 

Re: [PATCH 4/4] libosd: write/read_sg_kern API

[Christoph Hellwig]

On Tue, Oct 19, 2010 at 02:22:42PM +0200, Boaz Harrosh wrote:
> From: John A. Chandy <john.chandy@uconn.edu>
> 
> This is a trivial addition to the SG API that can receive kernel
> pointers. It is only used by the out-of-tree test module. So
> it's immediate need is questionable. For maintenance ease it might
> just get in, as it's very small.
> 
> John.
> do you need this in the Kernel, or is it only for osd_ktest.ko?

It really shouldn't be in the tree at all. s/g lists are not something
that should be passed to a logical I/O layer.

Re: [PATCH 4/4] libosd: write/read_sg_kern API

[Boaz Harrosh]

On 11/01/2010 06:13 PM, Christoph Hellwig wrote:
> On Tue, Oct 19, 2010 at 02:22:42PM +0200, Boaz Harrosh wrote:
>> From: John A. Chandy <john.chandy@uconn.edu>
>>
>> This is a trivial addition to the SG API that can receive kernel
>> pointers. It is only used by the out-of-tree test module. So
>> it's immediate need is questionable. For maintenance ease it might
>> just get in, as it's very small.
>>
>> John.
>> do you need this in the Kernel, or is it only for osd_ktest.ko?
> 
> It really shouldn't be in the tree at all. s/g lists are not something
> that should be passed to a logical I/O layer.
> 

Please read the patch ;-)
It has been asked before, these are not Linux Kernel sg(s). These are
OSD scatter-gather. Osd scatter-gather are an array of extents
[offset, length] to which to write/read to/from in the object. The
memory buffer information is the same as a simple write/read. Only
that the target will random-seek+fetch bytes in a single on-the-wire
IO. There exist the same command for SCSI-disk targets as well.

I know that the conceptual name means something else, and I should have
perhaps chosen a new name. But the STD calls it Scatter-Gather and I have
not yet invented any new names, and always used the STD names for things.
It is not until people got mixed up that I realized it might have been
a mistake to call it that.

Should I submit a patch that renames anything "sg" to "extents", or some
other name? (Please note, there is no name collision it is all osd_sg_xxx)

Thanks
Boaz