<div dir="ltr"><div dir="ltr">On Mon, Jun 10, 2019 at 7:49 PM NeilBrown <<a href="mailto:neilb@suse.com">neilb@suse.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Jun 06 2019, Shaun Tancheff wrote:<br>
<br>
> On Thu, Jun 6, 2019 at 7:57 PM NeilBrown <<a href="mailto:neilb@suse.com" target="_blank">neilb@suse.com</a>> wrote:<br>
>><br>
>> On Thu, Jun 06 2019, Shaun Tancheff wrote:<br>
>><br>
>> > Ensure all uses of cl_page->cp_state are smp-safe.<br>
>> ><br>
>> > LBUG Output:<br>
>> > In __cl_page_state_set()<br>
>> > ..<br>
>> > old = page->cp_state;<br>
>> > PASSERT(env, page, allowed_transitions[old][state]);<br>
>> > ..<br>
>> > Asserted with the following:<br>
>> > page@ffff80be1fcd6600[3 ffff80be1fca5cc0 0 1 ffff809e60e2 8cc0]<br>
<br>
I now see that this space..... ^^^<br>
shouldn't be there..<br>
.....<br>
<br>
> Also known caller was cl_page_assume+0xdc/0x3e0 [obdclass] (from the<br>
> backtrace)<br>
> So we know the on-stack / register value of state == CPS_OWNED<br>
><br>
> The backtrace:<br>
> ...<br>
> dump_backtrace+0x0/0x248<br>
> show_stack+0x24/0x30<br>
> dump_stack+0xbc/0xf4<br>
> libcfs_call_trace+0xec/0x120 [libcfs]<br>
> lbug_with_loc+0x4c/0xb8 [libcfs]<br>
> cl_page_state_set0+0x2b4/0x6e0 [obdclass]<br>
> cl_page_assume+0xdc/0x3e0 [obdclass]<br>
> ll_io_read_page+0x144c/0x1de0 [lustre]<br>
> ll_write_begin+0x3d4/0xee8 [lustre]<br>
<br>
Thanks for this extra detail.<br>
I agree that the evidence suggests that page->cp_state changed (to<br>
CPS_CACHED) during the processing of the PASSERT().<br>
<br>
It isn't clear to me though that this problem can be fixed just by<br>
adding barriers - particularly if this was on x86_64 as I suspect it<br>
is. Intel architecture processors have strongly ordered memory<br>
semantics. I'm not entirely sure what that means, but as<br>
smp_load_acquire() is very nearly a no-op on x86, I doubt that the<br>
memory barriers you added actually fix anything there.<br></blockquote><div><br></div><div>Agree. This occurred on aarch64. I suspect one of the reasons that this has not</div><div>been seen is that the majority of users are x86_64. It appears to be very rare on</div><div>aarch64 but it could also, theoretically, happen on the other relaxed architectures.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
My understanding (based on a fairly shallow reading of the code) is that<br>
a partial-page write is being attempted, and the target page was not in<br>
memory, so it was read in synchronously.<br>
The fragment of ll_io_read_page happening is:<br>
<br>
if (anchor != NULL && !cl_page_is_owned(page, io)) { /* have sent */<br>
rc = cl_sync_io_wait(env, anchor, 0);<br>
<br>
cl_page_assume(env, io, page);<br>
<br>
So cl_sync_io_wait() has just run. The page was (presumably) previously<br>
owned by something else. cl_sync_io_wait() waited for csi_sync_nr<br>
to drop to zero, which should say it isn't owned any more.<br>
cl_page_assume() then takes ownership.<br>
But it seems that the old ownership wasn't quite gone yet.<br>
<br>
The VM page remains locked the whole time, so the barriers implicit in<br>
locking and unlocking a page do not some into play here.<br>
There is an implicit barrier on the 'store' side when cl_sync_io_note()<br>
calls atomic_dec_and_test() to decrement csi_sync_nr, but I cannot see<br>
any implicit barrier on the 'load' side. If l_wait_event() in<br>
cl_sync_io_wait() took a spinlock, that would be enough. But it is<br>
possible for the wait to complete without any locking.<br>
<br>
So I can just about see the possibility of the race that you think you<br>
have hit. But it couldn't happen on x86, as it has strong memory<br>
ordering.<br>
<br>
What CPU architecture did this ASSERT trigger on?<br></blockquote><div><br></div><div>Agree with everything above, unfortunately was aarch64.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks,<br>
NeilBrown<br></blockquote><div> </div></div></div>