On 2021/02/17 17:03, Michal Hocko wrote:
> On Wed 17-02-21 06:42:37, Johannes Thumshirn wrote:
>> 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.
>
> You should be extending noio scope then if this allocation falls into
> the same category. Ideally the scope should start at the recursion place
> and end where the scope really ened.
But it does not look like __vmalloc_node() (fallback in kvmalloc_node() if
kmalloc() fails) cares about the context allocation flags... I can't see
if/where the context allocation flags are taken into account. It looks like only
the gfp_mask argument is used. Am I missing something ?
>>
>> Would a kmem_cache for these revalidations help us in any way?
>
> I am not sure what you mean here.
>
--
Damien Le Moal
Western Digital Research
