Re: [PATCH 4/4] block: simplify tag allocation policy selection

[John Garry <john.g.garry@xxxxxxxxxx>]

>   };
>   #undef HCTX_FLAG_NAME
> @@ -192,22 +186,11 @@ static const char *const hctx_flag_name[] = {
>   static int hctx_flags_show(void *data, struct seq_file *m)
>   {
>   	struct blk_mq_hw_ctx *hctx = data;
> -	const int alloc_policy = BLK_MQ_FLAG_TO_ALLOC_POLICY(hctx->flags);
>   
> -	BUILD_BUG_ON(ARRAY_SIZE(hctx_flag_name) !=
> -			BLK_MQ_F_ALLOC_POLICY_START_BIT);
> -	BUILD_BUG_ON(ARRAY_SIZE(alloc_policy_name) != BLK_TAG_ALLOC_MAX);
> +	BUILD_BUG_ON(ARRAY_SIZE(hctx_flag_name) != ilog2(BLK_MQ_F_MAX));
>   
> -	seq_puts(m, "alloc_policy=");

Maybe it's nice to preserve this formatting, but I know that debugfs is 
not a stable ABI...

> -	if (alloc_policy < ARRAY_SIZE(alloc_policy_name) &&
> -	    alloc_policy_name[alloc_policy])
> -		seq_puts(m, alloc_policy_name[alloc_policy]);
> -	else
> -		seq_printf(m, "%d", alloc_policy);
> -	seq_puts(m, " ");
> -	blk_flags_show(m,
> -		       hctx->flags ^ BLK_ALLOC_POLICY_TO_MQ_FLAG(alloc_policy),
> -		       hctx_flag_name, ARRAY_SIZE(hctx_flag_name));
> +	blk_flags_show(m, hctx->flags, hctx_flag_name,
> +			ARRAY_SIZE(hctx_flag_name));
>   	seq_puts(m, "\n");
>   	return 0;



>   /*
> diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
> index 72c03cbdaff4..d51eeba4da3c 100644
> --- a/drivers/ata/sata_sil24.c
> +++ b/drivers/ata/sata_sil24.c
> @@ -378,7 +378,7 @@ static const struct scsi_host_template sil24_sht = {
>   	.can_queue		= SIL24_MAX_CMDS,
>   	.sg_tablesize		= SIL24_MAX_SGE,
>   	.dma_boundary		= ATA_DMA_BOUNDARY,
> -	.tag_alloc_policy	= BLK_TAG_ALLOC_FIFO,
> +	.tag_alloc_policy	= SCSI_TAG_ALLOC_FIFO,

do we actually need to set tag_alloc_policy to the default 
(SCSI_TAG_ALLOC_FIFO)?

>   	.sdev_groups		= ata_ncq_sdev_groups,

> @@ -39,6 +39,11 @@ enum scsi_timeout_action {
>   	SCSI_EH_NOT_HANDLED,
>   };
>   
> +enum scsi_tag_alloc_policy {
> +	SCSI_TAG_ALLOC_FIFO,	/* allocate starting from 0 */
> +	SCSI_TAG_ALLOC_RR,	/* allocate starting from last allocated tag */
> +};


Could we just have

struct scsi_host_template {
	...
	int tag_alloc_policy_rr:1

instead of this enum?

Or does that cause issues for the ATA SHT and descendants where it 
overrides members? I didn't think that it would.

> +
>   struct scsi_host_template {
>   	/*
>   	 * Put fields referenced in IO submission path together in
> @@ -439,7 +444,7 @@ struct scsi_host_template {
>   	short cmd_per_lun;
>   
>   	/* If use block layer to manage tags, this is tag allocation policy */
> -	int tag_alloc_policy;
> +	enum scsi_tag_alloc_policy tag_alloc_policy;
>   
>   	/*
>   	 * Track QUEUE_FULL events and reduce queue depth on demand.