On Wed, Apr 23, 2025 at 11:36:21AM +0200, Christoph Hellwig wrote: > On Tue, Apr 22, 2025 at 10:47:03AM -0400, Kent Overstreet wrote: > > On Tue, Apr 22, 2025 at 04:26:01PM +0200, Christoph Hellwig wrote: > > > Hi all, > > > > > > this series adds more block layer helpers to remove boilerplate code when > > > adding memory to a bio or to even do the entire synchronous I/O. > > > > > > The main aim is to avoid having to convert to a struct page in the caller > > > when adding kernel direct mapping or vmalloc memory. > > > > have you seen (bch,bch2)_bio_map? > > Now I have. > > > > > it's a nicer interface than your bio_add_vmalloc(), and also handles the > > if (is_vmalloc_addr()) > > Can you explain how it's nicer? > > For use with non-vmalloc memory it does a lot of extra work > and generates less optimal bios using more vecs than required, but > maybe that wasn't the point? > > For vmalloc it might also build suboptimal bios when using large vmalloc > mappings due to the lack of merging, but I don't think anyone does I/O to > those yet. > > It lacks a API description and it or the callers miss handling for VIVT > caches, maybe because of that. > > Besides optimizing the direct map case that always needs just one vec > that is also one of the reasons why I want the callers to know about > vmalloc vs non-vmalloc memory.
Sure, that code predates multipage bvecs - the interface is what I was referring to. > It also don't support bio chaining or error handling and requires a > single bio that is guaranteed to fit the required number of vectors. Why would bio chaining ever be required? The caller allocates both the buf and the bio, I've never seen an instance where you'd want that; just allocate a bio with the correct number of vecs, which your bio_vmalloc_max_vecs() helps with. > OTOH for callers where that applies it would be nice to have a > helper that loops over bio_add_vmalloc. I actually had one initially, > but given that I only found two obvious users I dropped it for now. > If we get more we can add one. The "abstract over vmalloc and normal physically contigious allocations" bit that bch2_bio_map() does is the important part. It's not uncommon to prefer physically contiguous allocations but have a vmalloc fallback; bcachefs does, and xfs does with a clever "try the big allocation if it's cheap, fall back to vmalloc to avoid waiting on compaction" that I might steal. is_vmalloc_addr() is also cheap, it's just a pointer comparison (and it really should be changed to a static inline).
