On Tue, 2012-12-04 at 09:48 +0100, Sebastian Andrzej Siewior wrote:
> On 12/04/2012 05:44 AM, Nicholas A. Bellinger wrote:
> > Hi Sebastian,
>
> Hi Nab,
>
> >> vfs_readv() returned -14 for non S_ISBLK
> >
> > Ok, this looks like a perfectly normal READ_10 of 64K (128 sectors) to
> > LBA 12349.
> >
> > Not sure why vfs_readv() of 64K is generating an -EFAULT here, but lets
> > first verify that SGLs being dispatched by tcm_loop to TCM -> FILEIO are
> > looking as expected on your setup.
> >
> > Please try to reproduce with the following patch:
>
> I modified it slightly in order to dump sg_virt() as well since this is
> what we pass to the function that fails:
> vfs_readv() returned -14 for non S_ISBLK
> SG[0]: f4188240 page: f5c1e5a0 virt: (null) length: 4096 offset: 0
> SG[1]: f4188258 page: f5c55da0 virt: (null) length: 4096 offset: 0
> SG[2]: f4188270 page: f5c54d80 virt: (null) length: 4096 offset: 0
> SG[3]: f4188288 page: f5c5bd20 virt: (null) length: 4096 offset: 0
> SG[4]: f41882a0 page: f5c5bca0 virt: (null) length: 4096 offset: 0
> SG[5]: f41882b8 page: f5c547e0 virt: (null) length: 4096 offset: 0
> SG[6]: f41882d0 page: f5c58b20 virt: (null) length: 4096 offset: 0
> SG[7]: f41882e8 page: f5c621e0 virt: (null) length: 4096 offset: 0
> SG[8]: f4188300 page: f5c54d40 virt: ff990000 length: 4096 offset: 0
> SG[9]: f4188318 page: f5c4c800 virt: (null) length: 4096 offset: 0
> SG[10]: f4188330 page: f5c54ca0 virt: (null) length: 4096 offset: 0
> SG[11]: f4188348 page: f5c57980 virt: (null) length: 4096 offset: 0
> SG[12]: f4188360 page: f5c544e0 virt: (null) length: 4096 offset: 0
> SG[13]: f4188378 page: f5c565c0 virt: (null) length: 4096 offset: 0
> SG[14]: f4188390 page: f5c5bce0 virt: ff995000 length: 4096 offset: 0
> SG[15]: f41883a8 page: f5c22980 virt: (null) length: 4096 offset: 0
>
> If you ask why is virt null then I got to tell you that I have highmem.
> I remember that while I fixed tcm_loop a year ago or so I pointed out
> that the code is not highmem safe but it is not a problem because all
> memory that is allocated by tcm_loop itself is non-highmem. No you got
> highmem from somewhere.
OK, so the SGL memory dispatched into tcm_loop LLD code is setup with
GFP_KERNEL allocations from SCSI core by default, which is breaking
32-bit highmem with FILEIO backends..
One possible workaround for tcm_loop w/ highmem is make SCSI use GFP_DMA
by setting scsi_host_template->unchecked_isa_dma=1 from within the LLD.
> The patch
>
> diff --git a/drivers/target/target_core_file.c
> b/drivers/target/target_core_file.c
> index 0360383..319b87a 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
> @@ -260,7 +260,7 @@ static int fd_do_readv(struct se_cmd *cmd, struct
> scatterlist *sgl,
>
> for_each_sg(sgl, sg, sgl_nents, i) {
> iov[i].iov_len = sg->length;
> - iov[i].iov_base = sg_virt(sg);
> + iov[i].iov_base = kmap(sg_page(sg));
> }
>
> old_fs = get_fs();
> @@ -268,6 +268,10 @@ static int fd_do_readv(struct se_cmd *cmd, struct
> scatterlist *sgl,
> ret = vfs_readv(fd, &iov[0], sgl_nents, &pos);
> set_fs(old_fs);
>
> + for_each_sg(sgl, sg, sgl_nents, i) {
> + kunmap(sg_page(sg));
> + }
> +
> kfree(iov);
> /*
> * Return zeros and GOOD status even if the READ did not return
>
> Does not work. It bails out even earlier, before the XFS mount
> completes. That means the complete code is not highmem aware and it
> simply works because page_address() returns something that does not
> cause a fault and the memory is not important enough to crash the box.
>
Mmmm.. Christoph, what do you think for the highmem FILEIO case..?
> >> commit 8f9f44f8957b262de717a48269a5ceca36c2b504
> >> Author: Nicholas Bellinger<nab@xxxxxxxxxxxxxxx>
> >> Date: Mon Oct 1 23:29:49 2012 -0700
> >>
> >> tcm_loop: Convert I/O path to use target_submit_cmd_map_sgls
> >>
> >> This patch converts tcm_loop to use target_submit_cmd_map_sgls() for
> >> I/O submission and mapping of pre-allocated SGL memory from incoming
> >> scsi_cmnd -> se_cmd descriptors.
> >>
> >> This includes removing the original open-coded fabric uses of target
> >> core callers to support transport_generic_map_mem_to_cmd() between
> >> target_setup_cmd_from_cdb() and transport_handle_cdb_direct() logic.
> >>
> >> (v2: Use renamed target_submit_cmd_map_sgls)
> >>
> >> Reported-by: Christoph Hellwig<hch@xxxxxx>
> >> Reviewed-by: Christoph Hellwig<hch@xxxxxx>
> >> Signed-off-by: Nicholas Bellinger<nab@xxxxxxxxxxxxxxx>
> >>
> >
> > Very strange. This is a non functional change for tcm_loop code using
> > the exact same underlying target_core_mod functions for se_cmd setup ->
> > SGL mapping -> se_cmd I/O dispatch ub target_submit_cmd_map_sg(), so I
> > don't see how it would effect this bug.
>
> Is it possible that now the memory allocation works different? I
> haven't look at the code…
>
Nope, the target is simply accepting pre-built SGLs from the tcm_loop
SCSI LLD, and skipping internal SGL allocation+setup all-together.
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
