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@xxxxxxxx>
SUSE Labs, CR
