Re: [PATCH 3/4] scsi: move target device list to xarray

[Matthew Wilcox <willy@infradead.org>]

On Fri, May 29, 2020 at 02:46:48PM +0200, Hannes Reinecke wrote:
> On 5/29/20 1:21 PM, Matthew Wilcox wrote:
> > On Fri, May 29, 2020 at 08:50:21AM +0200, Hannes Reinecke wrote:
> > > > I meant just use xa_alloc() for everything instead of using xa_insert for
> > > > 0-255.
> > > > 
> > > But then I'll have to use xa_find() to get to the actual element as the 1:1
> > > mapping between SCSI LUN and array index is lost.
> > > And seeing that most storage arrays will expose only up to 256 LUNs I
> > > thought this was a good improvement on lookup.
> > > Of course, this only makes sense if xa_load() is more efficient than
> > > xa_find(). If not then of course it's a bit futile.
> > 
> > xa_load() is absolutely more efficient than xa_find().  It's just a
> > question of whether it matters ;-)  Carry on ...
> > 
> Thanks. I will.
> 
> BTW, care to update the documentation?
> 
>  * Return: 0 on success, -ENOMEM if memory could not be allocated or
>  * -EBUSY if there are no free entries in @limit.
>  */
> int __xa_alloc(struct xarray *xa, u32 *id, void *entry,
> 		struct xa_limit limit, gfp_t gfp)
> {
> 	XA_STATE(xas, xa, 0);
> 
> 	if (WARN_ON_ONCE(xa_is_advanced(entry)))
> 		return -EINVAL;
> 	if (WARN_ON_ONCE(!xa_track_free(xa)))
> 		return -EINVAL;
> 
> So looks as if the documentation is in need of updating to cover -EINVAL.
> And, please, state somewhere that whenever you want to use xa_alloc() you
> _need_ to specify XA_ALLOC_FLAGS in xa_init_flags(), otherwise
> you trip across the above.

Like this?

Allocating XArrays
------------------

If you use DEFINE_XARRAY_ALLOC() to define the XArray, or
initialise it by passing ``XA_FLAGS_ALLOC`` to xa_init_flags(),
the XArray changes to track whether entries are in use or not.

You can call xa_alloc() to store the entry at an unused index
in the XArray.  If you need to modify the array from interrupt context,
you can use xa_alloc_bh() or xa_alloc_irq() to disable
interrupts while allocating the ID.


> It's not entirely obvious, and took me the better half of a day to figure
> out.

Really?  You get a nice WARN_ON and backtrace so you can see where
you went wrong ... What could I change to make this easier?