Re: [PATCH 2/2] scsi: core: avoid to pre-allocate big chunk for sg list

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Wed, 2019-04-24 at 08:32 -0700, Bart Van Assche wrote:
> On Wed, 2019-04-24 at 08:24 -0700, James Bottomley wrote:
> > On Wed, 2019-04-24 at 15:52 +0800, Ming Lei wrote:
> > > On Tue, Apr 23, 2019 at 08:37:15AM -0700, Bart Van Assche wrote:
> > > > On Tue, 2019-04-23 at 18:32 +0800, Ming Lei wrote:
> > > > >  #define  SCSI_INLINE_PROT_SG_CNT  1
> > > > >  
> > > > > +#define  SCSI_INLINE_SG_CNT  2
> > > > 
> > > > So this patch inserts one kmalloc() and one kfree() call in the
> > > > hot path for every SCSI request with more than two elements in
> > > > its scatterlist? Isn't
> > > 
> > > Slab or its variants are designed for fast path, and NVMe PCI
> > > uses slab for allocating sg list in fast path too.
> > 
> > Actually, that's not really true  base kmalloc can do all sorts of
> > things including kick off reclaim so it's not really something we
> > like using in the fast path.  The only fast and safe kmalloc you
> > can rely on  in the fast path is GFP_ATOMIC which will fail quickly
> > if no memory can easily be found.  *However* the sg_table
> > allocation functions are all pool backed (lib/sg_pool.c), so they
> > use the lightweight GFP_ATOMIC mechanism for kmalloc initially
> > coupled with a backing pool in case of failure to ensure forward
> > progress.
> > 
> > So, I think you're both right: you shouldn't simply use kmalloc,
> > but this implementation doesn't, it uses the sg_table allocation
> > functions which correctly control kmalloc to be lightweight and
> > efficient and able to make forward progress.
> 
> Another concern is whether this change can cause a livelock. If the
> system is running out of memory and the page cache submits a write
> request with a scatterlist with more than two elements, if the
> kmalloc() for the scatterlist fails, will that prevent the page cache
> from making any progress with writeback?

It's pool backed, as I said.  Is the concern there isn't enough depth
in the pools for a large write?

James