Re: [PATCH 0/42 RESEND+NEW] Target updates for May 27

[Christoph Hellwig <hch@infradead.org>]

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.