[PATCH 1/2] cifs: convert to add_to_page_cache()

[PATCH 1/2] cifs: convert to add_to_page_cache()

[Kent Overstreet]

This is just open coding add_to_page_cache(), and the next patch will
delete add_to_page_cache_locked().

Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
---
 fs/cifs/file.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index be46fab4c9..a17a21181e 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -4296,20 +4296,12 @@ readpages_get_pages(struct address_space *mapping, struct list_head *page_list,
 
 	page = lru_to_page(page_list);
 
-	/*
-	 * Lock the page and put it in the cache. Since no one else
-	 * should have access to this page, we're safe to simply set
-	 * PG_locked without checking it first.
-	 */
-	__SetPageLocked(page);
-	rc = add_to_page_cache_locked(page, mapping,
-				      page->index, gfp);
+	rc = add_to_page_cache(page, mapping,
+			       page->index, gfp);
 
 	/* give up if we can't stick it in the cache */
-	if (rc) {
-		__ClearPageLocked(page);
+	if (rc)
 		return rc;
-	}
 
 	/* move first page to the tmplist */
 	*offset = (loff_t)page->index << PAGE_SHIFT;
@@ -4328,12 +4320,9 @@ readpages_get_pages(struct address_space *mapping, struct list_head *page_list,
 		if (*bytes + PAGE_SIZE > rsize)
 			break;
 
-		__SetPageLocked(page);
-		rc = add_to_page_cache_locked(page, mapping, page->index, gfp);
-		if (rc) {
-			__ClearPageLocked(page);
+		rc = add_to_page_cache(page, mapping, page->index, gfp);
+		if (rc)
 			break;
-		}
 		list_move_tail(&page->lru, tmplist);
 		(*bytes) += PAGE_SIZE;
 		expected_index++;
-- 
2.28.0

[PATCH 2/2] fs: kill add_to_page_cache_locked()

[Kent Overstreet]

No longer has any users, so remove it.

Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
---
 include/linux/pagemap.h | 20 ++-----------
 mm/filemap.c            | 62 ++++++++++++++++++++---------------------
 2 files changed, 32 insertions(+), 50 deletions(-)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 434c9c34ae..aceaebfaab 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -689,8 +689,8 @@ static inline int fault_in_pages_readable(const char __user *uaddr, int size)
 	return 0;
 }
 
-int add_to_page_cache_locked(struct page *page, struct address_space *mapping,
-				pgoff_t index, gfp_t gfp_mask);
+int add_to_page_cache(struct page *page, struct address_space *mapping,
+		      pgoff_t index, gfp_t gfp_mask);
 int add_to_page_cache_lru(struct page *page, struct address_space *mapping,
 				pgoff_t index, gfp_t gfp_mask);
 extern void delete_from_page_cache(struct page *page);
@@ -710,22 +710,6 @@ void page_cache_readahead_unbounded(struct address_space *, struct file *,
 		pgoff_t index, unsigned long nr_to_read,
 		unsigned long lookahead_count);
 
-/*
- * Like add_to_page_cache_locked, but used to add newly allocated pages:
- * the page is new, so we can just run __SetPageLocked() against it.
- */
-static inline int add_to_page_cache(struct page *page,
-		struct address_space *mapping, pgoff_t offset, gfp_t gfp_mask)
-{
-	int error;
-
-	__SetPageLocked(page);
-	error = add_to_page_cache_locked(page, mapping, offset, gfp_mask);
-	if (unlikely(error))
-		__ClearPageLocked(page);
-	return error;
-}
-
 /**
  * struct readahead_control - Describes a readahead request.
  *
diff --git a/mm/filemap.c b/mm/filemap.c
index 82e5e0ba24..c562ad7e05 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -827,10 +827,10 @@ int replace_page_cache_page(struct page *old, struct page *new, gfp_t gfp_mask)
 }
 EXPORT_SYMBOL_GPL(replace_page_cache_page);
 
-static int __add_to_page_cache_locked(struct page *page,
-				      struct address_space *mapping,
-				      pgoff_t offset, gfp_t gfp_mask,
-				      void **shadowp)
+static int __add_to_page_cache(struct page *page,
+			       struct address_space *mapping,
+			       pgoff_t offset, gfp_t gfp_mask,
+			       void **shadowp)
 {
 	XA_STATE(xas, &mapping->i_pages, offset);
 	int huge = PageHuge(page);
@@ -841,6 +841,7 @@ static int __add_to_page_cache_locked(struct page *page,
 	VM_BUG_ON_PAGE(PageSwapBacked(page), page);
 	mapping_set_update(&xas, mapping);
 
+	__SetPageLocked(page);
 	get_page(page);
 	page->mapping = mapping;
 	page->index = offset;
@@ -885,29 +886,30 @@ static int __add_to_page_cache_locked(struct page *page,
 	page->mapping = NULL;
 	/* Leave page->index set: truncation relies upon it */
 	put_page(page);
+	__ClearPageLocked(page);
 	return error;
 }
-ALLOW_ERROR_INJECTION(__add_to_page_cache_locked, ERRNO);
 
 /**
- * add_to_page_cache_locked - add a locked page to the pagecache
+ * add_to_page_cache - add a newly allocated page to the pagecache
  * @page:	page to add
  * @mapping:	the page's address_space
  * @offset:	page index
  * @gfp_mask:	page allocation mode
  *
- * This function is used to add a page to the pagecache. It must be locked.
- * This function does not add the page to the LRU.  The caller must do that.
+ * This function is used to add a page to the pagecache. It must be newly
+ * allocated.  This function does not add the page to the LRU.  The caller must
+ * do that.
  *
  * Return: %0 on success, negative error code otherwise.
  */
-int add_to_page_cache_locked(struct page *page, struct address_space *mapping,
-		pgoff_t offset, gfp_t gfp_mask)
+int add_to_page_cache(struct page *page, struct address_space *mapping,
+		      pgoff_t offset, gfp_t gfp_mask)
 {
-	return __add_to_page_cache_locked(page, mapping, offset,
-					  gfp_mask, NULL);
+	return __add_to_page_cache(page, mapping, offset, gfp_mask, NULL);
 }
-EXPORT_SYMBOL(add_to_page_cache_locked);
+EXPORT_SYMBOL(add_to_page_cache);
+ALLOW_ERROR_INJECTION(add_to_page_cache, ERRNO);
 
 int add_to_page_cache_lru(struct page *page, struct address_space *mapping,
 				pgoff_t offset, gfp_t gfp_mask)
@@ -915,26 +917,22 @@ int add_to_page_cache_lru(struct page *page, struct address_space *mapping,
 	void *shadow = NULL;
 	int ret;
 
-	__SetPageLocked(page);
-	ret = __add_to_page_cache_locked(page, mapping, offset,
-					 gfp_mask, &shadow);
+	ret = __add_to_page_cache(page, mapping, offset, gfp_mask, &shadow);
 	if (unlikely(ret))
-		__ClearPageLocked(page);
-	else {
-		/*
-		 * The page might have been evicted from cache only
-		 * recently, in which case it should be activated like
-		 * any other repeatedly accessed page.
-		 * The exception is pages getting rewritten; evicting other
-		 * data from the working set, only to cache data that will
-		 * get overwritten with something else, is a waste of memory.
-		 */
-		WARN_ON_ONCE(PageActive(page));
-		if (!(gfp_mask & __GFP_WRITE) && shadow)
-			workingset_refault(page, shadow);
-		lru_cache_add(page);
-	}
-	return ret;
+		return ret;
+
+	/*
+	 * The page might have been evicted from cache only recently, in which
+	 * case it should be activated like any other repeatedly accessed page.
+	 * The exception is pages getting rewritten; evicting other data from
+	 * the working set, only to cache data that will get overwritten with
+	 * something else, is a waste of memory.
+	 */
+	WARN_ON_ONCE(PageActive(page));
+	if (!(gfp_mask & __GFP_WRITE) && shadow)
+		workingset_refault(page, shadow);
+	lru_cache_add(page);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(add_to_page_cache_lru);
 
-- 
2.28.0

Re: [PATCH 2/2] fs: kill add_to_page_cache_locked()

[Matthew Wilcox]

On Mon, Oct 19, 2020 at 02:59:11PM -0400, Kent Overstreet wrote:
> @@ -885,29 +886,30 @@ static int __add_to_page_cache_locked(struct page *page,
>  	page->mapping = NULL;
>  	/* Leave page->index set: truncation relies upon it */
>  	put_page(page);
> +	__ClearPageLocked(page);
>  	return error;
>  }
> -ALLOW_ERROR_INJECTION(__add_to_page_cache_locked, ERRNO);

I think you're missing:

+ALLOW_ERROR_INJECTION(__add_to_page_cache, ERRNO);

I see this:
> +int add_to_page_cache(struct page *page, struct address_space *mapping,
> +		      pgoff_t offset, gfp_t gfp_mask)
>  {
> -	return __add_to_page_cache_locked(page, mapping, offset,
> -					  gfp_mask, NULL);
> +	return __add_to_page_cache(page, mapping, offset, gfp_mask, NULL);
>  }
> -EXPORT_SYMBOL(add_to_page_cache_locked);
> +EXPORT_SYMBOL(add_to_page_cache);
> +ALLOW_ERROR_INJECTION(add_to_page_cache, ERRNO);

but I think that's insufficient because most calls are to
add_to_page_cache_lru(), which doesn't have an error injection point.

By the way, that CIFS code is going to go away with the fscache rewrite.

Re: [PATCH 1/2] cifs: convert to add_to_page_cache()

[Christoph Hellwig]

> +	rc = add_to_page_cache(page, mapping,
> +			       page->index, gfp);

This trivially fits onto a single line.

Re: [PATCH 1/2] cifs: convert to add_to_page_cache()

[Steve French]

Other than the unnecessary split line which Christoph pointed out, looks fine.

You can add my Reviewed--by: Steve French <stfrench@microsoft.com>

On Tue, Oct 20, 2020 at 5:10 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> > +     rc = add_to_page_cache(page, mapping,
> > +                            page->index, gfp);
>
> This trivially fits onto a single line.



-- 
Thanks,

Steve