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

[Michal Hocko <mhocko@suse.com>]

On Wed 17-02-21 09:13:57, 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 ?

current_gfp_context in the page allocator. vmalloc doesn't do any
reclaim on its own. It relies on the page allocator for that.
-- 
Michal Hocko
SUSE Labs