readahead stats/tracing, backwards prefetching and more (v2)

If the file size is 20kb and readahead request is [0, 16kb), it's better to expand the readahead request to [0, 20kb), which will likely save one followup I/O for [16kb, 20kb).

If the readahead request already covers EOF, trimm it down to EOF. Also don't set the PG_readahead mark to avoid an unnecessary future invocation of the readahead code.

This special handling looks worthwhile because small to medium sized files are pretty common.

Signed-off-by Wu Fengguang <[email protected]>

---
 mm/readahead.c |    8 ++++++++
 1 file changed, 8 insertions(+)

mm/readahead.c +2 -0

@@ -251,8 +251,16 @@ unsigned long max_sane_readahead(unsigne
 unsigned long ra_submit(struct file_ra_state *ra,
 		       struct address_space *mapping, struct file *filp)
 {
+	pgoff_t eof = ((i_size_read(mapping->host)-1) >> PAGE_CACHE_SHIFT) + 1;
+	pgoff_t start = ra->start;

Hmm, wouldn't it be cleaner to do this already in ondemand_readahead()?
All other updates of readahead window seem to be there.

Also shouldn't we
take maximum readahead size into account? Reading 3/2 of max readahead window seems like a relatively big deal for large files...

Quoted 2 lines

Hmm, wouldn't it be cleaner to do this already in ondemand_readahead()?
All other updates of readahead window seem to be there.

Yeah it's not that clean, however the intention is to cover the other call site -- mmap read-around, too.

Quoted 3 lines

Also shouldn't we
take maximum readahead size into account? Reading 3/2 of max readahead window seems like a relatively big deal for large files...

Good point, the max readahead size is actually a must, in order to prevent it expanding the readahead size for ever in the backwards reading case.

This limits the size expansion to 1/4 max readahead. That means, if the next expected readahead size will be less than 1/4 max size, it will be merged into the current readahead window to avoid one small IO.

The backwards reading is not special cased here because it's not frequent anyway.

unsigned long ra_submit(struct file_ra_state *ra,

Review

Quoted 2 lines

(quoted diff) +2 -0

+	/* snap to EOF */
+	size += min(size, ra->ra_pages / 4);

I'd probably choose:

	size += min(size / 2, ra->ra_pages / 4);

to increase current window only to 3/2 and not twice but I don't have a

strong opinion. Otherwise I think the code is fine now so you can add:

Acked-by Jan Kara <[email protected]>

Record the readahead pattern in ra->pattern and extend the ra_submit() parameters, to be used by the next readahead tracing/stats patches.

7 patterns are defined:

pattern readahead for
-----------------------------------------------------------
RA_PATTERN_INITIAL start-of-file read
RA_PATTERN_SUBSEQUENT trivial sequential read
RA_PATTERN_CONTEXT interleaved sequential read
RA_PATTERN_OVERSIZE oversize read
RA_PATTERN_MMAP_AROUND mmap fault
RA_PATTERN_FADVISE posix_fadvise()
RA_PATTERN_RANDOM random read

Note that random reads will be recorded in file_ra_state now. This won't deteriorate cache bouncing because the ra->prev_pos update in do_generic_file_read() already pollutes the data cache, and filemap_fault() will stop calling into us after MMAP_LOTSAMISS.

CC Ingo Molnar <[email protected]>

CC Jens Axboe <[email protected]>

CC Peter Zijlstra <[email protected]>

Acked-by Rik van Riel <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 include/linux/fs.h |   36 +++++++++++++++++++++++++++++++++++-
 include/linux/mm.h |    4 +++-
 mm/filemap.c       |    3 ++-
 mm/readahead.c     |   29 ++++++++++++++++++++++-------
 4 files changed, 62 insertions(+), 10 deletions(-)

include/linux/fs.h +35 -1

@@ -945,11 +945,45 @@ struct file_ra_state {
 					   there are only # of pages ahead */
 
 	unsigned int ra_pages;		/* Maximum readahead window */
-	unsigned int mmap_miss;		/* Cache miss stat for mmap accesses */
+	u16 mmap_miss;			/* Cache miss stat for mmap accesses */
+	u8 pattern;			/* one of RA_PATTERN_* */
+
 	loff_t prev_pos;		/* Cache last read() position */
 };
 
 /*
+ * Which policy makes decision to do the current read-ahead IO?
+ *
+ * RA_PATTERN_INITIAL		readahead window is initially opened,
+ *				normally when reading from start of file
+ * RA_PATTERN_SUBSEQUENT	readahead window is pushed forward
+ * RA_PATTERN_CONTEXT		no readahead window available, querying the
+ *				page cache to decide readahead start/size.
+ *				This typically happens on interleaved reads (eg.
+ *				reading pages 0, 1000, 1, 1001, 2, 1002, ...)
+ *				where one file_ra_state struct is not enough
+ *				for recording 2+ interleaved sequential read
+ *				streams.
+ * RA_PATTERN_MMAP_AROUND	read-around on mmap page faults
+ *				(w/o any sequential/random hints)
+ * RA_PATTERN_FADVISE		triggered by POSIX_FADV_WILLNEED or FMODE_RANDOM
+ * RA_PATTERN_OVERSIZE		a random read larger than max readahead size,
+ *				do max readahead to break down the read size
+ * RA_PATTERN_RANDOM		a small random read
+ */
+enum readahead_pattern {
+	RA_PATTERN_INITIAL,
+	RA_PATTERN_SUBSEQUENT,
+	RA_PATTERN_CONTEXT,
+	RA_PATTERN_MMAP_AROUND,
+	RA_PATTERN_FADVISE,
+	RA_PATTERN_OVERSIZE,
+	RA_PATTERN_RANDOM,
+	RA_PATTERN_ALL,		/* for summary stats */
+	RA_PATTERN_MAX
+};
+
+/*
  * Check if @index falls in the readahead windows.
  */
 static inline int ra_has_index(struct file_ra_state *ra, pgoff_t index)

mm/readahead.c +22 -7

@@ -249,7 +249,10 @@ unsigned long max_sane_readahead(unsigne
  * Submit IO for the read-ahead request in file_ra_state.
  */
 unsigned long ra_submit(struct file_ra_state *ra,
-		       struct address_space *mapping, struct file *filp)
+			struct address_space *mapping,
+			struct file *filp,
+			pgoff_t offset,
+			unsigned long req_size)
 {
 	pgoff_t eof = ((i_size_read(mapping->host)-1) >> PAGE_CACHE_SHIFT) + 1;
 	pgoff_t start = ra->start;

@@ -390,6 +393,7 @@ static int try_context_readahead(struct 
 	if (size >= offset)
 		size *= 2;
 
+	ra->pattern = RA_PATTERN_CONTEXT;
 	ra->start = offset;
 	ra->size = get_init_ra_size(size + req_size, max);
 	ra->async_size = ra->size;

@@ -411,8 +415,10 @@ ondemand_readahead(struct address_space 
 	/*
 	 * start of file
 	 */
-	if (!offset)
+	if (!offset) {
+		ra->pattern = RA_PATTERN_INITIAL;
 		goto initial_readahead;
+	}
 
 	/*
 	 * It's the expected callback offset, assume sequential access.

@@ -420,6 +426,7 @@ ondemand_readahead(struct address_space 
 	 */
 	if ((offset == (ra->start + ra->size - ra->async_size) ||
 	     offset == (ra->start + ra->size))) {
+		ra->pattern = RA_PATTERN_SUBSEQUENT;
 		ra->start += ra->size;
 		ra->size = get_next_ra_size(ra, max);
 		ra->async_size = ra->size;

@@ -442,6 +449,7 @@ ondemand_readahead(struct address_space 
 		if (!start || start - offset > max)
 			return 0;
 
+		ra->pattern = RA_PATTERN_CONTEXT;
 		ra->start = start;
 		ra->size = start - offset;	/* old async_size */
 		ra->size += req_size;

@@ -453,14 +461,18 @@ ondemand_readahead(struct address_space 
 	/*
 	 * oversize read
 	 */
-	if (req_size > max)
+	if (req_size > max) {
+		ra->pattern = RA_PATTERN_OVERSIZE;
 		goto initial_readahead;
+	}
 
 	/*
 	 * sequential cache miss
 	 */
-	if (offset - (ra->prev_pos >> PAGE_CACHE_SHIFT) <= 1UL)
+	if (offset - (ra->prev_pos >> PAGE_CACHE_SHIFT) <= 1UL) {
+		ra->pattern = RA_PATTERN_INITIAL;
 		goto initial_readahead;
+	}
 
 	/*
 	 * Query the page cache and look for the traces(cached history pages)

@@ -471,9 +483,12 @@ ondemand_readahead(struct address_space 
 
 	/*
 	 * standalone, small random read
-	 * Read as is, and do not pollute the readahead state.
 	 */
-	return __do_page_cache_readahead(mapping, filp, offset, req_size, 0);
+	ra->pattern = RA_PATTERN_RANDOM;
+	ra->start = offset;
+	ra->size = req_size;
+	ra->async_size = 0;
+	goto readit;
 
 initial_readahead:
 	ra->start = offset;

@@ -491,7 +506,7 @@ readit:
 		ra->size += ra->async_size;
 	}
 
-	return ra_submit(ra, mapping, filp);
+	return ra_submit(ra, mapping, filp, offset, req_size);
 }
 
 /**

include/linux/mm.h +3 -1

@@ -1456,7 +1456,9 @@ void page_cache_async_readahead(struct a
 unsigned long max_sane_readahead(unsigned long nr);
 unsigned long ra_submit(struct file_ra_state *ra,
 			struct address_space *mapping,
-			struct file *filp);
+			struct file *filp,
+			pgoff_t offset,
+			unsigned long req_size);
 
 /* Generic expand stack which grows the stack according to GROWS{UP,DOWN} */
 extern int expand_stack(struct vm_area_struct *vma, unsigned long address);

mm/filemap.c +2 -2

@@ -1611,11 +1611,12 @@ static void do_sync_mmap_readahead(struc
 	/*
 	 * mmap read-around
 	 */
+	ra->pattern = RA_PATTERN_MMAP_AROUND;
 	ra_pages = max_sane_readahead(ra->ra_pages);
 	ra->start = max_t(long, 0, offset - ra_pages / 2);
 	ra->size = ra_pages;
 	ra->async_size = ra_pages / 4;
-	ra_submit(ra, mapping, file);
+	ra_submit(ra, mapping, file, offset, 1);
 }
 
 /*
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Record the readahead pattern in ra->pattern and extend the ra_submit() parameters, to be used by the next readahead tracing/stats patches.

Quoted 2 lines · Wu Fengguang

Record the readahead pattern in ra->pattern and extend the ra_submit() parameters, to be used by the next readahead tracing/stats patches.

I like this, could it be exported it a bit more formally in /proc for each file descriptor?

I could imagine a monitoring tool that you run on a process that tells you what pattern state the various file descriptors are in and how large the window is. That would be similar to the tools for monitoring network connections, which are extremly useful
in practice.

Quoted 2 lines · Wu Fengguang

Record the readahead pattern in ra->pattern and extend the ra_submit() parameters, to be used by the next readahead tracing/stats patches.

I like this, could it be exported it a bit more formally in /proc for each file descriptor?

Quoted 6 lines · Andi Kleen

Quoted 2 lines · Wu Fengguang

Record the readahead pattern in ra->pattern and extend the ra_submit() parameters, to be used by the next readahead tracing/stats patches.

I like this, could it be exported it a bit more formally in /proc for each file descriptor?

How about this?
---
Subject: proc: show readahead state in fdinfo
Date: Thu Dec 15 14:35:56 CST 2011

Append three readahead states to /proc/<PID>/fdinfo/<FD>:

Introduce a bit field ra->for_mmap for tagging mmap reads. The tag will be cleared immediate after submitting the IO.

Signed-off-by Wu Fengguang <[email protected]>

---
 include/linux/fs.h |    1 +
 mm/filemap.c       |    6 +++++-
 mm/readahead.c     |    1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

include/linux/fs.h +1 -0

@@ -947,6 +947,7 @@ struct file_ra_state {
 	unsigned int ra_pages;		/* Maximum readahead window */
 	u16 mmap_miss;			/* Cache miss stat for mmap accesses */
 	u8 pattern;			/* one of RA_PATTERN_* */
+	unsigned int for_mmap:1;	/* readahead for mmap accesses */
 
 	loff_t prev_pos;		/* Cache last read() position */
 };

mm/filemap.c +5 -1

@@ -1592,6 +1592,7 @@ static void do_sync_mmap_readahead(struc
 		return;
 
 	if (VM_SequentialReadHint(vma)) {
+		ra->for_mmap = 1;
 		page_cache_sync_readahead(mapping, ra, file, offset,
 					  ra->ra_pages);
 		return;

@@ -1611,6 +1612,7 @@ static void do_sync_mmap_readahead(struc
 	/*
 	 * mmap read-around
 	 */
+	ra->for_mmap = 1;
 	ra->pattern = RA_PATTERN_MMAP_AROUND;
 	ra_pages = max_sane_readahead(ra->ra_pages);
 	ra->start = max_t(long, 0, offset - ra_pages / 2);

@@ -1636,9 +1638,11 @@ static void do_async_mmap_readahead(stru
 		return;
 	if (ra->mmap_miss > 0)
 		ra->mmap_miss--;
-	if (PageReadahead(page))
+	if (PageReadahead(page)) {
+		ra->for_mmap = 1;
 		page_cache_async_readahead(mapping, ra, file,
 					   page, offset, ra->ra_pages);
+	}
 }
 
 /**

mm/readahead.c +1 -1

@@ -267,6 +267,7 @@ unsigned long ra_submit(struct file_ra_s
 	actual = __do_page_cache_readahead(mapping, filp,
 					ra->start, ra->size, ra->async_size);
 
+	ra->for_mmap = 0;
 	return actual;
 }
 
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

We may be doing more metadata readahead in future.

Signed-off-by Wu Fengguang <[email protected]>

---
 fs/ext3/dir.c      |    1 +
 fs/ext4/dir.c      |    1 +
 include/linux/fs.h |    1 +
 mm/readahead.c     |    1 +
 4 files changed, 4 insertions(+)

fs/ext3/dir.c +1 -0

@@ -136,6 +136,7 @@ static int ext3_readdir(struct file * fi
 			pgoff_t index = map_bh.b_blocknr >>
 					(PAGE_CACHE_SHIFT - inode->i_blkbits);
 			if (!ra_has_index(&filp->f_ra, index))
+				filp->f_ra.for_metadata = 1;
 				page_cache_sync_readahead(
 					sb->s_bdev->bd_inode->i_mapping,
 					&filp->f_ra, filp,

fs/ext4/dir.c +1 -0

@@ -153,6 +153,7 @@ static int ext4_readdir(struct file *fil
 			pgoff_t index = map.m_pblk >>
 					(PAGE_CACHE_SHIFT - inode->i_blkbits);
 			if (!ra_has_index(&filp->f_ra, index))
+				filp->f_ra.for_metadata = 1;
 				page_cache_sync_readahead(
 					sb->s_bdev->bd_inode->i_mapping,
 					&filp->f_ra, filp,

include/linux/fs.h +1 -0

@@ -948,6 +948,7 @@ struct file_ra_state {
 	u16 mmap_miss;			/* Cache miss stat for mmap accesses */
 	u8 pattern;			/* one of RA_PATTERN_* */
 	unsigned int for_mmap:1;	/* readahead for mmap accesses */
+	unsigned int for_metadata:1;	/* readahead for meta data */
 
 	loff_t prev_pos;		/* Cache last read() position */
 };

mm/readahead.c +1 -1

@@ -268,6 +268,7 @@ unsigned long ra_submit(struct file_ra_s
 					ra->start, ra->size, ra->async_size);
 
 	ra->for_mmap = 0;
+	ra->for_metadata = 0;
 	return actual;
 }
 
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

The accounting code will be compiled in by default (CONFIG_READAHEAD_STATS=y), and will remain inactive by default.

It can be runtime enabled/disabled through the debugfs interface

echo 1 > /debug/readahead/stats_enable
echo 0 > /debug/readahead/stats_enable

The added overheads are two readahead_stats() calls per readahead. Which is trivial costs unless there are concurrent random reads on super fast SSDs, which may lead to cache bouncing when updating the global ra_stats[][]. Considering that normal users won't need this except when debugging performance problems, it's disabled by default. So it looks reasonable to keep this debug code simple rather than trying to improve its scalability.

Example output:
(taken from a fresh booted NFS-ROOT console box with rsize=524288)

$ cat /debug/readahead/stats
pattern     readahead    eof_hit  cache_hit         io    sync_io    mmap_io    meta_io       size async_size    io_size
initial           702        511          0        692        692          0          0          2          0          2
subsequent          7          0          1          7          1          1          0         23         22         23
context           160        161          0          2          0          1          0          0          0         16
around            184        184        177        184        184        184          0         58          0         53
backwards           2          0          2          2          2          0          0          4          0          3
fadvise          2593         47          8       2588       2588          0          0          1          0          1
oversize            0          0          0          0          0          0          0          0          0          0
random             45         20          0         44         44          0          0          1          0          1
all              3697        923        188       3519       3511        186          0          4          0          4

The two most important columns are
- io number of readahead IO
- io_size average readahead IO size

CC Ingo Molnar <[email protected]>

CC Jens Axboe <[email protected]>

CC Peter Zijlstra <[email protected]>

Acked-by Rik van Riel <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 mm/Kconfig     |   15 +++
 mm/readahead.c |  194 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 209 insertions(+)

mm/readahead.c +194 -0

@@ -18,6 +18,17 @@
 #include <linux/pagevec.h>
 #include <linux/pagemap.h>
 
+static const char * const ra_pattern_names[] = {
+	[RA_PATTERN_INITIAL]            = "initial",
+	[RA_PATTERN_SUBSEQUENT]         = "subsequent",
+	[RA_PATTERN_CONTEXT]            = "context",
+	[RA_PATTERN_MMAP_AROUND]        = "around",
+	[RA_PATTERN_FADVISE]            = "fadvise",
+	[RA_PATTERN_OVERSIZE]           = "oversize",
+	[RA_PATTERN_RANDOM]             = "random",
+	[RA_PATTERN_ALL]                = "all",
+};
+
 /*
  * Initialise a struct file's readahead state.  Assumes that the caller has
  * memset *ra to zero.

@@ -32,6 +43,181 @@ EXPORT_SYMBOL_GPL(file_ra_state_init);
 
 #define list_to_page(head) (list_entry((head)->prev, struct page, lru))
 
+#ifdef CONFIG_READAHEAD_STATS
+#include <linux/seq_file.h>
+#include <linux/debugfs.h>
+
+static u32 readahead_stats_enable __read_mostly;
+
+enum ra_account {
+	/* number of readaheads */
+	RA_ACCOUNT_COUNT,	/* readahead request */
+	RA_ACCOUNT_EOF,		/* readahead request covers EOF */
+	RA_ACCOUNT_CACHE_HIT,	/* readahead request covers some cached pages */
+	RA_ACCOUNT_IOCOUNT,	/* readahead IO */
+	RA_ACCOUNT_SYNC,	/* readahead IO that is synchronous */
+	RA_ACCOUNT_MMAP,	/* readahead IO by mmap page faults */
+	RA_ACCOUNT_METADATA,	/* readahead IO on metadata */
+	/* number of readahead pages */
+	RA_ACCOUNT_SIZE,	/* readahead size */
+	RA_ACCOUNT_ASYNC_SIZE,	/* readahead async size */
+	RA_ACCOUNT_ACTUAL,	/* readahead actual IO size */
+	/* end mark */
+	RA_ACCOUNT_MAX,
+};
+
+static unsigned long ra_stats[RA_PATTERN_MAX][RA_ACCOUNT_MAX];
+
+static void readahead_stats(struct address_space *mapping,
+			    pgoff_t offset,
+			    unsigned long req_size,
+			    bool for_mmap,
+			    bool for_metadata,
+			    enum readahead_pattern pattern,
+			    pgoff_t start,
+			    unsigned long size,
+			    unsigned long async_size,
+			    int actual)
+{
+	pgoff_t eof = ((i_size_read(mapping->host)-1) >> PAGE_CACHE_SHIFT) + 1;
+
+recount:
+	ra_stats[pattern][RA_ACCOUNT_COUNT]++;
+	ra_stats[pattern][RA_ACCOUNT_SIZE] += size;
+	ra_stats[pattern][RA_ACCOUNT_ASYNC_SIZE] += async_size;
+	ra_stats[pattern][RA_ACCOUNT_ACTUAL] += actual;
+
+	if (start + size >= eof)
+		ra_stats[pattern][RA_ACCOUNT_EOF]++;
+	if (actual < size)
+		ra_stats[pattern][RA_ACCOUNT_CACHE_HIT]++;
+
+	if (actual) {
+		ra_stats[pattern][RA_ACCOUNT_IOCOUNT]++;
+
+		if (start <= offset && offset < start + size)
+			ra_stats[pattern][RA_ACCOUNT_SYNC]++;
+
+		if (for_mmap)
+			ra_stats[pattern][RA_ACCOUNT_MMAP]++;
+		if (for_metadata)
+			ra_stats[pattern][RA_ACCOUNT_METADATA]++;
+	}
+
+	if (pattern != RA_PATTERN_ALL) {
+		pattern = RA_PATTERN_ALL;
+		goto recount;
+	}
+}
+
+static int readahead_stats_show(struct seq_file *s, void *_)
+{
+	unsigned long i;
+
+	seq_printf(s,
+		   "%-10s %10s %10s %10s %10s %10s %10s %10s %10s %10s %10s\n",
+		   "pattern", "readahead", "eof_hit", "cache_hit",
+		   "io", "sync_io", "mmap_io", "meta_io",
+		   "size", "async_size", "io_size");
+
+	for (i = 0; i < RA_PATTERN_MAX; i++) {
+		unsigned long count = ra_stats[i][RA_ACCOUNT_COUNT];
+		unsigned long iocount = ra_stats[i][RA_ACCOUNT_IOCOUNT];
+		/*
+		 * avoid division-by-zero
+		 */
+		if (count == 0)
+			count = 1;
+		if (iocount == 0)
+			iocount = 1;
+
+		seq_printf(s, "%-10s %10lu %10lu %10lu %10lu %10lu "
+			   "%10lu %10lu %10lu %10lu %10lu\n",
+				ra_pattern_names[i],
+				ra_stats[i][RA_ACCOUNT_COUNT],
+				ra_stats[i][RA_ACCOUNT_EOF],
+				ra_stats[i][RA_ACCOUNT_CACHE_HIT],
+				ra_stats[i][RA_ACCOUNT_IOCOUNT],
+				ra_stats[i][RA_ACCOUNT_SYNC],
+				ra_stats[i][RA_ACCOUNT_MMAP],
+				ra_stats[i][RA_ACCOUNT_METADATA],
+				ra_stats[i][RA_ACCOUNT_SIZE] / count,
+				ra_stats[i][RA_ACCOUNT_ASYNC_SIZE] / count,
+				ra_stats[i][RA_ACCOUNT_ACTUAL] / iocount);
+	}
+
+	return 0;
+}
+
+static int readahead_stats_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, readahead_stats_show, NULL);
+}
+
+static ssize_t readahead_stats_write(struct file *file, const char __user *buf,
+				     size_t size, loff_t *offset)
+{
+	memset(ra_stats, 0, sizeof(ra_stats));
+	return size;
+}
+
+static const struct file_operations readahead_stats_fops = {
+	.owner		= THIS_MODULE,
+	.open		= readahead_stats_open,
+	.write		= readahead_stats_write,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static int __init readahead_create_debugfs(void)
+{
+	struct dentry *root;
+	struct dentry *entry;
+
+	root = debugfs_create_dir("readahead", NULL);
+	if (!root)
+		goto out;
+
+	entry = debugfs_create_file("stats", 0644, root,
+				    NULL, &readahead_stats_fops);
+	if (!entry)
+		goto out;
+
+	entry = debugfs_create_bool("stats_enable", 0644, root,
+				    &readahead_stats_enable);
+	if (!entry)
+		goto out;
+
+	return 0;
+out:
+	printk(KERN_ERR "readahead: failed to create debugfs entries\n");
+	return -ENOMEM;
+}
+
+late_initcall(readahead_create_debugfs);
+#endif
+
+static inline void readahead_event(struct address_space *mapping,
+				   pgoff_t offset,
+				   unsigned long req_size,
+				   bool for_mmap,
+				   bool for_metadata,
+				   enum readahead_pattern pattern,
+				   pgoff_t start,
+				   unsigned long size,
+				   unsigned long async_size,
+				   int actual)
+{
+#ifdef CONFIG_READAHEAD_STATS
+	if (readahead_stats_enable)
+		readahead_stats(mapping, offset, req_size,
+				for_mmap, for_metadata,
+				pattern, start, size, async_size, actual);
+#endif
+}
+
+
 /*
  * see if a page needs releasing upon read_cache_pages() failure
  * - the caller of read_cache_pages() may have set PG_private or PG_fscache

@@ -228,6 +414,9 @@ int force_page_cache_readahead(struct ad
 			ret = err;
 			break;
 		}
+		readahead_event(mapping, offset, nr_to_read, 0, 0,
+				RA_PATTERN_FADVISE, offset, this_chunk, 0,
+				err);
 		ret += err;
 		offset += this_chunk;
 		nr_to_read -= this_chunk;

@@ -267,6 +456,11 @@ unsigned long ra_submit(struct file_ra_s
 	actual = __do_page_cache_readahead(mapping, filp,
 					ra->start, ra->size, ra->async_size);
 
+	readahead_event(mapping, offset, req_size,
+			ra->for_mmap, ra->for_metadata,
+			ra->pattern, ra->start, ra->size, ra->async_size,
+			actual);
+
 	ra->for_mmap = 0;
 	ra->for_metadata = 0;
 	return actual;

mm/Kconfig +15 -1

@@ -373,3 +373,18 @@ config CLEANCACHE
 	  in a negligible performance hit.
 
 	  If unsure, say Y to enable cleancache
+
+config READAHEAD_STATS
+	bool "Collect page cache readahead stats"
+	depends on DEBUG_FS
+	default y
+	help
+	  This provides the readahead events accounting facilities.
+
+	  To do readahead accounting for a workload:
+
+	  echo 1 > /sys/kernel/debug/readahead/stats_enable
+	  echo 0 > /sys/kernel/debug/readahead/stats  # reset counters
+	  # run the workload
+	  cat /sys/kernel/debug/readahead/stats       # check counters
+	  echo 0 > /sys/kernel/debug/readahead/stats_enable
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

This looks all inherently racy (which doesn't matter much as you suggest)
so I just wanted to suggest that if you used per-cpu counters you'd get race-free and faster code at the cost of larger data structures and using percpu_counter_add() instead of ++ (which doesn't seem like a big complication to me).

This looks all inherently racy (which doesn't matter much as you suggest)
so I just wanted to suggest that if you used per-cpu counters you'd get race-free and faster code at the cost of larger data structures and using percpu_counter_add() instead of ++ (which doesn't seem like a big complication to me).

Quoted 5 lines

This looks all inherently racy (which doesn't matter much as you suggest)
so I just wanted to suggest that if you used per-cpu counters you'd get race-free and faster code at the cost of larger data structures and using percpu_counter_add() instead of ++ (which doesn't seem like a big complication to me).

OK, here is the incremental patch to use per-cpu counters :)

---
 mm/readahead.c |   61 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 17 deletions(-)

mm/readahead.c +44 -17

@@ -68,7 +68,7 @@ enum ra_account {
 	RA_ACCOUNT_MAX,
 };
 
-static unsigned long ra_stats[RA_PATTERN_MAX][RA_ACCOUNT_MAX];
+static DEFINE_PER_CPU(unsigned long[RA_PATTERN_ALL][RA_ACCOUNT_MAX], ra_stat);
 
 static void readahead_stats(struct address_space *mapping,
 			    pgoff_t offset,

@@ -83,38 +83,62 @@ static void readahead_stats(struct addre
 {
 	pgoff_t eof = ((i_size_read(mapping->host)-1) >> PAGE_CACHE_SHIFT) + 1;
 
-recount:
-	ra_stats[pattern][RA_ACCOUNT_COUNT]++;
-	ra_stats[pattern][RA_ACCOUNT_SIZE] += size;
-	ra_stats[pattern][RA_ACCOUNT_ASYNC_SIZE] += async_size;
-	ra_stats[pattern][RA_ACCOUNT_ACTUAL] += actual;
+	preempt_disable();
+
+	__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_COUNT]);
+	__this_cpu_add(ra_stat[pattern][RA_ACCOUNT_SIZE], size);
+	__this_cpu_add(ra_stat[pattern][RA_ACCOUNT_ASYNC_SIZE], async_size);
+	__this_cpu_add(ra_stat[pattern][RA_ACCOUNT_ACTUAL], actual);
 
 	if (start + size >= eof)
-		ra_stats[pattern][RA_ACCOUNT_EOF]++;
+		__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_EOF]);
 	if (actual < size)
-		ra_stats[pattern][RA_ACCOUNT_CACHE_HIT]++;
+		__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_CACHE_HIT]);
 
 	if (actual) {
-		ra_stats[pattern][RA_ACCOUNT_IOCOUNT]++;
+		__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_IOCOUNT]);
 
 		if (start <= offset && offset < start + size)
-			ra_stats[pattern][RA_ACCOUNT_SYNC]++;
+			__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_SYNC]);
 
 		if (for_mmap)
-			ra_stats[pattern][RA_ACCOUNT_MMAP]++;
+			__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_MMAP]);
 		if (for_metadata)
-			ra_stats[pattern][RA_ACCOUNT_METADATA]++;
+			__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_METADATA]);
 	}
 
-	if (pattern != RA_PATTERN_ALL) {
-		pattern = RA_PATTERN_ALL;
-		goto recount;
-	}
+	preempt_enable();
+}
+
+static void ra_stats_clear(void)
+{
+	int cpu;
+	int i, j;
+
+	for_each_online_cpu(cpu)
+		for (i = 0; i < RA_PATTERN_ALL; i++)
+			for (j = 0; j < RA_ACCOUNT_MAX; j++)
+				per_cpu(ra_stat[i][j], cpu) = 0;
+}
+
+static void ra_stats_sum(unsigned long ra_stats[RA_PATTERN_MAX][RA_ACCOUNT_MAX])
+{
+	int cpu;
+	int i, j;
+
+	for_each_online_cpu(cpu)
+		for (i = 0; i < RA_PATTERN_ALL; i++)
+			for (j = 0; j < RA_ACCOUNT_MAX; j++) {
+				unsigned long n = per_cpu(ra_stat[i][j], cpu);
+				ra_stats[i][j] += n;
+				ra_stats[RA_PATTERN_ALL][j] += n;
+			}
 }
 
 static int readahead_stats_show(struct seq_file *s, void *_)
 {
 	unsigned long i;
+	unsigned long ra_stats[RA_PATTERN_MAX][RA_ACCOUNT_MAX];
 
 	seq_printf(s,
 		   "%-10s %10s %10s %10s %10s %10s %10s %10s %10s %10s %10s\n",

@@ -122,6 +146,9 @@ static int readahead_stats_show(struct s
 		   "io", "sync_io", "mmap_io", "meta_io",
 		   "size", "async_size", "io_size");
 
+	memset(ra_stats, 0, sizeof(ra_stats));
+	ra_stats_sum(ra_stats);
+
 	for (i = 0; i < RA_PATTERN_MAX; i++) {
 		unsigned long count = ra_stats[i][RA_ACCOUNT_COUNT];
 		unsigned long iocount = ra_stats[i][RA_ACCOUNT_IOCOUNT];

@@ -159,7 +186,7 @@ static int readahead_stats_open(struct i
 static ssize_t readahead_stats_write(struct file *file, const char __user *buf,
 				     size_t size, loff_t *offset)
 {
-	memset(ra_stats, 0, sizeof(ra_stats));
+	ra_stats_clear();
 	return size;
 }
 

Quoted 6 lines · Wu Fengguang

Quoted 5 lines

This looks all inherently racy (which doesn't matter much as you suggest)
so I just wanted to suggest that if you used per-cpu counters you'd get race-free and faster code at the cost of larger data structures and using percpu_counter_add() instead of ++ (which doesn't seem like a big complication to me).

Quoted 1 line

OK, here is the incremental patch to use per-cpu counters :)

Thanks! This looks better. I just thought you would use per-cpu counters
as defined in include/linux/percpu_counter.h and are used e.g. by bdi stats. This is more standard for statistics in the kernel than using per-cpu variables directly.

Quoted 7 lines · Jan Kara

Quoted 6 lines · Wu Fengguang

Quoted 5 lines

This looks all inherently racy (which doesn't matter much as you suggest)
so I just wanted to suggest that if you used per-cpu counters you'd get race-free and faster code at the cost of larger data structures and using percpu_counter_add() instead of ++ (which doesn't seem like a big complication to me).

Quoted 5 lines

Quoted 1 line

OK, here is the incremental patch to use per-cpu counters :)

Thanks! This looks better. I just thought you would use per-cpu counters
as defined in include/linux/percpu_counter.h and are used e.g. by bdi stats. This is more standard for statistics in the kernel than using per-cpu variables directly.

Ah yes, I overlooked that facility! However the percpu_counter's ability to maintain and quickly retrieve the global value seems unnecessary feature/overheads for readahead stats, because here we only need to sum up the global value when the user requests it. If switching to percpu_counter, I'm afraid every readahead(1MB) event will lead to the update of percpu_counter global value (grabbing the spinlock) due to 1MB > some small batch size. This actually performs worse than the plain global array of values in the v1 patch.

Quoted 14 lines · Wu Fengguang

Quoted 7 lines · Jan Kara

Quoted 6 lines · Wu Fengguang

Quoted 5 lines

This looks all inherently racy (which doesn't matter much as you suggest)
so I just wanted to suggest that if you used per-cpu counters you'd get race-free and faster code at the cost of larger data structures and using percpu_counter_add() instead of ++ (which doesn't seem like a big complication to me).

Quoted 5 lines

Quoted 1 line

OK, here is the incremental patch to use per-cpu counters :)

Thanks! This looks better. I just thought you would use per-cpu counters
as defined in include/linux/percpu_counter.h and are used e.g. by bdi stats. This is more standard for statistics in the kernel than using per-cpu variables directly.

Quoted 8 lines

Ah yes, I overlooked that facility! However the percpu_counter's ability to maintain and quickly retrieve the global value seems unnecessary feature/overheads for readahead stats, because here we only need to sum up the global value when the user requests it. If switching to percpu_counter, I'm afraid every readahead(1MB) event will lead to the update of percpu_counter global value (grabbing the spinlock) due to 1MB > some small batch size. This actually performs worse than the plain global array of values in the v1 patch.

So use a custom batch size so that typical increments don't require locking for every add. The bdi stat counters are an example of this sort of setup to reduce lock contention on typical IO workloads as concurrency increases.

All these stats have is a requirement for a different batch size to avoid frequent lock grabs. The stats don't have to update the global counter very often (only to prvent overflow!) so you count get away with a batch size in the order of 2^30 without any issues....

We have a general per-cpu counter infrastructure - we should be using it and improving it and not reinventing it a different way every time we need a per-cpu counter.

Quoted 34 lines · Dave Chinner

Quoted 14 lines · Wu Fengguang

Quoted 7 lines · Jan Kara

Quoted 6 lines · Wu Fengguang

Quoted 5 lines

This looks all inherently racy (which doesn't matter much as you suggest)
so I just wanted to suggest that if you used per-cpu counters you'd get race-free and faster code at the cost of larger data structures and using percpu_counter_add() instead of ++ (which doesn't seem like a big complication to me).

Quoted 5 lines

Quoted 1 line

OK, here is the incremental patch to use per-cpu counters :)

Thanks! This looks better. I just thought you would use per-cpu counters
as defined in include/linux/percpu_counter.h and are used e.g. by bdi stats. This is more standard for statistics in the kernel than using per-cpu variables directly.

Quoted 8 lines

Ah yes, I overlooked that facility! However the percpu_counter's ability to maintain and quickly retrieve the global value seems unnecessary feature/overheads for readahead stats, because here we only need to sum up the global value when the user requests it. If switching to percpu_counter, I'm afraid every readahead(1MB) event will lead to the update of percpu_counter global value (grabbing the spinlock) due to 1MB > some small batch size. This actually performs worse than the plain global array of values in the v1 patch.

So use a custom batch size so that typical increments don't require locking for every add. The bdi stat counters are an example of this sort of setup to reduce lock contention on typical IO workloads as concurrency increases.

All these stats have is a requirement for a different batch size to avoid frequent lock grabs. The stats don't have to update the global counter very often (only to prvent overflow!) so you count get away with a batch size in the order of 2^30 without any issues....

Quoted 3 lines

We have a general per-cpu counter infrastructure - we should be using it and improving it and not reinventing it a different way every time we need a per-cpu counter.

OK, let's try using percpu_counter, with a huge batch size.

It actually adds both code size and runtime overheads slightly. Are you sure you like this incremental patch?

---
 mm/readahead.c |   74 ++++++++++++++++++++++++++---------------------
 1 file changed, 41 insertions(+), 33 deletions(-)

mm/readahead.c +37 -33

@@ -61,7 +61,18 @@ enum ra_account {
 	RA_ACCOUNT_MAX,
 };
 
-static DEFINE_PER_CPU(unsigned long[RA_PATTERN_ALL][RA_ACCOUNT_MAX], ra_stat);
+#define RA_STAT_BATCH	(INT_MAX / 2)
+static struct percpu_counter ra_stat[RA_PATTERN_ALL][RA_ACCOUNT_MAX];
+
+static inline void add_ra_stat(int i, int j, s64 amount)
+{
+	__percpu_counter_add(&ra_stat[i][j], amount, RA_STAT_BATCH);
+}
+
+static inline void inc_ra_stat(int i, int j)
+{
+	add_ra_stat(i, j, 1);
+}
 
 static void readahead_stats(struct address_space *mapping,
 			    pgoff_t offset,

@@ -76,62 +87,54 @@ static void readahead_stats(struct addre
 {
 	pgoff_t eof = ((i_size_read(mapping->host)-1) >> PAGE_CACHE_SHIFT) + 1;
 
-	preempt_disable();
-
-	__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_COUNT]);
-	__this_cpu_add(ra_stat[pattern][RA_ACCOUNT_SIZE], size);
-	__this_cpu_add(ra_stat[pattern][RA_ACCOUNT_ASYNC_SIZE], async_size);
-	__this_cpu_add(ra_stat[pattern][RA_ACCOUNT_ACTUAL], actual);
+	inc_ra_stat(pattern, RA_ACCOUNT_COUNT);
+	add_ra_stat(pattern, RA_ACCOUNT_SIZE, size);
+	add_ra_stat(pattern, RA_ACCOUNT_ASYNC_SIZE, async_size);
+	add_ra_stat(pattern, RA_ACCOUNT_ACTUAL, actual);
 
 	if (start + size >= eof)
-		__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_EOF]);
+		inc_ra_stat(pattern, RA_ACCOUNT_EOF);
 	if (actual < size)
-		__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_CACHE_HIT]);
+		inc_ra_stat(pattern, RA_ACCOUNT_CACHE_HIT);
 
 	if (actual) {
-		__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_IOCOUNT]);
+		inc_ra_stat(pattern, RA_ACCOUNT_IOCOUNT);
 
 		if (start <= offset && offset < start + size)
-			__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_SYNC]);
+			inc_ra_stat(pattern, RA_ACCOUNT_SYNC);
 
 		if (for_mmap)
-			__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_MMAP]);
+			inc_ra_stat(pattern, RA_ACCOUNT_MMAP);
 		if (for_metadata)
-			__this_cpu_inc(ra_stat[pattern][RA_ACCOUNT_METADATA]);
+			inc_ra_stat(pattern, RA_ACCOUNT_METADATA);
 	}
-
-	preempt_enable();
 }
 
 static void ra_stats_clear(void)
 {
-	int cpu;
 	int i, j;
 
-	for_each_online_cpu(cpu)
-		for (i = 0; i < RA_PATTERN_ALL; i++)
-			for (j = 0; j < RA_ACCOUNT_MAX; j++)
-				per_cpu(ra_stat[i][j], cpu) = 0;
+	for (i = 0; i < RA_PATTERN_ALL; i++)
+		for (j = 0; j < RA_ACCOUNT_MAX; j++)
+			percpu_counter_set(&ra_stat[i][j], 0);
 }
 
-static void ra_stats_sum(unsigned long ra_stats[RA_PATTERN_MAX][RA_ACCOUNT_MAX])
+static void ra_stats_sum(long long ra_stats[RA_PATTERN_MAX][RA_ACCOUNT_MAX])
 {
-	int cpu;
 	int i, j;
 
-	for_each_online_cpu(cpu)
-		for (i = 0; i < RA_PATTERN_ALL; i++)
-			for (j = 0; j < RA_ACCOUNT_MAX; j++) {
-				unsigned long n = per_cpu(ra_stat[i][j], cpu);
-				ra_stats[i][j] += n;
-				ra_stats[RA_PATTERN_ALL][j] += n;
-			}
+	for (i = 0; i < RA_PATTERN_ALL; i++)
+		for (j = 0; j < RA_ACCOUNT_MAX; j++) {
+			s64 n = percpu_counter_sum(&ra_stat[i][j]);
+			ra_stats[i][j] += n;
+			ra_stats[RA_PATTERN_ALL][j] += n;
+		}
 }
 
 static int readahead_stats_show(struct seq_file *s, void *_)
 {
-	unsigned long i;
-	unsigned long ra_stats[RA_PATTERN_MAX][RA_ACCOUNT_MAX];
+	long long ra_stats[RA_PATTERN_MAX][RA_ACCOUNT_MAX];
+	int i;
 
 	seq_printf(s,
 		   "%-10s %10s %10s %10s %10s %10s %10s %10s %10s %10s %10s\n",

@@ -153,8 +156,8 @@ static int readahead_stats_show(struct s
 		if (iocount == 0)
 			iocount = 1;
 
-		seq_printf(s, "%-10s %10lu %10lu %10lu %10lu %10lu "
-			   "%10lu %10lu %10lu %10lu %10lu\n",
+		seq_printf(s, "%-10s %10lld %10lld %10lld %10lld %10lld "
+			   "%10lld %10lld %10lld %10lld %10lld\n",
 				ra_pattern_names[i].name,
 				ra_stats[i][RA_ACCOUNT_COUNT],
 				ra_stats[i][RA_ACCOUNT_EOF],

@@ -196,6 +199,7 @@ static int __init readahead_create_debug
 {
 	struct dentry *root;
 	struct dentry *entry;
+	int i, j;
 
 	root = debugfs_create_dir("readahead", NULL);
 	if (!root)

This is very useful for verifying whether the readahead algorithms are working to the expectation.

Example output:

# echo 1 > /debug/tracing/events/vfs/readahead/enable
# cp test-file /dev/null
# cat /debug/tracing/trace  # trimmed output
readahead-initial(dev=0:15, ino=100177, req=0+2, ra=0+4-2, async=0) = 4
readahead-subsequent(dev=0:15, ino=100177, req=2+2, ra=4+8-8, async=1) = 8
readahead-subsequent(dev=0:15, ino=100177, req=4+2, ra=12+16-16, async=1) = 16
readahead-subsequent(dev=0:15, ino=100177, req=12+2, ra=28+32-32, async=1) = 32
readahead-subsequent(dev=0:15, ino=100177, req=28+2, ra=60+60-60, async=1) = 24
readahead-subsequent(dev=0:15, ino=100177, req=60+2, ra=120+60-60, async=1) = 0

CC Ingo Molnar <[email protected]>

CC Jens Axboe <[email protected]>

CC Steven Rostedt <[email protected]>

CC Peter Zijlstra <[email protected]>

Acked-by Rik van Riel <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 include/trace/events/vfs.h |   64 +++++++++++++++++++++++++++++++++++
 mm/readahead.c             |    5 ++
 2 files changed, 69 insertions(+)

include/trace/events/vfs.h new file +64 -0

@@ -0,0 +1,64 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM vfs
+
+#if !defined(_TRACE_VFS_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_VFS_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(readahead,
+	TP_PROTO(struct address_space *mapping,
+		 pgoff_t offset,
+		 unsigned long req_size,
+		 enum readahead_pattern pattern,
+		 pgoff_t start,
+		 unsigned long size,
+		 unsigned long async_size,
+		 unsigned int actual),
+
+	TP_ARGS(mapping, offset, req_size, pattern, start, size, async_size,
+		actual),
+
+	TP_STRUCT__entry(
+		__field(	dev_t,		dev		)
+		__field(	ino_t,		ino		)
+		__field(	pgoff_t,	offset		)
+		__field(	unsigned long,	req_size	)
+		__field(	unsigned int,	pattern		)
+		__field(	pgoff_t,	start		)
+		__field(	unsigned int,	size		)
+		__field(	unsigned int,	async_size	)
+		__field(	unsigned int,	actual		)
+	),
+
+	TP_fast_assign(
+		__entry->dev		= mapping->host->i_sb->s_dev;
+		__entry->ino		= mapping->host->i_ino;
+		__entry->offset		= offset;
+		__entry->req_size	= req_size;
+		__entry->pattern	= pattern;
+		__entry->start		= start;
+		__entry->size		= size;
+		__entry->async_size	= async_size;
+		__entry->actual		= actual;
+	),
+
+	TP_printk("readahead-%s(dev=%d:%d, ino=%lu, "
+		  "req=%lu+%lu, ra=%lu+%d-%d, async=%d) = %d",
+			ra_pattern_names[__entry->pattern],
+			MAJOR(__entry->dev),
+			MINOR(__entry->dev),
+			__entry->ino,
+			__entry->offset,
+			__entry->req_size,
+			__entry->start,
+			__entry->size,
+			__entry->async_size,
+			__entry->start > __entry->offset,
+			__entry->actual)
+);
+
+#endif /* _TRACE_VFS_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>

mm/readahead.c +5 -1

@@ -29,6 +29,9 @@ static const char * const ra_pattern_nam
 	[RA_PATTERN_ALL]                = "all",
 };
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/vfs.h>
+
 /*
  * Initialise a struct file's readahead state.  Assumes that the caller has
  * memset *ra to zero.

@@ -215,6 +218,8 @@ static inline void readahead_event(struc
 				for_mmap, for_metadata,
 				pattern, start, size, async_size, actual);
 #endif
+	trace_readahead(mapping, offset, req_size,
+			pattern, start, size, async_size, actual);
 }
 
 
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 15 lines · Wu Fengguang

This is very useful for verifying whether the readahead algorithms are working to the expectation.

Example output:

# echo 1 > /debug/tracing/events/vfs/readahead/enable
# cp test-file /dev/null
# cat /debug/tracing/trace  # trimmed output
readahead-initial(dev=0:15, ino=100177, req=0+2, ra=0+4-2, async=0) = 4
readahead-subsequent(dev=0:15, ino=100177, req=2+2, ra=4+8-8, async=1) = 8
readahead-subsequent(dev=0:15, ino=100177, req=4+2, ra=12+16-16, async=1) = 16
readahead-subsequent(dev=0:15, ino=100177, req=12+2, ra=28+32-32, async=1) = 32
readahead-subsequent(dev=0:15, ino=100177, req=28+2, ra=60+60-60, async=1) = 24
readahead-subsequent(dev=0:15, ino=100177, req=60+2, ra=120+60-60, async=1) = 0

Quoted 6 lines

CC Ingo Molnar <[email protected]>

CC Jens Axboe <[email protected]>

CC Steven Rostedt <[email protected]>

CC Peter Zijlstra <[email protected]>

Acked-by Rik van Riel <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

Looks OK.

Acked-by Jan Kara <[email protected]>

include/trace/events/vfs.h line 50

Quoted 5 lines

include/trace/events/vfs.h new file +5 -0

+	TP_printk("readahead-%s(dev=%d:%d, ino=%lu, "
+		  "req=%lu+%lu, ra=%lu+%d-%d, async=%d) = %d",
+			ra_pattern_names[__entry->pattern],
+			MAJOR(__entry->dev),
+			MINOR(__entry->dev),

Quoted 22 lines · Jan Kara

Quoted 15 lines · Wu Fengguang

This is very useful for verifying whether the readahead algorithms are working to the expectation.

Example output:

# echo 1 > /debug/tracing/events/vfs/readahead/enable
# cp test-file /dev/null
# cat /debug/tracing/trace  # trimmed output
readahead-initial(dev=0:15, ino=100177, req=0+2, ra=0+4-2, async=0) = 4
readahead-subsequent(dev=0:15, ino=100177, req=2+2, ra=4+8-8, async=1) = 8
readahead-subsequent(dev=0:15, ino=100177, req=4+2, ra=12+16-16, async=1) = 16
readahead-subsequent(dev=0:15, ino=100177, req=12+2, ra=28+32-32, async=1) = 32
readahead-subsequent(dev=0:15, ino=100177, req=28+2, ra=60+60-60, async=1) = 24
readahead-subsequent(dev=0:15, ino=100177, req=60+2, ra=120+60-60, async=1) = 0

Quoted 6 lines

CC Ingo Molnar <[email protected]>

CC Jens Axboe <[email protected]>

CC Steven Rostedt <[email protected]>

CC Peter Zijlstra <[email protected]>

Acked-by Rik van Riel <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

Quoted 3 lines

Looks OK.

Acked-by Jan Kara <[email protected]>

Thank you.

Quoted 5 lines

include/trace/events/vfs.h line 50

Quoted 5 lines

include/trace/events/vfs.h new file +5 -0

+	TP_printk("readahead-%s(dev=%d:%d, ino=%lu, "
+		  "req=%lu+%lu, ra=%lu+%d-%d, async=%d) = %d",
+			ra_pattern_names[__entry->pattern],
+			MAJOR(__entry->dev),
+			MINOR(__entry->dev),

One thing I'm not certain is the dev=MAJOR:MINOR. The other option used in many trace events are bdi=BDI_NAME_OR_NUMBER. Will bdi be more suitable here?

Quoted 29 lines · Wu Fengguang

Quoted 22 lines · Jan Kara

Quoted 15 lines · Wu Fengguang

This is very useful for verifying whether the readahead algorithms are working to the expectation.

Example output:

# echo 1 > /debug/tracing/events/vfs/readahead/enable
# cp test-file /dev/null
# cat /debug/tracing/trace  # trimmed output
readahead-initial(dev=0:15, ino=100177, req=0+2, ra=0+4-2, async=0) = 4
readahead-subsequent(dev=0:15, ino=100177, req=2+2, ra=4+8-8, async=1) = 8
readahead-subsequent(dev=0:15, ino=100177, req=4+2, ra=12+16-16, async=1) = 16
readahead-subsequent(dev=0:15, ino=100177, req=12+2, ra=28+32-32, async=1) = 32
readahead-subsequent(dev=0:15, ino=100177, req=28+2, ra=60+60-60, async=1) = 24
readahead-subsequent(dev=0:15, ino=100177, req=60+2, ra=120+60-60, async=1) = 0

Quoted 6 lines

CC Ingo Molnar <[email protected]>

CC Jens Axboe <[email protected]>

CC Steven Rostedt <[email protected]>

CC Peter Zijlstra <[email protected]>

Acked-by Rik van Riel <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

Quoted 3 lines

Looks OK.

Acked-by Jan Kara <[email protected]>

Thank you.

Quoted 9 lines

Quoted 5 lines

include/trace/events/vfs.h line 50

Quoted 5 lines

include/trace/events/vfs.h new file +5 -0

+	TP_printk("readahead-%s(dev=%d:%d, ino=%lu, "
+		  "req=%lu+%lu, ra=%lu+%d-%d, async=%d) = %d",
+			ra_pattern_names[__entry->pattern],
+			MAJOR(__entry->dev),
+			MINOR(__entry->dev),

One thing I'm not certain is the dev=MAJOR:MINOR. The other option used in many trace events are bdi=BDI_NAME_OR_NUMBER. Will bdi be more suitable here?

Probably bdi name will be more consistent (e.g. with writeback) but I
don't think it makes a big difference in practice.

include/trace/events/vfs.h line 46

Quoted 1 line

include/trace/events/vfs.h new file +1 -0

+	TP_printk("readahead-%s(dev=%d:%d, ino=%lu, "

please don't duplicate the tracepoint name in the output string. Also don't use braces, as it jsut complicates parsing.

include/trace/events/vfs.h line 48

Quoted 2 lines

include/trace/events/vfs.h new file +2 -0

+		  "req=%lu+%lu, ra=%lu+%d-%d, async=%d) = %d",
+			ra_pattern_names[__entry->pattern],

Instead of doing a manual array lookup please use __print_symbolic so that users of the binary interface (like trace-cmd) also get the right output.

mm/readahead.c static const char * const ra_pattern_nam · line 33

Quoted 8 lines

mm/readahead.c +2 -0

@@ -29,6 +29,9 @@ static const char * const ra_pattern_nam
 	[RA_PATTERN_ALL]                = "all",
 };
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/vfs.h>

Maybe we should create a new fs/trace.c just for this instead of stickin it into the first file that created a tracepoint in the "vfs" namespace.

Review

Quoted 2 lines

(quoted diff) +2 -0

+		  "req=%lu+%lu, ra=%lu+%d-%d, async=%d) = %d",
+			ra_pattern_names[__entry->pattern],

Instead of doing a manual array lookup please use __print_symbolic so that users of the binary interface (like trace-cmd) also get the right output.

Add the backwards prefetching feature. It's pretty simple if we don't support async prefetching and interleaved reads.

Here is the behavior with an 8-page read sequence from 10000 down to 0. (The readahead size is a bit large since it's an NFS mount.)

readahead-random(dev=0:16, ino=3948605, req=10000+8, ra=10000+8-0, async=0) = 8 readahead-backwards(dev=0:16, ino=3948605, req=9992+8, ra=9968+32-0, async=0) = 32 readahead-backwards(dev=0:16, ino=3948605, req=9960+8, ra=9840+128-0, async=0) = 128 readahead-backwards(dev=0:16, ino=3948605, req=9832+8, ra=9584+256-0, async=0) = 256 readahead-backwards(dev=0:16, ino=3948605, req=9576+8, ra=9072+512-0, async=0) = 512 readahead-backwards(dev=0:16, ino=3948605, req=9064+8, ra=8048+1024-0, async=0) = 1024 readahead-backwards(dev=0:16, ino=3948605, req=8040+8, ra=6128+1920-0, async=0) = 1920 readahead-backwards(dev=0:16, ino=3948605, req=6120+8, ra=4208+1920-0, async=0) = 1920 readahead-backwards(dev=0:16, ino=3948605, req=4200+8, ra=2288+1920-0, async=0) = 1920 readahead-backwards(dev=0:16, ino=3948605, req=2280+8, ra=368+1920-0, async=0) = 1920 readahead-backwards(dev=0:16, ino=3948605, req=360+8, ra=0+368-0, async=0) = 368

And a simple 1-page read sequence from 10000 down to 0.

readahead-random(dev=0:16, ino=3948605, req=10000+1, ra=10000+1-0, async=0) = 1 readahead-backwards(dev=0:16, ino=3948605, req=9999+1, ra=9996+4-0, async=0) = 4 readahead-backwards(dev=0:16, ino=3948605, req=9995+1, ra=9980+16-0, async=0) = 16 readahead-backwards(dev=0:16, ino=3948605, req=9979+1, ra=9916+64-0, async=0) = 64 readahead-backwards(dev=0:16, ino=3948605, req=9915+1, ra=9660+256-0, async=0) = 256 readahead-backwards(dev=0:16, ino=3948605, req=9659+1, ra=9148+512-0, async=0) = 512 readahead-backwards(dev=0:16, ino=3948605, req=9147+1, ra=8124+1024-0, async=0) = 1024 readahead-backwards(dev=0:16, ino=3948605, req=8123+1, ra=6204+1920-0, async=0) = 1920 readahead-backwards(dev=0:16, ino=3948605, req=6203+1, ra=4284+1920-0, async=0) = 1920 readahead-backwards(dev=0:16, ino=3948605, req=4283+1, ra=2364+1920-0, async=0) = 1920 readahead-backwards(dev=0:16, ino=3948605, req=2363+1, ra=444+1920-0, async=0) = 1920 readahead-backwards(dev=0:16, ino=3948605, req=443+1, ra=0+444-0, async=0) = 444

CC Andi Kleen <[email protected]>

CC Li Shaohua <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

include/linux/fs.h +0 -0

include/linux/fs.h +0 -0

@@ -676,6 +677,20 @@ ondemand_readahead(struct address_space 
 	}
 
 	/*

Someone already mentioned this earlier and I don't think I've seen a
response: Do you have a realistic usecase for this? I don't think I've ever seen an application reading file backwards...

Quoted 3 lines · Jan Kara

Someone already mentioned this earlier and I don't think I've seen a
response: Do you have a realistic usecase for this? I don't think I've ever seen an application reading file backwards...

tac, tail -n$large, ...

Quoted 13 lines

mm/readahead.c +9 -0

@@ -676,6 +677,20 @@ ondemand_readahead(struct address_space 
 	}
 
 	/*
+	 * backwards reading
+	 */
+	if (offset < ra->start && offset + req_size >= ra->start) {
+		ra->pattern = RA_PATTERN_BACKWARDS;
+		ra->size = get_next_ra_size(ra, max);
+		max = ra->start;
+		if (ra->size > max)
+			ra->size = max;
+		ra->async_size = 0;

mm/readahead.c ondemand_readahead(struct address_space · line 689

Quoted 1 line

mm/readahead.c +1 -0

+		ra->start -= ra->size;

IMHO much more obvious way to write this is:
ra->size = get_next_ra_size(ra, max);
if (ra->size > ra->start) {
ra->size = ra->start;
ra->start = 0;
} else
ra->start -= ra->size;

(snip)

Quoted 13 lines

Quoted 13 lines

mm/readahead.c +9 -0

@@ -676,6 +677,20 @@ ondemand_readahead(struct address_space 
 	}
 
 	/*
+	 * backwards reading
+	 */
+	if (offset < ra->start && offset + req_size >= ra->start) {
+		ra->pattern = RA_PATTERN_BACKWARDS;
+		ra->size = get_next_ra_size(ra, max);
+		max = ra->start;
+		if (ra->size > max)
+			ra->size = max;
+		ra->async_size = 0;

Quoted 8 lines

mm/readahead.c ondemand_readahead(struct address_space · line 689

Quoted 1 line

mm/readahead.c +1 -0

+		ra->start -= ra->size;

IMHO much more obvious way to write this is:
ra->size = get_next_ra_size(ra, max);
if (ra->size > ra->start) {
ra->size = ra->start;
ra->start = 0;
} else
ra->start -= ra->size;

Good idea! Here is the updated code:

        /*
         * backwards reading
         */
        if (offset < ra->start && offset + req_size >= ra->start) {
                ra->pattern = RA_PATTERN_BACKWARDS;
                ra->size = get_next_ra_size(ra, max);
                if (ra->size > ra->start) {
                        /*
                         * ra->start may be concurrently set to some huge
                         * value, the min() at least avoids submitting huge IO
                         * in this race condition

                         */
                        ra->size = min(ra->start, max);
                        ra->start = 0;
                } else
                        ra->start -= ra->size;
                ra->async_size = 0;
                goto readit;
        }