On Sat, Apr 14, 2018 at 07:12:31AM -0700, Matthew Wilcox wrote:
> - xa_lock_irq(&mapping->i_pages);
> - error = page_cache_tree_insert(mapping, page, shadowp);
> - radix_tree_preload_end();
> - if (unlikely(error))
> - goto err_insert;
> + do {
> + xas_lock_irq(&xas);
> + old = xas_create(&xas);
> + if (xas_error(&xas))
> + goto unlock;
> + if (xa_is_value(old)) {
> + mapping->nrexceptional--;
> + if (shadowp)
> + *shadowp = old;
> + } else if (old) {
> + xas_set_err(&xas, -EEXIST);
> + goto unlock;
> + }
> +
> + xas_store(&xas, page);
> + mapping->nrpages++;
At LSFMM, I was unable to explain in the moment why I was doing
xas_create() followed by xas_store(), rather than just calling
xas_store(). Looking at this code on my laptop, it was immediately
obvious to me -- the semantics of attempting to insert a page into the
page cache when there's already one there is to return -EEXIST.
We could also write this function this way:
+ old = xas_load(&xas);
+ if (old && !xa_is_value(old))
+ xas_set_err(&xas, -EEXIST);
+ xas_store(&xas, page);
+ if (xas_error(&xas))
+ goto unlock;
+ if (old) {
+ mapping->nrexceptional--;
+ if (shadowp)
+ *shadowp = old;
+ }
+ mapping->nrpages++;
which I think is slightly clearer. Or for those allergic to gotos, the
entire function could look like (option 3):
+ do {
+ xas_lock_irq(&xas);
+ old = xas_load(&xas);
+ if (old && !xa_is_value(old))
+ xas_set_err(&xas, -EEXIST);
+ xas_store(&xas, page);
+
+ if (!xas_error(&xas)) {
+ if (old) {
+ mapping->nrexceptional--;
+ if (shadowp)
+ *shadowp = old;
+ }
+ mapping->nrpages++;
+
+ /*
+ * hugetlb pages do not participate in
+ * page cache accounting.
+ */
+ if (!huge)
+ __inc_node_page_state(page, NR_FILE_PAGES);
+ }
+ xas_unlock_irq(&xas);
+ } while (xas_nomem(&xas, gfp_mask & ~__GFP_HIGHMEM));
