[PATCH] nvmet: parametrize maximum number of ANA groups

[Hannes Reinecke]

On 2/22/22 11:25, Alex Talker wrote:
>> Please no module param for this, right place is configfs subsys
>> attr.
> 
> Did you meant port attribute? Since the value mainly makes sense there.
> 
No, I _did_ mean the subsystem.
anagrpmax and nanagrpids are values in the identify controller data, and
will need to be derived from the subsystem values.

Also, the port merely controls which ANA groups are visible through that
port. It does not define how many ANA groups are present, as strictly
speaking this is a per-subsystem value, and it might well be that not
all ANA groups values are available through all ports.

> The main obstacle for me here, since again,
> i wanted to keep the change as simple as possible,
> is the 'nvmet_ana_group_enabled' "array".

And my suggestion is to keep that, with the difference being that each
subsystem can restrict how many ANA groups they will be using.

> I have a real doubt that I do understand what it is for much
> but the main issue would be if maximum number of ANA groups would
> differentiate per-port,
> then how does one pre-allocates such an 'array'?
> What this should be refactored to then?
> 

Primarily the patch will be introducing eg a per-subsystem
'attr_nanagrpids' configfs attribute. That will be set to
NVMET_MAX_ANAGRPS initially.

And then the 'nanagrpids' value will be used as the upper limit for
iterating in nvmet_execute_get_log_page_ana().

And when creating the 'subsystem' link in the configfs ports directory
we need to check if the ANA group IDs in the port matches with what the
subsystem declares, and refuse the link otherwise.

>> Also, if you are doing this please make sure to write a block test
>> for these two configfs params, since it definitely adds a code that
>> user input might missuse with invalid numbers.
> 
> Could you please give me an example of such a test?
> 
See above. You'll need to add a check in nvmet_port_subsys_allow_link()
to check the ana grp ids in the port against the maximum number of ana
groups as specified by the subsystem.

> And...
> 
>> We should keep anagrpmax a compile-time default, and have 'nanagrpids'
> a per-subsystem configfs setting. That way we can limit the memory
> requirements on the connecting host (as it can/will size the ANA log
> buffer by nanagrpids), and have the value configurable per subsystem.
> 
> Would that imply that ANAGRPMAX would be set to less than the maximum
> possible value? If so, which reasonable value do you think will fit?
> 2^16 or more?

Yes, I would have it less than the possible value, as there might be
implementation rely on that value to size internal structures, and we
don't want to overload them. I would put them at 1024, to have a nice
round number.

> Again, why 'nanagrpids' would be a subsystem attribute if validation
> comes into play(for creation of ANA group) on the port level at the moment?
> And the same issue as before, what to do if the value decreases while
> groups are in use? Deny the change?
> 
As mentioned, this is to reduce the memory consumption on the host side.
Linux is using nanagrpids to size the ANA log buffer, so if we put an
arbitrarily large number in here we'll end up having a massive memory
allocation just for the ANA log buffer (we've measured an order-7
allocation with one particular installation). And by allowing to have a
lower number here we'll get both, supporting large installations _and_
reduce the memory consumption.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		        Kernel Storage Architect
hare at suse.de			               +49 911 74053 688
SUSE Software Solutions Germany GmbH, 90409 Nürnberg
GF: F. Imendörffer, HRB 36809 (AG Nürnberg)