Re: [PATCH v6 4/6] mm/swapcache: support to handle the exceptional entries in swapcache

[Joonsoo Kim]

On Wed, Jun 17, 2020 at 05:17:17AM -0700, Matthew Wilcox wrote:
> On Wed, Jun 17, 2020 at 02:26:21PM +0900, js1304@xxxxxxxxx wrote:
> > From: Joonsoo Kim <iamjoonsoo.kim@xxxxxxx>
> > 
> > Swapcache doesn't handle the exceptional entries since there is no case
> 
> Don't call them exceptional entries.
> 
> The radix tree has/had the concecpt of exceptional entries.  The swapcache
> doesn't use the radix tree any more, it uses the XArray.  The XArray
> has value entries.
> 
> But you shouldn't call them value entries either; that's an XArray
> concept.  The swap cache and indeed page cache use value entries to
> implement shadow entries (they're also used to implement dax entries and
> swap entries in the page cache).  So just call them shadow entries here.
> 
> I know there are still places which use the term 'nrexceptional' in
> the kernel.  I just haven't got round to replacing them yet.  Please
> don't add more.

Okay! Thanks for commenting.

> 
> > +void clear_shadow_from_swap_cache(int type, unsigned long begin,
> > +				unsigned long end)
> > +{
> > +	unsigned long curr;
> > +	void *old;
> > +	swp_entry_t entry = swp_entry(type, begin);
> > +	struct address_space *address_space = swap_address_space(entry);
> > +	XA_STATE(xas, &address_space->i_pages, begin);
> > +
> > +retry:
> > +	xa_lock_irq(&address_space->i_pages);
> > +	for (curr = begin; curr <= end; curr++) {
> > +		entry = swp_entry(type, curr);
> > +		if (swap_address_space(entry) != address_space) {
> > +			xa_unlock_irq(&address_space->i_pages);
> > +			address_space = swap_address_space(entry);
> > +			begin = curr;
> > +			xas_set(&xas, begin);
> > +			goto retry;
> > +		}
> > +
> > +		old = xas_load(&xas);
> > +		if (!xa_is_value(old))
> > +			continue;
> > +		xas_store(&xas, NULL);
> > +		address_space->nrexceptional--;
> > +		xas_next(&xas);
> > +	}
> > +	xa_unlock_irq(&address_space->i_pages);
> > +}
> 
> This is a very clunky loop.  I'm not sure it's even right, given that
> you change address space without changing the xas's address space.  How
> about this?

You are correct. The xas's address space should be changed.


> 	for (;;) {
> 		XA_STATE(xas, &address_space->i_pages, begin);
> 		unsigned long nr_shadows = 0;
> 
> 		xas_lock_irq(&xas);
> 		xas_for_each(&xas, entry, end) {
> 			if (!xa_is_value(entry))
> 				continue;
> 			xas_store(&xas, NULL);
> 			nr_shadows++;
> 		}
> 		address_space->nr_exceptionals -= nr_shadows;
> 		xas_unlock_irq(&xas);
> 
> 		if (xas.xa_index >= end)
> 			break;
> 		entry = swp_entry(type, xas.xa_index);
> 		address_space = swap_address_space(entry);
> 	}

Thanks for suggestion.

I make a patch based on your suggestion. IIUC about Xarray,
after running xas_for_each(), xas.xa_index can be less than the end if
there are empty slots on last portion of array. Handling this case is
also considered in my patch.

Thanks.

------------------->8--------------------------------