Re: [PATCH] 3/7 consolidate single_lun code

[Luben Tuikov <luben@splentec.com>]

Patrick Mansfield wrote:
> Consolidate scsi single_lun code.
> 
> diff -purN -X /home/patman/dontdiff put_cmd-25/drivers/scsi/scsi_lib.c lun-single-25/drivers/scsi/scsi_lib.c
> --- put_cmd-25/drivers/scsi/scsi_lib.c	Mon Mar 24 12:14:51 2003
> +++ lun-single-25/drivers/scsi/scsi_lib.c	Mon Mar 24 12:14:55 2003
> @@ -323,6 +323,49 @@ void scsi_setup_cmd_retry(struct scsi_cm
>  }
>  
>  /*
> + * Called for single_lun devices. The target associated with current_sdev can
> + * only handle one active command at a time. Scan through all of the luns
> + * on the same target as current_sdev, return 1 if any are active.
> + */
> +static int scsi_single_lun_check(struct scsi_device *current_sdev)

This is such a general name, that it does not mean anything.
What kind of ``check'' is this?

Why not like this:

/**
  * scsi_target_ok2queue: return non-zero if it is ok to queue
  * to this target, else 0.
  */

<< but a nicely formatted comment>>

static int scsi_target_ok2queue(struct scsi_target *target)
{
	if (!target->single_lu)
		return 1;
	for all devices of the target
		if an LU is active
			return 0;
	return 1;
}

That is, 0 is returned when there is NO error code.
Since this function does NOT return error codes, but performes
a LOGICAL nature test, you'd return 1 and 0, so that this makes sense:

	if (scsi_target_ok2queue(target)) {
		/* we're ok to queue, go! */
	} else {
		/* no we're not ok to queue */
	}

and the negation:
	if (!scsi_target_ok2queue(target)) {
	...

I'd rather have it named int scsi_target_canqueue(), but there
might be some confusion with the can_queue struct member.

> +{
> +	struct scsi_device *sdev;
> +
> +	list_for_each_entry(sdev, &current_sdev->same_target_siblings,
> +			    same_target_siblings)
> +		if (sdev->device_busy)
> +			return 1;
> +	return 0;
> +}
> +
> +/*
> + * Called for single_lun devices on IO completion. If no requests
> + * outstanding for current_sdev, call __blk_run_queue for the next
> + * scsi_device on the same target that has requests.
> + *
> + * Called with queue_lock held.
> + */
> +static void scsi_single_lun_run(struct scsi_device *current_sdev,
> +				 struct request_queue *q)

This will be incorporated by default by the starved_devs list
priority queue.

I.e. you should try not to have a partucular function for this but
arrange the infrastructure in such a way that this is handled
by default (via the infrastructure itself).

> +{
> +	struct scsi_device *sdev;
> +	struct Scsi_Host *shost;
> +
> +	shost = current_sdev->host;
> +	if (blk_queue_empty(q) && current_sdev->device_busy == 0 &&
> +	    !shost->host_blocked && !shost->host_self_blocked &&
> +	    !((shost->can_queue > 0) &&
> +	      (shost->host_busy >= shost->can_queue)))
> +		list_for_each_entry(sdev, &current_sdev->same_target_siblings,
> +				    same_target_siblings)
> +			if (!sdev->device_blocked &&
> +			    !blk_queue_empty(sdev->request_queue)) {
> +				__blk_run_queue(sdev->request_queue);
> +				break;
> +			}
> +}
> +
> +/*
>   * Function:    scsi_queue_next_request()
>   *
>   * Purpose:     Handle post-processing of completed commands.
> @@ -390,29 +433,11 @@ void scsi_queue_next_request(request_que
>  	}
>  
>  	sdev = q->queuedata;
> -	shost = sdev->host;
>  
> -	/*
> -	 * If this is a single-lun device, and we are currently finished
> -	 * with this device, then see if we need to get another device
> -	 * started.  FIXME(eric) - if this function gets too cluttered
> -	 * with special case code, then spin off separate versions and
> -	 * use function pointers to pick the right one.
> -	 */
> -	if (sdev->single_lun && blk_queue_empty(q) && sdev->device_busy ==0 &&
> -			!shost->host_blocked && !shost->host_self_blocked &&
> -			!((shost->can_queue > 0) && (shost->host_busy >=
> -				       		     shost->can_queue))) {
> -		list_for_each_entry(sdev2, &sdev->same_target_siblings,
> -			       same_target_siblings) {
> -			if (!sdev2->device_blocked &&
> -			    !blk_queue_empty(sdev2->request_queue)) {
> -				__blk_run_queue(sdev2->request_queue);
> -				break;
> -			}
> -		}
> -	}
> +	if (sdev->single_lun)
> +		scsi_single_lun_run(sdev, q);
>  
> +	shost = sdev->host;
>  	while (!list_empty(&shost->starved_list) &&
>  	       !shost->host_blocked && !shost->host_self_blocked &&
>  		!((shost->can_queue > 0) &&
> @@ -917,22 +942,6 @@ static int scsi_init_io(struct scsi_cmnd
>  	return BLKPREP_KILL;
>  }
>  
> -/*
> - * The target associated with myself can only handle one active command at
> - * a time. Scan through all of the luns on the same target as myself,
> - * return 1 if any are active.
> - */
> -static int check_all_luns(struct scsi_device *myself)
> -{
> -	struct scsi_device *sdev;
> -
> -	list_for_each_entry(sdev, &myself->same_target_siblings,
> -			    same_target_siblings)
> -		if (sdev->device_busy)
> -			return 1;
> -	return 0;
> -}
> -
>  static int scsi_prep_fn(struct request_queue *q, struct request *req)
>  {
>  	struct Scsi_Device_Template *sdt;
> @@ -1077,9 +1086,6 @@ static void scsi_request_fn(request_queu
>  		if (sdev->device_busy >= sdev->queue_depth)
>  			break;
>  
> -		if (sdev->single_lun && check_all_luns(sdev))
> -			break;
> -
>  		if (shost->host_busy == 0 && shost->host_blocked) {
>  			/* unblock after host_blocked iterates to zero */
>  			if (--shost->host_blocked == 0) {
> @@ -1120,6 +1126,9 @@ static void scsi_request_fn(request_queu
>  			break;
>  		}
>  
> +		if (sdev->single_lun && scsi_single_lun_check(sdev))
> +			break;
> +
>  		/*
>  		 * If we couldn't find a request that could be queued, then we
>  		 * can also quit.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Luben Tuikov, Senior Software Engineer, Splentec Ltd.
Bus: +1-905-707-1954x112, 9-5 EDT. Fax: +1-905-707-1974.