On Mon, Jun 10, 2019 at 04:02:27PM -0700, James Bottomley wrote:
> On Mon, 2019-06-10 at 15:36 -0400, Ewan D. Milne wrote:
> > On Mon, 2019-06-10 at 11:37 -0700, James Bottomley wrote:
> > > On Mon, 2019-06-10 at 23:03 +0800, Ming Lei wrote:
> > > > The current way isn't safe for chained sgl, so use sgl helper to
> > > > operate sgl.
> > >
> > > The advansys driver doesn't currently use a chained
> > > scatterlist. In
> > > theory it could; the
> > >
> > > if (shost->sg_tablesize > SG_ALL) {
> > > shost->sg_tablesize = SG_ALL;
> > > }
> > >
> > > At around line 11226 is what prevents it and that could be
> > > eliminated provided someone actually has the hardware to test.
> > >
> > > However, provided drivers make the correct SG_ALL or less
> > > declaration, they're entitled to treat scatterlists as fully
> > > contiguous, so there's no real justification (beyond uniformity)
> > > for making it use the chain helpers.
> > >
> > > James
> > >
> >
> > I thought the whole issue came about because Ming's earlier changes
> > to scsi_lib.c made the previously SG_CHUNK_SIZE scatterlist allocated
> > with the struct request much smaller, (SCSI_INLINE_SG_CNT is 2) so
> > everything needs to support it?
>
> Um, well, I'm just going by the commit log. If the problem is someone
> broke the SG_ALL no chaining assumption in an earlier commit, finger
> that as the buggy commit you're fixing rather than implying the drivers
> had a longstanding bug. In fact, preferably do this work as a
> precursor to the original instead of leaving the drivers broken for a
> bisect.
Yeah, these drivers should have been converted from the beginning.
Let's discuss how to move on now:
1) we introduce the following patches to avoid big pre-allocation since
it won't be an accepted behaviour to consume GB maybe for nothing:
scsi: core: avoid pre-allocating big SGL for data
scsi: core: avoid pre-allocating big SGL for protection information
scsi: lib/sg_pool.c: improve APIs for allocating sg pool
2) it is flexible to reserve a small SGL for avoiding runtime
allocation, which needs driver to support chained SG.
3) I grep all scsi drivers, and found most of them are chained sgl
ready, except for the 6 drivers in this patchset and esi_scsi which
is fixed already.
Given the above 3 patches are just in 5.3/scsi-queue, I am fine with
either way:
1) revert the 3 first, then re-organize the whole patchset in correct
order(convert drivers first, then the 3 above drivers)
2) simply apply the 5 patches now
Any comments?
Thanks,
Ming
