Re: [PATCH v3 1/2] scsi: sd: enable sector size > PAGE_SIZE in scsi sd driver

[Damien Le Moal]

On 2/14/26 10:18, sw.prabhu6@xxxxxxxxx wrote:
> From: Swarna Prabhu <sw.prabhu6@xxxxxxxxx>
> 
> The WRITE SAME(16) and WRITE SAME(10) scsi commands uses
> a page from a dedicated mempool('sd_page_pool') for its
> payload. This pool was initialized to allocate single
> pages, which was sufficient as long as the device sector
> size did not exceed the PAGE_SIZE.
> 
> Given that block layer now supports block size upto
> 64K ie beyond PAGE_SIZE, initialize large page pool in
> 'sd_probe()' if a higher sector device is attached ensuring
> atomicity. Adapt 'sd_set_special_bvec()' to use large page
> pool when a higher sector size device is attached. Hence
> enable sector sizes > PAGE_SIZE in scsi sd driver.
> 
> Signed-off-by: Swarna Prabhu <s.prabhu@xxxxxxxxxxx>
> Co-developed-by: Pankaj Raghav <p.raghav@xxxxxxxxxxx>
> Signed-off-by: Pankaj Raghav <p.raghav@xxxxxxxxxxx>

One nit below. With that fixed, feel free to add:

Reviewed-by: Damien Le Moal <dlemoal@xxxxxxxxxx>

> ---
>  drivers/scsi/sd.c | 80 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 68 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index d76996d6cbc9..ae6ccfed79b9 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -107,8 +107,11 @@ static void sd_config_write_same(struct scsi_disk *sdkp,
>  static void  sd_revalidate_disk(struct gendisk *);
>  
>  static DEFINE_IDA(sd_index_ida);
> +static DEFINE_MUTEX(sd_mutex_lock);
>  
>  static mempool_t *sd_page_pool;
> +static mempool_t *sd_large_page_pool;
> +static atomic_t sd_large_page_pool_users = ATOMIC_INIT(0);
>  static struct lock_class_key sd_bio_compl_lkclass;
>  
>  static const char *sd_cache_types[] = {
> @@ -116,6 +119,33 @@ static const char *sd_cache_types[] = {
>  	"write back, no read (daft)"
>  };
>  
> +static int sd_large_pool_create_lock(void)

I do not see the point of the "_lock" suffix since you do not have a "no lock"
variant of this function. So let's drop that suffix.

> +static void sd_large_pool_destroy_lock(void)

Same here.

-- 
Damien Le Moal
Western Digital Research