[PATCHv2 1/1] ext4: mballoc: Use this_cpu_read instead of this_cpu_ptr

[PATCHv2 1/1] ext4: mballoc: Use this_cpu_read instead of this_cpu_ptr

[Ritesh Harjani]

Simplify reading a seq variable by directly using this_cpu_read API
instead of doing this_cpu_ptr and then dereferencing it.

This also avoid the below kernel BUG: which happens when
CONFIG_DEBUG_PREEMPT is enabled

BUG: using smp_processor_id() in preemptible [00000000] code: syz-fuzzer/6927
caller is ext4_mb_new_blocks+0xa4d/0x3b70 fs/ext4/mballoc.c:4711
CPU: 1 PID: 6927 Comm: syz-fuzzer Not tainted 5.7.0-next-20200602-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x18f/0x20d lib/dump_stack.c:118
 check_preemption_disabled+0x20d/0x220 lib/smp_processor_id.c:48
 ext4_mb_new_blocks+0xa4d/0x3b70 fs/ext4/mballoc.c:4711
 ext4_ext_map_blocks+0x201b/0x33e0 fs/ext4/extents.c:4244
 ext4_map_blocks+0x4cb/0x1640 fs/ext4/inode.c:626
 ext4_getblk+0xad/0x520 fs/ext4/inode.c:833
 ext4_bread+0x7c/0x380 fs/ext4/inode.c:883
 ext4_append+0x153/0x360 fs/ext4/namei.c:67
 ext4_init_new_dir fs/ext4/namei.c:2757 [inline]
 ext4_mkdir+0x5e0/0xdf0 fs/ext4/namei.c:2802
 vfs_mkdir+0x419/0x690 fs/namei.c:3632
 do_mkdirat+0x21e/0x280 fs/namei.c:3655
 do_syscall_64+0x60/0xe0 arch/x86/entry/common.c:359
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

Fixes: 42f56b7a4a7d ("ext4: mballoc: introduce pcpu seqcnt for freeing PA
to improve ENOSPC handling")
Suggested-by: Borislav Petkov <bp@alien8.de>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
Reported-by: syzbot+82f324bb69744c5f6969@syzkaller.appspotmail.com
---
 fs/ext4/mballoc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index a9083113a8c0..c0a331e2feb0 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -4708,7 +4708,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle,
 	}
 
 	ac->ac_op = EXT4_MB_HISTORY_PREALLOC;
-	seq = *this_cpu_ptr(&discard_pa_seq);
+	seq = this_cpu_read(discard_pa_seq);
 	if (!ext4_mb_use_preallocated(ac)) {
 		ac->ac_op = EXT4_MB_HISTORY_ALLOC;
 		ext4_mb_normalize_request(ac, ar);
-- 
2.25.4

Re: [PATCHv2 1/1] ext4: mballoc: Use this_cpu_read instead of this_cpu_ptr

[Christoph Hellwig]

On Tue, Jun 09, 2020 at 04:23:10PM +0530, Ritesh Harjani wrote:
> Simplify reading a seq variable by directly using this_cpu_read API
> instead of doing this_cpu_ptr and then dereferencing it.
> 
> This also avoid the below kernel BUG: which happens when
> CONFIG_DEBUG_PREEMPT is enabled

I see this warning all the time with ext4 using tests VMs, so lets get
this fixed ASAP before -rc1:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv2 1/1] ext4: mballoc: Use this_cpu_read instead of this_cpu_ptr

[Ritesh Harjani]

On 6/10/20 11:55 AM, Christoph Hellwig wrote:
> On Tue, Jun 09, 2020 at 04:23:10PM +0530, Ritesh Harjani wrote:
>> Simplify reading a seq variable by directly using this_cpu_read API
>> instead of doing this_cpu_ptr and then dereferencing it.
>>
>> This also avoid the below kernel BUG: which happens when
>> CONFIG_DEBUG_PREEMPT is enabled
> 
> I see this warning all the time with ext4 using tests VMs, so lets get
> this fixed ASAP before -rc1:

Couldn't agree more.

> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 

Thanks

-ritesh

Re: [PATCHv2 1/1] ext4: mballoc: Use this_cpu_read instead of this_cpu_ptr

[Theodore Y. Ts'o]

On Tue, Jun 09, 2020 at 11:25:38PM -0700, Christoph Hellwig wrote:
> On Tue, Jun 09, 2020 at 04:23:10PM +0530, Ritesh Harjani wrote:
> > Simplify reading a seq variable by directly using this_cpu_read API
> > instead of doing this_cpu_ptr and then dereferencing it.
> > 
> > This also avoid the below kernel BUG: which happens when
> > CONFIG_DEBUG_PREEMPT is enabled
> 
> I see this warning all the time with ext4 using tests VMs, so lets get
> this fixed ASAP before -rc1:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks, applied.

						- Ted

Re: [PATCHv2 1/1] ext4: mballoc: Use this_cpu_read instead of this_cpu_ptr

[Ritesh Harjani]

On 6/9/20 6:07 PM, Hillf Danton wrote:
> 
> On Tue, 9 Jun 2020 18:53:23 +0800 Ritesh Harjani wrote:
>>
>> Simplify reading a seq variable by directly using this_cpu_read API
>> instead of doing this_cpu_ptr and then dereferencing it.
> 
> Two of the quick questions
> 1) Why can blocks discarded in a ext4 FS help allocators in another?

I am not sure if I understand your Q correctly. But here is a brief 
about the patchset. If there were PA blocks just or about to be 
discarded by another thread, then the current thread who is doing block 
allocation should not fail with ENOSPC error instead should be able to 
allocate those blocks from another thread. The concept is better 
explained in the commit msgs, if more details are required.
Without this patchset (in some heavy multi-threaded use case) allocation 
was failing when the overall filesystem space available was more then 50%.

> 
> 2) Why is a percpu seqcount prefered over what <linux/seqlock.h>
> can offer?
> 

Since this could be a multi-threaded use case, per cpu variable helps in 
avoid cache line bouncing problem, which could happen when the same 
variable is updated by multiple threads on different cpus.

-ritesh