Hi Willy, On 2023-03-17 17:14, Pankaj Raghav wrote: > On 2023-03-17 16:31, Matthew Wilcox wrote: >>> + >>> + while ((folio = readahead_folio(rac))) { >>> + folio_mark_uptodate(folio); >>> + folio_unlock(folio); >>> } >> >> readahead_folio() is a bit too heavy-weight for that, IMO. I'd do this >> as; >> >> while ((folio = readahead_folio(rac))) { >> if (!ret) >> folio_mark_uptodate(folio); >> folio_unlock(folio); >> } >> > > This looks good. > >> (there's no need to call folio_set_error(), nor folio_clear_uptodate()) > > I am trying to understand why these calls are not needed for the error case. > I see similar pattern, for e.g. in iomap_finish_folio_read() where we call these > functions for the error case. > I am planning to send the next version. It would be great if I can get a rationale for your statement regarding not needing to call folio_set_error() or folio_clear_uptodate(). > If we don't need to call these anymore, can the mpage code also be shortened like this: > > -static void mpage_end_io(struct bio *bio) > +static void mpage_read_end_io(struct bio *bio) > { > - struct bio_vec *bv; > - struct bvec_iter_all iter_all; > + struct folio_iter fi; > + int err = blk_status_to_errno(bio->bi_status); > > - bio_for_each_segment_all(bv, bio, iter_all) { > - struct page *page = bv->bv_page; > - page_endio(page, bio_op(bio), > - blk_status_to_errno(bio->bi_status)); > + bio_for_each_folio_all(fi, bio) { > + struct folio *folio = fi.folio; > + > + if (!err) > + folio_mark_uptodate(folio); > + folio_unlock(folio); > + } > + > + bio_put(bio); > +} > + > +static void mpage_write_end_io(struct bio *bio) > +{ > .... > +