Re: ext2, possible circular locking dependency detected

[Jan Kara <jack@suse.cz>]

Hello!

On Sat 22-02-20 11:53:16, J. R. Okajima wrote:
> Hello ext2 maintainers,
> 
> During my local fs stress test, I've encounter this.
> Is it false positive?
> Otherwise, I've made a small patch to stop reclaming recursively into FS
> from ext2_xattr_set().  Please consider taking this.
> 
> Once I've considered about whether it should be done in VFS layer or
> not.  I mean, every i_op->brabra() calls in VFS should be surrounded by
> memalloc_nofs_{save,restore}(), by a macro or something.  But I am
> afraid it may introduce unnecesary overheads, especially when FS code
> doesn't allocate memory.  So it is better to do it in real FS
> operations.

Thanks for debugging this and for the patch. One comment below:

...

> @@ -532,7 +534,9 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
>  
>  			unlock_buffer(bh);
>  			ea_bdebug(bh, "cloning");
> +			nofs_flag = memalloc_nofs_save();
>  			header = kmemdup(HDR(bh), bh->b_size, GFP_KERNEL);
> +			memalloc_nofs_restore(nofs_flag);
>  			error = -ENOMEM;
>  			if (header == NULL)
>  				goto cleanup;
> @@ -545,7 +549,9 @@ ext2_xattr_set(struct inode *inode, int name_index, const char *name,
>  		}
>  	} else {
>  		/* Allocate a buffer where we construct the new block. */
> +		nofs_flag = memalloc_nofs_save();
>  		header = kzalloc(sb->s_blocksize, GFP_KERNEL);
> +		memalloc_nofs_restore(nofs_flag);
>  		error = -ENOMEM;
>  		if (header == NULL)
>  			goto cleanup;

This is not the right way how memalloc_nofs_save() should be used (you
could just use GFP_NOFS instead of GFP_KERNEL instead of wrapping the
allocation inside memalloc_nofs_save/restore()). The
memalloc_nofs_save/restore() API is created so that you can change the
allocation context at the place which mandates the new context - i.e., in
this case when acquiring / dropping xattr_sem. That way you don't have to
propagate the context information down to function calls and the code is
also future-proof - if you add new allocation, they will use correct
allocation context.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR