| Message ID | PUZPR04MB6316C5AC606AABE0E08AC2A6819EA@PUZPR04MB6316.apcprd04.prod.outlook.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exfat: improve sync dentry | expand |
> This helper is used to lookup empty dentry set. If there are no enough > empty dentries at the input location, this helper will return the number > of dentries that need to be skipped for the next lookup. > > Signed-off-by: Yuezhang Mo <Yuezhang.Mo@sony.com> > Reviewed-by: Andy Wu <Andy.Wu@sony.com> > Reviewed-by: Aoyama Wataru <wataru.aoyama@sony.com> > --- > fs/exfat/dir.c | 77 +++++++++++++++++++++++++++++++++++++++++++++ > fs/exfat/exfat_fs.h | 3 ++ > 2 files changed, 80 insertions(+) > > diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index > cea9231d2fda..a5c8cd19aca6 100644 > --- a/fs/exfat/dir.c > +++ b/fs/exfat/dir.c > @@ -950,6 +950,83 @@ int exfat_get_dentry_set(struct exfat_entry_set_cache > *es, > return -EIO; > } > > +static int exfat_validate_empty_dentry_set(struct exfat_entry_set_cache > +*es) { > + struct exfat_dentry *ep; > + struct buffer_head *bh; > + int i, off; > + bool unused_hit = false; > + > + for (i = 0; i < es->num_entries; i++) { > + ep = exfat_get_dentry_cached(es, i); > + if (ep->type == EXFAT_UNUSED) > + unused_hit = true; > + else if (IS_EXFAT_DELETED(ep->type)) { Although it violates the specification for a deleted entry to follow an unused entry, some exFAT implementations could work like this. Therefore, to improve compatibility, why don't we allow this? I believe there will be no functional problem even if this is allowed. > + if (unused_hit) > + goto out; > + } else { > + if (unused_hit) > + goto out; Label "out" does not look like an error situation. Let's use "out_err" instead of "out". > + > + i++; > + goto count_skip_entries; > + } > + } > + > + return 0;
> > + for (i = 0; i < es->num_entries; i++) { > > + ep = exfat_get_dentry_cached(es, i); > > + if (ep->type == EXFAT_UNUSED) > > + unused_hit = true; > > + else if (IS_EXFAT_DELETED(ep->type)) { > > Although it violates the specification for a deleted entry to follow an unused > entry, some exFAT implementations could work like this. > > Therefore, to improve compatibility, why don't we allow this? > I believe there will be no functional problem even if this is allowed. This check existed before this patch set. This patch set is intended to improve the performance of sync dentry, I don't think it is a good idea to change other logic in this patch set. Patch [7/10] moves the check from exfat_search_empty_slot() to exfat_validate_empty_dentry_set(). - if (hint_femp->eidx != EXFAT_HINT_NONE && - hint_femp->count == CNT_UNUSED_HIT) { - /* unused empty group means - * an empty group which includes - * unused dentry - */ - exfat_fs_error(sb, - "found bogus dentry(%d) beyond unused empty group(%d) (start_clu : %u, cur_clu : %u)", - dentry, hint_femp->eidx, - p_dir->dir, clu.dir); > > > + if (unused_hit) > > + goto out; > > + } else { > > + if (unused_hit) > > + goto out; > Label "out" does not look like an error situation. > Let's use "out_err" instead of "out". Makes sense, I will rename the label to "err_deleted_after_unused".
diff --git a/fs/exfat/dir.c b/fs/exfat/dir.c index cea9231d2fda..a5c8cd19aca6 100644 --- a/fs/exfat/dir.c +++ b/fs/exfat/dir.c @@ -950,6 +950,83 @@ int exfat_get_dentry_set(struct exfat_entry_set_cache *es, return -EIO; } +static int exfat_validate_empty_dentry_set(struct exfat_entry_set_cache *es) +{ + struct exfat_dentry *ep; + struct buffer_head *bh; + int i, off; + bool unused_hit = false; + + for (i = 0; i < es->num_entries; i++) { + ep = exfat_get_dentry_cached(es, i); + if (ep->type == EXFAT_UNUSED) + unused_hit = true; + else if (IS_EXFAT_DELETED(ep->type)) { + if (unused_hit) + goto out; + } else { + if (unused_hit) + goto out; + + i++; + goto count_skip_entries; + } + } + + return 0; + +out: + off = es->start_off + (i << DENTRY_SIZE_BITS); + bh = es->bh[EXFAT_B_TO_BLK(off, es->sb)]; + + exfat_fs_error(es->sb, + "in sector %lld, dentry %d should be unused, but 0x%x", + bh->b_blocknr, off >> DENTRY_SIZE_BITS, ep->type); + + return -EIO; + +count_skip_entries: + es->num_entries = EXFAT_B_TO_DEN(EXFAT_BLK_TO_B(es->num_bh, es->sb) - es->start_off); + for (; i < es->num_entries; i++) { + ep = exfat_get_dentry_cached(es, i); + if (IS_EXFAT_DELETED(ep->type)) + break; + } + + return i; +} + +/* + * Get an empty dentry set. + * + * in: + * sb+p_dir+entry: indicates the empty dentry location + * num_entries: specifies how many empty dentries should be included. + * out: + * es: pointer of empty dentry set on success. + * return: + * 0 : on success + * >0 : the dentries are not empty, the return value is the number of + * dentries to be skipped for the next lookup. + * <0 : on failure + */ +int exfat_get_empty_dentry_set(struct exfat_entry_set_cache *es, + struct super_block *sb, struct exfat_chain *p_dir, + int entry, unsigned int num_entries) +{ + int ret; + + ret = __exfat_get_dentry_set(es, sb, p_dir, entry, num_entries); + if (ret < 0) + return ret; + + ret = exfat_validate_empty_dentry_set(es); + if (ret) + exfat_put_dentry_set(es, false); + + return ret; +} + static inline void exfat_reset_empty_hint(struct exfat_hint_femp *hint_femp) { hint_femp->eidx = EXFAT_HINT_NONE; diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index d6c4b75cdf6f..542136b14a2e 100644 --- a/fs/exfat/exfat_fs.h +++ b/fs/exfat/exfat_fs.h @@ -502,6 +502,9 @@ struct exfat_dentry *exfat_get_dentry_cached(struct exfat_entry_set_cache *es, int exfat_get_dentry_set(struct exfat_entry_set_cache *es, struct super_block *sb, struct exfat_chain *p_dir, int entry, unsigned int num_entries); +int exfat_get_empty_dentry_set(struct exfat_entry_set_cache *es, + struct super_block *sb, struct exfat_chain *p_dir, int entry, + unsigned int num_entries); int exfat_put_dentry_set(struct exfat_entry_set_cache *es, int sync); int exfat_count_dir_entries(struct super_block *sb, struct exfat_chain *p_dir);