RE: [PATCH 2/2] exfat: unify name extraction

[Sungjong Seo]

> Thanks for your reply.
> 
> On 2020/08/09 2:19, Sungjong Seo wrote:
> > [snip]
> >> @@ -963,80 +942,38 @@ int exfat_find_dir_entry(struct super_block
> >> *sb, struct exfat_inode_info *ei,
> >>   			num_empty = 0;
> >>   			candi_empty.eidx = EXFAT_HINT_NONE;
> >>
> > [snip]
> >>
> >> -			if (entry_type &
> >> -					(TYPE_CRITICAL_SEC |
> > TYPE_BENIGN_SEC)) {
> >> -				if (step == DIRENT_STEP_SECD) {
> >> -					if (++order == num_ext)
> >> -						goto found;
> >> -					continue;
> >> -				}
> >> +			exfat_get_uniname_from_name_entries(es, &uni_name);
> >
> > It is needed to check a return value.
> 
> I'll fix it in v2.
> 
> 
> >> +			exfat_free_dentry_set(es, false);
> >> +
> >> +			if (!exfat_uniname_ncmp(sb,
> >> +						p_uniname->name,
> >> +						uni_name.name,
> >> +						name_len)) {
> >> +				/* set the last used position as hint */
> >> +				hint_stat->clu = clu.dir;
> >> +				hint_stat->eidx = dentry;
> >
> > eidx and clu of hint_stat should have one for the next entry we'll
> > start looking for.
> > Did you intentionally change the concept?
> 
> Yes, this is intentional.
> Essentially, the "Hint" concept is to reduce the next seek cost with
> minimal cost.
> There is a difference in the position of the hint, but the concept is the
> same.
> As you can see, the patched code strategy doesn't move from current
> position.
> Basically, the original code strategy is advancing only one dentry.(It's
> the "minimum cost") However, when it reaches the cluster boundary, it gets
> the next cluster and error handling.

I didn't get exactly what "original code" is.
Do you mean whole code lines for exfat_find_dir_entry()?
Or just only for handling the hint in it?

The strategy of original code for hint is advancing not one dentry but one dentry_set.
If a hint position is not moved to next like the patched code,
caller have to start at old dentry_set that could be already loaded on dentry cache.

Let's think the case of searching through all files sequentially.
The patched code should check twice per a file.
No better than the original policy.

> Getting the next cluster The error handling already exists at the end of
> the while loop, so the code is duplicated.
> These costs should be paid next time and are no longer the "minimum cost".

I agree with your words, "These costs should be paid next time".
If so, how about moving the cluster handling for a hint dentry to
the beginning of the function while keeping the original policy?

BTW, this patch is not related to the hint code.
I think it would be better to keep the original code in this patch and improve it with a separate patch.

> Should I add this to the commit-message?
> 
> 
> BR
> ---
> Tetsuhiro Kohada <kohada.t2@xxxxxxxxx>