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
