On 17/02/2021 10:13, Damien Le Moal wrote:
> 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 ?
It does here:
void *kvmalloc_node(size_t size, gfp_t flags, int node)
{
gfp_t kmalloc_flags = flags;
void *ret;
/*
* vmalloc uses GFP_KERNEL for some internal allocations (e.g page tables)
* so the given set of flags has to be compatible.
*/
if ((flags & GFP_KERNEL) != GFP_KERNEL)
return kmalloc_node(size, flags, node);
/* [...] */
return __vmalloc_node(size, 1, flags, node,
__builtin_return_address(0));
}
EXPORT_SYMBOL(kvmalloc_node);
We're not reaching __vmalloc_node() if GFP_KERNEL isn't set. Also, from the, function's
documentation:
* @flags: gfp mask for the allocation - must be compatible (superset) with GFP_KERNEL.
So yeah, we should've read that before calling kvcalloc(..., GFP_NOIO).
