[lustre-devel] [PATCH] Place a memory barrier around cp_state changes

[NeilBrown]

On Mon, Jun 10 2019, Shaun Tancheff wrote:

> On Mon, Jun 10, 2019 at 7:49 PM NeilBrown <neilb at suse.com> wrote:
>
>>>
>> So I can just about see the possibility of the race that you think you
>> have hit.  But it couldn't happen on x86, as it has strong memory
>> ordering.
>>
>> What CPU architecture did this ASSERT trigger on?
>>
>
> Agree with everything above, unfortunately was aarch64.

Ahh.
OK, I'm now reasonably convinced that the patch probably fixes the bug.
It is a shame that there is no standard barrier between the wake_up and
the wait_event(), but there isn't and we need to work with that.

But I still don't like the patch.... :-(

I have a few problems with it.

Firstly, I generally don't like accessor functions.  They tend to hide
useful information.
Rather than cl_page_state_get() I would rather have
	 smp_load_acquire(&pg->cp_state)

where ever it is needed, with a comment explaining why it is needed in
that particular context.
Some people seem to like accessor functions - there certainly are a lot
of them in the kernel - but I don't think they help.
Having an accessor function that just adds a barrier is, I think,
particularly ugly as the comment can only be general in nature, and so
doesn't really help the reader to understand why the barrier is needed.
But even that isn't completely without precedent.
"key_read_state()" in include/linux/key.h simply wraps
smp_load_acquire(), and the comment there doesn't really tell me
anything useful.
"inet_sk_state_load" is slightly better it says it is for use in places
where the socket lock isn't held - and names some of those.

My second problem is that has "get" in the accessor function name is
potentially misleading and "get" normally has a matching "put", and
usually increments a refcount.  Having "read" or "load" in place of
"get" in the accessor function name would remove my second objection.

My third objection is that the bug doesn't exist in the upstream client
code.  I'm not sure whether this is luck or good management, but either
way it is gone.

cl_sync_io_wait() has been changed to always take csi_waitq.lock.  This
avoids the need for anchor->csi_barrier, and provides a more robust way
to ensure that cl_sync_io_note() is completely finished before
cl_sync_io_wait() completes.

That isn't quite the outcome I was expecting...

I'm sorry if I seem to have been rather harsh on your patch - my
intention was only to make sure it was the best it could be.

It really needed to have the stack trace and the arch were the LBUG
happened to be at all complete.
I would have been nice if it identified the particular code where the
barrier was needed: between cl_sync_io_note() and cl_sync_io_wait()
because that mediates and ownership transition that happens while the VM
page is locked.

The rest are probably just issues of taste.

Thanks,
NeilBrown



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20190612/6b656056/attachment.sig>