Messages in this thread | | | Subject | Re: [PATCH] Revert "mm/z3fold.c: fix race between migration and destruction" | From | Vlastimil Babka <> | Date | Tue, 10 Sep 2019 13:19:04 +0200 |
| |
On 9/10/19 12:31 PM, Vitaly Wool wrote: > With the original commit applied, z3fold_zpool_destroy() may > get blocked on wait_event() for indefinite time. Revert this > commit for the time being to get rid of this problem since the > issue the original commit addresses is less severe. > > This reverts commit d776aaa9895eb6eb770908e899cb7f5bd5025b3c.
Let's make it clear that the revert should go to 5.3 immediately, because the commit above was introduced before 5.3-rc6.
> Reported-by: Agustín Dall'Alba <agustin@dallalba.com.ar> > Signed-off-by: Vitaly Wool <vitalywool@gmail.com> Fixes: d776aaa9895e ("mm/z3fold.c: fix race between migration and destruction") Cc: stable <stable@vger.kernel.org>
d776aaa9895e was Cc: stable, so if stable kernels picked it up, they should pick the revert as well.
> --- > mm/z3fold.c | 90 ----------------------------------------------------- > 1 file changed, 90 deletions(-) > > diff --git a/mm/z3fold.c b/mm/z3fold.c > index 75b7962439ff..ed19d98c9dcd 100644 > --- a/mm/z3fold.c > +++ b/mm/z3fold.c > @@ -41,7 +41,6 @@ > #include <linux/workqueue.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > -#include <linux/wait.h> > #include <linux/zpool.h> > #include <linux/magic.h> > > @@ -146,8 +145,6 @@ struct z3fold_header { > * @release_wq: workqueue for safe page release > * @work: work_struct for safe page release > * @inode: inode for z3fold pseudo filesystem > - * @destroying: bool to stop migration once we start destruction > - * @isolated: int to count the number of pages currently in isolation > * > * This structure is allocated at pool creation time and maintains metadata > * pertaining to a particular z3fold pool. > @@ -166,11 +163,8 @@ struct z3fold_pool { > const struct zpool_ops *zpool_ops; > struct workqueue_struct *compact_wq; > struct workqueue_struct *release_wq; > - struct wait_queue_head isolate_wait; > struct work_struct work; > struct inode *inode; > - bool destroying; > - int isolated; > }; > > /* > @@ -775,7 +769,6 @@ static struct z3fold_pool *z3fold_create_pool(const char *name, gfp_t gfp, > goto out_c; > spin_lock_init(&pool->lock); > spin_lock_init(&pool->stale_lock); > - init_waitqueue_head(&pool->isolate_wait); > pool->unbuddied = __alloc_percpu(sizeof(struct list_head)*NCHUNKS, 2); > if (!pool->unbuddied) > goto out_pool; > @@ -815,15 +808,6 @@ static struct z3fold_pool *z3fold_create_pool(const char *name, gfp_t gfp, > return NULL; > } > > -static bool pool_isolated_are_drained(struct z3fold_pool *pool) > -{ > - bool ret; > - > - spin_lock(&pool->lock); > - ret = pool->isolated == 0; > - spin_unlock(&pool->lock); > - return ret; > -} > /** > * z3fold_destroy_pool() - destroys an existing z3fold pool > * @pool: the z3fold pool to be destroyed > @@ -833,22 +817,6 @@ static bool pool_isolated_are_drained(struct z3fold_pool *pool) > static void z3fold_destroy_pool(struct z3fold_pool *pool) > { > kmem_cache_destroy(pool->c_handle); > - /* > - * We set pool-> destroying under lock to ensure that > - * z3fold_page_isolate() sees any changes to destroying. This way we > - * avoid the need for any memory barriers. > - */ > - > - spin_lock(&pool->lock); > - pool->destroying = true; > - spin_unlock(&pool->lock); > - > - /* > - * We need to ensure that no pages are being migrated while we destroy > - * these workqueues, as migration can queue work on either of the > - * workqueues. > - */ > - wait_event(pool->isolate_wait, !pool_isolated_are_drained(pool)); > > /* > * We need to destroy pool->compact_wq before pool->release_wq, > @@ -1339,28 +1307,6 @@ static u64 z3fold_get_pool_size(struct z3fold_pool *pool) > return atomic64_read(&pool->pages_nr); > } > > -/* > - * z3fold_dec_isolated() expects to be called while pool->lock is held. > - */ > -static void z3fold_dec_isolated(struct z3fold_pool *pool) > -{ > - assert_spin_locked(&pool->lock); > - VM_BUG_ON(pool->isolated <= 0); > - pool->isolated--; > - > - /* > - * If we have no more isolated pages, we have to see if > - * z3fold_destroy_pool() is waiting for a signal. > - */ > - if (pool->isolated == 0 && waitqueue_active(&pool->isolate_wait)) > - wake_up_all(&pool->isolate_wait); > -} > - > -static void z3fold_inc_isolated(struct z3fold_pool *pool) > -{ > - pool->isolated++; > -} > - > static bool z3fold_page_isolate(struct page *page, isolate_mode_t mode) > { > struct z3fold_header *zhdr; > @@ -1387,34 +1333,6 @@ static bool z3fold_page_isolate(struct page *page, isolate_mode_t mode) > spin_lock(&pool->lock); > if (!list_empty(&page->lru)) > list_del(&page->lru); > - /* > - * We need to check for destruction while holding pool->lock, as > - * otherwise destruction could see 0 isolated pages, and > - * proceed. > - */ > - if (unlikely(pool->destroying)) { > - spin_unlock(&pool->lock); > - /* > - * If this page isn't stale, somebody else holds a > - * reference to it. Let't drop our refcount so that they > - * can call the release logic. > - */ > - if (unlikely(kref_put(&zhdr->refcount, > - release_z3fold_page_locked))) { > - /* > - * If we get here we have kref problems, so we > - * should freak out. > - */ > - WARN(1, "Z3fold is experiencing kref problems\n"); > - z3fold_page_unlock(zhdr); > - return false; > - } > - z3fold_page_unlock(zhdr); > - return false; > - } > - > - > - z3fold_inc_isolated(pool); > spin_unlock(&pool->lock); > z3fold_page_unlock(zhdr); > return true; > @@ -1483,10 +1401,6 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa > > queue_work_on(new_zhdr->cpu, pool->compact_wq, &new_zhdr->work); > > - spin_lock(&pool->lock); > - z3fold_dec_isolated(pool); > - spin_unlock(&pool->lock); > - > page_mapcount_reset(page); > put_page(page); > return 0; > @@ -1506,14 +1420,10 @@ static void z3fold_page_putback(struct page *page) > INIT_LIST_HEAD(&page->lru); > if (kref_put(&zhdr->refcount, release_z3fold_page_locked)) { > atomic64_dec(&pool->pages_nr); > - spin_lock(&pool->lock); > - z3fold_dec_isolated(pool); > - spin_unlock(&pool->lock); > return; > } > spin_lock(&pool->lock); > list_add(&page->lru, &pool->lru); > - z3fold_dec_isolated(pool); > spin_unlock(&pool->lock); > z3fold_page_unlock(zhdr); > } >
| |