remove no longer use of pdflush interface

From: Wanpeng Li <[email protected]>

------------------

Change in v2:

* add printk warning
* add description in Documentation

------------------

Signed-off-by Wanpeng Li <[email protected]>

---

.../ABI/obsolete/proc-sys-vm-nr_pdflush_threads    |    5 +++++
Documentation/feature-removal-schedule.txt         |    8 ++++++++
Documentation/sysctl/vm.txt                        |   11 -----------
fs/fs-writeback.c                                  |    5 -----
include/linux/sysctl.h                             |    3 +++
include/linux/writeback.h                          |    5 -----
kernel/sysctl.c                                    |   18 +++++++++++++-----
kernel/sysctl_binary.c                             |    2 +-
8 files changed, 30 insertions(+), 27 deletions(-)

create mode 100644 Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads

Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads new file +5 -0

new file mode 100644
index 0000000..edbe548
@@ -0,0 +1,5 @@
+What:		/proc/sys/vm/nr_pdflush_threads
+Date:		June 2012
+Contact:	Wanpeng Li <[email protected]>
+Description: Since pdflush is replaced by per-BDI flusher, the interface of old pdflush
+             exported in /proc/sys/vm/ should be removed.

Documentation/feature-removal-schedule.txt +8 -1

index e4b5775..7eea1ee 100644
@@ -6,6 +6,14 @@ be removed from this file.
 
---------------------------
 
+What: /proc/sys/vm/nr_pdflush_threads
+When: 2012
+Why: Since pdflush is deprecated, the interface exported in /proc/sys/vm/
+     should be removed.
+Who: Wanpeng Li <[email protected]>
+
+---------------------------
+

What: CONFIG_APM_CPU_IDLE, and its ability to call APM BIOS in idle
When: 2012
Why: This optional sub-feature of APM is of dubious reliability,

Documentation/sysctl/vm.txt +0 -7

index 96f0ee8..71c17d2 100644
@@ -42,7 +42,6 @@ Currently, these files are in /proc/sys/vm:
- mmap_min_addr
- nr_hugepages
- nr_overcommit_hugepages
-- nr_pdflush_threads
- nr_trim_pages         (only if CONFIG_MMU=n)
- numa_zonelist_order
- oom_dump_tasks

@@ -426,16 +425,6 @@ See Documentation/vm/hugetlbpage.txt
 

==============================================================

Documentation/sysctl/vm.txt +0 -10

-nr_pdflush_threads
-
-The current number of pdflush threads.  This value is read-only.
-The value changes according to the number of dirty pages in the system.
-
-When necessary, additional pdflush threads are created, one per second, up to
-nr_pdflush_threads_max.
-
-==============================================================
-

nr_trim_pages

This is available only on NOMMU kernels.

fs/fs-writeback.c +0 -5

index 539f36c..30ddb8a 100644
@@ -52,11 +52,6 @@ struct wb_writeback_work {
 	struct completion *done;	/* set if the caller waits */
 };
 
-/*
- * We don't actually have pdflush, but this one is exported though /proc...
- */
-int nr_pdflush_threads;
-
 /**
 * writeback_in_progress - determine whether there is writeback in progress
 * @bdi: the device's backing_dev_info structure.

include/linux/sysctl.h +3 -0

index c34b4c8..f25be74 100644
@@ -964,6 +964,9 @@ extern int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int,
 				      void __user *, size_t *, loff_t *);
 extern int proc_do_large_bitmap(struct ctl_table *, int,
 				void __user *, size_t *, loff_t *);
+extern int proc_deprecated(struct ctl_table *, int,
+				void __user *, size_t *, loff_t *);
+
 
 /*
 * Register a set of sysctl names by calling register_sysctl_table

include/linux/writeback.h +0 -5

index a2b84f5..13b5df7 100644
@@ -193,9 +193,4 @@ void tag_pages_for_writeback(struct address_space *mapping,
 
 void account_page_redirty(struct page *page);
 
-/* pdflush.c */
-extern int nr_pdflush_threads;	/* Global so it can be exported to sysctl
-				   read-only. */
-
-
 #endif		/* WRITEBACK_H */

kernel/sysctl.c +13 -5

index 4ab1187..1ab53cb 100644
@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
 		.extra1		= &zero,
 	},
 	{
-		.procname	= "nr_pdflush_threads",
-		.data		= &nr_pdflush_threads,
-		.maxlen		= sizeof nr_pdflush_threads,
-		.mode		= 0444 /* read-only*/,
-		.proc_handler	= proc_dointvec,
+		.procname       = "nr_pdflush_threads",
+		.mode           = 0444 /* read-only */,
+		.proc_handler   = proc_deprecated,
 	},
 	{
 		.procname	= "swappiness",

@@ -2505,6 +2503,15 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
 
 #endif /* CONFIG_PROC_SYSCTL */
 
+/* notice associated proc deprecated */
+int proc_deprecated(struct ctl_table *table, int write,
+		    void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	printk(KERN_WARNING "%s exported in /proc is deprecated\n",
+			table->procname);
+	return -ENOSYS;
+}
+
 /*
 * No sense putting this after each symbol definition, twice,
 * exception granted :-)

@@ -2517,3 +2524,4 @@ EXPORT_SYMBOL(proc_dointvec_ms_jiffies);
 EXPORT_SYMBOL(proc_dostring);
 EXPORT_SYMBOL(proc_doulongvec_minmax);
 EXPORT_SYMBOL(proc_doulongvec_ms_jiffies_minmax);
+EXPORT_SYMBOL(proc_deprecated);

kernel/sysctl_binary.c +1 -1

index a650694..65bdcf1 100644
@@ -147,7 +147,7 @@ static const struct bin_table bin_vm_table[] = {
 	{ CTL_INT,	VM_DIRTY_RATIO,			"dirty_ratio" },
 	/* VM_DIRTY_WB_CS "dirty_writeback_centisecs" no longer used */
 	/* VM_DIRTY_EXPIRE_CS "dirty_expire_centisecs" no longer used */
-	{ CTL_INT,	VM_NR_PDFLUSH_THREADS,		"nr_pdflush_threads" },
+	/* VM_NR_PDFLUSH_THREADS "nr_pdflush_threads" no longer used */
 	{ CTL_INT,	VM_OVERCOMMIT_RATIO,		"overcommit_ratio" },
 	/* VM_PAGEBUF unused */
 	/* VM_HUGETLB_PAGES "nr_hugepages" no longer used */

Signature · 1 line

1.7.9.5

- Why not just remove the proc entry since we needn't it anymore?

- If we really want to keep the entry and just output warning message, we *only* can see the message out of system console. It's reasonable to put the message into the buffer directly so that any users from system console/telnet/ssh can see the warning message?

Quoted 184 lines

From: Wanpeng Li <[email protected]>

------------------

Change in v2:

* add printk warning
* add description in Documentation

------------------

Signed-off-by Wanpeng Li <[email protected]>

---

.../ABI/obsolete/proc-sys-vm-nr_pdflush_threads    |    5 +++++
Documentation/feature-removal-schedule.txt         |    8 ++++++++
Documentation/sysctl/vm.txt                        |   11 -----------
fs/fs-writeback.c                                  |    5 -----
include/linux/sysctl.h                             |    3 +++
include/linux/writeback.h                          |    5 -----
kernel/sysctl.c                                    |   18 +++++++++++++-----
kernel/sysctl_binary.c                             |    2 +-
8 files changed, 30 insertions(+), 27 deletions(-)

create mode 100644 Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads

Documentation/ABI/obsolete/proc-sys-vm-nr_pdflush_threads new file +5 -0

new file mode 100644
index 0000000..edbe548
@@ -0,0 +1,5 @@
+What:		/proc/sys/vm/nr_pdflush_threads
+Date:		June 2012
+Contact:	Wanpeng Li <[email protected]>
+Description: Since pdflush is replaced by per-BDI flusher, the interface of old pdflush
+             exported in /proc/sys/vm/ should be removed.

Documentation/feature-removal-schedule.txt +8 -1

index e4b5775..7eea1ee 100644
@@ -6,6 +6,14 @@ be removed from this file.
 
---------------------------
 
+What: /proc/sys/vm/nr_pdflush_threads
+When: 2012
+Why: Since pdflush is deprecated, the interface exported in /proc/sys/vm/
+     should be removed.
+Who: Wanpeng Li <[email protected]>
+
+---------------------------
+

What: CONFIG_APM_CPU_IDLE, and its ability to call APM BIOS in idle
When: 2012
Why: This optional sub-feature of APM is of dubious reliability,

Documentation/sysctl/vm.txt +0 -7

index 96f0ee8..71c17d2 100644
@@ -42,7 +42,6 @@ Currently, these files are in /proc/sys/vm:
- mmap_min_addr
- nr_hugepages
- nr_overcommit_hugepages
-- nr_pdflush_threads
- nr_trim_pages         (only if CONFIG_MMU=n)
- numa_zonelist_order
- oom_dump_tasks

@@ -426,16 +425,6 @@ See Documentation/vm/hugetlbpage.txt
 

==============================================================

Documentation/sysctl/vm.txt +0 -10

-nr_pdflush_threads
-
-The current number of pdflush threads.  This value is read-only.
-The value changes according to the number of dirty pages in the system.
-
-When necessary, additional pdflush threads are created, one per second, up to
-nr_pdflush_threads_max.
-
-==============================================================
-

nr_trim_pages

This is available only on NOMMU kernels.

fs/fs-writeback.c +0 -5

index 539f36c..30ddb8a 100644
@@ -52,11 +52,6 @@ struct wb_writeback_work {
 	struct completion *done;	/* set if the caller waits */
 };
 
-/*
- * We don't actually have pdflush, but this one is exported though /proc...
- */
-int nr_pdflush_threads;
-
 /**
 * writeback_in_progress - determine whether there is writeback in progress
 * @bdi: the device's backing_dev_info structure.

include/linux/sysctl.h +3 -0

index c34b4c8..f25be74 100644
@@ -964,6 +964,9 @@ extern int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int,
 				      void __user *, size_t *, loff_t *);
 extern int proc_do_large_bitmap(struct ctl_table *, int,
 				void __user *, size_t *, loff_t *);
+extern int proc_deprecated(struct ctl_table *, int,
+				void __user *, size_t *, loff_t *);
+
 
 /*
 * Register a set of sysctl names by calling register_sysctl_table

include/linux/writeback.h +0 -5

index a2b84f5..13b5df7 100644
@@ -193,9 +193,4 @@ void tag_pages_for_writeback(struct address_space *mapping,
 
 void account_page_redirty(struct page *page);
 
-/* pdflush.c */
-extern int nr_pdflush_threads;	/* Global so it can be exported to sysctl
-				   read-only. */
-
-
 #endif		/* WRITEBACK_H */

kernel/sysctl.c +13 -5

index 4ab1187..1ab53cb 100644
@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
 		.extra1		= &zero,
 	},
 	{
-		.procname	= "nr_pdflush_threads",
-		.data		= &nr_pdflush_threads,
-		.maxlen		= sizeof nr_pdflush_threads,
-		.mode		= 0444 /* read-only*/,
-		.proc_handler	= proc_dointvec,
+		.procname       = "nr_pdflush_threads",
+		.mode           = 0444 /* read-only */,
+		.proc_handler   = proc_deprecated,
 	},
 	{
 		.procname	= "swappiness",

@@ -2505,6 +2503,15 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
 
 #endif /* CONFIG_PROC_SYSCTL */
 
+/* notice associated proc deprecated */
+int proc_deprecated(struct ctl_table *table, int write,
+		    void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	printk(KERN_WARNING "%s exported in /proc is deprecated\n",
+			table->procname);
+	return -ENOSYS;
+}
+
 /*
 * No sense putting this after each symbol definition, twice,
 * exception granted :-)

@@ -2517,3 +2524,4 @@ EXPORT_SYMBOL(proc_dointvec_ms_jiffies);
 EXPORT_SYMBOL(proc_dostring);
 EXPORT_SYMBOL(proc_doulongvec_minmax);
 EXPORT_SYMBOL(proc_doulongvec_ms_jiffies_minmax);
+EXPORT_SYMBOL(proc_deprecated);

kernel/sysctl_binary.c +1 -1

index a650694..65bdcf1 100644
@@ -147,7 +147,7 @@ static const struct bin_table bin_vm_table[] = {
 	{ CTL_INT,	VM_DIRTY_RATIO,			"dirty_ratio" },
 	/* VM_DIRTY_WB_CS "dirty_writeback_centisecs" no longer used */
 	/* VM_DIRTY_EXPIRE_CS "dirty_expire_centisecs" no longer used */
-	{ CTL_INT,	VM_NR_PDFLUSH_THREADS,		"nr_pdflush_threads" },
+	/* VM_NR_PDFLUSH_THREADS "nr_pdflush_threads" no longer used */
 	{ CTL_INT,	VM_OVERCOMMIT_RATIO,		"overcommit_ratio" },
 	/* VM_PAGEBUF unused */
 	/* VM_HUGETLB_PAGES "nr_hugepages" no longer used */

Signature · 1 line

1.7.9.5

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

From: Wanpeng Li <[email protected]>

------------------

Change in v2:

* add printk warning
* add description in Documentation

kernel/sysctl.c +3 -5

@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
 		.extra1		= &zero,
 	},
 	{
-		.procname	= "nr_pdflush_threads",
-		.data		= &nr_pdflush_threads,
-		.maxlen		= sizeof nr_pdflush_threads,
-		.mode		= 0444 /* read-only*/,
-		.proc_handler	= proc_dointvec,
+		.procname       = "nr_pdflush_threads",
+		.mode           = 0444 /* read-only */,
+		.proc_handler   = proc_deprecated,
 	},
 	{
 		.procname	= "swappiness",

@@ -2505,6 +2503,15 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
 

I do agree with the intent of the patch and the creation of proc_deprecated() seems a good idea - something we can use in the future.

Quoted 20 lines

kernel/sysctl.c +3 -5

@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
 		.extra1		= &zero,
 	},
 	{
-		.procname	= "nr_pdflush_threads",
-		.data		= &nr_pdflush_threads,
-		.maxlen		= sizeof nr_pdflush_threads,
-		.mode		= 0444 /* read-only*/,
-		.proc_handler	= proc_dointvec,
+		.procname       = "nr_pdflush_threads",
+		.mode           = 0444 /* read-only */,
+		.proc_handler   = proc_deprecated,
 	},
 	{
 		.procname	= "swappiness",

@@ -2505,6 +2503,15 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,
 

kernel/sysctl.c int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write, · line 2513

Quoted 10 lines

kernel/sysctl.c +8 -0

 #endif /* CONFIG_PROC_SYSCTL */
 
+/* notice associated proc deprecated */
+int proc_deprecated(struct ctl_table *table, int write,
+		    void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+	printk(KERN_WARNING "%s exported in /proc is deprecated\n",
+			table->procname);
+	return -ENOSYS;
+}

I see a couple of things here.

Firstly, I'd change the text from "deprecated" to "is scheduled for removal". Which implies that the function should be called proc_obsolete().

Secondly, this code will permit unprivileged users to flood the logs, by repeatedly reading /proc/sys/vm/nr_pdflush_threads. We try to avoid this, as it is a form of denial-of-service attack.

This is a bit hard to fix. The typical way of addressing this is to use printk_once(), so the message only appears once per boot. But that doesn't work for a generic function - we'd need to add one bit of state to the ctl_table to do this. We can of course do that, but it's not obvious that it's _worth_ doing that just for handling obsolete entries.

So perhaps the solution is to give up on the generic proc_obsolete() idea, and just add a handler specifically for nr_pdflush_threads, whcih uses printk_once().

Quoted 11 lines

Secondly, this code will permit unprivileged users to flood the logs, by repeatedly reading /proc/sys/vm/nr_pdflush_threads. We try to avoid this, as it is a form of denial-of-service attack.

This is a bit hard to fix. The typical way of addressing this is to use printk_once(), so the message only appears once per boot. But that doesn't work for a generic function - we'd need to add one bit of state to the ctl_table to do this. We can of course do that, but it's not obvious that it's _worth_ doing that just for handling obsolete entries.

Quoted 3 lines

So perhaps the solution is to give up on the generic proc_obsolete() idea, and just add a handler specifically for nr_pdflush_threads, whcih uses printk_once().

What about modify the generic proc_obsolete just to put the warning message into the buffer, then transfer to userspace, in order to users can see this warning.Do you think this is a better idea?

On Tue, 5 Jun 2012 17:26:11 +0800
Wanpeng Li <[email protected]> wrote:

So perhaps the solution is to give up on the generic proc_obsolete() idea, and just add a handler specifically for nr_pdflush_threads, whcih uses printk_once().

Quoted 7 lines · Andrew Morton

On Tue, 5 Jun 2012 17:26:11 +0800
Wanpeng Li <[email protected]> wrote:

So perhaps the solution is to give up on the generic proc_obsolete() idea, and just add a handler specifically for nr_pdflush_threads, whcih uses printk_once().

If there are many users than a self made print once is worth it we can use the ctl_table->data pointer for this, some thing like:

int nr_pdflush_was_printed;

(quoted diff) +4 -5

@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
 		.extra1		= &zero,
 	},
 	{
-		.procname	= "nr_pdflush_threads",
-		.data		= &nr_pdflush_threads,
-		.maxlen		= sizeof nr_pdflush_threads,
-		.mode		= 0444 /* read-only*/,
-		.proc_handler	= proc_dointvec,
+		.data		= &nr_pdflush_was_printed,
+		.procname       = "nr_pdflush_threads",
+		.mode           = 0444 /* read-only */,
+		.proc_handler   = proc_obsolete,
 	},

kernel/sysctl.c +3 -0

+	}
+	return -ENOSYS;
+}

Quoted 7 lines · Andrew Morton

On Tue, 5 Jun 2012 17:26:11 +0800
Wanpeng Li <[email protected]> wrote:

So perhaps the solution is to give up on the generic proc_obsolete() idea, and just add a handler specifically for nr_pdflush_threads, whcih uses printk_once().

If there are many users than a self made print once is worth it we can use the ctl_table->data pointer for this, some thing like:

int nr_pdflush_was_printed;

kernel/sysctl.c +2 -5

@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
 		.extra1		= &zero,
 	},
 	{
-		.procname	= "nr_pdflush_threads",
-		.data		= &nr_pdflush_threads,
-		.maxlen		= sizeof nr_pdflush_threads,
-		.mode		= 0444 /* read-only*/,
-		.proc_handler	= proc_dointvec,
+		.data		= &nr_pdflush_was_printed,
+		.procname       = "nr_pdflush_threads",

kernel/sysctl.c +3 -0

+	}
+	return -ENOSYS;
+}

Quoted 37 lines · Boaz Harrosh

Quoted 7 lines · Andrew Morton

On Tue, 5 Jun 2012 17:26:11 +0800
Wanpeng Li <[email protected]> wrote:

So perhaps the solution is to give up on the generic proc_obsolete() idea, and just add a handler specifically for nr_pdflush_threads, whcih uses printk_once().

If there are many users than a self made print once is worth it we can use the ctl_table->data pointer for this, some thing like:

int nr_pdflush_was_printed;

kernel/sysctl.c +9 -5

@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
 		.extra1		= &zero,
 	},
 	{
-		.procname	= "nr_pdflush_threads",
-		.data		= &nr_pdflush_threads,
-		.maxlen		= sizeof nr_pdflush_threads,
-		.mode		= 0444 /* read-only*/,
-		.proc_handler	= proc_dointvec,
+		.data		= &nr_pdflush_was_printed,
+		.procname       = "nr_pdflush_threads",
+		.mode           = 0444 /* read-only */,
+		.proc_handler   = proc_obsolete,
 	},
 
 
+/* notice associated proc deprecated */
+int proc_deprecated(struct ctl_table *table, int write,
+		    void __user *buffer, size_t *lenp, loff_t *ppos, )
+{
+	if (*ctl_table->data) {

stupid me, time for coffee

Boaz

Quoted 3 lines

kernel/sysctl.c +3 -0

+	}
+	return -ENOSYS;
+}

If there are many users than a self made print once is worth it we can use the ctl_table->data pointer for this, some thing like:

int nr_pdflush_was_printed;

(quoted diff) +5 -5

@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
 		.extra1		= &zero,
 	},
 	{
-		.procname	= "nr_pdflush_threads",
-		.data		= &nr_pdflush_threads,
-		.maxlen		= sizeof nr_pdflush_threads,
-		.mode		= 0444 /* read-only*/,
-		.proc_handler	= proc_dointvec,
+		.data		= &nr_pdflush_was_printed,
+		.procname       = "nr_pdflush_threads",
+		.mode           = 0444 /* read-only */,
+		.proc_handler   = proc_obsolete,
 	},
 
 
+/* notice associated proc deprecated */

Quoted 22 lines · Boaz Harrosh

If there are many users than a self made print once is worth it we can use the ctl_table->data pointer for this, some thing like:

int nr_pdflush_was_printed;

kernel/sysctl.c +4 -5

@@ -1095,11 +1095,9 @@ static struct ctl_table vm_table[] = {
 		.extra1		= &zero,
 	},
 	{
-		.procname	= "nr_pdflush_threads",
-		.data		= &nr_pdflush_threads,
-		.maxlen		= sizeof nr_pdflush_threads,
-		.mode		= 0444 /* read-only*/,
-		.proc_handler	= proc_dointvec,
+		.data		= &nr_pdflush_was_printed,
+		.procname       = "nr_pdflush_threads",
+		.mode           = 0444 /* read-only */,
+		.proc_handler   = proc_obsolete,
 	},
 

kernel/sysctl.c int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write,

Quoted 11 lines

kernel/sysctl.c +11 -0

+/* notice associated proc deprecated */
+int proc_deprecated(struct ctl_table *table, int write,
+		    void __user *buffer, size_t *lenp, loff_t *ppos, )
+{
+	if (*ctl_table->data) {
+		printk(KERN_WARNING "%s exported in /proc is deprecated\n",
+			table->procname);
+		*ctl_table->data = 0;
+	}
+	return -ENOSYS;
+}

But this time you add another variable nr_pdflush_was_printed. So remove variable nr_pdflush_threads does not make sense.