From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Price <anprice@redhat.com>
Date: Thu, 29 Oct 2015 17:21:24 +0000
Subject: [Cluster-devel] [PATCH 2/4] GFS2: Reduce inode size by using
 32-bit i_generation
In-Reply-To: <1446042009-30312-3-git-send-email-rpeterso@redhat.com>
References: <1446042009-30312-1-git-send-email-rpeterso@redhat.com>
	<1446042009-30312-3-git-send-email-rpeterso@redhat.com>
Message-ID: <56325594.70105@redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

Hi Bob,

On 28/10/15 14:20, Bob Peterson wrote:
> This patch removes variable i_generation from gfs2_inode. In its
> place, we use inode->i_generation which is pretty much the same thing,
> in 32-bits. The loss of 32 bits should not be a problem.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>   fs/gfs2/glops.c                  |  2 +-
>   fs/gfs2/incore.h                 |  9 +++++++--
>   fs/gfs2/inode.c                  |  5 +++--
>   fs/gfs2/rgrp.c                   | 16 ++++++++--------
>   fs/gfs2/rgrp.h                   |  2 +-
>   fs/gfs2/super.c                  |  2 +-
>   include/uapi/linux/gfs2_ondisk.h | 25 +++++++++++++++++++++----
>   7 files changed, 42 insertions(+), 19 deletions(-)
>
> diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
> index 1f6c9c3..79cd4a9 100644
> --- a/fs/gfs2/glops.c
> +++ b/fs/gfs2/glops.c
> @@ -358,7 +358,7 @@ static int gfs2_dinode_in(struct gfs2_inode *ip, const void *buf)
>   	ip->i_inode.i_ctime.tv_nsec = be32_to_cpu(str->di_ctime_nsec);
>
>   	ip->i_goal = be64_to_cpu(str->di_goal_meta);
> -	ip->i_generation = be64_to_cpu(str->di_generation);
> +	ip->i_inode.i_generation = be32_to_cpu(str->di_gen2);
>
>   	ip->i_diskflags = be32_to_cpu(str->di_flags);
>   	ip->i_eattr = be64_to_cpu(str->di_eattr);
> diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
> index 76c9c77..8ca88b9 100644
> --- a/fs/gfs2/incore.h
> +++ b/fs/gfs2/incore.h
> @@ -88,7 +88,13 @@ struct gfs2_rgrpd {
>   	u32 rd_reserved;                /* number of blocks reserved */
>   	u32 rd_free_clone;
>   	u32 rd_dinodes;
> -	u64 rd_igeneration;
> +	union {
> +		u64 rd_igeneration;
> +		struct {
> +			u32 rd_igen1;
> +			u32 rd_igen2;
> +		};
> +	};

Is this union necessary given that it's not an on-disk structure?

>   	struct gfs2_bitmap *rd_bits;
>   	struct gfs2_sbd *rd_sbd;
>   	struct gfs2_rgrp_lvb *rd_rgl;
> @@ -385,7 +391,6 @@ struct gfs2_inode {
>   	struct inode i_inode;
>   	u64 i_no_addr;
>   	u64 i_no_formal_ino;
> -	u64 i_generation;
>   	u64 i_eattr;
>   	unsigned long i_flags;		/* GIF_... */
>   	struct gfs2_glock *i_gl; /* Move into i_gh? */
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 063fdfc..cc815ab 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -394,8 +394,9 @@ static int alloc_dinode(struct gfs2_inode *ip, u32 flags, unsigned *dblocks)
>   	if (error)
>   		goto out_ipreserv;
>
> -	error = gfs2_alloc_blocks(ip, &ip->i_no_addr, dblocks, 1, &ip->i_generation);
> -	ip->i_no_formal_ino = ip->i_generation;
> +	error = gfs2_alloc_blocks(ip, &ip->i_no_addr, dblocks, 1,
> +				  &ip->i_inode.i_generation);
> +	ip->i_no_formal_ino = ip->i_inode.i_generation;
>   	ip->i_inode.i_ino = ip->i_no_addr;
>   	ip->i_goal = ip->i_no_addr;
>
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index 475985d..34c42a8 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -1062,7 +1062,7 @@ static void gfs2_rgrp_in(struct gfs2_rgrpd *rgd, const void *buf)
>   	rgd->rd_flags |= rg_flags;
>   	rgd->rd_free = be32_to_cpu(str->rg_free);
>   	rgd->rd_dinodes = be32_to_cpu(str->rg_dinodes);
> -	rgd->rd_igeneration = be64_to_cpu(str->rg_igeneration);
> +	rgd->rd_igen2 = be32_to_cpu(str->rg_igen2);
>   }
>
>   static void gfs2_rgrp_out(struct gfs2_rgrpd *rgd, void *buf)
> @@ -1073,7 +1073,7 @@ static void gfs2_rgrp_out(struct gfs2_rgrpd *rgd, void *buf)
>   	str->rg_free = cpu_to_be32(rgd->rd_free);
>   	str->rg_dinodes = cpu_to_be32(rgd->rd_dinodes);
>   	str->__pad = cpu_to_be32(0);
> -	str->rg_igeneration = cpu_to_be64(rgd->rd_igeneration);
> +	str->rg_igen2 = cpu_to_be64(rgd->rd_igen2);

Probably should be using cpu_to_be32()

>   	memset(&str->rg_reserved, 0, sizeof(str->rg_reserved));
>   }
>
> @@ -1084,7 +1084,7 @@ static int gfs2_rgrp_lvb_valid(struct gfs2_rgrpd *rgd)
>
>   	if (rgl->rl_flags != str->rg_flags || rgl->rl_free != str->rg_free ||
>   	    rgl->rl_dinodes != str->rg_dinodes ||
> -	    rgl->rl_igeneration != str->rg_igeneration)
> +	    rgl->rl_igen2 != str->rg_igen2)
>   		return 0;
>   	return 1;
>   }
> @@ -1097,7 +1097,7 @@ static void gfs2_rgrp_ondisk2lvb(struct gfs2_rgrp_lvb *rgl, const void *buf)
>   	rgl->rl_flags = str->rg_flags;
>   	rgl->rl_free = str->rg_free;
>   	rgl->rl_dinodes = str->rg_dinodes;
> -	rgl->rl_igeneration = str->rg_igeneration;
> +	rgl->rl_igen2 = str->rg_igen2;
>   	rgl->__pad = 0UL;
>   }
>
> @@ -1229,7 +1229,7 @@ static int update_rgrp_lvb(struct gfs2_rgrpd *rgd)
>   	rgd->rd_free = be32_to_cpu(rgd->rd_rgl->rl_free);
>   	rgd->rd_free_clone = rgd->rd_free;
>   	rgd->rd_dinodes = be32_to_cpu(rgd->rd_rgl->rl_dinodes);
> -	rgd->rd_igeneration = be64_to_cpu(rgd->rd_rgl->rl_igeneration);
> +	rgd->rd_igen2 = be64_to_cpu(rgd->rd_rgl->rl_igen2);

Probably should be using be32_to_cpu()

>   	return 0;
>   }
>
> @@ -2334,7 +2334,7 @@ static void gfs2_set_alloc_start(struct gfs2_rbm *rbm,
>    */
>
>   int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *nblocks,
> -		      bool dinode, u64 *generation)
> +		      bool dinode, u32 *generation)
>   {
>   	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
>   	struct buffer_head *dibh;
> @@ -2390,9 +2390,9 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *nblocks,
>   	rbm.rgd->rd_free -= *nblocks;
>   	if (dinode) {
>   		rbm.rgd->rd_dinodes++;
> -		*generation = rbm.rgd->rd_igeneration++;
> +		*generation = rbm.rgd->rd_igen2++;
>   		if (*generation == 0)
> -			*generation = rbm.rgd->rd_igeneration++;
> +			*generation = rbm.rgd->rd_igen2++;
>   	}
>
>   	gfs2_trans_add_meta(rbm.rgd->rd_gl, rbm.rgd->rd_bits[0].bi_bh);
> diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
> index c0ab33f..71c1a8e 100644
> --- a/fs/gfs2/rgrp.h
> +++ b/fs/gfs2/rgrp.h
> @@ -47,7 +47,7 @@ extern int gfs2_inplace_reserve(struct gfs2_inode *ip,
>   extern void gfs2_inplace_release(struct gfs2_inode *ip);
>
>   extern int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
> -			     bool dinode, u64 *generation);
> +			     bool dinode, u32 *generation);
>
>   extern int gfs2_rs_alloc(struct gfs2_inode *ip);
>   extern void gfs2_rs_deltree(struct gfs2_blkreserv *rs);
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 894fb01..928be2f 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -714,7 +714,7 @@ void gfs2_dinode_out(const struct gfs2_inode *ip, void *buf)
>
>   	str->di_goal_meta = cpu_to_be64(ip->i_goal);
>   	str->di_goal_data = cpu_to_be64(ip->i_goal);
> -	str->di_generation = cpu_to_be64(ip->i_generation);
> +	str->di_gen2 = cpu_to_be64(ip->i_inode.i_generation);

Again here.

>
>   	str->di_flags = cpu_to_be32(ip->i_diskflags);
>   	str->di_height = cpu_to_be16(ip->i_height);
> diff --git a/include/uapi/linux/gfs2_ondisk.h b/include/uapi/linux/gfs2_ondisk.h
> index 1a763ea..bef244e 100644
> --- a/include/uapi/linux/gfs2_ondisk.h
> +++ b/include/uapi/linux/gfs2_ondisk.h
> @@ -175,7 +175,13 @@ struct gfs2_rgrp_lvb {
>   	__be32 rl_flags;
>   	__be32 rl_free;
>   	__be32 rl_dinodes;
> -	__be64 rl_igeneration;
> +	union {
> +		__be64 rl_igeneration;
> +		struct {
> +			__be32 rl_igen1;
> +			__be32 rl_igen2;
> +		};
> +	};
>   	__be32 rl_unlinked;
>   	__be32 __pad;
>   };
> @@ -187,7 +193,13 @@ struct gfs2_rgrp {
>   	__be32 rg_free;
>   	__be32 rg_dinodes;
>   	__be32 __pad;
> -	__be64 rg_igeneration;
> +	union {
> +		__be64 rg_igeneration;
> +		struct {
> +			__be32 rg_igen1;
> +			__be32 rg_igen2;
> +		};
> +	};
>
>   	__u8 rg_reserved[80]; /* Several fields from gfs1 now reserved */
>   };
> @@ -268,8 +280,13 @@ struct gfs2_dinode {
>   	 */
>   	__be64 di_goal_meta;	/* rgrp to alloc from next */
>   	__be64 di_goal_data;	/* data block goal */
> -	__be64 di_generation;	/* generation number for NFS */
> -
> +	union {
> +		__be64 di_generation;	/* generation number for NFS */
> +		struct {
> +			__be32 di_gen1;
> +			__be32 di_gen2;
> +		};
> +	};
>   	__be32 di_flags;	/* GFS2_DIF_... */
>   	__be32 di_payload_format;  /* GFS2_FORMAT_... */
>   	__u16 __pad1;	/* Was ditype in gfs1 */
>

It would be useful to have some description of how the two u32s are 
meant to be used now that the u64 is split. Although, I notice that only 
the *gen2 fields are being used now, so perhaps the *gen2 fields could 
be given a more descriptive name and the *gen1 fields could follow the 
__pad convention instead.

I assume there are no endianness problems here as the generation number 
is always big-endian on-disk so it will always be found in *gen2.

I'm not too familiar with how the generation number is used so I hope 
it's also safe to assume that inode->i_generation will never be bumped 
up to 64 bits to avoid some kind of i_generation-Y2K bug :)

Cheers,
Andy