[Cluster-devel] [GFS2] fsck.gfs2: replace recent i_goal fixes with simple logic
This patch reverses the recent set of i_goal fixes for fsck.gfs2.
This is because of two problems.
1. It is not possible to determine if a valid block within the fs
is the correct goal block for a given inode.
2. Conversely, given an inode, it is also not possible to accurately
determine what its goal block should be.
The previous patches assumed that the last block of a file is its
goal block, but that is not true if the file is a directory or if
its blocks are not allocated sequentially. fsck.gfs2 would flag
these inodes incorrectly as having bad i_goal values.
This patch takes a simple approach. It checks if the i_goal of a
given inode is out of bounds of the fs. If so, we can be certain
that it is wrong and we set it to the inode metadata block. This
is a safe starting point for gfs2 to determine where to allocate
from next.
Resolves: rhbz#1186515
Signed-off-by: Abhi Das a...@redhat.com
---
gfs2/fsck/metawalk.c | 92 +++---
gfs2/fsck/metawalk.h | 5 ---
gfs2/fsck/pass1.c | 35 ---
gfs2/libgfs2/libgfs2.h | 1 -
4 files changed, 34 insertions(+), 99 deletions(-)
diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index f05fb51..4d5a660 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -1428,8 +1428,7 @@ static int build_and_check_metalist(struct gfs2_inode
*ip, osi_list_t *mlp,
*/
static int check_data(struct gfs2_inode *ip, struct metawalk_fxns *pass,
struct gfs2_buffer_head *bh, int head_size,
- uint64_t *last_block, uint64_t *blks_checked,
- uint64_t *error_blk)
+ uint64_t *blks_checked, uint64_t *error_blk)
{
int error = 0, rc = 0;
uint64_t block, *ptr;
@@ -1444,7 +1443,7 @@ static int check_data(struct gfs2_inode *ip, struct
metawalk_fxns *pass,
if (skip_this_pass || fsck_abort)
return error;
- *last_block = block = be64_to_cpu(*ptr);
+ block = be64_to_cpu(*ptr);
/* It's important that we don't call valid_block() and
bypass calling check_data on invalid blocks because that
would defeat the rangecheck_block related functions in
@@ -1548,15 +1547,12 @@ int check_metatree(struct gfs2_inode *ip, struct
metawalk_fxns *pass)
struct gfs2_buffer_head *bh;
uint32_t height = ip-i_di.di_height;
int i, head_size;
- uint64_t blks_checked = 0, last_block = 0;
+ uint64_t blks_checked = 0;
int error, rc;
int metadata_clean = 0;
uint64_t error_blk = 0;
int hit_error_blk = 0;
- if (!height pass-check_i_goal)
- pass-check_i_goal(ip, ip-i_di.di_num.no_addr,
- pass-private);
if (!height !is_dir(ip-i_di, ip-i_sbd-gfs1))
return 0;
@@ -1575,9 +1571,6 @@ int check_metatree(struct gfs2_inode *ip, struct
metawalk_fxns *pass)
* comprise the directory hash table, so we perform the directory
* checks and exit. */
if (is_dir(ip-i_di, ip-i_sbd-gfs1)) {
- last_block = ip-i_di.di_num.no_addr;
- if (pass-check_i_goal)
- pass-check_i_goal(ip, last_block, pass-private);
if (!(ip-i_di.di_flags GFS2_DIF_EXHASH))
goto out;
/* check validity of leaf blocks and leaf chains */
@@ -1604,7 +1597,7 @@ int check_metatree(struct gfs2_inode *ip, struct
metawalk_fxns *pass)
if (pass-check_data)
error = check_data(ip, pass, bh, head_size,
- last_block, blks_checked,
error_blk);
+ blks_checked, error_blk);
if (pass-big_file_msg ip-i_di.di_blocks COMFORTABLE_BLKS)
pass-big_file_msg(ip, blks_checked);
}
@@ -1616,8 +1609,6 @@ int check_metatree(struct gfs2_inode *ip, struct
metawalk_fxns *pass)
(unsigned long long)ip-i_di.di_num.no_addr);
fflush(stdout);
}
- if (!error pass-check_i_goal)
- pass-check_i_goal(ip, last_block, pass-private);
undo_metalist:
if (!error)
goto out;
@@ -1958,80 +1949,6 @@ static int alloc_leaf(struct gfs2_inode *ip, uint64_t
block, void *private)
return 0;
}
-/**
- * rgrp_contains_block - Check if the rgrp provided contains the
- * given block. Taken directly from the gfs2 kernel code
- * @rgd: The rgrp to search within
- * @block: The block to search for
- *
- * Returns: 1 if present, 0 if not.
- */
-static inline int rgrp_contains_block(struct rgrp_tree *rgd, uint64_t block)
-{
- uint64_t first = rgd-ri.ri_data0;
- uint64_t last = first + rgd-ri.ri_data;
- return first = block block last;
-}
-
-/**
- * check_i_goal
- * @ip
Re: [Cluster-devel] [GFS2] fsck.gfs2: replace recent i_goal fixes with simple logic
Hi Abhi,
On 15/04/15 14:37, Abhi Das wrote:
This patch reverses the recent set of i_goal fixes for fsck.gfs2.
This is because of two problems.
1. It is not possible to determine if a valid block within the fs
is the correct goal block for a given inode.
2. Conversely, given an inode, it is also not possible to accurately
determine what its goal block should be.
The previous patches assumed that the last block of a file is its
goal block, but that is not true if the file is a directory or if
its blocks are not allocated sequentially. fsck.gfs2 would flag
these inodes incorrectly as having bad i_goal values.
This patch takes a simple approach. It checks if the i_goal of a
given inode is out of bounds of the fs. If so, we can be certain
that it is wrong and we set it to the inode metadata block. This
is a safe starting point for gfs2 to determine where to allocate
from next.
Resolves: rhbz#1186515
Signed-off-by: Abhi Das a...@redhat.com
This looks good to me, and it fixes the test failures that I was seeing
due to the non-sequential last block issue.
Thanks,
Andy
---
gfs2/fsck/metawalk.c | 92 +++---
gfs2/fsck/metawalk.h | 5 ---
gfs2/fsck/pass1.c | 35 ---
gfs2/libgfs2/libgfs2.h | 1 -
4 files changed, 34 insertions(+), 99 deletions(-)
diff --git a/gfs2/fsck/metawalk.c b/gfs2/fsck/metawalk.c
index f05fb51..4d5a660 100644
--- a/gfs2/fsck/metawalk.c
+++ b/gfs2/fsck/metawalk.c
@@ -1428,8 +1428,7 @@ static int build_and_check_metalist(struct gfs2_inode
*ip, osi_list_t *mlp,
*/
static int check_data(struct gfs2_inode *ip, struct metawalk_fxns *pass,
struct gfs2_buffer_head *bh, int head_size,
- uint64_t *last_block, uint64_t *blks_checked,
- uint64_t *error_blk)
+ uint64_t *blks_checked, uint64_t *error_blk)
{
int error = 0, rc = 0;
uint64_t block, *ptr;
@@ -1444,7 +1443,7 @@ static int check_data(struct gfs2_inode *ip, struct
metawalk_fxns *pass,
if (skip_this_pass || fsck_abort)
return error;
- *last_block = block = be64_to_cpu(*ptr);
+ block = be64_to_cpu(*ptr);
/* It's important that we don't call valid_block() and
bypass calling check_data on invalid blocks because that
would defeat the rangecheck_block related functions in
@@ -1548,15 +1547,12 @@ int check_metatree(struct gfs2_inode *ip, struct
metawalk_fxns *pass)
struct gfs2_buffer_head *bh;
uint32_t height = ip-i_di.di_height;
int i, head_size;
- uint64_t blks_checked = 0, last_block = 0;
+ uint64_t blks_checked = 0;
int error, rc;
int metadata_clean = 0;
uint64_t error_blk = 0;
int hit_error_blk = 0;
- if (!height pass-check_i_goal)
- pass-check_i_goal(ip, ip-i_di.di_num.no_addr,
- pass-private);
if (!height !is_dir(ip-i_di, ip-i_sbd-gfs1))
return 0;
@@ -1575,9 +1571,6 @@ int check_metatree(struct gfs2_inode *ip, struct
metawalk_fxns *pass)
* comprise the directory hash table, so we perform the directory
* checks and exit. */
if (is_dir(ip-i_di, ip-i_sbd-gfs1)) {
- last_block = ip-i_di.di_num.no_addr;
- if (pass-check_i_goal)
- pass-check_i_goal(ip, last_block, pass-private);
if (!(ip-i_di.di_flags GFS2_DIF_EXHASH))
goto out;
/* check validity of leaf blocks and leaf chains */
@@ -1604,7 +1597,7 @@ int check_metatree(struct gfs2_inode *ip, struct
metawalk_fxns *pass)
if (pass-check_data)
error = check_data(ip, pass, bh, head_size,
- last_block, blks_checked,
error_blk);
+ blks_checked, error_blk);
if (pass-big_file_msg ip-i_di.di_blocks COMFORTABLE_BLKS)
pass-big_file_msg(ip, blks_checked);
}
@@ -1616,8 +1609,6 @@ int check_metatree(struct gfs2_inode *ip, struct
metawalk_fxns *pass)
(unsigned long long)ip-i_di.di_num.no_addr);
fflush(stdout);
}
- if (!error pass-check_i_goal)
- pass-check_i_goal(ip, last_block, pass-private);
undo_metalist:
if (!error)
goto out;
@@ -1958,80 +1949,6 @@ static int alloc_leaf(struct gfs2_inode *ip, uint64_t
block, void *private)
return 0;
}
-/**
- * rgrp_contains_block - Check if the rgrp provided contains the
- * given block. Taken directly from the gfs2 kernel code
- * @rgd: The rgrp to search within
- * @block: The block to search for
- *
- * Returns: 1 if present, 0 if not.
- */
-static inline int rgrp_contains_block(struct rgrp_tree
