On Fri, Nov 12, 2021 at 08:31:57AM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-11-11 15:11:34 [-0800], Minchan Kim wrote:
> > > > @@ -2077,8 +2043,13 @@ static unsigned long __zs_compact(struct zs_pool *pool,
> > > > struct zspage *dst_zspage = NULL;
> > > > unsigned long pages_freed = 0;
> > > >
> > > > + /* protect the race between zpage migration and zs_free */
> > > > + write_lock(&pool->migrate_lock);
> > > > + /* protect zpage allocation/free */
> > > > spin_lock(&class->lock);
> > > > while ((src_zspage = isolate_zspage(class, true))) {
> > > > + /* protect someone accessing the zspage(i.e., zs_map_object) */
> > > > + migrate_write_lock(src_zspage);
> > > >
> > > > if (!zs_can_compact(class))
> > > > break;
> > > > @@ -2087,6 +2058,8 @@ static unsigned long __zs_compact(struct zs_pool *pool,
> > > > cc.s_page = get_first_page(src_zspage);
> > > >
> > > > while ((dst_zspage = isolate_zspage(class, false))) {
> > > > + migrate_write_lock_nested(dst_zspage);
> > > > +
> > > > cc.d_page = get_first_page(dst_zspage);
> > > > /*
> > > > * If there is no more space in dst_page, resched
> > >
> > > Looking at the these two chunks, the page here comes from a list, you
> > > remove that page from that list and this ensures that you can't lock the
> > > very same pages in reverse order as in:
> > >
> > > migrate_write_lock(dst_zspage);
> > > …
> > > migrate_write_lock(src_zspage);
> > >
> > > right?
> >
> > Sure.
>
> Out of curiosity: why do you need to lock it then if you grab it from
> the list and there is no other reference to it? Is it because the page
> might be referenced by other means but only by a reader?
Yub, zs_map_object.
