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 };
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;
/* 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)
----- 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 */
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,
----- 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 */
/**
* 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)
*/
+ 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;
----- 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(-)
----- 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
11-18-2011, 12:59 PM
Bob Peterson
GFS2: combine gfs2_alloc_block and gfs2_alloc_di
----- 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
01-05-2012, 10:51 AM
Steven Whitehouse
GFS2: combine gfs2_alloc_block and gfs2_alloc_di
From: Bob Peterson <rpeterso@redhat.com>
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.
Signed-off-by: Bob Peterson <rpeterso@redhat.com>
Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
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 };
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;
/* 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)
GFS2: combine gfs2_alloc_block and gfs2_alloc_di
It just seemed
