| Message ID | 20160921031604.23334-3-quwenruo@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Sep 21, 2016 at 11:15:52AM +0800, Qu Wenruo wrote: > From: Lu Fengqi <lufq.fnst@cn.fujitsu.com> > > Introduce a new function find_dir_item() to find DIR_ITEM for the given > key, and check it with the specified INODE_REF/INODE_EXTREF match. > > Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > --- > cmds-check.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 140 insertions(+) > > diff --git a/cmds-check.c b/cmds-check.c > index 998ba63..4e25804 100644 > --- a/cmds-check.c > +++ b/cmds-check.c > @@ -3848,6 +3848,146 @@ out: > return err; > } > > +#define ROOT_DIR_ERROR (1<<1) /* bad root_dir */ > +#define DIR_ITEM_MISSING (1<<2) /* DIR_ITEM not found */ > +#define DIR_ITEM_MISMATCH (1<<3) /* DIR_ITEM found but not match */ What's the reason for another definition of the error codes? They're mostly copied from te I_ERR_* counterparts. I'd rather have one set of error codes. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At 11/02/2016 11:21 PM, David Sterba wrote: > On Wed, Sep 21, 2016 at 11:15:52AM +0800, Qu Wenruo wrote: >> From: Lu Fengqi <lufq.fnst@cn.fujitsu.com> >> >> Introduce a new function find_dir_item() to find DIR_ITEM for the given >> key, and check it with the specified INODE_REF/INODE_EXTREF match. >> >> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> >> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >> --- >> cmds-check.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 140 insertions(+) >> >> diff --git a/cmds-check.c b/cmds-check.c >> index 998ba63..4e25804 100644 >> --- a/cmds-check.c >> +++ b/cmds-check.c >> @@ -3848,6 +3848,146 @@ out: >> return err; >> } >> >> +#define ROOT_DIR_ERROR (1<<1) /* bad root_dir */ >> +#define DIR_ITEM_MISSING (1<<2) /* DIR_ITEM not found */ >> +#define DIR_ITEM_MISMATCH (1<<3) /* DIR_ITEM found but not match */ > > What's the reason for another definition of the error codes? They're > mostly copied from te I_ERR_* counterparts. I'd rather have one set of > error codes. The main reason is, in lowmem fsck mode, we are not checking inodes or ref/backref in batch. If using I_ERR and REF_ERR, we can mixing them up as they share the same bits. So we introduced the new error bitmap, to make sure all error bits won't cover each other. It may be better if we rearrange I_ERR/REF_ERR to avoid conflicts. E.g, let REF_ERR_ starts from lowest bit and let I_ERR_ starts from high bits. Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 03, 2016 at 09:58:21AM +0800, Qu Wenruo wrote: > > > At 11/02/2016 11:21 PM, David Sterba wrote: > > On Wed, Sep 21, 2016 at 11:15:52AM +0800, Qu Wenruo wrote: > >> From: Lu Fengqi <lufq.fnst@cn.fujitsu.com> > >> > >> Introduce a new function find_dir_item() to find DIR_ITEM for the given > >> key, and check it with the specified INODE_REF/INODE_EXTREF match. > >> > >> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> > >> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> > >> --- > >> cmds-check.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 140 insertions(+) > >> > >> diff --git a/cmds-check.c b/cmds-check.c > >> index 998ba63..4e25804 100644 > >> --- a/cmds-check.c > >> +++ b/cmds-check.c > >> @@ -3848,6 +3848,146 @@ out: > >> return err; > >> } > >> > >> +#define ROOT_DIR_ERROR (1<<1) /* bad root_dir */ > >> +#define DIR_ITEM_MISSING (1<<2) /* DIR_ITEM not found */ > >> +#define DIR_ITEM_MISMATCH (1<<3) /* DIR_ITEM found but not match */ > > > > What's the reason for another definition of the error codes? They're > > mostly copied from te I_ERR_* counterparts. I'd rather have one set of > > error codes. > > The main reason is, in lowmem fsck mode, we are not checking inodes or > ref/backref in batch. > > If using I_ERR and REF_ERR, we can mixing them up as they share the same > bits. > > So we introduced the new error bitmap, to make sure all error bits won't > cover each other. > > It may be better if we rearrange I_ERR/REF_ERR to avoid conflicts. > E.g, let REF_ERR_ starts from lowest bit and let I_ERR_ starts from high > bits. Yes please. Third namespace for existing error bits is not a good option. Move the I_ERR bits to start from 32 and use them in the low-mem code that's been merged to devel. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At 11/08/2016 01:05 AM, David Sterba wrote: > On Thu, Nov 03, 2016 at 09:58:21AM +0800, Qu Wenruo wrote: >> >> >> At 11/02/2016 11:21 PM, David Sterba wrote: >>> On Wed, Sep 21, 2016 at 11:15:52AM +0800, Qu Wenruo wrote: >>>> From: Lu Fengqi <lufq.fnst@cn.fujitsu.com> >>>> >>>> Introduce a new function find_dir_item() to find DIR_ITEM for the given >>>> key, and check it with the specified INODE_REF/INODE_EXTREF match. >>>> >>>> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> >>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >>>> --- >>>> cmds-check.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 140 insertions(+) >>>> >>>> diff --git a/cmds-check.c b/cmds-check.c >>>> index 998ba63..4e25804 100644 >>>> --- a/cmds-check.c >>>> +++ b/cmds-check.c >>>> @@ -3848,6 +3848,146 @@ out: >>>> return err; >>>> } >>>> >>>> +#define ROOT_DIR_ERROR (1<<1) /* bad root_dir */ >>>> +#define DIR_ITEM_MISSING (1<<2) /* DIR_ITEM not found */ >>>> +#define DIR_ITEM_MISMATCH (1<<3) /* DIR_ITEM found but not match */ >>> >>> What's the reason for another definition of the error codes? They're >>> mostly copied from te I_ERR_* counterparts. I'd rather have one set of >>> error codes. >> >> The main reason is, in lowmem fsck mode, we are not checking inodes or >> ref/backref in batch. >> >> If using I_ERR and REF_ERR, we can mixing them up as they share the same >> bits. >> >> So we introduced the new error bitmap, to make sure all error bits won't >> cover each other. >> >> It may be better if we rearrange I_ERR/REF_ERR to avoid conflicts. >> E.g, let REF_ERR_ starts from lowest bit and let I_ERR_ starts from high >> bits. > > Yes please. Third namespace for existing error bits is not a good > option. Move the I_ERR bits to start from 32 and use them in the low-mem > code that's been merged to devel. > > Should I submit a separate fix or replace the patchset? Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At 11/08/2016 01:05 AM, David Sterba wrote: > On Thu, Nov 03, 2016 at 09:58:21AM +0800, Qu Wenruo wrote: >> >> >> At 11/02/2016 11:21 PM, David Sterba wrote: >>> On Wed, Sep 21, 2016 at 11:15:52AM +0800, Qu Wenruo wrote: >>>> From: Lu Fengqi <lufq.fnst@cn.fujitsu.com> >>>> >>>> Introduce a new function find_dir_item() to find DIR_ITEM for the given >>>> key, and check it with the specified INODE_REF/INODE_EXTREF match. >>>> >>>> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> >>>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >>>> --- >>>> cmds-check.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 140 insertions(+) >>>> >>>> diff --git a/cmds-check.c b/cmds-check.c >>>> index 998ba63..4e25804 100644 >>>> --- a/cmds-check.c >>>> +++ b/cmds-check.c >>>> @@ -3848,6 +3848,146 @@ out: >>>> return err; >>>> } >>>> >>>> +#define ROOT_DIR_ERROR (1<<1) /* bad root_dir */ >>>> +#define DIR_ITEM_MISSING (1<<2) /* DIR_ITEM not found */ >>>> +#define DIR_ITEM_MISMATCH (1<<3) /* DIR_ITEM found but not match */ >>> >>> What's the reason for another definition of the error codes? They're >>> mostly copied from te I_ERR_* counterparts. I'd rather have one set of >>> error codes. >> >> The main reason is, in lowmem fsck mode, we are not checking inodes or >> ref/backref in batch. >> >> If using I_ERR and REF_ERR, we can mixing them up as they share the same >> bits. >> >> So we introduced the new error bitmap, to make sure all error bits won't >> cover each other. >> >> It may be better if we rearrange I_ERR/REF_ERR to avoid conflicts. >> E.g, let REF_ERR_ starts from lowest bit and let I_ERR_ starts from high >> bits. > > Yes please. Third namespace for existing error bits is not a good > option. Move the I_ERR bits to start from 32 and use them in the low-mem > code that's been merged to devel. Hi David, I didn't see such fix in devel branch. In cmds-check.c, there I_ERR_* and REF_ERR_* are still using the old range. Is the fix in another branch? Or I missed something again? Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 08, 2016 at 09:45:54AM +0800, Qu Wenruo wrote: > > Yes please. Third namespace for existing error bits is not a good > > option. Move the I_ERR bits to start from 32 and use them in the low-mem > > code that's been merged to devel. > > > Should I submit a separate fix or replace the patchset? Separate patches please. The check patches are at the beginning of devel and there are several cleanup patches on top of them so that would probably cause too many merge conflicts. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 16, 2016 at 10:27:59AM +0800, Qu Wenruo wrote: > > Yes please. Third namespace for existing error bits is not a good > > option. Move the I_ERR bits to start from 32 and use them in the low-mem > > code that's been merged to devel. > > I didn't see such fix in devel branch. Well, that's because nobody implemented it and I was not intending to do it myself as it's a followup to yor lowmem patchset in devel. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At 12/01/2016 12:34 AM, David Sterba wrote: > On Wed, Nov 16, 2016 at 10:27:59AM +0800, Qu Wenruo wrote: >>> Yes please. Third namespace for existing error bits is not a good >>> option. Move the I_ERR bits to start from 32 and use them in the low-mem >>> code that's been merged to devel. >> >> I didn't see such fix in devel branch. > > Well, that's because nobody implemented it and I was not intending to do > it myself as it's a followup to yor lowmem patchset in devel. > > OK, I'm going to fix it soon. Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
At 11/02/2016 11:21 PM, David Sterba wrote: > On Wed, Sep 21, 2016 at 11:15:52AM +0800, Qu Wenruo wrote: >> From: Lu Fengqi <lufq.fnst@cn.fujitsu.com> >> >> Introduce a new function find_dir_item() to find DIR_ITEM for the given >> key, and check it with the specified INODE_REF/INODE_EXTREF match. >> >> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com> >> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com> >> --- >> cmds-check.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 140 insertions(+) >> >> diff --git a/cmds-check.c b/cmds-check.c >> index 998ba63..4e25804 100644 >> --- a/cmds-check.c >> +++ b/cmds-check.c >> @@ -3848,6 +3848,146 @@ out: >> return err; >> } >> >> +#define ROOT_DIR_ERROR (1<<1) /* bad root_dir */ >> +#define DIR_ITEM_MISSING (1<<2) /* DIR_ITEM not found */ >> +#define DIR_ITEM_MISMATCH (1<<3) /* DIR_ITEM found but not match */ > > What's the reason for another definition of the error codes? They're > mostly copied from te I_ERR_* counterparts. I'd rather have one set of > error codes. I tried to merge them into a 32bit error bits. But things turns out that, I_ERR and REF_ERR have already taken 28 bits. For a int type, we only have extra 4bits. All fs tree level error code are OK to merge. But extent tree level errors, including extent ref/backref error bits, and tree block level errors, like bad key type in current content or bad item size, have no corresponding bits. These bits are already over 4 bits. Yes, we can expand the error bit to u64, but that will be a huge modification for both the original mode and lowmem mode. What about letting me separate the lowmem code from cmds-check.c into check/lowmem.c and using the current error when you're going to create check/ directory? It should be OK to merge all lowmem error bits into one int type, but not possible to do it with original mode error bits without expanding the int type. Since we have different error bit standard, this leads to completely different usage on these bits. A lot of lowmem bit won't be used by original mode and vice-verse. Thanks, Qu -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/cmds-check.c b/cmds-check.c index 998ba63..4e25804 100644 --- a/cmds-check.c +++ b/cmds-check.c @@ -3848,6 +3848,146 @@ out: return err; } +#define ROOT_DIR_ERROR (1<<1) /* bad root_dir */ +#define DIR_ITEM_MISSING (1<<2) /* DIR_ITEM not found */ +#define DIR_ITEM_MISMATCH (1<<3) /* DIR_ITEM found but not match */ + +/* + * Find DIR_ITEM/DIR_INDEX for the given key and check it with the specified + * INODE_REF/INODE_EXTREF match. + * + * @root: the root of the fs/file tree + * @ref_key: the key of the INODE_REF/INODE_EXTREF + * @key: the key of the DIR_ITEM/DIR_INDEX + * @index: the index in the INODE_REF/INODE_EXTREF, be used to + * distinguish root_dir between normal dir/file + * @name: the name in the INODE_REF/INODE_EXTREF + * @namelen: the length of name in the INODE_REF/INODE_EXTREF + * @mode: the st_mode of INODE_ITEM + * + * Return 0 if no error occurred. + * Return ROOT_DIR_ERROR if found DIR_ITEM/DIR_INDEX for root_dir. + * Return DIR_ITEM_MISSING if couldn't find DIR_ITEM/DIR_INDEX for normal + * dir/file. + * Return DIR_ITEM_MISMATCH if INODE_REF/INODE_EXTREF and DIR_ITEM/DIR_INDEX + * not match for normal dir/file. + */ +static int find_dir_item(struct btrfs_root *root, struct btrfs_key *ref_key, + struct btrfs_key *key, u64 index, char *name, + u32 namelen, u32 mode) +{ + struct btrfs_path path; + struct extent_buffer *node; + struct btrfs_dir_item *di; + struct btrfs_key location; + char namebuf[BTRFS_NAME_LEN] = {0}; + u32 total; + u32 cur = 0; + u32 len; + u32 name_len; + u32 data_len; + u8 filetype; + int slot; + int ret; + + btrfs_init_path(&path); + ret = btrfs_search_slot(NULL, root, key, &path, 0, 0); + if (ret < 0) { + ret = DIR_ITEM_MISSING; + goto out; + } + + /* Process root dir and goto out*/ + if (index == 0) { + if (ret == 0) { + ret = ROOT_DIR_ERROR; + error( + "root %llu INODE %s[%llu %llu] ROOT_DIR shouldn't have %s", + root->objectid, + ref_key->type == BTRFS_INODE_REF_KEY ? + "REF" : "EXTREF", + ref_key->objectid, ref_key->offset, + key->type == BTRFS_DIR_ITEM_KEY ? + "DIR_ITEM" : "DIR_INDEX"); + } else { + ret = 0; + } + + goto out; + } + + /* Process normal file/dir */ + if (ret > 0) { + ret = DIR_ITEM_MISSING; + error( + "root %llu INODE %s[%llu %llu] doesn't have related %s[%llu %llu] namelen %u filename %s filetype %d", + root->objectid, + ref_key->type == BTRFS_INODE_REF_KEY ? "REF" : "EXTREF", + ref_key->objectid, ref_key->offset, + key->type == BTRFS_DIR_ITEM_KEY ? + "DIR_ITEM" : "DIR_INDEX", + key->objectid, key->offset, namelen, name, + imode_to_type(mode)); + goto out; + } + + /* Check whether inode_id/filetype/name match */ + node = path.nodes[0]; + slot = path.slots[0]; + di = btrfs_item_ptr(node, slot, struct btrfs_dir_item); + total = btrfs_item_size_nr(node, slot); + while (cur < total) { + ret = DIR_ITEM_MISMATCH; + name_len = btrfs_dir_name_len(node, di); + data_len = btrfs_dir_data_len(node, di); + + btrfs_dir_item_key_to_cpu(node, di, &location); + if (location.objectid != ref_key->objectid || + location.type != BTRFS_INODE_ITEM_KEY || + location.offset != 0) + goto next; + + filetype = btrfs_dir_type(node, di); + if (imode_to_type(mode) != filetype) + goto next; + + if (name_len <= BTRFS_NAME_LEN) { + len = name_len; + } else { + len = BTRFS_NAME_LEN; + fprintf(stderr, + "Warning: root %llu %s[%llu %llu] name too long\n", + root->objectid, + key->type == BTRFS_DIR_ITEM_KEY ? + "DIR_ITEM" : "DIR_INDEX", + key->objectid, key->offset); + } + read_extent_buffer(node, namebuf, (unsigned long)(di + 1), len); + if (len != namelen || strncmp(namebuf, name, len)) + goto next; + + ret = 0; + goto out; +next: + len = sizeof(*di) + name_len + data_len; + di = (struct btrfs_dir_item *)((char *)di + len); + cur += len; + } + if (ret == DIR_ITEM_MISMATCH) + error( + "root %llu INODE %s[%llu %llu] and %s[%llu %llu] mismatch namelen %u filename %s filetype %d", + root->objectid, + ref_key->type == BTRFS_INODE_REF_KEY ? "REF" : "EXTREF", + ref_key->objectid, ref_key->offset, + key->type == BTRFS_DIR_ITEM_KEY ? + "DIR_ITEM" : "DIR_INDEX", + key->objectid, key->offset, namelen, name, + imode_to_type(mode)); +out: + btrfs_release_path(&path); + return ret; +} + static int all_backpointers_checked(struct extent_record *rec, int print_errs) { struct list_head *cur = rec->backrefs.next;