[bug report] scsi: sd_zbc: emulate ZONE_APPEND commands

[bug report] scsi: sd_zbc: emulate ZONE_APPEND commands

[Dan Carpenter]

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.

   742          if (!sdkp->rev_wp_offset) {
   743                  ret = -ENOMEM;
   744                  goto unlock;
   745          }
   746  
   747          ret = blk_revalidate_disk_zones(disk, sd_zbc_revalidate_zones_cb);
   748  

regards,
dan carpenter

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

[Damien Le Moal]

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.

> 
>    742          if (!sdkp->rev_wp_offset) {
>    743                  ret = -ENOMEM;
>    744                  goto unlock;
>    745          }
>    746  
>    747          ret = blk_revalidate_disk_zones(disk, sd_zbc_revalidate_zones_cb);
>    748  
> 
> regards,
> dan carpenter
> 


-- 
Damien Le Moal
Western Digital Research

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

[Johannes Thumshirn]

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?

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

[Damien Le Moal]

On 2021/02/17 15:42, 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.

Yes, but blk_revalidate_disk_zones() only allocates the zone bitmaps and these
are much smaller. So kmalloc is fine and GFP_NOIO is natural. For the wp array,
I think we really need kvmalloc() due to the potential very large size (and
growing with new drive models) and GFP_NOIO does not work for that. Not sure if
memalloc_noio_{save,restore}() can change that in vmalloc context (I do not
think so).

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

I am not familiar with that... Would need to dig into it.

For this to be safe, we only need to guarantee forward progress, and in this
case this means not causing problems if a GFP_KERNEL allocation causes us to
reenter the scsi driver for I/Os. Since (I think) revalidation never happens in
FS or I/O context, GFP_KERNEL allocation should be safe if done outside of the
rev_mutex lock. Not 100% sure here, just a hunch... We may need to check the
block layer level to check if there are any locks being held when revalidation
triggers.



-- 
Damien Le Moal
Western Digital Research

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

[Michal Hocko]

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.
> 
> Would a kmem_cache for these revalidations help us in any way?

I am not sure what you mean here.
-- 
Michal Hocko
SUSE Labs

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

[Johannes Thumshirn]

On 17/02/2021 09:03, Michal Hocko wrote:
>> 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.

That means all callers of blk_revalidate_disk_zones() should do 
memalloc_noio_{save,restore}? If yes, can we somehow runtime assert
that this is done, so we don't end up with bad surprises?

>>
>> Would a kmem_cache for these revalidations help us in any way?
> 
> I am not sure what you mean here.
> 

Using a kmem_cache for the allocations passed into blk_revalidate_disk_zones().
I've looked into kmem_cache_alloc() and I couldn't find anything that speaks 
against it, but I'm not too familiar with the code.

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

[Damien Le Moal]

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

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

[Johannes Thumshirn]

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).

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

[Michal Hocko]

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

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

[Michal Hocko]

On Wed 17-02-21 09:08:07, Johannes Thumshirn wrote:
> On 17/02/2021 09:03, Michal Hocko wrote:
> >> 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.
> 
> That means all callers of blk_revalidate_disk_zones() should do 
> memalloc_noio_{save,restore}?

I am not really familiar with the IO area to answer this. The base idea
is to start the NOIO scope at the boundary which defines "unsafe to
re-enter or cannot deal with a new IO" from the reclaim path.

> If yes, can we somehow runtime assert that this is done, so we don't
> end up with bad surprises?

Could you elaborate?
 
> >> Would a kmem_cache for these revalidations help us in any way?
> > 
> > I am not sure what you mean here.
> > 
> 
> Using a kmem_cache for the allocations passed into blk_revalidate_disk_zones().
> I've looked into kmem_cache_alloc() and I couldn't find anything that speaks 
> against it, but I'm not too familiar with the code.

kmem_cache_alloc is only an extension to allow to allocate from a
specific cache. I do not really see how it is going to help with larger
allocation and my current understanding is that kvmalloc is used because
the requested allocation size can be large.

-- 
Michal Hocko
SUSE Labs

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

[Johannes Thumshirn]

On 17/02/2021 10:36, Michal Hocko wrote:
> On Wed 17-02-21 09:08:07, Johannes Thumshirn wrote:
>> On 17/02/2021 09:03, Michal Hocko wrote:
>>>> 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.
>>
>> That means all callers of blk_revalidate_disk_zones() should do 
>> memalloc_noio_{save,restore}?
> 
> I am not really familiar with the IO area to answer this. The base idea
> is to start the NOIO scope at the boundary which defines "unsafe to
> re-enter or cannot deal with a new IO" from the reclaim path.
> 
>> If yes, can we somehow runtime assert that this is done, so we don't
>> end up with bad surprises?
> 
> Could you elaborate?

I though of lifting the noio scope into the callers of 
blk_revalidate_disk_zones() and then "check" in blk_revalidate_disk_zones()
this has been done. But it looks like memalloc_noio_save() can handle nesting,
so this is actually unneeded.


>  
>>>> Would a kmem_cache for these revalidations help us in any way?
>>>
>>> I am not sure what you mean here.
>>>
>>
>> Using a kmem_cache for the allocations passed into blk_revalidate_disk_zones().
>> I've looked into kmem_cache_alloc() and I couldn't find anything that speaks 
>> against it, but I'm not too familiar with the code.
> 
> kmem_cache_alloc is only an extension to allow to allocate from a
> specific cache. I do not really see how it is going to help with larger
> allocation and my current understanding is that kvmalloc is used because
> the requested allocation size can be large.
> 

Ah ok so we can't set aside a big enough pool to do allocations from there,
this was a misunderstanding from my side.

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

[Michal Hocko]

On Wed 17-02-21 09:41:58, Johannes Thumshirn wrote:
> On 17/02/2021 10:36, Michal Hocko wrote:
> > On Wed 17-02-21 09:08:07, Johannes Thumshirn wrote:
> >> On 17/02/2021 09:03, Michal Hocko wrote:
> >>>> 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.
> >>
> >> That means all callers of blk_revalidate_disk_zones() should do 
> >> memalloc_noio_{save,restore}?
> > 
> > I am not really familiar with the IO area to answer this. The base idea
> > is to start the NOIO scope at the boundary which defines "unsafe to
> > re-enter or cannot deal with a new IO" from the reclaim path.
> > 
> >> If yes, can we somehow runtime assert that this is done, so we don't
> >> end up with bad surprises?
> > 
> > Could you elaborate?
> 
> I though of lifting the noio scope into the callers of 
> blk_revalidate_disk_zones() and then "check" in blk_revalidate_disk_zones()
> this has been done. But it looks like memalloc_noio_save() can handle nesting,
> so this is actually unneeded.

Yes, it can handle nesting. As to where to start the scope, this should
be at the boundary of "do not reenter IO path from the reclaim". This
can be a lock which is held for the allocation which will be needed for
an IO induced by the reclaim or any other resouce that will block from
the IO path. Is this more clear now?
-- 
Michal Hocko
SUSE Labs

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

[Dan Carpenter]

On Wed, Feb 17, 2021 at 09:18:37AM +0000, Johannes Thumshirn wrote:
>  * @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).

This is a Smatch check now and checks are better than comments.  :)

regards,
dan carpenter

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

[Johannes Thumshirn]

On 17/02/2021 12:17, Dan Carpenter wrote:
> On Wed, Feb 17, 2021 at 09:18:37AM +0000, Johannes Thumshirn wrote:
>>  * @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).
> 
> This is a Smatch check now and checks are better than comments.  :)

Definitively