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. > 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(). >> + s->insert_bio_sectors = min3(size_limit, sectors, bio_sectors(bio)); > Also I don't really understand the units involved here. > s->insert_bio_sectors, sectors, and bio_sectors is in unit of 512 byte > sectors. Yes, they are in sectors, this is the maximum permitted bio size for 1MB bio size. Now we can have bio > 1MB, and you modify bio_alloc_bioset() parameter 'nr_iovecs from unsigned int to unsigned short, so bio-size/page-size can be > 256 and overflow, e.g. 258 overflows to 2, then the BUG in biovec_slab() is triggered. I feel this is a long time existing issue in bcache code, and should be fixed from bcache side, and your change helps to trigger the problem explicitly. >> - miss = bio_next_split(bio, sectors, GFP_NOIO, &s->d->bio_split); >> + miss = bio_next_split(bio, s->insert_bio_sectors, GFP_NOIO, &s->d->bio_split); > Overly long line. 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. Thanks. Coly Li
