On 2017/9/26 下午3:15, 박병철/선임연구원/SW
Platform(연)AOT팀(byungchul.park@lge.com) wrote:

quoted

-----Original Message-----
From: Coly Li [mailto:i@coly.li]
Sent: Tuesday, September 26, 2017 4:09 PM
To: 박병철/선임연구원/SW Platform(연)AOT팀(byungchul.park@lge.com);
Michael Lyle; Coly Li
Cc: linux-bcache@vger.kernel.org; linux-block@vger.kernel.org;
axboe@kernel.dk; Eric Wheeler; Kent Overstreet; kernel-team@lge.com
Subject: Re: [PATCH 04/12] bcache: Don't reinvent the wheel but use existing
llist API

On 2017/9/26 下午2:39, 박병철/선임연구원/SW
Platform(연)AOT팀(byungchul.park@lge.com) wrote:

quoted

quoted

-----Original Message-----
From: Michael Lyle [mailto:mlyle@lyle.org]
Sent: Tuesday, September 26, 2017 1:38 PM
To: Coly Li
Cc: linux-bcache@vger.kernel.org; linux-block@vger.kernel.org;
axboe@kernel.dk; Eric Wheeler; Byungchul Park; Kent Overstreet
Subject: Re: [PATCH 04/12] bcache: Don't reinvent the wheel but use existing
llist API

I believe this introduces a critical bug.

cl->list is used to link together the llists for both things waiting,
and for things that are being woken.

If a closure that is woken decides to wait again, it will corrupt the
llist that __closure_wake_up is using.

The previous iteration structure gets the next element of the list
before waking and is therefore safe.

Do you mean we have to use llist_for_each_entry_safe() instead of non-safe

version?

quoted

Is it ok if we use it instead?

Yes, we should use llist_for_each_entry_safe(), there is a quite
implicit race here.

Hi coly,

As you know, my first patch used the safe version, but you suggested to replace
It with non-safe one. :(

No doubt, it's my fault. I didn't find the implicit potential race.

I will change it so it does the same as the original patch did. :)

Yes, please. Because the original patch is merged into Linux upstream,
post a fix to replace llist_for_each_entry() by
llist_for_each_entry_safe(). We still have chance to catch up 4.14.

Thank you !

-- 
Coly Li