On Sat, May 28, 2011 at 07:23:43PM -0700, Andy Grover wrote: > I guess this is my fault for not CCing linux-scsi. I'm working on tcm > cleanups and have been sending them to linux-iscsi-target-dev, because > there are a lot of them. Aren't you guys on that list too? I'm not on it. In fact I didn't really know about it until known. I'll take all my complains back in that case, my fault for not beeing on the proper list. > In any case, here's my git tree for review hch, I've been developing > against nab's lio-41, under the perhaps mistaken assumption that it all > would go in and then patched up to snuff: > > git://fedorapeople.org/home/fedora/grover/public_git/linux-2.6.git for-nab Looks pretty good, and removes a few items from my todo list. Some comments on the patches: "target: get_cdb should never return NULL": looks good by itself, but I think the real fix here is to not leave the CDB allocation to the I/O backend, but just include it in the se_task. Or maybe even in the se_cmd once that whole CDB splitting mess is moved into pscsi, which will simplify the common code further, "target: inline struct se_transport_task into struct se_task" The description is wrong, it's moved into the se_cmd, which is the correct thing to do anyway. "target: Rename transport_generic_handle_cdb to _new_cmd" About the naming: For now it's transport_generic for exported APIs I think. IMHO it's a pretty bad choice and should be replaced by target_, but let's do that in one big sweep. "target: Have iscsi fabric allocate its own buffers" ceil seems to be a reimplementation of DIV_ROUND_UP from kernel.h "target/iscsi: Do not use t_mem_list anymore" Very nice cleanup! "target: Call transport_new_cmd instead of adding to cmd queue" Did you test all other frontends that they are fine with this change? Also the code really needs to handle errors from transport_new_cmd. The comment above transport_generic_new_cmd needs an update, or even better the wrapper should be killed and callers should use transport_new_cmd directly. The TRANSPORT_NEW_CMD enum value and code switch case in transport_processing_thread cab be removed now. Btw, it seems like transport_processing_thread and the various helpers to queue up work to it could be nicely cleaned up by using a workqueue with a work item embedded into the se_cmd. That way this work could a) be made to scale to multiple CPUs, and b) we could kill new_cmd_map by just allowing the frontend to setup their own work queue handlers instead of going through this abstraction. "target/file: Alloc iov[] off the stack" We can get pretty large iovecs, and blindly allocation them on the stack seems like a bad idea. It might make sense to do so for the fast path, which means we'd need a __fd_do_readv that gets the vector passed, and then a smart wrapper using the stack for small vectors, and some technic making sure gcc doesn't bloat the stack otherwise. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
