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

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

[Yuezhang.Mo]

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)) {
+			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);
 
-- 
2.25.1

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

[Sungjong Seo]

> 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;

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

[Yuezhang.Mo]

> > +	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".

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

[서성종]

> > > +        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 :)

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

[Yuezhang.Mo]

> From: 서성종 <sj1557.seo@samsung.com>
> Sent: Monday, March 4, 2024 12:43 PM
> >
> > 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.

Which commit changed to allow deleted dentries to follow unused dentries?

The following code still exists if without this patch set. It does not allow
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);
> >

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

[Sungjong Seo]

> > Sent: Monday, March 4, 2024 12:43 PM
> > >
> > > 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.
> 
> Which commit changed to allow deleted dentries to follow unused dentries?
Not changed. It has been allowed from the initial commit as follows.
5f2aa075070c ("exfat: add inode operations")

> 
> The following code still exists if without this patch set. It does not
> allow deleted dentries to follow unused dentries.

It may be the same part as the code you mentioned, but remember that
the first if-statement handles both an unused dentry and
a deleted dentry together.

static int exfat_search_empty_slot(...)
{
    ...
    if (type == TYPE_UNUSED || type == TYPE_DELETED) {
        ...
    } else {
        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);
             return -EIO;
       }
       ...
    }
    ...
}

> 
> > 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);
> > >

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

[Yuezhang.Mo]

> From: Sungjong Seo <sj1557.seo@samsung.com>
> Sent: Monday, March 4, 2024 4:43 PM
> To: Mo, Yuezhang <Yuezhang.Mo@sony.com>; linkinjeon@kernel.org
>
> >
> > The following code still exists if without this patch set. It does not
> > allow deleted dentries to follow unused dentries.
> 
> It may be the same part as the code you mentioned, but remember that
> the first if-statement handles both an unused dentry and
> a deleted dentry together.

Thanks for your detailed explanation. 

I will update the code as follows, and I think it is very necessary to add such
comments.

        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, allow it.
                         */
                        if (unused_hit)
                                continue;
                } else {
                        /* Used entry are not allowed to follow unused entry */
                        if (unused_hit)
                                goto err_used_follow_unused;

                        i++;
                        goto count_skip_entries;
                }
        }

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

[Sungjong Seo]

> Wataru.Aoyama@sony.com
> Subject: RE: [PATCH v2 02/10] exfat: add exfat_get_empty_dentry_set()
> helper
> 
> > From: Sungjong Seo <sj1557.seo@samsung.com>
> > Sent: Monday, March 4, 2024 4:43 PM
> > To: Mo, Yuezhang <Yuezhang.Mo@sony.com>; linkinjeon@kernel.org
> >
> > >
> > > The following code still exists if without this patch set. It does
> > > not allow deleted dentries to follow unused dentries.
> >
> > It may be the same part as the code you mentioned, but remember that
> > the first if-statement handles both an unused dentry and a deleted
> > dentry together.
> 
> Thanks for your detailed explanation.
> 
> I will update the code as follows, and I think it is very necessary to add
> such comments.
I think so :)

> 
>         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, allow it.
>                          */
>                         if (unused_hit)
However, I don't think it's good idea to check for unused_hit here.
How about moving the comment at the start of the for-loop and changing
the code as follows?

/*
 * ONLY UNUSED OR DELETED DENTRIES ARE ALLOWED:
 * 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, let's allow it.
 */
 for (i = 0; i < es->num_entries; i++) {
         ep = exfat_get_dentry_cached(es, i);
         if (ep->type == EXFAT_UNUSED) {
                 unused_hit = true;
                 continue;
         }
         if (IS_EXFAT_DELETED(ep->type))
                 continue;
         if (unused_hit)
                 goto err_used_follow_unused;
         i++;
         goto count_skip_entries;
 }

Or we could use if / else-if as follows.

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 err_used_follow_unused;
                i++;
                goto count_skip_entries;
        }
}

>                                 continue;
>                 } else {
>                         /* Used entry are not allowed to follow unused
entry */
>                         if (unused_hit)
>                                 goto err_used_follow_unused;
> 
>                         i++;
>                         goto count_skip_entries;
>                 }
>         }