writeback cleanups and trivial fixes

Signed-off-by Richard Kennedy <[email protected]>

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

---
 mm/page-writeback.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

mm/page-writeback.c +1 -1

@@ -503,11 +503,12 @@ static void balance_dirty_pages(struct a
 		};
 
 		get_dirty_limits(&background_thresh, &dirty_thresh,
-				&bdi_thresh, bdi);
+				 &bdi_thresh, bdi);

Quoted 23 lines · On Sun, 11 Jul 2010 10:06:57 +0800 Wu Fengguang <[email protected]> wrote:

Signed-off-by Richard Kennedy <[email protected]>

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

---
 mm/page-writeback.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

mm/page-writeback.c +4 -3

@@ -503,11 +503,12 @@ static void balance_dirty_pages(struct a
 		};
 
 		get_dirty_limits(&background_thresh, &dirty_thresh,
-				&bdi_thresh, bdi);
+				 &bdi_thresh, bdi);
 
 		nr_reclaimable = global_page_state(NR_FILE_DIRTY) +
-					global_page_state(NR_UNSTABLE_NFS);
-		nr_writeback = global_page_state(NR_WRITEBACK);
+				 global_page_state(NR_UNSTABLE_NFS);
+		nr_writeback = global_page_state(NR_WRITEBACK) +
+			       global_page_state(NR_WRITEBACK_TEMP);
 

mm/page-writeback.c static void balance_dirty_pages(struct a · line 514

Quoted 3 lines

mm/page-writeback.c +0 -0

 		bdi_nr_reclaimable = bdi_stat(bdi, BDI_RECLAIMABLE);
 		bdi_nr_writeback = bdi_stat(bdi, BDI_WRITEBACK);

hm, OK.

I wonder whether we could/should have unified NR_WRITEBACK_TEMP and NR_UNSTABLE_NFS. Their "meanings" aren't quite the same, but perhaps some "treat page as dirty because the fs is futzing with it" thing.

Quoted 26 lines · Andrew Morton

Quoted 23 lines · On Sun, 11 Jul 2010 10:06:57 +0800 Wu Fengguang <[email protected]> wrote:

Signed-off-by Richard Kennedy <[email protected]>

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

---
 mm/page-writeback.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

mm/page-writeback.c +4 -3

@@ -503,11 +503,12 @@ static void balance_dirty_pages(struct a
 		};
 
 		get_dirty_limits(&background_thresh, &dirty_thresh,
-				&bdi_thresh, bdi);
+				 &bdi_thresh, bdi);
 
 		nr_reclaimable = global_page_state(NR_FILE_DIRTY) +
-					global_page_state(NR_UNSTABLE_NFS);
-		nr_writeback = global_page_state(NR_WRITEBACK);
+				 global_page_state(NR_UNSTABLE_NFS);
+		nr_writeback = global_page_state(NR_WRITEBACK) +
+			       global_page_state(NR_WRITEBACK_TEMP);
 

Quoted 5 lines

mm/page-writeback.c static void balance_dirty_pages(struct a · line 514

Quoted 3 lines

mm/page-writeback.c +0 -0

 		bdi_nr_reclaimable = bdi_stat(bdi, BDI_RECLAIMABLE);
 		bdi_nr_writeback = bdi_stat(bdi, BDI_WRITEBACK);

hm, OK.

Hm, hm. I'm not sure this is right. The VM has absolutely no control over NR_WRITEBACK_TEMP pages, they may clear quickly or may not make any progress. So it's usually wrong to make a decision based on NR_WRITEBACK_TEMP for an unrelated device.

Using it in throttle_vm_writeout() would actually be deadlocky, since the userspace filesystem will probably depend on memory allocations to complete the writeout.

The only place where we should be taking NR_WRITEBACK_TEMP into account is calculating the remaining memory that can be devided between dirtyers, and that's (clip_bdi_dirty_limit) where it is already used.

Quoted 3 lines

I wonder whether we could/should have unified NR_WRITEBACK_TEMP and NR_UNSTABLE_NFS. Their "meanings" aren't quite the same, but perhaps some "treat page as dirty because the fs is futzing with it" thing.

AFAICS NR_UNSTABLE_NFS is something akin to NR_DIRTY, only on the server side. So nfs can very much do something about making NR_UNSTABLE_NFS go away, while there's nothing that can be done about NR_WRITEBACK_TEMP.

Reducing the number of times balance_dirty_pages calls global_page_state reduces the cache references and so improves write performance on a variety of workloads.

'perf stats' of simple fio write tests shows the reduction in cache access. Where the test is fio 'write,mmap,600Mb,pre_read' on AMD AthlonX2 with 3Gb memory (dirty_threshold approx 600 Mb) running each test 10 times, dropping the fasted & slowest values then taking the average & standard deviation

		average (s.d.) in millions (10^6)

2.6.31-rc8 648.6 (14.6)
+patch 620.1 (16.5)

Achieving this reduction is by dropping clip_bdi_dirty_limit as it rereads the counters to apply the dirty_threshold and moving this check up into balance_dirty_pages where it has already read the counters.

Also by rearrange the for loop to only contain one copy of the limit tests allows the pdflush test after the loop to use the local copies of the counters rather than rereading them.

In the common case with no throttling it now calls global_page_state 5 fewer times and bdi_stat 2 fewer.

Fengguang:

This patch slightly changes behavior by replacing clip_bdi_dirty_limit() with the explicit check (nr_reclaimable + nr_writeback >= dirty_thresh) to avoid exceeding the dirty limit. Since the bdi dirty limit is mostly accurate we don't need to do routinely clip. A simple dirty limit check would be enough.

The check is necessary because, in principle we should throttle everything calling balance_dirty_pages() when we're over the total limit, as said by Peter.

We now set and clear dirty_exceeded not only based on bdi dirty limits, but also on the global dirty limits. This is a bit counterintuitive, but the global limits are the ultimate goal and shall be always imposed.

We may now start background writeback work based on outdated conditions. That's safe because the bdi flush thread will (and have to) double check the states. It reduces overall overheads because the test based on old states still have good chance to be right.

CC Jan Kara <[email protected]>

CC Peter Zijlstra <[email protected]>

Signed-off-by Richard Kennedy <[email protected]>

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

---
 mm/page-writeback.c |   95 ++++++++++++++----------------------------
 1 file changed, 33 insertions(+), 62 deletions(-)

mm/page-writeback.c +33 -62

@@ -253,32 +253,6 @@ static void bdi_writeout_fraction(struct
 	}
 }
 
-/*
- * Clip the earned share of dirty pages to that which is actually available.
- * This avoids exceeding the total dirty_limit when the floating averages
- * fluctuate too quickly.
- */
-static void clip_bdi_dirty_limit(struct backing_dev_info *bdi,
-		unsigned long dirty, unsigned long *pbdi_dirty)
-{
-	unsigned long avail_dirty;
-
-	avail_dirty = global_page_state(NR_FILE_DIRTY) +
-		 global_page_state(NR_WRITEBACK) +
-		 global_page_state(NR_UNSTABLE_NFS) +
-		 global_page_state(NR_WRITEBACK_TEMP);
-
-	if (avail_dirty < dirty)
-		avail_dirty = dirty - avail_dirty;
-	else
-		avail_dirty = 0;
-
-	avail_dirty += bdi_stat(bdi, BDI_RECLAIMABLE) +
-		bdi_stat(bdi, BDI_WRITEBACK);
-
-	*pbdi_dirty = min(*pbdi_dirty, avail_dirty);
-}
-
 static inline void task_dirties_fraction(struct task_struct *tsk,
 		long *numerator, long *denominator)
 {

@@ -469,7 +443,6 @@ get_dirty_limits(unsigned long *pbackgro
 			bdi_dirty = dirty * bdi->max_ratio / 100;
 
 		*pbdi_dirty = bdi_dirty;
-		clip_bdi_dirty_limit(bdi, dirty, pbdi_dirty);
 		task_dirty_limit(current, pbdi_dirty);
 	}
 }

@@ -491,7 +464,7 @@ static void balance_dirty_pages(struct a
 	unsigned long bdi_thresh;
 	unsigned long pages_written = 0;
 	unsigned long pause = 1;
-
+	int dirty_exceeded;
 	struct backing_dev_info *bdi = mapping->backing_dev_info;
 
 	for (;;) {

@@ -510,10 +483,35 @@ static void balance_dirty_pages(struct a
 		nr_writeback = global_page_state(NR_WRITEBACK) +
 			       global_page_state(NR_WRITEBACK_TEMP);
 
-		bdi_nr_reclaimable = bdi_stat(bdi, BDI_RECLAIMABLE);
-		bdi_nr_writeback = bdi_stat(bdi, BDI_WRITEBACK);
+		/*
+		 * In order to avoid the stacked BDI deadlock we need
+		 * to ensure we accurately count the 'dirty' pages when
+		 * the threshold is low.
+		 *
+		 * Otherwise it would be possible to get thresh+n pages
+		 * reported dirty, even though there are thresh-m pages
+		 * actually dirty; with m+n sitting in the percpu
+		 * deltas.
+		 */
+		if (bdi_thresh < 2*bdi_stat_error(bdi)) {
+			bdi_nr_reclaimable = bdi_stat_sum(bdi, BDI_RECLAIMABLE);
+			bdi_nr_writeback = bdi_stat_sum(bdi, BDI_WRITEBACK);
+		} else {
+			bdi_nr_reclaimable = bdi_stat(bdi, BDI_RECLAIMABLE);
+			bdi_nr_writeback = bdi_stat(bdi, BDI_WRITEBACK);
+		}
+
+		/*
+		 * The bdi thresh is somehow "soft" limit derived from the
+		 * global "hard" limit. The former helps to prevent heavy IO
+		 * bdi or process from holding back light ones; The latter is
+		 * the last resort safeguard.
+		 */
+		dirty_exceeded =
+			(bdi_nr_reclaimable + bdi_nr_writeback >= bdi_thresh)
+			|| (nr_reclaimable + nr_writeback >= dirty_thresh);
 
-		if (bdi_nr_reclaimable + bdi_nr_writeback <= bdi_thresh)
+		if (!dirty_exceeded)
 			break;
 
 		/*

@@ -541,34 +539,10 @@ static void balance_dirty_pages(struct a
 		if (bdi_nr_reclaimable > bdi_thresh) {
 			writeback_inodes_wb(&bdi->wb, &wbc);
 			pages_written += write_chunk - wbc.nr_to_write;
-			get_dirty_limits(&background_thresh, &dirty_thresh,
-				       &bdi_thresh, bdi);
 			trace_wbc_balance_dirty_written(&wbc, bdi);
+			if (pages_written >= write_chunk)
+				break;		/* We've done our duty */
 		}
-
-		/*
-		 * In order to avoid the stacked BDI deadlock we need
-		 * to ensure we accurately count the 'dirty' pages when
-		 * the threshold is low.
-		 *
-		 * Otherwise it would be possible to get thresh+n pages
-		 * reported dirty, even though there are thresh-m pages
-		 * actually dirty; with m+n sitting in the percpu
-		 * deltas.
-		 */
-		if (bdi_thresh < 2*bdi_stat_error(bdi)) {
-			bdi_nr_reclaimable = bdi_stat_sum(bdi, BDI_RECLAIMABLE);
-			bdi_nr_writeback = bdi_stat_sum(bdi, BDI_WRITEBACK);
-		} else if (bdi_nr_reclaimable) {
-			bdi_nr_reclaimable = bdi_stat(bdi, BDI_RECLAIMABLE);
-			bdi_nr_writeback = bdi_stat(bdi, BDI_WRITEBACK);
-		}
-
-		if (bdi_nr_reclaimable + bdi_nr_writeback <= bdi_thresh)
-			break;
-		if (pages_written >= write_chunk)
-			break;		/* We've done our duty */
-
 		trace_wbc_balance_dirty_wait(&wbc, bdi);
 		__set_current_state(TASK_INTERRUPTIBLE);
 		io_schedule_timeout(pause);

@@ -582,8 +556,7 @@ static void balance_dirty_pages(struct a
 			pause = HZ / 10;
 	}
 
-	if (bdi_nr_reclaimable + bdi_nr_writeback < bdi_thresh &&
-			bdi->dirty_exceeded)
+	if (!dirty_exceeded && bdi->dirty_exceeded)
 		bdi->dirty_exceeded = 0;
 
 	if (writeback_in_progress(bdi))

@@ -598,9 +571,7 @@ static void balance_dirty_pages(struct a
 	 * background_thresh, to keep the amount of dirty memory low.
 	 */
 	if ((laptop_mode && pages_written) ||
-	    (!laptop_mode && ((global_page_state(NR_FILE_DIRTY)
-			       + global_page_state(NR_UNSTABLE_NFS))
-					  > background_thresh)))
+	    (!laptop_mode && (nr_reclaimable > background_thresh)))
 		bdi_start_background_writeback(bdi);
 }
 

Quoted 6 lines

This patch slightly changes behavior by replacing clip_bdi_dirty_limit() with the explicit check (nr_reclaimable + nr_writeback >= dirty_thresh) to avoid exceeding the dirty limit. Since the bdi dirty limit is mostly accurate we don't need to do routinely clip. A simple dirty limit check would be enough.

Quoted 7 lines

The check is necessary because, in principle we should throttle everything calling balance_dirty_pages() when we're over the total limit, as said by Peter.

We now set and clear dirty_exceeded not only based on bdi dirty limits, but also on the global dirty limits. This is a bit counterintuitive, but the global limits are the ultimate goal and shall be always imposed.

Thinking about this again - what you did is rather big change for systems
with more active BDIs. For example if I have two disks sda and sdb and write for some time to sda, then dirty limit for sdb gets scaled down. So when we start writing to sbd we'll heavily throttle the threads until the dirty limit for sdb ramps up regardless of how far are we to reach the global limit...

Quoted 13 lines

Quoted 6 lines

This patch slightly changes behavior by replacing clip_bdi_dirty_limit() with the explicit check (nr_reclaimable + nr_writeback >= dirty_thresh) to avoid exceeding the dirty limit. Since the bdi dirty limit is mostly accurate we don't need to do routinely clip. A simple dirty limit check would be enough.

Quoted 7 lines

The check is necessary because, in principle we should throttle everything calling balance_dirty_pages() when we're over the total limit, as said by Peter.

We now set and clear dirty_exceeded not only based on bdi dirty limits, but also on the global dirty limits. This is a bit counterintuitive, but the global limits are the ultimate goal and shall be always imposed.

Quoted 6 lines

Thinking about this again - what you did is rather big change for systems
with more active BDIs. For example if I have two disks sda and sdb and write for some time to sda, then dirty limit for sdb gets scaled down. So when we start writing to sbd we'll heavily throttle the threads until the dirty limit for sdb ramps up regardless of how far are we to reach the global limit...

The global threshold check is added in place of clip_bdi_dirty_limit() for safety and not intended as a behavior change. If ever leading to big behavior change and regression, that it would be indicating some too permissive per-bdi threshold calculation.

Did you see the global dirty threshold get exceeded when writing to 2+ devices? Occasional small exceeding should be OK though. I tried the following debug patch and see no warnings when doing two concurrent cp over local disk and NFS.

Quoted 9 lines

The global threshold check is added in place of clip_bdi_dirty_limit() for safety and not intended as a behavior change. If ever leading to big behavior change and regression, that it would be indicating some too permissive per-bdi threshold calculation.

Did you see the global dirty threshold get exceeded when writing to 2+ devices? Occasional small exceeding should be OK though. I tried the following debug patch and see no warnings when doing two concurrent cp over local disk and NFS.

Oops, sorry. I've misread the code. You're right. There shouldn't be a big
change in the behavior.

CC Jan Kara <[email protected]>

Signed-off-by Richard Kennedy <[email protected]>

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

---
 mm/page-writeback.c |   95 ++++++++++++++----------------------------
 1 file changed, 33 insertions(+), 62 deletions(-)

mm/page-writeback.c 2010-07-11 08:44:49.000000000 +0800 renamed from mm/page-writeback.c 2010-07-11 08:42:14.000000000 +0800 +32 -0

@@ -253,32 +253,6 @@ static void bdi_writeout_fraction(struct
        }
 }
 
 static inline void task_dirties_fraction(struct task_struct *tsk,
                long *numerator, long *denominator)
 {

@@ -469,7 +443,6 @@ get_dirty_limits(unsigned long *pbackgro
                        bdi_dirty = dirty * bdi->max_ratio / 100;
 
                *pbdi_dirty = bdi_dirty;
                task_dirty_limit(current, pbdi_dirty);
        }
 }

@@ -491,7 +464,7 @@ static void balance_dirty_pages(struct a
        unsigned long bdi_thresh;
        unsigned long pages_written = 0;
        unsigned long pause = 1;
+       int dirty_exceeded;
        struct backing_dev_info *bdi = mapping->backing_dev_info;
 
        for (;;) {

@@ -510,10 +483,35 @@ static void balance_dirty_pages(struct a
                nr_writeback = global_page_state(NR_WRITEBACK) +
                               global_page_state(NR_WRITEBACK_TEMP);
 
+               /*
+                * In order to avoid the stacked BDI deadlock we need
+                * to ensure we accurately count the 'dirty' pages when
+                * the threshold is low.
+                *
+                * Otherwise it would be possible to get thresh+n pages
+                * reported dirty, even though there are thresh-m pages
+                * actually dirty; with m+n sitting in the percpu
+                * deltas.
+                */
+               if (bdi_thresh < 2*bdi_stat_error(bdi)) {
+                       bdi_nr_reclaimable = bdi_stat_sum(bdi, BDI_RECLAIMABLE);
+                       bdi_nr_writeback = bdi_stat_sum(bdi, BDI_WRITEBACK);
+               } else {
+                       bdi_nr_reclaimable = bdi_stat(bdi, BDI_RECLAIMABLE);
+                       bdi_nr_writeback = bdi_stat(bdi, BDI_WRITEBACK);
+               }
+
+               /*
+                * The bdi thresh is somehow "soft" limit derived from the
+                * global "hard" limit. The former helps to prevent heavy IO
+                * bdi or process from holding back light ones; The latter is
+                * the last resort safeguard.
+                */
+               dirty_exceeded =
+                       (bdi_nr_reclaimable + bdi_nr_writeback >= bdi_thresh)
+                       || (nr_reclaimable + nr_writeback >= dirty_thresh);
 
+               if (!dirty_exceeded)
                        break;
 
                /*

@@ -541,34 +539,10 @@ static void balance_dirty_pages(struct a
                if (bdi_nr_reclaimable > bdi_thresh) {
                        writeback_inodes_wb(&bdi->wb, &wbc);
                        pages_written += write_chunk - wbc.nr_to_write;
                        trace_wbc_balance_dirty_written(&wbc, bdi);
+                       if (pages_written >= write_chunk)
+                               break;          /* We've done our duty */
                }
                trace_wbc_balance_dirty_wait(&wbc, bdi);
                __set_current_state(TASK_INTERRUPTIBLE);
                io_schedule_timeout(pause);

@@ -582,8 +556,7 @@ static void balance_dirty_pages(struct a
                        pause = HZ / 10;
        }
 
+       if (!dirty_exceeded && bdi->dirty_exceeded)
                bdi->dirty_exceeded = 0;
 
        if (writeback_in_progress(bdi))

Quoted 2 lines · On Sun, 11 Jul 2010 10:06:59 +0800 Wu Fengguang <[email protected]> wrote:

+void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty)

Review

Quoted 4 lines

...

(quoted diff) +2 -0

+unsigned long bdi_dirty_limit(struct backing_dev_info *bdi,
+			       unsigned long dirty)

It'd be nice to have some documentation for these things. They're non-static, non-obvious and are stuffed to the gills with secret magic numbers.

Split get_dirty_limits() into global_dirty_limits()+bdi_dirty_limit(), so that the latter can be avoided when under global dirty background threshold (which is the normal state for most systems).

Quoted 4 lines · On Sun, 11 Jul 2010 10:06:59 +0800 Wu Fengguang <[email protected]> wrote:

Split get_dirty_limits() into global_dirty_limits()+bdi_dirty_limit(), so that the latter can be avoided when under global dirty background threshold (which is the normal state for most systems).

mm/page-writeback.c: In function 'balance_dirty_pages_ratelimited_nr':
mm/page-writeback.c:466: warning: 'dirty_exceeded' may be used uninitialized in this function

This was a real bug.

mm/page-writeback.c renamed from mm/page-writeback.c~writeback-avoid-unnecessary-calculation-of-bdi-dirty-thresholds-fix +1 -1

@@ -463,7 +463,7 @@ static void balance_dirty_pages(struct a
 	unsigned long bdi_thresh;
 	unsigned long pages_written = 0;
 	unsigned long pause = 1;
-	int dirty_exceeded;
+	bool dirty_exceeded = false;
 	struct backing_dev_info *bdi = mapping->backing_dev_info;
 
 	for (;;) {

_

Quoted 8 lines · Andrew Morton

Quoted 4 lines · On Sun, 11 Jul 2010 10:06:59 +0800 Wu Fengguang <[email protected]> wrote:

Split get_dirty_limits() into global_dirty_limits()+bdi_dirty_limit(), so that the latter can be avoided when under global dirty background threshold (which is the normal state for most systems).

Quoted 4 lines

mm/page-writeback.c: In function 'balance_dirty_pages_ratelimited_nr':
mm/page-writeback.c:466: warning: 'dirty_exceeded' may be used uninitialized in this function

This was a real bug.

Thanks! But how do you catch this? There are no warnings in my compile test.

plain text document attachment (writeback-less-bdi-calc.patch) Split get_dirty_limits() into global_dirty_limits()+bdi_dirty_limit(), so that the latter can be avoided when under global dirty background threshold (which is the normal state for most systems).

mm/page-writeback.c +8 -0

+unsigned long bdi_dirty_limit(struct backing_dev_info *bdi,
+			       unsigned long dirty)
+{
+	u64 bdi_dirty;
+	long numerator, denominator;
 
+	/*
+	 * Calculate this BDI's share of the dirty ratio.
+	 */

}

Quoted 4 lines · Wu Fengguang

plain text document attachment (writeback-less-bdi-calc.patch) Split get_dirty_limits() into global_dirty_limits()+bdi_dirty_limit(), so that the latter can be avoided when under global dirty background threshold (which is the normal state for most systems).

The patch looks OK, although esp with the proposed comments in the follow up email, bdi_dirty_limit() gets a bit confusing wrt to how and what the limit is.

Maybe its clearer to not call task_dirty_limit() from bdi_dirty_limit(), that way the comment can focus on the device write request completion proportion thing.

Quoted 9 lines

mm/page-writeback.c +8 -0

+unsigned long bdi_dirty_limit(struct backing_dev_info *bdi,
+			       unsigned long dirty)
+{
+	u64 bdi_dirty;
+	long numerator, denominator;
 
+	/*
+	 * Calculate this BDI's share of the dirty ratio.
+	 */

mm/page-writeback.c get_dirty_limits(unsigned long *pbackgro · line 445

Quoted 10 lines

mm/page-writeback.c +8 -0

+	bdi_writeout_fraction(bdi, &numerator, &denominator);
 
+	bdi_dirty = (dirty * (100 - bdi_min_ratio)) / 100;
+	bdi_dirty *= numerator;
+	do_div(bdi_dirty, denominator);
 
+	bdi_dirty += (dirty * bdi->min_ratio) / 100;
+	if (bdi_dirty > (dirty * bdi->max_ratio) / 100)
+		bdi_dirty = dirty * bdi->max_ratio / 100;
+

+ return bdi_dirty;

Quoted 1 line

}

And then add the call to task_dirty_limit() here:

Quoted 5 lines

+++ linux-next/mm/backing-dev.c 2010-07-11 08:53:44.000000000 +0800

mm/backing-dev.c +0 -0

@@ -83,7 +83,8 @@ static int bdi_debug_stats_show(struct s
 		nr_more_io++;
 	spin_unlock(&inode_lock);
 

mm/backing-dev.c static int bdi_debug_stats_show(struct s · line 87

Quoted 3 lines

mm/backing-dev.c +2 -1

-	get_dirty_limits(&background_thresh, &dirty_thresh, &bdi_thresh, bdi);
+	global_dirty_limits(&background_thresh, &dirty_thresh);
+	bdi_thresh = bdi_dirty_limit(bdi, dirty_thresh);

+ bdi_thresh = task_dirty_limit(current, bdi_thresh);

And add a comment to task_dirty_limit() as well, explaining its reason for existence (protecting light/slow dirtying tasks from heavier/fast ones).

Quoted 4 lines · Wu Fengguang

plain text document attachment (writeback-less-bdi-calc.patch) Split get_dirty_limits() into global_dirty_limits()+bdi_dirty_limit(), so that the latter can be avoided when under global dirty background threshold (which is the normal state for most systems).

The patch looks OK, although esp with the proposed comments in the follow up email, bdi_dirty_limit() gets a bit confusing wrt to how and what the limit is.

Maybe its clearer to not call task_dirty_limit() from bdi_dirty_limit(), that way the comment can focus on the device write request completion proportion thing.

Quoted 15 lines

(quoted diff) +12 -0

+unsigned long bdi_dirty_limit(struct backing_dev_info *bdi,
+			       unsigned long dirty)
+{
+	u64 bdi_dirty;
+	long numerator, denominator;
 
+	/*
+	 * Calculate this BDI's share of the dirty ratio.
+	 */
+	bdi_writeout_fraction(bdi, &numerator, &denominator);
 
+	bdi_dirty = (dirty * (100 - bdi_min_ratio)) / 100;
+	bdi_dirty *= numerator;
+	do_div(bdi_dirty, denominator);

Review

Quoted 4 lines

(quoted diff) +4 -0

+	bdi_dirty += (dirty * bdi->min_ratio) / 100;
+	if (bdi_dirty > (dirty * bdi->max_ratio) / 100)
+		bdi_dirty = dirty * bdi->max_ratio / 100;
+

+ return bdi_dirty;

Quoted 1 line

}

And then add the call to task_dirty_limit() here:

Quoted 5 lines

+++ linux-next/mm/backing-dev.c 2010-07-11 08:53:44.000000000 +0800

(quoted diff) +0 -0

@@ -83,7 +83,8 @@ static int bdi_debug_stats_show(struct s
 		nr_more_io++;
 	spin_unlock(&inode_lock);
 

Review static int bdi_debug_stats_show(struct s · line 87

Quoted 3 lines

(quoted diff) +2 -1

-	get_dirty_limits(&background_thresh, &dirty_thresh, &bdi_thresh, bdi);
+	global_dirty_limits(&background_thresh, &dirty_thresh);
+	bdi_thresh = bdi_dirty_limit(bdi, dirty_thresh);

+ bdi_thresh = task_dirty_limit(current, bdi_thresh);

And add a comment to task_dirty_limit() as well, explaining its reason for existence (protecting light/slow dirtying tasks from heavier/fast ones).

Comments updated as below. Any suggestions/corrections?

This avoids delaying writeback for an expired (XFS) inode with lots of dirty pages, but no active dirtier at the moment. Previously we only do that for the kupdate case.

CC Dave Chinner <[email protected]>

CC Christoph Hellwig <[email protected]>

Acked-by Jan Kara <[email protected]>

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

---
 fs/fs-writeback.c |   20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

fs/fs-writeback.c +1 -13

@@ -367,18 +367,7 @@ writeback_single_inode(struct inode *ino
 	spin_lock(&inode_lock);
 	inode->i_state &= ~I_SYNC;
 	if (!(inode->i_state & I_FREEING)) {
-		if ((inode->i_state & I_DIRTY_PAGES) && wbc->for_kupdate) {
-			/*
-			 * More pages get dirtied by a fast dirtier.
-			 */
-			goto select_queue;
-		} else if (inode->i_state & I_DIRTY) {
-			/*
-			 * At least XFS will redirty the inode during the
-			 * writeback (delalloc) and on io completion (isize).
-			 */
-			redirty_tail(inode);
-		} else if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
+		if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
 			/*
 			 * We didn't write back all the pages.  nfs_writepages()
 			 * sometimes bales out without doing anything. Redirty

@@ -400,7 +389,6 @@ writeback_single_inode(struct inode *ino
 				 * soon as the queue becomes uncongested.
 				 */
 				inode->i_state |= I_DIRTY_PAGES;
-select_queue:
 				if (wbc->nr_to_write <= 0) {
 					/*
 					 * slice used up: queue for next turn

Review

Quoted 6 lines

(quoted diff) +6 -0

+		} else if (inode->i_state & I_DIRTY) {
+			/*
+			 * At least XFS will redirty the inode during the
+			 * writeback (delalloc) and on io completion (isize).
+			 */
+			redirty_tail(inode);

I'd drop the mention of XFS here - any filesystem that does delayed allocation or unwritten extent conversion after Io completion will cause this. Perhaps make the comment:

/*
* Filesystems can dirty the inode during writeback
* operations, such as delayed allocation during submission
* or metadata updates after data IO completion. */

Quoted 11 lines

fs/fs-writeback.c select_queue: · line 421

Quoted 6 lines

fs/fs-writeback.c +6 -0

+		} else if (inode->i_state & I_DIRTY) {
+			/*
+			 * At least XFS will redirty the inode during the
+			 * writeback (delalloc) and on io completion (isize).
+			 */
+			redirty_tail(inode);

I'd drop the mention of XFS here - any filesystem that does delayed allocation or unwritten extent conversion after Io completion will cause this. Perhaps make the comment:

Quoted 5 lines

/*
* Filesystems can dirty the inode during writeback
* operations, such as delayed allocation during submission
* or metadata updates after data IO completion. */

Thanks, comments updated accordingly.

---
writeback: don't redirty tail an inode with dirty pages

This avoids delaying writeback for an expired (XFS) inode with lots of dirty pages, but no active dirtier at the moment. Previously we only do that for the kupdate case.

Quoted 22 lines · On Mon, 12 Jul 2010 23:31:27 +0800 Wu Fengguang <[email protected]> wrote:

Quoted 11 lines

fs/fs-writeback.c select_queue: · line 421

Quoted 6 lines

fs/fs-writeback.c +6 -0

+		} else if (inode->i_state & I_DIRTY) {
+			/*
+			 * At least XFS will redirty the inode during the
+			 * writeback (delalloc) and on io completion (isize).
+			 */
+			redirty_tail(inode);

I'd drop the mention of XFS here - any filesystem that does delayed allocation or unwritten extent conversion after Io completion will cause this. Perhaps make the comment:

Quoted 5 lines

/*
* Filesystems can dirty the inode during writeback
* operations, such as delayed allocation during submission
* or metadata updates after data IO completion. */

Thanks, comments updated accordingly.

---
writeback: don't redirty tail an inode with dirty pages

Quoted 4 lines

This avoids delaying writeback for an expired (XFS) inode with lots of dirty pages, but no active dirtier at the moment. Previously we only do that for the kupdate case.

You didn't actually explain the _reason_ for making this change. Please always do that.

The patch is... surprisingly complicated, although the end result looks OK. This is not aided by the partial duplication between mapping_tagged(PAGECACHE_TAG_DIRTY) and I_DIRTY_PAGES. I don't think we can easily remove I_DIRTY_PAGES because it's used for the did-someone-just-dirty-a-page test here.

This code is way too complex and fragile and I fear that anything we do to it will break something :(

This was not a bug, since b_io is empty for kupdate writeback. The next patch will do requeue_io() for non-kupdate writeback, so let's fix it.

CC Dave Chinner <[email protected]>

Cc Martin Bligh <[email protected]>

Cc Michael Rubin <[email protected]>

Cc Peter Zijlstra <[email protected]>

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

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

---
 fs/fs-writeback.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

fs/fs-writeback.c +1 -1

@@ -252,11 +252,14 @@ static void move_expired_inodes(struct l
 }
 
 /*
- * Queue all expired dirty inodes for io, eldest first.
+ * Queue all expired dirty inodes for io, eldest first:

Unify the logic for kupdate and non-kupdate cases.
There won't be starvation because the inodes requeued into b_more_io will later be spliced _after_ the remaining inodes in b_io, hence won't stand in the way of other inodes in the next run.

It avoids unnecessary redirty_tail() calls, hence the update of i_dirtied_when. The timestamp update is undesirable because it could later delay the inode's periodic writeback, or exclude the inode from the data integrity sync operation (which will check timestamp to avoid extra work and livelock).

CC Dave Chinner <[email protected]>

Cc Martin Bligh <[email protected]>

Cc Michael Rubin <[email protected]>

Cc Peter Zijlstra <[email protected]>

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

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

---
 fs/fs-writeback.c |   39 ++++++---------------------------------
 1 file changed, 6 insertions(+), 33 deletions(-)

fs/fs-writeback.c +5 -27

@@ -373,45 +373,18 @@ writeback_single_inode(struct inode *ino
 		if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
 			/*
 			 * We didn't write back all the pages.  nfs_writepages()
-			 * sometimes bales out without doing anything. Redirty
-			 * the inode; Move it from b_io onto b_more_io/b_dirty.
+			 * sometimes bales out without doing anything.
 			 */
-			/*
-			 * akpm: if the caller was the kupdate function we put
-			 * this inode at the head of b_dirty so it gets first
-			 * consideration.  Otherwise, move it to the tail, for
-			 * the reasons described there.  I'm not really sure
-			 * how much sense this makes.  Presumably I had a good
-			 * reasons for doing it this way, and I'd rather not
-			 * muck with it at present.
-			 */
-			if (wbc->for_kupdate) {
+			inode->i_state |= I_DIRTY_PAGES;
+			if (wbc->nr_to_write <= 0) {
 				/*
-				 * For the kupdate function we move the inode
-				 * to b_more_io so it will get more writeout as
-				 * soon as the queue becomes uncongested.
+				 * slice used up: queue for next turn
 				 */
-				inode->i_state |= I_DIRTY_PAGES;
-				if (wbc->nr_to_write <= 0) {
-					/*
-					 * slice used up: queue for next turn
-					 */
-					requeue_io(inode);
-				} else {
-					/*
-					 * somehow blocked: retry later
-					 */
-					redirty_tail(inode);
-				}
+				requeue_io(inode);
 			} else {
 				/*

Quoted 69 lines · Wu Fengguang

Unify the logic for kupdate and non-kupdate cases.
There won't be starvation because the inodes requeued into b_more_io will later be spliced _after_ the remaining inodes in b_io, hence won't stand in the way of other inodes in the next run.

It avoids unnecessary redirty_tail() calls, hence the update of i_dirtied_when. The timestamp update is undesirable because it could later delay the inode's periodic writeback, or exclude the inode from the data integrity sync operation (which will check timestamp to avoid extra work and livelock).

CC Dave Chinner <[email protected]>

Cc Martin Bligh <[email protected]>

Cc Michael Rubin <[email protected]>

Cc Peter Zijlstra <[email protected]>

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

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

---
 fs/fs-writeback.c |   39 ++++++---------------------------------
 1 file changed, 6 insertions(+), 33 deletions(-)

fs/fs-writeback.c +5 -32

@@ -373,45 +373,18 @@ writeback_single_inode(struct inode *ino
 		if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
 			/*
 			 * We didn't write back all the pages.  nfs_writepages()
-			 * sometimes bales out without doing anything. Redirty
-			 * the inode; Move it from b_io onto b_more_io/b_dirty.
+			 * sometimes bales out without doing anything.
 			 */
-			/*
-			 * akpm: if the caller was the kupdate function we put
-			 * this inode at the head of b_dirty so it gets first
-			 * consideration.  Otherwise, move it to the tail, for
-			 * the reasons described there.  I'm not really sure
-			 * how much sense this makes.  Presumably I had a good
-			 * reasons for doing it this way, and I'd rather not
-			 * muck with it at present.
-			 */
-			if (wbc->for_kupdate) {
+			inode->i_state |= I_DIRTY_PAGES;
+			if (wbc->nr_to_write <= 0) {
 				/*
-				 * For the kupdate function we move the inode
-				 * to b_more_io so it will get more writeout as
-				 * soon as the queue becomes uncongested.
+				 * slice used up: queue for next turn
 				 */
-				inode->i_state |= I_DIRTY_PAGES;
-				if (wbc->nr_to_write <= 0) {
-					/*
-					 * slice used up: queue for next turn
-					 */
-					requeue_io(inode);
-				} else {
-					/*
-					 * somehow blocked: retry later
-					 */
-					redirty_tail(inode);
-				}
+				requeue_io(inode);
 			} else {
 				/*
-				 * Otherwise fully redirty the inode so that
-				 * other inodes on this superblock will get some
-				 * writeout.  Otherwise heavy writing to one
-				 * file would indefinitely suspend writeout of
-				 * all the other files.

fs/fs-writeback.c writeback_single_inode(struct inode *ino · line 389

Quoted 5 lines

fs/fs-writeback.c +1 -1

+				 * somehow blocked: retry later
 				 */
-				inode->i_state |= I_DIRTY_PAGES;
 				redirty_tail(inode);
 			}

This means that congestion will always trigger redirty_tail(). Is that really what we want for that case?

Also, I'd prefer that the
comments remain somewhat more descriptive of the circumstances that we are operating under. Comments like "retry later to avoid blocking writeback of other inodes" is far, far better than "retry later" because it has "why" component that explains the reason for the logic. You may remember why, but I sure won't in a few months time....

Quoted 39 lines · Dave Chinner

Quoted 38 lines · Wu Fengguang

fs/fs-writeback.c +4 -30

-			/*
-			 * akpm: if the caller was the kupdate function we put
-			 * this inode at the head of b_dirty so it gets first
-			 * consideration.  Otherwise, move it to the tail, for
-			 * the reasons described there.  I'm not really sure
-			 * how much sense this makes.  Presumably I had a good
-			 * reasons for doing it this way, and I'd rather not
-			 * muck with it at present.
-			 */
-			if (wbc->for_kupdate) {
+			inode->i_state |= I_DIRTY_PAGES;
+			if (wbc->nr_to_write <= 0) {
 				/*
-				 * For the kupdate function we move the inode
-				 * to b_more_io so it will get more writeout as
-				 * soon as the queue becomes uncongested.
+				 * slice used up: queue for next turn
 				 */
-				inode->i_state |= I_DIRTY_PAGES;
-				if (wbc->nr_to_write <= 0) {
-					/*
-					 * slice used up: queue for next turn
-					 */
-					requeue_io(inode);
-				} else {
-					/*
-					 * somehow blocked: retry later
-					 */
-					redirty_tail(inode);
-				}
+				requeue_io(inode);
 			} else {
 				/*
-				 * Otherwise fully redirty the inode so that
-				 * other inodes on this superblock will get some
-				 * writeout.  Otherwise heavy writing to one
-				 * file would indefinitely suspend writeout of
-				 * all the other files.

Quoted 8 lines

fs/fs-writeback.c writeback_single_inode(struct inode *ino · line 389

Quoted 5 lines

fs/fs-writeback.c +1 -1

+				 * somehow blocked: retry later
 				 */
-				inode->i_state |= I_DIRTY_PAGES;
 				redirty_tail(inode);
 			}

This means that congestion will always trigger redirty_tail(). Is that really what we want for that case?

This patch actually converts some redirty_tail() cases to use requeue_io(), so are reducing the use of redirty_tail(). Also recent kernels are blocked _inside_ get_request() on congestion instead of returning to writeback_single_inode() on congestion. So the "somehow blocked" comment for redirty_tail() no longer includes the congestion case.

Quoted 6 lines

Also, I'd prefer that the
comments remain somewhat more descriptive of the circumstances that we are operating under. Comments like "retry later to avoid blocking writeback of other inodes" is far, far better than "retry later" because it has "why" component that explains the reason for the logic. You may remember why, but I sure won't in a few months time....

Ah yes the comment is too simple. However the redirty_tail() is not to avoid blocking writeback of other inodes, but to avoid eating 100% CPU on busy retrying a dirty inode/page that cannot perform writeback for a while. (In theory redirty_tail() can still busy retry though, when there is only one single dirty inode.) So how about

        /*
         * somehow blocked: avoid busy retrying
         */

Quoted 6 lines

Also, I'd prefer that the
comments remain somewhat more descriptive of the circumstances that we are operating under. Comments like "retry later to avoid blocking writeback of other inodes" is far, far better than "retry later" because it has "why" component that explains the reason for the logic. You may remember why, but I sure won't in a few months time....

Ah yes the comment is too simple. However the redirty_tail() is not to avoid blocking writeback of other inodes, but to avoid eating 100% CPU on busy retrying a dirty inode/page that cannot perform writeback for a while. (In theory redirty_tail() can still busy retry though, when there is only one single dirty inode.) So how about

        /*
         * somehow blocked: avoid busy retrying
         */

Quoted 6 lines · On Mon, 12 Jul 2010 23:52:39 +0800 Wu Fengguang <[email protected]> wrote:

Quoted 6 lines

Also, I'd prefer that the
comments remain somewhat more descriptive of the circumstances that we are operating under. Comments like "retry later to avoid blocking writeback of other inodes" is far, far better than "retry later" because it has "why" component that explains the reason for the logic. You may remember why, but I sure won't in a few months time....

me2 (of course). This code is waaaay too complex to be scrimping on comments.

Quoted 6 lines

Ah yes the comment is too simple. However the redirty_tail() is not to avoid blocking writeback of other inodes, but to avoid eating 100% CPU on busy retrying a dirty inode/page that cannot perform writeback for a while. (In theory redirty_tail() can still busy retry though, when there is only one single dirty inode.) So how about

Quoted 3 lines

        /*
         * somehow blocked: avoid busy retrying
         */

That's much too short. Expand on the "somehow" - provide an example, describe the common/expected cause. Fully explain what the "busy" retry _is_ and how it can come about.

Andrew,

Here are some writeback cleanups to avoid unnecessary calculation overheads, and relative simple bug fixes.

The patch applies to latest linux-next tree. The mmotm tree will need rebase to include commit 32422c79 (writeback: Add tracing to balance_dirty_pages) in order to avoid merge conflicts.

Quoted 5 lines · Wu Fengguang

Andrew,

Here are some writeback cleanups to avoid unnecessary calculation overheads, and relative simple bug fixes.

Quoted 3 lines

The patch applies to latest linux-next tree. The mmotm tree will need rebase to include commit 32422c79 (writeback: Add tracing to balance_dirty_pages) in order to avoid merge conflicts.

Maybe it's a better idea to get them in through Jens' tree? At least he has handled all my flusher thread patches, and I have another big series in preparation which might cause some interesting conflicts if not merged through the same tree.