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

[Jan Kara <jack@suse.cz>]

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.

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

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR