| Message ID | 20190905075800.1633-3-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: check: Repair invalid inode mode in | expand |
On 5.09.19 г. 10:57 ч., Qu Wenruo wrote: > Introduce a function, find_file_type(), to find filetype using > INODE_REF. > > This function will: > - Search DIR_INDEX first > DIR_INDEX is easier since there is only one item in it. > > - Valid the DIR_INDEX item > If the DIR_INDEX is valid, use the filetype and call it a day. > > - Search DIR_ITEM then > > - Valide the DIR_ITEM > If valid, call it a day. Or return -ENOENT; > > This would be used as the primary method to determine the imode in later > imode repair code. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > check/mode-common.c | 99 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 99 insertions(+) > > diff --git a/check/mode-common.c b/check/mode-common.c > index 195b6efaa7aa..c0ddc50a1dd0 100644 > --- a/check/mode-common.c > +++ b/check/mode-common.c > @@ -16,6 +16,7 @@ > > #include <time.h> > #include "ctree.h" > +#include "hash.h" > #include "common/internal.h" > #include "common/messages.h" > #include "transaction.h" > @@ -836,6 +837,104 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root, > return ret; > } > > +static int find_file_type(struct btrfs_root *root, u64 ino, u64 dirid, > + u64 index, const char *name, u32 name_len, > + u32 *imode_ret) > +{ > + struct btrfs_path path; > + struct btrfs_key location; > + struct btrfs_key key; > + struct btrfs_dir_item *di; > + char namebuf[BTRFS_NAME_LEN] = {0}; > + unsigned long cur; > + unsigned long end; > + bool found = false; > + u8 filetype; > + u32 len; > + int ret; > + > + btrfs_init_path(&path); > + > + /* Search DIR_INDEX first */ > + key.objectid = dirid; > + key.offset = index; > + key.type = BTRFS_DIR_INDEX_KEY; > + > + ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0); > + if (ret > 0) > + ret = -ENOENT; Even if it returns 1 meaning there is no DIR_INDEX item perhaps it still makes sense to go to dir_item: label to search for DIR_ITEM, what if the corruption has affected just the DIR_INDEX item? > + if (ret < 0) > + goto dir_item; nit: Use elseif to make it more explicit it is a single construct. > + di = btrfs_item_ptr(path.nodes[0], path.slots[0], > + struct btrfs_dir_item); > + btrfs_dir_item_key_to_cpu(path.nodes[0], di, &location); > + > + /* Various basic check */ > + if (location.objectid != ino || location.type != BTRFS_INODE_ITEM_KEY || > + location.offset != 0) > + goto dir_item; > + filetype = btrfs_dir_type(path.nodes[0], di); > + if (filetype >= BTRFS_FT_MAX || filetype == BTRFS_FT_UNKNOWN) > + goto dir_item; > + len = min_t(u32, btrfs_item_size_nr(path.nodes[0], path.slots[0]) - > + sizeof(*di), BTRFS_NAME_LEN); > + len = min_t(u32, len, btrfs_dir_name_len(path.nodes[0], di)); > + read_extent_buffer(path.nodes[0], namebuf, (unsigned long)(di + 1), len); > + if (name_len != len || memcmp(namebuf, name, len)) > + goto dir_item; > + > + /* Got a correct filetype */ > + found = true; > + *imode_ret = btrfs_type_to_imode(filetype); > + ret = 0; > + goto out; > + > +dir_item: This function is needlessly structured in an awkward way. E.g. there is no dependence between "searching by DIR INDEX item" and "searching by DIR_ITEM". I think the best way is to have 3 function: Top level find_file_type which will call find_file_type_by_dir_index if the reeturn value is negative -> call 2nd function find_file_type_by_dir_item. This will result in 3 fairly short and easy to read/parse functions. > + btrfs_release_path(&path); > + key.objectid = dirid; > + key.offset = btrfs_name_hash(name, name_len); > + > + ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0); > + if (ret > 0) > + ret = -ENOENT; > + if (ret < 0) { > + btrfs_release_path(&path); > + return ret; > + } ditto about the else if construct > + cur = btrfs_item_ptr_offset(path.nodes[0], path.slots[0]); > + end = cur + btrfs_item_size_nr(path.nodes[0], path.slots[0]); > + while (cur < end) { Just checking : this is needed in case we have items whose names create a crc32 collision in the DIR_ITEM offset? > + di = (struct btrfs_dir_item *)cur; > + btrfs_dir_item_key_to_cpu(path.nodes[0], di, &location); > + /* Various basic check */ > + if (location.objectid != ino || > + location.type != BTRFS_INODE_ITEM_KEY || > + location.offset != 0) > + goto next;> + filetype = btrfs_dir_type(path.nodes[0], di); > + if (filetype >= BTRFS_FT_MAX || filetype == BTRFS_FT_UNKNOWN) > + goto next; > + len = min_t(u32, BTRFS_NAME_LEN, > + btrfs_item_size_nr(path.nodes[0], path.slots[0]) - > + sizeof(*di)); > + len = min_t(u32, len, btrfs_dir_name_len(path.nodes[0], di)); > + read_extent_buffer(path.nodes[0], namebuf, > + (unsigned long)(di + 1), len); > + if (name_len != len || memcmp(namebuf, name, len)) > + goto next; > + *imode_ret = btrfs_type_to_imode(filetype); > + found = true; > + goto out; > +next: > + cur += btrfs_dir_name_len(path.nodes[0], di) + sizeof(*di); If this line is moved right after assigning to 'di' instead of having a 'next' label you can simply use the 'continue' keyword > + } > +out: > + btrfs_release_path(&path); > + if (!found && !ret) > + ret = -ENOENT; > + return ret; > +} > + > /* > * Reset the inode mode of the inode specified by @path. > * >
On 5.09.19 г. 10:57 ч., Qu Wenruo wrote: > Introduce a function, find_file_type(), to find filetype using > INODE_REF. This is confusing, there is not a single reference to INODE_REF in the code. I guess you must replace this with DIR_ITEM/DIR_INDEX ?
On 2019/9/9 下午9:25, Nikolay Borisov wrote: > > > On 5.09.19 г. 10:57 ч., Qu Wenruo wrote: >> Introduce a function, find_file_type(), to find filetype using >> INODE_REF. >> >> This function will: >> - Search DIR_INDEX first >> DIR_INDEX is easier since there is only one item in it. >> >> - Valid the DIR_INDEX item >> If the DIR_INDEX is valid, use the filetype and call it a day. >> >> - Search DIR_ITEM then >> >> - Valide the DIR_ITEM >> If valid, call it a day. Or return -ENOENT; >> >> This would be used as the primary method to determine the imode in later >> imode repair code. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> check/mode-common.c | 99 +++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 99 insertions(+) >> >> diff --git a/check/mode-common.c b/check/mode-common.c >> index 195b6efaa7aa..c0ddc50a1dd0 100644 >> --- a/check/mode-common.c >> +++ b/check/mode-common.c >> @@ -16,6 +16,7 @@ >> >> #include <time.h> >> #include "ctree.h" >> +#include "hash.h" >> #include "common/internal.h" >> #include "common/messages.h" >> #include "transaction.h" >> @@ -836,6 +837,104 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root, >> return ret; >> } >> >> +static int find_file_type(struct btrfs_root *root, u64 ino, u64 dirid, >> + u64 index, const char *name, u32 name_len, >> + u32 *imode_ret) >> +{ >> + struct btrfs_path path; >> + struct btrfs_key location; >> + struct btrfs_key key; >> + struct btrfs_dir_item *di; >> + char namebuf[BTRFS_NAME_LEN] = {0}; >> + unsigned long cur; >> + unsigned long end; >> + bool found = false; >> + u8 filetype; >> + u32 len; >> + int ret; >> + >> + btrfs_init_path(&path); >> + >> + /* Search DIR_INDEX first */ >> + key.objectid = dirid; >> + key.offset = index; >> + key.type = BTRFS_DIR_INDEX_KEY; >> + >> + ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0); >> + if (ret > 0) >> + ret = -ENOENT; > > Even if it returns 1 meaning there is no DIR_INDEX item perhaps it still > makes sense to go to dir_item: label to search for DIR_ITEM, what if the > corruption has affected just the DIR_INDEX item? I didn't get the point. The next line is just going to do dir_item search, just as you mentioned. > >> + if (ret < 0) >> + goto dir_item; > nit: Use elseif to make it more explicit it is a single construct. Not sure such usage is recommened, but I see a lot of usage like: ret = btrfs_search_slot(); if (ret > 0) ret = -ENOENT; if (ret < 0) goto error; So I just followed this practice. > >> + di = btrfs_item_ptr(path.nodes[0], path.slots[0], >> + struct btrfs_dir_item); >> + btrfs_dir_item_key_to_cpu(path.nodes[0], di, &location); >> + >> + /* Various basic check */ >> + if (location.objectid != ino || location.type != BTRFS_INODE_ITEM_KEY || >> + location.offset != 0) >> + goto dir_item; >> + filetype = btrfs_dir_type(path.nodes[0], di); >> + if (filetype >= BTRFS_FT_MAX || filetype == BTRFS_FT_UNKNOWN) >> + goto dir_item; >> + len = min_t(u32, btrfs_item_size_nr(path.nodes[0], path.slots[0]) - >> + sizeof(*di), BTRFS_NAME_LEN); >> + len = min_t(u32, len, btrfs_dir_name_len(path.nodes[0], di)); >> + read_extent_buffer(path.nodes[0], namebuf, (unsigned long)(di + 1), len); >> + if (name_len != len || memcmp(namebuf, name, len)) >> + goto dir_item; >> + >> + /* Got a correct filetype */ >> + found = true; >> + *imode_ret = btrfs_type_to_imode(filetype); >> + ret = 0; >> + goto out; >> + >> +dir_item: > > This function is needlessly structured in an awkward way. E.g. there is > no dependence between "searching by DIR INDEX item" and "searching by > DIR_ITEM". I think the best way is to have 3 function: > > Top level find_file_type which will call find_file_type_by_dir_index if > the reeturn value is negative -> call 2nd function > find_file_type_by_dir_item. This will result in 3 fairly short and easy > to read/parse functions. I'm OK with that, which is also my original plan, but it's just too easy to put them altogether into one function, thus I forgot that plan. > >> + btrfs_release_path(&path); >> + key.objectid = dirid; >> + key.offset = btrfs_name_hash(name, name_len); >> + >> + ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0); >> + if (ret > 0) >> + ret = -ENOENT; >> + if (ret < 0) { >> + btrfs_release_path(&path); >> + return ret; >> + } > > ditto about the else if construct > >> + cur = btrfs_item_ptr_offset(path.nodes[0], path.slots[0]); >> + end = cur + btrfs_item_size_nr(path.nodes[0], path.slots[0]); >> + while (cur < end) { > > Just checking : this is needed in case we have items whose names create > a crc32 collision in the DIR_ITEM offset? Yep. For DIR_ITEM we can have collision while for DIR_INDEX it won't cause collision. > >> + di = (struct btrfs_dir_item *)cur; >> + btrfs_dir_item_key_to_cpu(path.nodes[0], di, &location); >> + /* Various basic check */ >> + if (location.objectid != ino || >> + location.type != BTRFS_INODE_ITEM_KEY || >> + location.offset != 0) >> + goto next;> + filetype = btrfs_dir_type(path.nodes[0], di); >> + if (filetype >= BTRFS_FT_MAX || filetype == BTRFS_FT_UNKNOWN) >> + goto next; >> + len = min_t(u32, BTRFS_NAME_LEN, >> + btrfs_item_size_nr(path.nodes[0], path.slots[0]) - >> + sizeof(*di)); >> + len = min_t(u32, len, btrfs_dir_name_len(path.nodes[0], di)); >> + read_extent_buffer(path.nodes[0], namebuf, >> + (unsigned long)(di + 1), len); >> + if (name_len != len || memcmp(namebuf, name, len)) >> + goto next; >> + *imode_ret = btrfs_type_to_imode(filetype); >> + found = true; >> + goto out; >> +next: >> + cur += btrfs_dir_name_len(path.nodes[0], di) + sizeof(*di); > > If this line is moved right after assigning to 'di' instead of having a > 'next' label you can simply use the 'continue' keyword Right, saving a tag is never a bad idea. Thanks, Qu > >> + } >> +out: >> + btrfs_release_path(&path); >> + if (!found && !ret) >> + ret = -ENOENT; >> + return ret; >> +} >> + >> /* >> * Reset the inode mode of the inode specified by @path. >> * >>
On 2019/9/9 下午9:42, Nikolay Borisov wrote: > > > On 5.09.19 г. 10:57 ч., Qu Wenruo wrote: >> Introduce a function, find_file_type(), to find filetype using >> INODE_REF. > > This is confusing, there is not a single reference to INODE_REF in the > code. I guess you must replace this with DIR_ITEM/DIR_INDEX ? Don't forget how you get the @dirid @index,@name,@namelen from. All these info are from INODE_REF item. But I totally understand your concern, it's sometimes really easy to get confused about 1) what we are searching for 2) what the search indexes are from. Thanks, Qu
On 9.09.19 г. 17:24 ч., Qu Wenruo wrote: > > > On 2019/9/9 下午9:25, Nikolay Borisov wrote: >> >> >> On 5.09.19 г. 10:57 ч., Qu Wenruo wrote: >>> Introduce a function, find_file_type(), to find filetype using >>> INODE_REF. >>> >>> This function will: >>> - Search DIR_INDEX first >>> DIR_INDEX is easier since there is only one item in it. >>> >>> - Valid the DIR_INDEX item >>> If the DIR_INDEX is valid, use the filetype and call it a day. >>> >>> - Search DIR_ITEM then >>> >>> - Valide the DIR_ITEM >>> If valid, call it a day. Or return -ENOENT; >>> >>> This would be used as the primary method to determine the imode in later >>> imode repair code. >>> >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>> --- >>> check/mode-common.c | 99 +++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 99 insertions(+) >>> >>> diff --git a/check/mode-common.c b/check/mode-common.c >>> index 195b6efaa7aa..c0ddc50a1dd0 100644 >>> --- a/check/mode-common.c >>> +++ b/check/mode-common.c >>> @@ -16,6 +16,7 @@ >>> >>> #include <time.h> >>> #include "ctree.h" >>> +#include "hash.h" >>> #include "common/internal.h" >>> #include "common/messages.h" >>> #include "transaction.h" >>> @@ -836,6 +837,104 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root, >>> return ret; >>> } >>> >>> +static int find_file_type(struct btrfs_root *root, u64 ino, u64 dirid, >>> + u64 index, const char *name, u32 name_len, >>> + u32 *imode_ret) >>> +{ >>> + struct btrfs_path path; >>> + struct btrfs_key location; >>> + struct btrfs_key key; >>> + struct btrfs_dir_item *di; >>> + char namebuf[BTRFS_NAME_LEN] = {0}; >>> + unsigned long cur; >>> + unsigned long end; >>> + bool found = false; >>> + u8 filetype; >>> + u32 len; >>> + int ret; >>> + >>> + btrfs_init_path(&path); >>> + >>> + /* Search DIR_INDEX first */ >>> + key.objectid = dirid; >>> + key.offset = index; >>> + key.type = BTRFS_DIR_INDEX_KEY; >>> + >>> + ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0); >>> + if (ret > 0) >>> + ret = -ENOENT; >> >> Even if it returns 1 meaning there is no DIR_INDEX item perhaps it still >> makes sense to go to dir_item: label to search for DIR_ITEM, what if the >> corruption has affected just the DIR_INDEX item? > > I didn't get the point. > > The next line is just going to do dir_item search, just as you mentioned. You are right, however, this is somewhat subtle and the fact I missed it just proves the point. The following will be much better (and explicit): if (ret) goto dir_item In fact setting the -ENOENT on ret > 0 is only needed because of the awkward way the return value of btrfs_search_slot is handled. So yeah, I'm even more convinced that a simple if (ret) goto dir_item (or call a function) is the way to go here. >> >>> + if (ret < 0) >>> + goto dir_item; >> nit: Use elseif to make it more explicit it is a single construct. > > Not sure such usage is recommened, but I see a lot of usage like: > ret = btrfs_search_slot(); > if (ret > 0) > ret = -ENOENT; > if (ret < 0) > goto error; > > So I just followed this practice. > I guess this is debatable. You are right that most of the retval handling is as you say but I think the more "purist" (so to say) way should be an if {} else if {}. Because ultimately you are handling one thing - the return value of btrfs_search_slot. David, what is your take on that ? <snip>
On 9.09.19 г. 17:26 ч., Qu Wenruo wrote: > > > On 2019/9/9 下午9:42, Nikolay Borisov wrote: >> >> >> On 5.09.19 г. 10:57 ч., Qu Wenruo wrote: >>> Introduce a function, find_file_type(), to find filetype using >>> INODE_REF. >> >> This is confusing, there is not a single reference to INODE_REF in the >> code. I guess you must replace this with DIR_ITEM/DIR_INDEX ? > > Don't forget how you get the @dirid @index,@name,@namelen from. > > All these info are from INODE_REF item. > > But I totally understand your concern, it's sometimes really easy to get > confused about 1) what we are searching for 2) what the search indexes > are from. Yes but that is only apparent when one reads the next patch. When you take this function in isolation it really gets input data and based on that tries to search for relevant DIR_ITEM/DIR_INDEX. Think about someone stumbling on this commit 6 months from now, without necessarily having reviewed the whole series. > > Thanks, > Qu > >
diff --git a/check/mode-common.c b/check/mode-common.c index 195b6efaa7aa..c0ddc50a1dd0 100644 --- a/check/mode-common.c +++ b/check/mode-common.c @@ -16,6 +16,7 @@ #include <time.h> #include "ctree.h" +#include "hash.h" #include "common/internal.h" #include "common/messages.h" #include "transaction.h" @@ -836,6 +837,104 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root, return ret; } +static int find_file_type(struct btrfs_root *root, u64 ino, u64 dirid, + u64 index, const char *name, u32 name_len, + u32 *imode_ret) +{ + struct btrfs_path path; + struct btrfs_key location; + struct btrfs_key key; + struct btrfs_dir_item *di; + char namebuf[BTRFS_NAME_LEN] = {0}; + unsigned long cur; + unsigned long end; + bool found = false; + u8 filetype; + u32 len; + int ret; + + btrfs_init_path(&path); + + /* Search DIR_INDEX first */ + key.objectid = dirid; + key.offset = index; + key.type = BTRFS_DIR_INDEX_KEY; + + ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0); + if (ret > 0) + ret = -ENOENT; + if (ret < 0) + goto dir_item; + di = btrfs_item_ptr(path.nodes[0], path.slots[0], + struct btrfs_dir_item); + btrfs_dir_item_key_to_cpu(path.nodes[0], di, &location); + + /* Various basic check */ + if (location.objectid != ino || location.type != BTRFS_INODE_ITEM_KEY || + location.offset != 0) + goto dir_item; + filetype = btrfs_dir_type(path.nodes[0], di); + if (filetype >= BTRFS_FT_MAX || filetype == BTRFS_FT_UNKNOWN) + goto dir_item; + len = min_t(u32, btrfs_item_size_nr(path.nodes[0], path.slots[0]) - + sizeof(*di), BTRFS_NAME_LEN); + len = min_t(u32, len, btrfs_dir_name_len(path.nodes[0], di)); + read_extent_buffer(path.nodes[0], namebuf, (unsigned long)(di + 1), len); + if (name_len != len || memcmp(namebuf, name, len)) + goto dir_item; + + /* Got a correct filetype */ + found = true; + *imode_ret = btrfs_type_to_imode(filetype); + ret = 0; + goto out; + +dir_item: + btrfs_release_path(&path); + key.objectid = dirid; + key.offset = btrfs_name_hash(name, name_len); + + ret = btrfs_search_slot(NULL, root, &key, &path, 0, 0); + if (ret > 0) + ret = -ENOENT; + if (ret < 0) { + btrfs_release_path(&path); + return ret; + } + cur = btrfs_item_ptr_offset(path.nodes[0], path.slots[0]); + end = cur + btrfs_item_size_nr(path.nodes[0], path.slots[0]); + while (cur < end) { + di = (struct btrfs_dir_item *)cur; + btrfs_dir_item_key_to_cpu(path.nodes[0], di, &location); + /* Various basic check */ + if (location.objectid != ino || + location.type != BTRFS_INODE_ITEM_KEY || + location.offset != 0) + goto next; + filetype = btrfs_dir_type(path.nodes[0], di); + if (filetype >= BTRFS_FT_MAX || filetype == BTRFS_FT_UNKNOWN) + goto next; + len = min_t(u32, BTRFS_NAME_LEN, + btrfs_item_size_nr(path.nodes[0], path.slots[0]) - + sizeof(*di)); + len = min_t(u32, len, btrfs_dir_name_len(path.nodes[0], di)); + read_extent_buffer(path.nodes[0], namebuf, + (unsigned long)(di + 1), len); + if (name_len != len || memcmp(namebuf, name, len)) + goto next; + *imode_ret = btrfs_type_to_imode(filetype); + found = true; + goto out; +next: + cur += btrfs_dir_name_len(path.nodes[0], di) + sizeof(*di); + } +out: + btrfs_release_path(&path); + if (!found && !ret) + ret = -ENOENT; + return ret; +} + /* * Reset the inode mode of the inode specified by @path. *
Introduce a function, find_file_type(), to find filetype using INODE_REF. This function will: - Search DIR_INDEX first DIR_INDEX is easier since there is only one item in it. - Valid the DIR_INDEX item If the DIR_INDEX is valid, use the filetype and call it a day. - Search DIR_ITEM then - Valide the DIR_ITEM If valid, call it a day. Or return -ENOENT; This would be used as the primary method to determine the imode in later imode repair code. Signed-off-by: Qu Wenruo <wqu@suse.com> --- check/mode-common.c | 99 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+)