Am Fr., 23. Dez. 2022 um 16:12 Uhr schrieb Christoph Hellwig <hch@xxxxxxxxxxxxx>: > On Fri, Dec 16, 2022 at 04:06:21PM +0100, Andreas Gruenbacher wrote: > > +static void iomap_folio_done(struct iomap_iter *iter, loff_t pos, size_t ret, > > + struct folio *folio) > > +{ > > + const struct iomap_page_ops *page_ops = iter->iomap.page_ops; > > + > > + if (folio) > > + folio_unlock(folio); > > + if (page_ops && page_ops->page_done) > > + page_ops->page_done(iter->inode, pos, ret, &folio->page); > > + if (folio) > > + folio_put(folio); > > +} > > How is the folio dereference going to work if folio is NULL? '&folio->page' is effectively a type cast, not a dereference. I realize iomap_folio_done() as introduced here is not pretty, but it's only an intermediary step and the ugliness goes away later in this series. > That being said, I really wonder if the current API is the right way to > go. Can't we just have a ->get_folio method with the same signature as > __filemap_get_folio, and then do the __filemap_get_folio from the file > system and avoid the page/folio == NULL clean path entirely? Then on > the done side move the unlock and put into the done method as well. Yes, this is what happens later in this series (as you've seen by now). > > if (!folio) { > > status = (iter->flags & IOMAP_NOWAIT) ? -EAGAIN : -ENOMEM; > > - goto out_no_page; > > + iomap_folio_done(iter, pos, 0, NULL); > > + return status; > > } > > > > /* > > @@ -656,13 +670,9 @@ static int iomap_write_begin(struct iomap_iter *iter, loff_t pos, > > return 0; > > > > out_unlock: > > - folio_unlock(folio); > > - folio_put(folio); > > + iomap_folio_done(iter, pos, 0, folio); > > iomap_write_failed(iter->inode, pos, len); > > > > -out_no_page: > > - if (page_ops && page_ops->page_done) > > - page_ops->page_done(iter->inode, pos, 0, NULL); > > return status; > > But for the current version I don't really understand why the error > unwinding changes here. Currently, we have this order of operations in iomap_write_begin(): folio_unlock() // folio_put() // iomap_write_failed() // ->page_done() and this order in iomap_write_end(): folio_unlock() // ->page_done() // folio_put() // iomap_write_failed() The unwinding in iomap_write_begin() works because this is the trivial case in which nothing happens to the page. We might just as well use the same order of operations there as in iomap_write_end() though, and when you switch to that, this is what you get. Thank you for the review. Andreas