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@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. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
