Received-SPF: pass (google.com: domain of linux-kernel+bounces-40225-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240125235723.39507-1-vinicius.gomes@intel.com> <20240125235723.39507-5-vinicius.gomes@intel.com>
In-Reply-To: <20240125235723.39507-5-vinicius.gomes@intel.com>
From: Amir Goldstein <amir73il@gmail.com>
Date: Fri, 26 Jan 2024 16:50:44 +0200
Message-ID: <CAOQ4uxi7MtVZECGXo-30YWjSU5ZFZP0AQzgBXLyowdOmNUc5DA@mail.gmail.com>
Subject: Re: [RFC v2 4/4] fs: Optimize credentials reference count for backing
 file ops
To: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Cc: brauner@kernel.org, hu1.chen@intel.com, miklos@szeredi.hu, 
	malini.bhandaru@intel.com, tim.c.chen@intel.com, mikko.ylinen@intel.com, 
	lizhen.you@intel.com, linux-unionfs@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-fsdevel <linux-fsdevel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jan 26, 2024 at 1:57=E2=80=AFAM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> For backing file operations, users are expected to pass credentials
> that will outlive the backing file common operations.
>
> Use the specialized guard statements to override/revert the
> credentials.
>

As I wrote before, I prefer to see this patch gets reviewed and merged
before the overlayfs large patch, so please reorder the series.

> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>  fs/backing-file.c | 124 ++++++++++++++++++++++------------------------
>  1 file changed, 60 insertions(+), 64 deletions(-)
>
> diff --git a/fs/backing-file.c b/fs/backing-file.c
> index a681f38d84d8..9874f09f860f 100644
> --- a/fs/backing-file.c
> +++ b/fs/backing-file.c
> @@ -140,7 +140,7 @@ ssize_t backing_file_read_iter(struct file *file, str=
uct iov_iter *iter,
>                                struct backing_file_ctx *ctx)
>  {
>         struct backing_aio *aio =3D NULL;
> -       const struct cred *old_cred;
> +       const struct cred *old_cred =3D ctx->cred;
>         ssize_t ret;
>
>         if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)))
> @@ -153,29 +153,28 @@ ssize_t backing_file_read_iter(struct file *file, s=
truct iov_iter *iter,
>             !(file->f_mode & FMODE_CAN_ODIRECT))
>                 return -EINVAL;
>
> -       old_cred =3D override_creds(ctx->cred);
> -       if (is_sync_kiocb(iocb)) {
> -               rwf_t rwf =3D iocb_to_rw_flags(flags);
> +       scoped_guard(cred, old_cred) {

This reads very strage.

Also, I see that e.g. scoped_guard(spinlock_irqsave, ... hides the local va=
r
used for save/restore of flags inside the macro.

Perhaps you use the same technique for scoped_guard(cred, ..
loose the local old_cred variable in all those functions and then the
code will read:

scoped_guard(cred, ctx->cred) {

which is nicer IMO.

> +               if (is_sync_kiocb(iocb)) {
> +                       rwf_t rwf =3D iocb_to_rw_flags(flags);
>
> -               ret =3D vfs_iter_read(file, iter, &iocb->ki_pos, rwf);
> -       } else {
> -               ret =3D -ENOMEM;
> -               aio =3D kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL)=
;
> -               if (!aio)
> -                       goto out;
> +                       ret =3D vfs_iter_read(file, iter, &iocb->ki_pos, =
rwf);
> +               } else {
> +                       ret =3D -ENOMEM;
> +                       aio =3D kmem_cache_zalloc(backing_aio_cachep, GFP=
_KERNEL);
> +                       if (!aio)
> +                               goto out;
>
> -               aio->orig_iocb =3D iocb;
> -               kiocb_clone(&aio->iocb, iocb, get_file(file));
> -               aio->iocb.ki_complete =3D backing_aio_rw_complete;
> -               refcount_set(&aio->ref, 2);
> -               ret =3D vfs_iocb_iter_read(file, &aio->iocb, iter);
> -               backing_aio_put(aio);
> -               if (ret !=3D -EIOCBQUEUED)
> -                       backing_aio_cleanup(aio, ret);
> +                       aio->orig_iocb =3D iocb;
> +                       kiocb_clone(&aio->iocb, iocb, get_file(file));
> +                       aio->iocb.ki_complete =3D backing_aio_rw_complete=
;
> +                       refcount_set(&aio->ref, 2);
> +                       ret =3D vfs_iocb_iter_read(file, &aio->iocb, iter=
);
> +                       backing_aio_put(aio);
> +                       if (ret !=3D -EIOCBQUEUED)
> +                               backing_aio_cleanup(aio, ret);
> +               }

if possible, I would rather avoid all this churn in functions that mostly
do work with the new cred, so either use guard(cred, ) directly or split a
helper that uses guard(cred, ) form the rest.

Thanks,
Amir.