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