[PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage

[PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage

[Nicholas A. Bellinger]

Greetings all,

This patch adds a new version of pscsi_map_task_SG() for the Linux/SCSI
passthrough subsystem plugin for target_core_mod v3.0 that uses Tejun's blk-map
patches on v2.6.30-rcX from git.kernel.org/pub/scm/linux/kernel/tj/misc.git blk-map.
This patch adds a LINUX_USE_NEW_BLK_MAP define to allow both blk_rq_map_kern_sg() and
legacy, non blk_rq_map_kern_sg() operation (with some limitiations with the latter) to
function.

Once Tejun's patches for block/blk-map.c:blk_rq_map_kern_sg() have been included
upstream, the legacy pscsi_map_task_SG() will be removed and blk-map will become
the preferred method for accessing struct scatterlist -> struct scsi_device for
SCSI target operations.  For now, I have created a blk-map branch in lio-core-2.6.git with
LINUX_USE_NEW_BLK_MAP=1 and left LINUX_USE_NEW_BLK_MAP=0 in branch master.

This patch also updates pscsi_map_task_non_SG()'s usage of blk_rq_map_kern(), whos
parameters have also been simplified in Tejun's patches.  It has been tested with
lio-core-2.6.git branch master and blk-map and tested on v2.6.30-rc3 x86 32-bit
HVM. The lio-core-2.6.git tree can be found at: 

http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=summary

So far, using Tejun's new code with LINUX_USE_NEW_BLK_MAP=1 is passing badblocks from
target_core_Mod/pSCSI exported Vmware mpt-fusion virtual SCSI HBA of TYPE_DISK.  Both the
ConfigFS 'file-descriptor' method and 'SCSI HCTL reference' method for accessing
struct scsi_device have been tested and are working with lio-core-2.6.git blk-map.

Currently *without* the blk-map patches on v2.6.30-rc3, is target_core_mod/pSCSI export is
limited to TYPE_DISK and TYPE_ROM that reference a struct block_device using the ConfigFS
'file descriptor' method.  This is because bio_add_page() expects struct block_device to be
each struct bio associated with the struct request w/o Tejun's blk_rq_map_kern_sg().

Thanks Tejun for this patch series!  Things have been stable so far and I hope to try some
'bare-metal' and IOV enabled Linux/SCSI target exports using this patch series, along with validating
blk-map on some non TYPE_DISK exports using target_core_mod/pSCSI.  I believe you intend this series for
v2.6.31 correct..? 

Boaz, have you had a chance to port your stuff over to this yet..?  Other comments..?

--nab

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 drivers/target/MCONFIG_TARGET      |    1 +
 drivers/target/Makefile            |    4 ++-
 drivers/target/target_core_pscsi.c |   49 ++++++++++++++++++++++++++++++++++-
 3 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/target/MCONFIG_TARGET b/drivers/target/MCONFIG_TARGET
index 8023deb..96541c4 100644
--- a/drivers/target/MCONFIG_TARGET
+++ b/drivers/target/MCONFIG_TARGET
@@ -22,3 +22,4 @@ LINUX_FILEIO ?= 1
 LINUX_VPD_PAGE_CHECK?=1
 
 LIO_TARGET_CONFIGFS?=1
+LINUX_USE_NEW_BLK_MAP?=0
diff --git a/drivers/target/Makefile b/drivers/target/Makefile
index 47fef07..a19490d 100644
--- a/drivers/target/Makefile
+++ b/drivers/target/Makefile
@@ -73,7 +73,9 @@ endif
 ifeq ($(LINUX_SCSI_MEDIA_ROM), 1)
 EXTRA_CFLAGS += -DLINUX_SCSI_MEDIA_ROM
 endif
-
+ifeq ($(LINUX_USE_NEW_BLK_MAP), 1)
+EXTRA_CFLAGS += -DLINUX_USE_NEW_BLK_MAP
+endif
 ifeq ($(DEBUG_DEV), 1)
 EXTRA_CFLAGS += -DDEBUG_DEV
 endif
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 0962563..0edcb9a 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -518,7 +518,12 @@ se_device_t *pscsi_create_virtdevice(
 				" parameter\n");
 		return NULL;
 	}
-
+#ifndef LINUX_USE_NEW_BLK_MAP
+	printk(KERN_ERR "Sorry, when running on >= v2.6.30 w/o blk-map branch"
+		" you need to use the ConfigFS file descriptor method for"
+		" accessing Linux/SCSI passthrough storage objects\n");
+	return -EINVAL;
+#endif
 	spin_lock_irq(sh->host_lock);
 	list_for_each_entry(sd, &sh->__devices, siblings) {
 		if (!(pdv->pdv_channel_id == sd->channel) ||
@@ -1269,6 +1274,39 @@ static inline struct bio *pscsi_get_bio(pscsi_dev_virt_t *pdv, int sg_num)
 #define DEBUG_PSCSI(x...)
 #endif
 
+#ifdef LINUX_USE_NEW_BLK_MAP
+
+/*	pscsi_map_task_SG()
+ *
+ *	This function uses the new struct scatterlist-> struct request mapping
+ *      from git.kernel.org/pub/scm/linux/kernel/tj/misc.git blk-map
+ *
+ *	This code is not upstream yet, so lio-core-2.6.git now has a blk-map
+ *	branch until this happens..
+ */
+int pscsi_map_task_SG(se_task_t *task)
+{
+	pscsi_plugin_task_t *pt = (pscsi_plugin_task_t *) task->transport_req;
+	struct request *rq = pt->pscsi_req;
+	int ret;
+	/*
+	 * For SCF_SCSI_DATA_SG_IO_CDB, use block/blk-map.c:blk_rq_map_kern_sg()
+	 * for mapping struct scatterlist to struct request.  Thanks Tejun!
+	 */
+	ret = blk_rq_map_kern_sg(rq, task->task_sg, task->task_sg_num,
+			GFP_KERNEL);
+	if (ret != 0) {
+		printk(KERN_ERR "blk_rq_map_kern_sg() failed for rq: %p"
+				" task_sg: %p task_sg_num: %u\n",
+			rq, task->task_sg, task->task_sg_num);
+		return -1;
+	}
+
+	return task->task_sg_num;
+}
+
+#else
+
 /*      pscsi_map_task_SG():
  *
  *
@@ -1376,6 +1414,9 @@ fail:
 	return ret;
 }
 
+#endif /* LINUX_USE_NEW_BLK_MAP */
+
+
 /*	pscsi_map_task_non_SG():
  *
  *
@@ -1389,10 +1430,14 @@ int pscsi_map_task_non_SG(se_task_t *task)
 
 	if (!task->task_size)
 		return 0;
-
+#ifdef LINUX_USE_NEW_BLK_MAP
+	ret = blk_rq_map_kern(pt->pscsi_req, T_TASK(cmd)->t_task_buf,
+			task->task_size, GFP_KERNEL);
+#else
 	ret = blk_rq_map_kern(pdv->pdv_sd->request_queue,
 			pt->pscsi_req, T_TASK(cmd)->t_task_buf,
 			task->task_size, GFP_KERNEL);
+#endif
 	if (ret < 0) {
 		printk(KERN_ERR "PSCSI: blk_rq_map_kern() failed: %d\n", ret);
 		return PYX_TRANSPORT_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-- 
1.5.4.1

Re: [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage

[Tejun Heo]

Hello, Nicholas.

Nicholas A. Bellinger wrote:
...
> Once Tejun's patches for block/blk-map.c:blk_rq_map_kern_sg() have
> been included upstream, the legacy pscsi_map_task_SG() will be
> removed and blk-map will become the preferred method for accessing
> struct scatterlist -> struct scsi_device for SCSI target operations.
> For now, I have created a blk-map branch in lio-core-2.6.git with
> LINUX_USE_NEW_BLK_MAP=1 and left LINUX_USE_NEW_BLK_MAP=0 in branch
> master.

Hmm... I don't think the patch will go in as-is although there seem to
be some places which can make use of sg mapping interface (including
OSD).  Currently there are following problems.

* Single kmalloc()ing the whole bio has higher chance of failing if
  the bvec becomes very large.

* Boaz is worried about performance implications with going back and
  forth between sgl and bvec.

In longer term, I think where we should be headed is...

* Expand sgl( or t) such that 1. it uses separate list for cpu and dma
  addresses so that it doesn't take up twice as much space
  unnecessarily, 2. sgl's can be easily chained (alerady somewhat
  there) and thus we don't have to worry about chaining in higher
  layer.

* Replace bvec with sgl in bio.

Dunno what we should do in the meantime tho.

Thanks.

-- 
tejun

Re: [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage

[Nicholas A. Bellinger]

On Thu, 2009-04-30 at 10:37 +0900, Tejun Heo wrote:
> Hello, Nicholas.
> 
> Nicholas A. Bellinger wrote:
> ...
> > Once Tejun's patches for block/blk-map.c:blk_rq_map_kern_sg() have
> > been included upstream, the legacy pscsi_map_task_SG() will be
> > removed and blk-map will become the preferred method for accessing
> > struct scatterlist -> struct scsi_device for SCSI target operations.
> > For now, I have created a blk-map branch in lio-core-2.6.git with
> > LINUX_USE_NEW_BLK_MAP=1 and left LINUX_USE_NEW_BLK_MAP=0 in branch
> > master.
> 
> Hmm... I don't think the patch will go in as-is although there seem to
> be some places which can make use of sg mapping interface (including
> OSD).  Currently there are following problems.
> 
> * Single kmalloc()ing the whole bio has higher chance of failing if
>   the bvec becomes very large.
> 

<nod>

> * Boaz is worried about performance implications with going back and
>   forth between sgl and bvec.

Yes, I did not think it would help performance, but it is still better
choice than NOT using struct request at all for v2.6.30..  is there
another choice atm? :-).  I am hoping to test with some non TYPE_DISK
with your code and see how it goes..

scsi_execute_async() is RIP for v2.6.30-rc3, just as struct scsi_request
went away in v2.6.18..

> 
> In longer term, I think where we should be headed is...
> 
> * Expand sgl( or t) such that 1. it uses separate list for cpu and dma
>   addresses so that it doesn't take up twice as much space
>   unnecessarily,

Hmm, interesting...


>  2. sgl's can be easily chained (alerady somewhat
>   there) and thus we don't have to worry about chaining in higher
>   layer.

<nod>

> 
> * Replace bvec with sgl in bio.
> 

Well, the target_core_mod logic assumes that each subsystem (pSCSI,
IBLOCK, FILEIO) is telling what limitiations on contigiously allocated
struct scatterlist array that has their single PAGE_SIZED struct
page_link setup from se_mem_t->se_page allocated physical memory.  As
the requirements for different Linux storage subsystems have grown for
target_core_mod logic, the concept of a 'storage object' passing
physical and/or virtual parameters for limitations of underlying
hardware via Linux subsystem is given us the default of PAGE_SIZE for
each allocated struct scatterlist in lio-core-2.6.git code.

Perhaps accepting a pre-formatted contigious array of struct scatterlist
in-place of bvec (so allocation can be avoided all together:-) from
incoming DATA I/O SG going into Linux/SCSI. The incoming sgl's into
struct request for target_core_mod/pSCSI are currently validated with
struct scsi_device provided 'physical device' parameters in the
target_core_mod esign.  These are represented in configfs for the
Linux/SCSI passthrough using your patches (and all other storage objects
and target_core_mod subsystem plugins, etc)

target:~# tree /sys/kernel/config/target/core/pscsi_0/
/sys/kernel/config/target/core/pscsi_0/
|-- hba_info
`-- sdd
    |-- alua_lu_gp
    |-- attrib
    |   |-- block_size
    |   |-- emulate_tas
    |   |-- emulate_ua_intlck_ctrl
    |   |-- hw_block_size
    |   |-- hw_max_sectors
    |   |-- hw_queue_depth
    |   |-- max_sectors
    |   |-- queue_depth
    |   `-- task_timeout
    |-- control
    |-- enable
    |-- fd
    |-- info

<SNIP>

The current target_core_mod v3.0 code allows block_size, max_sectors,
 and queue_depth to be enforced (and changed w/o active fabric module
exports to said storage object for v3.0) for IBLOCK, FILEIO, RAMDISK
plugins, but pSCSI with the blk-map patches on v2.6.30-rc3 is enforced
by the underlying struct scsi_device prefixed by hw_* values in
pscsi_0/sdd/attrib/ as expected by a kernel level target mode storage
engine.

I think for long term, there will be a need for a least three codepaths,
one in Block code, one in Linux/SCSI Target mode, and a Linux/SATA
target mode path

I) Accepting a preformatted and pre-validated (based on physical device
limitiations for struct scsi_device) into struct bio for struct request
in place of bvec allocation + memory map.  This API will expect a
contigious array of struct scatterlist containing page_links using
include/linux/scatterlist.h macros for init, link, etc.  This will allow
existing target_core_mod/pSCSI code (and others like Boaz) to take
advantage of the API.

II) A upstream SCSI kernel target infrastructure that does all of this
exforcement for you in drivers/scsi *TO* Linux/SCSI.  This would be
using the underlying logic of I) but drivers/scsi upstream code would be
doign the the validation of struct scatterlist memory into the struct
scsi_device for future and existing SCSI target code.

Then for v3.1 target_core_mod code, the idea is add configfs attributes
for each storage object to allow it control the number of passed
sg_extents and doing multiple PAGE_SIZE aligned struct scatterlists that
are passed into I) and II) above from a pre-allocated pool.  These
tunables will be added to allocate for mapping for target mode storage
fabric modules into virtually any method of struct scatterlist
allocation and mapping into Linux storage subsystems.  Doing this for
internally allocated struct pages and subsystem struct scatterlists will
be supported first, and then through using passthrough and then storage
target fabric plugin modules in v3.1 code..

III) Make target_core_mod/ConfigFS v3.x do subsystem independent target
mode and allow a generic kernel target to run on top of Linux/SCSI for
Linux/SATA physical subsystem plugins.

This means moving target_core_mod/configFS into direction of fabric
indepedent target operation so that both drivers/scsi and drivers/ata
can have access to a generic target mode kernel infrastructure.  This
would be building upon I) and II) and whatever yourself and the
Linux-ATA folks would like to do for target mode SATA ops into direct
target_core_mod/pSCSI, and virtual target_core_Mod/iBLOCK+FILEIO export.

Just a few thoughts, thanks for your comments!

--nab

> Dunno what we should do in the meantime tho.
> 
> Thanks.
> 

Re: [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage

[Boaz Harrosh]

[-- Attachment #1: Type: text/plain, Size: 7911 bytes --]

On 04/29/2009 03:43 AM, Nicholas A. Bellinger wrote:
> Greetings all,
> 
> This patch adds a new version of pscsi_map_task_SG() for the Linux/SCSI
> passthrough subsystem plugin for target_core_mod v3.0 that uses Tejun's blk-map
> patches on v2.6.30-rcX from git.kernel.org/pub/scm/linux/kernel/tj/misc.git blk-map.
> This patch adds a LINUX_USE_NEW_BLK_MAP define to allow both blk_rq_map_kern_sg() and
> legacy, non blk_rq_map_kern_sg() operation (with some limitiations with the latter) to
> function.
> 
> Once Tejun's patches for block/blk-map.c:blk_rq_map_kern_sg() have been included
> upstream, the legacy pscsi_map_task_SG() will be removed and blk-map will become
> the preferred method for accessing struct scatterlist -> struct scsi_device for
> SCSI target operations.  For now, I have created a blk-map branch in lio-core-2.6.git with
> LINUX_USE_NEW_BLK_MAP=1 and left LINUX_USE_NEW_BLK_MAP=0 in branch master.
> 
> This patch also updates pscsi_map_task_non_SG()'s usage of blk_rq_map_kern(), whos
> parameters have also been simplified in Tejun's patches.  It has been tested with
> lio-core-2.6.git branch master and blk-map and tested on v2.6.30-rc3 x86 32-bit
> HVM. The lio-core-2.6.git tree can be found at: 
> 
> http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=summary
> 
> So far, using Tejun's new code with LINUX_USE_NEW_BLK_MAP=1 is passing badblocks from
> target_core_Mod/pSCSI exported Vmware mpt-fusion virtual SCSI HBA of TYPE_DISK.  Both the
> ConfigFS 'file-descriptor' method and 'SCSI HCTL reference' method for accessing
> struct scsi_device have been tested and are working with lio-core-2.6.git blk-map.
> 
> Currently *without* the blk-map patches on v2.6.30-rc3, is target_core_mod/pSCSI export is
> limited to TYPE_DISK and TYPE_ROM that reference a struct block_device using the ConfigFS
> 'file descriptor' method.  This is because bio_add_page() expects struct block_device to be

Better use bio_add_pc_page(). bio_add_page is only meant for fs requests.

> each struct bio associated with the struct request w/o Tejun's blk_rq_map_kern_sg().
> 
> Thanks Tejun for this patch series!  Things have been stable so far and I hope to try some
> 'bare-metal' and IOV enabled Linux/SCSI target exports using this patch series, along with validating
> blk-map on some non TYPE_DISK exports using target_core_mod/pSCSI.  I believe you intend this series for
> v2.6.31 correct..? 
> 
> Boaz, have you had a chance to port your stuff over to this yet..?  Other comments..?
> 

No. As I said, these patches were not good for me. I do not have scatterlist at all.
I have a pre-made bio, both from filesystem and a block device. So my needs are different.

Please note that the patches as last sent, were not good in my opinion, for regressing on
some robustness and performance issues.

There might be another solution for you, BTW. I'll be reposting a James Bottomley's
patch in 1-2 days that makes blk_rq_map_kern() append the buffers it receives instead
of only supporting a single call. So you can allocate the request and call blk_rq_map_kern()
in a loop for-each-sg. The bad thing with this is that the biovec inside will be allocated
multiple times, jumping from small pools to bigger ones. If only there was a way to specify
a pre-allocated bio-size.

Patch is attached for convenience

> --nab
> 
> Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/target/MCONFIG_TARGET      |    1 +
>  drivers/target/Makefile            |    4 ++-
>  drivers/target/target_core_pscsi.c |   49 ++++++++++++++++++++++++++++++++++-
>  3 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/target/MCONFIG_TARGET b/drivers/target/MCONFIG_TARGET
> index 8023deb..96541c4 100644
> --- a/drivers/target/MCONFIG_TARGET
> +++ b/drivers/target/MCONFIG_TARGET
> @@ -22,3 +22,4 @@ LINUX_FILEIO ?= 1
>  LINUX_VPD_PAGE_CHECK?=1
>  
>  LIO_TARGET_CONFIGFS?=1
> +LINUX_USE_NEW_BLK_MAP?=0
> diff --git a/drivers/target/Makefile b/drivers/target/Makefile
> index 47fef07..a19490d 100644
> --- a/drivers/target/Makefile
> +++ b/drivers/target/Makefile
> @@ -73,7 +73,9 @@ endif
>  ifeq ($(LINUX_SCSI_MEDIA_ROM), 1)
>  EXTRA_CFLAGS += -DLINUX_SCSI_MEDIA_ROM
>  endif
> -
> +ifeq ($(LINUX_USE_NEW_BLK_MAP), 1)
> +EXTRA_CFLAGS += -DLINUX_USE_NEW_BLK_MAP
> +endif
>  ifeq ($(DEBUG_DEV), 1)
>  EXTRA_CFLAGS += -DDEBUG_DEV
>  endif
> diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
> index 0962563..0edcb9a 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -518,7 +518,12 @@ se_device_t *pscsi_create_virtdevice(
>  				" parameter\n");
>  		return NULL;
>  	}
> -
> +#ifndef LINUX_USE_NEW_BLK_MAP
> +	printk(KERN_ERR "Sorry, when running on >= v2.6.30 w/o blk-map branch"
> +		" you need to use the ConfigFS file descriptor method for"
> +		" accessing Linux/SCSI passthrough storage objects\n");
> +	return -EINVAL;
> +#endif
>  	spin_lock_irq(sh->host_lock);
>  	list_for_each_entry(sd, &sh->__devices, siblings) {
>  		if (!(pdv->pdv_channel_id == sd->channel) ||
> @@ -1269,6 +1274,39 @@ static inline struct bio *pscsi_get_bio(pscsi_dev_virt_t *pdv, int sg_num)
>  #define DEBUG_PSCSI(x...)
>  #endif
>  
> +#ifdef LINUX_USE_NEW_BLK_MAP

As I understand, you have a full Linux tree, at the git above.
The LINUX_USE_NEW_BLK_MAP define is not nice. And only causes more work
for you.

You leave master branch code based on current Kernel.

Then at blk-map branch you apply the correct lio patches together with Tejun's
at the proper places. After the fixture or change you are looking for. As if
lio was in kernel and this is the proper propagation of patches.

If you do this because of an out-of-tree compilation support, then here
too, you keep two branches, easily rebase two chains of patches, and provide
a tar-ball for the kernel in question. (git-web just does that for you
automatically.)

Because look, as long as you have LINUX_USE_NEW_BLK_MAP usage (in any branch)
then the code is not acceptable into mainline, and is just an experiment.

> +
> +/*	pscsi_map_task_SG()
> + *
> + *	This function uses the new struct scatterlist-> struct request mapping
> + *      from git.kernel.org/pub/scm/linux/kernel/tj/misc.git blk-map
> + *
> + *	This code is not upstream yet, so lio-core-2.6.git now has a blk-map
> + *	branch until this happens..
> + */
> +int pscsi_map_task_SG(se_task_t *task)
> +{
> +	pscsi_plugin_task_t *pt = (pscsi_plugin_task_t *) task->transport_req;
> +	struct request *rq = pt->pscsi_req;
> +	int ret;
> +	/*
> +	 * For SCF_SCSI_DATA_SG_IO_CDB, use block/blk-map.c:blk_rq_map_kern_sg()
> +	 * for mapping struct scatterlist to struct request.  Thanks Tejun!
> +	 */
> +	ret = blk_rq_map_kern_sg(rq, task->task_sg, task->task_sg_num,
> +			GFP_KERNEL);
> +	if (ret != 0) {
> +		printk(KERN_ERR "blk_rq_map_kern_sg() failed for rq: %p"
> +				" task_sg: %p task_sg_num: %u\n",
> +			rq, task->task_sg, task->task_sg_num);
> +		return -1;
> +	}
> +
> +	return task->task_sg_num;
> +}
> +
> +#else
> +
>  /*      pscsi_map_task_SG():
>   *
>   *
> @@ -1376,6 +1414,9 @@ fail:
>  	return ret;
>  }
>  
> +#endif /* LINUX_USE_NEW_BLK_MAP */
> +
> +
>  /*	pscsi_map_task_non_SG():
>   *
>   *
> @@ -1389,10 +1430,14 @@ int pscsi_map_task_non_SG(se_task_t *task)
>  
>  	if (!task->task_size)
>  		return 0;
> -
> +#ifdef LINUX_USE_NEW_BLK_MAP

source code is not a version management system, Use git for that

> +	ret = blk_rq_map_kern(pt->pscsi_req, T_TASK(cmd)->t_task_buf,
> +			task->task_size, GFP_KERNEL);
> +#else
>  	ret = blk_rq_map_kern(pdv->pdv_sd->request_queue,
>  			pt->pscsi_req, T_TASK(cmd)->t_task_buf,
>  			task->task_size, GFP_KERNEL);
> +#endif
>  	if (ret < 0) {
>  		printk(KERN_ERR "PSCSI: blk_rq_map_kern() failed: %d\n", ret);
>  		return PYX_TRANSPORT_LOGICAL_UNIT_COMMUNICATION_FAILURE;


Cheers
Boaz

[-- Attachment #2: 0001-allow-blk_rq_map_kern-to-append-to-requests.patch --]
[-- Type: text/plain, Size: 1713 bytes --]

From 2981f9b73e70679d473706984ad1295c6745c4bd Mon Sep 17 00:00:00 2001
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Mon, 16 Mar 2009 15:46:53 +0200
Subject: [PATCH] allow blk_rq_map_kern to append to requests

Use blk_rq_append_bio() internally instead of blk_rq_bio_prep()
so blk_rq_map_kern can be called multiple times, to map multiple
buffers.

This is in the effort to un-export blk_rq_append_bio()

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 block/blk-map.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/blk-map.c b/block/blk-map.c
index f103729..ada399e 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -282,7 +282,8 @@ EXPORT_SYMBOL(blk_rq_unmap_user);
  *
  * Description:
  *    Data will be mapped directly if possible. Otherwise a bounce
- *    buffer is used.
+ *    buffer is used. Can be called multple times to append multple
+ *    buffers.
  */
 int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
 		    unsigned int len, gfp_t gfp_mask)
@@ -290,6 +291,7 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
 	int reading = rq_data_dir(rq) == READ;
 	int do_copy = 0;
 	struct bio *bio;
+	int ret;
 
 	if (len > (q->max_hw_sectors << 9))
 		return -EINVAL;
@@ -311,7 +313,13 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf,
 	if (do_copy)
 		rq->cmd_flags |= REQ_COPY_USER;
 
-	blk_rq_bio_prep(q, rq, bio);
+	ret = blk_rq_append_bio(q, rq, bio);
+	if (unlikely(ret)) {
+		/* request is too big */
+		bio_put(bio);
+		return ret;
+	}
+
 	blk_queue_bounce(q, &rq->bio);
 	rq->buffer = rq->data = NULL;
 	return 0;
-- 
1.6.2.1

Re: [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage

[Nicholas A. Bellinger]

On Thu, 2009-04-30 at 12:37 +0300, Boaz Harrosh wrote:
> On 04/29/2009 03:43 AM, Nicholas A. Bellinger wrote:
> > Greetings all,
> > 
> > This patch adds a new version of pscsi_map_task_SG() for the Linux/SCSI
> > passthrough subsystem plugin for target_core_mod v3.0 that uses Tejun's blk-map
> > patches on v2.6.30-rcX from git.kernel.org/pub/scm/linux/kernel/tj/misc.git blk-map.
> > This patch adds a LINUX_USE_NEW_BLK_MAP define to allow both blk_rq_map_kern_sg() and
> > legacy, non blk_rq_map_kern_sg() operation (with some limitiations with the latter) to
> > function.
> > 
> > Once Tejun's patches for block/blk-map.c:blk_rq_map_kern_sg() have been included
> > upstream, the legacy pscsi_map_task_SG() will be removed and blk-map will become
> > the preferred method for accessing struct scatterlist -> struct scsi_device for
> > SCSI target operations.  For now, I have created a blk-map branch in lio-core-2.6.git with
> > LINUX_USE_NEW_BLK_MAP=1 and left LINUX_USE_NEW_BLK_MAP=0 in branch master.
> > 
> > This patch also updates pscsi_map_task_non_SG()'s usage of blk_rq_map_kern(), whos
> > parameters have also been simplified in Tejun's patches.  It has been tested with
> > lio-core-2.6.git branch master and blk-map and tested on v2.6.30-rc3 x86 32-bit
> > HVM. The lio-core-2.6.git tree can be found at: 
> > 
> > http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=summary
> > 
> > So far, using Tejun's new code with LINUX_USE_NEW_BLK_MAP=1 is passing badblocks from
> > target_core_Mod/pSCSI exported Vmware mpt-fusion virtual SCSI HBA of TYPE_DISK.  Both the
> > ConfigFS 'file-descriptor' method and 'SCSI HCTL reference' method for accessing
> > struct scsi_device have been tested and are working with lio-core-2.6.git blk-map.
> > 
> > Currently *without* the blk-map patches on v2.6.30-rc3, is target_core_mod/pSCSI export is
> > limited to TYPE_DISK and TYPE_ROM that reference a struct block_device using the ConfigFS
> > 'file descriptor' method.  This is because bio_add_page() expects struct block_device to be
> 
> Better use bio_add_pc_page(). bio_add_page is only meant for fs requests.
> 

<nod>, I was only using bio_add_page() in the pre v2.6.30 code for
target_core_mod/pSCSI because bio_add_pc_page() is not exported from
fs/bio.c.  Perhaps since bio_add_pc_page() is intended to be for SCSI
target mode with struct request it should be EXPORT_SYMBOL_GPL()..?

> > each struct bio associated with the struct request w/o Tejun's blk_rq_map_kern_sg().
> > 
> > Thanks Tejun for this patch series!  Things have been stable so far and I hope to try some
> > 'bare-metal' and IOV enabled Linux/SCSI target exports using this patch series, along with validating
> > blk-map on some non TYPE_DISK exports using target_core_mod/pSCSI.  I believe you intend this series for
> > v2.6.31 correct..? 
> > 
> > Boaz, have you had a chance to port your stuff over to this yet..?  Other comments..?
> > 
> 
> No. As I said, these patches were not good for me. I do not have scatterlist at all.
> I have a pre-made bio, both from filesystem and a block device. So my needs are different.

Understood..

> Please note that the patches as last sent, were not good in my opinion, for regressing on
> some robustness and performance issues.
> 
> There might be another solution for you, BTW. I'll be reposting a James Bottomley's
> patch in 1-2 days that makes blk_rq_map_kern() append the buffers it receives instead
> of only supporting a single call. So you can allocate the request and call blk_rq_map_kern()
> in a loop for-each-sg. The bad thing with this is that the biovec inside will be allocated
> multiple times, jumping from small pools to bigger ones.

Ok, I will plan on testing both methods (single call
blk_rq_map_kern_sg() vs. appending buffers with blk_rq_map_kern()) using
the pSCSI export on v2.6.30-rc3..

>  If only there was a way to specify a pre-allocated bio-size.

Hrrmm, can you explain a bit more about what this would entail..?  From
the SCSI target API side, mapping a contigious array of struct
scatterlist's from the caller into struct request (and struct bio) in
place of bvec would still make the most sense I think.

In the OSD case where you are already passing into pre-formatted bio's
it would be up to the caller to format and validate the your pages via
an internally allocated (or preallocated) array of scatterlists.

Anyways, I will think about it some more and see what can be found..

> 
> Patch is attached for convenience
> 
> > --nab
> > 
> > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > ---
> >  drivers/target/MCONFIG_TARGET      |    1 +
> >  drivers/target/Makefile            |    4 ++-
> >  drivers/target/target_core_pscsi.c |   49 ++++++++++++++++++++++++++++++++++-
> >  3 files changed, 51 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/target/MCONFIG_TARGET b/drivers/target/MCONFIG_TARGET
> > index 8023deb..96541c4 100644
> > --- a/drivers/target/MCONFIG_TARGET
> > +++ b/drivers/target/MCONFIG_TARGET
> > @@ -22,3 +22,4 @@ LINUX_FILEIO ?= 1
> >  LINUX_VPD_PAGE_CHECK?=1
> >  
> >  LIO_TARGET_CONFIGFS?=1
> > +LINUX_USE_NEW_BLK_MAP?=0
> > diff --git a/drivers/target/Makefile b/drivers/target/Makefile
> > index 47fef07..a19490d 100644
> > --- a/drivers/target/Makefile
> > +++ b/drivers/target/Makefile
> > @@ -73,7 +73,9 @@ endif
> >  ifeq ($(LINUX_SCSI_MEDIA_ROM), 1)
> >  EXTRA_CFLAGS += -DLINUX_SCSI_MEDIA_ROM
> >  endif
> > -
> > +ifeq ($(LINUX_USE_NEW_BLK_MAP), 1)
> > +EXTRA_CFLAGS += -DLINUX_USE_NEW_BLK_MAP
> > +endif
> >  ifeq ($(DEBUG_DEV), 1)
> >  EXTRA_CFLAGS += -DDEBUG_DEV
> >  endif
> > diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
> > index 0962563..0edcb9a 100644
> > --- a/drivers/target/target_core_pscsi.c
> > +++ b/drivers/target/target_core_pscsi.c
> > @@ -518,7 +518,12 @@ se_device_t *pscsi_create_virtdevice(
> >  				" parameter\n");
> >  		return NULL;
> >  	}
> > -
> > +#ifndef LINUX_USE_NEW_BLK_MAP
> > +	printk(KERN_ERR "Sorry, when running on >= v2.6.30 w/o blk-map branch"
> > +		" you need to use the ConfigFS file descriptor method for"
> > +		" accessing Linux/SCSI passthrough storage objects\n");
> > +	return -EINVAL;
> > +#endif
> >  	spin_lock_irq(sh->host_lock);
> >  	list_for_each_entry(sd, &sh->__devices, siblings) {
> >  		if (!(pdv->pdv_channel_id == sd->channel) ||
> > @@ -1269,6 +1274,39 @@ static inline struct bio *pscsi_get_bio(pscsi_dev_virt_t *pdv, int sg_num)
> >  #define DEBUG_PSCSI(x...)
> >  #endif
> >  
> > +#ifdef LINUX_USE_NEW_BLK_MAP
> 
> As I understand, you have a full Linux tree, at the git above.
> The LINUX_USE_NEW_BLK_MAP define is not nice. And only causes more work
> for you.
> 
> You leave master branch code based on current Kernel.
> 
> Then at blk-map branch you apply the correct lio patches together with Tejun's
> at the proper places. After the fixture or change you are looking for. As if
> lio was in kernel and this is the proper propagation of patches.
> 
> If you do this because of an out-of-tree compilation support, then here
> too, you keep two branches, easily rebase two chains of patches, and provide
> a tar-ball for the kernel in question. (git-web just does that for you
> automatically.)
> 
> Because look, as long as you have LINUX_USE_NEW_BLK_MAP usage (in any branch)
> then the code is not acceptable into mainline, and is just an experiment.
> 

Yeah, I was using LINUX_USE_NEW_BLK_MAP as a temporary measure so that I
could keep both branches in sync for now.  I will kill it moving
forward.

Thanks for your comments,

--nab

> > +
> > +/*	pscsi_map_task_SG()
> > + *
> > + *	This function uses the new struct scatterlist-> struct request mapping
> > + *      from git.kernel.org/pub/scm/linux/kernel/tj/misc.git blk-map
> > + *
> > + *	This code is not upstream yet, so lio-core-2.6.git now has a blk-map
> > + *	branch until this happens..
> > + */
> > +int pscsi_map_task_SG(se_task_t *task)
> > +{
> > +	pscsi_plugin_task_t *pt = (pscsi_plugin_task_t *) task->transport_req;
> > +	struct request *rq = pt->pscsi_req;
> > +	int ret;
> > +	/*
> > +	 * For SCF_SCSI_DATA_SG_IO_CDB, use block/blk-map.c:blk_rq_map_kern_sg()
> > +	 * for mapping struct scatterlist to struct request.  Thanks Tejun!
> > +	 */
> > +	ret = blk_rq_map_kern_sg(rq, task->task_sg, task->task_sg_num,
> > +			GFP_KERNEL);
> > +	if (ret != 0) {
> > +		printk(KERN_ERR "blk_rq_map_kern_sg() failed for rq: %p"
> > +				" task_sg: %p task_sg_num: %u\n",
> > +			rq, task->task_sg, task->task_sg_num);
> > +		return -1;
> > +	}
> > +
> > +	return task->task_sg_num;
> > +}
> > +
> > +#else
> > +
> >  /*      pscsi_map_task_SG():
> >   *
> >   *
> > @@ -1376,6 +1414,9 @@ fail:
> >  	return ret;
> >  }
> >  
> > +#endif /* LINUX_USE_NEW_BLK_MAP */
> > +
> > +
> >  /*	pscsi_map_task_non_SG():
> >   *
> >   *
> > @@ -1389,10 +1430,14 @@ int pscsi_map_task_non_SG(se_task_t *task)
> >  
> >  	if (!task->task_size)
> >  		return 0;
> > -
> > +#ifdef LINUX_USE_NEW_BLK_MAP
> 
> source code is not a version management system, Use git for that
> 
> > +	ret = blk_rq_map_kern(pt->pscsi_req, T_TASK(cmd)->t_task_buf,
> > +			task->task_size, GFP_KERNEL);
> > +#else
> >  	ret = blk_rq_map_kern(pdv->pdv_sd->request_queue,
> >  			pt->pscsi_req, T_TASK(cmd)->t_task_buf,
> >  			task->task_size, GFP_KERNEL);
> > +#endif
> >  	if (ret < 0) {
> >  		printk(KERN_ERR "PSCSI: blk_rq_map_kern() failed: %d\n", ret);
> >  		return PYX_TRANSPORT_LOGICAL_UNIT_COMMUNICATION_FAILURE;
> 
> 
> Cheers
> Boaz

Re: [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage

[Boaz Harrosh]

On 04/30/2009 10:10 PM, Nicholas A. Bellinger wrote:
> On Thu, 2009-04-30 at 12:37 +0300, Boaz Harrosh wrote:
>>> Currently *without* the blk-map patches on v2.6.30-rc3, is target_core_mod/pSCSI export is
>>> limited to TYPE_DISK and TYPE_ROM that reference a struct block_device using the ConfigFS
>>> 'file descriptor' method.  This is because bio_add_page() expects struct block_device to be
>> Better use bio_add_pc_page(). bio_add_page is only meant for fs requests.
>>
> 
> <nod>, I was only using bio_add_page() in the pre v2.6.30 code for
> target_core_mod/pSCSI because bio_add_pc_page() is not exported from
> fs/bio.c.  Perhaps since bio_add_pc_page() is intended to be for SCSI
> target mode with struct request it should be EXPORT_SYMBOL_GPL()..?
> 

It is very much exported with plain EXPORT_SYMBOL(). If not then I would have
trouble in exofs as a module and so will lots of other Kernel modules. It is
used in more then one place.

>>> each struct bio associated with the struct request w/o Tejun's blk_rq_map_kern_sg().
>>>
>>> Thanks Tejun for this patch series!  Things have been stable so far and I hope to try some
>>> 'bare-metal' and IOV enabled Linux/SCSI target exports using this patch series, along with validating
>>> blk-map on some non TYPE_DISK exports using target_core_mod/pSCSI.  I believe you intend this series for
>>> v2.6.31 correct..? 
>>>
>>> Boaz, have you had a chance to port your stuff over to this yet..?  Other comments..?
>>>
>> No. As I said, these patches were not good for me. I do not have scatterlist at all.
>> I have a pre-made bio, both from filesystem and a block device. So my needs are different.
> 
> Understood..
> 
>> Please note that the patches as last sent, were not good in my opinion, for regressing on
>> some robustness and performance issues.
>>
>> There might be another solution for you, BTW. I'll be reposting a James Bottomley's
>> patch in 1-2 days that makes blk_rq_map_kern() append the buffers it receives instead
>> of only supporting a single call. So you can allocate the request and call blk_rq_map_kern()
>> in a loop for-each-sg. The bad thing with this is that the biovec inside will be allocated
>> multiple times, jumping from small pools to bigger ones.
> 
> Ok, I will plan on testing both methods (single call
> blk_rq_map_kern_sg() vs. appending buffers with blk_rq_map_kern()) using
> the pSCSI export on v2.6.30-rc3..
> 
>>  If only there was a way to specify a pre-allocated bio-size.
> 
> Hrrmm, can you explain a bit more about what this would entail..?  From
> the SCSI target API side, mapping a contigious array of struct

"contiguous" scatterlist array is only up to 128 max on x86_64. This is
the trivial case that is easy to implement with direct bio_xxx exported
calls. The problem start when we need chained scatterlist, to chained
bios, and do not want to invent the block-layer for that.

> scatterlist's from the caller into struct request (and struct bio) in
> place of bvec would still make the most sense I think.
> 

If you want to consider long term. Then I think Tejum's suggestion is best.
That is: To separate scatterlist into a virtual-part and physical-dma-part.
The former just being a reincarnation of bvec (Held at bio as you imagined)
and the later filled and returned by IOMMUs.

> In the OSD case where you are already passing into pre-formatted bio's
> it would be up to the caller to format and validate the your pages via
> an internally allocated (or preallocated) array of scatterlists.
> 

No, in the OSD case I receive a bio that was either built by the block-layer
itself, in the case of a stacking block device, or one built from a filesystem
with the use of exported bio API, that is bio_alloc+bio_add_pc_page*128.

So if bio will use the new-scatterlist internally, it will not affect any of
OSD code since it tries not to touch any bio internal parts.

> Anyways, I will think about it some more and see what can be found..
> 

Yes, time will tell. One good thing is that things are chainging and
a solution will present itself.

Boaz