| Message ID | 20180917072852.25831-5-suy.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: lowmem: bug fixes and inode_extref repair | expand |
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; >
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; >>
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; >>>
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; >> >
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;
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(+)