[Cluster-devel] [GFS2 PATCH] GFS2: combine gfs2_alloc_block and gfs2_alloc_di

[Cluster-devel] [GFS2 PATCH] GFS2: combine gfs2_alloc_block and gfs2_alloc_di

[Bob Peterson]

Hi,

GFS2 functions gfs2_alloc_block and gfs2_alloc_di do basically
the same things, with a few exceptions. This patch combines
the two functions into a slightly more generic gfs2_alloc_block.
Having one centralized block allocation function will reduce
code redundancy and make it easier to implement multi-block
reservations to reduce file fragmentation in the future.

Regards,

Bob Peterson
Red Hat File Systems
--
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index f6be14f..b69235b 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -133,7 +133,7 @@ int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
 		   and write it out to disk */
 
 		unsigned int n = 1;
-		error = gfs2_alloc_block(ip, &block, &n);
+		error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
 		if (error)
 			goto out_brelse;
 		if (isdir) {
@@ -503,7 +503,7 @@ static int gfs2_bmap_alloc(struct inode *inode, const sector_t lblock,
 	do {
 		int error;
 		n = blks - alloced;
-		error = gfs2_alloc_block(ip, &bn, &n);
+		error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
 		if (error)
 			return error;
 		alloced += n;
diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index 946b6f8..ae75319 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -823,7 +823,7 @@ static struct gfs2_leaf *new_leaf(struct inode *inode, struct buffer_head **pbh,
 	struct gfs2_dirent *dent;
 	struct qstr name = { .name = "", .len = 0, .hash = 0 };
 
-	error = gfs2_alloc_block(ip, &bn, &n);
+	error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
 	if (error)
 		return NULL;
 	bh = gfs2_meta_new(ip->i_gl, bn);
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 377920d..de2668f 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -402,7 +402,7 @@ static int alloc_dinode(struct gfs2_inode *dip, u64 *no_addr, u64 *generation)
 	if (error)
 		goto out_ipreserv;
 
-	error = gfs2_alloc_di(dip, no_addr, generation);
+	error = gfs2_alloc_block(dip, no_addr, NULL, 1, generation);
 
 	gfs2_trans_end(sdp);
 
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index a1a815b..995f4e6 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1304,19 +1304,24 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
  * @ip: the inode to allocate the block for
  * @bn: Used to return the starting block number
  * @n: requested number of blocks/extent length (value/result)
+ * dinode: 1 if we're allocating a dinode, 0 if it's a data block
+ * @generation: the generation number of the inode
  *
  * Returns: 0 or error
  */
 
-int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n)
+int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
+		     int dinode, u64 *generation)
 {
 	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
 	struct buffer_head *dibh;
 	struct gfs2_alloc *al = ip->i_alloc;
 	struct gfs2_rgrpd *rgd;
-	u32 goal, blk;
-	u64 block;
+	u32 goal, blk; /* block, within the rgrp scope */
+	u64 block; /* block, within the file system scope */
+	unsigned int extn = 1;
 	int error;
+	unsigned char blk_type = dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED;
 
 	/* Only happens if there is a bug in gfs2, return something distinctive
 	 * to ensure that it is noticed.
@@ -1324,14 +1329,16 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n)
 	if (al == NULL)
 		return -ECANCELED;
 
+	if (n == NULL)
+		n = &extn;
 	rgd = ip->i_rgd;
 
-	if (rgrp_contains_block(rgd, ip->i_goal))
+	if (!dinode && rgrp_contains_block(rgd, ip->i_goal))
 		goal = ip->i_goal - rgd->rd_data0;
 	else
 		goal = rgd->rd_last_alloc;
 
-	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, GFS2_BLKST_USED, n);
+	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, blk_type, n);
 
 	/* Since all blocks are reserved in advance, this shouldn't happen */
 	if (blk == BFITNOENT)
@@ -1339,82 +1346,43 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n)
 
 	rgd->rd_last_alloc = blk;
 	block = rgd->rd_data0 + blk;
-	ip->i_goal = block + *n - 1;
-	error = gfs2_meta_inode_buffer(ip, &dibh);
-	if (error == 0) {
-		struct gfs2_dinode *di = (struct gfs2_dinode *)dibh->b_data;
-		gfs2_trans_add_bh(ip->i_gl, dibh, 1);
-		di->di_goal_meta = di->di_goal_data = cpu_to_be64(ip->i_goal);
-		brelse(dibh);
+	if (!dinode) {
+		ip->i_goal = block + *n - 1;
+		error = gfs2_meta_inode_buffer(ip, &dibh);
+		if (error == 0) {
+			struct gfs2_dinode *di =
+				(struct gfs2_dinode *)dibh->b_data;
+			gfs2_trans_add_bh(ip->i_gl, dibh, 1);
+			di->di_goal_meta = di->di_goal_data =
+				cpu_to_be64(ip->i_goal);
+			brelse(dibh);
+		}
 	}
 	if (rgd->rd_free < *n)
 		goto rgrp_error;
 
 	rgd->rd_free -= *n;
+	if (dinode) {
+		rgd->rd_dinodes++;
+		*generation = rgd->rd_igeneration++;
+		if (*generation == 0)
+			*generation = rgd->rd_igeneration++;
+	}
 
 	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);
 
 	al->al_alloced += *n;
 
-	gfs2_statfs_change(sdp, 0, -(s64)*n, 0);
-	gfs2_quota_change(ip, *n, ip->i_inode.i_uid, ip->i_inode.i_gid);
+	gfs2_statfs_change(sdp, 0, -(s64)*n, dinode ? 1 : 0);
+	if (dinode)
+		gfs2_trans_add_unrevoke(sdp, block, 1);
+	else
+		gfs2_quota_change(ip, *n, ip->i_inode.i_uid,
+				  ip->i_inode.i_gid);
 
 	rgd->rd_free_clone -= *n;
-	trace_gfs2_block_alloc(ip, block, *n, GFS2_BLKST_USED);
-	*bn = block;
-	return 0;
-
-rgrp_error:
-	gfs2_rgrp_error(rgd);
-	return -EIO;
-}
-
-/**
- * gfs2_alloc_di - Allocate a dinode
- * @dip: the directory that the inode is going in
- * @bn: the block number which is allocated
- * @generation: the generation number of the inode
- *
- * Returns: 0 on success or error
- */
-
-int gfs2_alloc_di(struct gfs2_inode *dip, u64 *bn, u64 *generation)
-{
-	struct gfs2_sbd *sdp = GFS2_SB(&dip->i_inode);
-	struct gfs2_alloc *al = dip->i_alloc;
-	struct gfs2_rgrpd *rgd = dip->i_rgd;
-	u32 blk;
-	u64 block;
-	unsigned int n = 1;
-
-	blk = rgblk_search(rgd, rgd->rd_last_alloc,
-			   GFS2_BLKST_FREE, GFS2_BLKST_DINODE, &n);
-
-	/* Since all blocks are reserved in advance, this shouldn't happen */
-	if (blk == BFITNOENT)
-		goto rgrp_error;
-
-	rgd->rd_last_alloc = blk;
-	block = rgd->rd_data0 + blk;
-	if (rgd->rd_free == 0)
-		goto rgrp_error;
-
-	rgd->rd_free--;
-	rgd->rd_dinodes++;
-	*generation = rgd->rd_igeneration++;
-	if (*generation == 0)
-		*generation = rgd->rd_igeneration++;
-	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);
-
-	al->al_alloced++;
-
-	gfs2_statfs_change(sdp, 0, -1, +1);
-	gfs2_trans_add_unrevoke(sdp, block, 1);
-
-	rgd->rd_free_clone--;
-	trace_gfs2_block_alloc(dip, block, 1, GFS2_BLKST_DINODE);
+	trace_gfs2_block_alloc(ip, block, *n, blk_type);
 	*bn = block;
 	return 0;
 
diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
index cf5c501..4cb5608 100644
--- a/fs/gfs2/rgrp.h
+++ b/fs/gfs2/rgrp.h
@@ -39,8 +39,8 @@ static inline void gfs2_alloc_put(struct gfs2_inode *ip)
 extern int gfs2_inplace_reserve(struct gfs2_inode *ip);
 extern void gfs2_inplace_release(struct gfs2_inode *ip);
 
-extern int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n);
-extern int gfs2_alloc_di(struct gfs2_inode *ip, u64 *bn, u64 *generation);
+extern int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
+			    int dinode, u64 *generation);
 
 extern void __gfs2_free_blocks(struct gfs2_inode *ip, u64 bstart, u32 blen, int meta);
 extern void gfs2_free_meta(struct gfs2_inode *ip, u64 bstart, u32 blen);
diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
index a201a1d..e4794a5 100644
--- a/fs/gfs2/xattr.c
+++ b/fs/gfs2/xattr.c
@@ -610,7 +610,7 @@ static int ea_alloc_blk(struct gfs2_inode *ip, struct buffer_head **bhp)
 	u64 block;
 	int error;
 
-	error = gfs2_alloc_block(ip, &block, &n);
+	error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
 	if (error)
 		return error;
 	gfs2_trans_add_unrevoke(sdp, block, 1);
@@ -672,7 +672,7 @@ static int ea_write(struct gfs2_inode *ip, struct gfs2_ea_header *ea,
 			int mh_size = sizeof(struct gfs2_meta_header);
 			unsigned int n = 1;
 
-			error = gfs2_alloc_block(ip, &block, &n);
+			error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
 			if (error)
 				return error;
 			gfs2_trans_add_unrevoke(sdp, block, 1);
@@ -992,7 +992,7 @@ static int ea_set_block(struct gfs2_inode *ip, struct gfs2_ea_request *er,
 	} else {
 		u64 blk;
 		unsigned int n = 1;
-		error = gfs2_alloc_block(ip, &blk, &n);
+		error = gfs2_alloc_block(ip, &blk, &n, 0, NULL);
 		if (error)
 			return error;
 		gfs2_trans_add_unrevoke(sdp, blk, 1);

[Cluster-devel] [GFS2 PATCH] GFS2: combine gfs2_alloc_block and gfs2_alloc_di

[Steven Whitehouse]

Hi,

Both patches are now in the -nmw tree. Thanks,

Steve.

On Mon, 2011-11-14 at 11:17 -0500, Bob Peterson wrote:
> Hi,
> 
> GFS2 functions gfs2_alloc_block and gfs2_alloc_di do basically
> the same things, with a few exceptions. This patch combines
> the two functions into a slightly more generic gfs2_alloc_block.
> Having one centralized block allocation function will reduce
> code redundancy and make it easier to implement multi-block
> reservations to reduce file fragmentation in the future.
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> --
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index f6be14f..b69235b 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -133,7 +133,7 @@ int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
>  		   and write it out to disk */
>  
>  		unsigned int n = 1;
> -		error = gfs2_alloc_block(ip, &block, &n);
> +		error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
>  		if (error)
>  			goto out_brelse;
>  		if (isdir) {
> @@ -503,7 +503,7 @@ static int gfs2_bmap_alloc(struct inode *inode, const sector_t lblock,
>  	do {
>  		int error;
>  		n = blks - alloced;
> -		error = gfs2_alloc_block(ip, &bn, &n);
> +		error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
>  		if (error)
>  			return error;
>  		alloced += n;
> diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
> index 946b6f8..ae75319 100644
> --- a/fs/gfs2/dir.c
> +++ b/fs/gfs2/dir.c
> @@ -823,7 +823,7 @@ static struct gfs2_leaf *new_leaf(struct inode *inode, struct buffer_head **pbh,
>  	struct gfs2_dirent *dent;
>  	struct qstr name = { .name = "", .len = 0, .hash = 0 };
>  
> -	error = gfs2_alloc_block(ip, &bn, &n);
> +	error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
>  	if (error)
>  		return NULL;
>  	bh = gfs2_meta_new(ip->i_gl, bn);
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 377920d..de2668f 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -402,7 +402,7 @@ static int alloc_dinode(struct gfs2_inode *dip, u64 *no_addr, u64 *generation)
>  	if (error)
>  		goto out_ipreserv;
>  
> -	error = gfs2_alloc_di(dip, no_addr, generation);
> +	error = gfs2_alloc_block(dip, no_addr, NULL, 1, generation);
>  
>  	gfs2_trans_end(sdp);
>  
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index a1a815b..995f4e6 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -1304,19 +1304,24 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
>   * @ip: the inode to allocate the block for
>   * @bn: Used to return the starting block number
>   * @n: requested number of blocks/extent length (value/result)
> + * dinode: 1 if we're allocating a dinode, 0 if it's a data block
> + * @generation: the generation number of the inode
>   *
>   * Returns: 0 or error
>   */
>  
> -int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n)
> +int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
> +		     int dinode, u64 *generation)
>  {
>  	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
>  	struct buffer_head *dibh;
>  	struct gfs2_alloc *al = ip->i_alloc;
>  	struct gfs2_rgrpd *rgd;
> -	u32 goal, blk;
> -	u64 block;
> +	u32 goal, blk; /* block, within the rgrp scope */
> +	u64 block; /* block, within the file system scope */
> +	unsigned int extn = 1;
>  	int error;
> +	unsigned char blk_type = dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED;
>  
>  	/* Only happens if there is a bug in gfs2, return something distinctive
>  	 * to ensure that it is noticed.
> @@ -1324,14 +1329,16 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n)
>  	if (al == NULL)
>  		return -ECANCELED;
>  
> +	if (n == NULL)
> +		n = &extn;
>  	rgd = ip->i_rgd;
>  
> -	if (rgrp_contains_block(rgd, ip->i_goal))
> +	if (!dinode && rgrp_contains_block(rgd, ip->i_goal))
>  		goal = ip->i_goal - rgd->rd_data0;
>  	else
>  		goal = rgd->rd_last_alloc;
>  
> -	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, GFS2_BLKST_USED, n);
> +	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, blk_type, n);
>  
>  	/* Since all blocks are reserved in advance, this shouldn't happen */
>  	if (blk == BFITNOENT)
> @@ -1339,82 +1346,43 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n)
>  
>  	rgd->rd_last_alloc = blk;
>  	block = rgd->rd_data0 + blk;
> -	ip->i_goal = block + *n - 1;
> -	error = gfs2_meta_inode_buffer(ip, &dibh);
> -	if (error == 0) {
> -		struct gfs2_dinode *di = (struct gfs2_dinode *)dibh->b_data;
> -		gfs2_trans_add_bh(ip->i_gl, dibh, 1);
> -		di->di_goal_meta = di->di_goal_data = cpu_to_be64(ip->i_goal);
> -		brelse(dibh);
> +	if (!dinode) {
> +		ip->i_goal = block + *n - 1;
> +		error = gfs2_meta_inode_buffer(ip, &dibh);
> +		if (error == 0) {
> +			struct gfs2_dinode *di =
> +				(struct gfs2_dinode *)dibh->b_data;
> +			gfs2_trans_add_bh(ip->i_gl, dibh, 1);
> +			di->di_goal_meta = di->di_goal_data =
> +				cpu_to_be64(ip->i_goal);
> +			brelse(dibh);
> +		}
>  	}
>  	if (rgd->rd_free < *n)
>  		goto rgrp_error;
>  
>  	rgd->rd_free -= *n;
> +	if (dinode) {
> +		rgd->rd_dinodes++;
> +		*generation = rgd->rd_igeneration++;
> +		if (*generation == 0)
> +			*generation = rgd->rd_igeneration++;
> +	}
>  
>  	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);
>  
>  	al->al_alloced += *n;
>  
> -	gfs2_statfs_change(sdp, 0, -(s64)*n, 0);
> -	gfs2_quota_change(ip, *n, ip->i_inode.i_uid, ip->i_inode.i_gid);
> +	gfs2_statfs_change(sdp, 0, -(s64)*n, dinode ? 1 : 0);
> +	if (dinode)
> +		gfs2_trans_add_unrevoke(sdp, block, 1);
> +	else
> +		gfs2_quota_change(ip, *n, ip->i_inode.i_uid,
> +				  ip->i_inode.i_gid);
>  
>  	rgd->rd_free_clone -= *n;
> -	trace_gfs2_block_alloc(ip, block, *n, GFS2_BLKST_USED);
> -	*bn = block;
> -	return 0;
> -
> -rgrp_error:
> -	gfs2_rgrp_error(rgd);
> -	return -EIO;
> -}
> -
> -/**
> - * gfs2_alloc_di - Allocate a dinode
> - * @dip: the directory that the inode is going in
> - * @bn: the block number which is allocated
> - * @generation: the generation number of the inode
> - *
> - * Returns: 0 on success or error
> - */
> -
> -int gfs2_alloc_di(struct gfs2_inode *dip, u64 *bn, u64 *generation)
> -{
> -	struct gfs2_sbd *sdp = GFS2_SB(&dip->i_inode);
> -	struct gfs2_alloc *al = dip->i_alloc;
> -	struct gfs2_rgrpd *rgd = dip->i_rgd;
> -	u32 blk;
> -	u64 block;
> -	unsigned int n = 1;
> -
> -	blk = rgblk_search(rgd, rgd->rd_last_alloc,
> -			   GFS2_BLKST_FREE, GFS2_BLKST_DINODE, &n);
> -
> -	/* Since all blocks are reserved in advance, this shouldn't happen */
> -	if (blk == BFITNOENT)
> -		goto rgrp_error;
> -
> -	rgd->rd_last_alloc = blk;
> -	block = rgd->rd_data0 + blk;
> -	if (rgd->rd_free == 0)
> -		goto rgrp_error;
> -
> -	rgd->rd_free--;
> -	rgd->rd_dinodes++;
> -	*generation = rgd->rd_igeneration++;
> -	if (*generation == 0)
> -		*generation = rgd->rd_igeneration++;
> -	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);
> -
> -	al->al_alloced++;
> -
> -	gfs2_statfs_change(sdp, 0, -1, +1);
> -	gfs2_trans_add_unrevoke(sdp, block, 1);
> -
> -	rgd->rd_free_clone--;
> -	trace_gfs2_block_alloc(dip, block, 1, GFS2_BLKST_DINODE);
> +	trace_gfs2_block_alloc(ip, block, *n, blk_type);
>  	*bn = block;
>  	return 0;
>  
> diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
> index cf5c501..4cb5608 100644
> --- a/fs/gfs2/rgrp.h
> +++ b/fs/gfs2/rgrp.h
> @@ -39,8 +39,8 @@ static inline void gfs2_alloc_put(struct gfs2_inode *ip)
>  extern int gfs2_inplace_reserve(struct gfs2_inode *ip);
>  extern void gfs2_inplace_release(struct gfs2_inode *ip);
>  
> -extern int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n);
> -extern int gfs2_alloc_di(struct gfs2_inode *ip, u64 *bn, u64 *generation);
> +extern int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
> +			    int dinode, u64 *generation);
>  
>  extern void __gfs2_free_blocks(struct gfs2_inode *ip, u64 bstart, u32 blen, int meta);
>  extern void gfs2_free_meta(struct gfs2_inode *ip, u64 bstart, u32 blen);
> diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
> index a201a1d..e4794a5 100644
> --- a/fs/gfs2/xattr.c
> +++ b/fs/gfs2/xattr.c
> @@ -610,7 +610,7 @@ static int ea_alloc_blk(struct gfs2_inode *ip, struct buffer_head **bhp)
>  	u64 block;
>  	int error;
>  
> -	error = gfs2_alloc_block(ip, &block, &n);
> +	error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
>  	if (error)
>  		return error;
>  	gfs2_trans_add_unrevoke(sdp, block, 1);
> @@ -672,7 +672,7 @@ static int ea_write(struct gfs2_inode *ip, struct gfs2_ea_header *ea,
>  			int mh_size = sizeof(struct gfs2_meta_header);
>  			unsigned int n = 1;
>  
> -			error = gfs2_alloc_block(ip, &block, &n);
> +			error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
>  			if (error)
>  				return error;
>  			gfs2_trans_add_unrevoke(sdp, block, 1);
> @@ -992,7 +992,7 @@ static int ea_set_block(struct gfs2_inode *ip, struct gfs2_ea_request *er,
>  	} else {
>  		u64 blk;
>  		unsigned int n = 1;
> -		error = gfs2_alloc_block(ip, &blk, &n);
> +		error = gfs2_alloc_block(ip, &blk, &n, 0, NULL);
>  		if (error)
>  			return error;
>  		gfs2_trans_add_unrevoke(sdp, blk, 1);
> 

[Cluster-devel] [GFS2 PATCH] GFS2: combine gfs2_alloc_block and gfs2_alloc_di

[Steven Whitehouse]

Hi,

As a follow up to this patch, I'd quite like to see rgblk_search split
into two functions. One to do the search and another to actually make
changes to the bitmap when a block is found. That should help
development of further alloc changes on top.

Also, if someone requests allocation of an extent > 1 block with an
inode type, then only the first block should be set to being an inode,
the others should be set to "used" and the various stats should be
updated accordingly. Otherwise it will never be possible to ask for more
than a single block when an inode is being allocated,

Steve.

On Mon, 2011-11-14 at 11:17 -0500, Bob Peterson wrote:
> Hi,
> 
> GFS2 functions gfs2_alloc_block and gfs2_alloc_di do basically
> the same things, with a few exceptions. This patch combines
> the two functions into a slightly more generic gfs2_alloc_block.
> Having one centralized block allocation function will reduce
> code redundancy and make it easier to implement multi-block
> reservations to reduce file fragmentation in the future.
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> --
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index f6be14f..b69235b 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -133,7 +133,7 @@ int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
>  		   and write it out to disk */
>  
>  		unsigned int n = 1;
> -		error = gfs2_alloc_block(ip, &block, &n);
> +		error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
>  		if (error)
>  			goto out_brelse;
>  		if (isdir) {
> @@ -503,7 +503,7 @@ static int gfs2_bmap_alloc(struct inode *inode, const sector_t lblock,
>  	do {
>  		int error;
>  		n = blks - alloced;
> -		error = gfs2_alloc_block(ip, &bn, &n);
> +		error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
>  		if (error)
>  			return error;
>  		alloced += n;
> diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
> index 946b6f8..ae75319 100644
> --- a/fs/gfs2/dir.c
> +++ b/fs/gfs2/dir.c
> @@ -823,7 +823,7 @@ static struct gfs2_leaf *new_leaf(struct inode *inode, struct buffer_head **pbh,
>  	struct gfs2_dirent *dent;
>  	struct qstr name = { .name = "", .len = 0, .hash = 0 };
>  
> -	error = gfs2_alloc_block(ip, &bn, &n);
> +	error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
>  	if (error)
>  		return NULL;
>  	bh = gfs2_meta_new(ip->i_gl, bn);
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 377920d..de2668f 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -402,7 +402,7 @@ static int alloc_dinode(struct gfs2_inode *dip, u64 *no_addr, u64 *generation)
>  	if (error)
>  		goto out_ipreserv;
>  
> -	error = gfs2_alloc_di(dip, no_addr, generation);
> +	error = gfs2_alloc_block(dip, no_addr, NULL, 1, generation);
>  
>  	gfs2_trans_end(sdp);
>  
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index a1a815b..995f4e6 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -1304,19 +1304,24 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
>   * @ip: the inode to allocate the block for
>   * @bn: Used to return the starting block number
>   * @n: requested number of blocks/extent length (value/result)
> + * dinode: 1 if we're allocating a dinode, 0 if it's a data block
> + * @generation: the generation number of the inode
>   *
>   * Returns: 0 or error
>   */
>  
> -int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n)
> +int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
> +		     int dinode, u64 *generation)
>  {
>  	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
>  	struct buffer_head *dibh;
>  	struct gfs2_alloc *al = ip->i_alloc;
>  	struct gfs2_rgrpd *rgd;
> -	u32 goal, blk;
> -	u64 block;
> +	u32 goal, blk; /* block, within the rgrp scope */
> +	u64 block; /* block, within the file system scope */
> +	unsigned int extn = 1;
>  	int error;
> +	unsigned char blk_type = dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED;
>  
>  	/* Only happens if there is a bug in gfs2, return something distinctive
>  	 * to ensure that it is noticed.
> @@ -1324,14 +1329,16 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n)
>  	if (al == NULL)
>  		return -ECANCELED;
>  
> +	if (n == NULL)
> +		n = &extn;
>  	rgd = ip->i_rgd;
>  
> -	if (rgrp_contains_block(rgd, ip->i_goal))
> +	if (!dinode && rgrp_contains_block(rgd, ip->i_goal))
>  		goal = ip->i_goal - rgd->rd_data0;
>  	else
>  		goal = rgd->rd_last_alloc;
>  
> -	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, GFS2_BLKST_USED, n);
> +	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, blk_type, n);
>  
>  	/* Since all blocks are reserved in advance, this shouldn't happen */
>  	if (blk == BFITNOENT)
> @@ -1339,82 +1346,43 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n)
>  
>  	rgd->rd_last_alloc = blk;
>  	block = rgd->rd_data0 + blk;
> -	ip->i_goal = block + *n - 1;
> -	error = gfs2_meta_inode_buffer(ip, &dibh);
> -	if (error == 0) {
> -		struct gfs2_dinode *di = (struct gfs2_dinode *)dibh->b_data;
> -		gfs2_trans_add_bh(ip->i_gl, dibh, 1);
> -		di->di_goal_meta = di->di_goal_data = cpu_to_be64(ip->i_goal);
> -		brelse(dibh);
> +	if (!dinode) {
> +		ip->i_goal = block + *n - 1;
> +		error = gfs2_meta_inode_buffer(ip, &dibh);
> +		if (error == 0) {
> +			struct gfs2_dinode *di =
> +				(struct gfs2_dinode *)dibh->b_data;
> +			gfs2_trans_add_bh(ip->i_gl, dibh, 1);
> +			di->di_goal_meta = di->di_goal_data =
> +				cpu_to_be64(ip->i_goal);
> +			brelse(dibh);
> +		}
>  	}
>  	if (rgd->rd_free < *n)
>  		goto rgrp_error;
>  
>  	rgd->rd_free -= *n;
> +	if (dinode) {
> +		rgd->rd_dinodes++;
> +		*generation = rgd->rd_igeneration++;
> +		if (*generation == 0)
> +			*generation = rgd->rd_igeneration++;
> +	}
>  
>  	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);
>  
>  	al->al_alloced += *n;
>  
> -	gfs2_statfs_change(sdp, 0, -(s64)*n, 0);
> -	gfs2_quota_change(ip, *n, ip->i_inode.i_uid, ip->i_inode.i_gid);
> +	gfs2_statfs_change(sdp, 0, -(s64)*n, dinode ? 1 : 0);
> +	if (dinode)
> +		gfs2_trans_add_unrevoke(sdp, block, 1);
> +	else
> +		gfs2_quota_change(ip, *n, ip->i_inode.i_uid,
> +				  ip->i_inode.i_gid);
>  
>  	rgd->rd_free_clone -= *n;
> -	trace_gfs2_block_alloc(ip, block, *n, GFS2_BLKST_USED);
> -	*bn = block;
> -	return 0;
> -
> -rgrp_error:
> -	gfs2_rgrp_error(rgd);
> -	return -EIO;
> -}
> -
> -/**
> - * gfs2_alloc_di - Allocate a dinode
> - * @dip: the directory that the inode is going in
> - * @bn: the block number which is allocated
> - * @generation: the generation number of the inode
> - *
> - * Returns: 0 on success or error
> - */
> -
> -int gfs2_alloc_di(struct gfs2_inode *dip, u64 *bn, u64 *generation)
> -{
> -	struct gfs2_sbd *sdp = GFS2_SB(&dip->i_inode);
> -	struct gfs2_alloc *al = dip->i_alloc;
> -	struct gfs2_rgrpd *rgd = dip->i_rgd;
> -	u32 blk;
> -	u64 block;
> -	unsigned int n = 1;
> -
> -	blk = rgblk_search(rgd, rgd->rd_last_alloc,
> -			   GFS2_BLKST_FREE, GFS2_BLKST_DINODE, &n);
> -
> -	/* Since all blocks are reserved in advance, this shouldn't happen */
> -	if (blk == BFITNOENT)
> -		goto rgrp_error;
> -
> -	rgd->rd_last_alloc = blk;
> -	block = rgd->rd_data0 + blk;
> -	if (rgd->rd_free == 0)
> -		goto rgrp_error;
> -
> -	rgd->rd_free--;
> -	rgd->rd_dinodes++;
> -	*generation = rgd->rd_igeneration++;
> -	if (*generation == 0)
> -		*generation = rgd->rd_igeneration++;
> -	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);
> -
> -	al->al_alloced++;
> -
> -	gfs2_statfs_change(sdp, 0, -1, +1);
> -	gfs2_trans_add_unrevoke(sdp, block, 1);
> -
> -	rgd->rd_free_clone--;
> -	trace_gfs2_block_alloc(dip, block, 1, GFS2_BLKST_DINODE);
> +	trace_gfs2_block_alloc(ip, block, *n, blk_type);
>  	*bn = block;
>  	return 0;
>  
> diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
> index cf5c501..4cb5608 100644
> --- a/fs/gfs2/rgrp.h
> +++ b/fs/gfs2/rgrp.h
> @@ -39,8 +39,8 @@ static inline void gfs2_alloc_put(struct gfs2_inode *ip)
>  extern int gfs2_inplace_reserve(struct gfs2_inode *ip);
>  extern void gfs2_inplace_release(struct gfs2_inode *ip);
>  
> -extern int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n);
> -extern int gfs2_alloc_di(struct gfs2_inode *ip, u64 *bn, u64 *generation);
> +extern int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
> +			    int dinode, u64 *generation);
>  
>  extern void __gfs2_free_blocks(struct gfs2_inode *ip, u64 bstart, u32 blen, int meta);
>  extern void gfs2_free_meta(struct gfs2_inode *ip, u64 bstart, u32 blen);
> diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
> index a201a1d..e4794a5 100644
> --- a/fs/gfs2/xattr.c
> +++ b/fs/gfs2/xattr.c
> @@ -610,7 +610,7 @@ static int ea_alloc_blk(struct gfs2_inode *ip, struct buffer_head **bhp)
>  	u64 block;
>  	int error;
>  
> -	error = gfs2_alloc_block(ip, &block, &n);
> +	error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
>  	if (error)
>  		return error;
>  	gfs2_trans_add_unrevoke(sdp, block, 1);
> @@ -672,7 +672,7 @@ static int ea_write(struct gfs2_inode *ip, struct gfs2_ea_header *ea,
>  			int mh_size = sizeof(struct gfs2_meta_header);
>  			unsigned int n = 1;
>  
> -			error = gfs2_alloc_block(ip, &block, &n);
> +			error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
>  			if (error)
>  				return error;
>  			gfs2_trans_add_unrevoke(sdp, block, 1);
> @@ -992,7 +992,7 @@ static int ea_set_block(struct gfs2_inode *ip, struct gfs2_ea_request *er,
>  	} else {
>  		u64 blk;
>  		unsigned int n = 1;
> -		error = gfs2_alloc_block(ip, &blk, &n);
> +		error = gfs2_alloc_block(ip, &blk, &n, 0, NULL);
>  		if (error)
>  			return error;
>  		gfs2_trans_add_unrevoke(sdp, blk, 1);
> 

[Cluster-devel] [GFS2 PATCH] GFS2: combine gfs2_alloc_block and gfs2_alloc_di

[Bob Peterson]

----- Original Message -----
| Hi,
| 
| As a follow up to this patch, I'd quite like to see rgblk_search
| split
| into two functions. One to do the search and another to actually make
| changes to the bitmap when a block is found. That should help
| development of further alloc changes on top.
| 
| Also, if someone requests allocation of an extent > 1 block with an
| inode type, then only the first block should be set to being an
| inode,
| the others should be set to "used" and the various stats should be
| updated accordingly. Otherwise it will never be possible to ask for
| more
| than a single block when an inode is being allocated,
| 
| Steve.

Hi,

Good suggestions. Hopefully I'll post a patch or two soon.
In the meantime, here's a clean-up for one of the patches.
I should have done this in the first place.

Regards,

Bob Peterson
Red Hat File Systems
--
commit e5091d2b20b98424cb6924ec499bdf5c5bc5827d
Author: Bob Peterson <rpeterso@redhat.com>
Date:   Wed Nov 16 08:17:33 2011 -0600

    GFS2: Clean up gfs2_block_alloc by passing in block type
    
    This patch simplifies function gfs2_block_alloc by making its
    callers pass in the requested block type rather than a flag to be
    translated as a block type.

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index b69235b..491e1f2 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -133,7 +133,7 @@ int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
 		   and write it out to disk */
 
 		unsigned int n = 1;
-		error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
+		error = gfs2_alloc_block(ip, &block, &n, GFS2_BLKST_USED, NULL);
 		if (error)
 			goto out_brelse;
 		if (isdir) {
@@ -503,7 +503,7 @@ static int gfs2_bmap_alloc(struct inode *inode, const sector_t lblock,
 	do {
 		int error;
 		n = blks - alloced;
-		error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
+		error = gfs2_alloc_block(ip, &bn, &n, GFS2_BLKST_USED, NULL);
 		if (error)
 			return error;
 		alloced += n;
diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index ae75319..c38b32c 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -823,7 +823,7 @@ static struct gfs2_leaf *new_leaf(struct inode *inode, struct buffer_head **pbh,
 	struct gfs2_dirent *dent;
 	struct qstr name = { .name = "", .len = 0, .hash = 0 };
 
-	error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
+	error = gfs2_alloc_block(ip, &bn, &n, GFS2_BLKST_USED, NULL);
 	if (error)
 		return NULL;
 	bh = gfs2_meta_new(ip->i_gl, bn);
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index de2668f..25b6657 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -402,7 +402,8 @@ static int alloc_dinode(struct gfs2_inode *dip, u64 *no_addr, u64 *generation)
 	if (error)
 		goto out_ipreserv;
 
-	error = gfs2_alloc_block(dip, no_addr, NULL, 1, generation);
+	error = gfs2_alloc_block(dip, no_addr, NULL, GFS2_BLKST_DINODE,
+				 generation);
 
 	gfs2_trans_end(sdp);
 
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 995f4e6..abac4bd 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1311,7 +1311,7 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
  */
 
 int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
-		     int dinode, u64 *generation)
+		     unsigned char blk_type, u64 *generation)
 {
 	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
 	struct buffer_head *dibh;
@@ -1321,7 +1321,6 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 	u64 block; /* block, within the file system scope */
 	unsigned int extn = 1;
 	int error;
-	unsigned char blk_type = dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED;
 
 	/* Only happens if there is a bug in gfs2, return something distinctive
 	 * to ensure that it is noticed.
@@ -1333,7 +1332,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 		n = &extn;
 	rgd = ip->i_rgd;
 
-	if (!dinode && rgrp_contains_block(rgd, ip->i_goal))
+	if (blk_type == GFS2_BLKST_USED && rgrp_contains_block(rgd, ip->i_goal))
 		goal = ip->i_goal - rgd->rd_data0;
 	else
 		goal = rgd->rd_last_alloc;
@@ -1346,7 +1345,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 
 	rgd->rd_last_alloc = blk;
 	block = rgd->rd_data0 + blk;
-	if (!dinode) {
+	if (blk_type == GFS2_BLKST_USED) {
 		ip->i_goal = block + *n - 1;
 		error = gfs2_meta_inode_buffer(ip, &dibh);
 		if (error == 0) {
@@ -1362,7 +1361,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 		goto rgrp_error;
 
 	rgd->rd_free -= *n;
-	if (dinode) {
+	if (blk_type == GFS2_BLKST_DINODE) {
 		rgd->rd_dinodes++;
 		*generation = rgd->rd_igeneration++;
 		if (*generation == 0)
@@ -1374,8 +1373,8 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 
 	al->al_alloced += *n;
 
-	gfs2_statfs_change(sdp, 0, -(s64)*n, dinode ? 1 : 0);
-	if (dinode)
+	gfs2_statfs_change(sdp, 0, -(s64)*n, (blk_type == GFS2_BLKST_DINODE));
+	if (blk_type == GFS2_BLKST_DINODE)
 		gfs2_trans_add_unrevoke(sdp, block, 1);
 	else
 		gfs2_quota_change(ip, *n, ip->i_inode.i_uid,
diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
index 4cb5608..6a81011 100644
--- a/fs/gfs2/rgrp.h
+++ b/fs/gfs2/rgrp.h
@@ -40,7 +40,7 @@ extern int gfs2_inplace_reserve(struct gfs2_inode *ip);
 extern void gfs2_inplace_release(struct gfs2_inode *ip);
 
 extern int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
-			    int dinode, u64 *generation);
+			    unsigned char blk_type, u64 *generation);
 
 extern void __gfs2_free_blocks(struct gfs2_inode *ip, u64 bstart, u32 blen, int meta);
 extern void gfs2_free_meta(struct gfs2_inode *ip, u64 bstart, u32 blen);
diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
index e4794a5..f2b77b4 100644
--- a/fs/gfs2/xattr.c
+++ b/fs/gfs2/xattr.c
@@ -610,7 +610,7 @@ static int ea_alloc_blk(struct gfs2_inode *ip, struct buffer_head **bhp)
 	u64 block;
 	int error;
 
-	error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
+	error = gfs2_alloc_block(ip, &block, &n, GFS2_BLKST_USED, NULL);
 	if (error)
 		return error;
 	gfs2_trans_add_unrevoke(sdp, block, 1);
@@ -672,7 +672,8 @@ static int ea_write(struct gfs2_inode *ip, struct gfs2_ea_header *ea,
 			int mh_size = sizeof(struct gfs2_meta_header);
 			unsigned int n = 1;
 
-			error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
+			error = gfs2_alloc_block(ip, &block, &n,
+						 GFS2_BLKST_USED, NULL);
 			if (error)
 				return error;
 			gfs2_trans_add_unrevoke(sdp, block, 1);
@@ -992,7 +993,7 @@ static int ea_set_block(struct gfs2_inode *ip, struct gfs2_ea_request *er,
 	} else {
 		u64 blk;
 		unsigned int n = 1;
-		error = gfs2_alloc_block(ip, &blk, &n, 0, NULL);
+		error = gfs2_alloc_block(ip, &blk, &n, GFS2_BLKST_USED, NULL);
 		if (error)
 			return error;
 		gfs2_trans_add_unrevoke(sdp, blk, 1);

[Cluster-devel] [GFS2 PATCH] GFS2: combine gfs2_alloc_block and gfs2_alloc_di

[Steven Whitehouse]

Hi,

On Wed, 2011-11-16 at 09:18 -0500, Bob Peterson wrote:
> ----- Original Message -----
> | Hi,
> | 
> | As a follow up to this patch, I'd quite like to see rgblk_search
> | split
> | into two functions. One to do the search and another to actually make
> | changes to the bitmap when a block is found. That should help
> | development of further alloc changes on top.
> | 
> | Also, if someone requests allocation of an extent > 1 block with an
> | inode type, then only the first block should be set to being an
> | inode,
> | the others should be set to "used" and the various stats should be
> | updated accordingly. Otherwise it will never be possible to ask for
> | more
> | than a single block when an inode is being allocated,
> | 
> | Steve.
> 
> Hi,
> 
> Good suggestions. Hopefully I'll post a patch or two soon.
> In the meantime, here's a clean-up for one of the patches.
> I should have done this in the first place.
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> --
> commit e5091d2b20b98424cb6924ec499bdf5c5bc5827d
> Author: Bob Peterson <rpeterso@redhat.com>
> Date:   Wed Nov 16 08:17:33 2011 -0600
> 
>     GFS2: Clean up gfs2_block_alloc by passing in block type
>     
>     This patch simplifies function gfs2_block_alloc by making its
>     callers pass in the requested block type rather than a flag to be
>     translated as a block type.
> 
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index b69235b..491e1f2 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -133,7 +133,7 @@ int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
>  		   and write it out to disk */
>  
>  		unsigned int n = 1;
> -		error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
> +		error = gfs2_alloc_block(ip, &block, &n, GFS2_BLKST_USED, NULL);
>  		if (error)
>  			goto out_brelse;
>  		if (isdir) {
> @@ -503,7 +503,7 @@ static int gfs2_bmap_alloc(struct inode *inode, const sector_t lblock,
>  	do {
>  		int error;
>  		n = blks - alloced;
> -		error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
> +		error = gfs2_alloc_block(ip, &bn, &n, GFS2_BLKST_USED, NULL);
>  		if (error)
>  			return error;
>  		alloced += n;
> diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
> index ae75319..c38b32c 100644
> --- a/fs/gfs2/dir.c
> +++ b/fs/gfs2/dir.c
> @@ -823,7 +823,7 @@ static struct gfs2_leaf *new_leaf(struct inode *inode, struct buffer_head **pbh,
>  	struct gfs2_dirent *dent;
>  	struct qstr name = { .name = "", .len = 0, .hash = 0 };
>  
> -	error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
> +	error = gfs2_alloc_block(ip, &bn, &n, GFS2_BLKST_USED, NULL);
>  	if (error)
>  		return NULL;
>  	bh = gfs2_meta_new(ip->i_gl, bn);
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index de2668f..25b6657 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -402,7 +402,8 @@ static int alloc_dinode(struct gfs2_inode *dip, u64 *no_addr, u64 *generation)
>  	if (error)
>  		goto out_ipreserv;
>  
> -	error = gfs2_alloc_block(dip, no_addr, NULL, 1, generation);
> +	error = gfs2_alloc_block(dip, no_addr, NULL, GFS2_BLKST_DINODE,
> +				 generation);
>  
>  	gfs2_trans_end(sdp);
>  
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index 995f4e6..abac4bd 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -1311,7 +1311,7 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
>   */
>  
>  int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
> -		     int dinode, u64 *generation)
> +		     unsigned char blk_type, u64 *generation)
>  {
>  	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
>  	struct buffer_head *dibh;
> @@ -1321,7 +1321,6 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  	u64 block; /* block, within the file system scope */
>  	unsigned int extn = 1;
>  	int error;
> -	unsigned char blk_type = dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED;
>  
>  	/* Only happens if there is a bug in gfs2, return something distinctive
>  	 * to ensure that it is noticed.
> @@ -1333,7 +1332,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  		n = &extn;
>  	rgd = ip->i_rgd;
>  
> -	if (!dinode && rgrp_contains_block(rgd, ip->i_goal))
> +	if (blk_type == GFS2_BLKST_USED && rgrp_contains_block(rgd, ip->i_goal))
>  		goal = ip->i_goal - rgd->rd_data0;
>  	else
>  		goal = rgd->rd_last_alloc;
> @@ -1346,7 +1345,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  
>  	rgd->rd_last_alloc = blk;
>  	block = rgd->rd_data0 + blk;
> -	if (!dinode) {
> +	if (blk_type == GFS2_BLKST_USED) {
>  		ip->i_goal = block + *n - 1;
>  		error = gfs2_meta_inode_buffer(ip, &dibh);
>  		if (error == 0) {
> @@ -1362,7 +1361,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  		goto rgrp_error;
>  
>  	rgd->rd_free -= *n;
> -	if (dinode) {
> +	if (blk_type == GFS2_BLKST_DINODE) {
>  		rgd->rd_dinodes++;
>  		*generation = rgd->rd_igeneration++;
>  		if (*generation == 0)
> @@ -1374,8 +1373,8 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  
>  	al->al_alloced += *n;
>  
> -	gfs2_statfs_change(sdp, 0, -(s64)*n, dinode ? 1 : 0);
> -	if (dinode)
> +	gfs2_statfs_change(sdp, 0, -(s64)*n, (blk_type == GFS2_BLKST_DINODE));
It is not a good plan to rely on this to give us 0 and 1 inode counts.
Using the ?: as before would be better. I suspect that if you maintain
separate counts of inodes (which can be 0 or 1 only) and other blocks
(any number) calculated at the start of the function then a number of
the tests you need to do will just become 0 or non-zero on one or other
other of these.

Also, it would be a good plan to always pass the extent size in, even if
the caller is requesting only a single block, then there is no need for
a NULL test,

Steve.


> +	if (blk_type == GFS2_BLKST_DINODE)
>  		gfs2_trans_add_unrevoke(sdp, block, 1);
>  	else
>  		gfs2_quota_change(ip, *n, ip->i_inode.i_uid,
> diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
> index 4cb5608..6a81011 100644
> --- a/fs/gfs2/rgrp.h
> +++ b/fs/gfs2/rgrp.h
> @@ -40,7 +40,7 @@ extern int gfs2_inplace_reserve(struct gfs2_inode *ip);
>  extern void gfs2_inplace_release(struct gfs2_inode *ip);
>  
>  extern int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
> -			    int dinode, u64 *generation);
> +			    unsigned char blk_type, u64 *generation);
>  
>  extern void __gfs2_free_blocks(struct gfs2_inode *ip, u64 bstart, u32 blen, int meta);
>  extern void gfs2_free_meta(struct gfs2_inode *ip, u64 bstart, u32 blen);
> diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
> index e4794a5..f2b77b4 100644
> --- a/fs/gfs2/xattr.c
> +++ b/fs/gfs2/xattr.c
> @@ -610,7 +610,7 @@ static int ea_alloc_blk(struct gfs2_inode *ip, struct buffer_head **bhp)
>  	u64 block;
>  	int error;
>  
> -	error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
> +	error = gfs2_alloc_block(ip, &block, &n, GFS2_BLKST_USED, NULL);
>  	if (error)
>  		return error;
>  	gfs2_trans_add_unrevoke(sdp, block, 1);
> @@ -672,7 +672,8 @@ static int ea_write(struct gfs2_inode *ip, struct gfs2_ea_header *ea,
>  			int mh_size = sizeof(struct gfs2_meta_header);
>  			unsigned int n = 1;
>  
> -			error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
> +			error = gfs2_alloc_block(ip, &block, &n,
> +						 GFS2_BLKST_USED, NULL);
>  			if (error)
>  				return error;
>  			gfs2_trans_add_unrevoke(sdp, block, 1);
> @@ -992,7 +993,7 @@ static int ea_set_block(struct gfs2_inode *ip, struct gfs2_ea_request *er,
>  	} else {
>  		u64 blk;
>  		unsigned int n = 1;
> -		error = gfs2_alloc_block(ip, &blk, &n, 0, NULL);
> +		error = gfs2_alloc_block(ip, &blk, &n, GFS2_BLKST_USED, NULL);
>  		if (error)
>  			return error;
>  		gfs2_trans_add_unrevoke(sdp, blk, 1);

[Cluster-devel] [GFS2 PATCH] GFS2: combine gfs2_alloc_block and gfs2_alloc_di

[Bob Peterson]

----- Original Message -----
| It is not a good plan to rely on this to give us 0 and 1 inode
| counts.
| Using the ?: as before would be better. I suspect that if you
| maintain
| separate counts of inodes (which can be 0 or 1 only) and other blocks
| (any number) calculated at the start of the function then a number of
| the tests you need to do will just become 0 or non-zero on one or
| other
| other of these.
| 
| Also, it would be a good plan to always pass the extent size in, even
| if
| the caller is requesting only a single block, then there is no need
| for
| a NULL test,
| 
| Steve.

Hi,

Okay, scratch that then. How about something like the following patch?

This patch moves more toward a generic multi-block allocator that
takes a pointer to the number of data blocks to allocate, plus whether
or not to allocate a dinode. In theory, it could be called to allocate
(1) a single dinode block, (2) a group of one or more data blocks, or
(3) a dinode plus several data blocks.

Regards,

Bob Peterson
Red Hat File Systems
--
 fs/gfs2/bmap.c  |    4 +-
 fs/gfs2/dir.c   |    2 +-
 fs/gfs2/inode.c |    3 +-
 fs/gfs2/rgrp.c  |   58 +++++++++++++++++++++++++++++-------------------------
 fs/gfs2/rgrp.h  |    4 +-
 fs/gfs2/xattr.c |    6 ++--
 6 files changed, 41 insertions(+), 36 deletions(-)

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index b69235b..cb74312 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -133,7 +133,7 @@ int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
 		   and write it out to disk */
 
 		unsigned int n = 1;
-		error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
+		error = gfs2_alloc_blocks(ip, &block, &n, 0, NULL);
 		if (error)
 			goto out_brelse;
 		if (isdir) {
@@ -503,7 +503,7 @@ static int gfs2_bmap_alloc(struct inode *inode, const sector_t lblock,
 	do {
 		int error;
 		n = blks - alloced;
-		error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
+		error = gfs2_alloc_blocks(ip, &bn, &n, 0, NULL);
 		if (error)
 			return error;
 		alloced += n;
diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index ae75319..f8485da 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -823,7 +823,7 @@ static struct gfs2_leaf *new_leaf(struct inode *inode, struct buffer_head **pbh,
 	struct gfs2_dirent *dent;
 	struct qstr name = { .name = "", .len = 0, .hash = 0 };
 
-	error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
+	error = gfs2_alloc_blocks(ip, &bn, &n, 0, NULL);
 	if (error)
 		return NULL;
 	bh = gfs2_meta_new(ip->i_gl, bn);
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index de2668f..3ab192b 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -389,6 +389,7 @@ static int alloc_dinode(struct gfs2_inode *dip, u64 *no_addr, u64 *generation)
 {
 	struct gfs2_sbd *sdp = GFS2_SB(&dip->i_inode);
 	int error;
+	int dblocks = 0;
 
 	if (gfs2_alloc_get(dip) == NULL)
 		return -ENOMEM;
@@ -402,7 +403,7 @@ static int alloc_dinode(struct gfs2_inode *dip, u64 *no_addr, u64 *generation)
 	if (error)
 		goto out_ipreserv;
 
-	error = gfs2_alloc_block(dip, no_addr, NULL, 1, generation);
+	error = gfs2_alloc_blocks(dip, no_addr, &dblocks, 1, generation);
 
 	gfs2_trans_end(sdp);
 
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 995f4e6..131289b 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -65,8 +65,8 @@ static const char valid_change[16] = {
 };
 
 static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
-                        unsigned char old_state, unsigned char new_state,
-			unsigned int *n);
+                        unsigned char old_state, bool dinode,
+			unsigned int *ndata);
 
 /**
  * gfs2_setbit - Set a bit in the bitmaps
@@ -921,8 +921,7 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skip
 	while (goal < rgd->rd_data) {
 		down_write(&sdp->sd_log_flush_lock);
 		n = 1;
-		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED,
-				     GFS2_BLKST_UNLINKED, &n);
+		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED, 0, &n);
 		up_write(&sdp->sd_log_flush_lock);
 		if (block == BFITNOENT)
 			break;
@@ -1115,7 +1114,7 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
  * @rgd: the resource group descriptor
  * @goal: the goal block within the RG (start here to search for avail block)
  * @old_state: GFS2_BLKST_XXX the before-allocation state to find
- * @new_state: GFS2_BLKST_XXX the after-allocation block state
+ * dinode: TRUE if the first block we allocate is for a dinode
  * @n: The extent length
  *
  * Walk rgrp's bitmap to find bits that represent a block in @old_state.
@@ -1132,8 +1131,7 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
  */
 
 static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
-			unsigned char old_state, unsigned char new_state,
-			unsigned int *n)
+			unsigned char old_state, bool dinode, unsigned int *n)
 {
 	struct gfs2_bitmap *bi = NULL;
 	const u32 length = rgd->rd_length;
@@ -1141,7 +1139,14 @@ static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
 	unsigned int buf, x;
 	const unsigned int elen = *n;
 	const u8 *buffer = NULL;
+	unsigned char new_state;
 
+	if (old_state == GFS2_BLKST_UNLINKED)
+		new_state = GFS2_BLKST_UNLINKED;
+	else if (dinode)
+		new_state = GFS2_BLKST_DINODE;
+	else
+		new_state = GFS2_BLKST_USED;
 	*n = 0;
 	/* Find bitmap block that contains bits for goal block */
 	for (buf = 0; buf < length; buf++) {
@@ -1192,13 +1197,15 @@ skip:
 	if (blk == BFITNOENT)
 		return blk;
 
-	*n = 1;
 	if (old_state == new_state)
 		goto out;
 
 	gfs2_trans_add_bh(rgd->rd_gl, bi->bi_bh, 1);
 	gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
 		    bi, blk, new_state);
+	if (new_state == GFS2_BLKST_USED)
+		*n = 1;
+	new_state = GFS2_BLKST_USED; /* for extents, we need data blocks */
 	goal = blk;
 	while (*n < elen) {
 		goal++;
@@ -1300,18 +1307,18 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
 }
 
 /**
- * gfs2_alloc_block - Allocate one or more blocks
+ * gfs2_alloc_blocks - Allocate one or more blocks of data and/or a dinode
  * @ip: the inode to allocate the block for
  * @bn: Used to return the starting block number
- * @n: requested number of blocks/extent length (value/result)
- * dinode: 1 if we're allocating a dinode, 0 if it's a data block
+ * @ndata: requested number of data blocks/extent length (value/result)
+ * dinode: 1 if we're allocating a dinode block, else 0
  * @generation: the generation number of the inode
  *
  * Returns: 0 or error
  */
 
-int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
-		     int dinode, u64 *generation)
+int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *ndata,
+		      bool dinode, u64 *generation)
 {
 	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
 	struct buffer_head *dibh;
@@ -1319,9 +1326,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 	struct gfs2_rgrpd *rgd;
 	u32 goal, blk; /* block, within the rgrp scope */
 	u64 block; /* block, within the file system scope */
-	unsigned int extn = 1;
 	int error;
-	unsigned char blk_type = dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED;
 
 	/* Only happens if there is a bug in gfs2, return something distinctive
 	 * to ensure that it is noticed.
@@ -1329,8 +1334,6 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 	if (al == NULL)
 		return -ECANCELED;
 
-	if (n == NULL)
-		n = &extn;
 	rgd = ip->i_rgd;
 
 	if (!dinode && rgrp_contains_block(rgd, ip->i_goal))
@@ -1338,7 +1341,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 	else
 		goal = rgd->rd_last_alloc;
 
-	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, blk_type, n);
+	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, dinode, ndata);
 
 	/* Since all blocks are reserved in advance, this shouldn't happen */
 	if (blk == BFITNOENT)
@@ -1347,7 +1350,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 	rgd->rd_last_alloc = blk;
 	block = rgd->rd_data0 + blk;
 	if (!dinode) {
-		ip->i_goal = block + *n - 1;
+		ip->i_goal = block + *ndata - 1;
 		error = gfs2_meta_inode_buffer(ip, &dibh);
 		if (error == 0) {
 			struct gfs2_dinode *di =
@@ -1358,11 +1361,12 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 			brelse(dibh);
 		}
 	}
-	if (rgd->rd_free < *n)
+	if (rgd->rd_free < (*ndata) + dinode)
 		goto rgrp_error;
 
-	rgd->rd_free -= *n;
+	rgd->rd_free -= *ndata;
 	if (dinode) {
+		rgd->rd_free--;
 		rgd->rd_dinodes++;
 		*generation = rgd->rd_igeneration++;
 		if (*generation == 0)
@@ -1372,17 +1376,17 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 	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);
 
-	al->al_alloced += *n;
+	al->al_alloced += (*ndata) + dinode;
 
-	gfs2_statfs_change(sdp, 0, -(s64)*n, dinode ? 1 : 0);
+	gfs2_statfs_change(sdp, 0, (-(s64)((*ndata) + dinode)), dinode);
 	if (dinode)
 		gfs2_trans_add_unrevoke(sdp, block, 1);
-	else
-		gfs2_quota_change(ip, *n, ip->i_inode.i_uid,
+	if (*ndata)
+		gfs2_quota_change(ip, *ndata, ip->i_inode.i_uid,
 				  ip->i_inode.i_gid);
 
-	rgd->rd_free_clone -= *n;
-	trace_gfs2_block_alloc(ip, block, *n, blk_type);
+	rgd->rd_free_clone -= (*ndata) + dinode;
+	trace_gfs2_block_alloc(ip, block, *ndata, dinode);
 	*bn = block;
 	return 0;
 
diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
index 4cb5608..b3b61b8 100644
--- a/fs/gfs2/rgrp.h
+++ b/fs/gfs2/rgrp.h
@@ -39,8 +39,8 @@ static inline void gfs2_alloc_put(struct gfs2_inode *ip)
 extern int gfs2_inplace_reserve(struct gfs2_inode *ip);
 extern void gfs2_inplace_release(struct gfs2_inode *ip);
 
-extern int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
-			    int dinode, u64 *generation);
+extern int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
+			     bool dinode, u64 *generation);
 
 extern void __gfs2_free_blocks(struct gfs2_inode *ip, u64 bstart, u32 blen, int meta);
 extern void gfs2_free_meta(struct gfs2_inode *ip, u64 bstart, u32 blen);
diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
index e4794a5..ef74e159 100644
--- a/fs/gfs2/xattr.c
+++ b/fs/gfs2/xattr.c
@@ -610,7 +610,7 @@ static int ea_alloc_blk(struct gfs2_inode *ip, struct buffer_head **bhp)
 	u64 block;
 	int error;
 
-	error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
+	error = gfs2_alloc_blocks(ip, &block, &n, 0, NULL);
 	if (error)
 		return error;
 	gfs2_trans_add_unrevoke(sdp, block, 1);
@@ -672,7 +672,7 @@ static int ea_write(struct gfs2_inode *ip, struct gfs2_ea_header *ea,
 			int mh_size = sizeof(struct gfs2_meta_header);
 			unsigned int n = 1;
 
-			error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
+			error = gfs2_alloc_blocks(ip, &block, &n, 0, NULL);
 			if (error)
 				return error;
 			gfs2_trans_add_unrevoke(sdp, block, 1);
@@ -992,7 +992,7 @@ static int ea_set_block(struct gfs2_inode *ip, struct gfs2_ea_request *er,
 	} else {
 		u64 blk;
 		unsigned int n = 1;
-		error = gfs2_alloc_block(ip, &blk, &n, 0, NULL);
+		error = gfs2_alloc_blocks(ip, &blk, &n, 0, NULL);
 		if (error)
 			return error;
 		gfs2_trans_add_unrevoke(sdp, blk, 1);

[Cluster-devel] [GFS2 PATCH] GFS2: combine gfs2_alloc_block and gfs2_alloc_di

[Bob Peterson]

----- Original Message -----
| Hi,
| 
| As a follow up to this patch, I'd quite like to see rgblk_search
| split
| into two functions. One to do the search and another to actually make
| changes to the bitmap when a block is found. That should help
| development of further alloc changes on top.

Hi,

So something like the patch that follows?

This patch splits function rgblk_search into two functions:
the finding and the bit setting function, now called gfs2_alloc_extent.

Regards,

Bob Peterson
Red Hat File Systems
--
 fs/gfs2/rgrp.c |   69 ++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 40 insertions(+), 29 deletions(-)

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 131289b..f67610e 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1109,6 +1109,44 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
 }
 
 /**
+ * gfs2_alloc_extent - allocate an extent from a given bitmap
+ *
+ * @rgd: the resource group descriptor
+ * @bi: the bitmap within the rgrp
+ * blk: the block within the RG
+ * dinode: TRUE if the first block we allocate is for a dinode
+ * @n: The extent length
+ */
+static void gfs2_alloc_extent(struct gfs2_rgrpd *rgd, struct gfs2_bitmap *bi,
+			      u32 blk, bool dinode, unsigned int *n)
+{
+	unsigned char new_state = dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED;
+	const unsigned int elen = *n;
+	u32 goal;
+	const u8 *buffer = NULL;
+
+	buffer = bi->bi_bh->b_data + bi->bi_offset;
+	gfs2_trans_add_bh(rgd->rd_gl, bi->bi_bh, 1);
+	gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
+		    bi, blk, new_state);
+	if (new_state == GFS2_BLKST_USED)
+		*n = 1;
+	new_state = GFS2_BLKST_USED; /* for extents, we need data blocks */
+	goal = blk;
+	while (*n < elen) {
+		goal++;
+		if (goal >= (bi->bi_len * GFS2_NBBY))
+			break;
+		if (gfs2_testbit(rgd, buffer, bi->bi_len, goal) !=
+		    GFS2_BLKST_FREE)
+			break;
+		gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
+			    bi, goal, new_state);
+		(*n)++;
+	}
+}
+
+/**
  * rgblk_search - find a block in @old_state, change allocation
  *           state to @new_state
  * @rgd: the resource group descriptor
@@ -1137,16 +1175,8 @@ static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
 	const u32 length = rgd->rd_length;
 	u32 blk = BFITNOENT;
 	unsigned int buf, x;
-	const unsigned int elen = *n;
 	const u8 *buffer = NULL;
-	unsigned char new_state;
 
-	if (old_state == GFS2_BLKST_UNLINKED)
-		new_state = GFS2_BLKST_UNLINKED;
-	else if (dinode)
-		new_state = GFS2_BLKST_DINODE;
-	else
-		new_state = GFS2_BLKST_USED;
 	*n = 0;
 	/* Find bitmap block that contains bits for goal block */
 	for (buf = 0; buf < length; buf++) {
@@ -1197,28 +1227,9 @@ skip:
 	if (blk == BFITNOENT)
 		return blk;
 
-	if (old_state == new_state)
-		goto out;
+	if (old_state != GFS2_BLKST_UNLINKED)
+		gfs2_alloc_extent(rgd, bi, blk, dinode, n);
 
-	gfs2_trans_add_bh(rgd->rd_gl, bi->bi_bh, 1);
-	gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
-		    bi, blk, new_state);
-	if (new_state == GFS2_BLKST_USED)
-		*n = 1;
-	new_state = GFS2_BLKST_USED; /* for extents, we need data blocks */
-	goal = blk;
-	while (*n < elen) {
-		goal++;
-		if (goal >= (bi->bi_len * GFS2_NBBY))
-			break;
-		if (gfs2_testbit(rgd, buffer, bi->bi_len, goal) !=
-		    GFS2_BLKST_FREE)
-			break;
-		gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
-			    bi, goal, new_state);
-		(*n)++;
-	}
-out:
 	return (bi->bi_start * GFS2_NBBY) + blk;
 }
 

[Cluster-devel] [GFS2 PATCH] GFS2: combine gfs2_alloc_block and gfs2_alloc_di

[Steven Whitehouse]

Hi,

On Wed, 2011-11-16 at 14:07 -0500, Bob Peterson wrote:
> ----- Original Message -----
> | It is not a good plan to rely on this to give us 0 and 1 inode
> | counts.
> | Using the ?: as before would be better. I suspect that if you
> | maintain
So the new patch is still doing bool to int conversions in various
places, I think.

> | separate counts of inodes (which can be 0 or 1 only) and other blocks
> | (any number) calculated at the start of the function then a number of
> | the tests you need to do will just become 0 or non-zero on one or
> | other
> | other of these.
> | 
> | Also, it would be a good plan to always pass the extent size in, even
> | if
> | the caller is requesting only a single block, then there is no need
> | for
> | a NULL test,
> | 
> | Steve.
> 
> Hi,
> 
> Okay, scratch that then. How about something like the following patch?
> 
> This patch moves more toward a generic multi-block allocator that
> takes a pointer to the number of data blocks to allocate, plus whether
> or not to allocate a dinode. In theory, it could be called to allocate
> (1) a single dinode block, (2) a group of one or more data blocks, or
> (3) a dinode plus several data blocks.
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> --
>  fs/gfs2/bmap.c  |    4 +-
>  fs/gfs2/dir.c   |    2 +-
>  fs/gfs2/inode.c |    3 +-
>  fs/gfs2/rgrp.c  |   58 +++++++++++++++++++++++++++++-------------------------
>  fs/gfs2/rgrp.h  |    4 +-
>  fs/gfs2/xattr.c |    6 ++--
>  6 files changed, 41 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index b69235b..cb74312 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -133,7 +133,7 @@ int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
>  		   and write it out to disk */
>  
>  		unsigned int n = 1;
> -		error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
> +		error = gfs2_alloc_blocks(ip, &block, &n, 0, NULL);
>  		if (error)
>  			goto out_brelse;
>  		if (isdir) {
> @@ -503,7 +503,7 @@ static int gfs2_bmap_alloc(struct inode *inode, const sector_t lblock,
>  	do {
>  		int error;
>  		n = blks - alloced;
> -		error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
> +		error = gfs2_alloc_blocks(ip, &bn, &n, 0, NULL);
>  		if (error)
>  			return error;
>  		alloced += n;
> diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
> index ae75319..f8485da 100644
> --- a/fs/gfs2/dir.c
> +++ b/fs/gfs2/dir.c
> @@ -823,7 +823,7 @@ static struct gfs2_leaf *new_leaf(struct inode *inode, struct buffer_head **pbh,
>  	struct gfs2_dirent *dent;
>  	struct qstr name = { .name = "", .len = 0, .hash = 0 };
>  
> -	error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
> +	error = gfs2_alloc_blocks(ip, &bn, &n, 0, NULL);
>  	if (error)
>  		return NULL;
>  	bh = gfs2_meta_new(ip->i_gl, bn);
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index de2668f..3ab192b 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -389,6 +389,7 @@ static int alloc_dinode(struct gfs2_inode *dip, u64 *no_addr, u64 *generation)
>  {
>  	struct gfs2_sbd *sdp = GFS2_SB(&dip->i_inode);
>  	int error;
> +	int dblocks = 0;
>  
>  	if (gfs2_alloc_get(dip) == NULL)
>  		return -ENOMEM;
> @@ -402,7 +403,7 @@ static int alloc_dinode(struct gfs2_inode *dip, u64 *no_addr, u64 *generation)
>  	if (error)
>  		goto out_ipreserv;
>  
> -	error = gfs2_alloc_block(dip, no_addr, NULL, 1, generation);
> +	error = gfs2_alloc_blocks(dip, no_addr, &dblocks, 1, generation);
>  
>  	gfs2_trans_end(sdp);
>  
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index 995f4e6..131289b 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -65,8 +65,8 @@ static const char valid_change[16] = {
>  };
>  
>  static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
> -                        unsigned char old_state, unsigned char new_state,
> -			unsigned int *n);
> +                        unsigned char old_state, bool dinode,
> +			unsigned int *ndata);
>  
>  /**
>   * gfs2_setbit - Set a bit in the bitmaps
> @@ -921,8 +921,7 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skip
>  	while (goal < rgd->rd_data) {
>  		down_write(&sdp->sd_log_flush_lock);
>  		n = 1;
> -		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED,
> -				     GFS2_BLKST_UNLINKED, &n);
> +		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED, 0, &n);
>  		up_write(&sdp->sd_log_flush_lock);
>  		if (block == BFITNOENT)
>  			break;
> @@ -1115,7 +1114,7 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
>   * @rgd: the resource group descriptor
>   * @goal: the goal block within the RG (start here to search for avail block)
>   * @old_state: GFS2_BLKST_XXX the before-allocation state to find
> - * @new_state: GFS2_BLKST_XXX the after-allocation block state
> + * dinode: TRUE if the first block we allocate is for a dinode
>   * @n: The extent length
>   *
>   * Walk rgrp's bitmap to find bits that represent a block in @old_state.
> @@ -1132,8 +1131,7 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
>   */
>  
>  static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
> -			unsigned char old_state, unsigned char new_state,
> -			unsigned int *n)
> +			unsigned char old_state, bool dinode, unsigned int *n)
>  {
>  	struct gfs2_bitmap *bi = NULL;
>  	const u32 length = rgd->rd_length;
> @@ -1141,7 +1139,14 @@ static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
>  	unsigned int buf, x;
>  	const unsigned int elen = *n;
>  	const u8 *buffer = NULL;
> +	unsigned char new_state;
>  
> +	if (old_state == GFS2_BLKST_UNLINKED)
> +		new_state = GFS2_BLKST_UNLINKED;
> +	else if (dinode)
> +		new_state = GFS2_BLKST_DINODE;
> +	else
> +		new_state = GFS2_BLKST_USED;

I know this vanishes in the next patch, but the code is a lot clearer
when it states explicitly what is being looked for and what changes are
going to be made.

>  	*n = 0;
>  	/* Find bitmap block that contains bits for goal block */
>  	for (buf = 0; buf < length; buf++) {
> @@ -1192,13 +1197,15 @@ skip:
>  	if (blk == BFITNOENT)
>  		return blk;
>  
> -	*n = 1;
>  	if (old_state == new_state)
>  		goto out;
>  
>  	gfs2_trans_add_bh(rgd->rd_gl, bi->bi_bh, 1);
>  	gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
>  		    bi, blk, new_state);
> +	if (new_state == GFS2_BLKST_USED)
> +		*n = 1;
> +	new_state = GFS2_BLKST_USED; /* for extents, we need data blocks */
If the extent length includes the inode, then we don't need to special
case this.

>  	goal = blk;
>  	while (*n < elen) {
>  		goal++;
> @@ -1300,18 +1307,18 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
>  }
>  
>  /**
> - * gfs2_alloc_block - Allocate one or more blocks
> + * gfs2_alloc_blocks - Allocate one or more blocks of data and/or a dinode
>   * @ip: the inode to allocate the block for
>   * @bn: Used to return the starting block number
> - * @n: requested number of blocks/extent length (value/result)
> - * dinode: 1 if we're allocating a dinode, 0 if it's a data block
> + * @ndata: requested number of data blocks/extent length (value/result)
> + * dinode: 1 if we're allocating a dinode block, else 0
A missing @

>   * @generation: the generation number of the inode
>   *
>   * Returns: 0 or error
>   */
>  
> -int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
> -		     int dinode, u64 *generation)
> +int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *ndata,
> +		      bool dinode, u64 *generation)
>  {
>  	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
>  	struct buffer_head *dibh;
> @@ -1319,9 +1326,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  	struct gfs2_rgrpd *rgd;
>  	u32 goal, blk; /* block, within the rgrp scope */
>  	u64 block; /* block, within the file system scope */
> -	unsigned int extn = 1;
>  	int error;
> -	unsigned char blk_type = dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED;
>  
>  	/* Only happens if there is a bug in gfs2, return something distinctive
>  	 * to ensure that it is noticed.
> @@ -1329,8 +1334,6 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  	if (al == NULL)
>  		return -ECANCELED;
>  
> -	if (n == NULL)
> -		n = &extn;
>  	rgd = ip->i_rgd;
>  
>  	if (!dinode && rgrp_contains_block(rgd, ip->i_goal))
> @@ -1338,7 +1341,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  	else
>  		goal = rgd->rd_last_alloc;
>  
> -	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, blk_type, n);
> +	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, dinode, ndata);
>  
>  	/* Since all blocks are reserved in advance, this shouldn't happen */
>  	if (blk == BFITNOENT)
> @@ -1347,7 +1350,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  	rgd->rd_last_alloc = blk;
>  	block = rgd->rd_data0 + blk;
>  	if (!dinode) {
> -		ip->i_goal = block + *n - 1;
> +		ip->i_goal = block + *ndata - 1;
>  		error = gfs2_meta_inode_buffer(ip, &dibh);
>  		if (error == 0) {
>  			struct gfs2_dinode *di =
> @@ -1358,11 +1361,12 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  			brelse(dibh);
>  		}
>  	}
> -	if (rgd->rd_free < *n)
> +	if (rgd->rd_free < (*ndata) + dinode)

This (*ndata) + dinode expression appears a lot, can we just have an
extent length variable, so it doesn't have to be calculated separately
each time?

>  		goto rgrp_error;
>  
> -	rgd->rd_free -= *n;
> +	rgd->rd_free -= *ndata;
>  	if (dinode) {
> +		rgd->rd_free--;
>  		rgd->rd_dinodes++;
>  		*generation = rgd->rd_igeneration++;
>  		if (*generation == 0)
> @@ -1372,17 +1376,17 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  	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);
>  
> -	al->al_alloced += *n;
> +	al->al_alloced += (*ndata) + dinode;
>  
> -	gfs2_statfs_change(sdp, 0, -(s64)*n, dinode ? 1 : 0);
> +	gfs2_statfs_change(sdp, 0, (-(s64)((*ndata) + dinode)), dinode);
>  	if (dinode)
>  		gfs2_trans_add_unrevoke(sdp, block, 1);
> -	else
> -		gfs2_quota_change(ip, *n, ip->i_inode.i_uid,
> +	if (*ndata)
> +		gfs2_quota_change(ip, *ndata, ip->i_inode.i_uid,
>  				  ip->i_inode.i_gid);
>  
Looks like we need to take a look at the inode allocation and see why we
don't do the quota change for that via this function. That can be a
separate patch though.

> -	rgd->rd_free_clone -= *n;
> -	trace_gfs2_block_alloc(ip, block, *n, blk_type);
> +	rgd->rd_free_clone -= (*ndata) + dinode;
> +	trace_gfs2_block_alloc(ip, block, *ndata, dinode);
The call to the tracepoint is wrong, the final argument is supposed to
be the type of the block that is being changed. That makes it a bit
awkward in terms of tracing with the combined functions :( Maybe resolve
this in the splitting rgblk_search patch, where it might be possible to
move the tracing to the gfs2_alloc_extent() function.

Otherwise, it looks good,

Steve.

>  	*bn = block;
>  	return 0;
>  
> diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
> index 4cb5608..b3b61b8 100644
> --- a/fs/gfs2/rgrp.h
> +++ b/fs/gfs2/rgrp.h
> @@ -39,8 +39,8 @@ static inline void gfs2_alloc_put(struct gfs2_inode *ip)
>  extern int gfs2_inplace_reserve(struct gfs2_inode *ip);
>  extern void gfs2_inplace_release(struct gfs2_inode *ip);
>  
> -extern int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
> -			    int dinode, u64 *generation);
> +extern int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
> +			     bool dinode, u64 *generation);
>  
>  extern void __gfs2_free_blocks(struct gfs2_inode *ip, u64 bstart, u32 blen, int meta);
>  extern void gfs2_free_meta(struct gfs2_inode *ip, u64 bstart, u32 blen);
> diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
> index e4794a5..ef74e159 100644
> --- a/fs/gfs2/xattr.c
> +++ b/fs/gfs2/xattr.c
> @@ -610,7 +610,7 @@ static int ea_alloc_blk(struct gfs2_inode *ip, struct buffer_head **bhp)
>  	u64 block;
>  	int error;
>  
> -	error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
> +	error = gfs2_alloc_blocks(ip, &block, &n, 0, NULL);
>  	if (error)
>  		return error;
>  	gfs2_trans_add_unrevoke(sdp, block, 1);
> @@ -672,7 +672,7 @@ static int ea_write(struct gfs2_inode *ip, struct gfs2_ea_header *ea,
>  			int mh_size = sizeof(struct gfs2_meta_header);
>  			unsigned int n = 1;
>  
> -			error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
> +			error = gfs2_alloc_blocks(ip, &block, &n, 0, NULL);
>  			if (error)
>  				return error;
>  			gfs2_trans_add_unrevoke(sdp, block, 1);
> @@ -992,7 +992,7 @@ static int ea_set_block(struct gfs2_inode *ip, struct gfs2_ea_request *er,
>  	} else {
>  		u64 blk;
>  		unsigned int n = 1;
> -		error = gfs2_alloc_block(ip, &blk, &n, 0, NULL);
> +		error = gfs2_alloc_blocks(ip, &blk, &n, 0, NULL);
>  		if (error)
>  			return error;
>  		gfs2_trans_add_unrevoke(sdp, blk, 1);

[Cluster-devel] [GFS2 PATCH] GFS2: combine gfs2_alloc_block and gfs2_alloc_di

[Steven Whitehouse]

Hi,

On Wed, 2011-11-16 at 14:59 -0500, Bob Peterson wrote:
> ----- Original Message -----
> | Hi,
> | 
> | As a follow up to this patch, I'd quite like to see rgblk_search
> | split
> | into two functions. One to do the search and another to actually make
> | changes to the bitmap when a block is found. That should help
> | development of further alloc changes on top.
> 
> Hi,
> 
> So something like the patch that follows?
> 
> This patch splits function rgblk_search into two functions:
> the finding and the bit setting function, now called gfs2_alloc_extent.
> 
Yes, but lets make gfs2_alloc_extent() a totally separate function and
not call it from rgblk_search, that way when looking for unlinked inodes
we don't need to care about gfs2_alloc_extent at all. The only issue is
passing a couple of things from rgblk_search to gfs2_alloc_extent (the
bi (either by index or by pointer) and the current position in the
bitmap.

However my thought was to separate these two completely rather than
leaving one calling the other,

Steve.

> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> --
>  fs/gfs2/rgrp.c |   69 ++++++++++++++++++++++++++++++++-----------------------
>  1 files changed, 40 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index 131289b..f67610e 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -1109,6 +1109,44 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
>  }
>  
>  /**
> + * gfs2_alloc_extent - allocate an extent from a given bitmap
> + *
> + * @rgd: the resource group descriptor
> + * @bi: the bitmap within the rgrp
> + * blk: the block within the RG
> + * dinode: TRUE if the first block we allocate is for a dinode
> + * @n: The extent length
> + */
> +static void gfs2_alloc_extent(struct gfs2_rgrpd *rgd, struct gfs2_bitmap *bi,
> +			      u32 blk, bool dinode, unsigned int *n)
> +{
> +	unsigned char new_state = dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED;
> +	const unsigned int elen = *n;
> +	u32 goal;
> +	const u8 *buffer = NULL;
> +
> +	buffer = bi->bi_bh->b_data + bi->bi_offset;
> +	gfs2_trans_add_bh(rgd->rd_gl, bi->bi_bh, 1);
> +	gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
> +		    bi, blk, new_state);
> +	if (new_state == GFS2_BLKST_USED)
> +		*n = 1;
> +	new_state = GFS2_BLKST_USED; /* for extents, we need data blocks */
> +	goal = blk;
> +	while (*n < elen) {
> +		goal++;
> +		if (goal >= (bi->bi_len * GFS2_NBBY))
> +			break;
> +		if (gfs2_testbit(rgd, buffer, bi->bi_len, goal) !=
> +		    GFS2_BLKST_FREE)
> +			break;
> +		gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
> +			    bi, goal, new_state);
> +		(*n)++;
> +	}
> +}
> +
> +/**
>   * rgblk_search - find a block in @old_state, change allocation
>   *           state to @new_state
>   * @rgd: the resource group descriptor
> @@ -1137,16 +1175,8 @@ static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
>  	const u32 length = rgd->rd_length;
>  	u32 blk = BFITNOENT;
>  	unsigned int buf, x;
> -	const unsigned int elen = *n;
>  	const u8 *buffer = NULL;
> -	unsigned char new_state;
>  
> -	if (old_state == GFS2_BLKST_UNLINKED)
> -		new_state = GFS2_BLKST_UNLINKED;
> -	else if (dinode)
> -		new_state = GFS2_BLKST_DINODE;
> -	else
> -		new_state = GFS2_BLKST_USED;
>  	*n = 0;
>  	/* Find bitmap block that contains bits for goal block */
>  	for (buf = 0; buf < length; buf++) {
> @@ -1197,28 +1227,9 @@ skip:
>  	if (blk == BFITNOENT)
>  		return blk;
>  
> -	if (old_state == new_state)
> -		goto out;
> +	if (old_state != GFS2_BLKST_UNLINKED)
> +		gfs2_alloc_extent(rgd, bi, blk, dinode, n);
>  
> -	gfs2_trans_add_bh(rgd->rd_gl, bi->bi_bh, 1);
> -	gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
> -		    bi, blk, new_state);
> -	if (new_state == GFS2_BLKST_USED)
> -		*n = 1;
> -	new_state = GFS2_BLKST_USED; /* for extents, we need data blocks */
> -	goal = blk;
> -	while (*n < elen) {
> -		goal++;
> -		if (goal >= (bi->bi_len * GFS2_NBBY))
> -			break;
> -		if (gfs2_testbit(rgd, buffer, bi->bi_len, goal) !=
> -		    GFS2_BLKST_FREE)
> -			break;
> -		gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
> -			    bi, goal, new_state);
> -		(*n)++;
> -	}
> -out:
>  	return (bi->bi_start * GFS2_NBBY) + blk;
>  }
>  

[Cluster-devel] [GFS2 PATCH] GFS2: combine gfs2_alloc_block and gfs2_alloc_di

[Bob Peterson]

----- Original Message -----
| Hi,
| 
| Yes, but lets make gfs2_alloc_extent() a totally separate function
| and
| not call it from rgblk_search, that way when looking for unlinked
| inodes
| we don't need to care about gfs2_alloc_extent at all. The only issue
| is
| passing a couple of things from rgblk_search to gfs2_alloc_extent
| (the
| bi (either by index or by pointer) and the current position in the
| bitmap.
| 
| However my thought was to separate these two completely rather than
| leaving one calling the other,
| 
| Steve.

Hi,

Okay, I was afraid that's what you wanted. :)  It just seemed
"messy" to me to pass the bi pointers and such, but I'll do it.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [GFS2 PATCH] GFS2: combine gfs2_alloc_block and gfs2_alloc_di

[Bob Peterson]

----- Original Message -----
| Hi,
| 
| On Wed, 2011-11-16 at 14:07 -0500, Bob Peterson wrote:
| > ----- Original Message -----
| > | It is not a good plan to rely on this to give us 0 and 1 inode
| > | counts.
| > | Using the ?: as before would be better. I suspect that if you
| > | maintain
| So the new patch is still doing bool to int conversions in various
| places, I think.

Hi,

Thanks for your suggestions. I'll clean this patch up as per your
recommendations and re-post.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [GFS2 PATCH TRY 2] GFS2: move toward a generic multi-block allocator

[Bob Peterson]

Hi,

This patch is a revision of the one I previously posted.
I tried to integrate all the suggestions Steve gave.
The purpose of the patch is to change function gfs2_alloc_block
(allocate either a dinode block or an extent of data blocks)
to a more generic gfs2_alloc_blocks function that can
allocate both a dinode _and_ an extent of data blocks in the
same call. This will ultimately help us create a multi-block
reservation scheme to reduce file fragmentation.

Regards,

Bob Peterson
Red Hat File Systems

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
--
GFS2: move toward a generic multi-block allocator

This patch moves more toward a generic multi-block allocator that
takes a pointer to the number of data blocks to allocate, plus whether
or not to allocate a dinode. In theory, it could be called to allocate
(1) a single dinode block, (2) a group of one or more data blocks, or
(3) a dinode plus several data blocks.

diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index b69235b..cb74312 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -133,7 +133,7 @@ int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
 		   and write it out to disk */
 
 		unsigned int n = 1;
-		error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
+		error = gfs2_alloc_blocks(ip, &block, &n, 0, NULL);
 		if (error)
 			goto out_brelse;
 		if (isdir) {
@@ -503,7 +503,7 @@ static int gfs2_bmap_alloc(struct inode *inode, const sector_t lblock,
 	do {
 		int error;
 		n = blks - alloced;
-		error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
+		error = gfs2_alloc_blocks(ip, &bn, &n, 0, NULL);
 		if (error)
 			return error;
 		alloced += n;
diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index ae75319..f8485da 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -823,7 +823,7 @@ static struct gfs2_leaf *new_leaf(struct inode *inode, struct buffer_head **pbh,
 	struct gfs2_dirent *dent;
 	struct qstr name = { .name = "", .len = 0, .hash = 0 };
 
-	error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
+	error = gfs2_alloc_blocks(ip, &bn, &n, 0, NULL);
 	if (error)
 		return NULL;
 	bh = gfs2_meta_new(ip->i_gl, bn);
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index de2668f..3ab192b 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -389,6 +389,7 @@ static int alloc_dinode(struct gfs2_inode *dip, u64 *no_addr, u64 *generation)
 {
 	struct gfs2_sbd *sdp = GFS2_SB(&dip->i_inode);
 	int error;
+	int dblocks = 0;
 
 	if (gfs2_alloc_get(dip) == NULL)
 		return -ENOMEM;
@@ -402,7 +403,7 @@ static int alloc_dinode(struct gfs2_inode *dip, u64 *no_addr, u64 *generation)
 	if (error)
 		goto out_ipreserv;
 
-	error = gfs2_alloc_block(dip, no_addr, NULL, 1, generation);
+	error = gfs2_alloc_blocks(dip, no_addr, &dblocks, 1, generation);
 
 	gfs2_trans_end(sdp);
 
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 855597a..b8935af 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -65,8 +65,8 @@ static const char valid_change[16] = {
 };
 
 static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
-                        unsigned char old_state, unsigned char new_state,
-			unsigned int *n);
+                        unsigned char old_state, bool dinode,
+			unsigned int *ndata);
 
 /**
  * gfs2_setbit - Set a bit in the bitmaps
@@ -921,8 +921,7 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skip
 	while (goal < rgd->rd_data) {
 		down_write(&sdp->sd_log_flush_lock);
 		n = 1;
-		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED,
-				     GFS2_BLKST_UNLINKED, &n);
+		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED, 0, &n);
 		up_write(&sdp->sd_log_flush_lock);
 		if (block == BFITNOENT)
 			break;
@@ -1115,7 +1114,7 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
  * @rgd: the resource group descriptor
  * @goal: the goal block within the RG (start here to search for avail block)
  * @old_state: GFS2_BLKST_XXX the before-allocation state to find
- * @new_state: GFS2_BLKST_XXX the after-allocation block state
+ * @dinode: TRUE if the first block we allocate is for a dinode
  * @n: The extent length
  *
  * Walk rgrp's bitmap to find bits that represent a block in @old_state.
@@ -1132,8 +1131,7 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
  */
 
 static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
-			unsigned char old_state, unsigned char new_state,
-			unsigned int *n)
+			unsigned char old_state, bool dinode, unsigned int *n)
 {
 	struct gfs2_bitmap *bi = NULL;
 	const u32 length = rgd->rd_length;
@@ -1192,13 +1190,14 @@ skip:
 	if (blk == BFITNOENT)
 		return blk;
 
-	*n = 1;
-	if (old_state == new_state)
+	if (old_state == GFS2_BLKST_UNLINKED)
 		goto out;
 
 	gfs2_trans_add_bh(rgd->rd_gl, bi->bi_bh, 1);
 	gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
-		    bi, blk, new_state);
+		    bi, blk, dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED);
+	if (!dinode)
+		(*n)++;
 	goal = blk;
 	while (*n < elen) {
 		goal++;
@@ -1208,7 +1207,7 @@ skip:
 		    GFS2_BLKST_FREE)
 			break;
 		gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
-			    bi, goal, new_state);
+			    bi, goal, GFS2_BLKST_USED);
 		(*n)++;
 	}
 out:
@@ -1300,28 +1299,26 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
 }
 
 /**
- * gfs2_alloc_block - Allocate one or more blocks
+ * gfs2_alloc_blocks - Allocate one or more blocks of data and/or a dinode
  * @ip: the inode to allocate the block for
  * @bn: Used to return the starting block number
- * @n: requested number of blocks/extent length (value/result)
- * dinode: 1 if we're allocating a dinode, 0 if it's a data block
+ * @ndata: requested number of data blocks/extent length (value/result)
+ * @dinode: 1 if we're allocating a dinode block, else 0
  * @generation: the generation number of the inode
  *
  * Returns: 0 or error
  */
 
-int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
-		     int dinode, u64 *generation)
+int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *ndata,
+		      bool dinode, u64 *generation)
 {
 	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
 	struct buffer_head *dibh;
 	struct gfs2_alloc *al = ip->i_alloc;
 	struct gfs2_rgrpd *rgd;
-	u32 goal, blk; /* block, within the rgrp scope */
+	u32 goal, extlen, blk; /* block, within the rgrp scope */
 	u64 block; /* block, within the file system scope */
-	unsigned int extn = 1;
 	int error;
-	unsigned char blk_type = dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED;
 
 	/* Only happens if there is a bug in gfs2, return something distinctive
 	 * to ensure that it is noticed.
@@ -1329,8 +1326,6 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 	if (al == NULL)
 		return -ECANCELED;
 
-	if (n == NULL)
-		n = &extn;
 	rgd = ip->i_rgd;
 
 	if (!dinode && rgrp_contains_block(rgd, ip->i_goal))
@@ -1338,7 +1333,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 	else
 		goal = rgd->rd_last_alloc;
 
-	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, blk_type, n);
+	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, dinode, ndata);
 
 	/* Since all blocks are reserved in advance, this shouldn't happen */
 	if (blk == BFITNOENT)
@@ -1347,7 +1342,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 	rgd->rd_last_alloc = blk;
 	block = rgd->rd_data0 + blk;
 	if (!dinode) {
-		ip->i_goal = block + *n - 1;
+		ip->i_goal = block + *ndata - 1;
 		error = gfs2_meta_inode_buffer(ip, &dibh);
 		if (error == 0) {
 			struct gfs2_dinode *di =
@@ -1358,10 +1353,13 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 			brelse(dibh);
 		}
 	}
-	if (rgd->rd_free < *n)
+	extlen = *ndata;
+	if (dinode)
+		extlen++;
+	if (rgd->rd_free < extlen)
 		goto rgrp_error;
 
-	rgd->rd_free -= *n;
+	rgd->rd_free -= extlen;
 	if (dinode) {
 		rgd->rd_dinodes++;
 		*generation = rgd->rd_igeneration++;
@@ -1372,15 +1370,16 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
 	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);
 
-	gfs2_statfs_change(sdp, 0, -(s64)*n, dinode ? 1 : 0);
+	gfs2_statfs_change(sdp, 0, -(s64)extlen, dinode ? 1 : 0);
 	if (dinode)
 		gfs2_trans_add_unrevoke(sdp, block, 1);
-	else
-		gfs2_quota_change(ip, *n, ip->i_inode.i_uid,
+	if (*ndata)
+		gfs2_quota_change(ip, *ndata, ip->i_inode.i_uid,
 				  ip->i_inode.i_gid);
 
-	rgd->rd_free_clone -= *n;
-	trace_gfs2_block_alloc(ip, block, *n, blk_type);
+	rgd->rd_free_clone -= extlen;
+	trace_gfs2_block_alloc(ip, block, *ndata,
+			       dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED);
 	*bn = block;
 	return 0;
 
diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
index 4cb5608..b3b61b8 100644
--- a/fs/gfs2/rgrp.h
+++ b/fs/gfs2/rgrp.h
@@ -39,8 +39,8 @@ static inline void gfs2_alloc_put(struct gfs2_inode *ip)
 extern int gfs2_inplace_reserve(struct gfs2_inode *ip);
 extern void gfs2_inplace_release(struct gfs2_inode *ip);
 
-extern int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
-			    int dinode, u64 *generation);
+extern int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
+			     bool dinode, u64 *generation);
 
 extern void __gfs2_free_blocks(struct gfs2_inode *ip, u64 bstart, u32 blen, int meta);
 extern void gfs2_free_meta(struct gfs2_inode *ip, u64 bstart, u32 blen);
diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
index e4794a5..ef74e159 100644
--- a/fs/gfs2/xattr.c
+++ b/fs/gfs2/xattr.c
@@ -610,7 +610,7 @@ static int ea_alloc_blk(struct gfs2_inode *ip, struct buffer_head **bhp)
 	u64 block;
 	int error;
 
-	error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
+	error = gfs2_alloc_blocks(ip, &block, &n, 0, NULL);
 	if (error)
 		return error;
 	gfs2_trans_add_unrevoke(sdp, block, 1);
@@ -672,7 +672,7 @@ static int ea_write(struct gfs2_inode *ip, struct gfs2_ea_header *ea,
 			int mh_size = sizeof(struct gfs2_meta_header);
 			unsigned int n = 1;
 
-			error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
+			error = gfs2_alloc_blocks(ip, &block, &n, 0, NULL);
 			if (error)
 				return error;
 			gfs2_trans_add_unrevoke(sdp, block, 1);
@@ -992,7 +992,7 @@ static int ea_set_block(struct gfs2_inode *ip, struct gfs2_ea_request *er,
 	} else {
 		u64 blk;
 		unsigned int n = 1;
-		error = gfs2_alloc_block(ip, &blk, &n, 0, NULL);
+		error = gfs2_alloc_blocks(ip, &blk, &n, 0, NULL);
 		if (error)
 			return error;
 		gfs2_trans_add_unrevoke(sdp, blk, 1);

[Cluster-devel] [GFS2 PATCH TRY2] GFS2: split function rgblk_search

[Bob Peterson]

----- Original Message -----
| Hi,
| 
| On Wed, 2011-11-16 at 14:59 -0500, Bob Peterson wrote:
| > ----- Original Message -----
| > | Hi,
| > | 
| > | As a follow up to this patch, I'd quite like to see rgblk_search
| > | split
| > | into two functions. One to do the search and another to actually
| > | make
| > | changes to the bitmap when a block is found. That should help
| > | development of further alloc changes on top.
| > 
| > Hi,
| > 
| > So something like the patch that follows?
| > 
| > This patch splits function rgblk_search into two functions:
| > the finding and the bit setting function, now called
| > gfs2_alloc_extent.
| > 
| Yes, but lets make gfs2_alloc_extent() a totally separate function
| and
| not call it from rgblk_search, that way when looking for unlinked
| inodes
| we don't need to care about gfs2_alloc_extent at all. The only issue
| is
| passing a couple of things from rgblk_search to gfs2_alloc_extent
| (the
| bi (either by index or by pointer) and the current position in the
| bitmap.
| 
| However my thought was to separate these two completely rather than
| leaving one calling the other,
| 
| Steve.

Hi,

This is another revision of a previously posted patch based on Steve's
suggestions. The idea here is to split function rgblk_search into two
functions: one to search for a block in the proper state and another
to assign multiple blocks starting at that location.

Splitting the two allowed for some code optimizations.

Regards,

Bob Peterson
Red Hat File Systems

Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
--
GFS2: split function rgblk_search

This patch splits function rgblk_search into a function that finds
blocks to allocate (rgblk_search) and a function that assigns those
blocks (gfs2_alloc_extent).

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index b8935af..8f87cd0 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -65,8 +65,8 @@ static const char valid_change[16] = {
 };
 
 static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
-                        unsigned char old_state, bool dinode,
-			unsigned int *ndata);
+			unsigned char old_state, bool dinode,
+			struct gfs2_bitmap **rbi);
 
 /**
  * gfs2_setbit - Set a bit in the bitmaps
@@ -912,19 +912,20 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skip
 	u32 goal = 0, block;
 	u64 no_addr;
 	struct gfs2_sbd *sdp = rgd->rd_sbd;
-	unsigned int n;
 	struct gfs2_glock *gl;
 	struct gfs2_inode *ip;
 	int error;
 	int found = 0;
+	struct gfs2_bitmap *bi;
 
 	while (goal < rgd->rd_data) {
 		down_write(&sdp->sd_log_flush_lock);
-		n = 1;
-		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED, 0, &n);
+		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED, 0, &bi);
 		up_write(&sdp->sd_log_flush_lock);
 		if (block == BFITNOENT)
 			break;
+
+		block = GFS2_BI2RGD_BLK(bi, block);
 		/* rgblk_search can return a block < goal, so we need to
 		   keep it marching forward. */
 		no_addr = block + rgd->rd_data0;
@@ -1109,38 +1110,35 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
 }
 
 /**
- * rgblk_search - find a block in @old_state, change allocation
- *           state to @new_state
+ * rgblk_search - find a block in @old_state
  * @rgd: the resource group descriptor
  * @goal: the goal block within the RG (start here to search for avail block)
  * @old_state: GFS2_BLKST_XXX the before-allocation state to find
  * @dinode: TRUE if the first block we allocate is for a dinode
- * @n: The extent length
+ * @rbi: address of the pointer to the bitmap containing the block found
  *
  * Walk rgrp's bitmap to find bits that represent a block in @old_state.
- * Add the found bitmap buffer to the transaction.
- * Set the found bits to @new_state to change block's allocation state.
  *
  * This function never fails, because we wouldn't call it unless we
  * know (from reservation results, etc.) that a block is available.
  *
- * Scope of @goal and returned block is just within rgrp, not the whole
- * filesystem.
+ * Scope of @goal is just within rgrp, not the whole filesystem.
+ * Scope of @returned block is just within bitmap, not the whole filesystem.
  *
- * Returns:  the block number allocated
+ * Returns: the block number found relative to the bitmap rbi
  */
 
 static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
-			unsigned char old_state, bool dinode, unsigned int *n)
+			unsigned char old_state, bool dinode,
+			struct gfs2_bitmap **rbi)
 {
 	struct gfs2_bitmap *bi = NULL;
 	const u32 length = rgd->rd_length;
 	u32 blk = BFITNOENT;
 	unsigned int buf, x;
-	const unsigned int elen = *n;
 	const u8 *buffer = NULL;
 
-	*n = 0;
+	*rbi = NULL;
 	/* Find bitmap block that contains bits for goal block */
 	for (buf = 0; buf < length; buf++) {
 		bi = rgd->rd_bits + buf;
@@ -1187,12 +1185,31 @@ skip:
 		goal = 0;
 	}
 
-	if (blk == BFITNOENT)
-		return blk;
+	if (blk != BFITNOENT)
+		*rbi = bi;
+
+	return blk;
+}
 
-	if (old_state == GFS2_BLKST_UNLINKED)
-		goto out;
+/**
+ * gfs2_alloc_extent - allocate an extent from a given bitmap
+ * @rgd: the resource group descriptor
+ * @bi: the bitmap within the rgrp
+ * @blk: the block within the bitmap
+ * @dinode: TRUE if the first block we allocate is for a dinode
+ * @n: The extent length
+ *
+ * Add the found bitmap buffer to the transaction.
+ * Set the found bits to @new_state to change block's allocation state.
+ */
+static void gfs2_alloc_extent(struct gfs2_rgrpd *rgd, struct gfs2_bitmap *bi,
+			      u32 blk, bool dinode, unsigned int *n)
+{
+	const unsigned int elen = *n;
+	u32 goal;
+	const u8 *buffer = NULL;
 
+	buffer = bi->bi_bh->b_data + bi->bi_offset;
 	gfs2_trans_add_bh(rgd->rd_gl, bi->bi_bh, 1);
 	gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
 		    bi, blk, dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED);
@@ -1210,8 +1227,6 @@ skip:
 			    bi, goal, GFS2_BLKST_USED);
 		(*n)++;
 	}
-out:
-	return (bi->bi_start * GFS2_NBBY) + blk;
 }
 
 /**
@@ -1319,6 +1334,7 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *ndata,
 	u32 goal, extlen, blk; /* block, within the rgrp scope */
 	u64 block; /* block, within the file system scope */
 	int error;
+	struct gfs2_bitmap *bi;
 
 	/* Only happens if there is a bug in gfs2, return something distinctive
 	 * to ensure that it is noticed.
@@ -1333,12 +1349,16 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *ndata,
 	else
 		goal = rgd->rd_last_alloc;
 
-	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, dinode, ndata);
+	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, dinode, &bi);
 
+	*ndata = 0;
 	/* Since all blocks are reserved in advance, this shouldn't happen */
 	if (blk == BFITNOENT)
 		goto rgrp_error;
 
+	gfs2_alloc_extent(rgd, bi, blk, dinode, ndata);
+
+	blk = GFS2_BI2RGD_BLK(bi, blk);
 	rgd->rd_last_alloc = blk;
 	block = rgd->rd_data0 + blk;
 	if (!dinode) {
diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
index b3b61b8..aacb3fd 100644
--- a/fs/gfs2/rgrp.h
+++ b/fs/gfs2/rgrp.h
@@ -12,6 +12,8 @@
 
 #include <linux/slab.h>
 
+#define GFS2_BI2RGD_BLK(bi, blk) (bi->bi_start * GFS2_NBBY) + blk
+
 struct gfs2_rgrpd;
 struct gfs2_sbd;
 struct gfs2_holder;

[Cluster-devel] [GFS2 PATCH TRY 2] GFS2: move toward a generic multi-block allocator

[Steven Whitehouse]

Hi,

On Fri, 2011-11-18 at 10:58 -0500, Bob Peterson wrote:
> Hi,
> 
> This patch is a revision of the one I previously posted.
> I tried to integrate all the suggestions Steve gave.
> The purpose of the patch is to change function gfs2_alloc_block
> (allocate either a dinode block or an extent of data blocks)
> to a more generic gfs2_alloc_blocks function that can
> allocate both a dinode _and_ an extent of data blocks in the
> same call. This will ultimately help us create a multi-block
> reservation scheme to reduce file fragmentation.
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> --
> GFS2: move toward a generic multi-block allocator
> 
> This patch moves more toward a generic multi-block allocator that
> takes a pointer to the number of data blocks to allocate, plus whether
> or not to allocate a dinode. In theory, it could be called to allocate
> (1) a single dinode block, (2) a group of one or more data blocks, or
> (3) a dinode plus several data blocks.
> 
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index b69235b..cb74312 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -133,7 +133,7 @@ int gfs2_unstuff_dinode(struct gfs2_inode *ip, struct page *page)
>  		   and write it out to disk */
>  
>  		unsigned int n = 1;
> -		error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
> +		error = gfs2_alloc_blocks(ip, &block, &n, 0, NULL);
>  		if (error)
>  			goto out_brelse;
>  		if (isdir) {
> @@ -503,7 +503,7 @@ static int gfs2_bmap_alloc(struct inode *inode, const sector_t lblock,
>  	do {
>  		int error;
>  		n = blks - alloced;
> -		error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
> +		error = gfs2_alloc_blocks(ip, &bn, &n, 0, NULL);
>  		if (error)
>  			return error;
>  		alloced += n;
> diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
> index ae75319..f8485da 100644
> --- a/fs/gfs2/dir.c
> +++ b/fs/gfs2/dir.c
> @@ -823,7 +823,7 @@ static struct gfs2_leaf *new_leaf(struct inode *inode, struct buffer_head **pbh,
>  	struct gfs2_dirent *dent;
>  	struct qstr name = { .name = "", .len = 0, .hash = 0 };
>  
> -	error = gfs2_alloc_block(ip, &bn, &n, 0, NULL);
> +	error = gfs2_alloc_blocks(ip, &bn, &n, 0, NULL);
>  	if (error)
>  		return NULL;
>  	bh = gfs2_meta_new(ip->i_gl, bn);
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index de2668f..3ab192b 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -389,6 +389,7 @@ static int alloc_dinode(struct gfs2_inode *dip, u64 *no_addr, u64 *generation)
>  {
>  	struct gfs2_sbd *sdp = GFS2_SB(&dip->i_inode);
>  	int error;
> +	int dblocks = 0;
>  
>  	if (gfs2_alloc_get(dip) == NULL)
>  		return -ENOMEM;
> @@ -402,7 +403,7 @@ static int alloc_dinode(struct gfs2_inode *dip, u64 *no_addr, u64 *generation)
>  	if (error)
>  		goto out_ipreserv;
>  
> -	error = gfs2_alloc_block(dip, no_addr, NULL, 1, generation);
> +	error = gfs2_alloc_blocks(dip, no_addr, &dblocks, 1, generation);
>  
>  	gfs2_trans_end(sdp);
>  
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index 855597a..b8935af 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -65,8 +65,8 @@ static const char valid_change[16] = {
>  };
>  
>  static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
> -                        unsigned char old_state, unsigned char new_state,
> -			unsigned int *n);
> +                        unsigned char old_state, bool dinode,
> +			unsigned int *ndata);
>  
>  /**
>   * gfs2_setbit - Set a bit in the bitmaps
> @@ -921,8 +921,7 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skip
>  	while (goal < rgd->rd_data) {
>  		down_write(&sdp->sd_log_flush_lock);
>  		n = 1;
> -		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED,
> -				     GFS2_BLKST_UNLINKED, &n);
> +		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED, 0, &n);
>  		up_write(&sdp->sd_log_flush_lock);
>  		if (block == BFITNOENT)
>  			break;
> @@ -1115,7 +1114,7 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
>   * @rgd: the resource group descriptor
>   * @goal: the goal block within the RG (start here to search for avail block)
>   * @old_state: GFS2_BLKST_XXX the before-allocation state to find
> - * @new_state: GFS2_BLKST_XXX the after-allocation block state
> + * @dinode: TRUE if the first block we allocate is for a dinode
>   * @n: The extent length
>   *
>   * Walk rgrp's bitmap to find bits that represent a block in @old_state.
> @@ -1132,8 +1131,7 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
>   */
>  
>  static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
> -			unsigned char old_state, unsigned char new_state,
> -			unsigned int *n)
> +			unsigned char old_state, bool dinode, unsigned int *n)
>  {
>  	struct gfs2_bitmap *bi = NULL;
>  	const u32 length = rgd->rd_length;
> @@ -1192,13 +1190,14 @@ skip:
>  	if (blk == BFITNOENT)
>  		return blk;
>  
> -	*n = 1;
> -	if (old_state == new_state)
> +	if (old_state == GFS2_BLKST_UNLINKED)
>  		goto out;
>  
>  	gfs2_trans_add_bh(rgd->rd_gl, bi->bi_bh, 1);
>  	gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
> -		    bi, blk, new_state);
> +		    bi, blk, dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED);
> +	if (!dinode)
> +		(*n)++;
>  	goal = blk;
>  	while (*n < elen) {
>  		goal++;
> @@ -1208,7 +1207,7 @@ skip:
>  		    GFS2_BLKST_FREE)
>  			break;
>  		gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
> -			    bi, goal, new_state);
> +			    bi, goal, GFS2_BLKST_USED);
>  		(*n)++;
>  	}
>  out:
> @@ -1300,28 +1299,26 @@ static void gfs2_rgrp_error(struct gfs2_rgrpd *rgd)
>  }
>  
>  /**
> - * gfs2_alloc_block - Allocate one or more blocks
> + * gfs2_alloc_blocks - Allocate one or more blocks of data and/or a dinode
>   * @ip: the inode to allocate the block for
>   * @bn: Used to return the starting block number
> - * @n: requested number of blocks/extent length (value/result)
> - * dinode: 1 if we're allocating a dinode, 0 if it's a data block
> + * @ndata: requested number of data blocks/extent length (value/result)
> + * @dinode: 1 if we're allocating a dinode block, else 0
>   * @generation: the generation number of the inode
>   *
>   * Returns: 0 or error
>   */
>  
> -int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
> -		     int dinode, u64 *generation)
> +int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *ndata,
> +		      bool dinode, u64 *generation)
>  {
>  	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
>  	struct buffer_head *dibh;
>  	struct gfs2_alloc *al = ip->i_alloc;
>  	struct gfs2_rgrpd *rgd;
> -	u32 goal, blk; /* block, within the rgrp scope */
> +	u32 goal, extlen, blk; /* block, within the rgrp scope */
>  	u64 block; /* block, within the file system scope */
> -	unsigned int extn = 1;
>  	int error;
> -	unsigned char blk_type = dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED;
>  
>  	/* Only happens if there is a bug in gfs2, return something distinctive
>  	 * to ensure that it is noticed.
> @@ -1329,8 +1326,6 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  	if (al == NULL)
>  		return -ECANCELED;
>  
> -	if (n == NULL)
> -		n = &extn;
>  	rgd = ip->i_rgd;
>  
>  	if (!dinode && rgrp_contains_block(rgd, ip->i_goal))
> @@ -1338,7 +1333,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  	else
>  		goal = rgd->rd_last_alloc;
>  
> -	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, blk_type, n);
> +	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, dinode, ndata);
>  
>  	/* Since all blocks are reserved in advance, this shouldn't happen */
>  	if (blk == BFITNOENT)
> @@ -1347,7 +1342,7 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  	rgd->rd_last_alloc = blk;
>  	block = rgd->rd_data0 + blk;
>  	if (!dinode) {
> -		ip->i_goal = block + *n - 1;
> +		ip->i_goal = block + *ndata - 1;
>  		error = gfs2_meta_inode_buffer(ip, &dibh);
>  		if (error == 0) {
>  			struct gfs2_dinode *di =
> @@ -1358,10 +1353,13 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  			brelse(dibh);
>  		}
>  	}
> -	if (rgd->rd_free < *n)
> +	extlen = *ndata;
> +	if (dinode)
> +		extlen++;
> +	if (rgd->rd_free < extlen)
>  		goto rgrp_error;
>  
> -	rgd->rd_free -= *n;
> +	rgd->rd_free -= extlen;
>  	if (dinode) {
>  		rgd->rd_dinodes++;
>  		*generation = rgd->rd_igeneration++;
> @@ -1372,15 +1370,16 @@ int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
>  	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);
>  
> -	gfs2_statfs_change(sdp, 0, -(s64)*n, dinode ? 1 : 0);
> +	gfs2_statfs_change(sdp, 0, -(s64)extlen, dinode ? 1 : 0);
>  	if (dinode)
>  		gfs2_trans_add_unrevoke(sdp, block, 1);
> -	else
> -		gfs2_quota_change(ip, *n, ip->i_inode.i_uid,
> +	if (*ndata)
> +		gfs2_quota_change(ip, *ndata, ip->i_inode.i_uid,
>  				  ip->i_inode.i_gid);
>  
> -	rgd->rd_free_clone -= *n;
> -	trace_gfs2_block_alloc(ip, block, *n, blk_type);
> +	rgd->rd_free_clone -= extlen;
> +	trace_gfs2_block_alloc(ip, block, *ndata,
> +			       dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED);

This is still going to give us the wrong tracing data here. In the
dinode case, this will give us a trace entry that says that starting at
the first block of the extent, that block and all following blocks are
dinodes up to *ndata, which is one block less than the extent size. So
the size is wrong (off by one) for the complete extent, and the type is
also wrong for all except the first block.

In the non-dinode case, it should give the correct answer though as
extlen and *ndata are the same.

What we could do, to save having to call the tracepoint twice in the
dinode case, would be to say that for an extent size greater than 1, we
"know" that all subsequent blocks will be "used". So while thats ok,
provided we document that, the *ndata still needs to be changed for
extlen to correct that, so I'll fix that up when I apply this,

Steve.



>  	*bn = block;
>  	return 0;
>  
> diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
> index 4cb5608..b3b61b8 100644
> --- a/fs/gfs2/rgrp.h
> +++ b/fs/gfs2/rgrp.h
> @@ -39,8 +39,8 @@ static inline void gfs2_alloc_put(struct gfs2_inode *ip)
>  extern int gfs2_inplace_reserve(struct gfs2_inode *ip);
>  extern void gfs2_inplace_release(struct gfs2_inode *ip);
>  
> -extern int gfs2_alloc_block(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
> -			    int dinode, u64 *generation);
> +extern int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
> +			     bool dinode, u64 *generation);
>  
>  extern void __gfs2_free_blocks(struct gfs2_inode *ip, u64 bstart, u32 blen, int meta);
>  extern void gfs2_free_meta(struct gfs2_inode *ip, u64 bstart, u32 blen);
> diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
> index e4794a5..ef74e159 100644
> --- a/fs/gfs2/xattr.c
> +++ b/fs/gfs2/xattr.c
> @@ -610,7 +610,7 @@ static int ea_alloc_blk(struct gfs2_inode *ip, struct buffer_head **bhp)
>  	u64 block;
>  	int error;
>  
> -	error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
> +	error = gfs2_alloc_blocks(ip, &block, &n, 0, NULL);
>  	if (error)
>  		return error;
>  	gfs2_trans_add_unrevoke(sdp, block, 1);
> @@ -672,7 +672,7 @@ static int ea_write(struct gfs2_inode *ip, struct gfs2_ea_header *ea,
>  			int mh_size = sizeof(struct gfs2_meta_header);
>  			unsigned int n = 1;
>  
> -			error = gfs2_alloc_block(ip, &block, &n, 0, NULL);
> +			error = gfs2_alloc_blocks(ip, &block, &n, 0, NULL);
>  			if (error)
>  				return error;
>  			gfs2_trans_add_unrevoke(sdp, block, 1);
> @@ -992,7 +992,7 @@ static int ea_set_block(struct gfs2_inode *ip, struct gfs2_ea_request *er,
>  	} else {
>  		u64 blk;
>  		unsigned int n = 1;
> -		error = gfs2_alloc_block(ip, &blk, &n, 0, NULL);
> +		error = gfs2_alloc_blocks(ip, &blk, &n, 0, NULL);
>  		if (error)
>  			return error;
>  		gfs2_trans_add_unrevoke(sdp, blk, 1);

[Cluster-devel] [GFS2 PATCH TRY2] GFS2: split function rgblk_search

[Steven Whitehouse]

Hi,

On Fri, 2011-11-18 at 13:15 -0500, Bob Peterson wrote:
> ----- Original Message -----
> | Hi,
> | 
> | On Wed, 2011-11-16 at 14:59 -0500, Bob Peterson wrote:
> | > ----- Original Message -----
> | > | Hi,
> | > | 
> | > | As a follow up to this patch, I'd quite like to see rgblk_search
> | > | split
> | > | into two functions. One to do the search and another to actually
> | > | make
> | > | changes to the bitmap when a block is found. That should help
> | > | development of further alloc changes on top.
> | > 
> | > Hi,
> | > 
> | > So something like the patch that follows?
> | > 
> | > This patch splits function rgblk_search into two functions:
> | > the finding and the bit setting function, now called
> | > gfs2_alloc_extent.
> | > 
> | Yes, but lets make gfs2_alloc_extent() a totally separate function
> | and
> | not call it from rgblk_search, that way when looking for unlinked
> | inodes
> | we don't need to care about gfs2_alloc_extent at all. The only issue
> | is
> | passing a couple of things from rgblk_search to gfs2_alloc_extent
> | (the
> | bi (either by index or by pointer) and the current position in the
> | bitmap.
> | 
> | However my thought was to separate these two completely rather than
> | leaving one calling the other,
> | 
> | Steve.
> 
> Hi,
> 
> This is another revision of a previously posted patch based on Steve's
> suggestions. The idea here is to split function rgblk_search into two
> functions: one to search for a block in the proper state and another
> to assign multiple blocks starting at that location.
> 
> Splitting the two allowed for some code optimizations.
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
> --
> GFS2: split function rgblk_search
> 
> This patch splits function rgblk_search into a function that finds
> blocks to allocate (rgblk_search) and a function that assigns those
> blocks (gfs2_alloc_extent).
> 
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index b8935af..8f87cd0 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -65,8 +65,8 @@ static const char valid_change[16] = {
>  };
>  
>  static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
> -                        unsigned char old_state, bool dinode,
> -			unsigned int *ndata);
> +			unsigned char old_state, bool dinode,
> +			struct gfs2_bitmap **rbi);
>  
>  /**
>   * gfs2_setbit - Set a bit in the bitmaps
> @@ -912,19 +912,20 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skip
>  	u32 goal = 0, block;
>  	u64 no_addr;
>  	struct gfs2_sbd *sdp = rgd->rd_sbd;
> -	unsigned int n;
>  	struct gfs2_glock *gl;
>  	struct gfs2_inode *ip;
>  	int error;
>  	int found = 0;
> +	struct gfs2_bitmap *bi;
>  
>  	while (goal < rgd->rd_data) {
>  		down_write(&sdp->sd_log_flush_lock);
> -		n = 1;
> -		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED, 0, &n);
> +		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED, 0, &bi);
>  		up_write(&sdp->sd_log_flush_lock);
>  		if (block == BFITNOENT)
>  			break;
> +
> +		block = GFS2_BI2RGD_BLK(bi, block);
>  		/* rgblk_search can return a block < goal, so we need to
>  		   keep it marching forward. */
>  		no_addr = block + rgd->rd_data0;
> @@ -1109,38 +1110,35 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
>  }
>  
>  /**
> - * rgblk_search - find a block in @old_state, change allocation
> - *           state to @new_state
> + * rgblk_search - find a block in @old_state
>   * @rgd: the resource group descriptor
>   * @goal: the goal block within the RG (start here to search for avail block)
>   * @old_state: GFS2_BLKST_XXX the before-allocation state to find
>   * @dinode: TRUE if the first block we allocate is for a dinode
> - * @n: The extent length
> + * @rbi: address of the pointer to the bitmap containing the block found
>   *
>   * Walk rgrp's bitmap to find bits that represent a block in @old_state.
> - * Add the found bitmap buffer to the transaction.
> - * Set the found bits to @new_state to change block's allocation state.
>   *
>   * This function never fails, because we wouldn't call it unless we
>   * know (from reservation results, etc.) that a block is available.
>   *
> - * Scope of @goal and returned block is just within rgrp, not the whole
> - * filesystem.
> + * Scope of @goal is just within rgrp, not the whole filesystem.
> + * Scope of @returned block is just within bitmap, not the whole filesystem.
>   *
> - * Returns:  the block number allocated
> + * Returns: the block number found relative to the bitmap rbi
>   */
>  
>  static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
> -			unsigned char old_state, bool dinode, unsigned int *n)
> +			unsigned char old_state, bool dinode,
> +			struct gfs2_bitmap **rbi)
>  {
>  	struct gfs2_bitmap *bi = NULL;
>  	const u32 length = rgd->rd_length;
>  	u32 blk = BFITNOENT;
>  	unsigned int buf, x;
> -	const unsigned int elen = *n;
>  	const u8 *buffer = NULL;
>  
> -	*n = 0;
> +	*rbi = NULL;
>  	/* Find bitmap block that contains bits for goal block */
>  	for (buf = 0; buf < length; buf++) {
>  		bi = rgd->rd_bits + buf;
> @@ -1187,12 +1185,31 @@ skip:
>  		goal = 0;
>  	}
>  
> -	if (blk == BFITNOENT)
> -		return blk;
> +	if (blk != BFITNOENT)
> +		*rbi = bi;
> +
Since bi is set to NULL above, this could presumably be unconditional

> +	return blk;
> +}
>  
> -	if (old_state == GFS2_BLKST_UNLINKED)
> -		goto out;
> +/**
> + * gfs2_alloc_extent - allocate an extent from a given bitmap
> + * @rgd: the resource group descriptor
> + * @bi: the bitmap within the rgrp
> + * @blk: the block within the bitmap
> + * @dinode: TRUE if the first block we allocate is for a dinode
> + * @n: The extent length
> + *
> + * Add the found bitmap buffer to the transaction.
> + * Set the found bits to @new_state to change block's allocation state.
> + */
> +static void gfs2_alloc_extent(struct gfs2_rgrpd *rgd, struct gfs2_bitmap *bi,
> +			      u32 blk, bool dinode, unsigned int *n)
> +{
> +	const unsigned int elen = *n;
> +	u32 goal;
> +	const u8 *buffer = NULL;
>  
> +	buffer = bi->bi_bh->b_data + bi->bi_offset;
>  	gfs2_trans_add_bh(rgd->rd_gl, bi->bi_bh, 1);
>  	gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
>  		    bi, blk, dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED);
> @@ -1210,8 +1227,6 @@ skip:
>  			    bi, goal, GFS2_BLKST_USED);
>  		(*n)++;
>  	}
> -out:
> -	return (bi->bi_start * GFS2_NBBY) + blk;
>  }
>  
>  /**
> @@ -1319,6 +1334,7 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *ndata,
>  	u32 goal, extlen, blk; /* block, within the rgrp scope */
>  	u64 block; /* block, within the file system scope */
>  	int error;
> +	struct gfs2_bitmap *bi;
>  
>  	/* Only happens if there is a bug in gfs2, return something distinctive
>  	 * to ensure that it is noticed.
> @@ -1333,12 +1349,16 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *ndata,
>  	else
>  		goal = rgd->rd_last_alloc;
>  
> -	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, dinode, ndata);
> +	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, dinode, &bi);
>  
> +	*ndata = 0;
>  	/* Since all blocks are reserved in advance, this shouldn't happen */
>  	if (blk == BFITNOENT)
>  		goto rgrp_error;
>  
> +	gfs2_alloc_extent(rgd, bi, blk, dinode, ndata);
> +
> +	blk = GFS2_BI2RGD_BLK(bi, blk);
>  	rgd->rd_last_alloc = blk;
>  	block = rgd->rd_data0 + blk;
If rd_last_alloc was set in gfs2_alloc_extent() then it could be set
(correctly) to the end of the extent, and not the beginning. Also
gfs2_alloc_extent could then return the starting block of the extent, so
these lines would just become:

block = gfs2_alloc_extent(rgd, bi, blk, dinode, ndata);

which would be a bit neater.

>  	if (!dinode) {
> diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
> index b3b61b8..aacb3fd 100644
> --- a/fs/gfs2/rgrp.h
> +++ b/fs/gfs2/rgrp.h
> @@ -12,6 +12,8 @@
>  
>  #include <linux/slab.h>
>  
> +#define GFS2_BI2RGD_BLK(bi, blk) (bi->bi_start * GFS2_NBBY) + blk
> +

Could you make this an inline function in rgrp.c, since it is not needed
outside of that file? A macro written like this can potentially have
some nasty side effects, as there are not enough brackets in the
expression,

Steve.


>  struct gfs2_rgrpd;
>  struct gfs2_sbd;
>  struct gfs2_holder;

[Cluster-devel] [GFS2 PATCH TRY2] GFS2: split function rgblk_search

[Bob Peterson]

Hi,

----- Original Message -----
| Since bi is set to NULL above, this could presumably be unconditional

Yes, bi is initialized to NULL, but it changes within the loop.
I was trying to avoid cases where an error condition occurs
and an extent is not found (as remote as the possibility is)
and yet the last bi is still returned and may be fiddled with.
Since the possibility is remote, I can still remove the if,
or I can leave it in for that reason. Your choice.

I've implemented your other suggestions, and here is the revised patch:

Regards,

Bob Peterson
Red Hat File Systems

Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
--
gfs2: split function rgblk_search

This patch splits function rgblk_search into a function that finds
blocks to allocate (rgblk_search) and a function that assigns those
blocks (gfs2_alloc_extent).

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index f1d1960..6b6cc09 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -65,8 +65,8 @@ static const char valid_change[16] = {
 };
 
 static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
-                        unsigned char old_state, bool dinode,
-			unsigned int *ndata);
+			unsigned char old_state, bool dinode,
+			struct gfs2_bitmap **rbi);
 
 /**
  * gfs2_setbit - Set a bit in the bitmaps
@@ -899,6 +899,11 @@ static int try_rgrp_fit(const struct gfs2_rgrpd *rgd, const struct gfs2_inode *i
 	return 0;
 }
 
+static inline u32 gfs2_bi2rgd_blk(struct gfs2_bitmap *bi, u32 blk)
+{
+	return (bi->bi_start * GFS2_NBBY) + blk;
+}
+
 /**
  * try_rgrp_unlink - Look for any unlinked, allocated, but unused inodes
  * @rgd: The rgrp
@@ -912,19 +917,20 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skip
 	u32 goal = 0, block;
 	u64 no_addr;
 	struct gfs2_sbd *sdp = rgd->rd_sbd;
-	unsigned int n;
 	struct gfs2_glock *gl;
 	struct gfs2_inode *ip;
 	int error;
 	int found = 0;
+	struct gfs2_bitmap *bi;
 
 	while (goal < rgd->rd_data) {
 		down_write(&sdp->sd_log_flush_lock);
-		n = 1;
-		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED, 0, &n);
+		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED, 0, &bi);
 		up_write(&sdp->sd_log_flush_lock);
 		if (block == BFITNOENT)
 			break;
+
+		block = gfs2_bi2rgd_blk(bi, block);
 		/* rgblk_search can return a block < goal, so we need to
 		   keep it marching forward. */
 		no_addr = block + rgd->rd_data0;
@@ -1109,38 +1115,35 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
 }
 
 /**
- * rgblk_search - find a block in @old_state, change allocation
- *           state to @new_state
+ * rgblk_search - find a block in @old_state
  * @rgd: the resource group descriptor
  * @goal: the goal block within the RG (start here to search for avail block)
  * @old_state: GFS2_BLKST_XXX the before-allocation state to find
  * @dinode: TRUE if the first block we allocate is for a dinode
- * @n: The extent length
+ * @rbi: address of the pointer to the bitmap containing the block found
  *
  * Walk rgrp's bitmap to find bits that represent a block in @old_state.
- * Add the found bitmap buffer to the transaction.
- * Set the found bits to @new_state to change block's allocation state.
  *
  * This function never fails, because we wouldn't call it unless we
  * know (from reservation results, etc.) that a block is available.
  *
- * Scope of @goal and returned block is just within rgrp, not the whole
- * filesystem.
+ * Scope of @goal is just within rgrp, not the whole filesystem.
+ * Scope of @returned block is just within bitmap, not the whole filesystem.
  *
- * Returns:  the block number allocated
+ * Returns: the block number found relative to the bitmap rbi
  */
 
 static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
-			unsigned char old_state, bool dinode, unsigned int *n)
+			unsigned char old_state, bool dinode,
+			struct gfs2_bitmap **rbi)
 {
 	struct gfs2_bitmap *bi = NULL;
 	const u32 length = rgd->rd_length;
 	u32 blk = BFITNOENT;
 	unsigned int buf, x;
-	const unsigned int elen = *n;
 	const u8 *buffer = NULL;
 
-	*n = 0;
+	*rbi = NULL;
 	/* Find bitmap block that contains bits for goal block */
 	for (buf = 0; buf < length; buf++) {
 		bi = rgd->rd_bits + buf;
@@ -1187,12 +1190,32 @@ skip:
 		goal = 0;
 	}
 
-	if (blk == BFITNOENT)
-		return blk;
+	if (blk != BFITNOENT)
+		*rbi = bi;
 
-	if (old_state == GFS2_BLKST_UNLINKED)
-		goto out;
+	return blk;
+}
 
+/**
+ * gfs2_alloc_extent - allocate an extent from a given bitmap
+ * @rgd: the resource group descriptor
+ * @bi: the bitmap within the rgrp
+ * @blk: the block within the bitmap
+ * @dinode: TRUE if the first block we allocate is for a dinode
+ * @n: The extent length
+ *
+ * Add the found bitmap buffer to the transaction.
+ * Set the found bits to @new_state to change block's allocation state.
+ * Returns: starting block number of the extent (fs scope)
+ */
+static u64 gfs2_alloc_extent(struct gfs2_rgrpd *rgd, struct gfs2_bitmap *bi,
+			     u32 blk, bool dinode, unsigned int *n)
+{
+	const unsigned int elen = *n;
+	u32 goal;
+	const u8 *buffer = NULL;
+
+	buffer = bi->bi_bh->b_data + bi->bi_offset;
 	gfs2_trans_add_bh(rgd->rd_gl, bi->bi_bh, 1);
 	gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
 		    bi, blk, dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED);
@@ -1210,8 +1233,9 @@ skip:
 			    bi, goal, GFS2_BLKST_USED);
 		(*n)++;
 	}
-out:
-	return (bi->bi_start * GFS2_NBBY) + blk;
+	blk = gfs2_bi2rgd_blk(bi, blk);
+	rgd->rd_last_alloc = blk;
+	return rgd->rd_data0 + blk;
 }
 
 /**
@@ -1319,6 +1343,7 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *ndata,
 	u32 goal, extlen, blk; /* block, within the rgrp scope */
 	u64 block; /* block, within the file system scope */
 	int error;
+	struct gfs2_bitmap *bi;
 
 	/* Only happens if there is a bug in gfs2, return something distinctive
 	 * to ensure that it is noticed.
@@ -1333,14 +1358,15 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *ndata,
 	else
 		goal = rgd->rd_last_alloc;
 
-	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, dinode, ndata);
+	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, dinode, &bi);
 
+	*ndata = 0;
 	/* Since all blocks are reserved in advance, this shouldn't happen */
 	if (blk == BFITNOENT)
 		goto rgrp_error;
 
-	rgd->rd_last_alloc = blk;
-	block = rgd->rd_data0 + blk;
+	block = gfs2_alloc_extent(rgd, bi, blk, dinode, ndata);
+
 	if (!dinode) {
 		ip->i_goal = block + *ndata - 1;
 		error = gfs2_meta_inode_buffer(ip, &dibh);

[Cluster-devel] [GFS2 PATCH TRY2] GFS2: split function rgblk_search

[Steven Whitehouse]

Hi,

I've applied this to the -nmw tree now. Thanks,

Steve.

On Mon, 2011-11-21 at 11:47 -0500, Bob Peterson wrote:
> Hi,
> 
> ----- Original Message -----
> | Since bi is set to NULL above, this could presumably be unconditional
> 
> Yes, bi is initialized to NULL, but it changes within the loop.
> I was trying to avoid cases where an error condition occurs
> and an extent is not found (as remote as the possibility is)
> and yet the last bi is still returned and may be fiddled with.
> Since the possibility is remote, I can still remove the if,
> or I can leave it in for that reason. Your choice.
> 
> I've implemented your other suggestions, and here is the revised patch:
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com> 
> --
> gfs2: split function rgblk_search
> 
> This patch splits function rgblk_search into a function that finds
> blocks to allocate (rgblk_search) and a function that assigns those
> blocks (gfs2_alloc_extent).
> 
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index f1d1960..6b6cc09 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -65,8 +65,8 @@ static const char valid_change[16] = {
>  };
>  
>  static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
> -                        unsigned char old_state, bool dinode,
> -			unsigned int *ndata);
> +			unsigned char old_state, bool dinode,
> +			struct gfs2_bitmap **rbi);
>  
>  /**
>   * gfs2_setbit - Set a bit in the bitmaps
> @@ -899,6 +899,11 @@ static int try_rgrp_fit(const struct gfs2_rgrpd *rgd, const struct gfs2_inode *i
>  	return 0;
>  }
>  
> +static inline u32 gfs2_bi2rgd_blk(struct gfs2_bitmap *bi, u32 blk)
> +{
> +	return (bi->bi_start * GFS2_NBBY) + blk;
> +}
> +
>  /**
>   * try_rgrp_unlink - Look for any unlinked, allocated, but unused inodes
>   * @rgd: The rgrp
> @@ -912,19 +917,20 @@ static void try_rgrp_unlink(struct gfs2_rgrpd *rgd, u64 *last_unlinked, u64 skip
>  	u32 goal = 0, block;
>  	u64 no_addr;
>  	struct gfs2_sbd *sdp = rgd->rd_sbd;
> -	unsigned int n;
>  	struct gfs2_glock *gl;
>  	struct gfs2_inode *ip;
>  	int error;
>  	int found = 0;
> +	struct gfs2_bitmap *bi;
>  
>  	while (goal < rgd->rd_data) {
>  		down_write(&sdp->sd_log_flush_lock);
> -		n = 1;
> -		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED, 0, &n);
> +		block = rgblk_search(rgd, goal, GFS2_BLKST_UNLINKED, 0, &bi);
>  		up_write(&sdp->sd_log_flush_lock);
>  		if (block == BFITNOENT)
>  			break;
> +
> +		block = gfs2_bi2rgd_blk(bi, block);
>  		/* rgblk_search can return a block < goal, so we need to
>  		   keep it marching forward. */
>  		no_addr = block + rgd->rd_data0;
> @@ -1109,38 +1115,35 @@ static unsigned char gfs2_get_block_type(struct gfs2_rgrpd *rgd, u64 block)
>  }
>  
>  /**
> - * rgblk_search - find a block in @old_state, change allocation
> - *           state to @new_state
> + * rgblk_search - find a block in @old_state
>   * @rgd: the resource group descriptor
>   * @goal: the goal block within the RG (start here to search for avail block)
>   * @old_state: GFS2_BLKST_XXX the before-allocation state to find
>   * @dinode: TRUE if the first block we allocate is for a dinode
> - * @n: The extent length
> + * @rbi: address of the pointer to the bitmap containing the block found
>   *
>   * Walk rgrp's bitmap to find bits that represent a block in @old_state.
> - * Add the found bitmap buffer to the transaction.
> - * Set the found bits to @new_state to change block's allocation state.
>   *
>   * This function never fails, because we wouldn't call it unless we
>   * know (from reservation results, etc.) that a block is available.
>   *
> - * Scope of @goal and returned block is just within rgrp, not the whole
> - * filesystem.
> + * Scope of @goal is just within rgrp, not the whole filesystem.
> + * Scope of @returned block is just within bitmap, not the whole filesystem.
>   *
> - * Returns:  the block number allocated
> + * Returns: the block number found relative to the bitmap rbi
>   */
>  
>  static u32 rgblk_search(struct gfs2_rgrpd *rgd, u32 goal,
> -			unsigned char old_state, bool dinode, unsigned int *n)
> +			unsigned char old_state, bool dinode,
> +			struct gfs2_bitmap **rbi)
>  {
>  	struct gfs2_bitmap *bi = NULL;
>  	const u32 length = rgd->rd_length;
>  	u32 blk = BFITNOENT;
>  	unsigned int buf, x;
> -	const unsigned int elen = *n;
>  	const u8 *buffer = NULL;
>  
> -	*n = 0;
> +	*rbi = NULL;
>  	/* Find bitmap block that contains bits for goal block */
>  	for (buf = 0; buf < length; buf++) {
>  		bi = rgd->rd_bits + buf;
> @@ -1187,12 +1190,32 @@ skip:
>  		goal = 0;
>  	}
>  
> -	if (blk == BFITNOENT)
> -		return blk;
> +	if (blk != BFITNOENT)
> +		*rbi = bi;
>  
> -	if (old_state == GFS2_BLKST_UNLINKED)
> -		goto out;
> +	return blk;
> +}
>  
> +/**
> + * gfs2_alloc_extent - allocate an extent from a given bitmap
> + * @rgd: the resource group descriptor
> + * @bi: the bitmap within the rgrp
> + * @blk: the block within the bitmap
> + * @dinode: TRUE if the first block we allocate is for a dinode
> + * @n: The extent length
> + *
> + * Add the found bitmap buffer to the transaction.
> + * Set the found bits to @new_state to change block's allocation state.
> + * Returns: starting block number of the extent (fs scope)
> + */
> +static u64 gfs2_alloc_extent(struct gfs2_rgrpd *rgd, struct gfs2_bitmap *bi,
> +			     u32 blk, bool dinode, unsigned int *n)
> +{
> +	const unsigned int elen = *n;
> +	u32 goal;
> +	const u8 *buffer = NULL;
> +
> +	buffer = bi->bi_bh->b_data + bi->bi_offset;
>  	gfs2_trans_add_bh(rgd->rd_gl, bi->bi_bh, 1);
>  	gfs2_setbit(rgd, bi->bi_bh->b_data, bi->bi_clone, bi->bi_offset,
>  		    bi, blk, dinode ? GFS2_BLKST_DINODE : GFS2_BLKST_USED);
> @@ -1210,8 +1233,9 @@ skip:
>  			    bi, goal, GFS2_BLKST_USED);
>  		(*n)++;
>  	}
> -out:
> -	return (bi->bi_start * GFS2_NBBY) + blk;
> +	blk = gfs2_bi2rgd_blk(bi, blk);
> +	rgd->rd_last_alloc = blk;
> +	return rgd->rd_data0 + blk;
>  }
>  
>  /**
> @@ -1319,6 +1343,7 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *ndata,
>  	u32 goal, extlen, blk; /* block, within the rgrp scope */
>  	u64 block; /* block, within the file system scope */
>  	int error;
> +	struct gfs2_bitmap *bi;
>  
>  	/* Only happens if there is a bug in gfs2, return something distinctive
>  	 * to ensure that it is noticed.
> @@ -1333,14 +1358,15 @@ int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *ndata,
>  	else
>  		goal = rgd->rd_last_alloc;
>  
> -	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, dinode, ndata);
> +	blk = rgblk_search(rgd, goal, GFS2_BLKST_FREE, dinode, &bi);
>  
> +	*ndata = 0;
>  	/* Since all blocks are reserved in advance, this shouldn't happen */
>  	if (blk == BFITNOENT)
>  		goto rgrp_error;
>  
> -	rgd->rd_last_alloc = blk;
> -	block = rgd->rd_data0 + blk;
> +	block = gfs2_alloc_extent(rgd, bi, blk, dinode, ndata);
> +
>  	if (!dinode) {
>  		ip->i_goal = block + *ndata - 1;
>  		error = gfs2_meta_inode_buffer(ip, &dibh);