Re: [PATCH 09/12] sd: convert to the atomic queue limits API

[Christoph Hellwig <hch@xxxxxx>]

On Thu, May 30, 2024 at 10:16:33AM +0100, John Garry wrote:
>> -static void sd_config_write_same(struct scsi_disk *);
>> +static void sd_config_discard(struct scsi_disk *sdkp, struct queue_limits 
>> *lim,
>> +            unsigned int mode);
>
> Are there any reasons why we keep forward declarations like this? AFAICS, 
> this sd_config_discard forward declaration could be removed.

Mostly to avoid churn.  This is a series that needs to feed into the
block tree, so I don't want major churn in sd.c.  Maybe after the dust
has settled it would be nice to bring sd.c into a natural order.

>> -    blk_queue_max_write_zeroes_sectors(q, sdkp->max_ws_blocks *
>> -                                     (logical_block_size >> 9));
>> +    lim->max_write_zeroes_sectors =
>> +            sdkp->max_ws_blocks * (logical_block_size >> 9);
>
> Would it be ok to use SECTOR_SHIFT here? A similar change is made in 
> sd_config_discard(), above

Sure.

>> +            sd_config_discard(sdkp, lim, sd_discard_mode(sdkp));
>>      }
>>      out:
>> @@ -3278,10 +3290,10 @@ static void sd_read_block_limits_ext(struct 
>> scsi_disk *sdkp)
>>   }
>>     /**
>
> below is not a kernel doc comment

And that is on the one hand intentional to avoid documenting all the
obvious paramters in a local function, but on the other hand requires
removing the double *. Fixed.

>>   @@ -3683,28 +3696,33 @@ static int sd_revalidate_disk(struct gendisk 
>> *disk)
>>      q->limits.max_dev_sectors = logical_to_sectors(sdp, dev_max);
>
>
> is setting q->limits.max_dev_sectors directly proper?

No, and I've already fixed it in my local tree.