Re: [PATCH v3 24/31] elx: efct: LIO backend interface routines

James Smart
2020-04-22T04:20:11Z

On 4/11/2020 9:57 PM, Bart Van Assche wrote:
> On 2020-04-11 20:32, James Smart wrote:
>> + return EFC_SUCCESS;
>> +}
>
> Redefining 0 is unusual in the Linux kernel. I prefer to see "return 0;"
> instead of "return ${DRIVER_NAME}_SUCCESS;".

Well, seems we have two different opinions. Prior v2 reviews wanted a
consistent EFC_XXX set of enums for return codes.

I think it's very odd to return 0, and EFC_xxx elsewhere. The issue I
have with "we all know how to interpret 0" - I don't agree. 0 doesn't
always mean success. It's implicitly an assumption to believe 0 means
success.

I'd like to stay with EFC_SUCCESS for consistency.

>
...
> This is one of the most unfriendly debugfs data formats I have seen so
> far: information about all sessions is dumped into one huge debugfs
> attribute.
>
> Is information about active sessions useful for other LIO target
> drivers? Wasn't it promised that this functionality would be moved into
> the LIO core instead of defining it for the efct driver only?

We will remove the debugfs code. We will look to utilize Mike's patches
for session info.

>
>> +static int efct_debugfs_session_open(struct inode *inode, struct file *filp)
>> +{
>> + struct efct_lio_sport *sport = inode->i_private;
>> + int size = 17 * PAGE_SIZE; /* 34 byte per session*2048 sessions */
>> +
>> + if (!(filp->f_mode & FMODE_READ)) {
>> + filp->private_data = sport;
>> + return EFC_SUCCESS;
>> + }
>> +
>> + filp->private_data = kmalloc(size, GFP_KERNEL);
>> + if (!filp->private_data)
>> + return -ENOMEM;
>> +
>> + memset(filp->private_data, 0, size);
>> + efct_lio_tgt_session_data(sport->efct, sport->wwpn, filp->private_data,
>> + size);
>> + return EFC_SUCCESS;
>> +}
>
> kmalloc() + memset() can be changed into kzalloc().
>
> The above code allocates 68 KB physically contiguous memory? Kernel code
> should not rely on higher order page allocations unless there is no
> other choice.
>
> Additionally, I see that the amount of memory allocated is independent
> of the number of sessions. I think there are better approaches.

Yes. Will address it.

>
>> +
>> + lio_wq = create_singlethread_workqueue("efct_lio_worker");
>> + if (!lio_wq) {
>> + efc_log_err(efct, "workqueue create failed\n");
>> + return -ENOMEM;
>> + }
>
> Is any work queued onto lio_wq that needs to be serialized against other
> work queued onto the same queue? If not, can one of the system
> workqueues be used, e.g. system_wq?
>

We are using "lio_wq" here to call the LIO backed during creation or
deletion of sessions. LIO api's target_remove_session/
target_setup_session are blocking calls so we don't want to process them
in the interrupt thread. "lio_wq" is used for these two events only and
this brings the serialization to session management as we create single
threaded work queue.

-- James