[Cluster-devel] [GFS2 PATCH] GFS2: Add rgrp information to block_alloc trace point

[Steven Whitehouse]

Hi,

On Thu, 2012-04-12 at 08:43 -0400, Bob Peterson wrote:
> Hi,
> 
> This patch adds rgrp information to the block allocation trace point.
> 
> Regards,
> 
> Bob Peterson
> Red Hat GFS
> 
> Signed-off-by: Bob Peterson <rpeterso at redhat.com> 
> --
> Author: Bob Peterson <rpeterso at redhat.com>
> Date:   Thu Apr 12 08:32:45 2012 -0500
> 
>     GFS2: Add rgrp information to block_alloc trace point
>     
>     This patch adds resource group information to the block allocation
>     trace point for GFS2. This makes it easier to debug problems with
>     resource groups, such as management of the number of free blocks.
> 
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index 7a1cf67..146c3d2 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -1566,7 +1566,7 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *nblocks,
>  				  ip->i_inode.i_gid);
>  
>  	rgd->rd_free_clone -= *nblocks;
> -	trace_gfs2_block_alloc(ip, block, *nblocks,
> +	trace_gfs2_block_alloc(ip, rgd, block, *nblocks,
>  			       dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED);
>  	*bn = block;
>  	return 0;
> @@ -1593,7 +1593,7 @@ void __gfs2_free_blocks(struct gfs2_inode *ip, u64 bstart, u32 blen, int meta)
>  	rgd = rgblk_free(sdp, bstart, blen, GFS2_BLKST_FREE);
>  	if (!rgd)
>  		return;
> -	trace_gfs2_block_alloc(ip, bstart, blen, GFS2_BLKST_FREE);
> +	trace_gfs2_block_alloc(ip, rgd, bstart, blen, GFS2_BLKST_FREE);
>  	rgd->rd_free += blen;
>  	rgd->rd_flags &= ~GFS2_RGF_TRIMMED;
>  	gfs2_trans_add_bh(rgd->rd_gl, rgd->rd_bits[0].bi_bh, 1);
> @@ -1631,7 +1631,7 @@ void gfs2_unlink_di(struct inode *inode)
>  	rgd = rgblk_free(sdp, blkno, 1, GFS2_BLKST_UNLINKED);
>  	if (!rgd)
>  		return;
> -	trace_gfs2_block_alloc(ip, blkno, 1, GFS2_BLKST_UNLINKED);
> +	trace_gfs2_block_alloc(ip, rgd, blkno, 1, GFS2_BLKST_UNLINKED);
>  	gfs2_trans_add_bh(rgd->rd_gl, rgd->rd_bits[0].bi_bh, 1);
>  	gfs2_rgrp_out(rgd, rgd->rd_bits[0].bi_bh->b_data);
>  }
> @@ -1661,7 +1661,7 @@ static void gfs2_free_uninit_di(struct gfs2_rgrpd *rgd, u64 blkno)
>  void gfs2_free_di(struct gfs2_rgrpd *rgd, struct gfs2_inode *ip)
>  {
>  	gfs2_free_uninit_di(rgd, ip->i_no_addr);
> -	trace_gfs2_block_alloc(ip, ip->i_no_addr, 1, GFS2_BLKST_FREE);
> +	trace_gfs2_block_alloc(ip, rgd, ip->i_no_addr, 1, GFS2_BLKST_FREE);
>  	gfs2_quota_change(ip, -1, ip->i_inode.i_uid, ip->i_inode.i_gid);
>  	gfs2_meta_wipe(ip, ip->i_no_addr, 1);
>  }
> diff --git a/fs/gfs2/trace_gfs2.h b/fs/gfs2/trace_gfs2.h
> index dfa89cd..981b360 100644
> --- a/fs/gfs2/trace_gfs2.h
> +++ b/fs/gfs2/trace_gfs2.h
> @@ -457,13 +457,15 @@ TRACE_EVENT(gfs2_bmap,
>  /* Keep track of blocks as they are allocated/freed */
>  TRACE_EVENT(gfs2_block_alloc,
>  
> -	TP_PROTO(const struct gfs2_inode *ip, u64 block, unsigned len,
> -		u8 block_state),
> +	TP_PROTO(const struct gfs2_inode *ip, struct gfs2_rgrpd *rgd,
> +		 u64 block, unsigned len, u8 block_state),
>  
> -	TP_ARGS(ip, block, len, block_state),
> +	TP_ARGS(ip, rgd, block, len, block_state),
>  
>  	TP_STRUCT__entry(
>  		__field(        dev_t,  dev                     )
> +		__field(        u64,	rd_addr			)
> +		__field(        u32,	rd_free_clone		)
>  		__field(	u64,	start			)
>  		__field(	u64,	inum			)
>  		__field(	u32,	len			)
> @@ -472,14 +474,18 @@ TRACE_EVENT(gfs2_block_alloc,
>  
>  	TP_fast_assign(
>  		__entry->dev		= ip->i_gl->gl_sbd->sd_vfs->s_dev;
> +		__entry->rd_addr	= rgd->rd_addr;
> +		__entry->rd_free_clone	= rgd->rd_free_clone;
>  		__entry->start		= block;
>  		__entry->inum		= ip->i_no_addr;
>  		__entry->len		= len;
>  		__entry->block_state	= block_state;
>  	),
>  
> -	TP_printk("%u,%u bmap %llu alloc %llu/%lu %s",
> +	TP_printk("%u,%u rg:%llu rf:%u bmap %llu alloc %llu/%lu %s",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
> +		  (unsigned long long)__entry->rd_addr,
> +		  __entry->rd_free_clone,
>  		  (unsigned long long)__entry->inum,
>  		  (unsigned long long)__entry->start,
>  		  (unsigned long)__entry->len,
> 
All the bmap group tracepoints start with the device number, followed by
the string bmap, the inode number and then the start/length of the
blocks, so I'd rather not change that, without good reason.

If we are going to add the rgrp information here, then it should be done
later in the structure/string. I'm also wondering whether we shouldn't
add some of the other fields as well... rd_free and rd_dinodes spring to
mind as obvious candidates.

Since there is quite a lot of information in each rgrp, it almost
warrants its own tracepoint rather than trying to add it into an
existing one...

So I think that this probably needs some more thought,

Steve.