Re: [PATCH 2/2 - v2] MM: Discard NR_UNSTABLE_NFS, use NR_WRITEBACK instead.

[Michal Hocko]

On Mon 06-04-20 10:14:16, Neil Brown wrote:
> On Fri, Apr 03 2020, Michal Hocko wrote:
> 
> > On Fri 03-04-20 11:42:20, Jan Kara wrote:
> > [...]
> >> > diff --git a/mm/vmstat.c b/mm/vmstat.c
> >> > index 78d53378db99..d1291537bbb9 100644
> >> > --- a/mm/vmstat.c
> >> > +++ b/mm/vmstat.c
> >> > @@ -1162,7 +1162,6 @@ const char * const vmstat_text[] = {
> >> >  	"nr_file_hugepages",
> >> >  	"nr_file_pmdmapped",
> >> >  	"nr_anon_transparent_hugepages",
> >> > -	"nr_unstable",
> >> >  	"nr_vmscan_write",
> >> >  	"nr_vmscan_immediate_reclaim",
> >> >  	"nr_dirtied",
> >> 
> >> This is probably the most tricky to deal with given how /proc/vmstat is
> >> formatted. OTOH for this file there's good chance we'd get away with just
> >> deleting nr_unstable line because there are entries added to it in the
> >> middle (e.g. in 60fbf0ab5da1 last September) and nobody complained yet.
> >> 
> >> What do mm people think? How were changes to vmstat counters handled in the
> >> past?
> >
> > Adding new counters in the middle seems to be generally OK. I would be
> > more worried about removing counters though. So if we can simply print a
> > phone value at the very end then this should be a reasonable workaround.
> 
> At the very end?
> Do you mean not have "nr_unstable 0" appear at all, but having "dummy 0"
> appear at the end just so that the number of lines doesn't decrease?
> Am I misunderstanding?

Sorry for not being clear. I meant semething like
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 78d53378db99..836e3f7a7aff 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1705,8 +1705,16 @@ static void *vmstat_start(struct seq_file *m, loff_t *pos)
 static void *vmstat_next(struct seq_file *m, void *arg, loff_t *pos)
 {
 	(*pos)++;
-	if (*pos >= NR_VMSTAT_ITEMS)
+	if (*pos >= NR_VMSTAT_ITEMS) {
+		/*
+		 * deprecated counters which are no longer represented
+		 * in vmstat arrays. We just lie about them to be always
+		 * 0 to not break userspace which might expect them in
+		 * int the output.
+		 */
+		seq_puts(m, "nr_unstable 0")
 		return NULL;
+	}
 	return (unsigned long *)m->private + *pos;
 }
 

-- 
Michal Hocko
SUSE Labs