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

[Christoph Hellwig <hch@lst.de>]

On Fri, Jan 03, 2025 at 09:33:31AM +0000, John Garry wrote:
>> -	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...

It's not a stable API indeed, and I'd rather not keep extra code for
debugging.

>> 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)?

libata uses weird inheritance where __ATA_BASE_SHT sets default fields
that can then later be override, so this is indeed needed to set the
field back to the original default after the previous assignment changed
it.  (Did I mentioned I hate this style of programming? :))

> Could we just have
>
> struct scsi_host_template {
> 	...
> 	int tag_alloc_policy_rr:1
>
> instead of this enum?

This should probably work.  I tried to reduce the churn a bit, but
due to the change of the enum value names that didn't actually work.

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

It'll still work fine.  It will be even less obvious, so the case
mentioned above by you will probably have to grow a comment explaining
it to preempt the cleanup brigade from removing it.