[RFC,v2,4/4] fs: Optimize credentials reference count for backing file ops
Commit Message
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.
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
fs/backing-file.c | 124 ++++++++++++++++++++++------------------------
1 file changed, 60 insertions(+), 64 deletions(-)
Comments
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.
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,
@@ -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) {
+ 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);
+ }
}
out:
- revert_creds(old_cred);
-
if (ctx->accessed)
ctx->accessed(ctx->user_file);
@@ -187,7 +186,7 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
struct kiocb *iocb, int flags,
struct backing_file_ctx *ctx)
{
- const struct cred *old_cred;
+ const struct cred *old_cred = ctx->cred;
ssize_t ret;
if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)))
@@ -210,39 +209,37 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
*/
flags &= ~IOCB_DIO_CALLER_COMP;
- old_cred = override_creds(ctx->cred);
- if (is_sync_kiocb(iocb)) {
- rwf_t rwf = iocb_to_rw_flags(flags);
+ scoped_guard(cred, old_cred) {
+ if (is_sync_kiocb(iocb)) {
+ rwf_t rwf = iocb_to_rw_flags(flags);
- ret = vfs_iter_write(file, iter, &iocb->ki_pos, rwf);
- if (ctx->end_write)
- ctx->end_write(ctx->user_file);
- } else {
- struct backing_aio *aio;
+ ret = vfs_iter_write(file, iter, &iocb->ki_pos, rwf);
+ if (ctx->end_write)
+ ctx->end_write(ctx->user_file);
+ } else {
+ struct backing_aio *aio;
- ret = backing_aio_init_wq(iocb);
- if (ret)
- goto out;
+ ret = backing_aio_init_wq(iocb);
+ if (ret)
+ return ret;
- ret = -ENOMEM;
- aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
- if (!aio)
- goto out;
+ ret = -ENOMEM;
+ aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
+ if (!aio)
+ return ret;
- aio->orig_iocb = iocb;
- aio->end_write = ctx->end_write;
- kiocb_clone(&aio->iocb, iocb, get_file(file));
- aio->iocb.ki_flags = flags;
- aio->iocb.ki_complete = backing_aio_queue_completion;
- refcount_set(&aio->ref, 2);
- ret = vfs_iocb_iter_write(file, &aio->iocb, iter);
- backing_aio_put(aio);
- if (ret != -EIOCBQUEUED)
- backing_aio_cleanup(aio, ret);
+ aio->orig_iocb = iocb;
+ aio->end_write = ctx->end_write;
+ kiocb_clone(&aio->iocb, iocb, get_file(file));
+ aio->iocb.ki_flags = flags;
+ aio->iocb.ki_complete = backing_aio_queue_completion;
+ refcount_set(&aio->ref, 2);
+ ret = vfs_iocb_iter_write(file, &aio->iocb, iter);
+ backing_aio_put(aio);
+ if (ret != -EIOCBQUEUED)
+ backing_aio_cleanup(aio, ret);
+ }
}
-out:
- revert_creds(old_cred);
-
return ret;
}
EXPORT_SYMBOL_GPL(backing_file_write_iter);
@@ -252,15 +249,15 @@ ssize_t backing_file_splice_read(struct file *in, loff_t *ppos,
unsigned int flags,
struct backing_file_ctx *ctx)
{
- const struct cred *old_cred;
+ const struct cred *old_cred = ctx->cred;
ssize_t ret;
if (WARN_ON_ONCE(!(in->f_mode & FMODE_BACKING)))
return -EIO;
- old_cred = override_creds(ctx->cred);
- ret = vfs_splice_read(in, ppos, pipe, len, flags);
- revert_creds(old_cred);
+ scoped_guard(cred, old_cred) {
+ ret = vfs_splice_read(in, ppos, pipe, len, flags);
+ }
if (ctx->accessed)
ctx->accessed(ctx->user_file);
@@ -274,7 +271,7 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
unsigned int flags,
struct backing_file_ctx *ctx)
{
- const struct cred *old_cred;
+ const struct cred *old_cred = ctx->cred;
ssize_t ret;
if (WARN_ON_ONCE(!(out->f_mode & FMODE_BACKING)))
@@ -284,12 +281,11 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
if (ret)
return ret;
- old_cred = override_creds(ctx->cred);
- file_start_write(out);
- ret = iter_file_splice_write(pipe, out, ppos, len, flags);
- file_end_write(out);
- revert_creds(old_cred);
-
+ scoped_guard(cred, old_cred) {
+ file_start_write(out);
+ ret = iter_file_splice_write(pipe, out, ppos, len, flags);
+ file_end_write(out);
+ }
if (ctx->end_write)
ctx->end_write(ctx->user_file);
@@ -300,7 +296,7 @@ EXPORT_SYMBOL_GPL(backing_file_splice_write);
int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
struct backing_file_ctx *ctx)
{
- const struct cred *old_cred;
+ const struct cred *old_cred = ctx->cred;
int ret;
if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) ||
@@ -312,9 +308,9 @@ int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
vma_set_file(vma, file);
- old_cred = override_creds(ctx->cred);
- ret = call_mmap(vma->vm_file, vma);
- revert_creds(old_cred);
+ scoped_guard(cred, old_cred) {
+ ret = call_mmap(vma->vm_file, vma);
+ }
if (ctx->accessed)
ctx->accessed(ctx->user_file);