[PATCH libnvme] fabrics: don't pass sqflow_disable if the kernel does not support the option

[Sagi Grimberg]

On 7/24/23 19:18, Daniel Wagner wrote:
> On Mon, Jul 24, 2023 at 06:35:55PM +0300, Sagi Grimberg wrote:
>>> How do we know options is initialized here? It looks like that's done
>>> in __nvmf_supported_options(), called from nvmf_add_ctrl(), which is
>>> called a few lines later in this function.
> 
> I've paired __nvmf_supported_options() with build_options(). We can
> move the call to __nvmf_supported_options() further up if it helps.

We really should just have an interface to check specific options,
because that is what consumers care about, not the entire opts set.

Inside you can do the open/read/close once and cache it in memory
(although I think this is a really unneeded optimization).

> 
>> This is for connecting discovery log entries, so the kernel cap was
>> already read once for the discovery controller.
>>
>> Although granted, this can be udev driven on an existing discovery
>> controller. Probably need to do it regardless of adding a controller.
>>
>> I remember when adding the capability to check supported opts I added
>> an interface to do it per feature. Not sure why it was changed, it is
>> much more sane to just read it on demand given that there is just a
>> simple and short string copy in the kernel...
> 
> I was trying to avoid to do a open/read/parse/close cycle for each argument we
> are going append in the build_options() function.
> 
>> Ideally this would become:
>> 	if (e->treq & NVMF_TREQ_DISABLE_SQFLOW &&
>> 	    nvmf_check_cap("disable_sqflow"))
>> 		c->cfg.disable_sqflow = true;
>> 	else
>> 		c->cfg.disable_sqflow = false;
>>
>> Daniel? Any reason why we are caching these opts in an executable
>> with a short lifetime? And apparently we need to be very precise when
>> we rely on this cache to be stable.
> 
> Ah I see, in this case let's bring nvmf_check_cap() (although I couldn't find it
> anywhere... I can remember I saw those patches)

I also remember making disable_sqflow check instead of a retry, so maybe
it got left behind...

> 
>> If you prefer to cache it, at least do it in the start of every
>> fabrics callback so we don't need to understand when this cache
>> is valid...
> 
> I don't want to cache it, I just wanted to avoid the above mentioned
> open/read/parse/close cycle for each build options.

So do you want to add the interface? Or should I? (assuming you agree
on individual opt check) ?