On Sat, Apr 30, 2022 at 10:40:31PM +0100, Matthew Wilcox wrote:
> On Sat, Apr 30, 2022 at 04:44:07AM +0100, Matthew Wilcox wrote:
> > (I do not love this, have not even compiled it; it's late. We may be
> > better off just storing next_folio inside the folio_iter).
>
> Does anyone have a preference for fixing this between Option A:
>
After seeing the trace in my previous mail and several thousand
successful iterations of the test hack, I had reworked it into this
(which survived weekend testing until it ran into some other XFS problem
that looks unrelated):
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 278cc81cc1e7..aa820e09978e 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -269,6 +269,7 @@ struct folio_iter {
size_t offset;
size_t length;
/* private: for use by the iterator */
+ struct folio *_next;
size_t _seg_count;
int _i;
};
@@ -279,6 +280,7 @@ static inline void bio_first_folio(struct folio_iter *fi, struct bio *bio,
struct bio_vec *bvec = bio_first_bvec_all(bio) + i;
fi->folio = page_folio(bvec->bv_page);
+ fi->_next = folio_next(fi->folio);
fi->offset = bvec->bv_offset +
PAGE_SIZE * (bvec->bv_page - &fi->folio->page);
fi->_seg_count = bvec->bv_len;
@@ -290,13 +292,15 @@ static inline void bio_next_folio(struct folio_iter *fi, struct bio *bio)
{
fi->_seg_count -= fi->length;
if (fi->_seg_count) {
- fi->folio = folio_next(fi->folio);
+ fi->folio = fi->_next;
+ fi->_next = folio_next(fi->folio);
fi->offset = 0;
fi->length = min(folio_size(fi->folio), fi->_seg_count);
} else if (fi->_i + 1 < bio->bi_vcnt) {
bio_first_folio(fi, bio, fi->_i + 1);
} else {
fi->folio = NULL;
+ fi->_next = NULL;
}
}
So FWIW, that is just to say that I find option A to be cleaner and more
readable.
Brian
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 49eff01fb829..55e2499beff6 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -269,6 +269,7 @@ struct folio_iter {
> size_t offset;
> size_t length;
> /* private: for use by the iterator */
> + struct folio *_next;
> size_t _seg_count;
> int _i;
> };
> @@ -280,19 +281,23 @@ static inline void bio_first_folio(struct folio_iter *fi,
> struct bio *bio,
>
> fi->folio = page_folio(bvec->bv_page);
> fi->offset = bvec->bv_offset +
> - PAGE_SIZE * (bvec->bv_page - &fi->folio->page);
> + PAGE_SIZE * folio_page_idx(fi->folio, bvec->bv_page);
> fi->_seg_count = bvec->bv_len;
> fi->length = min(folio_size(fi->folio) - fi->offset, fi->_seg_count);
> fi->_i = i;
> + if (fi->_seg_count > fi->length)
> + fi->_next = folio_next(fi->folio);
> }
>
> static inline void bio_next_folio(struct folio_iter *fi, struct bio *bio)
> {
> fi->_seg_count -= fi->length;
> if (fi->_seg_count) {
> - fi->folio = folio_next(fi->folio);
> + fi->folio = fi->_next;
> fi->offset = 0;
> fi->length = min(folio_size(fi->folio), fi->_seg_count);
> + if (fi->_seg_count > fi->length)
> + fi->_next = folio_next(fi->folio);
> } else if (fi->_i + 1 < bio->bi_vcnt) {
> bio_first_folio(fi, bio, fi->_i + 1);
> } else {
>
>
> and Option B:
>
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 49eff01fb829..554f5fce060c 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -290,7 +290,8 @@ static inline void bio_next_folio(struct folio_iter *fi, struct bio *bio)
> {
> fi->_seg_count -= fi->length;
> if (fi->_seg_count) {
> - fi->folio = folio_next(fi->folio);
> + fi->folio = __folio_next(fi->folio,
> + (fi->offset + fi->length) / PAGE_SIZE);
> fi->offset = 0;
> fi->length = min(folio_size(fi->folio), fi->_seg_count);
> } else if (fi->_i + 1 < bio->bi_vcnt) {
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index de32c0383387..9c5547af8d0e 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1642,6 +1642,12 @@ static inline long folio_nr_pages(struct folio *folio)
> return compound_nr(&folio->page);
> }
>
> +static inline struct folio *__folio_next(struct folio *folio,
> + unsigned long nr_pages)
> +{
> + return (struct folio *)folio_page(folio, nr_pages);
> +}
> +
> /**
> * folio_next - Move to the next physical folio.
> * @folio: The folio we're currently operating on.
> @@ -1658,7 +1664,7 @@ static inline long folio_nr_pages(struct folio *folio)
> */
> static inline struct folio *folio_next(struct folio *folio)
> {
> - return (struct folio *)folio_page(folio, folio_nr_pages(folio));
> + return __folio_next(folio, folio_nr_pages(folio));
> }
>
> /**
>
>
> Currently running Option A through its paces.
>
