[RFC 0/2] Fix false warning in inode_to_wb

[RFC 0/2] Fix false warning in inode_to_wb

[Andreas Gruenbacher]

Hello,

when CONFIG_LOCKDEP is enabled, gfs2 triggers the following warning in
inode_to_wb() (include/linux/backing-dev.h) for a number of
filesystem-internal inodes:

  static inline struct bdi_writeback *inode_to_wb(const struct inode *inode)
  {
  #ifdef CONFIG_LOCKDEP
          WARN_ON_ONCE(debug_locks &&
                       (!lockdep_is_held(&inode->i_lock) &&
                        !lockdep_is_held(&inode->i_mapping->i_pages.xa_lock) &&
                        !lockdep_is_held(&inode->i_wb->list_lock)));
  #endif
          return inode->i_wb;
  }

This unfortunately makes lockdep unusable for gfs2.

In the most recent bug report about that problem [1], Jan Kara dug into
this and he concluded that when cgroup writeback is disabled, inode->i_wb
should be stable without any additional locking and the warnings are not
justified.  So can we please add an inode_cgwb_enabled() check to
inode_to_wb() as in Jan's patch in this series?


With that, a minor problem remains at the filesystem level:

Cgroup writeback is only enabled on filesystems that enable the
SB_I_CGROUPWB super block flag.  Unfortunately, gfs2 creates a separate
address space for filesystem metadata (sd_aspace) and sets its
mapping->host to sb->s_bdev->bd_mapping->host.  That's a "bdev" inode
with a super block that has SB_I_CGROUPWB set.  I'm not aware of any
other filesystems doing that.

To fix that, the first patch in this series creates an anonymous inode
instead of an isolated address space.  In that inode, ->i_mapping->host
points back at the inode and ->i_sb points at a gfs2 super block which
and doesn't have SB_I_CGROUPWB set.


And then there is another peculiarity of gfs2: while an 'ordinary' inode
has one address space for managing the inode's page cache, a gfs2 inode
also has a second address space for managing the inode's metadata cache.
These address spaces also point at sb->s_bdev->bd_mapping->host, causing
the same problem as above.  To fix that, the first patch changes ->host
of those address spaces to point at the new anonymous inode as well.

Using address spaces in this particular way seems to be pretty unusual
and there's a real risk that it will break some day, but I haven't found
a reasonable alternative so far.


Two previous discussions about this bug can be found here:

  [1] https://lore.kernel.org/gfs2/ebfe4024-f908-458d-9979-6ff2931c460d@I-love.SAKURA.ne.jp/
  [2] https://lore.kernel.org/all/20210713180958.66995-11-rpeterso@redhat.com/


Thanks,
Andreas


Andreas Gruenbacher (1):
  gfs2: replace sd_aspace with sd_inode

Jan Kara (1):
  writeback: Fix false warning in inode_to_wb()

 fs/gfs2/glock.c             |  3 +--
 fs/gfs2/glops.c             |  4 ++--
 fs/gfs2/incore.h            |  9 ++++++++-
 fs/gfs2/meta_io.c           |  2 +-
 fs/gfs2/meta_io.h           |  4 +---
 fs/gfs2/ops_fstype.c        | 24 +++++++++++++-----------
 fs/gfs2/super.c             |  2 +-
 include/linux/backing-dev.h |  3 ++-
 8 files changed, 29 insertions(+), 22 deletions(-)

-- 
2.48.1

[RFC 1/2] gfs2: replace sd_aspace with sd_inode

[Andreas Gruenbacher]

Use a dummy inode as mapping->host of the address spaces for global as
well as per-inode metadata.  The global metadata address space is now
accessed as gfs2_aspace(sdp) instead of sdp->sd_aspace.  The per-inode
metadata address spaces are still accessed as
gfs2_glock2aspace(GFS2_I(inode)->i_gl).

Based on a previous version from Bob Peterson from several years ago.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/glock.c      |  3 +--
 fs/gfs2/glops.c      |  4 ++--
 fs/gfs2/incore.h     |  9 ++++++++-
 fs/gfs2/meta_io.c    |  2 +-
 fs/gfs2/meta_io.h    |  4 +---
 fs/gfs2/ops_fstype.c | 24 +++++++++++++-----------
 fs/gfs2/super.c      |  2 +-
 7 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index d7220a6fe8f5..ba25b884169e 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -1166,7 +1166,6 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
 		   const struct gfs2_glock_operations *glops, int create,
 		   struct gfs2_glock **glp)
 {
-	struct super_block *s = sdp->sd_vfs;
 	struct lm_lockname name = { .ln_number = number,
 				    .ln_type = glops->go_type,
 				    .ln_sbd = sdp };
@@ -1229,7 +1228,7 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
 	mapping = gfs2_glock2aspace(gl);
 	if (mapping) {
                 mapping->a_ops = &gfs2_meta_aops;
-		mapping->host = s->s_bdev->bd_mapping->host;
+		mapping->host = sdp->sd_inode;
 		mapping->flags = 0;
 		mapping_set_gfp_mask(mapping, GFP_NOFS);
 		mapping->i_private_data = NULL;
diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index eb4714f299ef..116efe335c32 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -168,7 +168,7 @@ void gfs2_ail_flush(struct gfs2_glock *gl, bool fsync)
 static int gfs2_rgrp_metasync(struct gfs2_glock *gl)
 {
 	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
-	struct address_space *metamapping = &sdp->sd_aspace;
+	struct address_space *metamapping = gfs2_aspace(sdp);
 	struct gfs2_rgrpd *rgd = gfs2_glock2rgrp(gl);
 	const unsigned bsize = sdp->sd_sb.sb_bsize;
 	loff_t start = (rgd->rd_addr * bsize) & PAGE_MASK;
@@ -225,7 +225,7 @@ static int rgrp_go_sync(struct gfs2_glock *gl)
 static void rgrp_go_inval(struct gfs2_glock *gl, int flags)
 {
 	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
-	struct address_space *mapping = &sdp->sd_aspace;
+	struct address_space *mapping = gfs2_aspace(sdp);
 	struct gfs2_rgrpd *rgd = gfs2_glock2rgrp(gl);
 	const unsigned bsize = sdp->sd_sb.sb_bsize;
 	loff_t start, end;
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index 74abbd4970f8..0a41c4e76b32 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -795,7 +795,7 @@ struct gfs2_sbd {
 
 	/* Log stuff */
 
-	struct address_space sd_aspace;
+	struct inode *sd_inode;
 
 	spinlock_t sd_log_lock;
 
@@ -851,6 +851,13 @@ struct gfs2_sbd {
 	unsigned long sd_glock_dqs_held;
 };
 
+#define GFS2_BAD_INO 1
+
+static inline struct address_space *gfs2_aspace(struct gfs2_sbd *sdp)
+{
+	return sdp->sd_inode->i_mapping;
+}
+
 static inline void gfs2_glstats_inc(struct gfs2_glock *gl, int which)
 {
 	gl->gl_stats.stats[which]++;
diff --git a/fs/gfs2/meta_io.c b/fs/gfs2/meta_io.c
index 198cc7056637..9dc8885c95d0 100644
--- a/fs/gfs2/meta_io.c
+++ b/fs/gfs2/meta_io.c
@@ -132,7 +132,7 @@ struct buffer_head *gfs2_getbuf(struct gfs2_glock *gl, u64 blkno, int create)
 	unsigned int bufnum;
 
 	if (mapping == NULL)
-		mapping = &sdp->sd_aspace;
+		mapping = gfs2_aspace(sdp);
 
 	shift = PAGE_SHIFT - sdp->sd_sb.sb_bsize_shift;
 	index = blkno >> shift;             /* convert block to page */
diff --git a/fs/gfs2/meta_io.h b/fs/gfs2/meta_io.h
index 831d988c2ceb..b7c8a6684d02 100644
--- a/fs/gfs2/meta_io.h
+++ b/fs/gfs2/meta_io.h
@@ -44,9 +44,7 @@ static inline struct gfs2_sbd *gfs2_mapping2sbd(struct address_space *mapping)
 		struct gfs2_glock_aspace *gla =
 			container_of(mapping, struct gfs2_glock_aspace, mapping);
 		return gla->glock.gl_name.ln_sbd;
-	} else if (mapping->a_ops == &gfs2_rgrp_aops)
-		return container_of(mapping, struct gfs2_sbd, sd_aspace);
-	else
+	} else
 		return inode->i_sb->s_fs_info;
 }
 
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index e83d293c3614..ce6ee494ffc4 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -72,7 +72,6 @@ void free_sbd(struct gfs2_sbd *sdp)
 static struct gfs2_sbd *init_sbd(struct super_block *sb)
 {
 	struct gfs2_sbd *sdp;
-	struct address_space *mapping;
 
 	sdp = kzalloc(sizeof(struct gfs2_sbd), GFP_KERNEL);
 	if (!sdp)
@@ -109,16 +108,6 @@ static struct gfs2_sbd *init_sbd(struct super_block *sb)
 
 	INIT_LIST_HEAD(&sdp->sd_sc_inodes_list);
 
-	mapping = &sdp->sd_aspace;
-
-	address_space_init_once(mapping);
-	mapping->a_ops = &gfs2_rgrp_aops;
-	mapping->host = sb->s_bdev->bd_mapping->host;
-	mapping->flags = 0;
-	mapping_set_gfp_mask(mapping, GFP_NOFS);
-	mapping->i_private_data = NULL;
-	mapping->writeback_index = 0;
-
 	spin_lock_init(&sdp->sd_log_lock);
 	atomic_set(&sdp->sd_log_pinned, 0);
 	INIT_LIST_HEAD(&sdp->sd_log_revokes);
@@ -1135,6 +1124,7 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc)
 	int silent = fc->sb_flags & SB_SILENT;
 	struct gfs2_sbd *sdp;
 	struct gfs2_holder mount_gh;
+	struct address_space *mapping;
 	int error;
 
 	sdp = init_sbd(sb);
@@ -1156,6 +1146,18 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc)
 	sb->s_flags |= SB_NOSEC;
 	sb->s_magic = GFS2_MAGIC;
 	sb->s_op = &gfs2_super_ops;
+
+	/* Set up an address space for metadata writes */
+	sdp->sd_inode = new_inode(sb);
+	if (!sdp->sd_inode)
+		goto fail_free;
+	sdp->sd_inode->i_ino = GFS2_BAD_INO;
+	sdp->sd_inode->i_size = OFFSET_MAX;
+
+	mapping = gfs2_aspace(sdp);
+	mapping->a_ops = &gfs2_rgrp_aops;
+	mapping_set_gfp_mask(mapping, GFP_NOFS);
+
 	sb->s_d_op = &gfs2_dops;
 	sb->s_export_op = &gfs2_export_ops;
 	sb->s_qcop = &gfs2_quotactl_ops;
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 44e5658b896c..4529b7dda8ca 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -648,7 +648,7 @@ static void gfs2_put_super(struct super_block *sb)
 	gfs2_jindex_free(sdp);
 	/*  Take apart glock structures and buffer lists  */
 	gfs2_gl_hash_clear(sdp);
-	truncate_inode_pages_final(&sdp->sd_aspace);
+	iput(sdp->sd_inode);
 	gfs2_delete_debugfs_file(sdp);
 
 	gfs2_sys_fs_del(sdp);
-- 
2.48.1

[RFC 2/2] writeback: Fix false warning in inode_to_wb()

[Andreas Gruenbacher]

From: Jan Kara <jack@suse.cz>

inode_to_wb() is used also for filesystems that don't support cgroup
writeback. For these filesystems inode->i_wb is stable during the
lifetime of the inode (it points to bdi->wb) and there's no need to hold
locks protecting the inode->i_wb dereference. Improve the warning in
inode_to_wb() to not trigger for these filesystems.

Signed-off-by: Jan Kara <jack@suse.cz>
Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 include/linux/backing-dev.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 8e7af9a03b41..4069a027582f 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -245,10 +245,11 @@ wb_get_create_current(struct backing_dev_info *bdi, gfp_t gfp)
  * holding either @inode->i_lock, the i_pages lock, or the
  * associated wb's list_lock.
  */
-static inline struct bdi_writeback *inode_to_wb(const struct inode *inode)
+static inline struct bdi_writeback *inode_to_wb(struct inode *inode)
 {
 #ifdef CONFIG_LOCKDEP
 	WARN_ON_ONCE(debug_locks &&
+		     inode_cgwb_enabled(inode) &&
 		     (!lockdep_is_held(&inode->i_lock) &&
 		      !lockdep_is_held(&inode->i_mapping->i_pages.xa_lock) &&
 		      !lockdep_is_held(&inode->i_wb->list_lock)));
-- 
2.48.1

Re: [RFC 1/2] gfs2: replace sd_aspace with sd_inode

[Tetsuo Handa]

On 2025/04/08 3:21, Andreas Gruenbacher wrote:
> @@ -1156,6 +1146,18 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc)
>  	sb->s_flags |= SB_NOSEC;
>  	sb->s_magic = GFS2_MAGIC;
>  	sb->s_op = &gfs2_super_ops;
> +
> +	/* Set up an address space for metadata writes */
> +	sdp->sd_inode = new_inode(sb);
> +	if (!sdp->sd_inode)
> +		goto fail_free;
> +	sdp->sd_inode->i_ino = GFS2_BAD_INO;
> +	sdp->sd_inode->i_size = OFFSET_MAX;
> +
> +	mapping = gfs2_aspace(sdp);
> +	mapping->a_ops = &gfs2_rgrp_aops;
> +	mapping_set_gfp_mask(mapping, GFP_NOFS);
> +
>  	sb->s_d_op = &gfs2_dops;
>  	sb->s_export_op = &gfs2_export_ops;
>  	sb->s_qcop = &gfs2_quotactl_ops;

This will be an inode leak when hitting e.g.

	error = init_names(sdp, silent);
	if (error)
		goto fail_free;

path, for what free_sbd() in

fail_free:
	free_sbd(sdp);
	sb->s_fs_info = NULL;
	return error;

path does is nothing but free_percpu() and kfree().

Re: [RFC 1/2] gfs2: replace sd_aspace with sd_inode

[Andreas Gruenbacher]

On Tue, Apr 8, 2025 at 8:04 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> On 2025/04/08 3:21, Andreas Gruenbacher wrote:
> > @@ -1156,6 +1146,18 @@ static int gfs2_fill_super(struct super_block *sb, struct fs_context *fc)
> >       sb->s_flags |= SB_NOSEC;
> >       sb->s_magic = GFS2_MAGIC;
> >       sb->s_op = &gfs2_super_ops;
> > +
> > +     /* Set up an address space for metadata writes */
> > +     sdp->sd_inode = new_inode(sb);
> > +     if (!sdp->sd_inode)
> > +             goto fail_free;
> > +     sdp->sd_inode->i_ino = GFS2_BAD_INO;
> > +     sdp->sd_inode->i_size = OFFSET_MAX;
> > +
> > +     mapping = gfs2_aspace(sdp);
> > +     mapping->a_ops = &gfs2_rgrp_aops;
> > +     mapping_set_gfp_mask(mapping, GFP_NOFS);
> > +
> >       sb->s_d_op = &gfs2_dops;
> >       sb->s_export_op = &gfs2_export_ops;
> >       sb->s_qcop = &gfs2_quotactl_ops;
>
> This will be an inode leak when hitting e.g.
>
>         error = init_names(sdp, silent);
>         if (error)
>                 goto fail_free;
>
> path, for what free_sbd() in
>
> fail_free:
>         free_sbd(sdp);
>         sb->s_fs_info = NULL;
>         return error;
>
> path does is nothing but free_percpu() and kfree().

Indeed, an iput() is indeed needed in that case.

Thanks,
Andreas

Re: [RFC 1/2] gfs2: replace sd_aspace with sd_inode

[Christoph Hellwig]

On Mon, Apr 07, 2025 at 08:21:01PM +0200, Andreas Gruenbacher wrote:
> Use a dummy inode as mapping->host of the address spaces for global as
> well as per-inode metadata.  The global metadata address space is now
> accessed as gfs2_aspace(sdp) instead of sdp->sd_aspace.  The per-inode
> metadata address spaces are still accessed as
> gfs2_glock2aspace(GFS2_I(inode)->i_gl).
> 
> Based on a previous version from Bob Peterson from several years ago.

Please explain why you are doing this, not just what.

Re: [RFC 2/2] writeback: Fix false warning in inode_to_wb()

[Christoph Hellwig]

On Mon, Apr 07, 2025 at 08:21:02PM +0200, Andreas Gruenbacher wrote:
> -static inline struct bdi_writeback *inode_to_wb(const struct inode *inode)
> +static inline struct bdi_writeback *inode_to_wb(struct inode *inode)
>  {
>  #ifdef CONFIG_LOCKDEP
>  	WARN_ON_ONCE(debug_locks &&
> +		     inode_cgwb_enabled(inode) &&
>  		     (!lockdep_is_held(&inode->i_lock) &&
>  		      !lockdep_is_held(&inode->i_mapping->i_pages.xa_lock) &&
>  		      !lockdep_is_held(&inode->i_wb->list_lock)));
> -- 

This means that even on cgroup aware file systems we now only get
the locking validation if cgroups are actually enabled for the file
system instance and thus hugely reducing coverage, which is rather
unfortunate.

Re: [RFC 2/2] writeback: Fix false warning in inode_to_wb()

[Andreas Gruenbacher]

On Thu, Apr 10, 2025 at 10:52 AM Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Apr 07, 2025 at 08:21:02PM +0200, Andreas Gruenbacher wrote:
> > -static inline struct bdi_writeback *inode_to_wb(const struct inode *inode)
> > +static inline struct bdi_writeback *inode_to_wb(struct inode *inode)
> >  {
> >  #ifdef CONFIG_LOCKDEP
> >       WARN_ON_ONCE(debug_locks &&
> > +                  inode_cgwb_enabled(inode) &&
> >                    (!lockdep_is_held(&inode->i_lock) &&
> >                     !lockdep_is_held(&inode->i_mapping->i_pages.xa_lock) &&
> >                     !lockdep_is_held(&inode->i_wb->list_lock)));
> > --
>
> This means that even on cgroup aware file systems we now only get
> the locking validation if cgroups are actually enabled for the file
> system instance and thus hugely reducing coverage, which is rather
> unfortunate.

Right. Is checking for (inode->i_sb->s_iflags & SB_I_CGROUPWB) instead okay?

Thanks,
Andreas

Re: [RFC 1/2] gfs2: replace sd_aspace with sd_inode

[Andreas Gruenbacher]

On Thu, Apr 10, 2025 at 11:01 AM Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Apr 07, 2025 at 08:21:01PM +0200, Andreas Gruenbacher wrote:
> > Use a dummy inode as mapping->host of the address spaces for global as
> > well as per-inode metadata.  The global metadata address space is now
> > accessed as gfs2_aspace(sdp) instead of sdp->sd_aspace.  The per-inode
> > metadata address spaces are still accessed as
> > gfs2_glock2aspace(GFS2_I(inode)->i_gl).
> >
> > Based on a previous version from Bob Peterson from several years ago.
>
> Please explain why you are doing this, not just what.

Right, I have this description now:

    Currently, sdp->sd_aspace and the per-inode metadata address spaces use
    sb->s_bdev->bd_mapping->host as their ->host.  Folios in those address
    spaces will thus appear to be on "bdev" rather than on "gfs2"
    filesystems.  Those "bdev" filesystems will have the SB_I_CGROUPWB flag
    set to indicate cgroup writeback support.  In fact, gfs2 doesn't support
    cgroup writeback, though.

    To fix that, use a "dummy" gfs2 inode as ->host of those address spaces
    instead.  This will then allow functions like inode_to_wb() to determine
    that the folio belongs to a a filesystem without cgroup writeback
    support.


Thanks,
Andreas