Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Sungjong Seo" <sj1557.seo@samsung.com>
To: "'Tetsuhiro Kohada'" <kohada.t2@gmail.com>
Cc: <kohada.tetsuhiro@dc.mitsubishielectric.co.jp>,
	<mori.takahiro@ab.mitsubishielectric.co.jp>,
	<motai.hirotaka@aj.mitsubishielectric.co.jp>,
	"'Namjae Jeon'" <namjae.jeon@samsung.com>,
	<linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 2/2] exfat: unify name extraction
Date: Fri, 21 Aug 2020 19:41:38 +0900	[thread overview]
Message-ID: <860b01d677a7$a62bf230$f283d690$@samsung.com> (raw)
In-Reply-To: <bbd9355c-cd48-b961-0a91-771a702c03df@gmail.com>

> 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@gmail.com>


  reply	other threads:[~2020-08-21 10:41 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20200806055718epcas1p33009b21ebf96b48d6e3f819065fade28@epcas1p3.samsung.com>
2020-08-06  5:56 ` [PATCH 1/2] exfat: add NameLength check when extracting name Tetsuhiro Kohada
2020-08-06  5:56   ` [PATCH 2/2] exfat: unify name extraction Tetsuhiro Kohada
2020-08-08 17:19     ` Sungjong Seo
2020-08-12  6:02       ` Tetsuhiro Kohada
2020-08-21 10:41         ` Sungjong Seo [this message]
2020-08-25 10:15           ` Tetsuhiro Kohada
2020-08-08 16:54   ` [PATCH 1/2] exfat: add NameLength check when extracting name Sungjong Seo
2020-08-12  4:53     ` Tetsuhiro Kohada
2020-08-10  6:13   ` Namjae Jeon
2020-08-12 15:04     ` Tetsuhiro Kohada
2020-08-13  2:53       ` Namjae Jeon
2020-08-17  9:26         ` Kohada.Tetsuhiro

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='860b01d677a7$a62bf230$f283d690$@samsung.com' \
    --to=sj1557.seo@samsung.com \
    --cc=kohada.t2@gmail.com \
    --cc=kohada.tetsuhiro@dc.mitsubishielectric.co.jp \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mori.takahiro@ab.mitsubishielectric.co.jp \
    --cc=motai.hirotaka@aj.mitsubishielectric.co.jp \
    --cc=namjae.jeon@samsung.com \
    --subject='RE: [PATCH 2/2] exfat: unify name extraction' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).