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

[Jan Kara <jack@suse.cz>]

On Wed 24-04-19 19:18:03, Andreas Gruenbacher wrote:
> Add a page_prepare calback that's called before a page is written to.  This
> will be used by gfs2 to start a transaction in page_prepare and end it in
> page_done.  Other filesystems that implement data journaling will require the
> same kind of mechanism.
> 
> Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>

Thanks for the patch. Some comments below.

> diff --git a/fs/iomap.c b/fs/iomap.c
> index 97cb9d486a7d..abd9aa76dbd1 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -684,6 +684,10 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags,
>  		status = __block_write_begin_int(page, pos, len, NULL, iomap);
>  	else
>  		status = __iomap_write_begin(inode, pos, len, page, iomap);
> +
> +	if (likely(!status) && iomap->page_prepare)
> +		status = iomap->page_prepare(inode, pos, len, page, iomap);
> +
>  	if (unlikely(status)) {
>  		unlock_page(page);
>  		put_page(page);

So this gets called after a page is locked. Is it OK for GFS2 to acquire
sd_log_flush_lock under page lock? Because e.g. gfs2_write_jdata_pagevec()
seems to acquire these locks the other way around so that could cause ABBA
deadlocks?

Also just looking at the code I was wondering about the following. E.g. in
iomap_write_end() we have code like:

        if (iomap->type == IOMAP_INLINE) {
		foo
	} else if (iomap->flags & IOMAP_F_BUFFER_HEAD) {
		bar
	} else {
		baz
	}

	if (iomap->page_done)
		iomap->page_done(...);

And now something very similar is in iomap_write_begin(). So won't it be
more natural to just mandate ->page_prepare() and ->page_done() callbacks
and each filesystem would set it to a helper function it needs? Probably we
could get rid of IOMAP_F_BUFFER_HEAD flag that way...

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