From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bob Peterson <rpeterso@redhat.com>
Date: Thu, 29 Oct 2015 14:39:50 -0400 (EDT)
Subject: [Cluster-devel] [PATCH 2/4] GFS2: Reduce inode size by using
 32-bit i_generation
In-Reply-To: <56325594.70105@redhat.com>
References: <1446042009-30312-1-git-send-email-rpeterso@redhat.com>
	<1446042009-30312-3-git-send-email-rpeterso@redhat.com>
	<56325594.70105@redhat.com>
Message-ID: <738004841.67890144.1446143990693.JavaMail.zimbra@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

----- Original Message -----
> 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
> 
Hi Andy,

Thanks for reviewing this. Good catches. I'll post a version 2 patch
to replace it shortly.

Regards,

Bob Peterson
Red Hat File Systems