On Thu, Jan 28, 2021 at 06:51:46PM +0800, Qu Wenruo wrote: > On 2021/1/28 下午6:34, David Sterba wrote: > > On Thu, Jan 28, 2021 at 08:30:21AM +0800, Qu Wenruo wrote: > >>>> 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.
