Re: [PATCH v11 18/63] page cache: Add and replace pages using the XArray

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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));




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux