Re: [PATCH 4/8] block: improve disk_zone_wplug_schedule_bio_work()

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

On 2/23/26 8:59 PM, Hannes Reinecke wrote:
> On 2/21/26 01:44, Damien Le Moal wrote:
>> The function disk_zone_wplug_schedule_bio_work() always takes a
>> reference on the zone write plug of the BIO work being scheduled. This
>> ensure that the zone write plug cannot be freed while the BIO work is
>> being scheduled but has not run yet. However, this unconditional
>> reference taking is fragile since the reference taken is realized by the
>> BIO work blk_zone_wplug_bio_work() function, which implies that there
>> always must be a 1:1 relation between the work being scheduled and the
>> work running.
>>
>> Make this less fragile by taking the reference on the BIO work if and
>> only if the BIO work has not been already scheduled.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>   block/blk-zoned.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/blk-zoned.c b/block/blk-zoned.c
>> index c5c91f927a71..4b388ae1acaa 100644
>> --- a/block/blk-zoned.c
>> +++ b/block/blk-zoned.c
>> @@ -1205,13 +1205,15 @@ static void disk_zone_wplug_schedule_bio_work(struct
>> gendisk *disk,
>>       lockdep_assert_held(&zwplug->lock);
>>         /*
>> -     * Take a reference on the zone write plug and schedule the submission
>> -     * of the next plugged BIO. blk_zone_wplug_bio_work() will release the
>> -     * reference we take here.
>> +     * Schedule the submission of the next plugged BIO. If the zone write
>> +     * plug BIO work is not already scheduled, take a reference on the zone
>> +     * write plug to ensure that it does not go away while the work is being
>> +     * scheduled but has not run yet. blk_zone_wplug_bio_work() will release
>> +     * the reference we take here.
>>        */
>>       WARN_ON_ONCE(!(zwplug->flags & BLK_ZONE_WPLUG_PLUGGED));
>> -    refcount_inc(&zwplug->ref);
>> -    queue_work(disk->zone_wplugs_wq, &zwplug->bio_work);
>> +    if (queue_work(disk->zone_wplugs_wq, &zwplug->bio_work))
>> +        refcount_inc(&zwplug->ref);
>>   }
>>     static inline void disk_zone_wplug_add_bio(struct gendisk *disk,
> 
> Urgh. Don't.
> There is a race condition; 'bio_work' might be scheduled directly and
> complete before 'refcount_inc()' is called.
> You have to invert the statement by keeping the 'refcount_inc()'
> before calling 'queue_work()', and then drop the reference again
> if queue_work failed.
> Ugly, I know.

As Bart pointed out, since this is done with the zone write plug spinlock held
and the work takes that lock first when it runs, there is no chance that the
refcount gets bad. That is indeed not super clear though, so I will imrove the
comment to describe that.

Note: the zone write plug refcounting is a bit a mess and needs cleaning up,
which I was planning to do on top of all this.


-- 
Damien Le Moal
Western Digital Research