[Cluster-devel] [PATCH 1/3] gfs2-utils: Fix two logic errors

[Cluster-devel] [PATCH 1/3] gfs2-utils: Fix two logic errors

[Andrew Price]

Two uses of lgfs2_get_leaf_ptr() were still expecting it to return 0 on
failure. Remove the negations.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/edit/savemeta.c | 2 +-
 gfs2/fsck/pass2.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
index e336100..a43045f 100644
--- a/gfs2/edit/savemeta.c
+++ b/gfs2/edit/savemeta.c
@@ -542,7 +542,7 @@ static void save_inode_data(struct metafd *mfd)
 		int li;
 
 		for (li = 0; li < (1 << inode->i_di.di_depth); li++) {
-			if (!lgfs2_get_leaf_ptr(inode, li, &leaf_no)) {
+			if (lgfs2_get_leaf_ptr(inode, li, &leaf_no)) {
 				fprintf(stderr, "Could not read leaf index %d in dinode %"PRIu64"\n", li,
 				        (uint64_t)inode->i_di.di_num.no_addr);
 				exit(-1);
diff --git a/gfs2/fsck/pass2.c b/gfs2/fsck/pass2.c
index 2474ccb..2971b8c 100644
--- a/gfs2/fsck/pass2.c
+++ b/gfs2/fsck/pass2.c
@@ -370,7 +370,7 @@ static int wrong_leaf(struct gfs2_inode *ip, struct gfs2_inum *entry,
 		    de->de_type) == 0) {
 		log_err(_("The misplaced directory entry was moved to a "
 			  "valid leaf block.\n"));
-		if (!lgfs2_get_leaf_ptr(ip, hash_index, &real_leaf)) {
+		if (lgfs2_get_leaf_ptr(ip, hash_index, &real_leaf)) {
 			log_err(_("Could not read leaf %d in dinode %"PRIu64": %s\n"), hash_index,
 			        (uint64_t)ip->i_di.di_num.no_addr, strerror(errno));
 		}
-- 
1.9.3

[Cluster-devel] [PATCH 2/3] gfs2_edit: Ensure all leaf blocks in per_node are saved

[Andrew Price]

block_is_in_per_node() only checks the first leaf block when the
per_node directory has been split (when the block size is small and/or
the number of journals is large). This caused savemeta to treat the
files in per_node as user data and so didn't save their metadata. Add an
outer loop to iterate over the indirect blocks.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/edit/savemeta.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
index a43045f..360b144 100644
--- a/gfs2/edit/savemeta.c
+++ b/gfs2/edit/savemeta.c
@@ -75,7 +75,7 @@ static int block_is_a_journal(void)
 
 static int block_is_in_per_node(void)
 {
-	int d;
+	int i;
 	struct gfs2_inode *per_node_di;
 
 	if (sbd.gfs1)
@@ -90,9 +90,12 @@ static int block_is_in_per_node(void)
 	do_dinode_extended(&per_node_di->i_di, per_node_di->i_bh);
 	inode_put(&per_node_di);
 
-	for (d = 0; d < indirect->ii[0].dirents; d++) {
-		if (block == indirect->ii[0].dirent[d].block)
-			return TRUE;
+	for (i = 0; i < indirect_blocks; i++) {
+		int d;
+		for (d = 0; d < indirect->ii[i].dirents; d++) {
+			if (block == indirect->ii[i].dirent[d].block)
+				return TRUE;
+		}
 	}
 	return FALSE;
 }
-- 
1.9.3

[Cluster-devel] [PATCH 3/3] gfs2-utils tests: Add small-block savemeta tests

[Andrew Price]

Add some savemeta/restoremeta test scenarios which were used to discover
some recently fixed bugs, to make sure they stay fixed. They mainly cover
scenarios relating to small block sizes.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 tests/edit.at | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/tests/edit.at b/tests/edit.at
index e2a702c..49f4a58 100644
--- a/tests/edit.at
+++ b/tests/edit.at
@@ -1,7 +1,7 @@
 AT_TESTED([gfs2_edit])
 AT_BANNER([gfs2_edit tests])
 
-AT_SETUP([Savemeta/restoremeta, defaults])
+AT_SETUP([Save/restoremeta, defaults])
 GFS_TGT_REGEN
 AT_CHECK([$GFS_MKFS -p lock_nolock $GFS_TGT $(($(gfs_max_blocks 4096)/2))], 0, [ignore], [ignore])
 AT_CHECK([gfs2_edit savemeta $GFS_TGT test.meta > savemeta.log], 0, [ignore], [ignore])
@@ -13,10 +13,38 @@ AT_CHECK([gfs2_edit restoremeta test.meta $GFS_TGT], 0, [ignore], [ignore])
 AT_CHECK([fsck.gfs2 -n $GFS_TGT], 0, [ignore], [ignore])
 AT_CLEANUP
 
-AT_SETUP([Savemeta/restoremeta, no compression])
+AT_SETUP([Save/restoremeta, no compression])
 GFS_TGT_REGEN
 AT_CHECK([$GFS_MKFS -p lock_nolock $GFS_TGT $(($(gfs_max_blocks 4096)/2))], 0, [ignore], [ignore])
 AT_CHECK([gfs2_edit savemeta -z0 $GFS_TGT test.meta], 0, [ignore], [ignore])
+GFS_TGT_REGEN
+AT_CHECK([gfs2_edit restoremeta test.meta $GFS_TGT], 0, [ignore], [ignore])
+AT_CHECK([fsck.gfs2 -n $GFS_TGT], 0, [ignore], [ignore])
+AT_CLEANUP
+
+AT_SETUP([Save/restoremeta, min. block size])
+GFS_TGT_REGEN
+AT_CHECK([$GFS_MKFS -p lock_nolock -b512 $GFS_TGT], 0, [ignore], [ignore])
+AT_CHECK([gfs2_edit savemeta -z0 $GFS_TGT test.meta], 0, [ignore], [ignore])
+GFS_TGT_REGEN
+AT_CHECK([gfs2_edit restoremeta test.meta $GFS_TGT], 0, [ignore], [ignore])
+AT_CHECK([fsck.gfs2 -n $GFS_TGT], 0, [ignore], [ignore])
+AT_CLEANUP
+
+AT_SETUP([Save/restoremeta, 4 journals])
+GFS_TGT_REGEN
+AT_CHECK([$GFS_MKFS -p lock_nolock -j4 -J8 $GFS_TGT], 0, [ignore], [ignore])
+AT_CHECK([gfs2_edit savemeta -z0 $GFS_TGT test.meta], 0, [ignore], [ignore])
+GFS_TGT_REGEN
+AT_CHECK([gfs2_edit restoremeta test.meta $GFS_TGT], 0, [ignore], [ignore])
+AT_CHECK([fsck.gfs2 -n $GFS_TGT], 0, [ignore], [ignore])
+AT_CLEANUP
+
+AT_SETUP([Save/restoremeta, min. block size, 4 journals])
+GFS_TGT_REGEN
+AT_CHECK([$GFS_MKFS -p lock_nolock -b512 -j4 -J8 $GFS_TGT], 0, [ignore], [ignore])
+AT_CHECK([gfs2_edit savemeta -z0 $GFS_TGT test.meta], 0, [ignore], [ignore])
+GFS_TGT_REGEN
 AT_CHECK([gfs2_edit restoremeta test.meta $GFS_TGT], 0, [ignore], [ignore])
 AT_CHECK([fsck.gfs2 -n $GFS_TGT], 0, [ignore], [ignore])
 AT_CLEANUP
-- 
1.9.3

[Cluster-devel] [PATCH 2/3] gfs2_edit: Ensure all leaf blocks in per_node are saved

[Steven Whitehouse]

Hi,

On 05/06/14 02:28, Andrew Price wrote:
> block_is_in_per_node() only checks the first leaf block when the
> per_node directory has been split (when the block size is small and/or
> the number of journals is large). This caused savemeta to treat the
> files in per_node as user data and so didn't save their metadata. Add an
> outer loop to iterate over the indirect blocks.
Why does it matter whether the block in the per_node subdir or not? Are 
we doing something special compared with other directories?

Steve.

> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---
>   gfs2/edit/savemeta.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
> index a43045f..360b144 100644
> --- a/gfs2/edit/savemeta.c
> +++ b/gfs2/edit/savemeta.c
> @@ -75,7 +75,7 @@ static int block_is_a_journal(void)
>   
>   static int block_is_in_per_node(void)
>   {
> -	int d;
> +	int i;
>   	struct gfs2_inode *per_node_di;
>   
>   	if (sbd.gfs1)
> @@ -90,9 +90,12 @@ static int block_is_in_per_node(void)
>   	do_dinode_extended(&per_node_di->i_di, per_node_di->i_bh);
>   	inode_put(&per_node_di);
>   
> -	for (d = 0; d < indirect->ii[0].dirents; d++) {
> -		if (block == indirect->ii[0].dirent[d].block)
> -			return TRUE;
> +	for (i = 0; i < indirect_blocks; i++) {
> +		int d;
> +		for (d = 0; d < indirect->ii[i].dirents; d++) {
> +			if (block == indirect->ii[i].dirent[d].block)
> +				return TRUE;
> +		}
>   	}
>   	return FALSE;
>   }

[Cluster-devel] [PATCH 2/3] gfs2_edit: Ensure all leaf blocks in per_node are saved

[Andrew Price]

On 05/06/14 10:39, Steven Whitehouse wrote:
> Hi,
>
> On 05/06/14 02:28, Andrew Price wrote:
>> block_is_in_per_node() only checks the first leaf block when the
>> per_node directory has been split (when the block size is small and/or
>> the number of journals is large). This caused savemeta to treat the
>> files in per_node as user data and so didn't save their metadata. Add an
>> outer loop to iterate over the indirect blocks.
> Why does it matter whether the block in the per_node subdir or not? Are
> we doing something special compared with other directories?

Yes, sorry I didn't make it very clear in the commit log: gfs2_edit 
savemeta saves less metadata when a block isn't part of a set of 
"system" structures to avoid saving user data while gathering enough 
metadata to aid debugging.

   static int block_is_systemfile(void)
   {
           return block_is_jindex() || block_is_inum_file() ||
                   block_is_statfs_file() || block_is_quota_file() ||
                   block_is_rindex() || block_is_a_journal() ||
                   block_is_per_node() || block_is_in_per_node();
   }

save_inode_data() uses this function to decide the height at which to 
stop saving metadata.

https://git.fedorahosted.org/cgit/gfs2-utils.git/tree/gfs2/edit/savemeta.c#n499

Andy

>
> Steve.
>
>> Signed-off-by: Andrew Price <anprice@redhat.com>
>> ---
>>   gfs2/edit/savemeta.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/gfs2/edit/savemeta.c b/gfs2/edit/savemeta.c
>> index a43045f..360b144 100644
>> --- a/gfs2/edit/savemeta.c
>> +++ b/gfs2/edit/savemeta.c
>> @@ -75,7 +75,7 @@ static int block_is_a_journal(void)
>>   static int block_is_in_per_node(void)
>>   {
>> -    int d;
>> +    int i;
>>       struct gfs2_inode *per_node_di;
>>       if (sbd.gfs1)
>> @@ -90,9 +90,12 @@ static int block_is_in_per_node(void)
>>       do_dinode_extended(&per_node_di->i_di, per_node_di->i_bh);
>>       inode_put(&per_node_di);
>> -    for (d = 0; d < indirect->ii[0].dirents; d++) {
>> -        if (block == indirect->ii[0].dirent[d].block)
>> -            return TRUE;
>> +    for (i = 0; i < indirect_blocks; i++) {
>> +        int d;
>> +        for (d = 0; d < indirect->ii[i].dirents; d++) {
>> +            if (block == indirect->ii[i].dirent[d].block)
>> +                return TRUE;
>> +        }
>>       }
>>       return FALSE;
>>   }
>