[PATCH 2/2] export command allocation and freeing functions independently of the host

[PATCH 2/2] export command allocation and freeing functions independently of the host

[James Bottomley]

This is needed by things like USB storage that want to set up static
commands for later use at start of day.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/scsi.c      |  149 ++++++++++++++++++++++++++++++++++-----------
 include/scsi/scsi_cmnd.h |    3 +
 2 files changed, 115 insertions(+), 37 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index d70e608..65dbb3e 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -330,30 +330,16 @@ void scsi_put_command(struct scsi_cmnd *cmd)
 }
 EXPORT_SYMBOL(scsi_put_command);
 
-/**
- * scsi_setup_command_freelist - Setup the command freelist for a scsi host.
- * @shost: host to allocate the freelist for.
- *
- * Description: The command freelist protects against system-wide out of memory
- * deadlock by preallocating one SCSI command structure for each host, so the
- * system can always write to a swap file on a device associated with that host.
- *
- * Returns:	Nothing.
- */
-int scsi_setup_command_freelist(struct Scsi_Host *shost)
+static struct scsi_host_cmd_pool *scsi_get_host_cmd_pool(gfp_t gfp_mask)
 {
-	struct scsi_host_cmd_pool *pool;
-	struct scsi_cmnd *cmd;
-
-	spin_lock_init(&shost->free_list_lock);
-	INIT_LIST_HEAD(&shost->free_list);
-
+	struct scsi_host_cmd_pool *retval = NULL, *pool;
 	/*
 	 * Select a command slab for this host and create it if not
 	 * yet existent.
 	 */
 	mutex_lock(&host_cmd_pool_mutex);
-	pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool);
+	pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
+		&scsi_cmd_pool;
 	if (!pool->users) {
 		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
 						   sizeof(struct scsi_cmnd), 0,
@@ -371,28 +357,122 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
 	}
 
 	pool->users++;
-	shost->cmd_pool = pool;
+	retval = pool;
+ fail:
 	mutex_unlock(&host_cmd_pool_mutex);
+	return retval;
+}
+
+static void scsi_put_host_cmd_pool(gfp_t gfp_mask)
+{
+	struct scsi_host_cmd_pool *pool;
 
+	mutex_lock(&host_cmd_pool_mutex);
+	pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
+		&scsi_cmd_pool;
 	/*
-	 * Get one backup command for this host.
+	 * This may happen if a driver has a mismatched get and put
+	 * of the command pool; the driver should be implicated in
+	 * the stack trace
 	 */
-	cmd = scsi_get_command_from_pool(shost->cmd_pool, GFP_KERNEL);
-	if (!cmd)
-		goto fail2;
+	BUG_ON(pool->users == 0);
 
-	list_add(&cmd->list, &shost->free_list);
-	return 0;
-
- fail2:
-	mutex_lock(&host_cmd_pool_mutex);
 	if (!--pool->users) {
 		kmem_cache_destroy(pool->cmd_slab);
 		kmem_cache_destroy(pool->sense_slab);
 	}
- fail:
 	mutex_unlock(&host_cmd_pool_mutex);
-	return -ENOMEM;
+}
+
+/**
+ * scsi_allocate_command - get a fully allocated SCSI command
+ * @gfp_mask:	allocation mask
+ *
+ * This function is for use outside of the normal host based pools.
+ * It allocates the relevant command and takes an additional reference
+ * on the pool it used.  This function *must* be paired with
+ * scsi_free_command which also has the identical mask, otherwise the
+ * free pool counts will eventually go wrong and you'll trigger a bug.
+ *
+ * This function should *only* be used by drivers that need a static
+ * command allocation at start of day for internal functions.
+ */
+struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask)
+{
+	struct scsi_host_cmd_pool *pool = scsi_get_host_cmd_pool(gfp_mask);
+
+	if (!pool)
+		return NULL;
+
+	return scsi_get_command_from_pool(pool, gfp_mask);
+}
+EXPORT_SYMBOL(scsi_allocate_command);
+
+/**
+ * scsi_free_command - free a command allocated by scsi_allocate_command
+ * @gfp_mask:	mask used in the original allocation
+ * @cmd:	command to free
+ *
+ * Note: using the original allocation mask is vital because that's
+ * what determines which command pool we use to free the command.  Any
+ * mismatch will cause the system to BUG eventually.
+ */
+void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd)
+{
+	struct scsi_host_cmd_pool *pool = scsi_get_host_cmd_pool(gfp_mask);
+
+	/*
+	 * this could trigger if the mask to scsi_allocate_command
+	 * doesn't match this mask.  Otherwise we're guaranteed that this
+	 * succeeds because scsi_allocate_command must have taken a reference
+	 * on the pool
+	 */
+	BUG_ON(!pool);
+
+	scsi_put_command_to_pool(pool, cmd);
+	/*
+	 * scsi_put_host_cmd_pool is called twice; once to release the
+	 * reference we took above, and once to release the reference
+	 * originally taken by scsi_allocate_command
+	 */
+	scsi_put_host_cmd_pool(gfp_mask);
+	scsi_put_host_cmd_pool(gfp_mask);
+}
+EXPORT_SYMBOL(scsi_free_command);
+
+/**
+ * scsi_setup_command_freelist - Setup the command freelist for a scsi host.
+ * @shost: host to allocate the freelist for.
+ *
+ * Description: The command freelist protects against system-wide out of memory
+ * deadlock by preallocating one SCSI command structure for each host, so the
+ * system can always write to a swap file on a device associated with that host.
+ *
+ * Returns:	Nothing.
+ */
+int scsi_setup_command_freelist(struct Scsi_Host *shost)
+{
+	struct scsi_cmnd *cmd;
+	const gfp_t gfp_mask = shost->unchecked_isa_dma ? GFP_DMA : GFP_KERNEL;
+
+	spin_lock_init(&shost->free_list_lock);
+	INIT_LIST_HEAD(&shost->free_list);
+
+	shost->cmd_pool = scsi_get_host_cmd_pool(gfp_mask);
+
+	if (!shost->cmd_pool)
+		return -ENOMEM;
+
+	/*
+	 * Get one backup command for this host.
+	 */
+	cmd = scsi_get_command_from_pool(shost->cmd_pool, gfp_mask);
+	if (!cmd) {
+		scsi_put_host_cmd_pool(gfp_mask);
+		return -ENOMEM;
+	}
+	list_add(&cmd->list, &shost->free_list);
+	return 0;
 }
 
 /**
@@ -410,13 +490,8 @@ void scsi_destroy_command_freelist(struct Scsi_Host *shost)
 				cmd->sense_buffer);
 		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
 	}
-
-	mutex_lock(&host_cmd_pool_mutex);
-	if (!--shost->cmd_pool->users) {
-		kmem_cache_destroy(shost->cmd_pool->cmd_slab);
-		kmem_cache_destroy(shost->cmd_pool->sense_slab);
-	}
-	mutex_unlock(&host_cmd_pool_mutex);
+	shost->cmd_pool = NULL;
+	scsi_put_host_cmd_pool(shost->unchecked_isa_dma ? GFP_DMA : GFP_KERNEL);
 }
 
 #ifdef CONFIG_SCSI_LOGGING
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index de28aab..a13348c 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -130,6 +130,9 @@ extern void scsi_release_buffers(struct scsi_cmnd *cmd);
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
 
+struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask);
+void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd);
+
 static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
 {
 	return cmd->sdb.table.nents;
-- 
1.5.4.1

Re: [PATCH 2/2] export command allocation and freeing functions independently of the host

[Boaz Harrosh]

On Thu, Mar 13 2008 at 18:53 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> This is needed by things like USB storage that want to set up static
> commands for later use at start of day.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  drivers/scsi/scsi.c      |  149 ++++++++++++++++++++++++++++++++++-----------
>  include/scsi/scsi_cmnd.h |    3 +
>  2 files changed, 115 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index d70e608..65dbb3e 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -330,30 +330,16 @@ void scsi_put_command(struct scsi_cmnd *cmd)
>  }
>  EXPORT_SYMBOL(scsi_put_command);
>  
> -/**
> - * scsi_setup_command_freelist - Setup the command freelist for a scsi host.
> - * @shost: host to allocate the freelist for.
> - *
> - * Description: The command freelist protects against system-wide out of memory
> - * deadlock by preallocating one SCSI command structure for each host, so the
> - * system can always write to a swap file on a device associated with that host.
> - *
> - * Returns:	Nothing.
> - */
> -int scsi_setup_command_freelist(struct Scsi_Host *shost)
> +static struct scsi_host_cmd_pool *scsi_get_host_cmd_pool(gfp_t gfp_mask)
>  {
> -	struct scsi_host_cmd_pool *pool;
> -	struct scsi_cmnd *cmd;
> -
> -	spin_lock_init(&shost->free_list_lock);
> -	INIT_LIST_HEAD(&shost->free_list);
> -
> +	struct scsi_host_cmd_pool *retval = NULL, *pool;
>  	/*
>  	 * Select a command slab for this host and create it if not
>  	 * yet existent.
>  	 */
>  	mutex_lock(&host_cmd_pool_mutex);
> -	pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool);
> +	pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
> +		&scsi_cmd_pool;
>  	if (!pool->users) {
>  		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
>  						   sizeof(struct scsi_cmnd), 0,
> @@ -371,28 +357,122 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
>  	}
>  
>  	pool->users++;
> -	shost->cmd_pool = pool;
> +	retval = pool;
> + fail:
>  	mutex_unlock(&host_cmd_pool_mutex);
> +	return retval;
> +}
> +
> +static void scsi_put_host_cmd_pool(gfp_t gfp_mask)
> +{
> +	struct scsi_host_cmd_pool *pool;
>  
> +	mutex_lock(&host_cmd_pool_mutex);
> +	pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
> +		&scsi_cmd_pool;
>  	/*
> -	 * Get one backup command for this host.
> +	 * This may happen if a driver has a mismatched get and put
> +	 * of the command pool; the driver should be implicated in
> +	 * the stack trace
>  	 */
> -	cmd = scsi_get_command_from_pool(shost->cmd_pool, GFP_KERNEL);
> -	if (!cmd)
> -		goto fail2;
> +	BUG_ON(pool->users == 0);
>  
> -	list_add(&cmd->list, &shost->free_list);
> -	return 0;
> -
> - fail2:
> -	mutex_lock(&host_cmd_pool_mutex);
>  	if (!--pool->users) {
>  		kmem_cache_destroy(pool->cmd_slab);
>  		kmem_cache_destroy(pool->sense_slab);
>  	}
> - fail:
>  	mutex_unlock(&host_cmd_pool_mutex);
> -	return -ENOMEM;
> +}
> +
> +/**
> + * scsi_allocate_command - get a fully allocated SCSI command
> + * @gfp_mask:	allocation mask
> + *
> + * This function is for use outside of the normal host based pools.
> + * It allocates the relevant command and takes an additional reference
> + * on the pool it used.  This function *must* be paired with
> + * scsi_free_command which also has the identical mask, otherwise the
> + * free pool counts will eventually go wrong and you'll trigger a bug.
> + *
> + * This function should *only* be used by drivers that need a static
> + * command allocation at start of day for internal functions.
> + */
> +struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask)
> +{
> +	struct scsi_host_cmd_pool *pool = scsi_get_host_cmd_pool(gfp_mask);
> +
> +	if (!pool)
> +		return NULL;
> +
> +	return scsi_get_command_from_pool(pool, gfp_mask);
> +}
> +EXPORT_SYMBOL(scsi_allocate_command);
> +
> +/**
> + * scsi_free_command - free a command allocated by scsi_allocate_command
> + * @gfp_mask:	mask used in the original allocation
> + * @cmd:	command to free
> + *
> + * Note: using the original allocation mask is vital because that's
> + * what determines which command pool we use to free the command.  Any
> + * mismatch will cause the system to BUG eventually.
> + */
> +void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd)
> +{
> +	struct scsi_host_cmd_pool *pool = scsi_get_host_cmd_pool(gfp_mask);
> +
> +	/*
> +	 * this could trigger if the mask to scsi_allocate_command
> +	 * doesn't match this mask.  Otherwise we're guaranteed that this
> +	 * succeeds because scsi_allocate_command must have taken a reference
> +	 * on the pool
> +	 */
> +	BUG_ON(!pool);
> +
> +	scsi_put_command_to_pool(pool, cmd);
> +	/*
> +	 * scsi_put_host_cmd_pool is called twice; once to release the
> +	 * reference we took above, and once to release the reference
> +	 * originally taken by scsi_allocate_command
> +	 */
> +	scsi_put_host_cmd_pool(gfp_mask);
> +	scsi_put_host_cmd_pool(gfp_mask);
> +}
> +EXPORT_SYMBOL(scsi_free_command);
> +
> +/**
> + * scsi_setup_command_freelist - Setup the command freelist for a scsi host.
> + * @shost: host to allocate the freelist for.
> + *
> + * Description: The command freelist protects against system-wide out of memory
> + * deadlock by preallocating one SCSI command structure for each host, so the
> + * system can always write to a swap file on a device associated with that host.
> + *
> + * Returns:	Nothing.
> + */
> +int scsi_setup_command_freelist(struct Scsi_Host *shost)
> +{
> +	struct scsi_cmnd *cmd;
> +	const gfp_t gfp_mask = shost->unchecked_isa_dma ? GFP_DMA : GFP_KERNEL;
> +
> +	spin_lock_init(&shost->free_list_lock);
> +	INIT_LIST_HEAD(&shost->free_list);
> +
> +	shost->cmd_pool = scsi_get_host_cmd_pool(gfp_mask);
> +
> +	if (!shost->cmd_pool)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Get one backup command for this host.
> +	 */
> +	cmd = scsi_get_command_from_pool(shost->cmd_pool, gfp_mask);
> +	if (!cmd) {
> +		scsi_put_host_cmd_pool(gfp_mask);
> +		return -ENOMEM;
> +	}
> +	list_add(&cmd->list, &shost->free_list);
> +	return 0;
>  }
>  
>  /**
> @@ -410,13 +490,8 @@ void scsi_destroy_command_freelist(struct Scsi_Host *shost)
>  				cmd->sense_buffer);
>  		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
>  	}
> -
> -	mutex_lock(&host_cmd_pool_mutex);
> -	if (!--shost->cmd_pool->users) {
> -		kmem_cache_destroy(shost->cmd_pool->cmd_slab);
> -		kmem_cache_destroy(shost->cmd_pool->sense_slab);
> -	}
> -	mutex_unlock(&host_cmd_pool_mutex);
> +	shost->cmd_pool = NULL;
> +	scsi_put_host_cmd_pool(shost->unchecked_isa_dma ? GFP_DMA : GFP_KERNEL);
>  }
>  
>  #ifdef CONFIG_SCSI_LOGGING
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index de28aab..a13348c 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -130,6 +130,9 @@ extern void scsi_release_buffers(struct scsi_cmnd *cmd);
>  extern int scsi_dma_map(struct scsi_cmnd *cmd);
>  extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
>  
> +struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask);
> +void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd);
> +
>  static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
>  {
>  	return cmd->sdb.table.nents;

Both me and Andi Kleen have audited all ISA drivers and found that none of them
use scsi_cmnd->cmnd or any other scsi_cmnd member for DMA. (Now that sense_buffer
is not a part of it). Should we not also kill the two pools and simplify the code
allot. This can be done now independently of the removal of unchecked_isa_dma?

Boaz
  

Re: [PATCH 2/2] export command allocation and freeing functions independently of the host

[James Bottomley]

On Thu, 2008-03-13 at 19:04 +0200, Boaz Harrosh wrote:
> Both me and Andi Kleen have audited all ISA drivers and found that none of them
> use scsi_cmnd->cmnd or any other scsi_cmnd member for DMA. (Now that sense_buffer
> is not a part of it). Should we not also kill the two pools and simplify the code
> allot. This can be done now independently of the removal of unchecked_isa_dma?

Sure, but it's not logically part of this patch.

James

Re: [PATCH 2/2] export command allocation and freeing functions independently of the host

[Boaz Harrosh]

On Thu, Mar 13 2008 at 18:53 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> This is needed by things like USB storage that want to set up static
> commands for later use at start of day.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  drivers/scsi/scsi.c      |  149 ++++++++++++++++++++++++++++++++++-----------
>  include/scsi/scsi_cmnd.h |    3 +
>  2 files changed, 115 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index d70e608..65dbb3e 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -330,30 +330,16 @@ void scsi_put_command(struct scsi_cmnd *cmd)
>  }
>  EXPORT_SYMBOL(scsi_put_command);
>  
> -/**
> - * scsi_setup_command_freelist - Setup the command freelist for a scsi host.
> - * @shost: host to allocate the freelist for.
> - *
> - * Description: The command freelist protects against system-wide out of memory
> - * deadlock by preallocating one SCSI command structure for each host, so the
> - * system can always write to a swap file on a device associated with that host.
> - *
> - * Returns:	Nothing.
> - */
> -int scsi_setup_command_freelist(struct Scsi_Host *shost)
> +static struct scsi_host_cmd_pool *scsi_get_host_cmd_pool(gfp_t gfp_mask)
>  {
> -	struct scsi_host_cmd_pool *pool;
> -	struct scsi_cmnd *cmd;
> -
> -	spin_lock_init(&shost->free_list_lock);
> -	INIT_LIST_HEAD(&shost->free_list);
> -
> +	struct scsi_host_cmd_pool *retval = NULL, *pool;
>  	/*
>  	 * Select a command slab for this host and create it if not
>  	 * yet existent.
>  	 */
>  	mutex_lock(&host_cmd_pool_mutex);
> -	pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool);
> +	pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
> +		&scsi_cmd_pool;
>  	if (!pool->users) {
>  		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
>  						   sizeof(struct scsi_cmnd), 0,
> @@ -371,28 +357,122 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
>  	}
>  
>  	pool->users++;
> -	shost->cmd_pool = pool;
> +	retval = pool;
> + fail:
>  	mutex_unlock(&host_cmd_pool_mutex);
> +	return retval;
> +}
> +
> +static void scsi_put_host_cmd_pool(gfp_t gfp_mask)
> +{
> +	struct scsi_host_cmd_pool *pool;
>  
> +	mutex_lock(&host_cmd_pool_mutex);
> +	pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
> +		&scsi_cmd_pool;
>  	/*
> -	 * Get one backup command for this host.
> +	 * This may happen if a driver has a mismatched get and put
> +	 * of the command pool; the driver should be implicated in
> +	 * the stack trace
>  	 */
> -	cmd = scsi_get_command_from_pool(shost->cmd_pool, GFP_KERNEL);
> -	if (!cmd)
> -		goto fail2;
> +	BUG_ON(pool->users == 0);
>  
> -	list_add(&cmd->list, &shost->free_list);
> -	return 0;
> -
> - fail2:
> -	mutex_lock(&host_cmd_pool_mutex);
>  	if (!--pool->users) {
>  		kmem_cache_destroy(pool->cmd_slab);
>  		kmem_cache_destroy(pool->sense_slab);
>  	}
> - fail:
>  	mutex_unlock(&host_cmd_pool_mutex);
> -	return -ENOMEM;
> +}
> +
> +/**
> + * scsi_allocate_command - get a fully allocated SCSI command
> + * @gfp_mask:	allocation mask
> + *
> + * This function is for use outside of the normal host based pools.
> + * It allocates the relevant command and takes an additional reference
> + * on the pool it used.  This function *must* be paired with
> + * scsi_free_command which also has the identical mask, otherwise the
> + * free pool counts will eventually go wrong and you'll trigger a bug.
> + *
> + * This function should *only* be used by drivers that need a static
> + * command allocation at start of day for internal functions.
> + */
> +struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask)
> +{
> +	struct scsi_host_cmd_pool *pool = scsi_get_host_cmd_pool(gfp_mask);
> +
> +	if (!pool)
> +		return NULL;
> +
> +	return scsi_get_command_from_pool(pool, gfp_mask);
> +}
> +EXPORT_SYMBOL(scsi_allocate_command);
> +
> +/**
> + * scsi_free_command - free a command allocated by scsi_allocate_command
> + * @gfp_mask:	mask used in the original allocation
> + * @cmd:	command to free
> + *
> + * Note: using the original allocation mask is vital because that's
> + * what determines which command pool we use to free the command.  Any
> + * mismatch will cause the system to BUG eventually.
> + */
> +void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd)
> +{
> +	struct scsi_host_cmd_pool *pool = scsi_get_host_cmd_pool(gfp_mask);
> +
> +	/*
> +	 * this could trigger if the mask to scsi_allocate_command
> +	 * doesn't match this mask.  Otherwise we're guaranteed that this
> +	 * succeeds because scsi_allocate_command must have taken a reference
> +	 * on the pool
> +	 */
> +	BUG_ON(!pool);
> +
> +	scsi_put_command_to_pool(pool, cmd);
> +	/*
> +	 * scsi_put_host_cmd_pool is called twice; once to release the
> +	 * reference we took above, and once to release the reference
> +	 * originally taken by scsi_allocate_command
> +	 */
> +	scsi_put_host_cmd_pool(gfp_mask);
> +	scsi_put_host_cmd_pool(gfp_mask);
> +}
> +EXPORT_SYMBOL(scsi_free_command);
> +
> +/**
> + * scsi_setup_command_freelist - Setup the command freelist for a scsi host.
> + * @shost: host to allocate the freelist for.
> + *
> + * Description: The command freelist protects against system-wide out of memory
> + * deadlock by preallocating one SCSI command structure for each host, so the
> + * system can always write to a swap file on a device associated with that host.
> + *
> + * Returns:	Nothing.
> + */
> +int scsi_setup_command_freelist(struct Scsi_Host *shost)
> +{
> +	struct scsi_cmnd *cmd;
> +	const gfp_t gfp_mask = shost->unchecked_isa_dma ? GFP_DMA : GFP_KERNEL;
> +
> +	spin_lock_init(&shost->free_list_lock);
> +	INIT_LIST_HEAD(&shost->free_list);
> +
> +	shost->cmd_pool = scsi_get_host_cmd_pool(gfp_mask);
> +
> +	if (!shost->cmd_pool)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Get one backup command for this host.
> +	 */
> +	cmd = scsi_get_command_from_pool(shost->cmd_pool, gfp_mask);
> +	if (!cmd) {
> +		scsi_put_host_cmd_pool(gfp_mask);
> +		return -ENOMEM;
> +	}
> +	list_add(&cmd->list, &shost->free_list);
> +	return 0;
>  }
>  
>  /**
> @@ -410,13 +490,8 @@ void scsi_destroy_command_freelist(struct Scsi_Host *shost)
>  				cmd->sense_buffer);
>  		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
>  	}
> -
> -	mutex_lock(&host_cmd_pool_mutex);
> -	if (!--shost->cmd_pool->users) {
> -		kmem_cache_destroy(shost->cmd_pool->cmd_slab);
> -		kmem_cache_destroy(shost->cmd_pool->sense_slab);
> -	}
> -	mutex_unlock(&host_cmd_pool_mutex);
> +	shost->cmd_pool = NULL;
> +	scsi_put_host_cmd_pool(shost->unchecked_isa_dma ? GFP_DMA : GFP_KERNEL);
>  }
>  
>  #ifdef CONFIG_SCSI_LOGGING
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index de28aab..a13348c 100644
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -130,6 +130,9 @@ extern void scsi_release_buffers(struct scsi_cmnd *cmd);
>  extern int scsi_dma_map(struct scsi_cmnd *cmd);
>  extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
>  
> +struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask);
> +void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd);
> +
>  static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
>  {
>  	return cmd->sdb.table.nents;

Looking long term. This will clash with Matthew Wilcox's effort of 
overridable per host command pool. I do have a scsi_host in the USB
initialization. Perhaps:

+struct scsi_cmnd *scsi_allocate_command(struct Scsi_Host*, gfp_t gfp_mask);
+void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd);
+
  

Thanks
Boaz

Re: [PATCH 2/2] export command allocation and freeing functions independently of the host

[Boaz Harrosh]

On Thu, Mar 13 2008 at 19:35 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Thu, 2008-03-13 at 19:04 +0200, Boaz Harrosh wrote:
>> Both me and Andi Kleen have audited all ISA drivers and found that none of them
>> use scsi_cmnd->cmnd or any other scsi_cmnd member for DMA. (Now that sense_buffer
>> is not a part of it). Should we not also kill the two pools and simplify the code
>> allot. This can be done now independently of the removal of unchecked_isa_dma?
> 
> Sure, but it's not logically part of this patch.
> 
> James
> 

You are right, but I was hoping for a removal patch previous to this one.
Well OK, sorry, I will need to rebase anyway.

Boaz

Re: [PATCH 2/2] export command allocation and freeing functions independently of the host

[James Bottomley]

On Thu, 2008-03-13 at 19:39 +0200, Boaz Harrosh wrote:
> On Thu, Mar 13 2008 at 18:53 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > This is needed by things like USB storage that want to set up static
> > commands for later use at start of day.
> > 
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > ---
> >  drivers/scsi/scsi.c      |  149 ++++++++++++++++++++++++++++++++++-----------
> >  include/scsi/scsi_cmnd.h |    3 +
> >  2 files changed, 115 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > index d70e608..65dbb3e 100644
> > --- a/drivers/scsi/scsi.c
> > +++ b/drivers/scsi/scsi.c
> > @@ -330,30 +330,16 @@ void scsi_put_command(struct scsi_cmnd *cmd)
> >  }
> >  EXPORT_SYMBOL(scsi_put_command);
> >  
> > -/**
> > - * scsi_setup_command_freelist - Setup the command freelist for a scsi host.
> > - * @shost: host to allocate the freelist for.
> > - *
> > - * Description: The command freelist protects against system-wide out of memory
> > - * deadlock by preallocating one SCSI command structure for each host, so the
> > - * system can always write to a swap file on a device associated with that host.
> > - *
> > - * Returns:	Nothing.
> > - */
> > -int scsi_setup_command_freelist(struct Scsi_Host *shost)
> > +static struct scsi_host_cmd_pool *scsi_get_host_cmd_pool(gfp_t gfp_mask)
> >  {
> > -	struct scsi_host_cmd_pool *pool;
> > -	struct scsi_cmnd *cmd;
> > -
> > -	spin_lock_init(&shost->free_list_lock);
> > -	INIT_LIST_HEAD(&shost->free_list);
> > -
> > +	struct scsi_host_cmd_pool *retval = NULL, *pool;
> >  	/*
> >  	 * Select a command slab for this host and create it if not
> >  	 * yet existent.
> >  	 */
> >  	mutex_lock(&host_cmd_pool_mutex);
> > -	pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool);
> > +	pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
> > +		&scsi_cmd_pool;
> >  	if (!pool->users) {
> >  		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
> >  						   sizeof(struct scsi_cmnd), 0,
> > @@ -371,28 +357,122 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
> >  	}
> >  
> >  	pool->users++;
> > -	shost->cmd_pool = pool;
> > +	retval = pool;
> > + fail:
> >  	mutex_unlock(&host_cmd_pool_mutex);
> > +	return retval;
> > +}
> > +
> > +static void scsi_put_host_cmd_pool(gfp_t gfp_mask)
> > +{
> > +	struct scsi_host_cmd_pool *pool;
> >  
> > +	mutex_lock(&host_cmd_pool_mutex);
> > +	pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
> > +		&scsi_cmd_pool;
> >  	/*
> > -	 * Get one backup command for this host.
> > +	 * This may happen if a driver has a mismatched get and put
> > +	 * of the command pool; the driver should be implicated in
> > +	 * the stack trace
> >  	 */
> > -	cmd = scsi_get_command_from_pool(shost->cmd_pool, GFP_KERNEL);
> > -	if (!cmd)
> > -		goto fail2;
> > +	BUG_ON(pool->users == 0);
> >  
> > -	list_add(&cmd->list, &shost->free_list);
> > -	return 0;
> > -
> > - fail2:
> > -	mutex_lock(&host_cmd_pool_mutex);
> >  	if (!--pool->users) {
> >  		kmem_cache_destroy(pool->cmd_slab);
> >  		kmem_cache_destroy(pool->sense_slab);
> >  	}
> > - fail:
> >  	mutex_unlock(&host_cmd_pool_mutex);
> > -	return -ENOMEM;
> > +}
> > +
> > +/**
> > + * scsi_allocate_command - get a fully allocated SCSI command
> > + * @gfp_mask:	allocation mask
> > + *
> > + * This function is for use outside of the normal host based pools.
> > + * It allocates the relevant command and takes an additional reference
> > + * on the pool it used.  This function *must* be paired with
> > + * scsi_free_command which also has the identical mask, otherwise the
> > + * free pool counts will eventually go wrong and you'll trigger a bug.
> > + *
> > + * This function should *only* be used by drivers that need a static
> > + * command allocation at start of day for internal functions.
> > + */
> > +struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask)
> > +{
> > +	struct scsi_host_cmd_pool *pool = scsi_get_host_cmd_pool(gfp_mask);
> > +
> > +	if (!pool)
> > +		return NULL;
> > +
> > +	return scsi_get_command_from_pool(pool, gfp_mask);
> > +}
> > +EXPORT_SYMBOL(scsi_allocate_command);
> > +
> > +/**
> > + * scsi_free_command - free a command allocated by scsi_allocate_command
> > + * @gfp_mask:	mask used in the original allocation
> > + * @cmd:	command to free
> > + *
> > + * Note: using the original allocation mask is vital because that's
> > + * what determines which command pool we use to free the command.  Any
> > + * mismatch will cause the system to BUG eventually.
> > + */
> > +void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd)
> > +{
> > +	struct scsi_host_cmd_pool *pool = scsi_get_host_cmd_pool(gfp_mask);
> > +
> > +	/*
> > +	 * this could trigger if the mask to scsi_allocate_command
> > +	 * doesn't match this mask.  Otherwise we're guaranteed that this
> > +	 * succeeds because scsi_allocate_command must have taken a reference
> > +	 * on the pool
> > +	 */
> > +	BUG_ON(!pool);
> > +
> > +	scsi_put_command_to_pool(pool, cmd);
> > +	/*
> > +	 * scsi_put_host_cmd_pool is called twice; once to release the
> > +	 * reference we took above, and once to release the reference
> > +	 * originally taken by scsi_allocate_command
> > +	 */
> > +	scsi_put_host_cmd_pool(gfp_mask);
> > +	scsi_put_host_cmd_pool(gfp_mask);
> > +}
> > +EXPORT_SYMBOL(scsi_free_command);
> > +
> > +/**
> > + * scsi_setup_command_freelist - Setup the command freelist for a scsi host.
> > + * @shost: host to allocate the freelist for.
> > + *
> > + * Description: The command freelist protects against system-wide out of memory
> > + * deadlock by preallocating one SCSI command structure for each host, so the
> > + * system can always write to a swap file on a device associated with that host.
> > + *
> > + * Returns:	Nothing.
> > + */
> > +int scsi_setup_command_freelist(struct Scsi_Host *shost)
> > +{
> > +	struct scsi_cmnd *cmd;
> > +	const gfp_t gfp_mask = shost->unchecked_isa_dma ? GFP_DMA : GFP_KERNEL;
> > +
> > +	spin_lock_init(&shost->free_list_lock);
> > +	INIT_LIST_HEAD(&shost->free_list);
> > +
> > +	shost->cmd_pool = scsi_get_host_cmd_pool(gfp_mask);
> > +
> > +	if (!shost->cmd_pool)
> > +		return -ENOMEM;
> > +
> > +	/*
> > +	 * Get one backup command for this host.
> > +	 */
> > +	cmd = scsi_get_command_from_pool(shost->cmd_pool, gfp_mask);
> > +	if (!cmd) {
> > +		scsi_put_host_cmd_pool(gfp_mask);
> > +		return -ENOMEM;
> > +	}
> > +	list_add(&cmd->list, &shost->free_list);
> > +	return 0;
> >  }
> >  
> >  /**
> > @@ -410,13 +490,8 @@ void scsi_destroy_command_freelist(struct Scsi_Host *shost)
> >  				cmd->sense_buffer);
> >  		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
> >  	}
> > -
> > -	mutex_lock(&host_cmd_pool_mutex);
> > -	if (!--shost->cmd_pool->users) {
> > -		kmem_cache_destroy(shost->cmd_pool->cmd_slab);
> > -		kmem_cache_destroy(shost->cmd_pool->sense_slab);
> > -	}
> > -	mutex_unlock(&host_cmd_pool_mutex);
> > +	shost->cmd_pool = NULL;
> > +	scsi_put_host_cmd_pool(shost->unchecked_isa_dma ? GFP_DMA : GFP_KERNEL);
> >  }
> >  
> >  #ifdef CONFIG_SCSI_LOGGING
> > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> > index de28aab..a13348c 100644
> > --- a/include/scsi/scsi_cmnd.h
> > +++ b/include/scsi/scsi_cmnd.h
> > @@ -130,6 +130,9 @@ extern void scsi_release_buffers(struct scsi_cmnd *cmd);
> >  extern int scsi_dma_map(struct scsi_cmnd *cmd);
> >  extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
> >  
> > +struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask);
> > +void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd);
> > +
> >  static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
> >  {
> >  	return cmd->sdb.table.nents;
> 
> Looking long term. This will clash with Matthew Wilcox's effort of 
> overridable per host command pool.

Not really, since the design is to obtain commands outside of the normal
host pool allocations for special purposes.  All that needs to be
updated for the per host override is the setup and teardown path, which
can be done in a few lines.

>  I do have a scsi_host in the USB
> initialization. Perhaps:
> 
> +struct scsi_cmnd *scsi_allocate_command(struct Scsi_Host*, gfp_t gfp_mask);
> +void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd);

James

Re: [PATCH 2/2] export command allocation and freeing functions independently of the host

[Boaz Harrosh]

On Thu, Mar 13 2008 at 19:46 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Thu, 2008-03-13 at 19:39 +0200, Boaz Harrosh wrote:
>> On Thu, Mar 13 2008 at 18:53 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
>>> This is needed by things like USB storage that want to set up static
>>> commands for later use at start of day.
>>>
>>> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>>> ---
>>>  drivers/scsi/scsi.c      |  149 ++++++++++++++++++++++++++++++++++-----------
>>>  include/scsi/scsi_cmnd.h |    3 +
>>>  2 files changed, 115 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>>> index d70e608..65dbb3e 100644
>>> --- a/drivers/scsi/scsi.c
>>> +++ b/drivers/scsi/scsi.c
>>> @@ -330,30 +330,16 @@ void scsi_put_command(struct scsi_cmnd *cmd)
>>>  }
>>>  EXPORT_SYMBOL(scsi_put_command);
>>>  
>>> -/**
>>> - * scsi_setup_command_freelist - Setup the command freelist for a scsi host.
>>> - * @shost: host to allocate the freelist for.
>>> - *
>>> - * Description: The command freelist protects against system-wide out of memory
>>> - * deadlock by preallocating one SCSI command structure for each host, so the
>>> - * system can always write to a swap file on a device associated with that host.
>>> - *
>>> - * Returns:	Nothing.
>>> - */
>>> -int scsi_setup_command_freelist(struct Scsi_Host *shost)
>>> +static struct scsi_host_cmd_pool *scsi_get_host_cmd_pool(gfp_t gfp_mask)
>>>  {
>>> -	struct scsi_host_cmd_pool *pool;
>>> -	struct scsi_cmnd *cmd;
>>> -
>>> -	spin_lock_init(&shost->free_list_lock);
>>> -	INIT_LIST_HEAD(&shost->free_list);
>>> -
>>> +	struct scsi_host_cmd_pool *retval = NULL, *pool;
>>>  	/*
>>>  	 * Select a command slab for this host and create it if not
>>>  	 * yet existent.
>>>  	 */
>>>  	mutex_lock(&host_cmd_pool_mutex);
>>> -	pool = (shost->unchecked_isa_dma ? &scsi_cmd_dma_pool : &scsi_cmd_pool);
>>> +	pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
>>> +		&scsi_cmd_pool;
>>>  	if (!pool->users) {
>>>  		pool->cmd_slab = kmem_cache_create(pool->cmd_name,
>>>  						   sizeof(struct scsi_cmnd), 0,
>>> @@ -371,28 +357,122 @@ int scsi_setup_command_freelist(struct Scsi_Host *shost)
>>>  	}
>>>  
>>>  	pool->users++;
>>> -	shost->cmd_pool = pool;
>>> +	retval = pool;
>>> + fail:
>>>  	mutex_unlock(&host_cmd_pool_mutex);
>>> +	return retval;
>>> +}
>>> +
>>> +static void scsi_put_host_cmd_pool(gfp_t gfp_mask)
>>> +{
>>> +	struct scsi_host_cmd_pool *pool;
>>>  
>>> +	mutex_lock(&host_cmd_pool_mutex);
>>> +	pool = (gfp_mask & __GFP_DMA) ? &scsi_cmd_dma_pool :
>>> +		&scsi_cmd_pool;
>>>  	/*
>>> -	 * Get one backup command for this host.
>>> +	 * This may happen if a driver has a mismatched get and put
>>> +	 * of the command pool; the driver should be implicated in
>>> +	 * the stack trace
>>>  	 */
>>> -	cmd = scsi_get_command_from_pool(shost->cmd_pool, GFP_KERNEL);
>>> -	if (!cmd)
>>> -		goto fail2;
>>> +	BUG_ON(pool->users == 0);
>>>  
>>> -	list_add(&cmd->list, &shost->free_list);
>>> -	return 0;
>>> -
>>> - fail2:
>>> -	mutex_lock(&host_cmd_pool_mutex);
>>>  	if (!--pool->users) {
>>>  		kmem_cache_destroy(pool->cmd_slab);
>>>  		kmem_cache_destroy(pool->sense_slab);
>>>  	}
>>> - fail:
>>>  	mutex_unlock(&host_cmd_pool_mutex);
>>> -	return -ENOMEM;
>>> +}
>>> +
>>> +/**
>>> + * scsi_allocate_command - get a fully allocated SCSI command
>>> + * @gfp_mask:	allocation mask
>>> + *
>>> + * This function is for use outside of the normal host based pools.
>>> + * It allocates the relevant command and takes an additional reference
>>> + * on the pool it used.  This function *must* be paired with
>>> + * scsi_free_command which also has the identical mask, otherwise the
>>> + * free pool counts will eventually go wrong and you'll trigger a bug.
>>> + *
>>> + * This function should *only* be used by drivers that need a static
>>> + * command allocation at start of day for internal functions.
>>> + */
>>> +struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask)
>>> +{
>>> +	struct scsi_host_cmd_pool *pool = scsi_get_host_cmd_pool(gfp_mask);
>>> +
>>> +	if (!pool)
>>> +		return NULL;
>>> +
>>> +	return scsi_get_command_from_pool(pool, gfp_mask);
>>> +}
>>> +EXPORT_SYMBOL(scsi_allocate_command);
>>> +
>>> +/**
>>> + * scsi_free_command - free a command allocated by scsi_allocate_command
>>> + * @gfp_mask:	mask used in the original allocation
>>> + * @cmd:	command to free
>>> + *
>>> + * Note: using the original allocation mask is vital because that's
>>> + * what determines which command pool we use to free the command.  Any
>>> + * mismatch will cause the system to BUG eventually.
>>> + */
>>> +void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd)
>>> +{
>>> +	struct scsi_host_cmd_pool *pool = scsi_get_host_cmd_pool(gfp_mask);
>>> +
>>> +	/*
>>> +	 * this could trigger if the mask to scsi_allocate_command
>>> +	 * doesn't match this mask.  Otherwise we're guaranteed that this
>>> +	 * succeeds because scsi_allocate_command must have taken a reference
>>> +	 * on the pool
>>> +	 */
>>> +	BUG_ON(!pool);
>>> +
>>> +	scsi_put_command_to_pool(pool, cmd);
>>> +	/*
>>> +	 * scsi_put_host_cmd_pool is called twice; once to release the
>>> +	 * reference we took above, and once to release the reference
>>> +	 * originally taken by scsi_allocate_command
>>> +	 */
>>> +	scsi_put_host_cmd_pool(gfp_mask);
>>> +	scsi_put_host_cmd_pool(gfp_mask);
>>> +}
>>> +EXPORT_SYMBOL(scsi_free_command);
>>> +
>>> +/**
>>> + * scsi_setup_command_freelist - Setup the command freelist for a scsi host.
>>> + * @shost: host to allocate the freelist for.
>>> + *
>>> + * Description: The command freelist protects against system-wide out of memory
>>> + * deadlock by preallocating one SCSI command structure for each host, so the
>>> + * system can always write to a swap file on a device associated with that host.
>>> + *
>>> + * Returns:	Nothing.
>>> + */
>>> +int scsi_setup_command_freelist(struct Scsi_Host *shost)
>>> +{
>>> +	struct scsi_cmnd *cmd;
>>> +	const gfp_t gfp_mask = shost->unchecked_isa_dma ? GFP_DMA : GFP_KERNEL;
>>> +
>>> +	spin_lock_init(&shost->free_list_lock);
>>> +	INIT_LIST_HEAD(&shost->free_list);
>>> +
>>> +	shost->cmd_pool = scsi_get_host_cmd_pool(gfp_mask);
>>> +
>>> +	if (!shost->cmd_pool)
>>> +		return -ENOMEM;
>>> +
>>> +	/*
>>> +	 * Get one backup command for this host.
>>> +	 */
>>> +	cmd = scsi_get_command_from_pool(shost->cmd_pool, gfp_mask);
>>> +	if (!cmd) {
>>> +		scsi_put_host_cmd_pool(gfp_mask);
>>> +		return -ENOMEM;
>>> +	}
>>> +	list_add(&cmd->list, &shost->free_list);
>>> +	return 0;
>>>  }
>>>  
>>>  /**
>>> @@ -410,13 +490,8 @@ void scsi_destroy_command_freelist(struct Scsi_Host *shost)
>>>  				cmd->sense_buffer);
>>>  		kmem_cache_free(shost->cmd_pool->cmd_slab, cmd);
>>>  	}
>>> -
>>> -	mutex_lock(&host_cmd_pool_mutex);
>>> -	if (!--shost->cmd_pool->users) {
>>> -		kmem_cache_destroy(shost->cmd_pool->cmd_slab);
>>> -		kmem_cache_destroy(shost->cmd_pool->sense_slab);
>>> -	}
>>> -	mutex_unlock(&host_cmd_pool_mutex);
>>> +	shost->cmd_pool = NULL;
>>> +	scsi_put_host_cmd_pool(shost->unchecked_isa_dma ? GFP_DMA : GFP_KERNEL);
>>>  }
>>>  
>>>  #ifdef CONFIG_SCSI_LOGGING
>>> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
>>> index de28aab..a13348c 100644
>>> --- a/include/scsi/scsi_cmnd.h
>>> +++ b/include/scsi/scsi_cmnd.h
>>> @@ -130,6 +130,9 @@ extern void scsi_release_buffers(struct scsi_cmnd *cmd);
>>>  extern int scsi_dma_map(struct scsi_cmnd *cmd);
>>>  extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
>>>  
>>> +struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask);
>>> +void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd);
>>> +
>>>  static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
>>>  {
>>>  	return cmd->sdb.table.nents;
>> Looking long term. This will clash with Matthew Wilcox's effort of 
>> overridable per host command pool.
> 
> Not really, since the design is to obtain commands outside of the normal
> host pool allocations for special purposes.  All that needs to be
> updated for the per host override is the setup and teardown path, which
> can be done in a few lines.
> 

Again the concept was that an host might want a special size command for
the + host_priv additions that will get allocated once. This still applies
with "special purpose" commands, they need to be the size the host expects
them to be, so it can use container_of() macro to retrieve the real structure.
(Or any other dynamic size calculations)

>>  I do have a scsi_host in the USB
>> initialization. Perhaps:
>>
>> +struct scsi_cmnd *scsi_allocate_command(struct Scsi_Host*, gfp_t gfp_mask);
>> +void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd);
> 
> James
> 

I guess it can change later when needed. Just that I wanted that new users of the
API get used to the need of an Scsi_Host

My $0.02
Boaz

Re: [PATCH 2/2] export command allocation and freeing functions independently of the host

[James Bottomley]

On Thu, 2008-03-13 at 20:01 +0200, Boaz Harrosh wrote:
> On Thu, Mar 13 2008 at 19:46 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > On Thu, 2008-03-13 at 19:39 +0200, Boaz Harrosh wrote:
> >> On Thu, Mar 13 2008 at 18:53 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> >> Looking long term. This will clash with Matthew Wilcox's effort of 
> >> overridable per host command pool.
> > 
> > Not really, since the design is to obtain commands outside of the normal
> > host pool allocations for special purposes.  All that needs to be
> > updated for the per host override is the setup and teardown path, which
> > can be done in a few lines.
> > 
> 
> Again the concept was that an host might want a special size command for
> the + host_priv additions that will get allocated once. This still applies
> with "special purpose" commands, they need to be the size the host expects
> them to be, so it can use container_of() macro to retrieve the real structure.
> (Or any other dynamic size calculations)

Well, the currently presented interface is to tidy up the command use
for precisely two drivers, neither of which seems to want to use special
sized commands.  Even if that patch set were ready for merging (which it
isn't), adding it to the command allocators with no users would still be
over engineering.

> >>  I do have a scsi_host in the USB
> >> initialization. Perhaps:
> >>
> >> +struct scsi_cmnd *scsi_allocate_command(struct Scsi_Host*, gfp_t gfp_mask);
> >> +void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd);
> > 
> > James
> > 
> 
> I guess it can change later when needed. Just that I wanted that new users of the
> API get used to the need of an Scsi_Host

Yes, for just two users, particularly when there's doubt over whether
they'd even need the potential feature set, simpler is better.

James

Re: [PATCH 2/2] export command allocation and freeing functions independently of the host

[Boaz Harrosh]

On Thu, Mar 13 2008 at 20:20 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Thu, 2008-03-13 at 20:01 +0200, Boaz Harrosh wrote:
>> On Thu, Mar 13 2008 at 19:46 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
>>> On Thu, 2008-03-13 at 19:39 +0200, Boaz Harrosh wrote:
>>>> On Thu, Mar 13 2008 at 18:53 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
>>>> Looking long term. This will clash with Matthew Wilcox's effort of 
>>>> overridable per host command pool.
>>> Not really, since the design is to obtain commands outside of the normal
>>> host pool allocations for special purposes.  All that needs to be
>>> updated for the per host override is the setup and teardown path, which
>>> can be done in a few lines.
>>>
>> Again the concept was that an host might want a special size command for
>> the + host_priv additions that will get allocated once. This still applies
>> with "special purpose" commands, they need to be the size the host expects
>> them to be, so it can use container_of() macro to retrieve the real structure.
>> (Or any other dynamic size calculations)
> 
> Well, the currently presented interface is to tidy up the command use
> for precisely two drivers, neither of which seems to want to use special
> sized commands.  Even if that patch set were ready for merging (which it
> isn't), adding it to the command allocators with no users would still be
> over engineering.
> 
>>>>  I do have a scsi_host in the USB
>>>> initialization. Perhaps:
>>>>
>>>> +struct scsi_cmnd *scsi_allocate_command(struct Scsi_Host*, gfp_t gfp_mask);
>>>> +void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd);
>>> James
>>>
>> I guess it can change later when needed. Just that I wanted that new users of the
>> API get used to the need of an Scsi_Host
> 
> Yes, for just two users, particularly when there's doubt over whether
> they'd even need the potential feature set, simpler is better.
> 
> James
> 

Do you mean gdth? No I think gdth is not a user of this above new API.
gdth's patch I sent should be as it is.

I know of only one client of the new API presented here. isd200?

Boaz

Re: [PATCH 2/2] export command allocation and freeing functions independently of the host

[Andi Kleen]

On Thu, Mar 13, 2008 at 07:04:44PM +0200, Boaz Harrosh wrote:
> On Thu, Mar 13 2008 at 18:53 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > This is needed by things like USB storage that want to set up static
> > commands for later use at start of day.

So what's up with applying my patchkit? You rewriting part of code
that it touches.

I don't think there is anything controversal in there anymore.

-Andi