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

[Douglas Gilbert <dgilbert@interlog.com>]

On 2020-05-29 8:46 a.m., 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.
> 
> It's not entirely obvious, and took me the better half of a day to figure out.

Ditto for me. At the time I did relay my frustration to Matthew who did
address it in the documentation. Another one is that xa_find*() requires the
XA_PRESENT mark, even when you are not using marks (or haven't yet learnt
about them ...). A clearer demarcation of the two xarray modes (i.e. either
the user supplies the index, or xarray does) would be helpful. That mode
impacts which parts of the xarray interface are used, for example in the sg
driver rewrite, xarrays are used for all collections but if memory serves,
there isn't a single xa_load() or xa_store() call.

But writing technical documentation is difficult. Very few third parties step
up to help, leaving the designer/implementer to do it. And it is extremely
difficult for someone who knows the code backwards (and where the skeletons are
buried) to stand on their heads and present the information in a pedagogic
manner.

Also traditional code documentation uses 7 bit ASCII text and "ACSII art" in
a sort of nod to earlier generations of programmers. Surely more modern
techniques, including colour diagrams and even animations, should now be
encouraged. Maybe when compilers start accepting html :-)

Doug Gilbert


P.S. I have tried to practice what I preach:
                http://sg.danny.cz/sg/sg_v40.html