[PATCH 0/2] btrfs: btrfs_super_block::log_root_transid related enhancement

[PATCH 0/2] btrfs: btrfs_super_block::log_root_transid related enhancement

[Qu Wenruo]

Even we have btrfs_super_block::log_root_transid, it's never really
populated.

This patchset will populate that member and use it to detect corrupted
log tree early.

Qu Wenruo (2):
  btrfs: Populate btrfs_super_block::log_root_transid
  btrfs: Validate btrfs_super_block::log_root_transid

 fs/btrfs/disk-io.c     | 28 ++++++++++++++++++++++------
 fs/btrfs/transaction.c |  1 +
 fs/btrfs/tree-log.c    |  2 ++
 3 files changed, 25 insertions(+), 6 deletions(-)

-- 
2.19.0

[PATCH 1/2] btrfs: Populate btrfs_super_block::log_root_transid

[Qu Wenruo]

We have btrfs_super_block::log_root_transid to record the transid of log
tree root.

However it's never populated and it's always 0, just as the following
dump-super:
 log_root                30572544
 log_root_transid        0
 log_root_level          0

This patch will populate it with log tree root correctly, so the result
will be:
 log_root                30572544
 log_root_transid        6
 log_root_level          0

This won't affect current kernel behavior or btrfs check result as we
already expect log tree root generation always to be super block
genration + 1.

But it could be later used to detect log tree corruption early.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/transaction.c | 1 +
 fs/btrfs/tree-log.c    | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 3b84f5015029..9d4947079aa3 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -2211,6 +2211,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 
 	btrfs_set_super_log_root(fs_info->super_copy, 0);
 	btrfs_set_super_log_root_level(fs_info->super_copy, 0);
+	btrfs_set_super_log_root_transid(fs_info->super_copy, 0);
 	memcpy(fs_info->super_for_commit, fs_info->super_copy,
 	       sizeof(*fs_info->super_copy));
 
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 1650dc44a5e3..3e16e7b54922 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3131,6 +3131,8 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 				 log_root_tree->node->start);
 	btrfs_set_super_log_root_level(fs_info->super_for_commit,
 				       btrfs_header_level(log_root_tree->node));
+	btrfs_set_super_log_root_transid(fs_info->super_for_commit,
+				btrfs_header_generation(log_root_tree->node));
 
 	log_root_tree->log_transid++;
 	mutex_unlock(&log_root_tree->log_mutex);
-- 
2.19.0

Re: [PATCH 1/2] btrfs: Populate btrfs_super_block::log_root_transid

[Anand Jain]

> we
> already expect log tree root generation always to be super block
> genration + 1.

  This is close to what I was looking for by reading the cover-letter,
  basically  what is the impact/bug by not populating it? but can you
  explain more, I wonder how was check working so long then?

> But it could be later used to detect log tree corruption early.

  If there is no impact/bug its rather a good idea to change this when
  the early log tree corruption detection part is ready. So that there
  is absolute clarity.

  The reason why I am grossly wary is - we have incomplete business
  about how do we handle the write-hole, say raid1 to begin with.

Thanks, Anand

Re: [PATCH 1/2] btrfs: Populate btrfs_super_block::log_root_transid

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1484 bytes --]



On 2018/10/4 下午4:07, Anand Jain wrote:
> 
>> we
>> already expect log tree root generation always to be super block
>> genration + 1.
> 
>  This is close to what I was looking for by reading the cover-letter,
>  basically  what is the impact/bug by not populating it?

Nothing really.

Both kernel and btrfs-progs completely ignore it.

> but can you
>  explain more, I wonder how was check working so long then?

For btrfs-progs, the behavior will not change at all.
It still assume log tree root's generation is super geneartion + 1.
And if doesn't match reports a transid mismatch error.

For kernel, super validation checker will do such generation check earlier.

So instead of reporting transid mismatch and then btrfs_log_recover
error, it will report log tree is bad.

> 
>> But it could be later used to detect log tree corruption early.
> 
>  If there is no impact/bug its rather a good idea to change this when
>  the early log tree corruption detection part is ready. So that there
>  is absolute clarity.
> 
>  The reason why I am grossly wary is - we have incomplete business
>  about how do we handle the write-hole, say raid1 to begin with.

If the solution is going to rely on log tree, then the patch shouldn't
affect anyone.

The main purpose here is to make the log_root_transid to follow all
other generation behavior (root, chunk).
Nothing to really affect users.

Thanks,
Qu

> 
> Thanks, Anand


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] btrfs: Populate btrfs_super_block::log_root_transid

[David Sterba]

On Thu, Oct 04, 2018 at 01:49:49PM +0800, Qu Wenruo wrote:
> We have btrfs_super_block::log_root_transid to record the transid of log
> tree root.
> 
> However it's never populated and it's always 0, just as the following
> dump-super:
>  log_root                30572544
>  log_root_transid        0
>  log_root_level          0
> 
> This patch will populate it with log tree root correctly, so the result
> will be:
>  log_root                30572544
>  log_root_transid        6
>  log_root_level          0
> 
> This won't affect current kernel behavior or btrfs check result as we
> already expect log tree root generation always to be super block
> genration + 1.
> 
> But it could be later used to detect log tree corruption early.

The backward compatibility seems to be ok in general, I found one
scenario where the check will fail:

* mount with unpatched kernel, log_root_transid = 0
* mount with patched kernel, log_root_transid 100, generation 101
* mount with unpatched kernel, log_root_transid 100 (unchanged), generation 201
* mount with patched kernel -> check fails

So the problem is when log_root_transid is not 0 and changes out of sync
with the generation. The above sequence of kernels can simply happen
when switching between old stable and current releases.

Re: [PATCH 1/2] btrfs: Populate btrfs_super_block::log_root_transid

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1663 bytes --]



On 2018/10/11 下午8:31, David Sterba wrote:
> On Thu, Oct 04, 2018 at 01:49:49PM +0800, Qu Wenruo wrote:
>> We have btrfs_super_block::log_root_transid to record the transid of log
>> tree root.
>>
>> However it's never populated and it's always 0, just as the following
>> dump-super:
>>  log_root                30572544
>>  log_root_transid        0
>>  log_root_level          0
>>
>> This patch will populate it with log tree root correctly, so the result
>> will be:
>>  log_root                30572544
>>  log_root_transid        6
>>  log_root_level          0
>>
>> This won't affect current kernel behavior or btrfs check result as we
>> already expect log tree root generation always to be super block
>> genration + 1.
>>
>> But it could be later used to detect log tree corruption early.
> 
> The backward compatibility seems to be ok in general, I found one
> scenario where the check will fail:
> 
> * mount with unpatched kernel, log_root_transid = 0
> * mount with patched kernel, log_root_transid 100, generation 101
> * mount with unpatched kernel, log_root_transid 100 (unchanged), generation 201
> * mount with patched kernel -> check fails

Indeed, this is a problem I missed.

It provides the old principle, if we're going to change how kernel
use/interprete a on-disk memeber, we have to introduce a new
incompatible flag.

Please just drop the series of patch.

Thanks,
Qu
> 
> So the problem is when log_root_transid is not 0 and changes out of sync
> with the generation. The above sequence of kernels can simply happen
> when switching between old stable and current releases.
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] btrfs: Populate btrfs_super_block::log_root_transid

[Nikolay Borisov]

On 11.10.2018 15:45, Qu Wenruo wrote:
> 
> 
> On 2018/10/11 下午8:31, David Sterba wrote:
>> On Thu, Oct 04, 2018 at 01:49:49PM +0800, Qu Wenruo wrote:
>>> We have btrfs_super_block::log_root_transid to record the transid of log
>>> tree root.
>>>
>>> However it's never populated and it's always 0, just as the following
>>> dump-super:
>>>  log_root                30572544
>>>  log_root_transid        0
>>>  log_root_level          0
>>>
>>> This patch will populate it with log tree root correctly, so the result
>>> will be:
>>>  log_root                30572544
>>>  log_root_transid        6
>>>  log_root_level          0
>>>
>>> This won't affect current kernel behavior or btrfs check result as we
>>> already expect log tree root generation always to be super block
>>> genration + 1.
>>>
>>> But it could be later used to detect log tree corruption early.
>>
>> The backward compatibility seems to be ok in general, I found one
>> scenario where the check will fail:
>>
>> * mount with unpatched kernel, log_root_transid = 0
>> * mount with patched kernel, log_root_transid 100, generation 101
>> * mount with unpatched kernel, log_root_transid 100 (unchanged), generation 201
>> * mount with patched kernel -> check fails
> 
> Indeed, this is a problem I missed.
> 
> It provides the old principle, if we're going to change how kernel
> use/interprete a on-disk memeber, we have to introduce a new
> incompatible flag.
> 
> Please just drop the series of patch.

Since this member really doesn't have any purpose now I think the
prudent thing to do is to rename it to "reserved1" or "padding" or
whatever but basically signal that it's no longer in used. In the future
when a new incompaat bit will be required for some feature it can be
re-used.

> 
> Thanks,
> Qu
>>
>> So the problem is when log_root_transid is not 0 and changes out of sync
>> with the generation. The above sequence of kernels can simply happen
>> when switching between old stable and current releases.
>>
> 

Re: [PATCH 1/2] btrfs: Populate btrfs_super_block::log_root_transid

[Qu Wenruo]

On 2018/10/11 下午9:11, Nikolay Borisov wrote:
> 
> 
> On 11.10.2018 15:45, Qu Wenruo wrote:
>>
>>
>> On 2018/10/11 下午8:31, David Sterba wrote:
>>> On Thu, Oct 04, 2018 at 01:49:49PM +0800, Qu Wenruo wrote:
>>>> We have btrfs_super_block::log_root_transid to record the transid of log
>>>> tree root.
>>>>
>>>> However it's never populated and it's always 0, just as the following
>>>> dump-super:
>>>>  log_root                30572544
>>>>  log_root_transid        0
>>>>  log_root_level          0
>>>>
>>>> This patch will populate it with log tree root correctly, so the result
>>>> will be:
>>>>  log_root                30572544
>>>>  log_root_transid        6
>>>>  log_root_level          0
>>>>
>>>> This won't affect current kernel behavior or btrfs check result as we
>>>> already expect log tree root generation always to be super block
>>>> genration + 1.
>>>>
>>>> But it could be later used to detect log tree corruption early.
>>>
>>> The backward compatibility seems to be ok in general, I found one
>>> scenario where the check will fail:
>>>
>>> * mount with unpatched kernel, log_root_transid = 0
>>> * mount with patched kernel, log_root_transid 100, generation 101
>>> * mount with unpatched kernel, log_root_transid 100 (unchanged), generation 201
>>> * mount with patched kernel -> check fails
>>
>> Indeed, this is a problem I missed.
>>
>> It provides the old principle, if we're going to change how kernel
>> use/interprete a on-disk memeber, we have to introduce a new
>> incompatible flag.
>>
>> Please just drop the series of patch.
> 
> Since this member really doesn't have any purpose now I think the
> prudent thing to do is to rename it to "reserved1" or "padding" or
> whatever but basically signal that it's no longer in used. In the future
> when a new incompaat bit will be required for some feature it can be
> re-used.

Makes sense.

I'll craft a patch for that.

Thanks,
Qu

> 
>>
>> Thanks,
>> Qu
>>>
>>> So the problem is when log_root_transid is not 0 and changes out of sync
>>> with the generation. The above sequence of kernels can simply happen
>>> when switching between old stable and current releases.
>>>
>>

[PATCH 2/2] btrfs: Validate btrfs_super_block::log_root_transid

[Qu Wenruo]

So we can detect log root related problem early and report the error
more meaningfully other than hitting it log root read time.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 5124c15705ce..34c68401c6bc 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2408,6 +2408,7 @@ static int validate_super(struct btrfs_fs_info *fs_info,
 {
 	u64 nodesize = btrfs_super_nodesize(sb);
 	u64 sectorsize = btrfs_super_sectorsize(sb);
+	u64 generation;
 	int ret = 0;
 
 	if (btrfs_super_magic(sb) != BTRFS_MAGIC) {
@@ -2534,21 +2535,36 @@ static int validate_super(struct btrfs_fs_info *fs_info,
 		ret = -EINVAL;
 	}
 
+	generation = btrfs_super_generation(sb);
 	/*
 	 * The generation is a global counter, we'll trust it more than the others
 	 * but it's still possible that it's the one that's wrong.
 	 */
-	if (btrfs_super_generation(sb) < btrfs_super_chunk_root_generation(sb))
+	if (generation < btrfs_super_chunk_root_generation(sb))
 		btrfs_warn(fs_info,
 			"suspicious: generation < chunk_root_generation: %llu < %llu",
-			btrfs_super_generation(sb),
-			btrfs_super_chunk_root_generation(sb));
-	if (btrfs_super_generation(sb) < btrfs_super_cache_generation(sb)
+			generation, btrfs_super_chunk_root_generation(sb));
+	if (generation < btrfs_super_cache_generation(sb)
 	    && btrfs_super_cache_generation(sb) != (u64)-1)
 		btrfs_warn(fs_info,
 			"suspicious: generation < cache_generation: %llu < %llu",
-			btrfs_super_generation(sb),
-			btrfs_super_cache_generation(sb));
+			generation, btrfs_super_cache_generation(sb));
+
+	/*
+	 * Check log root transid against super block generation.
+	 *
+	 * Older kernel doesn't populate log_root_transid, only need to check
+	 * it when it's not zero.
+	 * Since replaying suspicious log tree can cause serious problem, refuse
+	 * to mount if it doesn't match.
+	 */
+	if (btrfs_super_log_root_transid(sb) &&
+	    btrfs_super_log_root_transid(sb) != generation + 1) {
+		btrfs_err(fs_info,
+	"bad log tree, log_root_transid != generation + 1: %llu != %llu + 1",
+			  btrfs_super_log_root_transid(sb), generation);
+		ret = -EINVAL;
+	}
 
 	return ret;
 }
-- 
2.19.0

Re: [PATCH 2/2] btrfs: Validate btrfs_super_block::log_root_transid

[Nikolay Borisov]

On  4.10.2018 08:49, Qu Wenruo wrote:
> So we can detect log root related problem early and report the error
> more meaningfully other than hitting it log root read time.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/disk-io.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 5124c15705ce..34c68401c6bc 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2408,6 +2408,7 @@ static int validate_super(struct btrfs_fs_info *fs_info,
>  {
>  	u64 nodesize = btrfs_super_nodesize(sb);
>  	u64 sectorsize = btrfs_super_sectorsize(sb);
> +	u64 generation;
>  	int ret = 0;
>  
>  	if (btrfs_super_magic(sb) != BTRFS_MAGIC) {
> @@ -2534,21 +2535,36 @@ static int validate_super(struct btrfs_fs_info *fs_info,
>  		ret = -EINVAL;
>  	}
>  
> +	generation = btrfs_super_generation(sb);
>  	/*
>  	 * The generation is a global counter, we'll trust it more than the others
>  	 * but it's still possible that it's the one that's wrong.
>  	 */
> -	if (btrfs_super_generation(sb) < btrfs_super_chunk_root_generation(sb))
> +	if (generation < btrfs_super_chunk_root_generation(sb))
>  		btrfs_warn(fs_info,
>  			"suspicious: generation < chunk_root_generation: %llu < %llu",
> -			btrfs_super_generation(sb),
> -			btrfs_super_chunk_root_generation(sb));
> -	if (btrfs_super_generation(sb) < btrfs_super_cache_generation(sb)
> +			generation, btrfs_super_chunk_root_generation(sb));
> +	if (generation < btrfs_super_cache_generation(sb)
>  	    && btrfs_super_cache_generation(sb) != (u64)-1)
>  		btrfs_warn(fs_info,
>  			"suspicious: generation < cache_generation: %llu < %llu",
> -			btrfs_super_generation(sb),
> -			btrfs_super_cache_generation(sb));
> +			generation, btrfs_super_cache_generation(sb));
> +
> +	/*
> +	 * Check log root transid against super block generation.
> +	 *
> +	 * Older kernel doesn't populate log_root_transid, only need to check
> +	 * it when it's not zero.
> +	 * Since replaying suspicious log tree can cause serious problem, refuse
> +	 * to mount if it doesn't match.
> +	 */
> +	if (btrfs_super_log_root_transid(sb) &&
> +	    btrfs_super_log_root_transid(sb) != generation + 1) {

nit: in the same vein as you do for btrfs_super_generation just assign
super_log_root_Transid to a local var and use that. IMO it's more
consistent with what's happening in this function.

> +		btrfs_err(fs_info,
> +	"bad log tree, log_root_transid != generation + 1: %llu != %llu + 1",
> +			  btrfs_super_log_root_transid(sb), generation);
> +		ret = -EINVAL;
> +	}
>  
>  	return ret;
>  }
> 

Re: [PATCH 2/2] btrfs: Validate btrfs_super_block::log_root_transid

[Qu Wenruo]

On 2018/10/4 下午2:57, Nikolay Borisov wrote:
> 
> 
> On  4.10.2018 08:49, Qu Wenruo wrote:
>> So we can detect log root related problem early and report the error
>> more meaningfully other than hitting it log root read time.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/disk-io.c | 28 ++++++++++++++++++++++------
>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 5124c15705ce..34c68401c6bc 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -2408,6 +2408,7 @@ static int validate_super(struct btrfs_fs_info *fs_info,
>>  {
>>  	u64 nodesize = btrfs_super_nodesize(sb);
>>  	u64 sectorsize = btrfs_super_sectorsize(sb);
>> +	u64 generation;
>>  	int ret = 0;
>>  
>>  	if (btrfs_super_magic(sb) != BTRFS_MAGIC) {
>> @@ -2534,21 +2535,36 @@ static int validate_super(struct btrfs_fs_info *fs_info,
>>  		ret = -EINVAL;
>>  	}
>>  
>> +	generation = btrfs_super_generation(sb);
>>  	/*
>>  	 * The generation is a global counter, we'll trust it more than the others
>>  	 * but it's still possible that it's the one that's wrong.
>>  	 */
>> -	if (btrfs_super_generation(sb) < btrfs_super_chunk_root_generation(sb))
>> +	if (generation < btrfs_super_chunk_root_generation(sb))
>>  		btrfs_warn(fs_info,
>>  			"suspicious: generation < chunk_root_generation: %llu < %llu",
>> -			btrfs_super_generation(sb),
>> -			btrfs_super_chunk_root_generation(sb));
>> -	if (btrfs_super_generation(sb) < btrfs_super_cache_generation(sb)
>> +			generation, btrfs_super_chunk_root_generation(sb));
>> +	if (generation < btrfs_super_cache_generation(sb)
>>  	    && btrfs_super_cache_generation(sb) != (u64)-1)
>>  		btrfs_warn(fs_info,
>>  			"suspicious: generation < cache_generation: %llu < %llu",
>> -			btrfs_super_generation(sb),
>> -			btrfs_super_cache_generation(sb));
>> +			generation, btrfs_super_cache_generation(sb));
>> +
>> +	/*
>> +	 * Check log root transid against super block generation.
>> +	 *
>> +	 * Older kernel doesn't populate log_root_transid, only need to check
>> +	 * it when it's not zero.
>> +	 * Since replaying suspicious log tree can cause serious problem, refuse
>> +	 * to mount if it doesn't match.
>> +	 */
>> +	if (btrfs_super_log_root_transid(sb) &&
>> +	    btrfs_super_log_root_transid(sb) != generation + 1) {
> 
> nit: in the same vein as you do for btrfs_super_generation just assign
> super_log_root_Transid to a local var and use that. IMO it's more
> consistent with what's happening in this function.

@generation is used for more than one location (chunk generation and
cache generation), while log root transid (along with chunk generation
and cache generation) isn't used more than twice.

Thus I only assign generation to @generation.

Thanks,
Qu

> 
>> +		btrfs_err(fs_info,
>> +	"bad log tree, log_root_transid != generation + 1: %llu != %llu + 1",
>> +			  btrfs_super_log_root_transid(sb), generation);
>> +		ret = -EINVAL;
>> +	}
>>  
>>  	return ret;
>>  }
>>

Re: [PATCH 2/2] btrfs: Validate btrfs_super_block::log_root_transid

[David Sterba]

On Thu, Oct 04, 2018 at 09:57:22AM +0300, Nikolay Borisov wrote:
> > +	/*
> > +	 * Check log root transid against super block generation.
> > +	 *
> > +	 * Older kernel doesn't populate log_root_transid, only need to check
> > +	 * it when it's not zero.
> > +	 * Since replaying suspicious log tree can cause serious problem, refuse
> > +	 * to mount if it doesn't match.
> > +	 */
> > +	if (btrfs_super_log_root_transid(sb) &&
> > +	    btrfs_super_log_root_transid(sb) != generation + 1) {
> 
> nit: in the same vein as you do for btrfs_super_generation just assign
> super_log_root_Transid to a local var and use that. IMO it's more
> consistent with what's happening in this function.

While I see your point I think the way Qu did it is also fine, there's
not much duplication and I find it readable.