Re: [PATCH] fixes and cleanups for the new command allocation code

[Patrick Mansfield <patmans@us.ibm.com>]

On Tue, Feb 04, 2003 at 04:23:27PM +0100, Christoph Hellwig wrote:

> -static int check_all_luns(struct Scsi_Host *shost, struct scsi_device *myself)
> -{
> -	struct scsi_device *sdev;
> -
> -	list_for_each_entry(sdev, &myself->same_target_siblings,
> -		       	same_target_siblings) {
> -		if (atomic_read(&sdev->device_active))
> -			return 1;
> -	}
> -
> -	return 0;
> -}

We should still be calling the above from somewhere.

> -struct scsi_cmnd *scsi_allocate_device(struct scsi_device *sdev, int wait)
> -{
> -	DECLARE_WAITQUEUE(wq, current);
> -	struct Scsi_Host *shost = sdev->host;
> -	struct scsi_cmnd *scmnd;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&device_request_lock, flags);
> -	while (1) {
> -		if (sdev->device_blocked)
> -			goto busy;
> -		if (sdev->single_lun && check_all_luns(shost, sdev))
> -			goto busy;
> -
> -		/*
> -		 * Now we can check for a free command block for this device.
> -		 */
> -		for (scmnd = sdev->device_queue; scmnd; scmnd = scmnd->next)
> -			if (!scmnd->request)
> -				goto found;
> -
> -busy:
> -		if (!wait)
> -			goto fail;
> -
> -		/*
> -		 * We need to wait for a free commandblock.  We need to
> -		 * insert ourselves into the list before we release the
> -		 * lock.  This way if a block were released the same
> -		 * microsecond that we released the lock, the call
> -		 * to schedule() wouldn't block (well, it might switch,
> -		 * but the current task will still be schedulable.
> -		 */
> -		add_wait_queue(&sdev->scpnt_wait, &wq);
> -		set_current_state(TASK_UNINTERRUPTIBLE);

We are missing the above checks and functionallity in scsi_get_command.

The limit previously imposed by current_queue_depth via the number of
scmnd's allocated is not checked - scsi_get_command should check
device_busy < new_queue_depth.

I was trying to fix/hit this - surprisingly, I did not see performance
problems (i.e. getting tons of QUEUE_FULLs), probably because my request
queue limits are 128, and the disks are not old.

Also the above did not properly get the host_lock/queue_lock when wait != 0
(the caller does not lock, so just when we might sleep we also need to
get/release a lock).

> +struct scsi_host_cmd_pool {
> +	kmem_cache_t	*slab;
> +	unsigned int	users;
> +	char		*name;
> +	unsigned int	slab_flags;
> +	unsigned int	gfp_mask;
> +};

> +static struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost,
> +					    int gfp_mask)
> +{
> +	struct scsi_cmnd *cmd;
> +	
> +	cmd = kmem_cache_alloc(shost->cmd_pool->slab,
> +			gfp_mask | shost->cmd_pool->gfp_mask);
> +
> +	if (unlikely(!cmd)) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&shost->free_list_lock, flags);
> +		if (likely(!list_empty(&shost->free_list))) {
> +			cmd = list_entry(shost->free_list.next,
> +					 struct scsi_cmnd, list);
> +			list_del_init(&cmd->list);
> +		}
> +		spin_unlock_irqrestore(&shost->free_list_lock, flags);
> +	}
> +
> +	return cmd;
> +}
> +
> +/*
> + * Function:	scsi_get_command()
> + *
> + * Purpose:	Allocate and setup a scsi command block
> + *
> + * Arguments:	dev	- parent scsi device
> + *		gfp_mask- allocator flags
> + *
> + * Returns:	The allocated scsi command structure.
> + */
> +struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, int gfp_mask)
> +{
> +	struct scsi_cmnd *cmd = __scsi_get_command(dev->host, gfp_mask);
> +
> +	if (likely(cmd)) {
> +		memset(cmd, 0, sizeof(*cmd));
> +		cmd->device = dev;
> +		cmd->state = SCSI_STATE_UNUSED;
> +		cmd->owner = SCSI_OWNER_NOBODY;
> +		init_timer(&cmd->eh_timeout);
> +		INIT_LIST_HEAD(&cmd->list);
> +	}
> +
> +	return cmd;
> +}				

So one of the above needs to (conditionally ... based on gfp_mask?) get
host/queue_lock, check limits, and conditionally add_wait_queue().

> +int scsi_setup_command_freelist(struct Scsi_Host *shost)
> +{
> +	struct scsi_host_cmd_pool *pool;
> +	struct scsi_cmnd *cmd;
> +
> +	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.
> +	 */
> +	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, NULL);
> +		if (!pool->slab)
> +			return -ENOMEM;
> +	}
> +
> +	pool->users++;
> +	shost->cmd_pool = pool;
> +
> +	/*
> +	 * 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 fail;
> +	list_add(&cmd->list, &shost->free_list);		
> +	return 0;
> +
> +fail:
> +	if (!--pool->users)
> +		kmem_cache_destroy(pool->slab);
> +	return -ENOMEM;
> +
> +}

What protects pool->users?

-- Patrick Mansfield