Re: [Cluster-devel] [PATCH 1/4] gfs2: stop using generic_writepages in gfs2_ail1_start_one
On Wed, Jan 18, 2023 at 10:22:20PM +0100, Andreas Gruenbacher wrote:
> The above change means that instead of calling generic_writepages(),
> we end up calling filemap_fdatawrite_wbc() -> do_writepages() ->
> mapping->a_ops->writepages(). But that's something completely
> different; the writepages address space operation operates is outward
> facing, while we really only want to write out the dirty buffers /
> pages in the underlying address space. In case of journaled data
> inodes, gfs2_jdata_writepages() actually ends up trying to create a
> filesystem transaction, which immediately hangs because we're in the
> middle of a log flush.
>
> So I'm tempted to revert the following two of your commits; luckily
> that's independent from the iomap_writepage() removal:
>
> d3d71901b1ea ("gfs2: remove ->writepage")
> b2b0a5e97855 ("gfs2: stop using generic_writepages in gfs2_ail1_start_one")
generic_writepages is gone in linux-next, and I'd really like to keep
it that way. So if you have to do this, please open code it
using write_cache_pages and a direct call to the writepage method of
choice.
> I think we could go through iomap_writepages() instead of
> generic_writepages() here as well, but that's for another day.
Well, that would obviously be much better, and actually help with the
goal of removing ->writepage.
Re: [Cluster-devel] [PATCH 1/4] gfs2: stop using generic_writepages in gfs2_ail1_start_one
Hi Christoph,
On Tue, Jul 19, 2022 at 7:07 AM Christoph Hellwig wrote:
>
> Use filemap_fdatawrite_wbc instead of generic_writepages in
> gfs2_ail1_start_one so that the functin can also cope with address_space
> operations that only implement ->writepages and to properly account
> for cgroup writeback.
>
> Signed-off-by: Christoph Hellwig
> Reviewed-by: Andreas Gruenbacher
> ---
> fs/gfs2/log.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index f0ee3ff6f9a87..a66e3b1f6d178 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -131,7 +131,7 @@ __acquires(>sd_ail_lock)
> if (!mapping)
> continue;
> spin_unlock(>sd_ail_lock);
> - ret = generic_writepages(mapping, wbc);
> + ret = filemap_fdatawrite_wbc(mapping, wbc);
this patch unfortunately breaks journaled data inodes.
We're in function gfs2_ail1_start_one() here, which is usually called
via gfs2_log_flush() -> empty_ail1_list() -> gfs2_ail1_start() ->
gfs2_ail1_flush() -> gfs2_ail1_start_one(), and we're going through
the list of buffer heads in the transaction to be flushed. We used to
submit each dirty buffer for I/O individually, but since commit
5ac048bb7ea6 ("GFS2: Use filemap_fdatawrite() to write back the AIL"),
we're submitting all the dirty pages in the metadata address space
this buffer head belongs to. That's slightly bizarre, but it happens
to catch exactly the same buffer heads that are in the transaction, so
we end up with the same result. From what I'm being told, this was
done as a performance optimization -- but nobody actually knows the
details anymore.
The above change means that instead of calling generic_writepages(),
we end up calling filemap_fdatawrite_wbc() -> do_writepages() ->
mapping->a_ops->writepages(). But that's something completely
different; the writepages address space operation operates is outward
facing, while we really only want to write out the dirty buffers /
pages in the underlying address space. In case of journaled data
inodes, gfs2_jdata_writepages() actually ends up trying to create a
filesystem transaction, which immediately hangs because we're in the
middle of a log flush.
So I'm tempted to revert the following two of your commits; luckily
that's independent from the iomap_writepage() removal:
d3d71901b1ea ("gfs2: remove ->writepage")
b2b0a5e97855 ("gfs2: stop using generic_writepages in gfs2_ail1_start_one")
I think we could go through iomap_writepages() instead of
generic_writepages() here as well, but that's for another day.
As far as cgroup writeback goes, this is journal I/O and I don't have
the faintest idea how the accounting for that is even supposed to
work.
> if (need_resched()) {
> blk_finish_plug(plug);
> cond_resched();
> @@ -222,8 +222,7 @@ void gfs2_ail1_flush(struct gfs2_sbd *sdp, struct
> writeback_control *wbc)
> spin_unlock(>sd_ail_lock);
> blk_finish_plug();
> if (ret) {
> - gfs2_lm(sdp, "gfs2_ail1_start_one (generic_writepages) "
> - "returned: %d\n", ret);
> + gfs2_lm(sdp, "gfs2_ail1_start_one returned: %d\n", ret);
> gfs2_withdraw(sdp);
> }
> trace_gfs2_ail_flush(sdp, wbc, 0);
> --
> 2.30.2
>
Thanks,
Andreas
[Cluster-devel] [PATCH 1/4] gfs2: stop using generic_writepages in gfs2_ail1_start_one
Use filemap_fdatawrite_wbc instead of generic_writepages in
gfs2_ail1_start_one so that the functin can also cope with address_space
operations that only implement ->writepages and to properly account
for cgroup writeback.
Signed-off-by: Christoph Hellwig
Reviewed-by: Andreas Gruenbacher
---
fs/gfs2/log.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index f0ee3ff6f9a87..a66e3b1f6d178 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -131,7 +131,7 @@ __acquires(>sd_ail_lock)
if (!mapping)
continue;
spin_unlock(>sd_ail_lock);
- ret = generic_writepages(mapping, wbc);
+ ret = filemap_fdatawrite_wbc(mapping, wbc);
if (need_resched()) {
blk_finish_plug(plug);
cond_resched();
@@ -222,8 +222,7 @@ void gfs2_ail1_flush(struct gfs2_sbd *sdp, struct
writeback_control *wbc)
spin_unlock(>sd_ail_lock);
blk_finish_plug();
if (ret) {
- gfs2_lm(sdp, "gfs2_ail1_start_one (generic_writepages) "
- "returned: %d\n", ret);
+ gfs2_lm(sdp, "gfs2_ail1_start_one returned: %d\n", ret);
gfs2_withdraw(sdp);
}
trace_gfs2_ail_flush(sdp, wbc, 0);
--
2.30.2
Re: [Cluster-devel] [PATCH 1/4] gfs2: stop using generic_writepages in gfs2_ail1_start_one
On Mon, Jul 11, 2022 at 12:27:47PM +0200, Andreas Gruenbacher wrote: > Can you add the below follow-up cleanup? > - gfs2_lm(sdp, "gfs2_ail1_start_one (generic_writepages) " > - "returned: %d\n", ret); > + gfs2_lm(sdp, "gfs2_ail1_start_one returned %d\n", ret); The cleanup looks fine to me, yes.
Re: [Cluster-devel] [PATCH 1/4] gfs2: stop using generic_writepages in gfs2_ail1_start_one
Hello,
On Mon, Jul 11, 2022 at 7:27 AM Christoph Hellwig wrote:
>
> Use filemap_fdatawrite_wbc instead of generic_writepages in
> gfs2_ail1_start_one so that the functin can also cope with address_space
> operations that only implement ->writepages and to properly account
> for cgroup writeback.
Reviewed-by: Andreas Gruenbacher
I assume you want to push this through the xfs tree.
Can you add the below follow-up cleanup?
Thanks,
Andreas
---
fs/gfs2/log.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index 624dffc96136..7fc6cb95dec8 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -222,8 +222,7 @@ void gfs2_ail1_flush(struct gfs2_sbd *sdp, struct
writeback_control *wbc)
spin_unlock(>sd_ail_lock);
blk_finish_plug();
if (ret) {
- gfs2_lm(sdp, "gfs2_ail1_start_one (generic_writepages) "
- "returned: %d\n", ret);
+ gfs2_lm(sdp, "gfs2_ail1_start_one returned %d\n", ret);
gfs2_withdraw(sdp);
}
trace_gfs2_ail_flush(sdp, wbc, 0);
--
2.36.1
[Cluster-devel] [PATCH 1/4] gfs2: stop using generic_writepages in gfs2_ail1_start_one
Use filemap_fdatawrite_wbc instead of generic_writepages in
gfs2_ail1_start_one so that the functin can also cope with address_space
operations that only implement ->writepages and to properly account
for cgroup writeback.
Signed-off-by: Christoph Hellwig
---
fs/gfs2/log.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index f0ee3ff6f9a87..624dffc96136b 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -131,7 +131,7 @@ __acquires(>sd_ail_lock)
if (!mapping)
continue;
spin_unlock(>sd_ail_lock);
- ret = generic_writepages(mapping, wbc);
+ ret = filemap_fdatawrite_wbc(mapping, wbc);
if (need_resched()) {
blk_finish_plug(plug);
cond_resched();
--
2.30.2
