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

[Andreas Gruenbacher]

On Tue, May 24, 2016 at 10:38 PM, James Simmons <jsimmons@xxxxxxxxxxxxx> wrote:
>> 2016-05-24 2:35 GMT+02:00 James Simmons <jsimmons@xxxxxxxxxxxxx>:
>> > 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