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

[Boaz Harrosh]

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@xxxxxxxxxxxxxxx>
> ---
>  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
From 2981f9b73e70679d473706984ad1295c6745c4bd Mon Sep 17 00:00:00 2001
From: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxx>
---
 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