RE: [PATCH v2 02/10] exfat: add exfat_get_empty_dentry_set() helper

[서성종 <sj1557.seo@samsung.com>]

> > > +        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.
Do you mean the part that will be deleted by the patch [7/10] mentioned below?
If so, I think you may be misunderstanding it.

>
> 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.
Yeah, as you said, this patch set should keep the original logic except
for the sync related parts. The reason I left a review comment is because
the code before this patch set allows deleted dentries to follow unused
dentries.

Please let me know if I missed anything.

> 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".
Sounds good :)