Make vm_max_readahead configurable at run-time

Make vm_max_readahead configurable at run-time. Expose a sysctl knob in procfs to change it. This would ensure that new disks added would use this value as their default read_ahead_kb.

int force_page_cache_readahead(struct address_space *mapping, struct file *filp,
pgoff_t offset, unsigned long nr_to_read);
Index: linux-2.6/mm/backing-dev.c
===================================================================

mm/backing-dev.c +1 -1

@@ -18,7 +18,7 @@ EXPORT_SYMBOL(default_unplug_io_fn);
 
 struct backing_dev_info default_backing_dev_info = {
 	.name		= "default",
-	.ra_pages	= VM_MAX_READAHEAD * 1024 / PAGE_CACHE_SIZE,
+	.ra_pages	= INITIAL_VM_MAX_READAHEAD * 1024 / PAGE_CACHE_SIZE,
 	.state		= 0,
 	.capabilities	= BDI_CAP_MAP_COPY,
 	.unplug_io_fn	= default_unplug_io_fn,

Index: linux-2.6/mm/readahead.c
===================================================================

mm/readahead.c +2 -0

@@ -17,6 +17,19 @@
 #include <linux/pagevec.h>
 #include <linux/pagemap.h>
 
+unsigned long vm_max_readahead = INITIAL_VM_MAX_READAHEAD;
+

{ }
};

Index: linux-2.6/Documentation/sysctl/vm.txt
===================================================================

Documentation/sysctl/vm.txt +1 -0

@@ -31,6 +31,7 @@ Currently, these files are in /proc/sys/
 - laptop_mode
 - legacy_va_layout
 - lowmem_reserve_ratio
+- max_readahead_kb
 - max_map_count
 - memory_failure_early_kill
 - memory_failure_recovery

Make vm_max_readahead configurable at run-time. Expose a sysctl knob in procfs to change it. This would ensure that new disks added would use this value as their default read_ahead_kb.

Signed-off-by Nikanth Karthikesan <[email protected]>

fs/fuse/inode.c +0 -0

 	fc->bdi.unplug_io_fn = default_unplug_io_fn;
 	/* fuse does it's own writeback accounting */
 	fc->bdi.capabilities = BDI_CAP_NO_ACCT_WB;

Index: linux-2.6/include/linux/mm.h
===================================================================

include/linux/mm.h +5 -1

@@ -1188,7 +1188,11 @@ int write_one_page(struct page *page, in
 void task_dirty_inc(struct task_struct *tsk);
 
 /* readahead.c */
-#define VM_MAX_READAHEAD	128	/* kbytes */
+#define INITIAL_VM_MAX_READAHEAD	128	/* kbytes */
+extern unsigned long vm_max_readahead;
+
+int sysctl_vm_max_readahead_handler(struct ctl_table *, int,
+					void __user *, size_t *, loff_t *);
 
 int force_page_cache_readahead(struct address_space *mapping, struct file *filp,
 			pgoff_t offset, unsigned long nr_to_read);

Index: linux-2.6/mm/backing-dev.c
===================================================================

mm/backing-dev.c +1 -1

@@ -18,7 +18,7 @@ EXPORT_SYMBOL(default_unplug_io_fn);
 
 struct backing_dev_info default_backing_dev_info = {
 	.name		= "default",
-	.ra_pages	= VM_MAX_READAHEAD * 1024 / PAGE_CACHE_SIZE,
+	.ra_pages	= INITIAL_VM_MAX_READAHEAD * 1024 / PAGE_CACHE_SIZE,
 	.state		= 0,
 	.capabilities	= BDI_CAP_MAP_COPY,
 	.unplug_io_fn	= default_unplug_io_fn,

Index: linux-2.6/mm/readahead.c
===================================================================

mm/readahead.c +0 -0

@@ -17,6 +17,19 @@
 #include <linux/pagevec.h>
 #include <linux/pagemap.h>
 

mm/readahead.c +3 -0

+	return 0;
+}
+
 /*
  * Initialise a struct file's readahead state.  Assumes that the caller has
  * memset *ra to zero.

Index: linux-2.6/kernel/sysctl.c
===================================================================

kernel/sysctl.c +7 -1

@@ -1273,7 +1273,13 @@ static struct ctl_table vm_table[] = {
 		.extra2		= &one,
 	},
 #endif
-
+	{
+		.procname	= "max_readahead_kb",
+		.data		= &vm_max_readahead,
+		.maxlen		= sizeof(vm_max_readahead),
+		.mode		= 0644,
+		.proc_handler	= sysctl_vm_max_readahead_handler,
+	},
 	{ }
 };
 

Index: linux-2.6/Documentation/sysctl/vm.txt
===================================================================

Documentation/sysctl/vm.txt +7 -1

@@ -31,6 +31,7 @@ Currently, these files are in /proc/sys/
 - laptop_mode
 - legacy_va_layout
 - lowmem_reserve_ratio
+- max_readahead_kb
 - max_map_count
 - memory_failure_early_kill
 - memory_failure_recovery

@@ -263,6 +264,12 @@ The minimum value is 1 (1/1 -> 100%).
 
 ==============================================================
 
+max_readahead_kb:
+
+This file contains the default maximum readahead that would be used.
+
+==============================================================
+
 max_map_count:
 
 This file contains the maximum number of memory map areas a process
 
--

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>

Make vm_max_readahead configurable at run-time. Expose a sysctl knob in procfs to change it. This would ensure that new disks added would use this value as their default read_ahead_kb.

Also filesystems which use default_backing_dev_info would also use this new value, even if they were already mounted.

Currently xfs, btrfs, nilfs, raw, mtd use the default_backing_dev_info.

Signed-off-by Nikanth Karthikesan <[email protected]>

---

Index: linux-2.6/block/blk-core.c
===================================================================

block/blk-core.c +1 -1

@@ -499,7 +499,7 @@ struct request_queue *blk_alloc_queue_no
 	q->backing_dev_info.unplug_io_fn = blk_backing_dev_unplug;
 	q->backing_dev_info.unplug_io_data = q;
 	q->backing_dev_info.ra_pages =
-			(VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE;
+			(vm_max_readahead_kb * 1024) / PAGE_CACHE_SIZE;
 	q->backing_dev_info.state = 0;
 	q->backing_dev_info.capabilities = BDI_CAP_MAP_COPY;
 	q->backing_dev_info.name = "block";

Index: linux-2.6/fs/fuse/inode.c
===================================================================

fs/fuse/inode.c +1 -1

@@ -870,7 +870,7 @@ static int fuse_bdi_init(struct fuse_con
 	int err;
 
 	fc->bdi.name = "fuse";
-	fc->bdi.ra_pages = (VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE;
+	fc->bdi.ra_pages = (vm_max_readahead_kb * 1024) / PAGE_CACHE_SIZE;
 	fc->bdi.unplug_io_fn = default_unplug_io_fn;
 	/* fuse does it's own writeback accounting */
 	fc->bdi.capabilities = BDI_CAP_NO_ACCT_WB;

Index: linux-2.6/include/linux/mm.h
===================================================================

include/linux/mm.h +5 -1

@@ -1188,7 +1188,11 @@ int write_one_page(struct page *page, in
 void task_dirty_inc(struct task_struct *tsk);
 
 /* readahead.c */
-#define VM_MAX_READAHEAD	128	/* kbytes */
+#define INITIAL_VM_MAX_READAHEAD_KB	128
+extern unsigned long vm_max_readahead_kb;
+
+int sysctl_vm_max_readahead_kb_handler(struct ctl_table *table, int write,
+		void __user *buffer, size_t *length, loff_t *ppos);
 
 int force_page_cache_readahead(struct address_space *mapping, struct file *filp,
 			pgoff_t offset, unsigned long nr_to_read);

Index: linux-2.6/mm/backing-dev.c
===================================================================

mm/backing-dev.c +2 -1

@@ -18,7 +18,8 @@ EXPORT_SYMBOL(default_unplug_io_fn);
 
 struct backing_dev_info default_backing_dev_info = {
 	.name		= "default",
-	.ra_pages	= VM_MAX_READAHEAD * 1024 / PAGE_CACHE_SIZE,
+	.ra_pages	= INITIAL_VM_MAX_READAHEAD_KB
+					>> (PAGE_CACHE_SHIFT - 10),
 	.state		= 0,
 	.capabilities	= BDI_CAP_MAP_COPY,
 	.unplug_io_fn	= default_unplug_io_fn,

Index: linux-2.6/mm/readahead.c
===================================================================

mm/readahead.c +13 -0

@@ -17,6 +17,19 @@
 #include <linux/pagevec.h>
 #include <linux/pagemap.h>
 
+unsigned long vm_max_readahead_kb = INITIAL_VM_MAX_READAHEAD_KB;
+
+int sysctl_vm_max_readahead_kb_handler(struct ctl_table *table, int write,
+		void __user *buffer, size_t *length, loff_t *ppos)
+{
+	proc_doulongvec_minmax(table, write, buffer, length, ppos);
+
+	default_backing_dev_info.ra_pages =
+			vm_max_readahead_kb >> (PAGE_CACHE_SHIFT - 10);
+
+	return 0;
+}
+
 /*
  * Initialise a struct file's readahead state.  Assumes that the caller has
  * memset *ra to zero.

Index: linux-2.6/kernel/sysctl.c
===================================================================

kernel/sysctl.c +7 -1

@@ -1273,7 +1273,13 @@ static struct ctl_table vm_table[] = {
 		.extra2		= &one,
 	},
 #endif
-
+	{
+		.procname	= "max_readahead_kb",
+		.data		= &vm_max_readahead_kb,
+		.maxlen		= sizeof(vm_max_readahead_kb),
+		.mode		= 0644,
+		.proc_handler	= sysctl_vm_max_readahead_kb_handler,
+	},
 	{ }
 };
 

Index: linux-2.6/Documentation/sysctl/vm.txt
===================================================================

Documentation/sysctl/vm.txt +6 -0

@@ -31,6 +31,7 @@ Currently, these files are in /proc/sys/
 - laptop_mode
 - legacy_va_layout
 - lowmem_reserve_ratio
+- max_readahead_kb
 - max_map_count
 - memory_failure_early_kill
 - memory_failure_recovery

@@ -263,6 +264,18 @@ The minimum value is 1 (1/1 -> 100%).
 
 ==============================================================
 
+max_readahead_kb:
+
+This file contains the default maximum readahead that would be
+used, when new disks would be added to the system.
+

Nikanth,

Quoted 3 lines

Make vm_max_readahead configurable at run-time. Expose a sysctl knob in procfs to change it. This would ensure that new disks added would use this value as their default read_ahead_kb.

Do you have use case, or customer demand for it?

Quoted 4 lines

Also filesystems which use default_backing_dev_info would also use this new value, even if they were already mounted.

Currently xfs, btrfs, nilfs, raw, mtd use the default_backing_dev_info.

This sounds like bad interface, in that users will be confused by the tricky details of "works for new devices" and "works for some fs".

One more tricky point is, btrfs/md/dm readahead size may not be influenced if some of the component disks are hot added.

So this patch is only going to work for hot-plugged disks that contains _standalone_ filesystem. Is this typical use case in servers?

Thanks,
Fengguang

Quoted 135 lines

Signed-off-by Nikanth Karthikesan <[email protected]>

---

Index: linux-2.6/block/blk-core.c
===================================================================

block/blk-core.c +1 -1

@@ -499,7 +499,7 @@ struct request_queue *blk_alloc_queue_no
 	q->backing_dev_info.unplug_io_fn = blk_backing_dev_unplug;
 	q->backing_dev_info.unplug_io_data = q;
 	q->backing_dev_info.ra_pages =
-			(VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE;
+			(vm_max_readahead_kb * 1024) / PAGE_CACHE_SIZE;
 	q->backing_dev_info.state = 0;
 	q->backing_dev_info.capabilities = BDI_CAP_MAP_COPY;
 	q->backing_dev_info.name = "block";

Index: linux-2.6/fs/fuse/inode.c
===================================================================

fs/fuse/inode.c +1 -1

@@ -870,7 +870,7 @@ static int fuse_bdi_init(struct fuse_con
 	int err;
 
 	fc->bdi.name = "fuse";
-	fc->bdi.ra_pages = (VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE;
+	fc->bdi.ra_pages = (vm_max_readahead_kb * 1024) / PAGE_CACHE_SIZE;
 	fc->bdi.unplug_io_fn = default_unplug_io_fn;
 	/* fuse does it's own writeback accounting */
 	fc->bdi.capabilities = BDI_CAP_NO_ACCT_WB;

Index: linux-2.6/include/linux/mm.h
===================================================================

include/linux/mm.h +4 -1

@@ -1188,7 +1188,11 @@ int write_one_page(struct page *page, in
 void task_dirty_inc(struct task_struct *tsk);
 
 /* readahead.c */
-#define VM_MAX_READAHEAD	128	/* kbytes */
+#define INITIAL_VM_MAX_READAHEAD_KB	128
+extern unsigned long vm_max_readahead_kb;
+
+int sysctl_vm_max_readahead_kb_handler(struct ctl_table *table, int
 write, +		void __user *buffer, size_t *length, loff_t *ppos);
 
 int force_page_cache_readahead(struct address_space *mapping, struct

file *filp, pgoff_t offset, unsigned long nr_to_read);
Index: linux-2.6/mm/backing-dev.c
===================================================================

mm/backing-dev.c +2 -1

@@ -18,7 +18,8 @@ EXPORT_SYMBOL(default_unplug_io_fn);
 
 struct backing_dev_info default_backing_dev_info = {
 	.name		= "default",
-	.ra_pages	= VM_MAX_READAHEAD * 1024 / PAGE_CACHE_SIZE,
+	.ra_pages	= INITIAL_VM_MAX_READAHEAD_KB
+					>> (PAGE_CACHE_SHIFT - 10),
 	.state		= 0,
 	.capabilities	= BDI_CAP_MAP_COPY,
 	.unplug_io_fn	= default_unplug_io_fn,

Index: linux-2.6/mm/readahead.c
===================================================================

mm/readahead.c +12 -0

@@ -17,6 +17,19 @@
 #include <linux/pagevec.h>
 #include <linux/pagemap.h>
 
+unsigned long vm_max_readahead_kb = INITIAL_VM_MAX_READAHEAD_KB;
+
+int sysctl_vm_max_readahead_kb_handler(struct ctl_table *table, int
 write, +		void __user *buffer, size_t *length, loff_t *ppos)
+{
+	proc_doulongvec_minmax(table, write, buffer, length, ppos);
+
+	default_backing_dev_info.ra_pages =
+			vm_max_readahead_kb >> (PAGE_CACHE_SHIFT - 10);
+
+	return 0;
+}
+
 /*
  * Initialise a struct file's readahead state.  Assumes that the caller

has * memset *ra to zero.
Index: linux-2.6/kernel/sysctl.c
===================================================================

kernel/sysctl.c +7 -1

@@ -1273,7 +1273,13 @@ static struct ctl_table vm_table[] = {
 		.extra2		= &one,
 	},
 #endif
-
+	{
+		.procname	= "max_readahead_kb",
+		.data		= &vm_max_readahead_kb,
+		.maxlen		= sizeof(vm_max_readahead_kb),
+		.mode		= 0644,
+		.proc_handler	= sysctl_vm_max_readahead_kb_handler,
+	},
 	{ }
 };
 

Index: linux-2.6/Documentation/sysctl/vm.txt
===================================================================

Documentation/sysctl/vm.txt +13 -0

@@ -31,6 +31,7 @@ Currently, these files are in /proc/sys/
 - laptop_mode
 - legacy_va_layout
 - lowmem_reserve_ratio
+- max_readahead_kb
 - max_map_count
 - memory_failure_early_kill
 - memory_failure_recovery

@@ -263,6 +264,18 @@ The minimum value is 1 (1/1 -> 100%).
 
 ==============================================================
 
+max_readahead_kb:
+
+This file contains the default maximum readahead that would be
+used, when new disks would be added to the system.
+
+Also filesystems which use default_backing_dev_info would also
+use this new value, even if they were already mounted.
+
+xfs, btrfs, nilfs, raw, mtd use the default_backing_dev_info.
+
+==============================================================
+
 max_map_count:
 
 This file contains the maximum number of memory map areas a process

Quoted 6 lines · On Wednesday 10 February 2010 16:35:51 Wu Fengguang wrote:

Nikanth,

Quoted 3 lines

Make vm_max_readahead configurable at run-time. Expose a sysctl knob in procfs to change it. This would ensure that new disks added would use this value as their default read_ahead_kb.

Quoted 2 lines

Do you have use case, or customer demand for it?

No body requested for it. But when doing some performance testing with readahead_kb re-compiling would be a pain, and thought that having a configurable default might be useful.

Quoted 8 lines

Quoted 4 lines

Also filesystems which use default_backing_dev_info would also use this new value, even if they were already mounted.

Currently xfs, btrfs, nilfs, raw, mtd use the default_backing_dev_info.

This sounds like bad interface, in that users will be confused by the tricky details of "works for new devices" and "works for some fs".

Quoted 6 lines

One more tricky point is, btrfs/md/dm readahead size may not be influenced if some of the component disks are hot added.

So this patch is only going to work for hot-plugged disks that contains _standalone_ filesystem. Is this typical use case in servers?

Yes, it would work only if the top-level disk is hot-plugged/created.

Quoted 134 lines

Quoted 134 lines

Signed-off-by Nikanth Karthikesan <[email protected]>

---

Index: linux-2.6/block/blk-core.c
===================================================================

block/blk-core.c +1 -1

@@ -499,7 +499,7 @@ struct request_queue *blk_alloc_queue_no
 	q->backing_dev_info.unplug_io_fn = blk_backing_dev_unplug;
 	q->backing_dev_info.unplug_io_data = q;
 	q->backing_dev_info.ra_pages =
-			(VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE;
+			(vm_max_readahead_kb * 1024) / PAGE_CACHE_SIZE;
 	q->backing_dev_info.state = 0;
 	q->backing_dev_info.capabilities = BDI_CAP_MAP_COPY;
 	q->backing_dev_info.name = "block";

Index: linux-2.6/fs/fuse/inode.c
===================================================================

fs/fuse/inode.c +1 -1

@@ -870,7 +870,7 @@ static int fuse_bdi_init(struct fuse_con
 	int err;
 
 	fc->bdi.name = "fuse";
-	fc->bdi.ra_pages = (VM_MAX_READAHEAD * 1024) / PAGE_CACHE_SIZE;
+	fc->bdi.ra_pages = (vm_max_readahead_kb * 1024) / PAGE_CACHE_SIZE;
 	fc->bdi.unplug_io_fn = default_unplug_io_fn;
 	/* fuse does it's own writeback accounting */
 	fc->bdi.capabilities = BDI_CAP_NO_ACCT_WB;

Index: linux-2.6/include/linux/mm.h
===================================================================

include/linux/mm.h +4 -1

@@ -1188,7 +1188,11 @@ int write_one_page(struct page *page, in
 void task_dirty_inc(struct task_struct *tsk);
 
 /* readahead.c */
-#define VM_MAX_READAHEAD	128	/* kbytes */
+#define INITIAL_VM_MAX_READAHEAD_KB	128
+extern unsigned long vm_max_readahead_kb;
+
+int sysctl_vm_max_readahead_kb_handler(struct ctl_table *table, int
 write, +		void __user *buffer, size_t *length, loff_t *ppos);
 
 int force_page_cache_readahead(struct address_space *mapping, struct

file *filp, pgoff_t offset, unsigned long nr_to_read);
Index: linux-2.6/mm/backing-dev.c
===================================================================

mm/backing-dev.c +2 -1

@@ -18,7 +18,8 @@ EXPORT_SYMBOL(default_unplug_io_fn);
 
 struct backing_dev_info default_backing_dev_info = {
 	.name		= "default",
-	.ra_pages	= VM_MAX_READAHEAD * 1024 / PAGE_CACHE_SIZE,
+	.ra_pages	= INITIAL_VM_MAX_READAHEAD_KB
+					>> (PAGE_CACHE_SHIFT - 10),
 	.state		= 0,
 	.capabilities	= BDI_CAP_MAP_COPY,
 	.unplug_io_fn	= default_unplug_io_fn,

Index: linux-2.6/mm/readahead.c
===================================================================

mm/readahead.c +12 -0

@@ -17,6 +17,19 @@
 #include <linux/pagevec.h>
 #include <linux/pagemap.h>
 
+unsigned long vm_max_readahead_kb = INITIAL_VM_MAX_READAHEAD_KB;
+
+int sysctl_vm_max_readahead_kb_handler(struct ctl_table *table, int
 write, +		void __user *buffer, size_t *length, loff_t *ppos)
+{
+	proc_doulongvec_minmax(table, write, buffer, length, ppos);
+
+	default_backing_dev_info.ra_pages =
+			vm_max_readahead_kb >> (PAGE_CACHE_SHIFT - 10);
+
+	return 0;
+}
+
 /*
  * Initialise a struct file's readahead state.  Assumes that the caller

has * memset *ra to zero.
Index: linux-2.6/kernel/sysctl.c
===================================================================

kernel/sysctl.c +7 -1

@@ -1273,7 +1273,13 @@ static struct ctl_table vm_table[] = {
 		.extra2		= &one,
 	},
 #endif
-
+	{
+		.procname	= "max_readahead_kb",
+		.data		= &vm_max_readahead_kb,
+		.maxlen		= sizeof(vm_max_readahead_kb),
+		.mode		= 0644,
+		.proc_handler	= sysctl_vm_max_readahead_kb_handler,
+	},
 	{ }
 };
 

Index: linux-2.6/Documentation/sysctl/vm.txt
===================================================================

Documentation/sysctl/vm.txt +13 -0

@@ -31,6 +31,7 @@ Currently, these files are in /proc/sys/
 - laptop_mode
 - legacy_va_layout
 - lowmem_reserve_ratio
+- max_readahead_kb
 - max_map_count
 - memory_failure_early_kill
 - memory_failure_recovery

@@ -263,6 +264,18 @@ The minimum value is 1 (1/1 -> 100%).
 
 ==============================================================
 
+max_readahead_kb:
+
+This file contains the default maximum readahead that would be
+used, when new disks would be added to the system.
+
+Also filesystems which use default_backing_dev_info would also
+use this new value, even if they were already mounted.
+
+xfs, btrfs, nilfs, raw, mtd use the default_backing_dev_info.
+
+==============================================================
+
 max_map_count:
 

Quoted 2 lines

Documentation/sysctl/vm.txt The minimum value is 1 (1/1 -> 100%). · line 281

Quoted 1 line

Documentation/sysctl/vm.txt +0 -0

 This file contains the maximum number of memory map areas a process

Quoted 7 lines · Nikanth Karthikesan

Quoted 6 lines · On Wednesday 10 February 2010 16:35:51 Wu Fengguang wrote:

Nikanth,

Quoted 3 lines

Make vm_max_readahead configurable at run-time. Expose a sysctl knob in procfs to change it. This would ensure that new disks added would use this value as their default read_ahead_kb.

Quoted 5 lines

Quoted 1 line

Do you have use case, or customer demand for it?

No body requested for it. But when doing some performance testing with readahead_kb re-compiling would be a pain, and thought that having a configurable default might be useful.

I wonder why you need to recompile kernel in the tests. There are three interfaces to change readahead size in runtime:

        blockdev --setra 1024 /dev/sda
        echo 512 > /sys/block/*/queue/read_ahead_kb
        echo 512 > /sys/devices/virtual/bdi/*/read_ahead_kb

Quoted 15 lines

Quoted 14 lines

Quoted 5 lines

Also filesystems which use default_backing_dev_info would also use this new value, even if they were already mounted.

Currently xfs, btrfs, nilfs, raw, mtd use the
default_backing_dev_info.

This sounds like bad interface, in that users will be confused by the tricky details of "works for new devices" and "works for some fs".

One more tricky point is, btrfs/md/dm readahead size may not be influenced if some of the component disks are hot added.

So this patch is only going to work for hot-plugged disks that contains _standalone_ filesystem. Is this typical use case in servers?

Quoted 1 line

Yes, it would work only if the top-level disk is hot-plugged/created.

Or maybe what you really want is a kernel parameter for setting the default readahead size at boot time?

In another thread, Christian Ehrhardt recommended to add a config option for it. If you like it, I can also do the kernel parameter by the way.

Review

Quoted 5 lines

(quoted diff) +5 -0

+static int __init readahead(char *str)
+{
+       if (!str)
+               return -EINVAL;
+       vm_max_readahead_kb = memparse(str, &str) / 1024ULL;

Just wondering, shouldn't you check whether the str had a valid value [memparse (str, &next); next > str ..] and if it didn't, then use the DEFAULT_VM_MAX_READAHEAD ? Otherwise, incase of a invalid value, the readahead value will become zero.

From: Nikanth Karthikesan <[email protected]>

Add new kernel parameter "readahead", which would be used instead of the value of VM_MAX_READAHEAD. If the parameter is not specified, the default of 128kb would be used.

Nikanth,

Quoted 5 lines

From: Nikanth Karthikesan <[email protected]>

Add new kernel parameter "readahead", which would be used instead of the value of VM_MAX_READAHEAD. If the parameter is not specified, the default of 128kb would be used.

The patch looks good to me, though it conflicts with my patches. Do you mind me including a modified version in my readahead patchset?

Quoted 11 lines · On Thursday 11 February 2010 16:45:24 Ankit Jain wrote:

init/main.c static int __init loglevel(char *str)

Quoted 5 lines

init/main.c +5 -0

+static int __init readahead(char *str)
+{
+       if (!str)
+               return -EINVAL;
+       vm_max_readahead_kb = memparse(str, &str) / 1024ULL;

Just wondering, shouldn't you check whether the str had a valid value [memparse (str, &next); next > str ..] and if it didn't, then use the DEFAULT_VM_MAX_READAHEAD ? Otherwise, incase of a invalid value, the readahead value will become zero.

Thanks for the review. Here is the fixed patch that checks whether all of the parameters value is consumed.

Thanks
Nikanth

From: Nikanth Karthikesan <[email protected]>

Add new kernel parameter "readahead", which would be used instead of the value of VM_MAX_READAHEAD. If the parameter is not specified, the default of 128kb would be used.

Signed-off-by Nikanth Karthikesan <[email protected]>

---

Documentation/kernel-parameters.txt +0 -0

index 736d456..354e6f1 100644

Quoted 36 lines · Nikanth Karthikesan

Quoted 10 lines · On Thursday 11 February 2010 16:45:24 Ankit Jain wrote:

init/main.c static int __init loglevel(char *str)

Quoted 5 lines

init/main.c +5 -0

+static int __init readahead(char *str)
+{
+       if (!str)
+               return -EINVAL;
+       vm_max_readahead_kb = memparse(str, &str) / 1024ULL;

Just wondering, shouldn't you check whether the str had a valid value [memparse (str, &next); next > str ..] and if it didn't, then use the DEFAULT_VM_MAX_READAHEAD ? Otherwise, incase of a invalid value, the readahead value will become zero.

Thanks for the review. Here is the fixed patch that checks whether all of the parameters value is consumed.

Thanks
Nikanth

From: Nikanth Karthikesan <[email protected]>

Add new kernel parameter "readahead", which would be used instead of the value of VM_MAX_READAHEAD. If the parameter is not specified, the default of 128kb would be used.

Signed-off-by Nikanth Karthikesan <[email protected]>

---

(quoted diff) +0 -0

b/Documentation/kernel-parameters.txt index 736d456..354e6f1 100644

Documentation/kernel-parameters.txt +0 -0

@@ -2148,6 +2148,8 @@ and is between 256 and 4096 characters. It is

defined in the file Format: <reboot_mode>[,<reboot_mode2>[,...]]
See arch/*/kernel/reboot.c or arch/*/kernel/process.c

Documentation/kernel-parameters.txt and is between 256 and 4096 characters. It is defined in the file · line 2152

Quoted 2 lines

Documentation/kernel-parameters.txt +2 -0

+	readahead=	Default readahead value for block devices.
+

I think the description should define the units (kb) and valid value ranges e.g. page size to something not excessive - say 65536kb. The above description is, IMO, useless without refering to the source to find out this information....

[snip]

Quoted 4 lines

Documentation/kernel-parameters.txt +0 -0

@@ -249,6 +250,24 @@ static int __init loglevel(char *str)
 
 early_param("loglevel", loglevel);
 

Documentation/kernel-parameters.txt static int __init loglevel(char *str) · line 261

Quoted 9 lines

Documentation/kernel-parameters.txt +9 -0

+static int __init readahead(char *str)
+{
+	unsigned long readahead_kb;
+
+	if (!str)
+		return -EINVAL;
+	readahead_kb = memparse(str, &str) / 1024ULL;
+	if (*str != '\0')
+		return -EINVAL;

And readahead_kb needs to be validated against the range of valid values here.

Documentation/kernel-parameters.txt static int __init loglevel(char *str) · line 270

Quoted 9 lines

Documentation/kernel-parameters.txt +9 -0

+
+	vm_max_readahead_kb = readahead_kb;
+	default_backing_dev_info.ra_pages = vm_max_readahead_kb
+						* 1024 / PAGE_CACHE_SIZE;
+	return 0;
+}
+
+early_param("readahead", readahead);
+

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	readahead=	Default readahead value for block devices.
+

I think the description should define the units (kb) and valid value ranges e.g. page size to something not excessive - say 65536kb. The above description is, IMO, useless without refering to the source to find out this information....

The parameter can be specified with/without any suffix(k/m/g) that memparse() helper function can accept. So it can take 1M, 1024k, 1050620. I checked other parameters that use memparse() to get similar values and they didn't document it. May be this should be described here.

Quoted 3 lines

And readahead_kb needs to be validated against the range of valid values here.

I didn't want to impose artificial restrictions. I think Wu's patch set would be adding some restrictions, like minimum readahead. He could fix it when he modifies the patch to include in his patch set.

Christian, with this patch and more patches to scale down readahead size on small memory/device size, I guess it's no longer necessary to introduce a CONFIG_READAHEAD_SIZE?

(quoted diff) +15 -0

+unsigned long max_readahead_pages = VM_MAX_READAHEAD * 1024 / PAGE_CACHE_SIZE;
+
+static int __init readahead(char *str)
+{
+	unsigned long bytes;
+
+	if (!str)
+		return -EINVAL;
+	bytes = memparse(str, &str);
+	if (*str != '\0')
+		return -EINVAL;
+
+	if (bytes) {
+		if (bytes < PAGE_CACHE_SIZE)	/* missed 'k'/'m' suffixes? */
+			return -EINVAL;

Nikanth,

Quoted 1 line

I didn't want to impose artificial restrictions. I think Wu's patch set would be adding some restrictions, like minimum readahead. He could fix it when he modifies the patch to include in his patch set.

OK, I imposed a larger bound -- 128MB.
And values 1-4095 (more exactly: PAGE_CACHE_SIZE) are prohibited mainly to catch "readahead=128" where the user really means to do 128 _KB_ readahead.

Christian, with this patch and more patches to scale down readahead size on small memory/device size, I guess it's no longer necessary to introduce a CONFIG_READAHEAD_SIZE?

Thanks,
Fengguang
---

Nikanth,

Quoted 9 lines

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	readahead=	Default readahead value for block devices.
+

I think the description should define the units (kb) and valid value ranges e.g. page size to something not excessive - say 65536kb. The above description is, IMO, useless without refering to the source to find out this information....

Quoted 4 lines

The parameter can be specified with/without any suffix(k/m/g) that memparse() helper function can accept. So it can take 1M, 1024k, 1050620. I checked other parameters that use memparse() to get similar values and they didn't document it. May be this should be described here.

Hope this helps clarify things to user: