[RFC 1/2] scsi core: alloc_cmnd

[RFC 1/2] scsi core: alloc_cmnd

[Matthew Wilcox]

This fairly naive patch introduces alloc_cmnd and destroy_cmnd.  I'm not
exactly happy about passing down the gfp_mask -- I'd prefer to be able
to allocate scsi_cmnds before we grab the queue_lock (and hence get
rid of the possibility we might need to GFP_ATOMIC), but I don't see
anywhere to do that.  Maybe there's a simple change we can make to the
block layer to allow it.

Merely-an-RFC-by: Matthew Wilcox <willy@linux.intel.com>

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1929488..30698da 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -160,8 +160,11 @@ struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost, gfp_t gfp_mask)
 {
 	struct scsi_cmnd *cmd;
 
-	cmd = kmem_cache_alloc(shost->cmd_pool->slab,
-			gfp_mask | shost->cmd_pool->gfp_mask);
+	if (shost->hostt->alloc_cmnd)
+		cmd = shost->hostt->alloc_cmnd(shost, gfp_mask);
+	else
+		cmd = kmem_cache_alloc(shost->cmd_pool->slab,
+					gfp_mask | shost->cmd_pool->gfp_mask);
 
 	if (unlikely(!cmd)) {
 		unsigned long flags;
@@ -230,8 +233,12 @@ void __scsi_put_command(struct Scsi_Host *shost, struct scsi_cmnd *cmd,
 	}
 	spin_unlock_irqrestore(&shost->free_list_lock, flags);
 
-	if (likely(cmd != NULL))
-		kmem_cache_free(shost->cmd_pool->slab, cmd);
+	if (likely(cmd != NULL)) {
+		if (shost->hostt->destroy_cmnd)
+			shost->hostt->destroy_cmnd(cmd);
+		else
+			kmem_cache_free(shost->cmd_pool->slab, cmd);
+	}
 
 	put_device(dev);
 }
@@ -280,31 +287,37 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
 	spin_lock_init(&shost->free_list_lock);
 	INIT_LIST_HEAD(&shost->free_list);
 
-	/*
-	 * Select a command slab for this host and create it if not
-	 * yet existant.
-	 */
-	mutex_lock(&host_cmd_pool_mutex);
-	pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool);
-	if (!pool->users) {
-		pool->slab = kmem_cache_create(pool->name,
-				sizeof(struct scsi_cmnd), 0,
-				pool->slab_flags, NULL);
-		if (!pool->slab)
-			goto fail;
-	}
+	if (shost->hostt->alloc_cmnd) {
+		cmd = shost->hostt->alloc_cmnd(shost, GFP_KERNEL);
+		if (!cmd)
+			return -ENOMEM;
+	} else {
+		/*
+		 * Select a command slab for this host and create it if not
+		 * yet existant.
+		 */
+		mutex_lock(&host_cmd_pool_mutex);
+		pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool);
+		if (!pool->users) {
+			pool->slab = kmem_cache_create(pool->name,
+					sizeof(struct scsi_cmnd), 0,
+					pool->slab_flags, NULL);
+			if (!pool->slab)
+				goto fail;
+		}
 
-	pool->users++;
-	shost->cmd_pool = pool;
-	mutex_unlock(&host_cmd_pool_mutex);
+		pool->users++;
+		shost->cmd_pool = pool;
+		mutex_unlock(&host_cmd_pool_mutex);
+		/*
+		 * Get one backup command for this host.
+		 */
+		cmd = kmem_cache_alloc(shost->cmd_pool->slab,
+				GFP_KERNEL | shost->cmd_pool->gfp_mask);
+		if (!cmd)
+			goto fail2;
+	}
 
-	/*
-	 * Get one backup command for this host.
-	 */
-	cmd = kmem_cache_alloc(shost->cmd_pool->slab,
-			GFP_KERNEL | shost->cmd_pool->gfp_mask);
-	if (!cmd)
-		goto fail2;
 	list_add(&cmd->list, &shost->free_list);		
 	return 0;
 
@@ -315,15 +328,13 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
  fail:
 	mutex_unlock(&host_cmd_pool_mutex);
 	return -ENOMEM;
-
 }
 
-/*
- * Function:	scsi_destroy_command_freelist()
- *
- * Purpose:	Release the command freelist for a scsi host.
+/**
+ * scsi_destroy_command_freelist - Release the command freelist for a scsi host
+ * @shost: Destroy the freelist for this host
  *
- * Arguments:	shost	- host that's freelist is going to be destroyed
+ * The @shost is being destroyed, so make sure we've freed this resource.
  */
 void scsi_destroy_command_freelist(struct Scsi_Host *shost)
 {
@@ -332,9 +343,15 @@ void scsi_destroy_command_freelist(struct Scsi_Host *shost)
 
 		cmd = list_entry(shost->free_list.next, struct scsi_cmnd, list);
 		list_del_init(&cmd->list);
-		kmem_cache_free(shost->cmd_pool->slab, cmd);
+		if (shost->hostt->destroy_cmnd)
+			shost->hostt->destroy_cmnd(cmd);
+		else
+			kmem_cache_free(shost->cmd_pool->slab, cmd);
 	}
 
+	if (shost->hostt->destroy_cmnd)
+		return;
+
 	mutex_lock(&host_cmd_pool_mutex);
 	if (!--shost->cmd_pool->users)
 		kmem_cache_destroy(shost->cmd_pool->slab);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 7d210cd..19e2241 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -284,6 +284,15 @@ struct scsi_host_template {
 	void (* scan_start)(struct Scsi_Host *);
 
 	/*
+	 * Fill in this pair of functions to allow the driver to allocate
+	 * extra per-command memory.
+	 *
+	 * Status: OPTIONAL (for now ...)
+	 */
+	struct scsi_cmnd *(*alloc_cmnd)(struct Scsi_Host *, gfp_t);
+	void (*destroy_cmnd)(struct scsi_cmnd *);
+
+	/*
 	 * fill in this function to allow the queue depth of this host
 	 * to be changeable (on a per device basis).  returns either
 	 * the current queue depth setting (may be different from what
-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [RFC 1/2] scsi core: alloc_cmnd

[Boaz Harrosh]

On Fri, Oct 19 2007 at 20:33 +0200, Matthew Wilcox <matthew@wil.cx> wrote:
> This fairly naive patch introduces alloc_cmnd and destroy_cmnd.  I'm not
> exactly happy about passing down the gfp_mask -- I'd prefer to be able
> to allocate scsi_cmnds before we grab the queue_lock (and hence get
> rid of the possibility we might need to GFP_ATOMIC), but I don't see
> anywhere to do that.  Maybe there's a simple change we can make to the
> block layer to allow it.
> 
> Merely-an-RFC-by: Matthew Wilcox <willy@linux.intel.com>

You know Matthew when you first talked about this, I envisioned
something else.

My idea was to have a new variable in scsi_host_template that states
the host_cmnd_extra_bytes. The mid-layer allocates a mempool with
sizeof(struct scsi_cmnd) + host_cmnd_extra_bytes.
A utility function will return that space for drivers given
a cmnd like:
void *host_cmnd_priv(struct scsi_cmnd *cmd)
{
	return cmd + 1;
}

This serves the drivers well because all they need to do is set
host_cmnd_extra_bytes = size_of(my_cmnd_stuff) and start using
it. much more simple than setting up cache pools. It also keeps
the Q per host vs Q per driver.

This also solves your problem with locks and allocation flags
since nothing changes, and it is all private to the mid-layer.

And it serves me, because when bidi comes I just do:
sizeof(struct scsi_cmnd) + host_cmnd_extra_bytes + sizeof(bidi_stuff)
for hosts that support bidi. And the rest of the work was done by you.

Do you need that I scribble some tryout patch for above + example driver

Thanks for doing this
Boaz

Re: [RFC 1/2] scsi core: alloc_cmnd

[Matthew Wilcox]

On Tue, Oct 23, 2007 at 06:49:17PM +0200, Boaz Harrosh wrote:
> You know Matthew when you first talked about this, I envisioned
> something else.

Right, so did I.  Christoph felt that alloc_cmnd/destroy_cmnd a la the
VFS layer's alloc_inode/destroy_inode was a better route.  I didn't have
a strong feeling, so I implemented that.

> This serves the drivers well because all they need to do is set
> host_cmnd_extra_bytes = size_of(my_cmnd_stuff) and start using
> it. much more simple than setting up cache pools. It also keeps
> the Q per host vs Q per driver.

The allocation pool is currently for the whole subsystem, not per host.
I assumed you meant 'pool' rather than 'queue', since I didn't touch the
queues.

Hosts already have allocation pools (er, I think they all do anyway
... and any that don't, I'm quite happy to write a generic_alloc_cmnd
and stick it in scsi_lib).

> This also solves your problem with locks and allocation flags
> since nothing changes, and it is all private to the mid-layer.

It doesn't solve that problem -- we already have that problem; it's just
that it's kept in the midlayer.

> And it serves me, because when bidi comes I just do:
> sizeof(struct scsi_cmnd) + host_cmnd_extra_bytes + sizeof(bidi_stuff)
> for hosts that support bidi. And the rest of the work was done by you.

I have to admit to not having a clear picture of how bidi is supposed to
work.  Could we do it something like this?

struct bidi_cmnd {
	struct scsi_cmnd;
	...
};

struct qla_cmnd {
	struct bidi_cmnd bd_cmnd;
	...
};

We could also add an alloc_bidi_cmnd/destroy_bidi_cmnd to the shost
template.  Presumably most commands won't be bidi for any given host,
so it'd be a waste of space to allocate them for all commands.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [RFC 1/2] scsi core: alloc_cmnd

[Boaz Harrosh]

On Tue, Oct 23 2007 at 19:48 +0200, Matthew Wilcox <matthew@wil.cx> wrote:
> On Tue, Oct 23, 2007 at 06:49:17PM +0200, Boaz Harrosh wrote:
>> You know Matthew when you first talked about this, I envisioned
>> something else.
> 
> Right, so did I.  Christoph felt that alloc_cmnd/destroy_cmnd a la the
> VFS layer's alloc_inode/destroy_inode was a better route.  I didn't have
> a strong feeling, so I implemented that.
> 
>> This serves the drivers well because all they need to do is set
>> host_cmnd_extra_bytes = size_of(my_cmnd_stuff) and start using
>> it. much more simple than setting up cache pools. It also keeps
>> the Q per host vs Q per driver.
> 
> The allocation pool is currently for the whole subsystem, not per host.
> I assumed you meant 'pool' rather than 'queue', since I didn't touch the
> queues.
> 
rrr Your right, I got confused here, the pools are global, I forgot.
That kind of kills my plan (I don't even dare a maybe)

> Hosts already have allocation pools (er, I think they all do anyway
> ... and any that don't, I'm quite happy to write a generic_alloc_cmnd
> and stick it in scsi_lib).
> 
Yes that would be nice. Make it as easy as possible for drivers.
Also for us when we sweep through them. (I'm here to help).

Most of them don't. Most of them either overload SCp or have
static arrays the size of their .can_queue .

>> This also solves your problem with locks and allocation flags
>> since nothing changes, and it is all private to the mid-layer.
> 
> It doesn't solve that problem -- we already have that problem; it's just
> that it's kept in the midlayer.
> 
OK I see...

>> And it serves me, because when bidi comes I just do:
>> sizeof(struct scsi_cmnd) + host_cmnd_extra_bytes + sizeof(bidi_stuff)
>> for hosts that support bidi. And the rest of the work was done by you.
> 
> I have to admit to not having a clear picture of how bidi is supposed to
> work.  Could we do it something like this?
> 
> struct bidi_cmnd {
> 	struct scsi_cmnd;
> 	...
> };
> 
Yes nice

> struct qla_cmnd {
> 	struct bidi_cmnd bd_cmnd;
> 	...
> };
> 
yes OK

> We could also add an alloc_bidi_cmnd/destroy_bidi_cmnd to the shost
> template.  Presumably most commands won't be bidi for any given host,
> so it'd be a waste of space to allocate them for all commands.
> 

Well no one really knows. The OSD scsi devices I use are bidi only commands
(OK not only, 99%). The rest are not yet defined. (Like raid arrays that do 
write-return-XOR)

Boaz

Re: [RFC 1/2] scsi core: alloc_cmnd

[Matthew Wilcox]

On Tue, Oct 23, 2007 at 08:46:52PM +0200, Boaz Harrosh wrote:
> > We could also add an alloc_bidi_cmnd/destroy_bidi_cmnd to the shost
> > template.  Presumably most commands won't be bidi for any given host,
> > so it'd be a waste of space to allocate them for all commands.
> 
> Well no one really knows. The OSD scsi devices I use are bidi only commands
> (OK not only, 99%). The rest are not yet defined. (Like raid arrays that do 
> write-return-XOR)

What's the usage scenario though?  Do we envisage one scsi_host being
dedicated to OSD, or do we envisage OSDs being one component on a FC
network?  I suspect the latter, which leads me to want to do something
like ...

struct qla_cmnd {
	char *sp;
	unsigned int compl_status;
	unsigned int resid_len;
	unsigned int scsi_status;
	unsigned int actual_snslen;
	unsigned int entry_status;
}

struct qla_bidi_cmnd {
	struct bidi_cmnd;
	struct qla_cmnd;
}

struct qla_cmnd {
	struct scsi_cmnd;
	struct qla_cmnd;
}

But then this requires us to have a bidi_queue_command.  That might not
be such a bad idea anyway ...

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [RFC 1/2] scsi core: alloc_cmnd

[Christoph Hellwig]

On Tue, Oct 23, 2007 at 11:48:04AM -0600, Matthew Wilcox wrote:
> On Tue, Oct 23, 2007 at 06:49:17PM +0200, Boaz Harrosh wrote:
> > You know Matthew when you first talked about this, I envisioned
> > something else.
> 
> Right, so did I.  Christoph felt that alloc_cmnd/destroy_cmnd a la the
> VFS layer's alloc_inode/destroy_inode was a better route.  I didn't have
> a strong feeling, so I implemented that.

Yeah.  Let me repeat my rationale for the method here:  If we only
have a size member that means we can only allocate things using the
slab allocator.  But there's a variety of drivers requiring other
allocations (e.g. pci_pool or the sym2 allocator) aswell, and having
the methods means we can always have a pre-constructed command for those
aswell, which makes these drivers deadlock-safe without much effort.

Re: [RFC 1/2] scsi core: alloc_cmnd

[Boaz Harrosh]

On Tue, Oct 23 2007 at 21:15 +0200, Matthew Wilcox <matthew@wil.cx> wrote:
> On Tue, Oct 23, 2007 at 08:46:52PM +0200, Boaz Harrosh wrote:
>>> We could also add an alloc_bidi_cmnd/destroy_bidi_cmnd to the shost
>>> template.  Presumably most commands won't be bidi for any given host,
>>> so it'd be a waste of space to allocate them for all commands.
>> Well no one really knows. The OSD scsi devices I use are bidi only commands
>> (OK not only, 99%). The rest are not yet defined. (Like raid arrays that do 
>> write-return-XOR)
> 
> What's the usage scenario though?  Do we envisage one scsi_host being
> dedicated to OSD, or do we envisage OSDs being one component on a FC
> network?  I suspect the latter, which leads me to want to do something
> like ...
> 
Yes, like I have OSD devices on the other side of an iscsi transport.
So there can be other none OSD devices on the iscsi initiator. It's
a per device thing.

> struct qla_cmnd {
> 	char *sp;
> 	unsigned int compl_status;
> 	unsigned int resid_len;
> 	unsigned int scsi_status;
> 	unsigned int actual_snslen;
> 	unsigned int entry_status;
> }
> 
> struct qla_bidi_cmnd {
> 	struct bidi_cmnd;
> 	struct qla_cmnd;
> }
> 
> struct qla_cmnd {
> 	struct scsi_cmnd;
> 	struct qla_cmnd;
> }
> 
> But then this requires us to have a bidi_queue_command.  That might not
> be such a bad idea anyway ...
> 
The current solution is pretty simple, we hang an extra scsi_data_buffer on
cmd->request->next_rq->special and allocate it when needed from a new cache_mem_pool
of bidi scsi_data_buffer's. Since for an OSD device all Cmnds are bidi, I thought
of saving that extra allocation. But it looks like the current model is not working
for my case, so I'll keep it as it is now.

You can see an example of this here:
http://www.bhalevy.com/open-osd/download/bidi_2.6.23/0004-SCSI-bidi-support.patch

Thanks
Boaz
 

Re: [RFC 1/2] scsi core: alloc_cmnd

[Christoph Hellwig]

On Fri, Oct 19, 2007 at 12:33:31PM -0600, Matthew Wilcox wrote:
> 
> This fairly naive patch introduces alloc_cmnd and destroy_cmnd.  I'm not
> exactly happy about passing down the gfp_mask -- I'd prefer to be able
> to allocate scsi_cmnds before we grab the queue_lock (and hence get
> rid of the possibility we might need to GFP_ATOMIC), but I don't see
> anywhere to do that.  Maybe there's a simple change we can make to the
> block layer to allow it.

This looks fine to.  It might be nice to factor the code out into
a few more helpers:

static struct scsi_cmnd *scsi_alloc_cmnd(struct Scsi_Host *shost, gfp_t mask)
{
	if (shost->hostt->alloc_cmnd)
		return shost->hostt->alloc_cmnd(shost, mask);
	else
		return kmem_cache_alloc(shost->cmd_pool->slab,
					mask | shost->cmd_pool->gfp_mask);
}

static struct scsi_cmnd *scsi_destroy_cmnd(struct Scsi_Host *shost,
		struct scsi_cmnd *cmd)
{
	if (likely(cmd != NULL)) {
		if (shost->hostt->destroy_cmnd)
			shost->hostt->destroy_cmnd(cmd);
		else
			kmem_cache_free(shost->cmd_pool->slab, cmd);
	}
}

plus helper to setup/teardown the pools if nessecary so that the code
flow is easier to read.  Also maybe a s/destroy/free/.  The VFS uses
destroy for inodes, but I'd personally prefer free.