From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Linus Torvalds <torvalds@linux-foundation.org>,
        Al Viro <viro@zeniv.linux.org.uk>
Cc:     linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [git pull] common helper for kmap_local_page() users in local filesystems
Date:   Sat, 11 Mar 2023 18:11:01 +0100
Message-ID: <8232398.NyiUUSuA9g@suse>
In-Reply-To: <20230310204431.GW3390869@ZenIV>
References: <20230310204431.GW3390869@ZenIV>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Precedence: bulk

On venerd=EC 10 marzo 2023 21:44:31 CET Al Viro wrote:
> 	kmap_local_page() conversions in local filesystems keep running into
> kunmap_local_page()+put_page() combinations; we can keep inventing names
> for identical inline helpers, but it's getting rather inconvenient.  I've
> added a trivial helper to linux/highmem.h instead.

Yeah, "put_and_unmap_page()". Nice helper :-)

Today I decided to prepare a series to convert all the functions of all the=
=20
filesystems where I had found the above-mentioned pattern but I stopped=20
immediately after converting dir_put_page() in fs/sysv.

Why? Just because I realized that I do not understand the reasons behind th=
e=20
choice of the name of the helper... =20

Why did you name it "put_and_unmap_page()" instead of "unmap_and_put_page()=
",=20
for we always unmap first _and_ put the page immediately the unmapping?

It seems it want to imply that instead we put first and unmap later (which=
=20
would be wrong). That name sounds misleading to me and not sound (logically=
=20
speaking).

Am I missing some obscure convention behind your choice of that name for th=
e=20
helper?

If not, can you please change it from "put_and_unmap_page()" to=20
"unmap_and_put_page()"?=20

Thanks,

=46abio

>=20
> 	I would've held that back until the merge window, if not for the
> mess it causes in tree topology - I've several branches merging from that
> one, and it's only going to get worse if e.g. ext2 stuff gets picked by
> Jan.
>=20
> 	The helper is trivial and it's early in the cycle.  It would=20
simplify
> the things if you could pull it - then I'd simply rebase the affected=20
branches
> to -rc2...
>=20
> The following changes since commit fe15c26ee26efa11741a7b632e9f23b01aca4c=
c6:
>=20
>   Linux 6.3-rc1 (2023-03-05 14:52:03 -0800)
>=20
> are available in the Git repository at:
>=20
>   git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git tags/pull-
highmem
>=20
> for you to fetch changes up to 849ad04cf562ac63b0371a825eed473d84de9c6d:
>=20
>   new helper: put_and_unmap_page() (2023-03-07 01:50:53 -0500)
>=20
> ----------------------------------------------------------------
> put_and_unmap_page() helper
>=20
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>=20
> ----------------------------------------------------------------
> Al Viro (1):
>       new helper: put_and_unmap_page()
>=20
>  include/linux/highmem.h | 6 ++++++
>  1 file changed, 6 insertions(+)