[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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);
> 



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]