Re: [PATCH] block: Add bio_limit

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2021/01/19 4:26, Matthew Wilcox wrote:
> On Mon, Jan 18, 2021 at 07:31:13PM +0100, Christoph Hellwig wrote:
>> On Mon, Jan 18, 2021 at 06:17:12PM +0000, Matthew Wilcox wrote:
>>> On Mon, Jan 18, 2021 at 07:13:38PM +0100, Christoph Hellwig wrote:
>>>> On Thu, Jan 14, 2021 at 07:47:06PM +0000, Matthew Wilcox (Oracle) wrote:
>>>>> It's often inconvenient to use BIO_MAX_PAGES due to min() requiring the
>>>>> sign to be the same.  Introduce bio_limit() and change BIO_MAX_PAGES to
>>>>> be unsigned to make it easier for the users.
>>>>>
>>>>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>>>>
>>>> I like the helper, but I'm not too happy with the naming.  Why not
>>>> something like bio_guess_nr_segs() or similar?
>>>
>>> This feels like it's a comment on an entirely different patch, like this one:
>>>
>>> https://git.infradead.org/users/willy/pagecache.git/commitdiff/fe9841debe24e15100359acadd0b561bbb2dceb1
>>>
>>> bio_limit() doesn't guess anything, it just clamps the argument to
>>> BIO_MAX_PAGES (which is itself misnamed; it's BIO_MAX_SEGS now)
>>
>> No, it was for thi patch.  Yes, it divides and clamps.  Which is sort of
>> a guess as often we might need less of them.  That being said I'm not
>> very fond of my suggestion either, but limit sounds wrong as well.
> 
> bio_limit() doesn't divide.  Some of the callers divide.
> 
> +static inline unsigned int bio_limit(unsigned int nr_segs)
> +{
> +       return min(nr_segs, BIO_MAX_PAGES);
> +}
> 
> I'd rather the callers didn't have to worry about this at all (just pass
> in a number and then deal with however many bvecs you were given), but
> there are callers which depend on the current if-too-big-return-NULL
> behaviour, and I don't want to track all of those down and fix them.
> 
> I chose limit because it's imposing the bio's limit.  Could be called
> bio_clamp(), but the bio also doesn't impose a minimum, so that seemed
> wrong.
> 

What about calling it bio_max_bvecs() or bio_max_segs() ? Together with renaming
BIO_MAX_PAGES to BIO_MAX_SEGS or BIO_MAX_BVECS, things would be clear on what
this is referring to. Since these days one bvec is one seg, but segment is more
struct request layer while bvec is more BIO layer, I would lean toward using
bvec for naming this one, but either way would be fine I think.

-- 
Damien Le Moal
Western Digital Research