pageout work and dirty reclaim throttling

From: Greg Thelen <[email protected]>

Add additional flags to page_cgroup to track dirty pages within a mem_cgroup.

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

Signed-off-by Andrea Righi <[email protected]>

Signed-off-by Greg Thelen <[email protected]>

Reviewed-by Minchan Kim <[email protected]>

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

---
 include/linux/page_cgroup.h |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

include/linux/page_cgroup.h +16 -0

@@ -10,6 +10,9 @@ enum {
 	/* flags for mem_cgroup and file and I/O status */
 	PCG_MOVE_LOCK, /* For race between move_account v.s. following bits */
 	PCG_FILE_MAPPED, /* page is accounted as "mapped" */
+	PCG_FILE_DIRTY, /* page is dirty */
+	PCG_FILE_WRITEBACK, /* page is under writeback */
+	PCG_FILE_UNSTABLE_NFS, /* page is NFS unstable */
 	__NR_PCG_FLAGS,
 };
 

@@ -64,6 +67,10 @@ static inline void ClearPageCgroup##unam
 static inline int TestClearPageCgroup##uname(struct page_cgroup *pc)	\
 	{ return test_and_clear_bit(PCG_##lname, &pc->flags);  }
 
+#define TESTSETPCGFLAG(uname, lname)			\
+static inline int TestSetPageCgroup##uname(struct page_cgroup *pc)	\
+	{ return test_and_set_bit(PCG_##lname, &pc->flags); }
+
 /* Cache flag is set only once (at allocation) */
 TESTPCGFLAG(Cache, CACHE)
 CLEARPCGFLAG(Cache, CACHE)

@@ -77,6 +84,22 @@ SETPCGFLAG(FileMapped, FILE_MAPPED)
 CLEARPCGFLAG(FileMapped, FILE_MAPPED)
 TESTPCGFLAG(FileMapped, FILE_MAPPED)
 
+SETPCGFLAG(FileDirty, FILE_DIRTY)
+CLEARPCGFLAG(FileDirty, FILE_DIRTY)
+TESTPCGFLAG(FileDirty, FILE_DIRTY)
+TESTCLEARPCGFLAG(FileDirty, FILE_DIRTY)
+TESTSETPCGFLAG(FileDirty, FILE_DIRTY)
+
+SETPCGFLAG(FileWriteback, FILE_WRITEBACK)
+CLEARPCGFLAG(FileWriteback, FILE_WRITEBACK)
+TESTPCGFLAG(FileWriteback, FILE_WRITEBACK)

Quoted 5 lines · On Tue, 28 Feb 2012 22:00:23 +0800 Fengguang Wu <[email protected]> wrote:

From: Greg Thelen <[email protected]>

Add additional flags to page_cgroup to track dirty pages within a mem_cgroup.

Quoted 5 lines

Signed-off-by KAMEZAWA Hiroyuki <[email protected]>

Signed-off-by Andrea Righi <[email protected]>

Signed-off-by Greg Thelen <[email protected]>

Reviewed-by Minchan Kim <[email protected]>

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

I'm sorry but I changed the design of page_cgroup's flags update and never want to add new flags (I'd like to remove page_cgroup->flags.)

Please see linux-next.

A good example is PCG_FILE_MAPPED, which I removed.

memcg: use new logic for page stat accounting
memcg: remove PCG_FILE_MAPPED

You can make use of PageDirty() and PageWriteback() instead of new flags.. (I hope.)

Quoted 45 lines

---
 include/linux/page_cgroup.h |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

include/linux/page_cgroup.h +21 -0

@@ -10,6 +10,9 @@ enum {
 	/* flags for mem_cgroup and file and I/O status */
 	PCG_MOVE_LOCK, /* For race between move_account v.s. following bits */
 	PCG_FILE_MAPPED, /* page is accounted as "mapped" */
+	PCG_FILE_DIRTY, /* page is dirty */
+	PCG_FILE_WRITEBACK, /* page is under writeback */
+	PCG_FILE_UNSTABLE_NFS, /* page is NFS unstable */
 	__NR_PCG_FLAGS,
 };
 

@@ -64,6 +67,10 @@ static inline void ClearPageCgroup##unam
 static inline int TestClearPageCgroup##uname(struct page_cgroup *pc)	\
 	{ return test_and_clear_bit(PCG_##lname, &pc->flags);  }
 
+#define TESTSETPCGFLAG(uname, lname)			\
+static inline int TestSetPageCgroup##uname(struct page_cgroup *pc)	\
+	{ return test_and_set_bit(PCG_##lname, &pc->flags); }
+
 /* Cache flag is set only once (at allocation) */
 TESTPCGFLAG(Cache, CACHE)
 CLEARPCGFLAG(Cache, CACHE)

@@ -77,6 +84,22 @@ SETPCGFLAG(FileMapped, FILE_MAPPED)
 CLEARPCGFLAG(FileMapped, FILE_MAPPED)
 TESTPCGFLAG(FileMapped, FILE_MAPPED)
 
+SETPCGFLAG(FileDirty, FILE_DIRTY)
+CLEARPCGFLAG(FileDirty, FILE_DIRTY)
+TESTPCGFLAG(FileDirty, FILE_DIRTY)
+TESTCLEARPCGFLAG(FileDirty, FILE_DIRTY)
+TESTSETPCGFLAG(FileDirty, FILE_DIRTY)
+
+SETPCGFLAG(FileWriteback, FILE_WRITEBACK)
+CLEARPCGFLAG(FileWriteback, FILE_WRITEBACK)
+TESTPCGFLAG(FileWriteback, FILE_WRITEBACK)
+
+SETPCGFLAG(FileUnstableNFS, FILE_UNSTABLE_NFS)
+CLEARPCGFLAG(FileUnstableNFS, FILE_UNSTABLE_NFS)
+TESTPCGFLAG(FileUnstableNFS, FILE_UNSTABLE_NFS)
+TESTCLEARPCGFLAG(FileUnstableNFS, FILE_UNSTABLE_NFS)

include/linux/page_cgroup.h SETPCGFLAG(FileMapped, FILE_MAPPED) · line 105

Quoted 8 lines

include/linux/page_cgroup.h +2 -0

+TESTSETPCGFLAG(FileUnstableNFS, FILE_UNSTABLE_NFS)
+
 SETPCGFLAG(Migration, MIGRATION)
 CLEARPCGFLAG(Migration, MIGRATION)
 TESTPCGFLAG(Migration, MIGRATION)

From: Greg Thelen <[email protected]>

Add memcg routines to count dirty, writeback, and unstable_NFS pages. These routines are not yet used by the kernel to count such pages. A later change adds kernel calls to these new routines.

As inode pages are marked dirty, if the dirtied page's cgroup differs from the inode's cgroup, then mark the inode shared across several cgroup.

...

mm/memcontrol.c +0 -0

@@ -1885,6 +1888,44 @@ void mem_cgroup_update_page_stat(struct 
 			ClearPageCgroupFileMapped(pc);
 		idx = MEM_CGROUP_STAT_FILE_MAPPED;
 		break;

...

Quoted 22 lines · On Tue, 28 Feb 2012 22:00:24 +0800 Fengguang Wu <[email protected]> wrote:

From: Greg Thelen <[email protected]>

Add memcg routines to count dirty, writeback, and unstable_NFS pages. These routines are not yet used by the kernel to count such pages. A later change adds kernel calls to these new routines.

As inode pages are marked dirty, if the dirtied page's cgroup differs from the inode's cgroup, then mark the inode shared across several cgroup.

...

mm/memcontrol.c +6 -0

@@ -1885,6 +1888,44 @@ void mem_cgroup_update_page_stat(struct 
 			ClearPageCgroupFileMapped(pc);
 		idx = MEM_CGROUP_STAT_FILE_MAPPED;
 		break;
+
+	case MEMCG_NR_FILE_DIRTY:
+		/* Use Test{Set,Clear} to only un/charge the memcg once. */
+		if (val > 0) {
+			if (TestSetPageCgroupFileDirty(pc))
+				val = 0;

mm/memcontrol.c void mem_cgroup_update_page_stat(struct · line 1900

Quoted 4 lines

mm/memcontrol.c +4 -0

+		} else {
+			if (!TestClearPageCgroupFileDirty(pc))
+				val = 0;
+		}

Made me scratch my head for a while, but I see now that the `val' arg to (the undocumented) mem_cgroup_update_page_stat() can only ever have the values 1 or -1. I hope.

From: Greg Thelen <[email protected]>

Add calls into memcg dirty page accounting. Notify memcg when pages transition between clean, file dirty, writeback, and unstable nfs. This allows the memory controller to maintain an accurate view of the amount of its memory that is dirty.

Signed-off-by Greg Thelen <[email protected]>

Signed-off-by Andrea Righi <[email protected]>

Acked-by KAMEZAWA Hiroyuki <[email protected]>

Reviewed-by Daisuke Nishimura <[email protected]>

Reviewed-by Minchan Kim <[email protected]>

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

---
 fs/nfs/write.c      |    4 ++++
 mm/filemap.c        |    1 +
 mm/page-writeback.c |    4 ++++
 mm/truncate.c       |    1 +
 4 files changed, 10 insertions(+)

fs/nfs/write.c +3 -0

 	inc_zone_page_state(req->wb_page, NR_UNSTABLE_NFS);
 	inc_bdi_stat(req->wb_page->mapping->backing_dev_info, BDI_RECLAIMABLE);
 	__mark_inode_dirty(inode, I_DIRTY_DATASYNC);

@@ -460,6 +461,7 @@ nfs_clear_request_commit(struct nfs_page
 	struct page *page = req->wb_page;
 
 	if (test_and_clear_bit(PG_CLEAN, &(req)->wb_flags)) {
+		mem_cgroup_dec_page_stat(page, MEMCG_NR_FILE_UNSTABLE_NFS);
 		dec_zone_page_state(page, NR_UNSTABLE_NFS);
 		dec_bdi_stat(page->mapping->backing_dev_info, BDI_RECLAIMABLE);
 		return 1;

@@ -1408,6 +1410,8 @@ void nfs_retry_commit(struct list_head *
 		req = nfs_list_entry(page_list->next);
 		nfs_list_remove_request(req);
 		nfs_mark_request_commit(req, lseg);
+		mem_cgroup_dec_page_stat(req->wb_page,
+					 MEMCG_NR_FILE_UNSTABLE_NFS);
 		dec_zone_page_state(req->wb_page, NR_UNSTABLE_NFS);
 		dec_bdi_stat(req->wb_page->mapping->backing_dev_info,
 			     BDI_RECLAIMABLE);

From: Greg Thelen <[email protected]>

Added memcg dirty page accounting support routines. These routines are used by later changes to provide memcg aware writeback and dirty page limiting. A mem_cgroup_dirty_info() tracepoint is is also included to allow for easier understanding of memcg writeback operation.

From: Greg Thelen <[email protected]>

Added memcg dirty page accounting support routines. These routines are used by later changes to provide memcg aware writeback and dirty page limiting. A mem_cgroup_dirty_info() tracepoint is is also included to allow for easier understanding of memcg writeback operation.

...

+/*
+ * Return the number of additional pages that the @memcg cgroup could allocate.
+ * If use_hierarchy is set, then this involves checking parent mem cgroups to
+ * find the cgroup with the smallest free space.
+ */

mm/memcontrol.c +35 -0

+static unsigned long
+mem_cgroup_hierarchical_free_pages(struct mem_cgroup *memcg)
+{
+	u64 free;
+	unsigned long min_free;
+
+	min_free = global_page_state(NR_FREE_PAGES);
+
+	while (memcg) {
+		free = mem_cgroup_margin(memcg);
+		min_free = min_t(u64, min_free, free);
+		memcg = parent_mem_cgroup(memcg);
+	}
+
+	return min_free;
+}
+
+/*
+ * mem_cgroup_page_stat() - get memory cgroup file cache statistics
+ * @memcg:     memory cgroup to query
+ * @item:      memory statistic item exported to the kernel
+ *
+ * Return the accounted statistic value.
+ */
+unsigned long mem_cgroup_page_stat(struct mem_cgroup *memcg,
+				   enum mem_cgroup_page_stat_item item)
+{
+	struct mem_cgroup *iter;
+	s64 value;
+
+	/*
+	 * If we're looking for dirtyable pages we need to evaluate free pages
+	 * depending on the limit and usage of the parents first of all.
+	 */
+	if (item == MEMCG_NR_DIRTYABLE_PAGES)

Quoted 7 lines · On Tue, 28 Feb 2012 22:00:26 +0800 Fengguang Wu <[email protected]> wrote:

From: Greg Thelen <[email protected]>

Added memcg dirty page accounting support routines. These routines are used by later changes to provide memcg aware writeback and dirty page limiting. A mem_cgroup_dirty_info() tracepoint is is also included to allow for easier understanding of memcg writeback operation.

Quoted 7 lines

...

+/*
+ * Return the number of additional pages that the @memcg cgroup could allocate.
+ * If use_hierarchy is set, then this involves checking parent mem cgroups to
+ * find the cgroup with the smallest free space.
+ */

Comment needs revisting - use_hierarchy does not exist.

Quoted 40 lines

mm/memcontrol.c +40 -0

+static unsigned long
+mem_cgroup_hierarchical_free_pages(struct mem_cgroup *memcg)
+{
+	u64 free;
+	unsigned long min_free;
+
+	min_free = global_page_state(NR_FREE_PAGES);
+
+	while (memcg) {
+		free = mem_cgroup_margin(memcg);
+		min_free = min_t(u64, min_free, free);
+		memcg = parent_mem_cgroup(memcg);
+	}
+
+	return min_free;
+}
+
+/*
+ * mem_cgroup_page_stat() - get memory cgroup file cache statistics
+ * @memcg:     memory cgroup to query
+ * @item:      memory statistic item exported to the kernel
+ *
+ * Return the accounted statistic value.
+ */
+unsigned long mem_cgroup_page_stat(struct mem_cgroup *memcg,
+				   enum mem_cgroup_page_stat_item item)
+{
+	struct mem_cgroup *iter;
+	s64 value;
+
+	/*
+	 * If we're looking for dirtyable pages we need to evaluate free pages
+	 * depending on the limit and usage of the parents first of all.
+	 */
+	if (item == MEMCG_NR_DIRTYABLE_PAGES)
+		value = mem_cgroup_hierarchical_free_pages(memcg);
+	else
+		value = 0;
+
+	/*

mm/memcontrol.c int mem_cgroup_swappiness(struct mem_cgr · line 1351

Quoted 5 lines

mm/memcontrol.c +5 -0

+	 * Recursively evaluate page statistics against all cgroup under
+	 * hierarchy tree
+	 */
+	for_each_mem_cgroup_tree(iter, memcg)
+		value += mem_cgroup_local_page_stat(iter, item);

What's the locking rule for for_each_mem_cgroup_tree()? It's unobvious from the code and isn't documented?

This relays file pageout IOs to the flusher threads.

It's much more important now that page reclaim generally does not writeout filesystem-backed pages.

The ultimate target is to gracefully handle the LRU lists pressured by dirty/writeback pages. In particular, problems (1-2) are addressed here.

1) I/O efficiency

The flusher will piggy back the nearby ~10ms worth of dirty pages for I/O.

This takes advantage of the time/spacial locality in most workloads: the nearby pages of one file are typically populated into the LRU at the same time, hence will likely be close to each other in the LRU list. Writing them in one shot helps clean more pages effectively for page reclaim.

For the common dd style sequential writes that have excellent locality, up to ~80ms data will be wrote around by the pageout work, which helps make I/O performance very close to that of the background writeback.

2) writeback work coordinations

To avoid memory allocations at page reclaim, a mempool for struct wb_writeback_work is created.

wakeup_flusher_threads() is removed because it can easily delay the more oriented pageout works and even exhaust the mempool reservations. It's also found to not I/O efficient by frequently submitting writeback works with small ->nr_pages.

Background/periodic works will quit automatically, so as to clean the pages under reclaim ASAP.

However for now the sync work can still block
us for long time.

Jan Kara: limit the search scope; remove works and unpin inodes on umount.

TODO: the pageout works may be starved by the sync work and maybe others. Need a proper way to guarantee fairness.

CC Jan Kara <[email protected]>

CC Mel Gorman <[email protected]>

Acked-by Rik van Riel <[email protected]>

CC Greg Thelen <[email protected]>

CC Minchan Kim <[email protected]>

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

---
 fs/fs-writeback.c                |  230 +++++++++++++++++++++++++++--
 fs/super.c                       |    1 
 include/linux/backing-dev.h      |    2 
 include/linux/writeback.h        |   16 +-
 include/trace/events/writeback.h |   12 +
 mm/vmscan.c                      |   36 ++--
 6 files changed, 268 insertions(+), 29 deletions(-)

fs/fs-writeback.c +9 -0

 	if (!work) {
 		if (bdi->wb.task) {
 			trace_writeback_nowork(bdi);

@@ -138,6 +161,7 @@ __bdi_start_writeback(struct backing_dev
 		return;
 	}
 
+	memset(work, 0, sizeof(*work));
 	work->sync_mode	= WB_SYNC_NONE;
 	work->nr_pages	= nr_pages;
 	work->range_cyclic = range_cyclic;

@@ -187,6 +211,181 @@ void bdi_start_background_writeback(stru
 }
 
 /*
+ * Check if @work already covers @offset, or try to extend it to cover @offset.
+ * Returns true if the wb_writeback_work now encompasses the requested offset.
+ */
+static bool extend_writeback_range(struct wb_writeback_work *work,
+				   pgoff_t offset,
+				   unsigned long unit)
+{
+	pgoff_t end = work->offset + work->nr_pages;

fs/fs-writeback.c +23 -0

+	/* the unsigned comparison helps eliminate one compare */
+	if (work->offset - offset < unit) {
+		work->nr_pages += unit;
+		work->offset -= unit;
+		return true;
+	}
+
+	if (offset - end < unit) {
+		work->nr_pages += unit;
+		return true;
+	}
+
+	return false;
+}
+
+/*
+ * schedule writeback on a range of inode pages.
+ */
+static struct wb_writeback_work *
+alloc_queue_pageout_work(struct backing_dev_info *bdi,
+			 struct inode *inode,
+			 pgoff_t offset,
+			 pgoff_t len)

fs/fs-writeback.c +28 -0

+	work = mempool_alloc(wb_work_mempool, GFP_NOWAIT);
+	if (!work) {
+		trace_printk("wb_work_mempool alloc fail\n");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	memset(work, 0, sizeof(*work));
+	work->sync_mode		= WB_SYNC_NONE;
+	work->inode		= inode;
+	work->offset		= offset;
+	work->nr_pages		= len;
+	work->reason		= WB_REASON_PAGEOUT;
+
+	bdi_queue_work(bdi, work);
+
+	return work;
+}
+
+/*
+ * Called by page reclaim code to flush the dirty page ASAP. Do write-around to
+ * improve IO throughput. The nearby pages will have good chance to reside in
+ * the same LRU list that vmscan is working on, and even close to each other
+ * inside the LRU list in the common case of sequential read/write.
+ *
+ * ret > 0: success, allocated/queued a new pageout work;
+ *	    there are at least @ret writeback works queued now
+ * ret = 0: success, reused/extended a previous pageout work
+ * ret < 0: failed

fs/fs-writeback.c +6 -0

+	i = 1;
+	spin_lock_bh(&bdi->wb_lock);
+	list_for_each_entry_reverse(work, &bdi->work_list, list) {
+		if (work->inode != inode)
+			continue;
+		if (extend_writeback_range(work, offset, write_around_pages)) {

fs/fs-writeback.c +14 -0

+	/*
+	 * Notify the caller of completion if this is a synchronous
+	 * work item, otherwise just free it.
+	 */
+	if (work->done)
+		complete(work->done);
+	else
+		mempool_free(work, wb_work_mempool);
+}
+
+/*
+ * Remove works for @sb; or if (@sb == NULL), remove all works on @bdi.
+ */
+void bdi_remove_writeback_works(struct backing_dev_info *bdi,

fs/fs-writeback.c +3 -0

+}
+
+/*
  * Remove the inode from the writeback list it is on.
  */
 void inode_wb_list_del(struct inode *inode)

fs/fs-writeback.c +5 -9

 static long wb_check_background_flush(struct bdi_writeback *wb)
 {
 	if (over_bground_thresh(wb->bdi)) {

@@ -905,16 +1119,12 @@ long wb_do_writeback(struct bdi_writebac
 
 		trace_writeback_exec(bdi, work);
 
-		wrote += wb_writeback(wb, work);
-
-		/*
-		 * Notify the caller of completion if this is a synchronous
-		 * work item, otherwise just free it.
-		 */
-		if (work->done)
-			complete(work->done);
+		if (!work->inode)
+			wrote += wb_writeback(wb, work);
 		else
-			kfree(work);
+			wrote += wb_pageout(wb, work);
+
+		wb_free_work(work);
 	}
 
 	/*
 
...

include/linux/backing-dev.h +2 -0

@@ -126,6 +126,8 @@ int bdi_has_dirty_io(struct backing_dev_
 void bdi_arm_supers_timer(void);
 void bdi_wakeup_thread_delayed(struct backing_dev_info *bdi);
 void bdi_lock_two(struct bdi_writeback *wb1, struct bdi_writeback *wb2);
+void bdi_remove_writeback_works(struct backing_dev_info *bdi,
+				struct super_block *sb);
 
 extern spinlock_t bdi_lock;
 extern struct list_head bdi_list;

mm/vmscan.c +15 -5

@@ -874,12 +874,22 @@ static unsigned long shrink_page_list(st
 			nr_dirty++;
 
 			/*
-			 * Only kswapd can writeback filesystem pages to
-			 * avoid risk of stack overflow but do not writeback
-			 * unless under significant pressure.
+			 * Pages may be dirtied anywhere inside the LRU. This
+			 * ensures they undergo a full period of LRU iteration
+			 * before considering pageout. The intention is to
+			 * delay writeout to the flusher thread, unless when
+			 * run into a long segment of dirty pages.
+			 */
+			if (references == PAGEREF_RECLAIM_CLEAN &&
+			    priority == DEF_PRIORITY)
+				goto keep_locked;
+
+			/*
+			 * Try relaying the pageout I/O to the flusher threads
+			 * for better I/O efficiency and avoid stack overflow.
 			 */
-			if (page_is_file_cache(page) &&
-					(!current_is_kswapd() || priority >= DEF_PRIORITY - 2)) {
+			if (page_is_file_cache(page) && mapping &&
+			    queue_pageout_work(mapping, page) >= 0) {
 				/*
 				 * Immediately reclaim when written back.
 				 * Similar in principal to deactivate_page()

Quoted 4 lines · On Tue, 28 Feb 2012 22:00:27 +0800 Fengguang Wu <[email protected]> wrote:

This relays file pageout IOs to the flusher threads.

It's much more important now that page reclaim generally does not writeout filesystem-backed pages.

It doesn't? We still do writeback in direct reclaim. This claim should be fleshed out rather a lot, please.

Quoted 7 lines

The ultimate target is to gracefully handle the LRU lists pressured by dirty/writeback pages. In particular, problems (1-2) are addressed here.

1) I/O efficiency

The flusher will piggy back the nearby ~10ms worth of dirty pages for I/O.

Quoted 4 lines

This takes advantage of the time/spacial locality in most workloads: the nearby pages of one file are typically populated into the LRU at the same time, hence will likely be close to each other in the LRU list. Writing them in one shot helps clean more pages effectively for page reclaim.

Yes, this is often true. But when adjacent pages from the same file are clustered together on the LRU, direct reclaim's LRU-based walk will also provide good I/O patterns.

Quoted 9 lines

For the common dd style sequential writes that have excellent locality, up to ~80ms data will be wrote around by the pageout work, which helps make I/O performance very close to that of the background writeback.

2) writeback work coordinations

To avoid memory allocations at page reclaim, a mempool for struct wb_writeback_work is created.

Quoted 4 lines

wakeup_flusher_threads() is removed because it can easily delay the more oriented pageout works and even exhaust the mempool reservations. It's also found to not I/O efficient by frequently submitting writeback works with small ->nr_pages.

The last sentence here needs help.

Quoted 2 lines

Background/periodic works will quit automatically, so as to clean the pages under reclaim ASAP.

I don't know what this means. How does a work "quit automatically" and why does that initiate I/O?

Quoted 2 lines

However for now the sync work can still block
us for long time.

Please define the term "sync work".

Quoted 4 lines

Jan Kara: limit the search scope; remove works and unpin inodes on umount.

TODO: the pageout works may be starved by the sync work and maybe others. Need a proper way to guarantee fairness.

Quoted 9 lines · On Tue, 28 Feb 2012 22:00:27 +0800 Fengguang Wu <[email protected]> wrote:

---
 fs/fs-writeback.c                |  230 +++++++++++++++++++++++++++--
 fs/super.c                       |    1 
 include/linux/backing-dev.h      |    2 
 include/linux/writeback.h        |   16 +-
 include/trace/events/writeback.h |   12 +
 mm/vmscan.c                      |   36 ++--
 6 files changed, 268 insertions(+), 29 deletions(-)

fs/fs-writeback.c struct wb_writeback_work { · line 45

Quoted 8 lines

fs/fs-writeback.c +2 -0

@@ -41,6 +41,8 @@ struct wb_writeback_work {
 	long nr_pages;
 	struct super_block *sb;
 	unsigned long *older_than_this;
+	struct inode *inode;
+	pgoff_t offset;

Please document `offset' here. What is it used for?

Quoted 7 lines

fs/fs-writeback.c +0 -0

 	enum writeback_sync_modes sync_mode;
 	unsigned int tagged_writepages:1;
 	unsigned int for_kupdate:1;

@@ -57,6 +59,27 @@ struct wb_writeback_work {
  */
 int nr_pdflush_threads;
 

fs/fs-writeback.c struct wb_writeback_work { · line 64

Quoted 3 lines

fs/fs-writeback.c +3 -0

+static mempool_t *wb_work_mempool;
+
+static void *wb_work_alloc(gfp_t gfp_mask, void *pool_data)

The gfp_mask is traditionally the last function argument.

fs/fs-writeback.c struct wb_writeback_work { · line 70

Quoted 6 lines

fs/fs-writeback.c +6 -0

+{
+	/*
+	 * alloc_queue_pageout_work() will be called on page reclaim
+	 */
+	if (current->flags & PF_MEMALLOC)
+		return NULL;

Do we need to test current->flags here?

Could we have checked !(gfp_mask & __GFP_IO) and/or __GFP_FILE?

I'm not really suggsting such a change - just trying to get my head around how this stuff works..

fs/fs-writeback.c struct wb_writeback_work { · line 80

Quoted 10 lines

fs/fs-writeback.c +10 -0

+	return kmalloc(sizeof(struct wb_writeback_work), gfp_mask);
+}
+
+static __init int wb_work_init(void)
+{
+	wb_work_mempool = mempool_create(WB_WORK_MEMPOOL_SIZE,
+					 wb_work_alloc, mempool_kfree, NULL);
+	return wb_work_mempool ? 0 : -ENOMEM;
+}
+fs_initcall(wb_work_init);

Please provide a description of the wb_writeback_work lifecycle: when they are allocated, when they are freed, how we ensure that a finite number are in flight.

Also, when a mempool_alloc() caller is waiting for wb_writeback_works to be freed, how are we dead certain that some *will* be freed? It means that the mempool_alloc() caller cannot be holding any explicit or implicit locks which would prevent wb_writeback_works would be freed. Where "implicit lock" means things like being inside ext3/4 journal_start.

This stuff is tricky and is hard to get right. Reviewing its correctness by staring at a patch is difficult.

fs/fs-writeback.c __bdi_start_writeback(struct backing_dev · line 155

Quoted 9 lines

fs/fs-writeback.c +1 -1

 /**
  * writeback_in_progress - determine whether there is writeback in progress
  * @bdi: the device's backing_dev_info structure.

@@ -129,7 +152,7 @@ __bdi_start_writeback(struct backing_dev
 	 * This is WB_SYNC_NONE writeback, so if allocation fails just
 	 * wakeup the thread for old dirty data writeback
 	 */
-	work = kzalloc(sizeof(*work), GFP_ATOMIC);
+	work = mempool_alloc(wb_work_mempool, GFP_NOWAIT);

Sneaky change from GFP_ATOMIC to GFP_NOWAIT is significant, but undescribed?

Quoted 27 lines

fs/fs-writeback.c +13 -0

 	if (!work) {
 		if (bdi->wb.task) {
 			trace_writeback_nowork(bdi);

@@ -138,6 +161,7 @@ __bdi_start_writeback(struct backing_dev
 		return;
 	}
 
+	memset(work, 0, sizeof(*work));
 	work->sync_mode	= WB_SYNC_NONE;
 	work->nr_pages	= nr_pages;
 	work->range_cyclic = range_cyclic;

@@ -187,6 +211,181 @@ void bdi_start_background_writeback(stru
 }
 
 /*
+ * Check if @work already covers @offset, or try to extend it to cover @offset.
+ * Returns true if the wb_writeback_work now encompasses the requested offset.
+ */
+static bool extend_writeback_range(struct wb_writeback_work *work,
+				   pgoff_t offset,
+				   unsigned long unit)
+{
+	pgoff_t end = work->offset + work->nr_pages;
+
+	if (offset >= work->offset && offset < end)
+		return true;
+

fs/fs-writeback.c void bdi_start_background_writeback(stru · line 231

Quoted 6 lines

fs/fs-writeback.c +6 -0

+	/*
+	 * for sequential workloads with good locality, include up to 8 times
+	 * more data in one chunk
+	 */
+	if (work->nr_pages >= 8 * unit)
+		return false;

argh, gack, puke. I thought I revoked your magic number license months ago!

Please, it's a HUGE red flag that bad things are happening. Would the kernel be better or worse if we were to use 9.5 instead? How do we know that "8" is optimum for all memory sizes, device bandwidths, etc?

It's a hack - it's *always* a hack. Find a better way.

Quoted 23 lines

fs/fs-writeback.c +23 -0

+	/* the unsigned comparison helps eliminate one compare */
+	if (work->offset - offset < unit) {
+		work->nr_pages += unit;
+		work->offset -= unit;
+		return true;
+	}
+
+	if (offset - end < unit) {
+		work->nr_pages += unit;
+		return true;
+	}
+
+	return false;
+}
+
+/*
+ * schedule writeback on a range of inode pages.
+ */
+static struct wb_writeback_work *
+alloc_queue_pageout_work(struct backing_dev_info *bdi,
+			 struct inode *inode,
+			 pgoff_t offset,
+			 pgoff_t len)

fs/fs-writeback.c void bdi_start_background_writeback(stru · line 264

Quoted 10 lines

fs/fs-writeback.c +10 -0

+{
+	struct wb_writeback_work *work;
+
+	/*
+	 * Grab the inode until the work is executed. We are calling this from
+	 * page reclaim context and the only thing pinning the address_space
+	 * for the moment is the page lock.
+	 */
+	if (!igrab(inode))
+		return ERR_PTR(-ENOENT);

uh-oh. igrab() means iput().

ENOENT means "no such file or directory" and makes no sense in this context.

Quoted 33 lines

fs/fs-writeback.c +33 -0

+	work = mempool_alloc(wb_work_mempool, GFP_NOWAIT);
+	if (!work) {
+		trace_printk("wb_work_mempool alloc fail\n");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	memset(work, 0, sizeof(*work));
+	work->sync_mode		= WB_SYNC_NONE;
+	work->inode		= inode;
+	work->offset		= offset;
+	work->nr_pages		= len;
+	work->reason		= WB_REASON_PAGEOUT;
+
+	bdi_queue_work(bdi, work);
+
+	return work;
+}
+
+/*
+ * Called by page reclaim code to flush the dirty page ASAP. Do write-around to
+ * improve IO throughput. The nearby pages will have good chance to reside in
+ * the same LRU list that vmscan is working on, and even close to each other
+ * inside the LRU list in the common case of sequential read/write.
+ *
+ * ret > 0: success, allocated/queued a new pageout work;
+ *	    there are at least @ret writeback works queued now
+ * ret = 0: success, reused/extended a previous pageout work
+ * ret < 0: failed
+ */
+int queue_pageout_work(struct address_space *mapping, struct page *page)
+{
+	struct backing_dev_info *bdi = mapping->backing_dev_info;
+	struct inode *inode = mapping->host;

fs/fs-writeback.c void bdi_start_background_writeback(stru · line 302

Quoted 5 lines

fs/fs-writeback.c +5 -0

+	struct wb_writeback_work *work;
+	unsigned long write_around_pages;
+	pgoff_t offset = page->index;
+	int i = 0;
+	int ret = -ENOENT;

ENOENT means "no such file or directory" and makes no sense in this context.

fs/fs-writeback.c void bdi_start_background_writeback(stru · line 304

Quoted 2 lines

fs/fs-writeback.c +2 -0

+	if (unlikely(!inode))
+		return ret;

How does this happen?

fs/fs-writeback.c void bdi_start_background_writeback(stru · line 309

Quoted 5 lines

fs/fs-writeback.c +5 -0

+	/*
+	 * piggy back 8-15ms worth of data
+	 */
+	write_around_pages = bdi->avg_write_bandwidth + MIN_WRITEBACK_PAGES;
+	write_around_pages = rounddown_pow_of_two(write_around_pages) >> 6;

Where did "6" come from?

Quoted 13 lines

fs/fs-writeback.c +13 -0

+	i = 1;
+	spin_lock_bh(&bdi->wb_lock);
+	list_for_each_entry_reverse(work, &bdi->work_list, list) {
+		if (work->inode != inode)
+			continue;
+		if (extend_writeback_range(work, offset, write_around_pages)) {
+			ret = 0;
+			break;
+		}
+		/*
+		 * vmscan will slow down page reclaim when there are more than
+		 * LOTS_OF_WRITEBACK_WORKS queued. Limit search depth to two
+		 * times larger.

fs/fs-writeback.c void bdi_start_background_writeback(stru · line 325

Quoted 3 lines

fs/fs-writeback.c +3 -0

+		 */
+		if (i++ > 2 * LOTS_OF_WRITEBACK_WORKS)
+			break;

I'm now totally lost. What are the units of "i"? (And why the heck was it called "i" anyway?) Afaict, "i" counts the number of times we successfully extended the writeback range by write_around_pages? What relationship does this have to yet-another-magic-number-times-two?

Please have a think about how to make the code comprehensible to (and hence maintainable by) others?

Quoted 7 lines

fs/fs-writeback.c +7 -0

+	}
+	spin_unlock_bh(&bdi->wb_lock);
+
+	if (ret) {
+		ret = i;
+		offset = round_down(offset, write_around_pages);
+		work = alloc_queue_pageout_work(bdi, inode,

fs/fs-writeback.c void bdi_start_background_writeback(stru · line 336

Quoted 4 lines

fs/fs-writeback.c +4 -0

+						offset, write_around_pages);
+		if (IS_ERR(work))
+			ret = PTR_ERR(work);
+	}

Need a comment over this code section. afacit it would be something like "if we failed to add pages to an existing wb_writeback_work then allocate and queue a new one".

fs/fs-writeback.c void bdi_start_background_writeback(stru · line 343

Quoted 7 lines

fs/fs-writeback.c +7 -0

+	return ret;
+}
+
+static void wb_free_work(struct wb_writeback_work *work)
+{
+	if (work->inode)
+		iput(work->inode);

And here is where at least two previous attempts to perform address_space-based writearound within direct reclaim have come unstuck.

Occasionally, iput() does a huge amount of stuff: when it is the final iput() on the inode. iirc this can include taking tons of VFS locks, truncating files, starting (and perhaps waiting upon) journal commits, etc. I forget, but it's a *lot*.

And quite possibly you haven't tested this at all, because it's pretty rare for an iput() like this to be the final one.

Let me give you an example to worry about: suppose code which holds fs locks calls into direct reclaim and then calls mempool_alloc(wb_work_mempool) and that mempool_alloc() has to wait for an item to be returned.

But no items will ever be returned, because
all the threads which own wb_writeback_works are stuck in wb_free_work->iput, trying to take an fs lock which is still held by the now-blocked direct-reclaim caller.

And a billion similar scenarios :( The really nasty thing about this is that it is very rare for this iput() to be a final iput(), so it's hard to get code coverage.

Please have a think about all of this and see if you can demonstrate how the iput() here is guaranteed safe.

Quoted 20 lines

fs/fs-writeback.c +20 -0

+	/*
+	 * Notify the caller of completion if this is a synchronous
+	 * work item, otherwise just free it.
+	 */
+	if (work->done)
+		complete(work->done);
+	else
+		mempool_free(work, wb_work_mempool);
+}
+
+/*
+ * Remove works for @sb; or if (@sb == NULL), remove all works on @bdi.
+ */
+void bdi_remove_writeback_works(struct backing_dev_info *bdi,
+				struct super_block *sb)
+{
+	struct wb_writeback_work *work, *tmp;
+	LIST_HEAD(dispose);
+
+	spin_lock_bh(&bdi->wb_lock);

fs/fs-writeback.c void bdi_start_background_writeback(stru · line 367

Quoted 4 lines

fs/fs-writeback.c +4 -0

+	list_for_each_entry_safe(work, tmp, &bdi->work_list, list) {
+		if (sb) {
+			if (work->sb && work->sb != sb)
+				continue;

What does it mean when wb_writeback_work.sb==NULL? This reader doesn't know, hence he can't understand (or review) this code. Perhaps describe it here, unless it is well described elsewhere?

fs/fs-writeback.c void bdi_start_background_writeback(stru · line 368

Quoted 1 line

fs/fs-writeback.c +1 -0

+			if (work->inode && work->inode->i_sb != sb)

And what is the meaning of wb_writeback_work.inode==NULL?

Seems odd that wb_writeback_work.sb exists, when it is accessible via wb_writeback_work.inode->i_sb.

As a person who reviews a huge amount of code, I can tell you this: the key to understanding code is to understand the data structures and the relationship between their fields and between different data structures. Including lifetime rules, locking rules and hidden information such as "what does it mean when wb_writeback_work.sb is NULL". Once one understands all this about the data structures, the code becomes pretty obvious and bugs can be spotted and fixed. But alas, wb_writeback_work is basically undocumented.

fs/fs-writeback.c void bdi_start_background_writeback(stru · line 371

Quoted 3 lines

fs/fs-writeback.c +3 -0

+				continue;
+		}
+		list_move(&work->list, &dispose);

So here we have queued for disposal a) works which refer to an inode on sb and b) works which have ->sb==NULL and ->inode==NULL. I don't know whether the b) type exist.

Quoted 4 lines

fs/fs-writeback.c +4 -0

+	}
+	spin_unlock_bh(&bdi->wb_lock);
+
+	while (!list_empty(&dispose)) {

fs/fs-writeback.c void bdi_start_background_writeback(stru · line 380

Quoted 5 lines

fs/fs-writeback.c +5 -0

+		work = list_entry(dispose.next,
+				  struct wb_writeback_work, list);
+		list_del_init(&work->list);
+		wb_free_work(work);
+	}

You should be able to do this operation without writing to all the
list_heads: no list_del(), no list_del_init().

Quoted 10 lines

fs/fs-writeback.c +3 -0

+}
+
+/*
  * Remove the inode from the writeback list it is on.
  */
 void inode_wb_list_del(struct inode *inode)

@@ -833,6 +1032,21 @@ static unsigned long get_nr_dirty_pages(
 		get_nr_dirty_inodes();
 }
 

fs/fs-writeback.c static unsigned long get_nr_dirty_pages( · line 1040

Quoted 6 lines

fs/fs-writeback.c +6 -0

+static long wb_pageout(struct bdi_writeback *wb, struct wb_writeback_work *work)
+{
+	struct writeback_control wbc = {
+		.sync_mode = WB_SYNC_NONE,
+		.nr_to_write = LONG_MAX,
+		.range_start = work->offset << PAGE_CACHE_SHIFT,

I think this will give you a 32->64 bit overflow on 32-bit machines.

fs/fs-writeback.c static unsigned long get_nr_dirty_pages( · line 1042

Quoted 2 lines

fs/fs-writeback.c +2 -0

+		.range_end = (work->offset + work->nr_pages - 1)
+						<< PAGE_CACHE_SHIFT,

Ditto.

Please include this in a patchset sometime ;)

fs/fs-writeback.c static unsigned long get_nr_dirty_pages( · line 1048

Quoted 6 lines

fs/fs-writeback.c +6 -0

+	};
+
+	do_writepages(work->inode->i_mapping, &wbc);
+
+	return LONG_MAX - wbc.nr_to_write;
+}

<infers the return semantics from the code> It took a while. Peeking at the caller helped.

Quoted 70 lines

fs/fs-writeback.c +5 -9

 static long wb_check_background_flush(struct bdi_writeback *wb)
 {
 	if (over_bground_thresh(wb->bdi)) {

@@ -905,16 +1119,12 @@ long wb_do_writeback(struct bdi_writebac
 
 		trace_writeback_exec(bdi, work);
 
-		wrote += wb_writeback(wb, work);
-
-		/*
-		 * Notify the caller of completion if this is a synchronous
-		 * work item, otherwise just free it.
-		 */
-		if (work->done)
-			complete(work->done);
+		if (!work->inode)
+			wrote += wb_writeback(wb, work);
 		else
-			kfree(work);
+			wrote += wb_pageout(wb, work);
+
+		wb_free_work(work);
 	}
 
 	/*
 
...

include/linux/backing-dev.h +2 -0

@@ -126,6 +126,8 @@ int bdi_has_dirty_io(struct backing_dev_
 void bdi_arm_supers_timer(void);
 void bdi_wakeup_thread_delayed(struct backing_dev_info *bdi);
 void bdi_lock_two(struct bdi_writeback *wb1, struct bdi_writeback *wb2);
+void bdi_remove_writeback_works(struct backing_dev_info *bdi,
+				struct super_block *sb);
 
 extern spinlock_t bdi_lock;
 extern struct list_head bdi_list;

mm/vmscan.c +15 -5

@@ -874,12 +874,22 @@ static unsigned long shrink_page_list(st
 			nr_dirty++;
 
 			/*
-			 * Only kswapd can writeback filesystem pages to
-			 * avoid risk of stack overflow but do not writeback
-			 * unless under significant pressure.
+			 * Pages may be dirtied anywhere inside the LRU. This
+			 * ensures they undergo a full period of LRU iteration
+			 * before considering pageout. The intention is to
+			 * delay writeout to the flusher thread, unless when
+			 * run into a long segment of dirty pages.
+			 */
+			if (references == PAGEREF_RECLAIM_CLEAN &&
+			    priority == DEF_PRIORITY)
+				goto keep_locked;
+
+			/*
+			 * Try relaying the pageout I/O to the flusher threads
+			 * for better I/O efficiency and avoid stack overflow.
 			 */
-			if (page_is_file_cache(page) &&
-					(!current_is_kswapd() || priority >= DEF_PRIORITY - 2)) {
+			if (page_is_file_cache(page) && mapping &&
+			    queue_pageout_work(mapping, page) >= 0) {
 				/*
 				 * Immediately reclaim when written back.
 				 * Similar in principal to deactivate_page()

mm/vmscan.c static unsigned long shrink_page_list(st · line 909

Quoted 9 lines

mm/vmscan.c +5 -1

@@ -892,8 +902,13 @@ static unsigned long shrink_page_list(st
 				goto keep_locked;
 			}
 
-			if (references == PAGEREF_RECLAIM_CLEAN)
+			/*
+			 * Only kswapd can writeback filesystem pages to
+			 * avoid risk of stack overflow.
+			 */
+			if (page_is_file_cache(page) && !current_is_kswapd())

And here we run into big problems.

When a page-allocator enters direct reclaim, that process is trying to allocate a page from a particular zone (or set of zones). For example, he wants a ZONE_NORMAL or ZONE_DMA page. Asking flusher threads to go off and write back three gigabytes of ZONE_HIGHMEM is pointless, inefficient and doesn't fix the caller's problem at all.

This has always been the biggest problem with the avoid-writeback-from-direct-reclaim patches. And your patchset (as far as I've read) doesn't address the problem at all and appears to be blissfully unaware of its existence.

I've attempted versions of this I think twice, and thrown the patches away in disgust. One approach I tried was, within direct reclaim, to grab the page I wanted (ie: one which is in one of the caller's desired zones) and to pass that page over to the kernel threads. The kernel threads would ensure that this particular page was included in the writearound preparation. So that we at least make *some* progress toward what the caller is asking us to do.

iirc, the way I "grabbed" the page was to actually lock it, with [try_]_lock_page(). And unlock it again way over within the writeback thread. I forget why I did it this way, rather than get_page() or whatever. Locking the page is a good way of preventing anyone else from futzing with it. It also pins the inode, which perhaps meant that with careful management, I could avoid the igrab()/iput() horrors discussed above.

Quoted 31 lines

mm/vmscan.c +15 -5

@@ -874,12 +874,22 @@ static unsigned long shrink_page_list(st
 			nr_dirty++;
 
 			/*
-			 * Only kswapd can writeback filesystem pages to
-			 * avoid risk of stack overflow but do not writeback
-			 * unless under significant pressure.
+			 * Pages may be dirtied anywhere inside the LRU. This
+			 * ensures they undergo a full period of LRU iteration
+			 * before considering pageout. The intention is to
+			 * delay writeout to the flusher thread, unless when
+			 * run into a long segment of dirty pages.
+			 */
+			if (references == PAGEREF_RECLAIM_CLEAN &&
+			    priority == DEF_PRIORITY)
+				goto keep_locked;
+
+			/*
+			 * Try relaying the pageout I/O to the flusher threads
+			 * for better I/O efficiency and avoid stack overflow.
 			 */
-			if (page_is_file_cache(page) &&
-					(!current_is_kswapd() || priority >= DEF_PRIORITY - 2)) {
+			if (page_is_file_cache(page) && mapping &&
+			    queue_pageout_work(mapping, page) >= 0) {
 				/*
 				 * Immediately reclaim when written back.
 				 * Similar in principal to deactivate_page()

mm/vmscan.c static unsigned long shrink_page_list(st · line 909

Quoted 9 lines

mm/vmscan.c +5 -1

@@ -892,8 +902,13 @@ static unsigned long shrink_page_list(st
 				goto keep_locked;
 			}
 
-			if (references == PAGEREF_RECLAIM_CLEAN)
+			/*
+			 * Only kswapd can writeback filesystem pages to
+			 * avoid risk of stack overflow.
+			 */
+			if (page_is_file_cache(page) && !current_is_kswapd())

And here we run into big problems.

When a page-allocator enters direct reclaim, that process is trying to allocate a page from a particular zone (or set of zones). For example, he wants a ZONE_NORMAL or ZONE_DMA page. Asking flusher threads to go off and write back three gigabytes of ZONE_HIGHMEM is pointless, inefficient and doesn't fix the caller's problem at all.

This has always been the biggest problem with the avoid-writeback-from-direct-reclaim patches. And your patchset (as far as I've read) doesn't address the problem at all and appears to be blissfully unaware of its existence.

I've attempted versions of this I think twice, and thrown the patches away in disgust. One approach I tried was, within direct reclaim, to grab the page I wanted (ie: one which is in one of the caller's desired zones) and to pass that page over to the kernel threads. The kernel threads would ensure that this particular page was included in the writearound preparation. So that we at least make *some* progress toward what the caller is asking us to do.

iirc, the way I "grabbed" the page was to actually lock it, with [try_]_lock_page(). And unlock it again way over within the writeback thread. I forget why I did it this way, rather than get_page() or whatever. Locking the page is a good way of preventing anyone else from futzing with it. It also pins the inode, which perhaps meant that with careful management, I could avoid the igrab()/iput() horrors discussed above.

On Tue 28-02-12 16:04:03, Andrew Morton wrote:
...

Quoted 63 lines

Quoted 31 lines

mm/vmscan.c +15 -5

@@ -874,12 +874,22 @@ static unsigned long shrink_page_list(st
 			nr_dirty++;
 
 			/*
-			 * Only kswapd can writeback filesystem pages to
-			 * avoid risk of stack overflow but do not writeback
-			 * unless under significant pressure.
+			 * Pages may be dirtied anywhere inside the LRU. This
+			 * ensures they undergo a full period of LRU iteration
+			 * before considering pageout. The intention is to
+			 * delay writeout to the flusher thread, unless when
+			 * run into a long segment of dirty pages.
+			 */
+			if (references == PAGEREF_RECLAIM_CLEAN &&
+			    priority == DEF_PRIORITY)
+				goto keep_locked;
+
+			/*
+			 * Try relaying the pageout I/O to the flusher threads
+			 * for better I/O efficiency and avoid stack overflow.
 			 */
-			if (page_is_file_cache(page) &&
-					(!current_is_kswapd() || priority >= DEF_PRIORITY - 2)) {
+			if (page_is_file_cache(page) && mapping &&
+			    queue_pageout_work(mapping, page) >= 0) {
 				/*
 				 * Immediately reclaim when written back.
 				 * Similar in principal to deactivate_page()

mm/vmscan.c static unsigned long shrink_page_list(st · line 909

Quoted 9 lines

mm/vmscan.c +5 -1

@@ -892,8 +902,13 @@ static unsigned long shrink_page_list(st
 				goto keep_locked;
 			}
 
-			if (references == PAGEREF_RECLAIM_CLEAN)
+			/*
+			 * Only kswapd can writeback filesystem pages to
+			 * avoid risk of stack overflow.
+			 */
+			if (page_is_file_cache(page) && !current_is_kswapd())

And here we run into big problems.

When a page-allocator enters direct reclaim, that process is trying to allocate a page from a particular zone (or set of zones). For example, he wants a ZONE_NORMAL or ZONE_DMA page. Asking flusher threads to go off and write back three gigabytes of ZONE_HIGHMEM is pointless, inefficient and doesn't fix the caller's problem at all.

This has always been the biggest problem with the avoid-writeback-from-direct-reclaim patches. And your patchset (as far as I've read) doesn't address the problem at all and appears to be blissfully unaware of its existence.

I've attempted versions of this I think twice, and thrown the patches away in disgust. One approach I tried was, within direct reclaim, to grab the page I wanted (ie: one which is in one of the caller's desired zones) and to pass that page over to the kernel threads. The kernel threads would ensure that this particular page was included in the writearound preparation. So that we at least make *some* progress toward what the caller is asking us to do.

Quoted 7 lines

iirc, the way I "grabbed" the page was to actually lock it, with [try_]_lock_page(). And unlock it again way over within the writeback thread. I forget why I did it this way, rather than get_page() or whatever. Locking the page is a good way of preventing anyone else from futzing with it. It also pins the inode, which perhaps meant that with careful management, I could avoid the igrab()/iput() horrors discussed above.

I think using get_page() might be a good way to go. Naive implementation:
If we need to write a page from kswapd, we do get_page(), attach page to wb_writeback_work and push it to flusher thread to deal with it. Flusher thread sees the work, takes a page lock, verifies the page is still attached to some inode & dirty (it could have been truncated / cleaned by someone else) and if yes, it submits page for IO (possibly with some writearound). This scheme won't have problems with iput() and won't have problems with umount. Also we guarantee some progress - either flusher thread does it, or some else must have done the work before flusher thread got to it.

Quoted 72 lines · Jan Kara

On Tue 28-02-12 16:04:03, Andrew Morton wrote:
...

Quoted 63 lines

Quoted 31 lines

mm/vmscan.c +15 -5

@@ -874,12 +874,22 @@ static unsigned long shrink_page_list(st
 			nr_dirty++;
 
 			/*
-			 * Only kswapd can writeback filesystem pages to
-			 * avoid risk of stack overflow but do not writeback
-			 * unless under significant pressure.
+			 * Pages may be dirtied anywhere inside the LRU. This
+			 * ensures they undergo a full period of LRU iteration
+			 * before considering pageout. The intention is to
+			 * delay writeout to the flusher thread, unless when
+			 * run into a long segment of dirty pages.
+			 */
+			if (references == PAGEREF_RECLAIM_CLEAN &&
+			    priority == DEF_PRIORITY)
+				goto keep_locked;
+
+			/*
+			 * Try relaying the pageout I/O to the flusher threads
+			 * for better I/O efficiency and avoid stack overflow.
 			 */
-			if (page_is_file_cache(page) &&
-					(!current_is_kswapd() || priority >= DEF_PRIORITY - 2)) {
+			if (page_is_file_cache(page) && mapping &&
+			    queue_pageout_work(mapping, page) >= 0) {
 				/*
 				 * Immediately reclaim when written back.
 				 * Similar in principal to deactivate_page()

mm/vmscan.c static unsigned long shrink_page_list(st · line 909

Quoted 9 lines

mm/vmscan.c +5 -1

@@ -892,8 +902,13 @@ static unsigned long shrink_page_list(st
 				goto keep_locked;
 			}
 
-			if (references == PAGEREF_RECLAIM_CLEAN)
+			/*
+			 * Only kswapd can writeback filesystem pages to
+			 * avoid risk of stack overflow.
+			 */
+			if (page_is_file_cache(page) && !current_is_kswapd())

And here we run into big problems.

When a page-allocator enters direct reclaim, that process is trying to allocate a page from a particular zone (or set of zones). For example, he wants a ZONE_NORMAL or ZONE_DMA page. Asking flusher threads to go off and write back three gigabytes of ZONE_HIGHMEM is pointless, inefficient and doesn't fix the caller's problem at all.

This has always been the biggest problem with the avoid-writeback-from-direct-reclaim patches. And your patchset (as far as I've read) doesn't address the problem at all and appears to be blissfully unaware of its existence.

I've attempted versions of this I think twice, and thrown the patches away in disgust. One approach I tried was, within direct reclaim, to grab the page I wanted (ie: one which is in one of the caller's desired zones) and to pass that page over to the kernel threads. The kernel threads would ensure that this particular page was included in the writearound preparation. So that we at least make *some* progress toward what the caller is asking us to do.

Quoted 7 lines

iirc, the way I "grabbed" the page was to actually lock it, with [try_]_lock_page(). And unlock it again way over within the writeback thread. I forget why I did it this way, rather than get_page() or whatever. Locking the page is a good way of preventing anyone else from futzing with it. It also pins the inode, which perhaps meant that with careful management, I could avoid the igrab()/iput() horrors discussed above.

Quoted 10 lines

I think using get_page() might be a good way to go. Naive implementation:
If we need to write a page from kswapd, we do get_page(), attach page to wb_writeback_work and push it to flusher thread to deal with it. Flusher thread sees the work, takes a page lock, verifies the page is still attached to some inode & dirty (it could have been truncated / cleaned by someone else) and if yes, it submits page for IO (possibly with some writearound). This scheme won't have problems with iput() and won't have problems with umount. Also we guarantee some progress - either flusher thread does it, or some else must have done the work before flusher thread got to it.

I like this idea.

get_page() looks the perfect solution to verify if the struct inode pointer (w/o igrab) is still live and valid.

[...upon rethinking...] Oh but still we need to lock some page to pin the inode during the writeout. Then there is the dilemma: if the page is locked, we effectively keep it from being written out...

Quoted 10 lines

I think using get_page() might be a good way to go. Naive implementation:
If we need to write a page from kswapd, we do get_page(), attach page to wb_writeback_work and push it to flusher thread to deal with it. Flusher thread sees the work, takes a page lock, verifies the page is still attached to some inode & dirty (it could have been truncated / cleaned by someone else) and if yes, it submits page for IO (possibly with some writearound). This scheme won't have problems with iput() and won't have problems with umount. Also we guarantee some progress - either flusher thread does it, or some else must have done the work before flusher thread got to it.

I like this idea.

get_page() looks the perfect solution to verify if the struct inode pointer (w/o igrab) is still live and valid.

[...upon rethinking...] Oh but still we need to lock some page to pin the inode during the writeout. Then there is the dilemma: if the page is locked, we effectively keep it from being written out...

Quoted 11 lines · On Thu, 1 Mar 2012 19:41:51 +0800 Fengguang Wu <[email protected]> wrote:

Quoted 10 lines

I think using get_page() might be a good way to go. Naive implementation:
If we need to write a page from kswapd, we do get_page(), attach page to wb_writeback_work and push it to flusher thread to deal with it. Flusher thread sees the work, takes a page lock, verifies the page is still attached to some inode & dirty (it could have been truncated / cleaned by someone else) and if yes, it submits page for IO (possibly with some writearound). This scheme won't have problems with iput() and won't have problems with umount. Also we guarantee some progress - either flusher thread does it, or some else must have done the work before flusher thread got to it.

Quoted 8 lines

I like this idea.

get_page() looks the perfect solution to verify if the struct inode pointer (w/o igrab) is still live and valid.

[...upon rethinking...] Oh but still we need to lock some page to pin the inode during the writeout. Then there is the dilemma: if the page is locked, we effectively keep it from being written out...

No, all you need to do is to structure the code so that after the page gets unlocked, the kernel thread does not touch the address_space. So the processing within the kthread is along the lines of

writearound(locked_page)
{
write some pages preceding locked_page; /* touches address_space */

	write locked_page;
	write pages following locked_page;	/* touches address_space */
	unlock_page(locked_page);
}

Quoted 11 lines

Quoted 6 lines

get_page() looks the perfect solution to verify if the struct inode pointer (w/o igrab) is still live and valid.

[...upon rethinking...] Oh but still we need to lock some page to pin the inode during the writeout. Then there is the dilemma: if the page is locked, we effectively keep it from being written out...

No, all you need to do is to structure the code so that after the page gets unlocked, the kernel thread does not touch the address_space. So the processing within the kthread is along the lines of

Quoted 3 lines

writearound(locked_page)
{
write some pages preceding locked_page; /* touches address_space */

It seems the above line will lead to ABBA deadlock.

At least btrfs will lock a number of pages in lock_delalloc_pages().

Quoted 15 lines · On Sat, 3 Mar 2012 21:25:55 +0800 Fengguang Wu <[email protected]> wrote:

Quoted 14 lines

Quoted 6 lines

get_page() looks the perfect solution to verify if the struct inode pointer (w/o igrab) is still live and valid.

[...upon rethinking...] Oh but still we need to lock some page to pin the inode during the writeout. Then there is the dilemma: if the page is locked, we effectively keep it from being written out...

No, all you need to do is to structure the code so that after the page gets unlocked, the kernel thread does not touch the address_space. So the processing within the kthread is along the lines of

writearound(locked_page)
{
write some pages preceding locked_page; /* touches address_space */

Quoted 3 lines

It seems the above line will lead to ABBA deadlock.

At least btrfs will lock a number of pages in lock_delalloc_pages().

Well, this code locks multiple pages too. I forget what I did about that - probably trylock. Dirty pages aren't locked for very long.

Quoted 7 lines

iirc, the way I "grabbed" the page was to actually lock it, with [try_]_lock_page(). And unlock it again way over within the writeback thread. I forget why I did it this way, rather than get_page() or whatever. Locking the page is a good way of preventing anyone else from futzing with it. It also pins the inode, which perhaps meant that with careful management, I could avoid the igrab()/iput() horrors discussed above.

I think using get_page() might be a good way to go.

Quoted 8 lines · On Thu, 1 Mar 2012 12:04:04 +0100 Jan Kara <[email protected]> wrote:

Quoted 7 lines

iirc, the way I "grabbed" the page was to actually lock it, with [try_]_lock_page(). And unlock it again way over within the writeback thread. I forget why I did it this way, rather than get_page() or whatever. Locking the page is a good way of preventing anyone else from futzing with it. It also pins the inode, which perhaps meant that with careful management, I could avoid the igrab()/iput() horrors discussed above.

Quoted 1 line

I think using get_page() might be a good way to go.

get_page() doesn't pin the inode - truncate() will still detach it from the address_space().

Quoted 13 lines · Andrew Morton

Quoted 9 lines · On Thu, 1 Mar 2012 12:04:04 +0100 Jan Kara <[email protected]> wrote:

Quoted 7 lines

iirc, the way I "grabbed" the page was to actually lock it, with [try_]_lock_page(). And unlock it again way over within the writeback thread. I forget why I did it this way, rather than get_page() or whatever. Locking the page is a good way of preventing anyone else from futzing with it. It also pins the inode, which perhaps meant that with careful management, I could avoid the igrab()/iput() horrors discussed above.

I think using get_page() might be a good way to go.

Quoted 2 lines

get_page() doesn't pin the inode - truncate() will still detach it from the address_space().

Yes, I know. And exactly because of that I'd like to use it. Flusher
thread would lock the page from the work item, check whether it is still attached to the inode and if yes, it will proceed. Otherwise it will just discard the work item because we know the page has already been written out by someone else or truncated.

Please have a think about all of this and see if you can demonstrate how the iput() here is guaranteed safe.

Quoted 2 lines

Please have a think about all of this and see if you can demonstrate how the iput() here is guaranteed safe.

There are already several __iget()/iput() calls inside fs-writeback.c. The existing iput() calls already demonstrate its safety?

Basically the flusher works in this way

- the dirty inode list i_wb_list does not reference count the inode at all

- the flusher thread does something analog to igrab() and set I_SYNC before going off to writeout the inode

- evict() will wait for completion of I_SYNC

Quoted 6 lines · Wu Fengguang

Quoted 2 lines

Please have a think about all of this and see if you can demonstrate how the iput() here is guaranteed safe.

There are already several __iget()/iput() calls inside fs-writeback.c. The existing iput() calls already demonstrate its safety?

Quoted 8 lines

Basically the flusher works in this way

- the dirty inode list i_wb_list does not reference count the inode at all

- the flusher thread does something analog to igrab() and set I_SYNC before going off to writeout the inode

- evict() will wait for completion of I_SYNC

Yes, you are right that currently writeback code already holds inode
references and so it can happen that flusher thread drops the last inode reference. But currently that could create problems only if someone waits for flusher thread to make progress while effectively blocking e.g. truncate from happening. Currently flusher thread handles sync(2) and background writeback and filesystems take care to not hold any locks blocking IO / truncate while possibly waiting for these.

But with your addition situation changes significantly - now anyone doing allocation can block and do allocation from all sorts of places including ones where we hold locks blocking other fs activity. The good news is that we use GFP_NOFS in such places. So if GFP_NOFS allocation cannot possibly depend on a completion of some writeback work, then I'd still be comfortable with dropping inode references from writeback code. But Andrew is right this at least needs some arguing...

Quoted 23 lines · Jan Kara

Quoted 6 lines · Wu Fengguang

Quoted 2 lines

Please have a think about all of this and see if you can demonstrate how the iput() here is guaranteed safe.

There are already several __iget()/iput() calls inside fs-writeback.c. The existing iput() calls already demonstrate its safety?

Quoted 8 lines

Basically the flusher works in this way

- the dirty inode list i_wb_list does not reference count the inode at all

- the flusher thread does something analog to igrab() and set I_SYNC before going off to writeout the inode

- evict() will wait for completion of I_SYNC

Yes, you are right that currently writeback code already holds inode
references and so it can happen that flusher thread drops the last inode reference. But currently that could create problems only if someone waits for flusher thread to make progress while effectively blocking e.g. truncate from happening. Currently flusher thread handles sync(2) and background writeback and filesystems take care to not hold any locks blocking IO / truncate while possibly waiting for these.

Quoted 7 lines

But with your addition situation changes significantly - now anyone doing allocation can block and do allocation from all sorts of places including ones where we hold locks blocking other fs activity. The good news is that we use GFP_NOFS in such places. So if GFP_NOFS allocation cannot possibly depend on a completion of some writeback work, then I'd still be comfortable with dropping inode references from writeback code. But Andrew is right this at least needs some arguing...

You seem to miss the point that we don't do wait or page allocations inside queue_pageout_work().

The final iput() will not block the
random tasks because the latter don't wait for completion of the work.

        random task                     flusher thread

        page allocation
          page reclaim
            queue_pageout_work()
              igrab()

                  ......  after a while  ......

                                        execute pageout work                
                                        iput()
                                        <work completed>

There will be some reclaim_wait()s if the pageout works are not executed quickly, in which case vmscan will be impacted and slowed down. However it's not waiting for any specific work to complete, so there is no chance to form a loop of dependencies leading to deadlocks.

The iput() does have the theoretic possibility to deadlock the flusher thread itself (but not with the other random tasks). Since the flusher thread has always been doing iput() w/o running into such bugs, we can reasonably expect the new iput() to be as safe in practical.

Quoted 32 lines · Wu Fengguang

Quoted 30 lines · Jan Kara

Quoted 6 lines · Wu Fengguang

Quoted 2 lines

Please have a think about all of this and see if you can demonstrate how the iput() here is guaranteed safe.

There are already several __iget()/iput() calls inside fs-writeback.c. The existing iput() calls already demonstrate its safety?

Quoted 8 lines

Basically the flusher works in this way

- the dirty inode list i_wb_list does not reference count the inode at all

- the flusher thread does something analog to igrab() and set I_SYNC before going off to writeout the inode

- evict() will wait for completion of I_SYNC

Yes, you are right that currently writeback code already holds inode
references and so it can happen that flusher thread drops the last inode reference. But currently that could create problems only if someone waits for flusher thread to make progress while effectively blocking e.g. truncate from happening. Currently flusher thread handles sync(2) and background writeback and filesystems take care to not hold any locks blocking IO / truncate while possibly waiting for these.

But with your addition situation changes significantly - now anyone doing allocation can block and do allocation from all sorts of places including ones where we hold locks blocking other fs activity. The good news is that we use GFP_NOFS in such places. So if GFP_NOFS allocation cannot possibly depend on a completion of some writeback work, then I'd still be comfortable with dropping inode references from writeback code. But Andrew is right this at least needs some arguing...

Quoted 2 lines

You seem to miss the point that we don't do wait or page allocations inside queue_pageout_work().

I didn't miss this point. I know we don't wait directly. But if the only

way to free pages from the zone where we need to do allocation is via flusher thread, then we effectively *are* waiting for the work to complete. And if the flusher thread is blocked, we have a problem.

And I agree it's unlikely but given enough time and people, I believe someone finds a way to (inadvertedly) trigger this.

Quoted 21 lines

The final iput() will not block the
random tasks because the latter don't wait for completion of the work.

        random task                     flusher thread

        page allocation
          page reclaim
            queue_pageout_work()
              igrab()

                  ......  after a while  ......

                                        execute pageout work                
                                        iput()
                                        <work completed>

There will be some reclaim_wait()s if the pageout works are not executed quickly, in which case vmscan will be impacted and slowed down. However it's not waiting for any specific work to complete, so there is no chance to form a loop of dependencies leading to deadlocks.

Quoted 4 lines

The iput() does have the theoretic possibility to deadlock the flusher thread itself (but not with the other random tasks). Since the flusher thread has always been doing iput() w/o running into such bugs, we can reasonably expect the new iput() to be as safe in practical.

But so far, kswapd could do writeout itself so even if flusher thread is
blocked in iput(), we could still do writeout from kswapd to clean zones.

Now I don't think blocking on iput() can be a problem because of reasons I outlined in another email yesterday (GFP_NOFS allocations and such). Just I don't agree with your reasoning that it cannot be a problem because it was not problem previously. That's just not true.

Quoted 2 lines

And I agree it's unlikely but given enough time and people, I believe someone finds a way to (inadvertedly) trigger this.

Right. The pageout works could add lots more iput() to the flusher and turn some hidden statistical impossible bugs into real ones.

Fortunately the "flusher deadlocks itself" case is easy to detect and prevent as illustrated in another email.

Quoted 3 lines · On Fri, 2 Mar 2012 18:39:51 +0800 Fengguang Wu <[email protected]> wrote:

Quoted 2 lines

And I agree it's unlikely but given enough time and people, I believe someone finds a way to (inadvertedly) trigger this.

Quoted 5 lines

Right. The pageout works could add lots more iput() to the flusher and turn some hidden statistical impossible bugs into real ones.

Fortunately the "flusher deadlocks itself" case is easy to detect and prevent as illustrated in another email.

It would be a heck of a lot safer and saner to avoid the iput(). We know how to do this, so why not do it?

Quoted 6 lines · Andrew Morton

Quoted 3 lines · On Fri, 2 Mar 2012 18:39:51 +0800 Fengguang Wu <[email protected]> wrote:

Quoted 2 lines

And I agree it's unlikely but given enough time and people, I believe someone finds a way to (inadvertedly) trigger this.

Quoted 8 lines

Quoted 5 lines

Right. The pageout works could add lots more iput() to the flusher and turn some hidden statistical impossible bugs into real ones.

Fortunately the "flusher deadlocks itself" case is easy to detect and prevent as illustrated in another email.

It would be a heck of a lot safer and saner to avoid the iput(). We know how to do this, so why not do it?

My concern about the page lock is, it costs more code and sounds like hacking around something. It seems we (including me) have been trying to shun away from the iput() problem. Since it's unlikely we are to get rid of the already existing iput() calls from the flusher context, why not face the problem, sort it out and use it with confident in new code?

Let me try it now. The only scheme iput() can deadlock the flusher is for the iput() path to come back to queue some work and wait for it. Here are the exhaust list of the queue+wait paths:

writeback_inodes_sb_nr_if_idle
ext4_nonda_switch
ext4_page_mkwrite # from page fault
ext4_da_write_begin # from user writes

writeback_inodes_sb_nr

  quotactl syscall                      # from syscall
  __sync_filesystem                     # from sync/umount
  shrink_liability                      # ubifs

make_free_space
ubifs_budget_space # from all over ubifs:

   2    274  /c/linux/fs/ubifs/dir.c <<ubifs_create>>
   3    531  /c/linux/fs/ubifs/dir.c <<ubifs_link>>
   4    586  /c/linux/fs/ubifs/dir.c <<ubifs_unlink>>
   5    675  /c/linux/fs/ubifs/dir.c <<ubifs_rmdir>>
   6    731  /c/linux/fs/ubifs/dir.c <<ubifs_mkdir>>
   7    803  /c/linux/fs/ubifs/dir.c <<ubifs_mknod>>
   8    871  /c/linux/fs/ubifs/dir.c <<ubifs_symlink>>
   9   1006  /c/linux/fs/ubifs/dir.c <<ubifs_rename>>
  10   1009  /c/linux/fs/ubifs/dir.c <<ubifs_rename>>
  11    246  /c/linux/fs/ubifs/file.c <<write_begin_slow>>
  12    388  /c/linux/fs/ubifs/file.c <<allocate_budget>>
  13   1125  /c/linux/fs/ubifs/file.c <<do_truncation>>   <===== deadlockable
  14   1217  /c/linux/fs/ubifs/file.c <<do_setattr>>
  15   1381  /c/linux/fs/ubifs/file.c <<update_mctime>>
  16   1486  /c/linux/fs/ubifs/file.c <<ubifs_vm_page_mkwrite>>
  17    110  /c/linux/fs/ubifs/ioctl.c <<setflags>>
  19    122  /c/linux/fs/ubifs/xattr.c <<create_xattr>>
  20    201  /c/linux/fs/ubifs/xattr.c <<change_xattr>>
  21    494  /c/linux/fs/ubifs/xattr.c <<remove_xattr>>

It seems they are all safe except for ubifs. ubifs may actually deadlock from the above do_truncation() caller. However it should be

fixable because the ubifs call for writeback_inodes_sb_nr() sounds very brute force writeback and wait and there may well be better way out.

CCing ubifs developers for possible thoughts..

Thanks,
Fengguang

PS. I'll be on travel in the following week and won't have much time for replying emails. Sorry about that.

[correct email addresses for Artem and Adrian]

Quoted 29 lines · Fengguang Wu

Quoted 6 lines · Andrew Morton

Quoted 3 lines · On Fri, 2 Mar 2012 18:39:51 +0800 Fengguang Wu <[email protected]> wrote:

Quoted 2 lines

And I agree it's unlikely but given enough time and people, I believe someone finds a way to (inadvertedly) trigger this.

Quoted 8 lines

Quoted 5 lines

Right. The pageout works could add lots more iput() to the flusher and turn some hidden statistical impossible bugs into real ones.

Fortunately the "flusher deadlocks itself" case is easy to detect and prevent as illustrated in another email.

It would be a heck of a lot safer and saner to avoid the iput(). We know how to do this, so why not do it?

My concern about the page lock is, it costs more code and sounds like hacking around something. It seems we (including me) have been trying to shun away from the iput() problem. Since it's unlikely we are to get rid of the already existing iput() calls from the flusher context, why not face the problem, sort it out and use it with confident in new code?

Let me try it now. The only scheme iput() can deadlock the flusher is for the iput() path to come back to queue some work and wait for it. Here are the exhaust list of the queue+wait paths:

writeback_inodes_sb_nr_if_idle

Quoted 31 lines

ext4_nonda_switch
ext4_page_mkwrite # from page fault
ext4_da_write_begin # from user writes

writeback_inodes_sb_nr

  quotactl syscall                      # from syscall
  __sync_filesystem                     # from sync/umount
  shrink_liability                      # ubifs

make_free_space
ubifs_budget_space # from all over ubifs:

   2    274  /c/linux/fs/ubifs/dir.c <<ubifs_create>>
   3    531  /c/linux/fs/ubifs/dir.c <<ubifs_link>>
   4    586  /c/linux/fs/ubifs/dir.c <<ubifs_unlink>>
   5    675  /c/linux/fs/ubifs/dir.c <<ubifs_rmdir>>
   6    731  /c/linux/fs/ubifs/dir.c <<ubifs_mkdir>>
   7    803  /c/linux/fs/ubifs/dir.c <<ubifs_mknod>>
   8    871  /c/linux/fs/ubifs/dir.c <<ubifs_symlink>>
   9   1006  /c/linux/fs/ubifs/dir.c <<ubifs_rename>>
  10   1009  /c/linux/fs/ubifs/dir.c <<ubifs_rename>>
  11    246  /c/linux/fs/ubifs/file.c <<write_begin_slow>>
  12    388  /c/linux/fs/ubifs/file.c <<allocate_budget>>
  13   1125  /c/linux/fs/ubifs/file.c <<do_truncation>>   <===== deadlockable
  14   1217  /c/linux/fs/ubifs/file.c <<do_setattr>>
  15   1381  /c/linux/fs/ubifs/file.c <<update_mctime>>
  16   1486  /c/linux/fs/ubifs/file.c <<ubifs_vm_page_mkwrite>>
  17    110  /c/linux/fs/ubifs/ioctl.c <<setflags>>
  19    122  /c/linux/fs/ubifs/xattr.c <<create_xattr>>
  20    201  /c/linux/fs/ubifs/xattr.c <<change_xattr>>
  21    494  /c/linux/fs/ubifs/xattr.c <<remove_xattr>>

Quoted 2 lines

It seems they are all safe except for ubifs. ubifs may actually deadlock from the above do_truncation() caller. However it should be

Sorry that do_truncation() is actually called from ubifs_setattr() which is not related to iput().

Are there other possibilities for iput() to call into the above list of ubifs functions, then start writeback work and wait for it which will deadlock the flusher? ubifs_unlink() and perhaps remove_xattr()?

Quoted 11 lines

fixable because the ubifs call for writeback_inodes_sb_nr() sounds very brute force writeback and wait and there may well be better way out.

CCing ubifs developers for possible thoughts..

Thanks,
Fengguang

PS. I'll be on travel in the following week and won't have much time for replying emails. Sorry about that.

13 1125 /c/linux/fs/ubifs/file.c <<do_truncation>> <===== deadlockable

It seems they are all safe except for ubifs. ubifs may actually deadlock from the above do_truncation() caller. However it should be fixable because the ubifs call for writeback_inodes_sb_nr() sounds very brute force writeback and wait and there may well be better way out.

Quoted 1 line · Fengguang Wu

13 1125 /c/linux/fs/ubifs/file.c <<do_truncation>> <===== deadlockable

Sorry, but could you please explain once again how the deadlock may happen?

Quoted 5 lines

It seems they are all safe except for ubifs. ubifs may actually deadlock from the above do_truncation() caller. However it should be fixable because the ubifs call for writeback_inodes_sb_nr() sounds very brute force writeback and wait and there may well be better way out.

I do not think this "fixable" - this is part of UBIFS design to force write-back when we are not sure we have enough space.

The problem is that we do not know how much space the dirty data in RAM will take on the flash media (after it is actually written-back) - e.g., because we compress all the data (UBIFS performs on-the-flight compression). So we do pessimistic assumptions and allow dirtying more and more data as long as we know for sure that there is enough flash space on the media for the worst-case scenario (data are not compressible). This is what the UBIFS budgeting subsystem does.

Once the budgeting sub-system sees that we are not going to have enough flash space for the worst-case scenario, it starts forcing write-back to push some dirty data out to the flash media and update the budgeting numbers, and get more realistic picture.

So basically, before you can change _anything_ on UBIFS file-system, you need to budget for the space. Even when you truncate - because truncation is also about allocating more space for writing the updated inode and update the FS index. (Remember, all writes are out-of-place in UBIFS because we work with raw flash, not a block device).

Quoted 5 lines · Artem Bityutskiy

Quoted 1 line · Fengguang Wu

13 1125 /c/linux/fs/ubifs/file.c <<do_truncation>> <===== deadlockable

Sorry, but could you please explain once again how the deadlock may happen?

Sorry I confused ubifs do_truncation() with the truncate_inode_pages() that may be called from iput().

The once suspected deadlock scheme is when the flusher thread calls the final iput:

flusher thread
iput_final
<some ubifs function>
ubifs_budget_space
shrink_liability
writeback_inodes_sb
writeback_inodes_sb_nr
bdi_queue_work
wait_for_completion => end up waiting for the flusher itself

However I cannot find any ubifs functions to form the above loop, so ubifs should be safe for now.

Quoted 2 lines

However I cannot find any ubifs functions to form the above loop, so ubifs should be safe for now.

Yeah, me neither but I also failed to find a place where
ubifs_evict_inode() truncates inode space when deleting the inode... Artem?

Quoted 2 lines

However I cannot find any ubifs functions to form the above loop, so ubifs should be safe for now.

Yeah, me neither but I also failed to find a place where
ubifs_evict_inode() truncates inode space when deleting the inode... Artem?

Quoted 4 lines · Jan Kara

Quoted 2 lines

However I cannot find any ubifs functions to form the above loop, so ubifs should be safe for now.

Yeah, me neither but I also failed to find a place where
ubifs_evict_inode() truncates inode space when deleting the inode... Artem?

We do call 'truncate_inode_pages()':

static void ubifs_evict_inode(struct inode *inode)

{
	...

        truncate_inode_pages(&inode->i_data, 0);

        ...
}

Quoted 12 lines · Artem Bityutskiy

Quoted 4 lines · Jan Kara

Quoted 2 lines

However I cannot find any ubifs functions to form the above loop, so ubifs should be safe for now.

Yeah, me neither but I also failed to find a place where
ubifs_evict_inode() truncates inode space when deleting the inode... Artem?

We do call 'truncate_inode_pages()':

static void ubifs_evict_inode(struct inode *inode)

{
	...

Quoted 4 lines

        truncate_inode_pages(&inode->i_data, 0);

        ...
}

Well, but that just removes pages from page cache. You should somewhere
also free allocated blocks and free the inode... And I'm sure you do, otherwise you would pretty quickly notice that file deletion does not work :) Just I could not find which function does it.

Quoted 13 lines · Jan Kara

Quoted 12 lines · Artem Bityutskiy

Quoted 4 lines · Jan Kara

Quoted 2 lines

However I cannot find any ubifs functions to form the above loop, so ubifs should be safe for now.

Yeah, me neither but I also failed to find a place where
ubifs_evict_inode() truncates inode space when deleting the inode... Artem?

We do call 'truncate_inode_pages()':

static void ubifs_evict_inode(struct inode *inode)

{
	...

Quoted 8 lines

Quoted 4 lines

        truncate_inode_pages(&inode->i_data, 0);

        ...
}

Well, but that just removes pages from page cache. You should somewhere
also free allocated blocks and free the inode... And I'm sure you do, otherwise you would pretty quickly notice that file deletion does not work :) Just I could not find which function does it.

ubifs_evict_inode() -> ubifs_jnl_delete_inode() -> ubifs_tnc_remove_ino()

Basically, deletion in UBIFS is about writing a so-called "deletion inode" to the journal and then removing all the data nodes of the truncated inode from the TNC (in-memory cache of the FS index, which is just a huge B-tree, like in reiser4 which inspired me long time ago, and like in btrfs).

The second part of the overall deletion job will be when we commit - the updated version of the FS index will be written to the flash media.

If we get a power cut before the commit, the journal reply will see the deletion inode and will clean-up the index. The deletion inode is never erased before the commit.

Basically, this design is dictated by the fact that we do not have a cheap way of doing in-place updates.

This is a short version of the story. Here are some docs as well:
http://www.linux-mtd.infradead.org/doc/ubifs.html#L_documentation

Quoted 2 lines

The second part of the overall deletion job will be when we commit - the updated version of the FS index will be written to the flash media.

Oh, I see. This is what I was missing. And I presume you always make sure
to have enough space for new FS index so it cannot deadlock when trying to push out dirty pages.

Quoted 6 lines · Andrew Morton

Quoted 3 lines · On Fri, 2 Mar 2012 18:39:51 +0800 Fengguang Wu <[email protected]> wrote:

Quoted 2 lines

And I agree it's unlikely but given enough time and people, I believe someone finds a way to (inadvertedly) trigger this.

Quoted 8 lines

Quoted 5 lines

Right. The pageout works could add lots more iput() to the flusher and turn some hidden statistical impossible bugs into real ones.

Fortunately the "flusher deadlocks itself" case is easy to detect and prevent as illustrated in another email.

It would be a heck of a lot safer and saner to avoid the iput(). We know how to do this, so why not do it?

My concern about the page lock is, it costs more code and sounds like hacking around something. It seems we (including me) have been trying to shun away from the iput() problem. Since it's unlikely we are to get rid of the already existing iput() calls from the flusher context, why not face the problem, sort it out and use it with confident in new code?

Let me try it now. The only scheme iput() can deadlock the flusher is for the iput() path to come back to queue some work and wait for it.

Quoted 16 lines · Wu Fengguang

Quoted 6 lines · Andrew Morton

Quoted 3 lines · On Fri, 2 Mar 2012 18:39:51 +0800 Fengguang Wu <[email protected]> wrote:

Quoted 2 lines

And I agree it's unlikely but given enough time and people, I believe someone finds a way to (inadvertedly) trigger this.

Quoted 8 lines

Quoted 5 lines

Right. The pageout works could add lots more iput() to the flusher and turn some hidden statistical impossible bugs into real ones.

Fortunately the "flusher deadlocks itself" case is easy to detect and prevent as illustrated in another email.

It would be a heck of a lot safer and saner to avoid the iput(). We know how to do this, so why not do it?

Quoted 6 lines

My concern about the page lock is, it costs more code and sounds like hacking around something. It seems we (including me) have been trying to shun away from the iput() problem. Since it's unlikely we are to get rid of the already existing iput() calls from the flusher context, why not face the problem, sort it out and use it with confident in new code?

We can get rid of it in the current code - see my patch set. And also we
don't have to introduce new iput() with your patch set... I don't think using ->writepage() directly on a locked page would be a good thing because filesystems tend to ignore it completely (e.g. ext4 if it needs to do an allocation, or btrfs) or are much less efficient than when ->writepages() is used. So I'd prefer going through writeback_single_inode() as the rest of flusher thread.

Quoted 2 lines

Let me try it now. The only scheme iput() can deadlock the flusher is for the iput() path to come back to queue some work and wait for it.

Let me stop you right here. You severely underestimate the complexity of
filesystems :). Take for example ext4. To do truncate you need to start a transaction, to start a transaction, you have to have a space in journal. To have a space in journal, you may have to wait for any other process to finish writing. If that process needs to wait for flusher thread to be able to finish writing, you have a deadlock. And there are other implicit dependencies like this. And it's similar for other filesystems as well. So you really want to make flusher thread as light as possible with the dependencies.