Received-SPF: pass (google.com: domain of linux-kernel+bounces-156624-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: David Laight <David.Laight@ACULAB.COM>
To: 'Stas Sergeev' <stsp2@yandex.ru>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
CC: Eric Biederman <ebiederm@xmission.com>, Alexander Viro
	<viro@zeniv.linux.org.uk>, Christian Brauner <brauner@kernel.org>, Jan Kara
	<jack@suse.cz>, Andy Lutomirski <luto@kernel.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: RE: [PATCH 1/2] fs: reorganize path_openat()
Thread-Topic: [PATCH 1/2] fs: reorganize path_openat()
Thread-Index: AQHalJJk8Yta93gecEuTWlpT1K4OxbF3JeNw
Date: Wed, 24 Apr 2024 09:17:57 +0000
Message-ID: <858f6fb6afcd450d85d1ff900f82d396@AcuMS.aculab.com>
References: <20240422084505.3465238-1-stsp2@yandex.ru>
In-Reply-To: <20240422084505.3465238-1-stsp2@yandex.ru>
Accept-Language: en-GB, en-US
Precedence: bulk
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

From: Stas Sergeev
> Sent: 22 April 2024 09:45

I seem to have 5 copies of this patch.....

> This patch moves the call to alloc_empty_file() below the call to
> path_init(). That changes is needed for the next patch, which adds
> a cred override for alloc_empty_file(). The needed cred info is only
> available after the call to path_init().
>=20
> No functional changes are intended by that patch.
..
> ---
>  fs/namei.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
>=20
> diff --git a/fs/namei.c b/fs/namei.c
> index c5b2a25be7d0..2fde2c320ae9 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3782,22 +3782,30 @@ static struct file *path_openat(struct nameidata =
*nd,
>  =09struct file *file;
>  =09int error;
>=20
> -=09file =3D alloc_empty_file(op->open_flag, current_cred());
> -=09if (IS_ERR(file))
> -=09=09return file;
> -
> -=09if (unlikely(file->f_flags & __O_TMPFILE)) {
> +=09if (unlikely(op->open_flag & __O_TMPFILE)) {
> +=09=09file =3D alloc_empty_file(op->open_flag, current_cred());
> +=09=09if (IS_ERR(file))
> +=09=09=09return file;
>  =09=09error =3D do_tmpfile(nd, flags, op, file);
> -=09} else if (unlikely(file->f_flags & O_PATH)) {
> +=09} else if (unlikely(op->open_flag & O_PATH)) {
> +=09=09file =3D alloc_empty_file(op->open_flag, current_cred());
> +=09=09if (IS_ERR(file))
> +=09=09=09return file;
>  =09=09error =3D do_o_path(nd, flags, file);

You probably ought to merge the two 'unlikely' tests.
Otherwise there'll be two conditionals in the 'hot path'.
(There probably always were.)
So something like:
=09if (unlikely(op->open_flag & (__O_TMPFILE | O_PATH))) {
=09=09file =3D alloc_empty_file(op->open_flag, current_cred());
=09=09if (IS_ERR(file))
=09=09=09return file;
=09=09if (op->open_flag & __O_TMFILE)
=09=09=09error =3D do_tmpfile(nd, flags, op, file);
=09=09else
=09=09=09error =3D do_o_path(nd, flags, file);
=09} else {
Copying op->open_flag to a local may also generate better code.

=09David
=09=09
>  =09} else {
>  =09=09const char *s =3D path_init(nd, flags);
> -=09=09while (!(error =3D link_path_walk(s, nd)) &&
> -=09=09       (s =3D open_last_lookups(nd, file, op)) !=3D NULL)
> -=09=09=09;
> +=09=09file =3D alloc_empty_file(op->open_flag, current_cred());
> +=09=09error =3D PTR_ERR_OR_ZERO(file);
> +=09=09if (!error) {
> +=09=09=09while (!(error =3D link_path_walk(s, nd)) &&
> +=09=09=09       (s =3D open_last_lookups(nd, file, op)) !=3D NULL)
> +=09=09=09=09;
> +=09=09}
>  =09=09if (!error)
>  =09=09=09error =3D do_open(nd, file, op);
>  =09=09terminate_walk(nd);
> +=09=09if (IS_ERR(file))
> +=09=09=09return file;
>  =09}
>  =09if (likely(!error)) {
>  =09=09if (likely(file->f_mode & FMODE_OPENED))
> --
> 2.44.0
>=20

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1=
PT, UK
Registration No: 1397386 (Wales)