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

[Bart Van Assche <bvanassche@acm.org>]

On 3/25/24 20:12, Damien Le Moal wrote:
> On 3/26/24 06:53, Bart Van Assche wrote:
>> On 3/24/24 21:44, Damien Le Moal wrote:
>>> -void disk_free_zone_bitmaps(struct gendisk *disk)
>>> +static bool disk_insert_zone_wplug(struct gendisk *disk,
>>> +				   struct blk_zone_wplug *zwplug)
>>> +{
>>> +	struct blk_zone_wplug *zwplg;
>>> +	unsigned long flags;
>>> +	unsigned int idx =
>>> +		hash_32(zwplug->zone_no, disk->zone_wplugs_hash_bits);
>>> +
>>> +	/*
>>> +	 * Add the new zone write plug to the hash table, but carefully as we
>>> +	 * are racing with other submission context, so we may already have a
>>> +	 * zone write plug for the same zone.
>>> +	 */
>>> +	spin_lock_irqsave(&disk->zone_wplugs_lock, flags);
>>> +	hlist_for_each_entry_rcu(zwplg, &disk->zone_wplugs_hash[idx], node) {
>>> +		if (zwplg->zone_no == zwplug->zone_no) {
>>> +			spin_unlock_irqrestore(&disk->zone_wplugs_lock, flags);
>>> +			return false;
>>> +		}
>>> +	}
>>> +	hlist_add_head_rcu(&zwplug->node, &disk->zone_wplugs_hash[idx]);
>>> +	spin_unlock_irqrestore(&disk->zone_wplugs_lock, flags);
>>> +
>>> +	return true;
>>> +}
>>
>> Since this function inserts an element into disk->zone_wplugs_hash[],
>> can it happen that another thread removes that element from the hash
>> list before this function returns?
> 
> No, that cannot happen. Both insertion and deletion of plugs in the hash table
> are serialized with disk->zone_wplugs_lock. See disk_remove_zone_wplug().

I think that documenting locking assumptions with lockdep_assert_held()
would make this patch easier to review.

Thanks,

Bart.