Re: [RFC v2 0/4] overlayfs: Optimize override/revert creds

Re: [RFC v2 0/4] overlayfs: Optimize override/revert creds

[Amir Goldstein]

cc: fsdevel

On Fri, Jan 26, 2024 at 1:57 AM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Hi,
>

Hi Vinicius,

I have some specific comments about the overlayfs patch,
but first I prefer to provide higher level feedback on the series.

> It was noticed that some workloads suffer from contention on
> increasing/decrementing the ->usage counter in their credentials,
> those refcount operations are associated with overriding/reverting the
> current task credentials. (the linked thread adds more context)
>
> In some specialized cases, overlayfs is one of them, the credentials
> in question have a longer lifetime than the override/revert "critical
> section". In the overlayfs case, the credentials are created when the
> fs is mounted and destroyed when it's unmounted. In this case of long
> lived credentials, the usage counter doesn't need to be
> incremented/decremented.
>
> Add a lighter version of credentials override/revert to be used in
> these specialized cases. To make sure that the override/revert calls
> are paired, add a cleanup guard macro. This was suggested here:
>
> https://lore.kernel.org/all/20231219-marken-pochen-26d888fb9bb9@brauner/
>
> With a small number of tweaks:
>  - Used inline functions instead of macros;
>  - A small change to store the credentials into the passed argument,
>    the guard is now defined as (note the added '_T ='):
>
>       DEFINE_GUARD(cred, const struct cred *, _T = override_creds_light(_T),
>                   revert_creds_light(_T));
>
>  - Allow "const" arguments to be used with these kind of guards;
>
> Some comments:
>  - If patch 1/4 is not a good idea (adding the cast), the alternative
>    I can see is using some kind of container for the credentials;
>  - The only user for the backing file ops is overlayfs, so these
>    changes make sense, but may not make sense in the most general
>    case;
>
> For the numbers, some from 'perf c2c', before this series:
> (edited to fit)
>
> #
> #        ----- HITM -----                                        Shared
> #   Num  RmtHitm  LclHitm                      Symbol            Object         Source:Line  Node
> # .....  .......  .......  ..........................  ................  ..................  ....
> #
>   -------------------------
>       0      412     1028
>   -------------------------
>           41.50%   42.22%  [k] revert_creds            [kernel.vmlinux]  atomic64_64.h:39     0  1
>           15.05%   10.60%  [k] override_creds          [kernel.vmlinux]  atomic64_64.h:25     0  1
>            0.73%    0.58%  [k] init_file               [kernel.vmlinux]  atomic64_64.h:25     0  1
>            0.24%    0.10%  [k] revert_creds            [kernel.vmlinux]  cred.h:266           0  1
>           32.28%   37.16%  [k] generic_permission      [kernel.vmlinux]  mnt_idmapping.h:81   0  1
>            9.47%    8.75%  [k] generic_permission      [kernel.vmlinux]  mnt_idmapping.h:81   0  1
>            0.49%    0.58%  [k] inode_owner_or_capable  [kernel.vmlinux]  mnt_idmapping.h:81   0  1
>            0.24%    0.00%  [k] generic_permission      [kernel.vmlinux]  namei.c:354          0
>
>   -------------------------
>       1       50      103
>   -------------------------
>          100.00%  100.00%  [k] update_cfs_group  [kernel.vmlinux]  atomic64_64.h:15   0  1
>
>   -------------------------
>       2       50       98
>   -------------------------
>           96.00%   96.94%  [k] update_cfs_group  [kernel.vmlinux]  atomic64_64.h:15   0  1
>            2.00%    1.02%  [k] update_load_avg   [kernel.vmlinux]  atomic64_64.h:25   0  1
>            0.00%    2.04%  [k] update_load_avg   [kernel.vmlinux]  fair.c:4118        0
>            2.00%    0.00%  [k] update_cfs_group  [kernel.vmlinux]  fair.c:3932        0  1
>
> after this series:
>
> #
> #        ----- HITM -----                                   Shared
> #   Num  RmtHitm  LclHitm                 Symbol            Object       Source:Line  Node
> # .....  .......  .......   ....................  ................  ................  ....
> #
>   -------------------------
>       0       54       88
>   -------------------------
>          100.00%  100.00%   [k] update_cfs_group  [kernel.vmlinux]  atomic64_64.h:15   0  1
>
>   -------------------------
>       1       48       83
>   -------------------------
>           97.92%   97.59%   [k] update_cfs_group  [kernel.vmlinux]  atomic64_64.h:15   0  1
>            2.08%    1.20%   [k] update_load_avg   [kernel.vmlinux]  atomic64_64.h:25   0  1
>            0.00%    1.20%   [k] update_load_avg   [kernel.vmlinux]  fair.c:4118        0  1
>
>   -------------------------
>       2       28       44
>   -------------------------
>           85.71%   79.55%   [k] generic_permission      [kernel.vmlinux]  mnt_idmapping.h:81   0  1
>           14.29%   20.45%   [k] generic_permission      [kernel.vmlinux]  mnt_idmapping.h:81   0  1
>
>
> The contention is practically gone.

That is very impressive.
Can you say which workloads were running during this test?
Specifically, I am wondering how much of the improvement came from
backing_file.c and how much from overlayfs/*.c.

The reason I am asking is because the overlayfs patch is quite large and can
take more time to review, so I am wondering out loud if we are not
better off this
course of action:

1. convert backing_file.c to use new helpers/guards
2. convert overlayfs to use new helpers/guards

#1 should definitely go in via Christian's tree and should get a wider review
from fsdevel (please CC fsdevel next time)

#2 is contained for overlayfs reviewers. Once the helpers are merged
and used by backing_file helpers, overlayfs can be converted independently.

#1 and #2 could both be merged in the same merge cycle, or not, it does not
matter. Most likely, #2 will go through Christian's tree as well, but I think we
need to work according to this merge order.

We can also work on the review in parallel and you may keep the overlayfs
patch in following posts, just wanted us to be on the same page w.r.t to
the process.

Thanks,
Amir.

Re: [RFC v2 0/4] overlayfs: Optimize override/revert creds

[Vinicius Costa Gomes]

Hi Amir,

Amir Goldstein <amir73il@gmail.com> writes:

> cc: fsdevel
>
> On Fri, Jan 26, 2024 at 1:57 AM Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>>
>> Hi,
>>
>
> Hi Vinicius,
>
> I have some specific comments about the overlayfs patch,
> but first I prefer to provide higher level feedback on the series.
>
>> It was noticed that some workloads suffer from contention on
>> increasing/decrementing the ->usage counter in their credentials,
>> those refcount operations are associated with overriding/reverting the
>> current task credentials. (the linked thread adds more context)
>>
>> In some specialized cases, overlayfs is one of them, the credentials
>> in question have a longer lifetime than the override/revert "critical
>> section". In the overlayfs case, the credentials are created when the
>> fs is mounted and destroyed when it's unmounted. In this case of long
>> lived credentials, the usage counter doesn't need to be
>> incremented/decremented.
>>
>> Add a lighter version of credentials override/revert to be used in
>> these specialized cases. To make sure that the override/revert calls
>> are paired, add a cleanup guard macro. This was suggested here:
>>
>> https://lore.kernel.org/all/20231219-marken-pochen-26d888fb9bb9@brauner/
>>
>> With a small number of tweaks:
>>  - Used inline functions instead of macros;
>>  - A small change to store the credentials into the passed argument,
>>    the guard is now defined as (note the added '_T ='):
>>
>>       DEFINE_GUARD(cred, const struct cred *, _T = override_creds_light(_T),
>>                   revert_creds_light(_T));
>>
>>  - Allow "const" arguments to be used with these kind of guards;
>>
>> Some comments:
>>  - If patch 1/4 is not a good idea (adding the cast), the alternative
>>    I can see is using some kind of container for the credentials;
>>  - The only user for the backing file ops is overlayfs, so these
>>    changes make sense, but may not make sense in the most general
>>    case;
>>
>> For the numbers, some from 'perf c2c', before this series:
>> (edited to fit)
>>
>> #
>> #        ----- HITM -----                                        Shared
>> #   Num  RmtHitm  LclHitm                      Symbol            Object         Source:Line  Node
>> # .....  .......  .......  ..........................  ................  ..................  ....
>> #
>>   -------------------------
>>       0      412     1028
>>   -------------------------
>>           41.50%   42.22%  [k] revert_creds            [kernel.vmlinux]  atomic64_64.h:39     0  1
>>           15.05%   10.60%  [k] override_creds          [kernel.vmlinux]  atomic64_64.h:25     0  1
>>            0.73%    0.58%  [k] init_file               [kernel.vmlinux]  atomic64_64.h:25     0  1
>>            0.24%    0.10%  [k] revert_creds            [kernel.vmlinux]  cred.h:266           0  1
>>           32.28%   37.16%  [k] generic_permission      [kernel.vmlinux]  mnt_idmapping.h:81   0  1
>>            9.47%    8.75%  [k] generic_permission      [kernel.vmlinux]  mnt_idmapping.h:81   0  1
>>            0.49%    0.58%  [k] inode_owner_or_capable  [kernel.vmlinux]  mnt_idmapping.h:81   0  1
>>            0.24%    0.00%  [k] generic_permission      [kernel.vmlinux]  namei.c:354          0
>>
>>   -------------------------
>>       1       50      103
>>   -------------------------
>>          100.00%  100.00%  [k] update_cfs_group  [kernel.vmlinux]  atomic64_64.h:15   0  1
>>
>>   -------------------------
>>       2       50       98
>>   -------------------------
>>           96.00%   96.94%  [k] update_cfs_group  [kernel.vmlinux]  atomic64_64.h:15   0  1
>>            2.00%    1.02%  [k] update_load_avg   [kernel.vmlinux]  atomic64_64.h:25   0  1
>>            0.00%    2.04%  [k] update_load_avg   [kernel.vmlinux]  fair.c:4118        0
>>            2.00%    0.00%  [k] update_cfs_group  [kernel.vmlinux]  fair.c:3932        0  1
>>
>> after this series:
>>
>> #
>> #        ----- HITM -----                                   Shared
>> #   Num  RmtHitm  LclHitm                 Symbol            Object       Source:Line  Node
>> # .....  .......  .......   ....................  ................  ................  ....
>> #
>>   -------------------------
>>       0       54       88
>>   -------------------------
>>          100.00%  100.00%   [k] update_cfs_group  [kernel.vmlinux]  atomic64_64.h:15   0  1
>>
>>   -------------------------
>>       1       48       83
>>   -------------------------
>>           97.92%   97.59%   [k] update_cfs_group  [kernel.vmlinux]  atomic64_64.h:15   0  1
>>            2.08%    1.20%   [k] update_load_avg   [kernel.vmlinux]  atomic64_64.h:25   0  1
>>            0.00%    1.20%   [k] update_load_avg   [kernel.vmlinux]  fair.c:4118        0  1
>>
>>   -------------------------
>>       2       28       44
>>   -------------------------
>>           85.71%   79.55%   [k] generic_permission      [kernel.vmlinux]  mnt_idmapping.h:81   0  1
>>           14.29%   20.45%   [k] generic_permission      [kernel.vmlinux]  mnt_idmapping.h:81   0  1
>>
>>
>> The contention is practically gone.
>
> That is very impressive.
> Can you say which workloads were running during this test?
> Specifically, I am wondering how much of the improvement came from
> backing_file.c and how much from overlayfs/*.c.
>

I received the workload packaged from one of our customer teams, it's a
docker image to run a wordpress/php/nginx thing, totally not my area and
not sure that I can give much more details. The only think that I know
is that this workload does *a lot* of faccessat().

Anyway, I did a experiment removing the backing ops patch, got this
numbers (edited for clarity):

#
#        ----- HITM -----  ------- Store Refs ------                                        Shared                          
#   Num  RmtHitm  LclHitm   L1 Hit  L1 Miss      N/A                     Symbol             Object         Source:Line  Node
# .....  .......  .......  .......  .......  ....... ..........................  .................  ..................  ....
#
  ---------------------------------------------------
      0       79       97        0        0        0 
  ---------------------------------------------------
           0.00%    1.03%    0.00%    0.00%    0.00% [k] revert_creds            [kernel.kallsyms]  atomic64_64.h:39     0  1
           1.27%    0.00%    0.00%    0.00%    0.00% [k] init_file               [kernel.kallsyms]  atomic64_64.h:25     0  1
          62.03%   71.13%    0.00%    0.00%    0.00% [k] generic_permission      [kernel.kallsyms]  mnt_idmapping.h:81   0  1
          35.44%   26.80%    0.00%    0.00%    0.00% [k] generic_permission      [kernel.kallsyms]  mnt_idmapping.h:81   0  1
           1.27%    0.00%    0.00%    0.00%    0.00% [k] generic_permission      [kernel.kallsyms]  mnt_idmapping.h:81   0
           0.00%    1.03%    0.00%    0.00%    0.00% [k] generic_permission      [kernel.kallsyms]  namei.c:354          0  1

  ---------------------------------------------------
      1       52      103        0        0        0 
  ---------------------------------------------------
          98.08%   98.06%    0.00%    0.00%    0.00% [k] update_cfs_group  [kernel.kallsyms]  atomic64_64.h:15   0  1
           0.00%    1.94%    0.00%    0.00%    0.00% [k] update_load_avg   [kernel.kallsyms]  atomic64_64.h:25   0  1
           1.92%    0.00%    0.00%    0.00%    0.00% [k] update_cfs_group  [kernel.kallsyms]  fair.c:3932        0

  ---------------------------------------------------
      2       59       77        0        0        0 
  ---------------------------------------------------
          93.22%   98.70%    0.00%    0.00%    0.00% [k] update_cfs_group  [kernel.kallsyms]  atomic64_64.h:15   0  1
           5.08%    1.30%    0.00%    0.00%    0.00% [k] update_cfs_group  [kernel.kallsyms]  fair.c:3932        0  1
           1.69%    0.00%    0.00%    0.00%    0.00% [k] update_load_avg   [kernel.kallsyms]  atomic64_64.h:25   0  1


So, the main source of contention is not in the backing file ops (but
there is still some). That seems to align with the numbers that Chen Hu
provided[1], that most of the contention was in ovl_permission().

[1] https://lore.kernel.org/all/20231018074553.41333-1-hu1.chen@intel.com/

> The reason I am asking is because the overlayfs patch is quite large and can
> take more time to review, so I am wondering out loud if we are not
> better off this
> course of action:
>
> 1. convert backing_file.c to use new helpers/guards
> 2. convert overlayfs to use new helpers/guards
>

For this particular workload, (2) is more important. But I am open to
propose (1) first, no problem at all.

Also, if you think that some other way of spliting the series, for
example, one patch per function being converted, would be easier/better,
I can do that too.

> #1 should definitely go in via Christian's tree and should get a wider review
> from fsdevel (please CC fsdevel next time)
>

Of course. Will CC fsdevel.

> #2 is contained for overlayfs reviewers. Once the helpers are merged
> and used by backing_file helpers, overlayfs can be converted independently.
>
> #1 and #2 could both be merged in the same merge cycle, or not, it does not
> matter. Most likely, #2 will go through Christian's tree as well, but I think we
> need to work according to this merge order.
>
> We can also work on the review in parallel and you may keep the overlayfs
> patch in following posts, just wanted us to be on the same page w.r.t to
> the process.
>
> Thanks,
> Amir.


Cheers,
-- 
Vinicius

Re: [RFC v2 4/4] fs: Optimize credentials reference count for backing file ops

[Amir Goldstein]

On Fri, Jan 26, 2024 at 1:57 AM 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, struct iov_iter *iter,
>                                struct backing_file_ctx *ctx)
>  {
>         struct backing_aio *aio = NULL;
> -       const struct cred *old_cred;
> +       const struct cred *old_cred = 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, struct iov_iter *iter,
>             !(file->f_mode & FMODE_CAN_ODIRECT))
>                 return -EINVAL;
>
> -       old_cred = override_creds(ctx->cred);
> -       if (is_sync_kiocb(iocb)) {
> -               rwf_t rwf = 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 var
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 = iocb_to_rw_flags(flags);
>
> -               ret = vfs_iter_read(file, iter, &iocb->ki_pos, rwf);
> -       } else {
> -               ret = -ENOMEM;
> -               aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
> -               if (!aio)
> -                       goto out;
> +                       ret = vfs_iter_read(file, iter, &iocb->ki_pos, rwf);
> +               } else {
> +                       ret = -ENOMEM;
> +                       aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
> +                       if (!aio)
> +                               goto out;
>
> -               aio->orig_iocb = iocb;
> -               kiocb_clone(&aio->iocb, iocb, get_file(file));
> -               aio->iocb.ki_complete = backing_aio_rw_complete;
> -               refcount_set(&aio->ref, 2);
> -               ret = vfs_iocb_iter_read(file, &aio->iocb, iter);
> -               backing_aio_put(aio);
> -               if (ret != -EIOCBQUEUED)
> -                       backing_aio_cleanup(aio, ret);
> +                       aio->orig_iocb = iocb;
> +                       kiocb_clone(&aio->iocb, iocb, get_file(file));
> +                       aio->iocb.ki_complete = backing_aio_rw_complete;
> +                       refcount_set(&aio->ref, 2);
> +                       ret = vfs_iocb_iter_read(file, &aio->iocb, iter);
> +                       backing_aio_put(aio);
> +                       if (ret != -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.

Re: [RFC v2 4/4] fs: Optimize credentials reference count for backing file ops

[Vinicius Costa Gomes]

Amir Goldstein <amir73il@gmail.com> writes:

> On Fri, Jan 26, 2024 at 1:57 AM 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.
>

Sure. Will do.

>> 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, struct iov_iter *iter,
>>                                struct backing_file_ctx *ctx)
>>  {
>>         struct backing_aio *aio = NULL;
>> -       const struct cred *old_cred;
>> +       const struct cred *old_cred = 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, struct iov_iter *iter,
>>             !(file->f_mode & FMODE_CAN_ODIRECT))
>>                 return -EINVAL;
>>
>> -       old_cred = override_creds(ctx->cred);
>> -       if (is_sync_kiocb(iocb)) {
>> -               rwf_t rwf = 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 var
> 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.

Most likely using DEFINE_LOCK_GUARD_1() would allow us to use the nicer version.

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

Yeah, I think what happened is that I tried to keep the scope of the
guard to be as close as possible to override/revert (as you said), and
that caused the churn.

Probably using guard() more will reduce these confusing code changes. I
am going to try that.

> Thanks,
> Amir.


Cheers,
-- 
Vinicius