[PATCH] Revert "gfs2: stop using generic_writepages in gfs2_ail1_start_one"

[PATCH] Revert "gfs2: stop using generic_writepages in gfs2_ail1_start_one"

[Andreas Gruenbacher]

Commit b2b0a5e97855 switched from generic_writepages() to
filemap_fdatawrite_wbc() in gfs2_ail1_start_one() on the path to
replacing ->writepage() with ->writepages() and eventually eliminating
the former.  Function gfs2_ail1_start_one() is called from
gfs2_log_flush(), our main function for flushing the filesystem log.

Unfortunately, at least as implemented today, ->writepage() and
->writepages() are entirely different operations for journaled data
inodes: while the former creates and submits transactions covering the
data to be written, the latter flushes dirty buffers out to disk.

With gfs2_ail1_start_one() now calling ->writepages(), we end up
creating filesystem transactions while we are in the course of a log
flush, which immediately deadlocks on the sdp->sd_log_flush_lock
semaphore.

Work around that by going back to how things used to work before commit
b2b0a5e97855 for now; figuring out a superior solution will take time we
don't have available right now.  However ...

Since the removal of generic_writepages() is imminent, open-code it
here.  We're already inside a blk_start_plug() ...  blk_finish_plug()
section here, so skip that part of the original generic_writepages().

This reverts commit b2b0a5e978552e348f85ad9c7568b630a5ede659.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/log.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index 723639376ae2..61323deb80bc 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -80,6 +80,15 @@ void gfs2_remove_from_ail(struct gfs2_bufdata *bd)
 	brelse(bd->bd_bh);
 }
 
+static int __gfs2_writepage(struct page *page, struct writeback_control *wbc,
+		       void *data)
+{
+	struct address_space *mapping = data;
+	int ret = mapping->a_ops->writepage(page, wbc);
+	mapping_set_error(mapping, ret);
+	return ret;
+}
+
 /**
  * gfs2_ail1_start_one - Start I/O on a transaction
  * @sdp: The superblock
@@ -131,7 +140,7 @@ __acquires(&sdp->sd_ail_lock)
 		if (!mapping)
 			continue;
 		spin_unlock(&sdp->sd_ail_lock);
-		ret = filemap_fdatawrite_wbc(mapping, wbc);
+		ret = write_cache_pages(mapping, wbc, __gfs2_writepage, mapping);
 		if (need_resched()) {
 			blk_finish_plug(plug);
 			cond_resched();
-- 
2.39.0

Re: [PATCH] Revert "gfs2: stop using generic_writepages in gfs2_ail1_start_one"

[Christoph Hellwig]

> +	struct address_space *mapping = data;
> +	int ret = mapping->a_ops->writepage(page, wbc);
> +	mapping_set_error(mapping, ret);
> +	return ret;

I guess beggars can't be choosers, but is there a chance to directly
call the relevant gfs2 writepage methods here instead of the
->writepage call?

Otherwise this looks good:

Acked-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] Revert "gfs2: stop using generic_writepages in gfs2_ail1_start_one"

[Andreas Gruenbacher]

On Sat, Jan 21, 2023 at 3:29 PM Christoph Hellwig <hch@lst.de> wrote:
>
> > +     struct address_space *mapping = data;
> > +     int ret = mapping->a_ops->writepage(page, wbc);
> > +     mapping_set_error(mapping, ret);
> > +     return ret;
>
> I guess beggars can't be choosers, but is there a chance to directly
> call the relevant gfs2 writepage methods here instead of the
> ->writepage call?

Yes, we could wrap struct address_space_operations and move the
writepage method into its wrapper structure relatively easily, but
that would still leave things in a messy state. So I'd really like to
reassess the validity of commit 5ac048bb7ea6 ("GFS2: Use
filemap_fdatawrite() to write back the AIL") before deciding to go
that way.

Also, we're really trying to iterate the list of inodes that are part
of the transaction here, not the list of blocks, and if we stick with
that, an actual list of inodes would help. That would be the
complement of our list of ordered inodes in a sense.

Until then, I'd like to stick with the simplest possible solution
though, which seems to be this.

> Otherwise this looks good:
>
> Acked-by: Christoph Hellwig <hch@lst.de>

Thanks a lot,
Andreas