Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Sungjong Seo" <sj1557.seo@samsung.com>
To:     "'Namjae Jeon'" <linkinjeon@kernel.org>
Cc:     "'linux-fsdevel'" <linux-fsdevel@vger.kernel.org>,
        "'linux-kernel'" <linux-kernel@vger.kernel.org>,
        <sj1557.seo@samsung.com>
In-Reply-To: <PUZPR04MB631665D55F4F0B290D0D543581379@PUZPR04MB6316.apcprd04.prod.outlook.com>
Subject: RE: [PATCH v1 1/2] exfat: simplify empty entry hint
Date:   Tue, 1 Nov 2022 15:26:24 +0900
Message-ID: <322d01d8edba$dd8e7370$98ab5a50$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQJj/zaSoZMPYtmG2fVrgFRBP80hpwJaGMqRAj8XxoYBK3U6ygIPfv+NrNRwc8A=
Content-Language: ko
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <CGME20221019072850epcas1p459b27e0d44eb0cc36ec09e9a734dcf60@epcas1p4.samsung.com>
        <PUZPR04MB6316EBE97C82DFBEFE3CCDAF812B9@PUZPR04MB6316.apcprd04.prod.outlook.com>
        <000001d8ece8$0241bca0$06c535e0$@samsung.com>
        <CAKYAXd__ypbjLpnNVDxf3UE4M+au2QwYYe2PeY8QsKZCBaO54w@mail.gmail.com>
        <PUZPR04MB631665D55F4F0B290D0D543581379@PUZPR04MB6316.apcprd04.prod.outlook.com>
Precedence: bulk

Hi, Yuezhang,
I am sorry that I cannot reply directly to you due to environmental restric=
tions.

> > > BTW, ei->hint_femp.count was already reset at the beginning of
> > > exfat_find_dir_entry(). So condition-check above could be removed.
> > > Is there any scenario I'm missing?
>=20
> If the search does not start from the first entry and there are not enoug=
h
> empty entries.
> This condition will be true when rewinding.

I didn't get what you said. do you mean =22ei->hint_femp.eidx > dentry=22?
Even so, it could be true, if the search does not start from the first entr=
y
and there are =22enough=22 empty entries.

Anyway, what I'm saying is =22ei->hint_femp.count < num_entries=22, and hin=
t_femp
Seems to be reset as EXFAT_HINT_NONE with 0 by below code.

+	if (ei->hint_femp.eidx =21=3D EXFAT_HINT_NONE &&
+	    ei->hint_femp.count < num_entries)
+		ei->hint_femp.eidx =3D EXFAT_HINT_NONE;
+
+	if (ei->hint_femp.eidx =3D=3D EXFAT_HINT_NONE)
+		ei->hint_femp.count =3D 0;

After then, ei->hint_femp can be updated only if there are enough free entr=
ies.

>=20
> > >> -	candi_empty.eidx =3D EXFAT_HINT_NONE;
> > >> +	if (ei->hint_femp.eidx =21=3D EXFAT_HINT_NONE &&
> > >> +	    ei->hint_femp.count < num_entries)
> > >> +		ei->hint_femp.eidx =3D EXFAT_HINT_NONE;
> > >> +
> > >> +	if (ei->hint_femp.eidx =3D=3D EXFAT_HINT_NONE)
> > >> +		ei->hint_femp.count =3D 0;
> > >> +
> > >> +	candi_empty =3D ei->hint_femp;
> > >> +
> > >
> > > It would be nice to make the code block above a static inline functio=
n
> > > as well.
>=20
> Since the code is called once only in exfat_find_dir_entry(), I didn't
> make a function for the code.
>=20
> How about make function exfat_reset_empty_hint_if_not_enough() for this
> code?
> The function name is a bit long=E2=98=B9,=20do=20you=20have=20a=20better=
=20idea?=0D=0A>=20=0D=0A>=20Or=20maybe,=20we=20can=20add=20exfat_reset_empt=
y_hint()=20and=20unconditionally=20reset=0D=0A>=20ei->hint_femp=20in=20it.=
=0D=0A=0D=0AIt's=20always=20difficult=20for=20me=20as=20well=20:).=0D=0AWha=
t=20do=20you=20think=20of=20exfat_test_reset_empty_hint()=20or=0D=0Aexfat_c=
ond_reset_empty_hint()?=0D=0A=0D=0A>=20>=20-----Original=20Message-----=0D=
=0A>=20>=20From:=20Namjae=20Jeon=20<linkinjeon=40kernel.org>=0D=0A>=20>=20S=
ent:=20Monday,=20October=2031,=202022=202:31=20PM=0D=0A>=20>=20To:=20Sungjo=
ng=20Seo=20<sj1557.seo=40samsung.com>;=20Mo,=20Yuezhang=0D=0A>=20>=20<Yuezh=
ang.Mo=40sony.com>=0D=0A>=20>=20Cc:=20linux-fsdevel=20<linux-fsdevel=40vger=
.kernel.org>;=20linux-kernel=0D=0A>=20>=20<linux-kernel=40vger.kernel.org>=
=0D=0A>=20>=20Subject:=20Re:=20=5BPATCH=20v1=201/2=5D=20exfat:=20simplify=
=20empty=20entry=20hint=0D=0A>=20>=0D=0A>=20>=20Add=20missing=20Cc:=20Yuezh=
ang=20Mo.=0D=0A>=20>=0D=0A>=20>=202022-10-31=2014:16=20GMT+09:00,=20Sungjon=
g=20Seo=20<sj1557.seo=40samsung.com>:=0D=0A>=20>=20>=20Hello,=20Yuezhang=20=
Mo,=0D=0A>=20>=20>=0D=0A>=20>=20>>=20This=20commit=20adds=20exfat_hint_empt=
y_entry()=20to=20reduce=20code=20complexity=0D=0A>=20>=20>>=20and=20make=20=
code=20more=20readable.=0D=0A>=20>=20>>=0D=0A>=20>=20>>=20Signed-off-by:=20=
Yuezhang=20Mo=20<Yuezhang.Mo=40sony.com>=0D=0A>=20>=20>>=20Reviewed-by:=20A=
ndy=20Wu=20<Andy.Wu=40sony.com>=0D=0A>=20>=20>>=20Reviewed-by:=20Aoyama=20W=
ataru=20<wataru.aoyama=40sony.com>=0D=0A>=20>=20>>=20---=0D=0A>=20>=20>>=20=
=20fs/exfat/dir.c=20=7C=2056=0D=0A>=20>=20>>=20++++++++++++++++++++++++++++=
----------------------=0D=0A>=20>=20>>=20=201=20file=20changed,=2032=20inse=
rtions(+),=2024=20deletions(-)=0D=0A>=20>=20>>=0D=0A>=20>=20>>=20diff=20--g=
it=20a/fs/exfat/dir.c=20b/fs/exfat/dir.c=20index=0D=0A>=20>=20>>=207b648b66=
62f0..a569f285f4fd=20100644=0D=0A>=20>=20>>=20---=20a/fs/exfat/dir.c=0D=0A>=
=20>=20>>=20+++=20b/fs/exfat/dir.c=0D=0A>=20>=20>>=20=40=40=20-934,6=20+934=
,24=20=40=40=20struct=20exfat_entry_set_cache=0D=0A>=20>=20>>=20*exfat_get_=
dentry_set(struct=20super_block=20*sb,=0D=0A>=20>=20>>=20=20=09return=20NUL=
L;=0D=0A>=20>=20>>=20=20=7D=0D=0A>=20>=20>>=0D=0A>=20>=20>>=20+static=20inl=
ine=20void=20exfat_hint_empty_entry(struct=20exfat_inode_info=0D=0A>=20*ei,=
=0D=0A>=20>=20>>=20+=09=09struct=20exfat_hint_femp=20*candi_empty,=20struct=
=20exfat_chain=20*clu,=0D=0A>=20>=20>>=20+=09=09int=20dentry,=20int=20num_e=
ntries)=0D=0A>=20>=20>>=20+=7B=0D=0A>=20>=20>>=20+=09if=20(ei->hint_femp.ei=
dx=20=3D=3D=20EXFAT_HINT_NONE=20=7C=7C=0D=0A>=20>=20>>=20+=09=20=20=20=20ei=
->hint_femp.count=20<=20num_entries=20=7C=7C=0D=0A>=20>=20>=0D=0A>=20>=20>=
=20It=20seems=20like=20a=20good=20approach.=0D=0A>=20>=20>=20BTW,=20ei->hin=
t_femp.count=20was=20already=20reset=20at=20the=20beginning=20of=0D=0A>=20>=
=20>=20exfat_find_dir_entry().=20So=20condition-check=20above=20could=20be=
=20removed.=0D=0A>=20>=20>=20Is=20there=20any=20scenario=20I'm=20missing?=
=0D=0A>=20>=20>=0D=0A>=20>=20>>=20+=09=20=20=20=20ei->hint_femp.eidx=20>=20=
dentry)=20=7B=0D=0A>=20>=20>>=20+=09=09if=20(candi_empty->count=20=3D=3D=20=
0)=20=7B=0D=0A>=20>=20>>=20+=09=09=09candi_empty->cur=20=3D=20*clu;=0D=0A>=
=20>=20>>=20+=09=09=09candi_empty->eidx=20=3D=20dentry;=0D=0A>=20>=20>>=20+=
=09=09=7D=0D=0A>=20>=20>>=20+=0D=0A>=20>=20>>=20+=09=09candi_empty->count++=
;=0D=0A>=20>=20>>=20+=09=09if=20(candi_empty->count=20=3D=3D=20num_entries)=
=0D=0A>=20>=20>>=20+=09=09=09ei->hint_femp=20=3D=20*candi_empty;=0D=0A>=20>=
=20>>=20+=09=7D=0D=0A>=20>=20>>=20+=7D=0D=0A>=20>=20>>=20+=0D=0A>=20>=20>>=
=20=20enum=20=7B=0D=0A>=20>=20>>=20=20=09DIRENT_STEP_FILE,=0D=0A>=20>=20>>=
=20=20=09DIRENT_STEP_STRM,=0D=0A>=20>=20>>=20=40=40=20-958,7=20+976,7=20=40=
=40=20int=20exfat_find_dir_entry(struct=20super_block=20*sb,=0D=0A>=20>=20>=
>=20struct=20exfat_inode_info=20*ei,=20=20=7B=0D=0A>=20>=20>>=20=20=09int=
=20i,=20rewind=20=3D=200,=20dentry=20=3D=200,=20end_eidx=20=3D=200,=20num_e=
xt=20=3D=200,=20len;=0D=0A>=20>=20>>=20=20=09int=20order,=20step,=20name_le=
n=20=3D=200;=0D=0A>=20>=20>>=20-=09int=20dentries_per_clu,=20num_empty=20=
=3D=200;=0D=0A>=20>=20>>=20+=09int=20dentries_per_clu;=0D=0A>=20>=20>>=20=
=20=09unsigned=20int=20entry_type;=0D=0A>=20>=20>>=20=20=09unsigned=20short=
=20*uniname=20=3D=20NULL;=0D=0A>=20>=20>>=20=20=09struct=20exfat_chain=20cl=
u;=0D=0A>=20>=20>>=20=40=40=20-976,7=20+994,15=20=40=40=20int=20exfat_find_=
dir_entry(struct=20super_block=20*sb,=0D=0A>=20>=20>>=20struct=20exfat_inod=
e_info=20*ei,=0D=0A>=20>=20>>=20=20=09=09end_eidx=20=3D=20dentry;=0D=0A>=20=
>=20>>=20=20=09=7D=0D=0A>=20>=20>>=0D=0A>=20>=20>>=20-=09candi_empty.eidx=
=20=3D=20EXFAT_HINT_NONE;=0D=0A>=20>=20>>=20+=09if=20(ei->hint_femp.eidx=20=
=21=3D=20EXFAT_HINT_NONE=20&&=0D=0A>=20>=20>>=20+=09=20=20=20=20ei->hint_fe=
mp.count=20<=20num_entries)=0D=0A>=20>=20>>=20+=09=09ei->hint_femp.eidx=20=
=3D=20EXFAT_HINT_NONE;=0D=0A>=20>=20>>=20+=0D=0A>=20>=20>>=20+=09if=20(ei->=
hint_femp.eidx=20=3D=3D=20EXFAT_HINT_NONE)=0D=0A>=20>=20>>=20+=09=09ei->hin=
t_femp.count=20=3D=200;=0D=0A>=20>=20>>=20+=0D=0A>=20>=20>>=20+=09candi_emp=
ty=20=3D=20ei->hint_femp;=0D=0A>=20>=20>>=20+=0D=0A>=20>=20>=0D=0A>=20>=20>=
=20It=20would=20be=20nice=20to=20make=20the=20code=20block=20above=20a=20st=
atic=20inline=20function=0D=0A>=20>=20>=20as=20well.=0D=0A>=20>=20>=0D=0A>=
=20>=20>>=20=20rewind:=0D=0A>=20>=20>>=20=20=09order=20=3D=200;=0D=0A>=20>=
=20>>=20=20=09step=20=3D=20DIRENT_STEP_FILE;=0D=0A>=20>=20>=20=5Bsnip=5D=0D=
=0A>=20>=20>>=20--=0D=0A>=20>=20>>=202.25.1=0D=0A>=20>=20>=0D=0A>=20>=20>=
=0D=0A=0D=0A