Re: generic/068 crash on 5.18-rc2?

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, May 02, 2022 at 08:20:00AM -0400, Brian Foster wrote:
> 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.

Me too.  I'll queue up the usual nightly tests with that patch added and
we'll see how that does.

--D

> 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.
> > 
>