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

[Hannes Reinecke <hare@suse.de>]

On 5/29/20 2:50 PM, Matthew Wilcox wrote:
> 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.
> 
> 
What I'm missing is the connection between the first paragraph and the 
second.
It starts with 'If you use ...', making no indication what would happen 
if you don't.
And the second paragraph just says '... to store the entry ...'; is 
never said anything about tracking entries.

Why not simply append this sentenct to the second paragraph:

In order to use xa_alloc() you need to pass the XA_FLAGS_ALLOC when
calling xa_init_flags()l

>> 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?
> 
It _could_ have said 'You forgot to pass XA_ALLOC_FLAGS in xa_init_flags()'.
Then it would've been immediately obvious without having to delve into 
xarray code.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer