Re: [PATCH v4 07/28] block: Introduce zone write plugging

[Damien Le Moal <dlemoal@kernel.org>]

On 4/3/24 01:12, Christoph Hellwig wrote:
>> +static inline struct blk_zone_wplug *
>> +disk_lookup_zone_wplug(struct gendisk *disk, sector_t sector)
>> +{
>> +	unsigned int zno = disk_zone_no(disk, sector);
>> +	unsigned int idx = hash_32(zno, disk->zone_wplugs_hash_bits);
>> +	struct blk_zone_wplug *zwplug;
>> +
>> +	rcu_read_lock();
>> +	hlist_for_each_entry_rcu(zwplug, &disk->zone_wplugs_hash[idx], node) {
>> +		if (zwplug->zone_no == zno)
>> +			goto unlock;
>> +	}
>> +	zwplug = NULL;
>> +
>> +unlock:
>> +	rcu_read_unlock();
>> +	return zwplug;
>> +}
> 
> Did we lose an atomic_inc_unless_zero here?  This now just does a lookup
> under RCU, but nothing to prevent the zwplug from beeing freed?

Nope. When disk_lookup_zone_wplug() is called directly, it is always for
handling requests/bios which are holding a reference on the plug and because
there are requests/BIOs in-flight, the plug is marked as busy
(BLK_ZONE_WPLUG_PLUGGED or BLK_ZONE_WPLUG_ERROR are set). In such state, the
plug is always hashed given that disk_should_remove_zone_wplug() retturns false
for busy plugs. So there is no reference increase here. The
atomic_inc_not_zero() is in disk_get_zone_wplug() which calls
disk_lookup_zone_wplug() + atomic_inc_not_zero() within an
rcu_read_lock()/rcu_read_unlock() section.

> 
>> +	/* Resize the zone write plug memory pool if needed. */
>> +	if (disk->zone_wplugs_pool->min_nr != pool_size)
>> +		return mempool_resize(disk->zone_wplugs_pool, pool_size);
> 
> Note that a mempool_resize to the current size work just fine.  It takes
> a pointless lock, but given that this is something that doesn't happen
> frequently that probably doesn't matter.
> 
>> +#include <linux/mempool.h>
> 
>> +	mempool_t		*zone_wplugs_pool;
> 
> Please use struct mempool_s here so that you only need a forward
> declaration instead of pulling in another header.

-- 
Damien Le Moal
Western Digital Research