On Thu, Jan 28, 2021 at 06:51:46PM +0800, Qu Wenruo wrote:

On 2021/1/28 下午6:34, David Sterba wrote:

quoted

On Thu, Jan 28, 2021 at 08:30:21AM +0800, Qu Wenruo wrote:

quoted

quoted

quoted

    btrfs: support subpage for extent buffer page release

I don't have this patch in my inbox so I can't reply to it directly, but
you include refcount.h, but then use normal atomics.  Please used the
actual refcount_t, as it gets us all the debugging stuff that makes
finding problems much easier.  Thanks,

My bad, my initial plan is to use refcount, but the use case has valid 0
refcount usage, thus refcount is not good here.

In case you need to shift the "0" you can use refcount_dec_not_one or
refcount_inc/dec_not_zero, but I haven't seen the code so don't know if
this applies in your case.

In the code, what we want is inc on zero, which will cause warning on 
refcount. (initial subpage allocation has zero ref, then increased to 
one when one eb is attached to the page)

But maybe I can change the timing so that we can use refcount.
Current code uses ASSERT()s to prevent underflow, so it would be 
sufficient for current code base though.

Assert for an underflow is ok but the refcount catches inc from zero ie.
a potential use after free.

With lifted refcount it should be possible to distinguish states where
it's really freed (0, to be deallocated) and 1 which is some middle
state like initialized, valid but not yet attached. Usage will increase
the ref, once there are no users, compare to 1, and then final put is
back to 0. A similar pattern is done for extent buffers, the subpage
data probably have similar lifetime.