Re: [bug report] scsi: sd_zbc: emulate ZONE_APPEND commands

[Johannes Thumshirn <Johannes.Thumshirn@wdc.com>]

On 17/02/2021 00:33, Damien Le Moal wrote:
> On 2021/02/17 4:42, Dan Carpenter wrote:
>> Hello Johannes Thumshirn,
>>
>> The patch 5795eb443060: "scsi: sd_zbc: emulate ZONE_APPEND commands"
>> from May 12, 2020, leads to the following static checker warning:
>>
>> 	drivers/scsi/sd_zbc.c:741 sd_zbc_revalidate_zones()
>> 	error: kvmalloc() only makes sense with GFP_KERNEL
>>
>> drivers/scsi/sd_zbc.c
>>    721          /*
>>    722           * There is nothing to do for regular disks, including host-aware disks
>>    723           * that have partitions.
>>    724           */
>>    725          if (!blk_queue_is_zoned(q))
>>    726                  return 0;
>>    727  
>>    728          /*
>>    729           * Make sure revalidate zones are serialized to ensure exclusive
>>    730           * updates of the scsi disk data.
>>    731           */
>>    732          mutex_lock(&sdkp->rev_mutex);
>>    733  
>>    734          if (sdkp->zone_blocks == zone_blocks &&
>>    735              sdkp->nr_zones == nr_zones &&
>>    736              disk->queue->nr_zones == nr_zones)
>>    737                  goto unlock;
>>    738  
>>    739          sdkp->zone_blocks = zone_blocks;
>>    740          sdkp->nr_zones = nr_zones;
>>    741          sdkp->rev_wp_offset = kvcalloc(nr_zones, sizeof(u32), GFP_NOIO);
>>                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> We're passing GFP_NOIO here so it just defaults to kcalloc() and will
>> not vmalloc() the memory.
> 
> Indeed... And the allocation can get a little too big for kmalloc().
> 
> Johannes, I think we need to move that allocation before the rev_mutex locking,
> using a local var for the allocated address, and then using GFP_KERNEL should be
> safe... But not entirely sure. Using kmalloc would be simpler but on large SMR
> drives, that allocation will soon need to be 400K or so (i.e. 100,000 zones or
> even more), too large for kmalloc to succeed reliably.
> 


No I don't think so. A mutex isn't a spinlock so we can sleep on the allocation.
We can't use GFP_KERNEL as we're about to do I/O. blk_revalidate_disk_zones() called
a few line below also does the memalloc_noio_{save,restore}() dance.

Would a kmem_cache for these revalidations help us in any way?