Re: [PATCH 1/2] btrfs: Populate btrfs_super_block::log_root_transid
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_root30572544 log_root_transid0 log_root_level 0 This patch will populate it with log tree root correctly, so the result will be: log_root30572544 log_root_transid6 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. >>> >>
Re: [PATCH 1/2] btrfs: Populate btrfs_super_block::log_root_transid
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_root30572544 >>> log_root_transid0 >>> log_root_level 0 >>> >>> This patch will populate it with log tree root correctly, so the result >>> will be: >>> log_root30572544 >>> log_root_transid6 >>> 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
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_root30572544 >> log_root_transid0 >> log_root_level 0 >> >> This patch will populate it with log tree root correctly, so the result >> will be: >> log_root30572544 >> log_root_transid6 >> 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. > signature.asc Description: OpenPGP digital signature
Re: [PATCH 1/2] btrfs: Populate btrfs_super_block::log_root_transid
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_root30572544 > log_root_transid0 > log_root_level 0 > > This patch will populate it with log tree root correctly, so the result > will be: > log_root30572544 > log_root_transid6 > 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
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 signature.asc Description: OpenPGP digital signature
Re: [PATCH 1/2] btrfs: Populate btrfs_super_block::log_root_transid
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
[PATCH 1/2] btrfs: Populate btrfs_super_block::log_root_transid
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_root30572544 log_root_transid0 log_root_level 0 This patch will populate it with log tree root correctly, so the result will be: log_root30572544 log_root_transid6 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 --- 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(_root_tree->log_mutex); -- 2.19.0
