Re: [PATCH] block: Add bio_limit

On 2021/01/19 4:26, Matthew Wilcox wrote:

On Mon, Jan 18, 2021 at 07:31:13PM +0100, Christoph Hellwig wrote:

quoted

On Mon, Jan 18, 2021 at 06:17:12PM +0000, Matthew Wilcox wrote:

quoted

On Mon, Jan 18, 2021 at 07:13:38PM +0100, Christoph Hellwig wrote:

quoted

On Thu, Jan 14, 2021 at 07:47:06PM +0000, Matthew Wilcox (Oracle) wrote:

quoted

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