| Message ID | 20220429064051.61552-9-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | A few fixup patches for z3fold | expand |
On Fri, Apr 29, 2022 at 8:40 AM Miaohe Lin <linmiaohe@huawei.com> wrote: > > Think about the below scene: > > CPU1 CPU2 > z3fold_reclaim_page z3fold_free > spin_lock(&pool->lock) get_z3fold_header -- hold page_lock > kref_get_unless_zero > kref_put--zhdr->refcount can be 1 now > !z3fold_page_trylock > kref_put -- zhdr->refcount is 0 now > release_z3fold_page > WARN_ON(!list_empty(&zhdr->buddy)); -- we're on buddy now! > spin_lock(&pool->lock); -- deadlock here! > > z3fold_reclaim_page might race with z3fold_free and will lead to pool lock > deadlock and zhdr buddy non-empty warning. To fix this, defer getting the > refcount until page_lock is held just like what __z3fold_alloc does. Note > this has the side effect that we won't break the reclaim if we meet a soon > to be released z3fold page now. > > Fixes: dcf5aedb24f8 ("z3fold: stricter locking and more careful reclaim") > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/z3fold.c | 18 +++--------------- > 1 file changed, 3 insertions(+), 15 deletions(-) > > diff --git a/mm/z3fold.c b/mm/z3fold.c > index 4a3cd2ff15b0..a7769befd74e 100644 > --- a/mm/z3fold.c > +++ b/mm/z3fold.c > @@ -519,13 +519,6 @@ static void __release_z3fold_page(struct z3fold_header *zhdr, bool locked) > atomic64_dec(&pool->pages_nr); > } > > -static void release_z3fold_page(struct kref *ref) > -{ > - struct z3fold_header *zhdr = container_of(ref, struct z3fold_header, > - refcount); > - __release_z3fold_page(zhdr, false); > -} > - > static void release_z3fold_page_locked(struct kref *ref) > { > struct z3fold_header *zhdr = container_of(ref, struct z3fold_header, > @@ -1317,12 +1310,7 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries) > break; > } > > - if (kref_get_unless_zero(&zhdr->refcount) == 0) { > - zhdr = NULL; > - break; > - } > if (!z3fold_page_trylock(zhdr)) { > - kref_put(&zhdr->refcount, release_z3fold_page); > zhdr = NULL; > continue; /* can't evict at this point */ > } > @@ -1333,14 +1321,14 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries) > */ > if (zhdr->foreign_handles || > test_and_set_bit(PAGE_CLAIMED, &page->private)) { > - if (!kref_put(&zhdr->refcount, > - release_z3fold_page_locked)) > - z3fold_page_unlock(zhdr); > + z3fold_page_unlock(zhdr); > zhdr = NULL; > continue; /* can't evict such page */ > } > list_del_init(&zhdr->buddy); > zhdr->cpu = -1; > + /* See comment in __z3fold_alloc. */ > + kref_get(&zhdr->refcount); > break; > } > Reviewed-by: Vitaly Wool <vitaly.wool@konsulko.com> > -- > 2.23.0 >
diff --git a/mm/z3fold.c b/mm/z3fold.c index 4a3cd2ff15b0..a7769befd74e 100644 --- a/mm/z3fold.c +++ b/mm/z3fold.c @@ -519,13 +519,6 @@ static void __release_z3fold_page(struct z3fold_header *zhdr, bool locked) atomic64_dec(&pool->pages_nr); } -static void release_z3fold_page(struct kref *ref) -{ - struct z3fold_header *zhdr = container_of(ref, struct z3fold_header, - refcount); - __release_z3fold_page(zhdr, false); -} - static void release_z3fold_page_locked(struct kref *ref) { struct z3fold_header *zhdr = container_of(ref, struct z3fold_header, @@ -1317,12 +1310,7 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries) break; } - if (kref_get_unless_zero(&zhdr->refcount) == 0) { - zhdr = NULL; - break; - } if (!z3fold_page_trylock(zhdr)) { - kref_put(&zhdr->refcount, release_z3fold_page); zhdr = NULL; continue; /* can't evict at this point */ } @@ -1333,14 +1321,14 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries) */ if (zhdr->foreign_handles || test_and_set_bit(PAGE_CLAIMED, &page->private)) { - if (!kref_put(&zhdr->refcount, - release_z3fold_page_locked)) - z3fold_page_unlock(zhdr); + z3fold_page_unlock(zhdr); zhdr = NULL; continue; /* can't evict such page */ } list_del_init(&zhdr->buddy); zhdr->cpu = -1; + /* See comment in __z3fold_alloc. */ + kref_get(&zhdr->refcount); break; }
Think about the below scene: CPU1 CPU2 z3fold_reclaim_page z3fold_free spin_lock(&pool->lock) get_z3fold_header -- hold page_lock kref_get_unless_zero kref_put--zhdr->refcount can be 1 now !z3fold_page_trylock kref_put -- zhdr->refcount is 0 now release_z3fold_page WARN_ON(!list_empty(&zhdr->buddy)); -- we're on buddy now! spin_lock(&pool->lock); -- deadlock here! z3fold_reclaim_page might race with z3fold_free and will lead to pool lock deadlock and zhdr buddy non-empty warning. To fix this, defer getting the refcount until page_lock is held just like what __z3fold_alloc does. Note this has the side effect that we won't break the reclaim if we meet a soon to be released z3fold page now. Fixes: dcf5aedb24f8 ("z3fold: stricter locking and more careful reclaim") Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/z3fold.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-)