Define some common used macros for the read-ahead logics.
Signed-off-by: Wu Fengguang <[email protected]>
---
mm/readahead.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)
--- linux-2.6.17-rc4-mm3.orig/mm/readahead.c
+++ linux-2.6.17-rc4-mm3/mm/readahead.c
@@ -5,6 +5,8 @@
*
* 09Apr2002 [email protected]
* Initial version.
+ * 21May2006 Wu Fengguang <[email protected]>
+ * Adaptive read-ahead framework.
*/
#include <linux/kernel.h>
@@ -14,6 +16,14 @@
#include <linux/blkdev.h>
#include <linux/backing-dev.h>
#include <linux/pagevec.h>
+#include <linux/writeback.h>
+#include <linux/nfsd/const.h>
+
+#define PAGES_BYTE(size) (((size) + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT)
+#define PAGES_KB(size) PAGES_BYTE((size)*1024)
+
+#define next_page(pg) (list_entry((pg)->lru.prev, struct page, lru))
+#define prev_page(pg) (list_entry((pg)->lru.next, struct page, lru))
void default_unplug_io_fn(struct backing_dev_info *bdi, struct page *page)
{
@@ -21,7 +31,7 @@ void default_unplug_io_fn(struct backing
EXPORT_SYMBOL(default_unplug_io_fn);
struct backing_dev_info default_backing_dev_info = {
- .ra_pages = (VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE,
+ .ra_pages = PAGES_KB(VM_MAX_READAHEAD),
.state = 0,
.capabilities = BDI_CAP_MAP_COPY,
.unplug_io_fn = default_unplug_io_fn,
@@ -50,7 +60,7 @@ static inline unsigned long get_max_read
static inline unsigned long get_min_readahead(struct file_ra_state *ra)
{
- return (VM_MIN_READAHEAD * 1024) / PAGE_CACHE_SIZE;
+ return PAGES_KB(VM_MIN_READAHEAD);
}
static inline void reset_ahead_window(struct file_ra_state *ra)
--
Wu Fengguang wrote:
>Define some common used macros for the read-ahead logics.
>
>Signed-off-by: Wu Fengguang <[email protected]>
>---
>
> mm/readahead.c | 14 ++++++++++++--
> 1 files changed, 12 insertions(+), 2 deletions(-)
>
>--- linux-2.6.17-rc4-mm3.orig/mm/readahead.c
>+++ linux-2.6.17-rc4-mm3/mm/readahead.c
>@@ -5,6 +5,8 @@
> *
> * 09Apr2002 [email protected]
> * Initial version.
>+ * 21May2006 Wu Fengguang <[email protected]>
>+ * Adaptive read-ahead framework.
> */
>
> #include <linux/kernel.h>
>@@ -14,6 +16,14 @@
> #include <linux/blkdev.h>
> #include <linux/backing-dev.h>
> #include <linux/pagevec.h>
>+#include <linux/writeback.h>
>+#include <linux/nfsd/const.h>
>
How come you're adding these includes?
>+
>+#define PAGES_BYTE(size) (((size) + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT)
>+#define PAGES_KB(size) PAGES_BYTE((size)*1024)
>
Don't really like the names. Don't think they do anything for clarity, but
if you can come up with something better for PAGES_BYTE I might change my
mind ;) (just forget about PAGES_KB - people know what *1024 means)
Also: the replacements are wrong: if you've defined VM_MAX_READAHEAD to be
4095 bytes, you don't want the _actual_ readahead to be 4096 bytes, do you?
It is saying nothing about minimum, so presumably 0 is the correct choice.
>+
>+#define next_page(pg) (list_entry((pg)->lru.prev, struct page, lru))
>+#define prev_page(pg) (list_entry((pg)->lru.next, struct page, lru))
>
Again, it is probably easier just to use the expanded version. Then the
reader can immediately say: ah, the next page on the LRU list (rather
than, maybe, the next page in the pagecache).
>
> void default_unplug_io_fn(struct backing_dev_info *bdi, struct page *page)
> {
>@@ -21,7 +31,7 @@ void default_unplug_io_fn(struct backing
> EXPORT_SYMBOL(default_unplug_io_fn);
>
> struct backing_dev_info default_backing_dev_info = {
>- .ra_pages = (VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE,
>+ .ra_pages = PAGES_KB(VM_MAX_READAHEAD),
> .state = 0,
> .capabilities = BDI_CAP_MAP_COPY,
> .unplug_io_fn = default_unplug_io_fn,
>@@ -50,7 +60,7 @@ static inline unsigned long get_max_read
>
> static inline unsigned long get_min_readahead(struct file_ra_state *ra)
> {
>- return (VM_MIN_READAHEAD * 1024) / PAGE_CACHE_SIZE;
>+ return PAGES_KB(VM_MIN_READAHEAD);
> }
>
> static inline void reset_ahead_window(struct file_ra_state *ra)
>
>--
>
Send instant messages to your online friends http://au.messenger.yahoo.com
On Thu, May 25, 2006 at 03:56:24PM +1000, Nick Piggin wrote:
> >+#include <linux/writeback.h>
> >+#include <linux/nfsd/const.h>
> >
>
> How come you're adding these includes?
For something added in the past and removed later...
> >+#define PAGES_BYTE(size) (((size) + PAGE_CACHE_SIZE - 1) >>
> >PAGE_CACHE_SHIFT)
> >+#define PAGES_KB(size) PAGES_BYTE((size)*1024)
> >
> Don't really like the names. Don't think they do anything for clarity, but
> if you can come up with something better for PAGES_BYTE I might change my
> mind ;) (just forget about PAGES_KB - people know what *1024 means)
No, they are mainly for concision. Don't you think it's cleaner to write
PAGES_KB(VM_MAX_READAHEAD)
than
(VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE
Admittedly the names are somewhat awkward though :)
> Also: the replacements are wrong: if you've defined VM_MAX_READAHEAD to be
> 4095 bytes, you don't want the _actual_ readahead to be 4096 bytes, do you?
> It is saying nothing about minimum, so presumably 0 is the correct choice.
The macros were first introduced exact for this reason ;)
It is rumored that there will be 64K page support, and this macro
helps round up the 16K sized VM_MIN_READAHEAD. The eof_index also
needs rounding up.
> >+#define next_page(pg) (list_entry((pg)->lru.prev, struct page, lru))
> >+#define prev_page(pg) (list_entry((pg)->lru.next, struct page, lru))
> >
>
> Again, it is probably easier just to use the expanded version. Then the
> reader can immediately say: ah, the next page on the LRU list (rather
> than, maybe, the next page in the pagecache).
Ok, will expand it.
Wu
On Thu, May 25, 2006 at 03:56:24PM +1000, Nick Piggin wrote:
> >+#define PAGES_BYTE(size) (((size) + PAGE_CACHE_SIZE - 1) >>
> >PAGE_CACHE_SHIFT)
> >+#define PAGES_KB(size) PAGES_BYTE((size)*1024)
> >
> Don't really like the names. Don't think they do anything for clarity, but
> if you can come up with something better for PAGES_BYTE I might change my
> mind ;) (just forget about PAGES_KB - people know what *1024 means)
>
> Also: the replacements are wrong: if you've defined VM_MAX_READAHEAD to be
> 4095 bytes, you don't want the _actual_ readahead to be 4096 bytes, do you?
> It is saying nothing about minimum, so presumably 0 is the correct choice.
Got an idea, how about these ones:
#define FULL_PAGES(bytes) ((bytes) >> PAGE_CACHE_SHIFT)
#define PARTIAL_PAGES(bytes) (((bytes)+PAGE_CACHE_SIZE-1) >> PAGE_CACHE_SHIFT)
Wu Fengguang <[email protected]> wrote:
>
> On Thu, May 25, 2006 at 03:56:24PM +1000, Nick Piggin wrote:
> > >+#define PAGES_BYTE(size) (((size) + PAGE_CACHE_SIZE - 1) >>
> > >PAGE_CACHE_SHIFT)
> > >+#define PAGES_KB(size) PAGES_BYTE((size)*1024)
> > >
> > Don't really like the names. Don't think they do anything for clarity, but
> > if you can come up with something better for PAGES_BYTE I might change my
> > mind ;) (just forget about PAGES_KB - people know what *1024 means)
> >
> > Also: the replacements are wrong: if you've defined VM_MAX_READAHEAD to be
> > 4095 bytes, you don't want the _actual_ readahead to be 4096 bytes, do you?
> > It is saying nothing about minimum, so presumably 0 is the correct choice.
>
> Got an idea, how about these ones:
>
> #define FULL_PAGES(bytes) ((bytes) >> PAGE_CACHE_SHIFT)
I dunno. We've traditionally open-coded things like this.
> #define PARTIAL_PAGES(bytes) (((bytes)+PAGE_CACHE_SIZE-1) >> PAGE_CACHE_SHIFT)
That's identical to include/linux/kernel.h:DIV_ROUND_UP(), from the gfs2 tree.
Wu Fengguang <[email protected]> wrote:
>
> Define some common used macros for the read-ahead logics.
>
> Signed-off-by: Wu Fengguang <[email protected]>
> ---
>
> mm/readahead.c | 14 ++++++++++++--
> 1 files changed, 12 insertions(+), 2 deletions(-)
>
> --- linux-2.6.17-rc4-mm3.orig/mm/readahead.c
> +++ linux-2.6.17-rc4-mm3/mm/readahead.c
> @@ -5,6 +5,8 @@
> *
> * 09Apr2002 [email protected]
> * Initial version.
> + * 21May2006 Wu Fengguang <[email protected]>
> + * Adaptive read-ahead framework.
> */
>
> #include <linux/kernel.h>
> @@ -14,6 +16,14 @@
> #include <linux/blkdev.h>
> #include <linux/backing-dev.h>
> #include <linux/pagevec.h>
> +#include <linux/writeback.h>
> +#include <linux/nfsd/const.h>
Why on earth are we including that file?
Whatever goodies it contains should be moved into fs.h or mm.h or something.
> +
> +#define PAGES_BYTE(size) (((size) + PAGE_CACHE_SIZE - 1) >> PAGE_CACHE_SHIFT)
> +#define PAGES_KB(size) PAGES_BYTE((size)*1024)
These aren't proving popular.
> +#define next_page(pg) (list_entry((pg)->lru.prev, struct page, lru))
> +#define prev_page(pg) (list_entry((pg)->lru.next, struct page, lru))
hm. Makes sense I guess, but normally we'll be iterating across lists with
the list_for_each*() helpers, so I'm a little surprised that the above are
needed.
Wu Fengguang wrote:
>On Thu, May 25, 2006 at 03:56:24PM +1000, Nick Piggin wrote:
>
>>>+#define PAGES_BYTE(size) (((size) + PAGE_CACHE_SIZE - 1) >>
>>>PAGE_CACHE_SHIFT)
>>>+#define PAGES_KB(size) PAGES_BYTE((size)*1024)
>>>
>>>
>>Don't really like the names. Don't think they do anything for clarity, but
>>if you can come up with something better for PAGES_BYTE I might change my
>>mind ;) (just forget about PAGES_KB - people know what *1024 means)
>>
>
>No, they are mainly for concision. Don't you think it's cleaner to write
> PAGES_KB(VM_MAX_READAHEAD)
>than
> (VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE
>
>
No. Apart from semantics being different (which I'll address below), anybody
with any business looking at this code will immediately know and understand
what the latter line means. Not so for the former.
>Admittedly the names are somewhat awkward though :)
>
>
>>Also: the replacements are wrong: if you've defined VM_MAX_READAHEAD to be
>>4095 bytes, you don't want the _actual_ readahead to be 4096 bytes, do you?
>>It is saying nothing about minimum, so presumably 0 is the correct choice.
>>
>
>The macros were first introduced exact for this reason ;)
>
>It is rumored that there will be 64K page support, and this macro
>helps round up the 16K sized VM_MIN_READAHEAD. The eof_index also
>needs rounding up.
>
But VM_MIN_READAHEAD of course should be rounded up, for the same
reasons I said VM_MAX_READAHEAD should be rounded down.
So OK as a bug fix, but it needs to be in its own patch, not in a "common
macros" one, and sufficiently commented (and preferably outside your core
adaptive readahead code so it can be quickly merged up)
--
Send instant messages to your online friends http://au.messenger.yahoo.com
Hello Nick and Andrew,
Updated the patch as recommended.
Thanks,
Wu
---
readahead-macros-min-max-rapages.patch
---
Subject: readahead: introduce {MIN,MAX}_RA_PAGES
Define two convenient macros for read-ahead:
- MAX_RA_PAGES: rounded down counterpart of VM_MAX_READAHEAD
- MIN_RA_PAGES: rounded _up_ counterpart of VM_MIN_READAHEAD
Note that the rounded _up_ MIN_RA_PAGES will work flawlessly with large
page sizes like 64k.
Signed-off-by: Wu Fengguang <[email protected]>
---
mm/readahead.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)
--- linux-2.6.17-rc4-mm3.orig/mm/readahead.c
+++ linux-2.6.17-rc4-mm3/mm/readahead.c
@@ -17,13 +17,21 @@
#include <linux/backing-dev.h>
#include <linux/pagevec.h>
+/*
+ * Convienent macros for min/max read-ahead pages.
+ * Note that MAX_RA_PAGES is rounded down, while MIN_RA_PAGES is rounded up.
+ * The latter is necessary for systems with large page size(i.e. 64k).
+ */
+#define MAX_RA_PAGES (VM_MAX_READAHEAD*1024 / PAGE_CACHE_SIZE)
+#define MIN_RA_PAGES DIV_ROUND_UP(VM_MIN_READAHEAD*1024, PAGE_CACHE_SIZE)
+
void default_unplug_io_fn(struct backing_dev_info *bdi, struct page *page)
{
}
EXPORT_SYMBOL(default_unplug_io_fn);
struct backing_dev_info default_backing_dev_info = {
- .ra_pages = (VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE,
+ .ra_pages = MAX_RA_PAGES,
.state = 0,
.capabilities = BDI_CAP_MAP_COPY,
.unplug_io_fn = default_unplug_io_fn,
@@ -52,7 +60,7 @@ static inline unsigned long get_max_read
static inline unsigned long get_min_readahead(struct file_ra_state *ra)
{
- return (VM_MIN_READAHEAD * 1024) / PAGE_CACHE_SIZE;
+ return MIN_RA_PAGES;
}
static inline void reset_ahead_window(struct file_ra_state *ra)