Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1405749 > unrolled thread

[PATCH] staging: lustre: llite: drop acl from cache

Back to article view | Back to linux.kernel

Contents

  [PATCH] staging: lustre: llite: drop acl from cache James Simmons <jsimmons@infradead.org> - 2016-05-24 02:40 +0200
    Re: [PATCH] staging: lustre: llite: drop acl from cache Andreas Grünbacher <andreas.gruenbacher@gmail.com> - 2016-05-24 04:10 +0200
      Re: [PATCH] staging: lustre: llite: drop acl from cache James Simmons <jsimmons@infradead.org> - 2016-05-24 22:40 +0200
        Re: [PATCH] staging: lustre: llite: drop acl from cache Andreas Gruenbacher <agruenba@redhat.com> - 2016-05-25 00:10 +0200

#1405749 — [PATCH] staging: lustre: llite: drop acl from cache

Commit b8a7a3a6 change get_acl() for posix xattr to always cache
the ACL which increases the reference count. That reference count
can be reduced by have ll_get_acl() call forget_cached_acl() which
it wasn't. When an inode gets deleted by Lustre the POSIX ACL
reference count is tested to ensure its 1 and if not produces an error.
Since forget_cached_acl() was not called Lustre started to complain.
This patch changes ll_get_acl() to call forget_cached_acl().

Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 drivers/staging/lustre/lustre/llite/file.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
index f47f2ac..0191945 100644
--- a/drivers/staging/lustre/lustre/llite/file.c
+++ b/drivers/staging/lustre/lustre/llite/file.c
@@ -3124,6 +3124,7 @@ struct posix_acl *ll_get_acl(struct inode *inode, int type)
 	spin_lock(&lli->lli_lock);
 	/* VFS' acl_permission_check->check_acl will release the refcount */
 	acl = posix_acl_dup(lli->lli_posix_acl);
+	forget_cached_acl(inode, type);
 	spin_unlock(&lli->lli_lock);
 
 	return acl;
-- 
1.7.1

[toc] | [next] | [standalone]

#1405774

2016-05-24 2:35 GMT+02:00 James Simmons <jsimmons@infradead.org>:
> Commit b8a7a3a6 change get_acl() for posix xattr to always cache
> the ACL which increases the reference count. That reference count
> can be reduced by have ll_get_acl() call forget_cached_acl() which
> it wasn't. When an inode gets deleted by Lustre the POSIX ACL
> reference count is tested to ensure its 1 and if not produces an error.

Lustre shouldn't assume that the VFS immediately drops the reference
it is passed. Please remove that check as well.

> Since forget_cached_acl() was not called Lustre started to complain.
> This patch changes ll_get_acl() to call forget_cached_acl().
>
> Signed-off-by: James Simmons <jsimmons@infradead.org>

Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>

> ---
>  drivers/staging/lustre/lustre/llite/file.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
> index f47f2ac..0191945 100644
> --- a/drivers/staging/lustre/lustre/llite/file.c
> +++ b/drivers/staging/lustre/lustre/llite/file.c
> @@ -3124,6 +3124,7 @@ struct posix_acl *ll_get_acl(struct inode *inode, int type)
>         spin_lock(&lli->lli_lock);
>         /* VFS' acl_permission_check->check_acl will release the refcount */
>         acl = posix_acl_dup(lli->lli_posix_acl);
> +       forget_cached_acl(inode, type);
>         spin_unlock(&lli->lli_lock);
>
>         return acl;
> --
> 1.7.1
>

[toc] | [prev] | [next] | [standalone]

#1406442

> 2016-05-24 2:35 GMT+02:00 James Simmons <jsimmons@infradead.org>:
> > Commit b8a7a3a6 change get_acl() for posix xattr to always cache
> > the ACL which increases the reference count. That reference count
> > can be reduced by have ll_get_acl() call forget_cached_acl() which
> > it wasn't. When an inode gets deleted by Lustre the POSIX ACL
> > reference count is tested to ensure its 1 and if not produces an error.
> 
> Lustre shouldn't assume that the VFS immediately drops the reference
> it is passed. Please remove that check as well.

The piece of code in question from ll_delete_inode() is

#ifdef CONFIG_FS_POSIX_ACL
        else if (lli->lli_posix_acl) {
                LASSERT(atomic_read(&lli->lli_posix_acl->a_refcount) == 
1);
                LASSERT(!lli->lli_remote_perms);
                posix_acl_release(lli->lli_posix_acl);
                lli->lli_posix_acl = NULL;
        }
#endif
 
So we want to prevent a leak should I do a 

while (atomic_read(&lli->lli_posix_acl->a_refcount))
	posix_acl_release(lli->lli_posix_acl);
lli->lli_posix_acl = NULL;

Or does the VFS do this cleanup for us?

> > Since forget_cached_acl() was not called Lustre started to complain.
> > This patch changes ll_get_acl() to call forget_cached_acl().
> >
> > Signed-off-by: James Simmons <jsimmons@infradead.org>
> 
> Reviewed-by: Andreas Gruenbacher <agruenba@redhat.com>
> 
> > ---
> >  drivers/staging/lustre/lustre/llite/file.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
> > index f47f2ac..0191945 100644
> > --- a/drivers/staging/lustre/lustre/llite/file.c
> > +++ b/drivers/staging/lustre/lustre/llite/file.c
> > @@ -3124,6 +3124,7 @@ struct posix_acl *ll_get_acl(struct inode *inode, int type)
> >         spin_lock(&lli->lli_lock);
> >         /* VFS' acl_permission_check->check_acl will release the refcount */
> >         acl = posix_acl_dup(lli->lli_posix_acl);
> > +       forget_cached_acl(inode, type);
> >         spin_unlock(&lli->lli_lock);
> >
> >         return acl;
> > --
> > 1.7.1
> >
> 

[toc] | [prev] | [next] | [standalone]

#1406471

On Tue, May 24, 2016 at 10:38 PM, James Simmons <jsimmons@infradead.org> wrote:
>> 2016-05-24 2:35 GMT+02:00 James Simmons <jsimmons@infradead.org>:
>> > Commit b8a7a3a6 change get_acl() for posix xattr to always cache
>> > the ACL which increases the reference count. That reference count
>> > can be reduced by have ll_get_acl() call forget_cached_acl() which
>> > it wasn't. When an inode gets deleted by Lustre the POSIX ACL
>> > reference count is tested to ensure its 1 and if not produces an error.
>>
>> Lustre shouldn't assume that the VFS immediately drops the reference
>> it is passed. Please remove that check as well.
>
> The piece of code in question from ll_delete_inode() is
>
> #ifdef CONFIG_FS_POSIX_ACL
>         else if (lli->lli_posix_acl) {
>                 LASSERT(atomic_read(&lli->lli_posix_acl->a_refcount) ==
> 1);
>                 LASSERT(!lli->lli_remote_perms);
>                 posix_acl_release(lli->lli_posix_acl);
>                 lli->lli_posix_acl = NULL;
>         }
> #endif
>
> So we want to prevent a leak should I do a
>
> while (atomic_read(&lli->lli_posix_acl->a_refcount))
>         posix_acl_release(lli->lli_posix_acl);
> lli->lli_posix_acl = NULL;
>
> Or does the VFS do this cleanup for us?

This conversation is unreal. Just remove the misguided assert and
you're good. After that, please have someone explain basic reference
counting to you.

Thanks,
Andreas

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web