[Cluster-devel] [GFS2 PATCH 10/10] gfs2: replace sd_aspace with sd_inode

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On Tue, 2021-07-13 at 13:09 -0500, Bob Peterson wrote:
> Before this patch, gfs2 kept its own address space for rgrps, but
> this
> caused a lockdep problem because vfs assumes a 1:1 relationship
> between
> address spaces and their inode. One problematic area is this:
> 
I don't think that is the case. The reason that the address space is a
separate structure in the first place is to allow them to exist without
an inode. Maybe that has changed, but we should see why that is, in
that case rather than just making this change immediately.

I can't see any reason why if we have to have an inode here that it
needs to be hashed... what would need to look it up via the hashes?

Steve.

> gfs2_unpin
>    mark_buffer_dirty(bh);
>       mapping = page_mapping(page);
>          __set_page_dirty(page, mapping, memcg, 0);
>             xa_lock_irqsave(&mapping->i_pages, flags);
>                  ^---locks page->mapping->i_pages
>             account_page_dirtied(page, mapping)
> 	       struct inode *inode = mapping->host;
>                  ^---assumes the mapping points to an inode
>                inode_to_wb(inode)
>                   WARN_ON_ONCE !lockdep_is_held(&inode->i_mapping->
>                                                 i_pages.xa_lock)
> 
> It manifests as a lockdep warning you see in the last line.
> 
> This patch removes sd_aspace in favor of an entire inode, sd_inode.
> Functions that need to access the address space may use a new
> function
> that follows the inode to its address space. This creates the 1:1
> relation
> between the inode and its address space, so lockdep doesn't complain.
> This is how some other file systems manage their metadata, such as
> btrfs.
> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>  fs/gfs2/glops.c      |  4 ++--
>  fs/gfs2/incore.h     |  7 ++++++-
>  fs/gfs2/meta_io.c    |  2 +-
>  fs/gfs2/meta_io.h    |  2 --
>  fs/gfs2/ops_fstype.c | 27 ++++++++++++++++-----------
>  fs/gfs2/super.c      |  2 +-
>  6 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> index 744cacd27213..5d755d30d91c 100644
> --- a/fs/gfs2/glops.c
> +++ b/fs/gfs2/glops.c
> @@ -162,7 +162,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;
> @@ -219,7 +219,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 = (rgd->rd_addr * bsize) & PAGE_MASK;
> diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
> index 566c0053b7c5..075e5db1d654 100644
> --- a/fs/gfs2/incore.h
> +++ b/fs/gfs2/incore.h
> @@ -797,7 +797,7 @@ struct gfs2_sbd {
>  
>  	/* Log stuff */
>  
> -	struct address_space sd_aspace;
> +	struct inode *sd_inode;
>  
>  	spinlock_t sd_log_lock;
>  
> @@ -857,6 +857,11 @@ struct gfs2_sbd {
>  	unsigned long sd_log_flush_start;
>  };
>  
> +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 7c9619997355..0123437d9c12 100644
> --- a/fs/gfs2/meta_io.c
> +++ b/fs/gfs2/meta_io.c
> @@ -120,7 +120,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 21880d72081a..70b9c41ecb46 100644
> --- a/fs/gfs2/meta_io.h
> +++ b/fs/gfs2/meta_io.h
> @@ -42,8 +42,6 @@ static inline struct gfs2_sbd
> *gfs2_mapping2sbd(struct address_space *mapping)
>  	struct inode *inode = mapping->host;
>  	if (mapping->a_ops == &gfs2_meta_aops)
>  		return (((struct gfs2_glock *)mapping) - 1)-
> >gl_name.ln_sbd;
> -	else if (mapping->a_ops == &gfs2_rgrp_aops)
> -		return container_of(mapping, struct gfs2_sbd,
> sd_aspace);
>  	else
>  		return inode->i_sb->s_fs_info;
>  }
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index b09e61457b23..3e252cfa7f17 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)
> @@ -112,16 +111,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_inode;
> -	mapping->flags = 0;
> -	mapping_set_gfp_mask(mapping, GFP_NOFS);
> -	mapping->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);
> @@ -1141,6 +1130,7 @@ static int gfs2_fill_super(struct super_block
> *sb, struct fs_context *fc)
>  	struct gfs2_sbd *sdp;
>  	struct gfs2_holder mount_gh;
>  	struct gfs2_holder freeze_gh;
> +	struct address_space *mapping;
>  	int error;
>  
>  	sdp = init_sbd(sb);
> @@ -1162,6 +1152,21 @@ 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_MAGIC;
> +	set_nlink(sdp->sd_inode, 1);
> +	sdp->sd_inode->i_size = i_size_read(sb->s_bdev->bd_inode);
> +	insert_inode_hash(sdp->sd_inode);
> +
> +	mapping = gfs2_aspace(sdp);
> +	mapping->a_ops = &gfs2_rgrp_aops;
> +	mapping_set_gfp_mask(mapping, GFP_NOFS);
> +	mapping->writeback_index = 0;
> +
>  	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 0a5b7dfa7a45..9857d8725b2d 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -617,13 +617,13 @@ 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);
>  	gfs2_delete_debugfs_file(sdp);
>  	/*  Unmount the locking protocol  */
>  	gfs2_lm_unmount(sdp);
>  
>  	/*  At this point, we're through participating in the
> lockspace  */
>  	gfs2_sys_fs_del(sdp);
> +	iput(sdp->sd_inode);
>  	free_sbd(sdp);
>  }
>