From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Wed, 16 Nov 2011 14:30:15 +0000 Subject: [Cluster-devel] [GFS2 PATCH] GFS2: combine gfs2_alloc_block and gfs2_alloc_di In-Reply-To: <9661058c-c3ea-4355-8548-243418b78703@zmail16.collab.prod.int.phx2.redhat.com> References: <9661058c-c3ea-4355-8548-243418b78703@zmail16.collab.prod.int.phx2.redhat.com> Message-ID: <1321453815.2713.64.camel@menhir> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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 > 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);