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@xxxxxxxxxx>
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@xxxxxxxx>
SUSE Labs, CR
