[v3,4/7] btrfs-progs: lowmem: search key of root again after check_fs_root() after repair
diff mbox series

Message ID 20180917072852.25831-5-suy.fnst@cn.fujitsu.com
State New
Headers show
Series
  • btrfs-progs: lowmem: bug fixes and inode_extref repair
Related show

Commit Message

Su Yue Sept. 17, 2018, 7:28 a.m. UTC
In check_fs_roots_lowmem(), we do search and follow the resulted path
to call check_fs_root(), then call btrfs_next_item() to check next
root.
However, if repair is enabled, the root tree can be cowed, the
existed path can cause strange errors.

Solution:
  If repair, save the key before calling check_fs_root,
  search the saved key again before checking next root.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 check/mode-lowmem.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Qu Wenruo Sept. 17, 2018, 12:51 p.m. UTC | #1
On 2018/9/17 下午3:28, Su Yue wrote:
> In check_fs_roots_lowmem(), we do search and follow the resulted path
> to call check_fs_root(), then call btrfs_next_item() to check next
> root.
> However, if repair is enabled, the root tree can be cowed, the
> existed path can cause strange errors.
> 
> Solution:
>   If repair, save the key before calling check_fs_root,
>   search the saved key again before checking next root.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>

The idea is pretty valid, however I'm not pretty sure about one practice
below.

> ---
>  check/mode-lowmem.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index 4db12cc7f9fe..db44456fd85b 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -4967,9 +4967,13 @@ int check_fs_roots_lowmem(struct btrfs_fs_info *fs_info)
>  	}
>  
>  	while (1) {
> +		struct btrfs_key saved_key;
> +
>  		node = path.nodes[0];
>  		slot = path.slots[0];
>  		btrfs_item_key_to_cpu(node, &key, slot);
> +		if (repair)
> +			saved_key = key;

Is this OK? Shouldn't it be something like memcpy(&saved_key, &key,
sizeof(key));

Or some new C standard make it work?

Although compiler doesn't give any warning on this.

Thanks,
Qu

>  		if (key.objectid > BTRFS_LAST_FREE_OBJECTID)
>  			goto out;
>  		if (key.type == BTRFS_ROOT_ITEM_KEY &&
> @@ -5000,6 +5004,24 @@ int check_fs_roots_lowmem(struct btrfs_fs_info *fs_info)
>  			err |= ret;
>  		}
>  next:
> +		/*
> +		 * Since root tree can be cowed during repair,
> +		 * here search the saved key again.
> +		 */
> +		if (repair) {
> +			btrfs_release_path(&path);
> +			ret = btrfs_search_slot(NULL, fs_info->tree_root,
> +						&saved_key, &path, 0, 0);
> +			/* Repair never deletes trees, search must succeed. */
> +			if (ret > 0)
> +				ret = -ENOENT;
> +			if (ret) {
> +				error(
> +			"search root key[%llu %u %llu] failed after repair: %s",
> +				      saved_key.objectid, saved_key.type,
> +				      saved_key.offset, strerror(-ret));
> +			}
> +		}
>  		ret = btrfs_next_item(tree_root, &path);
>  		if (ret > 0)
>  			goto out;
>
Su Yue Sept. 17, 2018, 12:55 p.m. UTC | #2
On 2018/9/17 8:51 PM, Qu Wenruo wrote:
> 
> 
> On 2018/9/17 下午3:28, Su Yue wrote:
>> In check_fs_roots_lowmem(), we do search and follow the resulted path
>> to call check_fs_root(), then call btrfs_next_item() to check next
>> root.
>> However, if repair is enabled, the root tree can be cowed, the
>> existed path can cause strange errors.
>>
>> Solution:
>>    If repair, save the key before calling check_fs_root,
>>    search the saved key again before checking next root.
>>
>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> 
> The idea is pretty valid, however I'm not pretty sure about one practice
> below.
> 
>> ---
>>   check/mode-lowmem.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
>> index 4db12cc7f9fe..db44456fd85b 100644
>> --- a/check/mode-lowmem.c
>> +++ b/check/mode-lowmem.c
>> @@ -4967,9 +4967,13 @@ int check_fs_roots_lowmem(struct btrfs_fs_info *fs_info)
>>   	}
>>   
>>   	while (1) {
>> +		struct btrfs_key saved_key;
>> +
>>   		node = path.nodes[0];
>>   		slot = path.slots[0];
>>   		btrfs_item_key_to_cpu(node, &key, slot);
>> +		if (repair)
>> +			saved_key = key;
> 
> Is this OK? Shouldn't it be something like memcpy(&saved_key, &key,
> sizeof(key));
> 
> Or some new C standard make it work?
> 
I don't quite know how C89 standard defines this behavior.
It should work in C99...

Seems you are not familiar with this style, will use memcpy.

Thanks,
Su
> Although compiler doesn't give any warning on this.
> 
> Thanks,
> Qu
> 
>>   		if (key.objectid > BTRFS_LAST_FREE_OBJECTID)
>>   			goto out;
>>   		if (key.type == BTRFS_ROOT_ITEM_KEY &&
>> @@ -5000,6 +5004,24 @@ int check_fs_roots_lowmem(struct btrfs_fs_info *fs_info)
>>   			err |= ret;
>>   		}
>>   next:
>> +		/*
>> +		 * Since root tree can be cowed during repair,
>> +		 * here search the saved key again.
>> +		 */
>> +		if (repair) {
>> +			btrfs_release_path(&path);
>> +			ret = btrfs_search_slot(NULL, fs_info->tree_root,
>> +						&saved_key, &path, 0, 0);
>> +			/* Repair never deletes trees, search must succeed. */
>> +			if (ret > 0)
>> +				ret = -ENOENT;
>> +			if (ret) {
>> +				error(
>> +			"search root key[%llu %u %llu] failed after repair: %s",
>> +				      saved_key.objectid, saved_key.type,
>> +				      saved_key.offset, strerror(-ret));
>> +			}
>> +		}
>>   		ret = btrfs_next_item(tree_root, &path);
>>   		if (ret > 0)
>>   			goto out;
>>
Qu Wenruo Sept. 17, 2018, 1:03 p.m. UTC | #3
On 2018/9/17 下午8:55, Su Yue wrote:
> 
> 
> On 2018/9/17 8:51 PM, Qu Wenruo wrote:
>>
>>
>> On 2018/9/17 下午3:28, Su Yue wrote:
>>> In check_fs_roots_lowmem(), we do search and follow the resulted path
>>> to call check_fs_root(), then call btrfs_next_item() to check next
>>> root.
>>> However, if repair is enabled, the root tree can be cowed, the
>>> existed path can cause strange errors.
>>>
>>> Solution:
>>>    If repair, save the key before calling check_fs_root,
>>>    search the saved key again before checking next root.
>>>
>>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
>>
>> The idea is pretty valid, however I'm not pretty sure about one practice
>> below.
>>
>>> ---
>>>   check/mode-lowmem.c | 22 ++++++++++++++++++++++
>>>   1 file changed, 22 insertions(+)
>>>
>>> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
>>> index 4db12cc7f9fe..db44456fd85b 100644
>>> --- a/check/mode-lowmem.c
>>> +++ b/check/mode-lowmem.c
>>> @@ -4967,9 +4967,13 @@ int check_fs_roots_lowmem(struct btrfs_fs_info
>>> *fs_info)
>>>       }
>>>         while (1) {
>>> +        struct btrfs_key saved_key;
>>> +
>>>           node =ath.nodes[0];
>>>           slot =ath.slots[0];
>>>           btrfs_item_key_to_cpu(node, &key, slot);
>>> +        if (repair)
>>> +            saved_key =ey;
>>
>> Is this OK? Shouldn't it be something like memcpy(&saved_key, &key,
>> sizeof(key));
>>
>> Or some new C standard make it work?
>>
> I don't quite know how C89 standard defines this behavior.
> It should work in C99...
> 
> Seems you are not familiar with this style, will use memcpy.

I just want to make sure what's the preferred way, I'm not saying it's
wrong, just wondering if it's OK for btrfs-progs coding style.
(As it indeed saves some chars)

It could completely turn out that I'm just too stubborn to learn new tricks.

Maybe David could answer this better?

Thanks,
Qu

> 
> Thanks,
> Su
>> Although compiler doesn't give any warning on this.
>>
>> Thanks,
>> Qu
>>
>>>           if (key.objectid > BTRFS_LAST_FREE_OBJECTID)
>>>               goto out;
>>>           if (key.type =BTRFS_ROOT_ITEM_KEY &&
>>> @@ -5000,6 +5004,24 @@ int check_fs_roots_lowmem(struct btrfs_fs_info
>>> *fs_info)
>>>               err |=et;
>>>           }
>>>   next:
>>> +        /*
>>> +         * Since root tree can be cowed during repair,
>>> +         * here search the saved key again.
>>> +         */
>>> +        if (repair) {
>>> +            btrfs_release_path(&path);
>>> +            ret =trfs_search_slot(NULL, fs_info->tree_root,
>>> +                        &saved_key, &path, 0, 0);
>>> +            /* Repair never deletes trees, search must succeed. */
>>> +            if (ret > 0)
>>> +                ret =ENOENT;
>>> +            if (ret) {
>>> +                error(
>>> +            "search root key[%llu %u %llu] failed after repair: %s",
>>> +                      saved_key.objectid, saved_key.type,
>>> +                      saved_key.offset, strerror(-ret));
>>> +            }
>>> +        }
>>>           ret =trfs_next_item(tree_root, &path);
>>>           if (ret > 0)
>>>               goto out;
>>>
Nikolay Borisov Sept. 17, 2018, 1:13 p.m. UTC | #4
On 17.09.2018 15:51, Qu Wenruo wrote:
> 
> 
> On 2018/9/17 下午3:28, Su Yue wrote:
>> In check_fs_roots_lowmem(), we do search and follow the resulted path
>> to call check_fs_root(), then call btrfs_next_item() to check next
>> root.
>> However, if repair is enabled, the root tree can be cowed, the
>> existed path can cause strange errors.
>>
>> Solution:
>>   If repair, save the key before calling check_fs_root,
>>   search the saved key again before checking next root.
>>
>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> 
> The idea is pretty valid, however I'm not pretty sure about one practice
> below.
> 
>> ---
>>  check/mode-lowmem.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
>> index 4db12cc7f9fe..db44456fd85b 100644
>> --- a/check/mode-lowmem.c
>> +++ b/check/mode-lowmem.c
>> @@ -4967,9 +4967,13 @@ int check_fs_roots_lowmem(struct btrfs_fs_info *fs_info)
>>  	}
>>  
>>  	while (1) {
>> +		struct btrfs_key saved_key;
>> +
>>  		node = path.nodes[0];
>>  		slot = path.slots[0];
>>  		btrfs_item_key_to_cpu(node, &key, slot);
>> +		if (repair)
>> +			saved_key = key;
> 
> Is this OK? Shouldn't it be something like memcpy(&saved_key, &key,
> sizeof(key));
> 
> Or some new C standard make it work?
It's not new, basically assigning one struct to another i.e a value
assign, pretty much results in memory copy. One has to be careful when
the structs contains pointer member because in this case both structures
will point to the same memory.

In this particular case since btrfs_key consists of primitive types
doing a direct assign is fine.
> 
> Although compiler doesn't give any warning on this.
> 
> Thanks,
> Qu
> 
>>  		if (key.objectid > BTRFS_LAST_FREE_OBJECTID)
>>  			goto out;
>>  		if (key.type == BTRFS_ROOT_ITEM_KEY &&
>> @@ -5000,6 +5004,24 @@ int check_fs_roots_lowmem(struct btrfs_fs_info *fs_info)
>>  			err |= ret;
>>  		}
>>  next:
>> +		/*
>> +		 * Since root tree can be cowed during repair,
>> +		 * here search the saved key again.
>> +		 */
>> +		if (repair) {
>> +			btrfs_release_path(&path);
>> +			ret = btrfs_search_slot(NULL, fs_info->tree_root,
>> +						&saved_key, &path, 0, 0);
>> +			/* Repair never deletes trees, search must succeed. */
>> +			if (ret > 0)
>> +				ret = -ENOENT;
>> +			if (ret) {
>> +				error(
>> +			"search root key[%llu %u %llu] failed after repair: %s",
>> +				      saved_key.objectid, saved_key.type,
>> +				      saved_key.offset, strerror(-ret));
>> +			}
>> +		}
>>  		ret = btrfs_next_item(tree_root, &path);
>>  		if (ret > 0)
>>  			goto out;
>>
>

Patch
diff mbox series

diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 4db12cc7f9fe..db44456fd85b 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -4967,9 +4967,13 @@  int check_fs_roots_lowmem(struct btrfs_fs_info *fs_info)
 	}
 
 	while (1) {
+		struct btrfs_key saved_key;
+
 		node = path.nodes[0];
 		slot = path.slots[0];
 		btrfs_item_key_to_cpu(node, &key, slot);
+		if (repair)
+			saved_key = key;
 		if (key.objectid > BTRFS_LAST_FREE_OBJECTID)
 			goto out;
 		if (key.type == BTRFS_ROOT_ITEM_KEY &&
@@ -5000,6 +5004,24 @@  int check_fs_roots_lowmem(struct btrfs_fs_info *fs_info)
 			err |= ret;
 		}
 next:
+		/*
+		 * Since root tree can be cowed during repair,
+		 * here search the saved key again.
+		 */
+		if (repair) {
+			btrfs_release_path(&path);
+			ret = btrfs_search_slot(NULL, fs_info->tree_root,
+						&saved_key, &path, 0, 0);
+			/* Repair never deletes trees, search must succeed. */
+			if (ret > 0)
+				ret = -ENOENT;
+			if (ret) {
+				error(
+			"search root key[%llu %u %llu] failed after repair: %s",
+				      saved_key.objectid, saved_key.type,
+				      saved_key.offset, strerror(-ret));
+			}
+		}
 		ret = btrfs_next_item(tree_root, &path);
 		if (ret > 0)
 			goto out;