Re: [PATCH v15 02/17] block: Add bio_for_each_folio_all()

Matthew Wilcox
2021-07-23T02:40:40Z

On Tue, Jul 20, 2021 at 08:48:07AM +0200, Christoph Hellwig wrote:
> On Mon, Jul 19, 2021 at 07:39:46PM +0100, Matthew Wilcox (Oracle) wrote:
> > #define bio_for_each_bvec_all(bvl, bio, i) \
> > for (i = 0, bvl = bio_first_bvec_all(bio); \
> > - i < (bio)->bi_vcnt; i++, bvl++) \
> > + i < (bio)->bi_vcnt; i++, bvl++)
>
> Pleae split out this unrelated fixup.
>
> > +static inline
> > +void bio_first_folio(struct folio_iter *fi, struct bio *bio, int i)
>
> Please fix the strange formatting.

static inline void bio_first_folio(struct folio_iter *fi, struct bio *bio,
int i)

> > +{
> > + struct bio_vec *bvec = bio_first_bvec_all(bio) + i;
> > +
> > + fi->folio = page_folio(bvec->bv_page);
> > + fi->offset = bvec->bv_offset +
> > + PAGE_SIZE * (bvec->bv_page - &fi->folio->page);
>
> Can we have a little helper for the offset in folio calculation, like:
>
> static inline size_t offset_of_page_in_folio(struct page *page)
> {
> return (bvec->bv_page - &page_folio(page)->page) * PAGE;
> }
>
> as that makes the callers a lot easier to read.

I've spent most of today thinking about this one. I actually don't
want to make this easy to read. This is code that, in an ideal world,
would not exist. A bio_vec should not contain a struct page; it should
probably be:

struct bio_vec {
phys_addr_t bv_start;
unsigned int bv_len;
};

and then the helper to get from a bio_vec to a folio_iter looks like:

fi->folio = pfn_folio(bvec->bv_start >> PAGE_SHIFT);
fi->offset = offset_in_folio(fi->folio, bvec->bv_start);

If instead we decide to keep bvecs the way they are, we can at
least turn the bv_page into bv_folio, and then we won't need this
code either.