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

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

On 4/3/24 03:26, Hannes Reinecke wrote:
>> diff --git a/block/bio.c b/block/bio.c
>> index d24420ed1c4c..4ece8cef1fbe 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -1576,6 +1576,13 @@ void bio_endio(struct bio *bio)
>>       if (!bio_integrity_endio(bio))
>>           return;
>>   +    /*
>> +     * For BIOs handled through a zone write plug, signal the completion
>> +     * of the BIO so that the next plugged BIO can be submitted.
>> +     */
>> +    if (bio_zone_write_plugging(bio))
>> +        blk_zone_write_plug_bio_endio(bio);
>> +
> 
> Can't we move this check into blk_zone_write_plug_bio_endio()?
> We'd need to check it anyway ...

The goal here is to avoid a useless function call for regular devices or read
BIOs. There is no double check as the flag is not checked again in
blk_zone_write_plug_bio_endio(). Same comment for the other points you raised
(except the last one, see below).

If you really insist, I could play games with inline functions to "hide" the
check though.

>> +/*
>> + * Called from bio_attempt_back_merge() when a BIO was merged with a request.
>> + */
>> +void blk_zone_write_plug_bio_merged(struct bio *bio)
>> +{
>> +    struct blk_zone_wplug *zwplug;
>> +    unsigned long flags;
>> +
>> +    /*
>> +     * If the BIO was already plugged, then we were called through
>> +     * blk_zone_write_plug_attempt_merge() -> blk_attempt_bio_merge().
>> +     * For this case, blk_zone_write_plug_attempt_merge() will handle the
>> +     * zone write pointer offset update.
>> +     */
>> +    if (bio_flagged(bio, BIO_ZONE_WRITE_PLUGGING))
>> +        return;
>> +
> See? you have to check anyway ...

This is the only function checking again, and for a good reason as the comment
above the check explains.

> 
> Cheers,
> 
> Hannes

-- 
Damien Le Moal
Western Digital Research