<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
</head>
<body>
As an interested observer (thanks to both of you for an interesting exchange), Shaun, whats your plan going forward for the OpenSFS/WC branch? It sounds like youre thinking youll try to emulate what Neil did upstream? (which sounds good to me, I always
prefer avoiding explicit memory barriers if reasonable) Will you be opening an LU for this one?<br>
<br>
Thanks,<br>
- Patrick
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> lustre-devel <lustre-devel-bounces@lists.lustre.org> on behalf of Shaun Tancheff <shaun@tancheff.com><br>
<b>Sent:</b> Tuesday, June 11, 2019 11:36:13 PM<br>
<b>To:</b> NeilBrown<br>
<b>Cc:</b> Bobi Jam; lustre-devel@lists.lustre.org<br>
<b>Subject:</b> Re: [lustre-devel] [PATCH] Place a memory barrier around cp_state changes</font>
<div> </div>
</div>
<div>
<div dir="ltr">
<div dir="ltr">On Tue, Jun 11, 2019 at 11:09 PM NeilBrown <<a href="mailto:neilb@suse.com">neilb@suse.com</a>> wrote:<br>
</div>
<div class="x_gmail_quote">
<blockquote class="x_gmail_quote" style="margin:0px 0px 0px 0.8ex; border-left:1px solid rgb(204,204,204); padding-left:1ex">
On Mon, Jun 10 2019, Shaun Tancheff wrote:<br>
<br>
> On Mon, Jun 10, 2019 at 7:49 PM NeilBrown <<a href="mailto:neilb@suse.com" target="_blank">neilb@suse.com</a>> wrote:<br>
><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>
>><br>
><br>
> Agree with everything above, unfortunately was aarch64.<br>
<br>
Ahh.<br>
OK, I'm now reasonably convinced that the patch probably fixes the bug.<br>
It is a shame that there is no standard barrier between the wake_up and<br>
the wait_event(), but there isn't and we need to work with that.<br>
<br>
But I still don't like the patch.... :-(<br>
<br>
I have a few problems with it.<br>
<br>
Firstly, I generally don't like accessor functions. They tend to hide<br>
useful information.<br>
Rather than cl_page_state_get() I would rather have<br>
smp_load_acquire(&pg->cp_state)<br>
<br>
where ever it is needed, with a comment explaining why it is needed in<br>
that particular context.<br>
Some people seem to like accessor functions - there certainly are a lot<br>
of them in the kernel - but I don't think they help.<br>
Having an accessor function that just adds a barrier is, I think,<br>
particularly ugly as the comment can only be general in nature, and so<br>
doesn't really help the reader to understand why the barrier is needed.<br>
But even that isn't completely without precedent.<br>
"key_read_state()" in include/linux/key.h simply wraps<br>
smp_load_acquire(), and the comment there doesn't really tell me<br>
anything useful.<br>
"inet_sk_state_load" is slightly better it says it is for use in places<br>
where the socket lock isn't held - and names some of those.<br>
</blockquote>
<div><br>
</div>
<div>Understood and a fair assessment.</div>
<div> </div>
<blockquote class="x_gmail_quote" style="margin:0px 0px 0px 0.8ex; border-left:1px solid rgb(204,204,204); padding-left:1ex">
My second problem is that has "get" in the accessor function name is<br>
potentially misleading and "get" normally has a matching "put", and<br>
usually increments a refcount. Having "read" or "load" in place of<br>
"get" in the accessor function name would remove my second objection.<br>
</blockquote>
<div><br>
</div>
<div>Yeah, I was trying to balance with the set .. but really I'm not enamored</div>
<div>with that either :P</div>
<div> </div>
<blockquote class="x_gmail_quote" style="margin:0px 0px 0px 0.8ex; border-left:1px solid rgb(204,204,204); padding-left:1ex">
My third objection is that the bug doesn't exist in the upstream client<br>
code. I'm not sure whether this is luck or good management, but either<br>
way it is gone.<br>
</blockquote>
<div><br>
</div>
<div>This is really good to know. Personally I was itching to refactor the whole</div>
<div>lot but this gives a me a much better path.</div>
<div> </div>
<blockquote class="x_gmail_quote" style="margin:0px 0px 0px 0.8ex; border-left:1px solid rgb(204,204,204); padding-left:1ex">
cl_sync_io_wait() has been changed to always take csi_waitq.lock. This<br>
avoids the need for anchor->csi_barrier, and provides a more robust way<br>
to ensure that cl_sync_io_note() is completely finished before<br>
cl_sync_io_wait() completes.<br>
<br>
That isn't quite the outcome I was expecting...<br>
<br>
I'm sorry if I seem to have been rather harsh on your patch - my<br>
intention was only to make sure it was the best it could be.<br>
<br>
It really needed to have the stack trace and the arch were the LBUG<br>
happened to be at all complete.<br>
I would have been nice if it identified the particular code where the<br>
barrier was needed: between cl_sync_io_note() and cl_sync_io_wait()<br>
because that mediates and ownership transition that happens while the VM<br>
page is locked.<br>
<br>
The rest are probably just issues of taste.<br>
<br>
Thanks,<br>
NeilBrown<br>
</blockquote>
<div><br>
</div>
<div>Thanks I really appreciate the time need to consider the issue.</div>
<div>And I agree any patch needs to be as good and tasteful as we can make them.</div>
<div><br>
</div>
<div>Regards,</div>
<div>Shaun</div>
<div><br>
</div>
</div>
</div>
</div>
</body>
</html>