POSIX_FADV_DONTNEED implemented wrong

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I believe the current implementation for this is wrong. For clean pages, it immediately discards them from the cache, and for dirty ones, it immediately tries to initiate writeout if the bdi is not congested. I believe this is wrong for three reasons:

1) It is completely useless for writing files. This hint should allow a program generating lots of writes to files that will not likely be read again to reduce the cache pressure that causes.

2) When there is little to no cache pressure, this hint should not cause the disk to spin up.

3) This is supposed to be a hint that caching this data is unlikely to do any good, so the cache should favor other data instead. Just because one process does not think it will be used again does not mean it won't be, so when there is little to no cache pressure, we shouldn't go discarding potentially useful data.

I'd like to change this to simply force the pages to the inactive list, so they will be reclaimed sooner than other pages, but not immediately discarded, or written out.

Quoted 4 lines

1) It is completely useless for writing files. This hint should allow a program generating lots of writes to files that will not likely be read again to reduce the cache pressure that causes.

Wouldn't direct IO make more sense in that case?

Minchan worked on deactivating pages on truncation. Maybe all it takes is to implement deactivate_mapping_range() or something to combine a page cache walk with deactivate_page().

While you are at it, madvise(MADV_DONTNEED) does not do anything to the page cache, but it probably should. :-)

2) Discarding pages under low cache pressure is a waste
3) It was useless on files being written, and thus full of dirty pages

Now we just move the pages to the inactive list so they will be reclaimed sooner.

Folks actually use this in practice to flush the page cache out:

http://git.sr71.net/?p=eyefi-config.git;a=blob;f=eyefi-linux.c;h=b77a891995109f6caa288925a13985cc495d7b2d;hb=HEAD#l62

I have really good reasons for really wanting to be _rid_ of the
page cache no matter how much memory pressure there is.

I've seen people at IBM using this to ensure that they stay out of memory reclaim completely. I don't completely agree with the approach, but this would completely ruin their performance since the VM-initiated writeout is so relatively slow for them.

I think this patch is a really bad idea. If you want the behavior you're proposing, I'd suggest using another flag.

This is the correct behavior prescribed by posix. If you have been using it for that purpose in the past, then you were using the wrong syscall. If you want to begin writeout now, then you should be using sync_file_range(). As it was, it only initiated writeout if the backing device was not already congested, which is going to no longer be the case rather soon if you ( or other tasks ) are writing significant amounts of data.

If you really want to stay out of memory reclaim entirely, then you should be using O_DIRECT.

These are folks that want to use the page cache, but also want to be in control of when it gets written out (sync_file_range() is used) and when it goes away. Sure, they can use O_DIRECT and do all of the buffering internally, but that means changing the application.

I actually really like the concept behind your patch. It looks like very useful functionality. I'm just saying that I know it will break _existing_ users.

Quoted 3 lines

Quoted 3 lines

I actually really like the concept behind your patch. It looks like very useful functionality. I'm just saying that I know it will break _existing_ users.

I'm not seeing how it will break anything. Which aspect of the current behavior is the app relying on? If it is the immediate removal of clean pages from the cache, then it should not care about the new behavior since the pages will still be removed very soon when under high cache pressure.

Essentially, they don't want any I/O initiated except that which is initiated by the app. If you let the system get in to reclaim, it'll start doing dirty writeout for pages other than those the app is interested in.

Quoted 4 lines · Dave Hansen

Essentially, they don't want any I/O initiated except that which is initiated by the app. If you let the system get in to reclaim, it'll start doing dirty writeout for pages other than those the app is interested in.

Are you talking about IO initiated by the app, or other tasks in the system? If the former then it won't be affected by this change.

From the looks of your patch, deactivating all the pages (if clean of

The previous implementation initiated writeout for a non congested bdi, and
then discarded any clean pages. This had 3 problems:

1) The writeout would spin up the disk unnecessarily
2) Discarding pages under low cache pressure is a waste
3) It was useless on files being written, and thus full of dirty pages

Now we just move the pages to the inactive list so they will be reclaimed sooner.

---
 include/linux/fs.h |  2 ++
 mm/fadvise.c       |  8 ++------
 mm/filemap.c       | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 6 deletions(-)

include/linux/fs.h +2 -0

index 7d2e893..2abd193 100644
@@ -2198,6 +2198,8 @@ extern int __filemap_fdatawrite_range(struct address_space *mapping,
 				loff_t start, loff_t end, int sync_mode);
 extern int filemap_fdatawrite_range(struct address_space *mapping,
 				loff_t start, loff_t end);
+extern void filemap_deactivate_range(struct address_space *mapping, pgoff_t start,
+				     pgoff_t end);
 
 extern int vfs_fsync_range(struct file *file, loff_t start, loff_t end,
 			   int datasync);

mm/fadvise.c +2 -6

index a47f0f5..fbd58b0 100644
@@ -112,17 +112,13 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, loff_t offset, loff_t len, int advice)
 	case POSIX_FADV_NOREUSE:
 		break;
 	case POSIX_FADV_DONTNEED:
-		if (!bdi_write_congested(mapping->backing_dev_info))
-			__filemap_fdatawrite_range(mapping, offset, endbyte,
-						   WB_SYNC_NONE);
-
 		/* First and last FULL page! */
 		start_index = (offset+(PAGE_CACHE_SIZE-1)) >> PAGE_CACHE_SHIFT;
 		end_index = (endbyte >> PAGE_CACHE_SHIFT);
 
 		if (end_index >= start_index)
-			invalidate_mapping_pages(mapping, start_index,
-						end_index);
+			filemap_deactivate_range(mapping, start_index,
+						 end_index);
 		break;
 	default:
 		ret = -EINVAL;

mm/filemap.c +43 -0

index c610076..bcdcdbf 100644
@@ -217,7 +217,49 @@ int __filemap_fdatawrite_range(struct address_space *mapping, loff_t start,
 	return ret;
 }
 
+/**
+ * filemap_deactivate_range - moves pages in range to the inactive list
+ * @mapping:	the address_space which holds the pages to deactivate
+ * @start:	offset where the range starts
+ * @end:	offset where the range ends (inclusive)
+ */
+void filemap_deactivate_range(struct address_space *mapping, pgoff_t start,
+			      pgoff_t end)
+{
+	struct pagevec pvec;
+	pgoff_t index = start;
+	int i;
+
+	/*
+	 * Note: this function may get called on a shmem/tmpfs mapping:
+	 * pagevec_lookup() might then return 0 prematurely (because it
+	 * got a gangful of swap entries); but it's hardly worth worrying
+	 * about - it can rarely have anything to free from such a mapping
+	 * (most pages are dirty), and already skips over any difficulties.
+	 */
+
+	pagevec_init(&pvec, 0);
+	while (index <= end && pagevec_lookup(&pvec, mapping, index,
+			min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1)) {
+		mem_cgroup_uncharge_start();
+		for (i = 0; i < pagevec_count(&pvec); i++) {
+			struct page *page = pvec.pages[i];
+
+			/* We rely upon deletion not changing page->index */
+			index = page->index;
+			if (index > end)
+				break;
+
+			WARN_ON(page->index != index);
+			deactivate_page(page);
+		}
+		pagevec_release(&pvec);
+		mem_cgroup_uncharge_end();
+		cond_resched();
+		index++;
+	}
+}
+
 static inline int __filemap_fdatawrite(struct address_space *mapping,
 	int sync_mode)
 {

Signature · 7 lines

1.8.1.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Hi Phillip,

I think we need to initiate writeout. IIRC, when we try to free pages, we would wait on page writeback. That will cause a huge latency for

some applications. If these pages have been written out, we just need to invalidate them. IMO we can move these pages to inactive list and write them out.

The previous implementation initiated writeout for a non congested bdi, and
then discarded any clean pages. This had 3 problems:

1) The writeout would spin up the disk unnecessarily
2) Discarding pages under low cache pressure is a waste
3) It was useless on files being written, and thus full of dirty pages

Now we just move the pages to the inactive list so they will be reclaimed sooner.

---
 include/linux/fs.h |  2 ++
 mm/fadvise.c       |  8 ++------
 mm/filemap.c       | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 6 deletions(-)

include/linux/fs.h +2 -0

index 7d2e893..2abd193 100644
@@ -2198,6 +2198,8 @@ extern int __filemap_fdatawrite_range(struct address_space *mapping,
 				loff_t start, loff_t end, int sync_mode);
 extern int filemap_fdatawrite_range(struct address_space *mapping,
 				loff_t start, loff_t end);
+extern void filemap_deactivate_range(struct address_space *mapping, pgoff_t start,
+				     pgoff_t end);
 
 extern int vfs_fsync_range(struct file *file, loff_t start, loff_t end,
 			   int datasync);

mm/fadvise.c +2 -6

index a47f0f5..fbd58b0 100644
@@ -112,17 +112,13 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, loff_t offset, loff_t len, int advice)
 	case POSIX_FADV_NOREUSE:
 		break;
 	case POSIX_FADV_DONTNEED:
-		if (!bdi_write_congested(mapping->backing_dev_info))
-			__filemap_fdatawrite_range(mapping, offset, endbyte,
-						   WB_SYNC_NONE);
-
 		/* First and last FULL page! */
 		start_index = (offset+(PAGE_CACHE_SIZE-1)) >> PAGE_CACHE_SHIFT;
 		end_index = (endbyte >> PAGE_CACHE_SHIFT);
 
 		if (end_index >= start_index)
-			invalidate_mapping_pages(mapping, start_index,
-						end_index);
+			filemap_deactivate_range(mapping, start_index,
+						 end_index);
 		break;
 	default:
 		ret = -EINVAL;

mm/filemap.c +43 -0

index c610076..bcdcdbf 100644
@@ -217,7 +217,49 @@ int __filemap_fdatawrite_range(struct address_space *mapping, loff_t start,
 	return ret;
 }
 
+/**
+ * filemap_deactivate_range - moves pages in range to the inactive list
+ * @mapping:	the address_space which holds the pages to deactivate
+ * @start:	offset where the range starts
+ * @end:	offset where the range ends (inclusive)
+ */
+void filemap_deactivate_range(struct address_space *mapping, pgoff_t start,
+			      pgoff_t end)
+{
+	struct pagevec pvec;
+	pgoff_t index = start;
+	int i;
+
+	/*
+	 * Note: this function may get called on a shmem/tmpfs mapping:
+	 * pagevec_lookup() might then return 0 prematurely (because it
+	 * got a gangful of swap entries); but it's hardly worth worrying
+	 * about - it can rarely have anything to free from such a mapping
+	 * (most pages are dirty), and already skips over any difficulties.
+	 */
+
+	pagevec_init(&pvec, 0);
+	while (index <= end && pagevec_lookup(&pvec, mapping, index,
+			min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1)) {
+		mem_cgroup_uncharge_start();
+		for (i = 0; i < pagevec_count(&pvec); i++) {
+			struct page *page = pvec.pages[i];
+
+			/* We rely upon deletion not changing page->index */
+			index = page->index;
+			if (index > end)
+				break;
+
+			WARN_ON(page->index != index);
+			deactivate_page(page);
+		}
+		pagevec_release(&pvec);
+		mem_cgroup_uncharge_end();
+		cond_resched();
+		index++;
+	}
+}
+
 static inline int __filemap_fdatawrite(struct address_space *mapping,
 	int sync_mode)
 {

Signature · 1 line

1.8.1.2

Hello,

Quoted 104 lines · Phillip Susi

The previous implementation initiated writeout for a non congested bdi, and
then discarded any clean pages. This had 3 problems:

1) The writeout would spin up the disk unnecessarily
2) Discarding pages under low cache pressure is a waste
3) It was useless on files being written, and thus full of dirty pages

Now we just move the pages to the inactive list so they will be reclaimed sooner.

---
 include/linux/fs.h |  2 ++
 mm/fadvise.c       |  8 ++------
 mm/filemap.c       | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+), 6 deletions(-)

include/linux/fs.h +2 -0

index 7d2e893..2abd193 100644
@@ -2198,6 +2198,8 @@ extern int __filemap_fdatawrite_range(struct address_space *mapping,
 				loff_t start, loff_t end, int sync_mode);
 extern int filemap_fdatawrite_range(struct address_space *mapping,
 				loff_t start, loff_t end);
+extern void filemap_deactivate_range(struct address_space *mapping, pgoff_t start,
+				     pgoff_t end);
 
 extern int vfs_fsync_range(struct file *file, loff_t start, loff_t end,
 			   int datasync);

mm/fadvise.c +2 -6

index a47f0f5..fbd58b0 100644
@@ -112,17 +112,13 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, loff_t offset, loff_t len, int advice)
 	case POSIX_FADV_NOREUSE:
 		break;
 	case POSIX_FADV_DONTNEED:
-		if (!bdi_write_congested(mapping->backing_dev_info))
-			__filemap_fdatawrite_range(mapping, offset, endbyte,
-						   WB_SYNC_NONE);
-
 		/* First and last FULL page! */
 		start_index = (offset+(PAGE_CACHE_SIZE-1)) >> PAGE_CACHE_SHIFT;
 		end_index = (endbyte >> PAGE_CACHE_SHIFT);
 
 		if (end_index >= start_index)
-			invalidate_mapping_pages(mapping, start_index,
-						end_index);
+			filemap_deactivate_range(mapping, start_index,
+						 end_index);
 		break;
 	default:
 		ret = -EINVAL;

mm/filemap.c +43 -0

index c610076..bcdcdbf 100644
@@ -217,7 +217,49 @@ int __filemap_fdatawrite_range(struct address_space *mapping, loff_t start,
 	return ret;
 }
 
+/**
+ * filemap_deactivate_range - moves pages in range to the inactive list
+ * @mapping:	the address_space which holds the pages to deactivate
+ * @start:	offset where the range starts
+ * @end:	offset where the range ends (inclusive)
+ */
+void filemap_deactivate_range(struct address_space *mapping, pgoff_t start,
+			      pgoff_t end)
+{
+	struct pagevec pvec;
+	pgoff_t index = start;
+	int i;
+
+	/*
+	 * Note: this function may get called on a shmem/tmpfs mapping:
+	 * pagevec_lookup() might then return 0 prematurely (because it
+	 * got a gangful of swap entries); but it's hardly worth worrying
+	 * about - it can rarely have anything to free from such a mapping
+	 * (most pages are dirty), and already skips over any difficulties.
+	 */
+
+	pagevec_init(&pvec, 0);
+	while (index <= end && pagevec_lookup(&pvec, mapping, index,
+			min(end - index, (pgoff_t)PAGEVEC_SIZE - 1) + 1)) {
+		mem_cgroup_uncharge_start();
+		for (i = 0; i < pagevec_count(&pvec); i++) {
+			struct page *page = pvec.pages[i];
+
+			/* We rely upon deletion not changing page->index */
+			index = page->index;
+			if (index > end)
+				break;
+
+			WARN_ON(page->index != index);
+			deactivate_page(page);
+		}
+		pagevec_release(&pvec);
+		mem_cgroup_uncharge_end();
+		cond_resched();
+		index++;
+	}
+}
+
 static inline int __filemap_fdatawrite(struct address_space *mapping,

Quoted 5 lines

mm/filemap.c +0 -0

 	int sync_mode)
 {

Signature · 1 line

1.8.1.2

Just FYI,
there was a person tried to solve similar problem, Andrea Righi. Ccing him,

Personally, I like this but unfortunately maintainer didn't like it due to breaking compatibility and I understand.
https://lkml.org/lkml/2011/6/28/468

You might need another round with akpm. :(

I also like this approach, it looks very similar to the one that I proposed a long time ago. However, last time we ended up saying that the next step should have been a proposal for a better page cache management interface for the userland, adding more fadvise() flags, obviously without breaking the current behavior.

We started with these ideal requirements, but unfortunately I didn't go ahead with this project:
http://marc.info/?l=linux-kernel&m=130917619416123&w=2

About breaking the compatibility, keep in mind that even tools like dd, for example, has been modified to support invalidating the cache for a file via POSIX_FADV_DONTNEED:
http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=commit;h=5f311553

And it expects to discard cache for the target pages, when possible, even if POSIX just says that it will not access the pages again any time soon.