[v2] ext2: Silence lockdep warning about reclaim under xattr_sem

Lockdep complains about a chain:
  sb_internal#2 --> &ei->xattr_sem#2 --> fs_reclaim

and shrink_dentry_list -> ext2_evict_inode -> ext2_xattr_delete_inode ->
down_write(ei->xattr_sem) creating a locking cycle in the reclaim path.
This is however a false positive because when we are in
ext2_evict_inode() we are the only holder of the inode reference and
nobody else should touch xattr_sem of that inode. So we cannot ever
block on acquiring the xattr_sem in the reclaim path.

Silence the lockdep warning by using down_write_trylock() in
ext2_xattr_delete_inode() to not create false locking dependency.

Reported-by: "J. R. Okajima" <hooanon05g@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/xattr.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Changes since v1:
- changed WARN_ON to WARN_ON_ONCE

On 2/25/20 5:38 PM, Jan Kara wrote:
> Lockdep complains about a chain:
>    sb_internal#2 --> &ei->xattr_sem#2 --> fs_reclaim
> 
> and shrink_dentry_list -> ext2_evict_inode -> ext2_xattr_delete_inode ->
> down_write(ei->xattr_sem) creating a locking cycle in the reclaim path.
> This is however a false positive because when we are in
> ext2_evict_inode() we are the only holder of the inode reference and
> nobody else should touch xattr_sem of that inode. So we cannot ever
> block on acquiring the xattr_sem in the reclaim path.
> 
> Silence the lockdep warning by using down_write_trylock() in
> ext2_xattr_delete_inode() to not create false locking dependency.
> 
> Reported-by: "J. R. Okajima" <hooanon05g@gmail.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

Agreed with evict() will only be called when it's the last reference 
going down and so we won't be blocked on xattr_sem.
Thanks for clearly explaining the problem in the cover letter.

Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>


> ---
>   fs/ext2/xattr.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> Changes since v1:
> - changed WARN_ON to WARN_ON_ONCE
> 
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 0456bc990b5e..9ad07c7ef0b3 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -790,7 +790,15 @@ ext2_xattr_delete_inode(struct inode *inode)
>   	struct buffer_head *bh = NULL;
>   	struct ext2_sb_info *sbi = EXT2_SB(inode->i_sb);
> 
> -	down_write(&EXT2_I(inode)->xattr_sem);
> +	/*
> +	 * We are the only ones holding inode reference. The xattr_sem should
> +	 * better be unlocked! We could as well just not acquire xattr_sem at
> +	 * all but this makes the code more futureproof. OTOH we need trylock
> +	 * here to avoid false-positive warning from lockdep about reclaim
> +	 * circular dependency.
> +	 */
> +	if (WARN_ON_ONCE(!down_write_trylock(&EXT2_I(inode)->xattr_sem)))
> +		return;
>   	if (!EXT2_I(inode)->i_file_acl)
>   		goto cleanup;
>

On Wed 26-02-20 17:02:18, Ritesh Harjani wrote:
> 
> 
> On 2/25/20 5:38 PM, Jan Kara wrote:
> > Lockdep complains about a chain:
> >    sb_internal#2 --> &ei->xattr_sem#2 --> fs_reclaim
> > 
> > and shrink_dentry_list -> ext2_evict_inode -> ext2_xattr_delete_inode ->
> > down_write(ei->xattr_sem) creating a locking cycle in the reclaim path.
> > This is however a false positive because when we are in
> > ext2_evict_inode() we are the only holder of the inode reference and
> > nobody else should touch xattr_sem of that inode. So we cannot ever
> > block on acquiring the xattr_sem in the reclaim path.
> > 
> > Silence the lockdep warning by using down_write_trylock() in
> > ext2_xattr_delete_inode() to not create false locking dependency.
> > 
> > Reported-by: "J. R. Okajima" <hooanon05g@gmail.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> 
> Agreed with evict() will only be called when it's the last reference going
> down and so we won't be blocked on xattr_sem.
> Thanks for clearly explaining the problem in the cover letter.
> 
> Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>

Thanks for review! I've now pushed the patch to my tree.

								Honza
> 
> 
> > ---
> >   fs/ext2/xattr.c | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > Changes since v1:
> > - changed WARN_ON to WARN_ON_ONCE
> > 
> > diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> > index 0456bc990b5e..9ad07c7ef0b3 100644
> > --- a/fs/ext2/xattr.c
> > +++ b/fs/ext2/xattr.c
> > @@ -790,7 +790,15 @@ ext2_xattr_delete_inode(struct inode *inode)
> >   	struct buffer_head *bh = NULL;
> >   	struct ext2_sb_info *sbi = EXT2_SB(inode->i_sb);
> > 
> > -	down_write(&EXT2_I(inode)->xattr_sem);
> > +	/*
> > +	 * We are the only ones holding inode reference. The xattr_sem should
> > +	 * better be unlocked! We could as well just not acquire xattr_sem at
> > +	 * all but this makes the code more futureproof. OTOH we need trylock
> > +	 * here to avoid false-positive warning from lockdep about reclaim
> > +	 * circular dependency.
> > +	 */
> > +	if (WARN_ON_ONCE(!down_write_trylock(&EXT2_I(inode)->xattr_sem)))
> > +		return;
> >   	if (!EXT2_I(inode)->i_file_acl)
> >   		goto cleanup;
> > 
>

Jan Kara:
> Lockdep complains about a chain:
>   sb_internal#2 --> &ei->xattr_sem#2 --> fs_reclaim
>
> and shrink_dentry_list -> ext2_evict_inode -> ext2_xattr_delete_inode ->
> down_write(ei->xattr_sem) creating a locking cycle in the reclaim path.
> This is however a false positive because when we are in
> ext2_evict_inode() we are the only holder of the inode reference and
> nobody else should touch xattr_sem of that inode. So we cannot ever
> block on acquiring the xattr_sem in the reclaim path.
>
> Silence the lockdep warning by using down_write_trylock() in
> ext2_xattr_delete_inode() to not create false locking dependency.

v5.6 is released.
But I cannot see this patch applied.  Sad.

Anyway I am wondering whether acquiring xattr_sem in
ext2_xattr_delete_inode() is really necessary or not.
It is necessary because this function refers and clears i_file_acl,
right?

But this function handles the removed (nlink==0) and unused inodes only.
If nobody else touches xattr_sem as you wrote, then it is same to
i_file_acl, isn't it?  Can we replace xattr_sem (only here) by memory
barrier, or remove xattr_sem from ext2_xattr_delete_inode()?


J. R. Okajima

On Mon 06-04-20 14:36:17, J. R. Okajima wrote:
> Jan Kara:
> > Lockdep complains about a chain:
> >   sb_internal#2 --> &ei->xattr_sem#2 --> fs_reclaim
> >
> > and shrink_dentry_list -> ext2_evict_inode -> ext2_xattr_delete_inode ->
> > down_write(ei->xattr_sem) creating a locking cycle in the reclaim path.
> > This is however a false positive because when we are in
> > ext2_evict_inode() we are the only holder of the inode reference and
> > nobody else should touch xattr_sem of that inode. So we cannot ever
> > block on acquiring the xattr_sem in the reclaim path.
> >
> > Silence the lockdep warning by using down_write_trylock() in
> > ext2_xattr_delete_inode() to not create false locking dependency.
> 
> v5.6 is released.
> But I cannot see this patch applied.  Sad.

It will go in for this merge window. Since this was just a problem with
lockdep reporting, there's no hurry in pushing it...

> Anyway I am wondering whether acquiring xattr_sem in
> ext2_xattr_delete_inode() is really necessary or not.
> It is necessary because this function refers and clears i_file_acl,
> right?
>
> But this function handles the removed (nlink==0) and unused inodes only.
> If nobody else touches xattr_sem as you wrote, then it is same to
> i_file_acl, isn't it?  Can we replace xattr_sem (only here) by memory
> barrier, or remove xattr_sem from ext2_xattr_delete_inode()?

It is not really necessary because the inode is completely private to the
process evicting it at that point. So any inode-local locking is not going
to serialize anything. But from a maintenance point of view it is better to
acquire the lock so that possible assertions that lock is held in some
helper functions don't barf or for the case the function gets used in a
different code path in the future.

								Honza

Jan Kara:
> to serialize anything. But from a maintenance point of view it is better to
> acquire the lock so that possible assertions that lock is held in some
> helper functions don't barf or for the case the function gets used in a
> different code path in the future.

Ok, thanx.
"a maintenance point of view" is a good keyword for me.
I will post "the fix passed my local stress test" tomorrow.


J. R. Okajima

> I will post "the fix passed my local stress test" tomorrow.

I did my long stress test many times.
And the fix passed my local stress test.

Thank you
J. R. Okajima

On Tue 07-04-20 14:22:16, J. R. Okajima wrote:
> > I will post "the fix passed my local stress test" tomorrow.
> 
> I did my long stress test many times.
> And the fix passed my local stress test.

Thanks for testing! BTW, the fix went to Linus yesterday.

								Honza