* [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage @ 2009-04-29 0:43 Nicholas A. Bellinger 2009-04-30 1:37 ` Tejun Heo 2009-04-30 9:37 ` Boaz Harrosh 0 siblings, 2 replies; 6+ messages in thread From: Nicholas A. Bellinger @ 2009-04-29 0:43 UTC (permalink / raw) To: linux-scsi, LKML Cc: Tejun Heo, Boaz Harrosh, James Bottomley, Hannes Reinecke, FUJITA Tomonori, Mike Christie, Douglas Gilbert, Christoph Hellwig, Jens Axboe, Andrew Morton, Vladislav Bolkhovitin 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage 2009-04-29 0:43 [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage Nicholas A. Bellinger @ 2009-04-30 1:37 ` Tejun Heo 2009-04-30 8:00 ` Nicholas A. Bellinger 2009-04-30 9:37 ` Boaz Harrosh 1 sibling, 1 reply; 6+ messages in thread From: Tejun Heo @ 2009-04-30 1:37 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: linux-scsi, LKML, Boaz Harrosh, James Bottomley, Hannes Reinecke, FUJITA Tomonori, Mike Christie, Douglas Gilbert, Christoph Hellwig, Jens Axboe, Andrew Morton, Vladislav Bolkhovitin 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage 2009-04-30 1:37 ` Tejun Heo @ 2009-04-30 8:00 ` Nicholas A. Bellinger 0 siblings, 0 replies; 6+ messages in thread From: Nicholas A. Bellinger @ 2009-04-30 8:00 UTC (permalink / raw) To: Tejun Heo Cc: linux-scsi, LKML, Boaz Harrosh, James Bottomley, Hannes Reinecke, FUJITA Tomonori, Mike Christie, Douglas Gilbert, Christoph Hellwig, Jens Axboe, Andrew Morton, Vladislav Bolkhovitin 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. > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage 2009-04-29 0:43 [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage Nicholas A. Bellinger 2009-04-30 1:37 ` Tejun Heo @ 2009-04-30 9:37 ` Boaz Harrosh 2009-04-30 19:10 ` Nicholas A. Bellinger 1 sibling, 1 reply; 6+ messages in thread From: Boaz Harrosh @ 2009-04-30 9:37 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: linux-scsi, LKML, Tejun Heo, James Bottomley, Hannes Reinecke, FUJITA Tomonori, Mike Christie, Douglas Gilbert, Christoph Hellwig, Jens Axboe, Andrew Morton, Vladislav Bolkhovitin [-- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage 2009-04-30 9:37 ` Boaz Harrosh @ 2009-04-30 19:10 ` Nicholas A. Bellinger 2009-05-03 13:32 ` Boaz Harrosh 0 siblings, 1 reply; 6+ messages in thread From: Nicholas A. Bellinger @ 2009-04-30 19:10 UTC (permalink / raw) To: Boaz Harrosh Cc: linux-scsi, LKML, Tejun Heo, James Bottomley, Hannes Reinecke, FUJITA Tomonori, Mike Christie, Douglas Gilbert, Christoph Hellwig, Jens Axboe, Andrew Morton, Vladislav Bolkhovitin 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage 2009-04-30 19:10 ` Nicholas A. Bellinger @ 2009-05-03 13:32 ` Boaz Harrosh 0 siblings, 0 replies; 6+ messages in thread From: Boaz Harrosh @ 2009-05-03 13:32 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: linux-scsi, LKML, Tejun Heo, James Bottomley, Hannes Reinecke, FUJITA Tomonori, Mike Christie, Douglas Gilbert, Christoph Hellwig, Jens Axboe, Andrew Morton, Vladislav Bolkhovitin 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-05-03 13:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-04-29 0:43 [PATCH] [Target_Core_Mod/pSCSI]: Add block/blk-map.c:blk_rq_map_kern_sg() usage Nicholas A. Bellinger 2009-04-30 1:37 ` Tejun Heo 2009-04-30 8:00 ` Nicholas A. Bellinger 2009-04-30 9:37 ` Boaz Harrosh 2009-04-30 19:10 ` Nicholas A. Bellinger 2009-05-03 13:32 ` Boaz Harrosh
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.