On Wed, 17 Oct 2007 13:01:42 +0200
Jens Axboe <jens.axboe@xxxxxxxxxx> wrote:
> On Wed, Oct 17 2007, Jens Axboe wrote:
> > On Wed, Oct 17 2007, David Miller wrote:
> > > From: Jens Axboe <jens.axboe@xxxxxxxxxx>
> > > Date: Wed, 17 Oct 2007 11:16:29 +0200
> > >
> > > > On Wed, Oct 17 2007, David Miller wrote:
> > > > > From: Jens Axboe <jens.axboe@xxxxxxxxxx>
> > > > > Date: Wed, 17 Oct 2007 10:45:28 +0200
> > > > >
> > > > > > Righto, it's invalid to call sg_next() on the last entry!
> > > > >
> > > > > Unfortunately, that's what the sparc64 code wanted to do, this
> > > > > transformation in the sparc64 sg chaining patch is not equilavent:
> > > > >
> > > > > - struct scatterlist *sg_end = sg + nelems;
> > > > > + struct scatterlist *sg_end = sg_last(sg, nelems);
> > > > > ...
> > > > > - while (sg < sg_end &&
> > > > > + while (sg != sg_end &&
> > > >
> > > > Auch indeed. That'd probably be better as a
> > > >
> > > > do {
> > > > ...
> > > > } while (sg != sg_end);
> > >
> > > Ok, next bug, introduced by this change:
> > >
> > > commit f565913ef8a8d0cfa46a1faaf8340cc357a46f3a
> > > Author: Jens Axboe <jens.axboe@xxxxxxxxxx>
> > > Date: Fri Sep 21 10:44:19 2007 +0200
> > >
> > > block: convert to using sg helpers
> > >
> > > Convert the main rq mapper (blk_rq_map_sg()) to the sg helper setup.
> > >
> > > Signed-off-by: Jens Axboe <jens.axboe@xxxxxxxxxx>
> > >
> > > Specifically this part:
> > >
> > > new_segment:
> > > - memset(&sg[nsegs],0,sizeof(struct scatterlist));
> > > - sg[nsegs].page = bvec->bv_page;
> > > - sg[nsegs].length = nbytes;
> > > - sg[nsegs].offset = bvec->bv_offset;
> > > + sg = next_sg;
> > > + next_sg = sg_next(sg);
> > >
> > > + sg->page = bvec->bv_page;
> > > + sg->length = nbytes;
> > > + sg->offset = bvec->bv_offset;
> > >
> > > You can't remove that memset(), it's there for a reason. The IOMMU
> > > layers depended upon the code zero'ing out the whole scatterlist
> > > struct, there might be more to it than page, length and offset :-)
> >
> > I realize that, and I was pretty worried about this specific change. But
> > there's only been one piece of fallout because if it until now - well
> > two, with the sparc64 stuff.
> >
> > The problem is that you cannot zero the entire sg entry, because then
> > you'd potentially overwrite the chain pointer.
> >
> > I'd propose just adding a
> >
> > sg_dma_address(sg) = 0;
> > sg_dma_len(sg) = 0;
> >
> > there for now, or provide an arch_clear_sg_entry() helper if we need
> > more killed.
>
> Actually, just clearing AFTER sg_next() would be fine, since we know
> that is not a link entry. Duh...
>
> diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
> index 9eabac9..1014d34 100644
> --- a/block/ll_rw_blk.c
> +++ b/block/ll_rw_blk.c
> @@ -1352,6 +1352,7 @@ new_segment:
> sg = next_sg;
> next_sg = sg_next(sg);
>
> + memset(sg, 0, sizeof(*sg));
> sg->page = bvec->bv_page;
> sg->length = nbytes;
> sg->offset = bvec->bv_offset;
>
> --
So now how about removing zero'ing out sglist in scsi-ml?
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index aac8a02..0c86be7 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -764,8 +764,6 @@ struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
if (unlikely(!sgl))
goto enomem;
- memset(sgl, 0, sizeof(*sgl) * sgp->size);
-
/*
* first loop through, set initial index and return value
*/
-
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
