Re: [PATCH v3 1/2] iomap: Add a page_prepare callback

[Andreas Gruenbacher <agruenba@redhat.com>]

On Fri, 26 Apr 2019 at 10:30, Jan Kara <jack@suse.cz> wrote:
>
> On Thu 25-04-19 18:09:12, Andreas Gruenbacher wrote:
> > Move the page_done callback into a separate iomap_page_ops structure and
> > add a page_prepare calback to be called before a page is written to.  In
> > gfs2, we'll want to start a transaction in page_prepare and end it in
> > page_done, and other filesystems that implement data journaling will
> > require the same kind of mechanism.
>
> ...
>
> > @@ -674,9 +675,17 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
> >       if (fatal_signal_pending(current))
> >               return -EINTR;
> >
> > +     if (page_ops) {
> > +             status = page_ops->page_prepare(inode, pos, len, iomap);
> > +             if (status)
> > +                     return status;
> > +     }
> > +
>
> Looks OK for now I guess, although I'm not sure if later some fs won't need
> to get hold of the actual page in ->page_prepare() and then we will need to
> switch to ->page_prepare() returning the page to use. But let's leave that
> for a time when such fs wants to use iomap.

Alright.

> > @@ -780,8 +794,8 @@ iomap_write_end(struct inode *inode, loff_t pos, unsigned len,
> >               ret = __iomap_write_end(inode, pos, len, copied, page, iomap);
> >       }
> >
> > -     if (iomap->page_done)
> > -             iomap->page_done(inode, pos, copied, page, iomap);
> > +     if (page_ops)
> > +             page_ops->page_done(inode, pos, copied, page, iomap);
>
> Looking at the code now, this is actually flawed (preexisting problem):
> __iomap_write_end or generic_write_end() will release the page reference
> and so you cannot just pass it to ->page_done(). That is a potential
> use-after-free...

Ouch. I'm sending a fix.

Thanks,
Andreas