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
