Re: [PATCH 1/8] block: fix zone write plug removal

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

On 2/24/26 5:21 AM, Bart Van Assche wrote:
> On 2/23/26 11:30 AM, Bart Van Assche wrote:
>> On 2/23/26 3:56 AM, Hannes Reinecke wrote:
>>> On 2/21/26 01:44, Damien Le Moal wrote:
>>>> +static inline void disk_check_and_put_zone_wplug(struct blk_zone_wplug
>>>> *zwplug)
>>>> +{
>>>> +    lockdep_assert_not_held(&zwplug->lock);
>>>> +
>>>> +    /*
>>>> +     * Check for the need to remove the zone write plug from the hash list
>>>> +     * when we see that only the caller is referencing the zone write plug.
>>>> +     * Since this is racy without holding the zone write plug lock, this
>>>> +     * check is done again in  disk_should_remove_zone_wplug().
>>>> +     */
>>>> +    if (refcount_read(&zwplug->ref) <= 2) {
>>>> +        struct gendisk *disk = zwplug->disk;
>>>> +        unsigned long flags;
>>>> +
>>>> +        spin_lock_irqsave(&zwplug->lock, flags);
>>>> +        if (disk_should_remove_zone_wplug(disk, zwplug))
>>>> +            disk_remove_zone_wplug(disk, zwplug);
>>>> +        spin_unlock_irqrestore(&zwplug->lock, flags);
>>>> +    }
>>>> +
>>>> +    disk_put_zone_wplug(zwplug);
>>>> +}
>>>> +
>>>>   static void blk_zone_wplug_bio_work(struct work_struct *work);
>>>>   /*
>>>
>>> This looks slightly odd; a simple 'refcount_read()' always trigger alarm
>>> bells with me as it inevitably races with 'refcount_dec()' /
>>> 'refcount_inc()'.
>>> And similar the check 'refcount <= 2' also looks odd; typically one
>>> would check against '< 1' and let the refcount destructor handle
>>> things.
>> Hmm ... I think that the above code is racy. Here is an example:
>>
>> thread 1                                thread 2
>> -------------------------------         -------------------------------
>> refcount_read() returns 3               refcount_read() returns 3
>>
>> disk_should_remove_zone_wplug() is      disk_should_remove_zone_wplug()
>> is not called                           is not called
>>
>> disk_put_zone_wplug() decreases         disk_put_zone_wplug() decreases
>> zwplug->ref from 3 to 2                 zwplug->ref from 2 to 1
>>
>>
>> Result: zwplug->ref became less than 2 without
>> disk_should_remove_zone_wplug() having been called.
> 
> A correction to the above: three threads are required before it becomes
> possible for the zwplug reference count to drop from 3 to zero without
> disk_should_remove_zone_wplug() having been called.

I do not see how your example is possible... If refcount read are form the BIO
work context, we only have one per zone, so there is not such concurrency.
The additional contexts can be from zone reset or zone finish, but these
unconditionally call disk_should_remove_zone_wplug(), so there is no such race.

> 
> Thanks,
> 
> Bart.


-- 
Damien Le Moal
Western Digital Research