Re: [PATCH v5 2/2] bcache: avoid oversized read request in cache missing code path

[Coly Li]

On 6/7/21 8:08 PM, Christoph Hellwig wrote:
> On Mon, Jun 07, 2021 at 07:55:22PM +0800, Coly Li wrote:
>> On 6/7/21 7:06 PM, Christoph Hellwig wrote:
>>> On Mon, Jun 07, 2021 at 06:35:39PM +0800, Coly Li wrote:
>>>> +	/* Limitation for valid replace key size and cache_bio bvecs number */
>>>> +	size_limit = min_t(unsigned int, bio_max_segs(UINT_MAX) * PAGE_SECTORS,
>>>> +			   (1 << KEY_SIZE_BITS) - 1);
>>> bio_max_segs kaps the argument to BIO_MAX_VECS, so you might as well
>> It was suggested to not directly access BIO_MAX_VECS by you, maybe I
>> misunderstood you.
> Yes, drivers really should not care about it.  But hiding that behind
> a tiny wrapper doesn't help either.

OK, I change back to BIO_MAX_VECS.
>>> directly write BIO_MAX_VECS.  Can you explain the PAGE_SECTORS here a bit
>>> more? Does this code path use discontiguous per-sector allocations?
>>> Preferably in a comment.
>>
>> It is just because bch_bio_map() assume the maximum bio size is 1MB. It
>> was not true since the multiple pages bvecs
>> was merged in mainline kernel.
>>  
>> The PAGE_SECTORS part is legacy for 1MB maximum size bio (256*4KB), it
>> should be fixed/improved later to
>> use multiple pages for bio size > 1MB and replace bch_bio_map().
> bch_bio_map and bch_bio_alloc_pages that poke directly into the bio are
> the root cause of a lot of these problems.
>
> I had a series fixing some of that but Kent did not like it.  Drivers must
> not diretly access bi_vcnt or directly build bios.

Last time when you posted the series, Ming Lei's multipages bvecs were
not merged yet.

But the bcache code has kind of implicit restriction, e.g.
cached_dev_cache_miss(), the refill
cache missing 'cache_bio' must be a single bio and not chained,
otherwise the following
cached_dev_read_done() will have problem. Therefore I give up to fix
such thing in this
series, maybe there is no unique way to fix, and I also need to improve
the bio related stuffs
case by case.


>> Not any more. Now the line limit is 100 characters. Though I still
>> prefer 80 characters, place 86 characters in single line
>> makes the change more obvious.
> It makes it really hard to read in a normal terminal.

I don't know you still use 80 characters terminal for coding. Sure you
have my respect and I will serve your
request in next post.

Thanks.

Coly Li