writeback fixes and trace events
Subsystems (6)
cover writeback fixes and trace events
Andrew,
Here are one more collection of simple writeback patches.
two already reviewed fixes
[PATCH 1/6] writeback: add bdi_dirty_limit() kernel-doc [PATCH 2/6] writeback: skip balance_dirty_pages() for in-memory fs
a straightforward write size optimization
[PATCH 3/6] writeback: make nr_to_write a per-file limit
two new trace events
[PATCH 4/6] writeback: trace event writeback_single_inode [PATCH 5/6] writeback: trace event writeback_queue_io
an RFC wbc_writeback_* trace format change
(please drop it if cannot get ACKs from others)
[PATCH 6/6] writeback: convert to relative older_than_this in trace events
Thanks,
Fengguang
PATCH 1/6 writeback: add bdi_dirty_limit() kernel-doc
Clarify the bdi_dirty_limit() comment.
--- mm/page-writeback.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
mm/page-writeback.c +9 -2
@@ -437,10 +437,17 @@ void global_dirty_limits(unsigned long * *pdirty = dirty; } -/* +/** * bdi_dirty_limit - @bdi's share of dirty throttling threshold +* @bdi: the backing_dev_info to query +* @dirty: global dirty limit in pages +* +* Returns @bdi's dirty limit in pages. The term "dirty" in the context of +* dirty balancing includes all PG_dirty, PG_writeback and NFS unstable pages. +* And the "limit" in the name is not seriously taken as hard limit in +* balance_dirty_pages(). * -* Allocate high/low dirty limits to fast/slow devices, in order to prevent +* It allocates high/low dirty limits to fast/slow devices, in order to prevent * - starving fast devices * - piling up dirty pages (that will take long time to sync) on slow devices *
PATCH 2/6 writeback: skip balance_dirty_pages() for in-memory fs
In reply to Wu FengguangThis avoids unnecessary checks and dirty throttling on tmpfs/ramfs.
It can also prevent
[ 388.126563] BUG: unable to handle kernel NULL pointer dereference at 0000000000000050
in the balance_dirty_pages tracepoint, which will call
dev_name(mapping->backing_dev_info->dev)
but shmem_backing_dev_info.dev is NULL.
Summary notes about the tmpfs/ramfs behavior changes:
As for 2.6.36 and older kernels, the tmpfs writes will sleep inside balance_dirty_pages() as long as we are over the (dirty+background)/2 global throttle threshold. This is because both the dirty pages and threshold will be 0 for tmpfs/ramfs. Hence this test will always evaluate to TRUE:
dirty_exceeded =
(bdi_nr_reclaimable + bdi_nr_writeback >= bdi_thresh) || (nr_reclaimable + nr_writeback >= dirty_thresh);
For 2.6.37, someone complained that the current logic does not allow the users to set vm.dirty_ratio=0. So commit 4cbec4c8b9 changed the test to
dirty_exceeded =
(bdi_nr_reclaimable + bdi_nr_writeback > bdi_thresh) || (nr_reclaimable + nr_writeback > dirty_thresh);
So 2.6.37 will behave differently for tmpfs/ramfs: it will never get throttled unless the global dirty threshold is exceeded (which is very unlikely to happen; once happen, will block many tasks).
I'd say that the 2.6.36 behavior is very bad for tmpfs/ramfs. It means for a busy writing server, tmpfs write()s may get livelocked! The "inadvertent" throttling can hardly bring help to any workload because of its "either no throttling, or get throttled to death" property.
So based on 2.6.37, this patch won't bring more noticeable changes.
--- mm/page-writeback.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
mm/page-writeback.c +4 -6
@@ -244,13 +244,8 @@ void task_dirty_inc(struct task_struct *static void bdi_writeout_fraction (struct backing_dev_info *bdi,long *numerator,long *denominator) { -if (bdi_cap_writeback_dirty (bdi)) { -prop_fraction_percpu (&vm_completions, &bdi->completions, +prop_fraction_percpu (&vm_completions, &bdi->completions, numerator, denominator); - }else { - *numerator =0 ; - *denominator =1 ; - } } static inline void task_dirties_fraction(struct task_struct *tsk,
@@ -495,6 +490,9 @@ static void balance_dirty_pages(struct abool dirty_exceeded = false;struct backing_dev_info *bdi = mapping->backing_dev_info; +if (!bdi_cap_account_dirty (bdi)) +return ; +for (;;) {struct writeback_control wbc = { .sync_mode =WB_SYNC_NONE ,
PATCH 3/6 writeback: make nr_to_write a per-file limit
In reply to Wu FengguangThis ensures large dirty files can be written in the full 4MB writeback chunk size, rather than whatever remained quota in wbc->nr_to_write.
--- fs/fs-writeback.c | 11 +++++++++++ include/linux/writeback.h | 1 + 2 files changed, 12 insertions(+)
fs/fs-writeback.c +11 -0
@@ -346,6 +346,8 @@ writeback_single_inode(struct inode *ino struct writeback_control *wbc) {struct address_space *mapping = inode->i_mapping; +long per_file_limit = wbc->per_file_limit; +long uninitialized_var (nr_to_write );unsigned dirty;int ret;
@@ -385,8 +387,16 @@ writeback_single_inode(struct inode *inospin_unlock (&inode->i_lock);spin_unlock (&wb->list_lock); +if (per_file_limit) { + nr_to_write = wbc->nr_to_write; + wbc->nr_to_write = per_file_limit; + } + ret =do_writepages (mapping, wbc); +if (per_file_limit) + wbc->nr_to_write += nr_to_write - per_file_limit; +/* * Make sure to wait on the data before writing out the metadata. * This is important for filesystems that modify metadata on data
@@ -710,6 +720,7 @@ static long wb_writeback(struct bdi_writ wbc.more_io =0 ; wbc.nr_to_write = write_chunk; + wbc.per_file_limit = write_chunk; wbc.pages_skipped =0 ; wbc.inodes_cleaned =0 ;
include/linux/writeback.h +1 -0
@@ -28,6 +28,7 @@ struct writeback_control {older than this */ long nr_to_write;/* Write this many pages, and decrement this for each page written */ +long per_file_limit;/* Write this many pages for one file */ long pages_skipped;/* Pages which were not written */ long inodes_cleaned;/* # of inodes cleaned */
Re: [PATCH 3/6] writeback: make nr_to_write a per-file limit
Quoted 2 lines · Wu Fengguang
This ensures large dirty files can be written in the full 4MB writeback chunk size, rather than whatever remained quota in
wbc->nr_to_write.
I like the high-level idea, but the implementation of overriding nr_to_write and then copying it back seems rather ugly.
The basic problem seems to be that struct writeback_control is designed to control writeback of a single file, but we keep abuse it for writing multiple files in writeback_sb_inodes and its callers.
It seems like we should only build the struct writeback_control from struct wb_writeback_work down in writeback_sb_inodes, even if that means passing some more information to it either in struct wb_writeback_work or on the stack.
Then writeback_sb_inodes can do something like
if (wbc.sync_mode == WB_SYNC_NONE) wbc.nr_to_write = min(MAX_WRITEBACK_PAGES, work->nr_pages); else wbc.nr_to_write = LONG_MAX;
for each inode it writes.
Re: [PATCH 3/6] writeback: make nr_to_write a per-file limit
Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Wu Fengguang
This ensures large dirty files can be written in the full 4MB writeback chunk size, rather than whatever remained quota in
wbc->nr_to_write.I like the high-level idea, but the implementation of overriding
nr_to_writeand then copying it back seems rather ugly.
Quoted 8 lines
The basic problem seems to be that struct
writeback_controlis designed to control writeback of a single file, but we keep abuse it for writing multiple files inwriteback_sb_inodesand its callers.It seems like we should only build the struct
writeback_controlfrom structwb_writeback_workdown inwriteback_sb_inodes, even if that means passing some more information to it either in structwb_writeback_workor on the stack.
Yes it's very reasonable and possible according to your notes in another email.
Quoted 4 lines
Then
writeback_sb_inodescan do something likeif (wbc.sync_mode == WB_SYNC_NONE) wbc.nr_to_write = min(MAX_WRITEBACK_PAGES, work->nr_pages);
I like the min() idea. However it have the side effect of (very possible) smallish IO from balance_dirty_pages(), which may call us with small ->nr_pages.
We may explicitly do "write_chunk = max(MAX_WRITEBACK_PAGES, write_chunk)" in balance_dirty_pages() to retain the old behavior.
Quoted 4 lines
else wbc.nr_to_write = LONG_MAX;for each inode it writes.
Thanks,
Fengguang
Re: [PATCH 3/6] writeback: make nr_to_write a per-file limit
Quoted 11 lines
Quoted 4 lines
Then
writeback_sb_inodescan do something likeif (wbc.sync_mode == WB_SYNC_NONE) wbc.nr_to_write = min(MAX_WRITEBACK_PAGES, work->nr_pages);I like the
min()idea. However it have the side effect of (very possible) smallish IO frombalance_dirty_pages(), which may call us with small ->nr_pages.We may explicitly do "
write_chunk=max(MAX_WRITEBACK_PAGES, write_chunk)" inbalance_dirty_pages()to retain the old behavior.
Sorry I was confused and please ignore the above. At least VFS won't enlarge the writeback request from balance_dirty_pages()..
Here is the scratch patch. I'll need to double check it tomorrow, but it's already running fine in the dd+tar+sync test and get pretty good performance numbers.
# test-tar-dd.sh
[...]
246.62 1941.38
1000+0 records in
1000+0 records out
1048576000 bytes (1.0 GB) copied, 11.4383 s, 91.7 MB/s
dd if=/dev/zero of=/fs/zero bs=1M count=1000 0.00s user 2.57s system 22% cpu 11.499 total
tar jxf /dev/shm/linux-2.6.38.3.tar.bz2 12.03s user 4.36s system 56% cpu 28.770 total
286.04 2204.50
elapsed: 263.23000000000025Thanks,
Fengguang
---
Subject: writeback: make writeback_control.nr_to_write straight
Date: Wed May 04 19:54:37 CST 2011
As suggested by Christoph:
Pass struct wb_writeback_work all the way down to writeback_sb_inodes(), and initialize the struct writeback_control there.
struct writeback_control is basically designed to control writeback of a single file, but we keep abuse it for writing multiple files in writeback_sb_inodes() and its callers.
It immediately clean things up, e.g. suddenly wbc.nr_to_write vs work->nr_pages starts to make sense, and instead of saving and restoring pages_skipped in writeback_sb_inodes it can always start with a clean zero value.
It also makes a neat IO pattern change: large dirty files are now written in the full 4MB writeback chunk size, rather than whatever remained quota in wbc->nr_to_write.
change set:
- move writeback_control from wb_writeback() down to writeback_sb_inodes()
- change return value of writeback_sb_inodes()/__writeback_inodes_wb() to enum writeback_progress
- move MAX_WRITEBACK_PAGES and wb_writeback_work definitions to writeback.h
- add wb_writeback_work.older_than_this
- remove writeback_control.inodes_cleaned
- remove wbc_writeback_* trace events for now
--- fs/fs-writeback.c | 187 +++++++++++++++--------------------- include/linux/writeback.h | 29 +++++ mm/backing-dev.c | 6 - mm/page-writeback.c | 11 -- 4 files changed, 116 insertions(+), 117 deletions(-)
fs/fs-writeback.c +82 -105
@@ -30,22 +30,6 @@#include "internal.h" /* -* Passed into wb_writeback(), essentially a subset of writeback_control -*/ -struct wb_writeback_work { -long nr_pages; -struct super_block *sb; -enum writeback_sync_modes sync_mode; -unsigned int tagged_sync:1 ; -unsigned int for_kupdate:1 ; -unsigned int range_cyclic:1 ; -unsigned int for_background:1 ; - -struct list_head list;/* pending work list */ -struct completion *done;/* set if the caller waits */ -}; - -/* * Include the creation of the trace points after defining the * wb_writeback_work structure so that the definition remains local to this * file.
@@ -463,7 +447,6 @@ writeback_single_inode(struct inode *ino* No need to add it back to the LRU. */ list_del_init (&inode->i_wb_list); - wbc->inodes_cleaned++; } }inode_sync_complete (inode);
@@ -502,19 +485,53 @@ static bool pin_sb_for_writeback(struct* If @only_this_sb is true, then find and write all such * inodes. Otherwise write only ones which go sequentially * in reverse order. -* -* Return 1, if the caller writeback routine should be -* interrupted. Otherwise return 0. */ -static int writeback_sb_inodes (struct super_block *sb,struct bdi_writeback *wb, -struct writeback_control *wbc,bool only_this_sb) +enum writeback_progress { +WROTE_FULL_CHUNK , +WROTE_SOME_PAGES , +WROTE_SOME_INODES , +WROTE_NOTHING , +}; + +static int writeback_sb_inodes (struct super_block *sb, +struct bdi_writeback *wb, +struct wb_writeback_work *work) { +struct writeback_control wbc = { + .sync_mode = work->sync_mode, + .tagged_sync = work->tagged_sync, + .older_than_this = work->older_than_this, + .for_kupdate = work->for_kupdate, + .for_background = work->for_background, + .range_cyclic = work->range_cyclic, + .range_start = 0; + .range_end = LLONG_MAX; + }; +long write_chunk =MAX_WRITEBACK_PAGES ; +long wrote =0 ; +bool inode_cleaned = false; + +/* +* WB_SYNC_ALL mode does livelock avoidance by syncing dirty +* inodes/pages in one big loop. Setting wbc.nr_to_write=LONG_MAX +* here avoids calling into writeback_inodes_wb() more than once. +* +* The intended call sequence for WB_SYNC_ALL writeback is: +* +* wb_writeback() +* writeback_sb_inodes() <== called only once +* write_cache_pages() <== called once for each inode +* (quickly) tag currently dirty pages +* (maybe slowly) sync all tagged pages +*/ +if (work->sync_mode ==WB_SYNC_ALL || work->tagged_sync) + write_chunk =LONG_MAX ; +while (!list_empty (&wb->b_io)) { -long pages_skipped;struct inode *inode =wb_inode (wb->b_io.prev);if (inode->i_sb != sb) { -if (only_this_sb) { +if (work->sb) {/* * We only want to write back data for this * superblock, move all inodes not belonging
@@ -529,7 +546,7 @@ static int writeback_sb_inodes(struct su* Bounce back to the caller to unpin this and * pin the next superblock. */ -return 0 ; +break ; }/*
@@ -543,34 +560,44 @@ static int writeback_sb_inodes(struct surequeue_io (inode, wb);continue ; } -__iget (inode); + write_chunk =min (write_chunk, work->nr_pages); + wbc.nr_to_write = write_chunk; + wbc.pages_skipped =0 ; + +writeback_single_inode (inode, wb, &wbc); - pages_skipped = wbc->pages_skipped; -writeback_single_inode (inode, wb, wbc); -if (wbc->pages_skipped != pages_skipped) { + work->nr_pages -= write_chunk - wbc.nr_to_write; + wrote += write_chunk - wbc.nr_to_write; +if (wbc.pages_skipped) {/* * writeback is not making progress due to locked * buffers. Skip this inode for now. */ redirty_tail (inode, wb); - } + }else if (!(inode->i_state &I_DIRTY )) + inode_cleaned = true;spin_unlock (&inode->i_lock);spin_unlock (&wb->list_lock);iput (inode);cond_resched ();spin_lock (&wb->list_lock); -if (wbc->nr_to_write <=0 ) -return 1 ; +if (wrote >=MAX_WRITEBACK_PAGES ) +return WROTE_FULL_CHUNK ; +if (work->nr_pages <=0 ) +return WROTE_FULL_CHUNK ; } -/* b_io is empty */ -return 1 ; +if (wrote) +return WROTE_SOME_PAGES ; +if (inode_cleaned) +return WROTE_SOME_INODES ; +return WROTE_NOTHING ; } -static void __writeback_inodes_wb (struct bdi_writeback *wb, -struct writeback_control *wbc) +static int __writeback_inodes_wb (struct bdi_writeback *wb, +struct wb_writeback_work *work) { -int ret =0 ; +int ret =WROTE_NOTHING ;while (!list_empty (&wb->b_io)) {struct inode *inode =wb_inode (wb->b_io.prev);
@@ -580,34 +607,26 @@ static void __writeback_inodes_wb(structrequeue_io (inode, wb);continue ; } - ret =writeback_sb_inodes (sb, wb, wbc, false); + ret =writeback_sb_inodes (sb, wb, work);drop_super (sb); -if (ret) +if (ret ==WROTE_FULL_CHUNK )break ; }/* Leave any unwritten inodes on b_io */ +return ret; }void writeback_inodes_wb (struct bdi_writeback *wb, -struct writeback_control *wbc) +struct wb_writeback_work *work) {spin_lock (&wb->list_lock);if (list_empty (&wb->b_io)) -queue_io (wb, wbc->older_than_this); -__writeback_inodes_wb (wb, wbc); +queue_io (wb, work->older_than_this); +__writeback_inodes_wb (wb, work);spin_unlock (&wb->list_lock); } -/* -* The maximum number of pages to writeout in a single bdi flush/kupdate -* operation. We do this so we don't hold I_SYNC against an inode for -* enormous amounts of time, which would block a userspace task which has -* been forced to throttle against that inode. Also, the code reevaluates -* the dirty each time it has written this many pages. -*/ -#define MAX_WRITEBACK_PAGES 1024 -static inline bool over_bground_thresh (void ) {unsigned long background_thresh, dirty_thresh;
@@ -636,42 +655,12 @@ static inline bool over_bground_thresh(vstatic long wb_writeback (struct bdi_writeback *wb,struct wb_writeback_work *work) { -struct writeback_control wbc = { - .sync_mode = work->sync_mode, - .tagged_sync = work->tagged_sync, - .older_than_this =NULL , - .for_kupdate = work->for_kupdate, - .for_background = work->for_background, - .range_cyclic = work->range_cyclic, - }; -unsigned long oldest_jif; -long wrote =0 ; -long write_chunk =MAX_WRITEBACK_PAGES ; +long nr_pages = work->nr_pages; +unsigned long oldest_jif = jiffies;struct inode *inode; +int progress; -if (!wbc.range_cyclic) { - wbc.range_start =0 ; - wbc.range_end =LLONG_MAX ; - } - -/* -* WB_SYNC_ALL mode does livelock avoidance by syncing dirty -* inodes/pages in one big loop. Setting wbc.nr_to_write=LONG_MAX -* here avoids calling into writeback_inodes_wb() more than once. -* -* The intended call sequence for WB_SYNC_ALL writeback is: -* -* wb_writeback() -* writeback_sb_inodes() <== called only once -* write_cache_pages() <== called once for each inode -* (quickly) tag currently dirty pages -* (maybe slowly) sync all tagged pages -*/ -if (wbc.sync_mode ==WB_SYNC_ALL || wbc.tagged_sync) { - write_chunk =LONG_MAX ; - oldest_jif = jiffies; - wbc.older_than_this = &oldest_jif; - } + work->older_than_this = &oldest_jif;for (;;) {/*
@@ -700,27 +689,18 @@ static long wb_writeback(struct bdi_writif (work->for_kupdate || work->for_background) { oldest_jif = jiffies -msecs_to_jiffies (dirty_expire_interval *10 ); - wbc.older_than_this = &oldest_jif; + work->older_than_this = &oldest_jif; } - wbc.nr_to_write = write_chunk; - wbc.pages_skipped =0 ; - wbc.inodes_cleaned =0 ; - retry: -trace_wbc_writeback_start (&wbc, wb->bdi);spin_lock (&wb->list_lock);if (list_empty (&wb->b_io)) -queue_io (wb, wbc->older_than_this); +queue_io (wb, work->older_than_this);if (work->sb) -writeback_sb_inodes (work->sb, wb, &wbc, true); + progress =writeback_sb_inodes (work->sb, wb, work);else -__writeback_inodes_wb (wb, &wbc); + progress =__writeback_inodes_wb (wb, work);spin_unlock (&wb->list_lock); -trace_wbc_writeback_written (&wbc, wb->bdi); - - work->nr_pages -= write_chunk - wbc.nr_to_write; - wrote += write_chunk - wbc.nr_to_write;/* * Did we write something? Try for more
@@ -730,9 +710,7 @@ retry:* mean the overall work is done. So we keep looping as long * as made some progress on cleaning pages or inodes. */ -if (wbc.nr_to_write < write_chunk) -continue ; -if (wbc.inodes_cleaned) +if (progress !=WROTE_NOTHING )continue ;/* * background writeback will start with expired inodes, and
@@ -741,10 +719,10 @@ retry:* lists and cause trouble to the page reclaim. */ if (work->for_background && - wbc.older_than_this && + work->older_than_this &&list_empty (&wb->b_io) &&list_empty (&wb->b_more_io)) { - wbc.older_than_this =NULL ; + work->older_than_this =NULL ; goto retry; }/*
@@ -760,7 +738,6 @@ retry:spin_lock (&wb->list_lock);if (!list_empty (&wb->b_more_io)) { inode =wb_inode (wb->b_more_io.prev); -trace_wbc_writeback_wait (&wbc, wb->bdi);spin_lock (&inode->i_lock);inode_wait_for_writeback (inode, wb);spin_unlock (&inode->i_lock);
@@ -768,7 +745,7 @@ retry:spin_unlock (&wb->list_lock); } -return wrote; +return nr_pages - work->nr_pages; }/*
mm/backing-dev.c +3 -3
@@ -262,14 +262,14 @@ int bdi_has_dirty_io(struct backing_dev_static void bdi_flush_io (struct backing_dev_info *bdi) { -struct writeback_control wbc = { +struct wb_writeback_work work = { .sync_mode =WB_SYNC_NONE , .older_than_this =NULL , .range_cyclic =1 , - .nr_to_write =1024 , + .nr_pages =1024 , }; -writeback_inodes_wb (&bdi->wb, &wbc); +writeback_inodes_wb (&bdi->wb, &work); }/*
mm/page-writeback.c +4 -7
@@ -494,10 +494,10 @@ static void balance_dirty_pages(struct areturn ;for (;;) { -struct writeback_control wbc = { +struct wb_writeback_work work = { .sync_mode =WB_SYNC_NONE , .older_than_this =NULL , - .nr_to_write = write_chunk, + .nr_pages = write_chunk, .range_cyclic =1 , };
@@ -562,15 +562,12 @@ static void balance_dirty_pages(struct a* threshold otherwise wait until the disk writes catch * up. */ -trace_wbc_balance_dirty_start (&wbc, bdi);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); +writeback_inodes_wb (&bdi->wb, &work); + pages_written += write_chunk - work.nr_pages;if (pages_written >= write_chunk)break ;/* We've done our duty */ } -trace_wbc_balance_dirty_wait (&wbc, bdi);__set_current_state (TASK_UNINTERRUPTIBLE );io_schedule_timeout (pause);
include/linux/writeback.h +27 -2
@@ -7,6 +7,15 @@#include <linux/sched.h> #include <linux/fs.h> +/* +* The maximum number of pages to writeout in a single bdi flush/kupdate +* operation. We do this so we don't hold I_SYNC against an inode for +* enormous amounts of time, which would block a userspace task which has +* been forced to throttle against that inode. Also, the code reevaluates +* the dirty each time it has written this many pages. +*/ +#define MAX_WRITEBACK_PAGES 1024 +struct backing_dev_info ;/*
@@ -29,7 +38,6 @@ struct writeback_control {long nr_to_write;/* Write this many pages, and decrement this for each page written */ long pages_skipped;/* Pages which were not written */ -long inodes_cleaned;/* # of inodes cleaned */ /* * For a_ops->writepages(): is start or end are non-zero then this is
@@ -49,6 +57,23 @@ struct writeback_control { };/* +* Passed into wb_writeback(), essentially a subset of writeback_control +*/ +struct wb_writeback_work { +long nr_pages; +struct super_block *sb; +enum writeback_sync_modes sync_mode; +unsigned long *older_than_this; +unsigned int tagged_sync:1 ; +unsigned int for_kupdate:1 ; +unsigned int range_cyclic:1 ; +unsigned int for_background:1 ; + +struct list_head list;/* pending work list */ +struct completion *done;/* set if the caller waits */ +}; + +/* * fs/fs-writeback.c */ struct bdi_writeback ;
@@ -59,7 +84,7 @@ int writeback_inodes_sb_if_idle(struct sint writeback_inodes_sb_nr_if_idle (struct super_block *,unsigned long nr);void sync_inodes_sb (struct super_block *);void writeback_inodes_wb (struct bdi_writeback *wb, -struct writeback_control *wbc); +struct wb_writeback_work *work);long wb_do_writeback (struct bdi_writeback *wb,int force_wait);void wakeup_flusher_threads (long nr_pages);
Re: [PATCH 3/6] writeback: make nr_to_write a per-file limit
Quoted 1 line
- move
MAX_WRITEBACK_PAGESandwb_writeback_workdefinitions towriteback.h
I think it would be a good idea to keep this in fs/fs-writeback.c, which means we'd need a small writeback_inodes_wb wrapper for balance_dirty_pages and bdi_flush_io. But IIRC your tree already has __writeback_inodes_wb for use in wb_writeback, so writeback_inodes_wb could be that wrapper.
Quoted 9 lines
fs/fs-writeback.c +9
-0+long write_chunk =MAX_WRITEBACK_PAGES ; +long wrote =0 ; +bool inode_cleaned = false; + +/* +* WB_SYNC_ALL mode does livelock avoidance by syncing dirty +* inodes/pages in one big loop. Setting wbc.nr_to_write=LONG_MAX +* here avoids calling into writeback_inodes_wb() more than once. +*
fs/fs-writeback.c static bool pin_sb_for_writeback(struct · line 528
Quoted 10 lines
fs/fs-writeback.c +10
-0+* The intended call sequence for WB_SYNC_ALL writeback is: +* +* wb_writeback() +* writeback_sb_inodes() <== called only once +* write_cache_pages() <== called once for each inode +* (quickly) tag currently dirty pages +* (maybe slowly) sync all tagged pages +*/ +if (work->sync_mode ==WB_SYNC_ALL || work->tagged_sync) + write_chunk =LONG_MAX ;
I think this would be easier to read if kept as and if / else clause with the MAX_WRITEBACK_PAGES usage.
fs/fs-writeback.c static int writeback_sb_inodes(struct su · line 564
Quoted 1 line
fs/fs-writeback.c +1
-0+ write_chunk =min (write_chunk, work->nr_pages);
Or in fact done here - for the WB_SYNC_ALL case LONG_MAX should always be larger than work->nr_pages, so the whole thing could be simplified to:
if (work->sync_mode == WB_SYNC_ALL || work->tagged_sync) write_chunk = LONG_MAX; else write_chunk = min(MAX_WRITEBACK_PAGES, work->nr_pages);
Other notes:
- older_than_this in writeback_control shouldn't be needed anymore
- is the early return for the mis-matching sb in writeback_sb_inodes handled correctly? Before it had the special 0 return value, and I'm not quite sure how that fits into your new enum scheme.
Re: [PATCH 3/6] writeback: make nr_to_write a per-file limit
Quoted 7 lines · Christoph Hellwig
Quoted 1 line
- move
MAX_WRITEBACK_PAGESandwb_writeback_workdefinitions towriteback.hI think it would be a good idea to keep this in
fs/fs-writeback.c, which means we'd need a smallwriteback_inodes_wbwrapper forbalance_dirty_pagesandbdi_flush_io. But IIRC your tree already has__writeback_inodes_wbfor use inwb_writeback, sowriteback_inodes_wbcould be that wrapper.
No problem. We can later move bdi_flush_io() to fs-writeback.c and kill the wbc=>work=>wbc double conversions once balance_dirty_pages() switches to IO-less.
Quoted 29 lines
Quoted 9 lines
(quoted diff) +9
-0+ long write_chunk = MAX_WRITEBACK_PAGES; + long wrote = 0; + bool inode_cleaned = false; + + /* + * WB_SYNC_ALL mode does livelock avoidance by syncing dirty + * inodes/pages in one big loop. Setting wbc.nr_to_write=LONG_MAX + * here avoids calling into writeback_inodes_wb() more than once. + *Review
Quoted 10 lines
(quoted diff) +10
-0+ * The intended call sequence for WB_SYNC_ALL writeback is: + * + * wb_writeback() + * writeback_sb_inodes() <== called only once + * write_cache_pages() <== called once for each inode + * (quickly) tag currently dirty pages + * (maybe slowly) sync all tagged pages + */ + if (work->sync_mode == WB_SYNC_ALL || work->tagged_sync) + write_chunk = LONG_MAX;I think this would be easier to read if kept as and if / else clause with the
MAX_WRITEBACK_PAGESusage.Review
Quoted 1 line
(quoted diff) +1
-0+ write_chunk = min(write_chunk, work->nr_pages);Or in fact done here - for the
WB_SYNC_ALLcaseLONG_MAXshould always be larger thanwork->nr_pages, so the whole thing could be simplified to:
Quoted 4 lines
if (work->sync_mode == WB_SYNC_ALL || work->tagged_sync) write_chunk = LONG_MAX; else write_chunk = min(MAX_WRITEBACK_PAGES, work->nr_pages);
OK, I created a standalone function writeback_chunk_size() for that. Otherwise the comment block will look very weird deep inside the function and deeply indented.
Quoted 4 lines
Other notes:
-
older_than_thisinwriteback_controlshouldn't be needed anymore
Yes. Good catch!
Quoted 3 lines
- is the early return for the mis-matching sb in
writeback_sb_inodeshandled correctly? Before it had the special 0 return value, and I'm not quite sure how that fits into your new enum scheme.
Good question. This is the tricky point of the patch.
The original 0/1 return values are to tell __writeback_inodes_wb() whether it's done with the current batch. Now __writeback_inodes_wb() will do the test on itself (the below v2 patch).
The v1 patch does have some tricky problem regarding the early return:
if there are two super blocks that return WROTE_SOME_PAGES and WROTE_NOTHING in turn, __writeback_inodes_wb() will wrongly pick up the latter as the final return value.
I updated the patch to accumulate the number of pages/inodes cleaned over multiple writeback_sb_inodes() invocations inside __writeback_inodes_wb(). This way it can reliably return to wb_writeback() to check various terminate conditions when made enough progress.
Thanks,
Fengguang
---
Subject: writeback: make writeback_control.nr_to_write straight
Date: Wed May 04 19:54:37 CST 2011
As suggested by Christoph:
Pass struct wb_writeback_work all the way down to writeback_sb_inodes(), and initialize the struct writeback_control there.
struct writeback_control is basically designed to control writeback of a single file, but we keep abuse it for writing multiple files in writeback_sb_inodes() and its callers.
It immediately clean things up, e.g. suddenly wbc.nr_to_write vs work->nr_pages starts to make sense, and instead of saving and restoring pages_skipped in writeback_sb_inodes it can always start with a clean zero value.
It also makes a neat IO pattern change: large dirty files are now written in the full 4MB writeback chunk size, rather than whatever remained quota in wbc->nr_to_write.
change set:
- move writeback_control from wb_writeback() down to writeback_sb_inodes()
- change return value of writeback_sb_inodes()/__writeback_inodes_wb() to the number of pages and/or inodes written
- move writeback_control.older_than_this to struct wb_writeback_work
- remove writeback_control.inodes_cleaned
- remove wbc_writeback_* trace events for now
--- fs/btrfs/extent_io.c | 2 fs/fs-writeback.c | 184 +++++++++++++++-------------- include/linux/writeback.h | 3 include/trace/events/writeback.h | 6 mm/backing-dev.c | 1 mm/page-writeback.c | 1 6 files changed, 99 insertions(+), 98 deletions(-)
fs/fs-writeback.c +98 -86
@@ -30,11 +30,21 @@#include "internal.h" /* +* The maximum number of pages to writeout in a single bdi flush/kupdate +* operation. We do this so we don't hold I_SYNC against an inode for +* enormous amounts of time, which would block a userspace task which has +* been forced to throttle against that inode. Also, the code reevaluates +* the dirty each time it has written this many pages. +*/ +#define MAX_WRITEBACK_PAGES 1024L + +/* * Passed into wb_writeback(), essentially a subset of writeback_control */ struct wb_writeback_work {long nr_pages;struct super_block *sb; +unsigned long *older_than_this;enum writeback_sync_modes sync_mode;unsigned int tagged_sync:1 ;unsigned int for_kupdate:1 ;
@@ -463,7 +473,6 @@ writeback_single_inode(struct inode *ino* No need to add it back to the LRU. */ list_del_init (&inode->i_wb_list); - wbc->inodes_cleaned++; } }inode_sync_complete (inode);
@@ -496,6 +505,31 @@ static bool pin_sb_for_writeback(structreturn false; } +static long writeback_chunk_size (struct wb_writeback_work *work) +{ +long pages; + +/* +* WB_SYNC_ALL mode does livelock avoidance by syncing dirty +* inodes/pages in one big loop. Setting wbc.nr_to_write=LONG_MAX +* here avoids calling into writeback_inodes_wb() more than once. +* +* The intended call sequence for WB_SYNC_ALL writeback is: +* +* wb_writeback() +* writeback_sb_inodes() <== called only once +* write_cache_pages() <== called once for each inode +* (quickly) tag currently dirty pages +* (maybe slowly) sync all tagged pages +*/ +if (work->sync_mode ==WB_SYNC_ALL || work->tagged_sync) + pages =LONG_MAX ; +else + pages =min (MAX_WRITEBACK_PAGES , work->nr_pages); + +return pages; +} +/* * Write a portion of b_io inodes which belong to @sb. *
@@ -503,18 +537,29 @@ static bool pin_sb_for_writeback(struct* inodes. Otherwise write only ones which go sequentially * in reverse order. * -* Return 1, if the caller writeback routine should be -* interrupted. Otherwise return 0. +* Return the number of pages and/or inodes cleaned. */ -static int writeback_sb_inodes (struct super_block *sb,struct bdi_writeback *wb, -struct writeback_control *wbc,bool only_this_sb) +static long writeback_sb_inodes (struct super_block *sb, +struct bdi_writeback *wb, +struct wb_writeback_work *work) { +struct writeback_control wbc = { + .sync_mode = work->sync_mode, + .tagged_sync = work->tagged_sync, + .for_kupdate = work->for_kupdate, + .for_background = work->for_background, + .range_cyclic = work->range_cyclic, + .range_start =0 , + .range_end =LLONG_MAX , + }; +long write_chunk; +long wrote =0 ;/* count both pages and inodes */ +while (!list_empty (&wb->b_io)) { -long pages_skipped;struct inode *inode =wb_inode (wb->b_io.prev);if (inode->i_sb != sb) { -if (only_this_sb) { +if (work->sb) {/* * We only want to write back data for this * superblock, move all inodes not belonging
@@ -529,7 +574,7 @@ static int writeback_sb_inodes(struct su* Bounce back to the caller to unpin this and * pin the next superblock. */ -return 0 ; +break ; }/*
@@ -543,34 +588,40 @@ static int writeback_sb_inodes(struct surequeue_io (inode, wb);continue ; } -__iget (inode); + write_chunk =writeback_chunk_size (work); + wbc.nr_to_write = write_chunk; + wbc.pages_skipped =0 ; + +writeback_single_inode (inode, wb, &wbc); - pages_skipped = wbc->pages_skipped; -writeback_single_inode (inode, wb, wbc); -if (wbc->pages_skipped != pages_skipped) { + work->nr_pages -= write_chunk - wbc.nr_to_write; + wrote += write_chunk - wbc.nr_to_write; +if (wbc.pages_skipped) {/* * writeback is not making progress due to locked * buffers. Skip this inode for now. */ redirty_tail (inode, wb); - } + }else if (!(inode->i_state &I_DIRTY )) + wrote++;spin_unlock (&inode->i_lock);spin_unlock (&wb->list_lock);iput (inode);cond_resched ();spin_lock (&wb->list_lock); -if (wbc->nr_to_write <=0 ) -return 1 ; +if (wrote >=MAX_WRITEBACK_PAGES ) +break ; +if (work->nr_pages <=0 ) +break ; } -/* b_io is empty */ -return 1 ; +return wrote; } -static void __writeback_inodes_wb (struct bdi_writeback *wb, -struct writeback_control *wbc) +static long __writeback_inodes_wb (struct bdi_writeback *wb, +struct wb_writeback_work *work) { -int ret =0 ; +long wrote =0 ;while (!list_empty (&wb->b_io)) {struct inode *inode =wb_inode (wb->b_io.prev);
@@ -580,33 +631,35 @@ static void __writeback_inodes_wb(structrequeue_io (inode, wb);continue ; } - ret =writeback_sb_inodes (sb, wb, wbc, false); + wrote +=writeback_sb_inodes (sb, wb, work);drop_super (sb); -if (ret) +if (wrote >=MAX_WRITEBACK_PAGES ) +break ; +if (work->nr_pages <=0 )break ; }/* Leave any unwritten inodes on b_io */ +return wrote; }void writeback_inodes_wb (struct bdi_writeback *wb, -struct writeback_control *wbc) +struct writeback_control *wbc) { +struct wb_writeback_work work = { + .nr_pages = wbc->nr_to_write, + .sync_mode = wbc->sync_mode, + .range_cyclic = wbc->range_cyclic, + }; +spin_lock (&wb->list_lock);if (list_empty (&wb->b_io)) -queue_io (wb, wbc->older_than_this); -__writeback_inodes_wb (wb, wbc); +queue_io (wb,NULL ); +__writeback_inodes_wb (wb, &work);spin_unlock (&wb->list_lock); -} -/* -* The maximum number of pages to writeout in a single bdi flush/kupdate -* operation. We do this so we don't hold I_SYNC against an inode for -* enormous amounts of time, which would block a userspace task which has -* been forced to throttle against that inode. Also, the code reevaluates -* the dirty each time it has written this many pages. -*/ -#define MAX_WRITEBACK_PAGES 1024 + wbc->nr_to_write = work.nr_pages; +}static inline bool over_bground_thresh (void ) {
@@ -636,42 +689,13 @@ static inline bool over_bground_thresh(vstatic long wb_writeback (struct bdi_writeback *wb,struct wb_writeback_work *work) { -struct writeback_control wbc = { - .sync_mode = work->sync_mode, - .tagged_sync = work->tagged_sync, - .older_than_this =NULL , - .for_kupdate = work->for_kupdate, - .for_background = work->for_background, - .range_cyclic = work->range_cyclic, - }; +long nr_pages = work->nr_pages;unsigned long oldest_jif; -long wrote =0 ; -long write_chunk =MAX_WRITEBACK_PAGES ;struct inode *inode; - -if (!wbc.range_cyclic) { - wbc.range_start =0 ; - wbc.range_end =LLONG_MAX ; - } - -/* -* WB_SYNC_ALL mode does livelock avoidance by syncing dirty -* inodes/pages in one big loop. Setting wbc.nr_to_write=LONG_MAX -* here avoids calling into writeback_inodes_wb() more than once. -* -* The intended call sequence for WB_SYNC_ALL writeback is: -* -* wb_writeback() -* writeback_sb_inodes() <== called only once -* write_cache_pages() <== called once for each inode -* (quickly) tag currently dirty pages -* (maybe slowly) sync all tagged pages -*/ -if (wbc.sync_mode ==WB_SYNC_ALL || wbc.tagged_sync) - write_chunk =LONG_MAX ; +long progress; oldest_jif = jiffies; - wbc.older_than_this = &oldest_jif; + work->older_than_this = &oldest_jif;for (;;) {/*
@@ -700,27 +724,18 @@ static long wb_writeback(struct bdi_writif (work->for_kupdate || work->for_background) { oldest_jif = jiffies -msecs_to_jiffies (dirty_expire_interval *10 ); - wbc.older_than_this = &oldest_jif; + work->older_than_this = &oldest_jif; } - wbc.nr_to_write = write_chunk; - wbc.pages_skipped =0 ; - wbc.inodes_cleaned =0 ; - retry: -trace_wbc_writeback_start (&wbc, wb->bdi);spin_lock (&wb->list_lock);if (list_empty (&wb->b_io)) -queue_io (wb, wbc->older_than_this); +queue_io (wb, work->older_than_this);if (work->sb) -writeback_sb_inodes (work->sb, wb, &wbc, true); + progress =writeback_sb_inodes (work->sb, wb, work);else -__writeback_inodes_wb (wb, &wbc); + progress =__writeback_inodes_wb (wb, work);spin_unlock (&wb->list_lock); -trace_wbc_writeback_written (&wbc, wb->bdi); - - work->nr_pages -= write_chunk - wbc.nr_to_write; - wrote += write_chunk - wbc.nr_to_write;/* * Did we write something? Try for more
@@ -730,9 +745,7 @@ retry:* mean the overall work is done. So we keep looping as long * as made some progress on cleaning pages or inodes. */ -if (wbc.nr_to_write < write_chunk) -continue ; -if (wbc.inodes_cleaned) +if (progress)continue ;/* * background writeback will start with expired inodes, and
@@ -741,10 +754,10 @@ retry:* lists and cause trouble to the page reclaim. */ if (work->for_background && - wbc.older_than_this && + work->older_than_this &&list_empty (&wb->b_io) &&list_empty (&wb->b_more_io)) { - wbc.older_than_this =NULL ; + work->older_than_this =NULL ; goto retry; }/*
@@ -760,7 +773,6 @@ retry:spin_lock (&wb->list_lock);if (!list_empty (&wb->b_more_io)) { inode =wb_inode (wb->b_more_io.prev); -trace_wbc_writeback_wait (&wbc, wb->bdi);spin_lock (&inode->i_lock);inode_wait_for_writeback (inode, wb);spin_unlock (&inode->i_lock);
@@ -768,7 +780,7 @@ retry:spin_unlock (&wb->list_lock); } -return wrote; +return nr_pages - work->nr_pages; }/*
include/linux/writeback.h +0 -3
@@ -24,12 +24,9 @@ enum writeback_sync_modes {*/ struct writeback_control {enum writeback_sync_modes sync_mode; -unsigned long *older_than_this;/* If !NULL, only write back inodes -older than this */ long nr_to_write;/* Write this many pages, and decrement this for each page written */ long pages_skipped;/* Pages which were not written */ -long inodes_cleaned;/* # of inodes cleaned */ /* * For a_ops->writepages(): is start or end are non-zero then this is
mm/backing-dev.c +0 -1
@@ -264,7 +264,6 @@ static void bdi_flush_io(struct backing_ {struct writeback_control wbc = { .sync_mode =WB_SYNC_NONE , - .older_than_this =NULL , .range_cyclic =1 , .nr_to_write =1024 , };
mm/page-writeback.c +0 -1
@@ -496,7 +496,6 @@ static void balance_dirty_pages(struct afor (;;) {struct writeback_control wbc = { .sync_mode =WB_SYNC_NONE , - .older_than_this =NULL , .nr_to_write = write_chunk, .range_cyclic =1 , };
include/trace/events/writeback.h +1 -5
@@ -101,7 +101,6 @@ DECLARE_EVENT_CLASS(wbc_class, __field(int, for_background) __field(int, for_reclaim) __field(int, range_cyclic) - __field(unsigned long, older_than_this) __field(long, range_start)__field (long, range_end) ),
@@ -115,14 +114,12 @@ DECLARE_EVENT_CLASS(wbc_class, __entry->for_background = wbc->for_background; __entry->for_reclaim = wbc->for_reclaim; __entry->range_cyclic = wbc->range_cyclic; - __entry->older_than_this = wbc->older_than_this ? - *wbc->older_than_this :0 ; __entry->range_start = (long )wbc->range_start; __entry->range_end = (long )wbc->range_end; ), TP_printk("bdi %s: towrt=%ld skip=%ld mode=%d kupd=%d " - "bgrd=%d reclm=%d cyclic=%d older=0x%lx " + "bgrd=%d reclm=%d cyclic=%d " "start=0x%lx end=0x%lx", __entry->name, __entry->nr_to_write,
@@ -132,7 +129,6 @@ DECLARE_EVENT_CLASS(wbc_class, __entry->for_background, __entry->for_reclaim, __entry->range_cyclic, - __entry->older_than_this, __entry->range_start, __entry->range_end) )
fs/btrfs/extent_io.c +0 -2
@@ -2587,7 +2587,6 @@ int extent_write_full_page(struct extent };struct writeback_control wbc_writepages = { .sync_mode = wbc->sync_mode, - .older_than_this =NULL , .nr_to_write =64 , .range_start =page_offset (page) +PAGE_CACHE_SIZE , .range_end = (loff_t)-1 ,
@@ -2620,7 +2619,6 @@ int extent_write_locked_range(struct ext };struct writeback_control wbc_writepages = { .sync_mode = mode, - .older_than_this =NULL , .nr_to_write = nr_pages *2 , .range_start = start, .range_end = end +1 ,
PATCH 4/6 writeback: trace event writeback_single_inode
In reply to Wu FengguangIt is valuable to know how the dirty inodes are iterated and their IO size.
"writeback_single_inode: bdi 8:0: ino=134246746 state=I_DIRTY_SYNC|I_SYNC age=414 index=0 wrote=0 to_write=1024"
- "state" reflects inode->i_state at the end of writeback_single_inode()
- "index" reflects mapping->writeback_index after the ->writepages() call
- "wrote" is the number of pages written in this writeback_single_inode()
- "to_write" is the remained wbc->nr_to_write
--- fs/fs-writeback.c | 12 +++--- include/trace/events/writeback.h | 56 +++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 5 deletions(-)
fs/fs-writeback.c +7 -5
@@ -347,7 +347,7 @@ writeback_single_inode(struct inode *ino {struct address_space *mapping = inode->i_mapping;long per_file_limit = wbc->per_file_limit; -long uninitialized_var (nr_to_write ); +long nr_to_write = wbc->nr_to_write;unsigned dirty;int ret;
@@ -370,7 +370,8 @@ writeback_single_inode(struct inode *ino*/ if (wbc->sync_mode !=WB_SYNC_ALL ) {requeue_io (inode, wb); -return 0 ; + ret =0 ; + goto out; }/*
@@ -387,10 +388,8 @@ writeback_single_inode(struct inode *inospin_unlock (&inode->i_lock);spin_unlock (&wb->list_lock); -if (per_file_limit) { - nr_to_write = wbc->nr_to_write; +if (per_file_limit) wbc->nr_to_write = per_file_limit; - } ret =do_writepages (mapping, wbc);
@@ -477,6 +476,9 @@ writeback_single_inode(struct inode *ino } }inode_sync_complete (inode); +out: +trace_writeback_single_inode (inode, wbc, + nr_to_write - wbc->nr_to_write);return ret; }
include/trace/events/writeback.h +56 -0
68 lines not on this page · raw message
PATCH 5/6 writeback: trace event writeback_queue_io
In reply to Wu FengguangNote that it adds a little overheads to account the moved/enqueued inodes from b_dirty to b_io. The "moved" accounting may be later used to limit the number of inodes that can be moved in one shot, in order to keep spinlock hold time under control.
--- fs/fs-writeback.c | 16 +++++++++++----- include/trace/events/writeback.h | 23 +++++++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-)
fs/fs-writeback.c +11 -5
@@ -248,15 +248,16 @@ static bool inode_dirtied_after(struct i/* * Move expired dirty inodes from @delaying_queue to @dispatch_queue. */ -static void move_expired_inodes (struct list_head *delaying_queue, -struct list_head *dispatch_queue, -struct writeback_control *wbc) +static int move_expired_inodes (struct list_head *delaying_queue, +struct list_head *dispatch_queue, +struct writeback_control *wbc) {LIST_HEAD (tmp);struct list_head *pos, *node;struct super_block *sb =NULL ;struct inode *inode;int do_sb_sort =0 ; +int moved =0 ;while (!list_empty (delaying_queue)) { inode =wb_inode (delaying_queue->prev);
@@ -267,12 +268,13 @@ static void move_expired_inodes(struct l do_sb_sort =1 ; sb = inode->i_sb;list_move (&inode->i_wb_list, &tmp); + moved++; }/* just one sb in list, splice to dispatch_queue and we're done */ if (!do_sb_sort) {list_splice (&tmp, dispatch_queue); -return ; + goto out; }/* Move inodes from one superblock together */
@@ -284,6 +286,8 @@ static void move_expired_inodes(struct llist_move (&inode->i_wb_list, dispatch_queue); } } +out: +return moved; }/*
@@ -299,9 +303,11 @@ static void move_expired_inodes(struct l*/ static void queue_io (struct bdi_writeback *wb,struct writeback_control *wbc) { +int moved;assert_spin_locked (&wb->list_lock);list_splice_init (&wb->b_more_io, &wb->b_io); -move_expired_inodes (&wb->b_dirty, &wb->b_io, wbc); + moved =move_expired_inodes (&wb->b_dirty, &wb->b_io, wbc); +trace_writeback_queue_io (wb, wbc, moved); }static int write_inode (struct inode *inode,struct writeback_control *wbc)
include/trace/events/writeback.h +23 -0
@@ -165,6 +165,29 @@ DEFINE_WBC_EVENT(wbc_balance_dirty_writtDEFINE_WBC_EVENT (wbc_balance_dirty_wait);DEFINE_WBC_EVENT (wbc_writepage); +TRACE_EVENT (writeback_queue_io, + TP_PROTO(struct bdi_writeback *wb, + struct writeback_control *wbc, + int moved), +TP_ARGS (wb, wbc, moved), +TP_STRUCT__entry ( + __array(char, name, 32) + __field(int, older) +__field (int, moved) + ), +TP_fast_assign ( + strncpy(__entry->name, dev_name(wb->bdi->dev), 32); + __entry->older = wbc->older_than_this ? + (jiffies - *wbc->older_than_this) *1000 /HZ + : -1; + __entry->moved = moved; + ), +TP_printk ("bdi %s: older=%d enqueue=%d" , + __entry->name, + __entry->older, + __entry->moved) +); + DECLARE_EVENT_CLASS(writeback_congest_waited_template, TP_PROTO(unsigned int usec_timeout, unsigned int usec_delayed),
PATCH 5/6 writeback: trace event writeback_queue_io (v2)
[changes from v1: due to concerns about the relative older_than_this, I added the raw older_than_this to the trace output. Hope the added overheads are not a big concern.]
Note that it adds a little overheads to account the moved/enqueued inodes from b_dirty to b_io. The "moved" accounting may be later used to limit the number of inodes that can be moved in one shot, in order to keep spinlock hold time under control.
--- fs/fs-writeback.c | 14 ++++++++++---- include/trace/events/writeback.h | 25 +++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-)
fs/fs-writeback.c +10 -4
@@ -258,15 +258,16 @@ static bool inode_dirtied_after(struct i/* * Move expired dirty inodes from @delaying_queue to @dispatch_queue. */ -static void move_expired_inodes (struct list_head *delaying_queue, +static int move_expired_inodes (struct list_head *delaying_queue,struct list_head *dispatch_queue, -unsigned long *older_than_this) +unsigned long *older_than_this) {LIST_HEAD (tmp);struct list_head *pos, *node;struct super_block *sb =NULL ;struct inode *inode;int do_sb_sort =0 ; +int moved =0 ;while (!list_empty (delaying_queue)) { inode =wb_inode (delaying_queue->prev);
@@ -277,12 +278,13 @@ static void move_expired_inodes(struct l do_sb_sort =1 ; sb = inode->i_sb;list_move (&inode->i_wb_list, &tmp); + moved++; }/* just one sb in list, splice to dispatch_queue and we're done */ if (!do_sb_sort) {list_splice (&tmp, dispatch_queue); -return ; + goto out; }/* Move inodes from one superblock together */
@@ -294,6 +296,8 @@ static void move_expired_inodes(struct llist_move (&inode->i_wb_list, dispatch_queue); } } +out: +return moved; }/*
@@ -309,9 +313,11 @@ static void move_expired_inodes(struct l*/ static void queue_io (struct bdi_writeback *wb,unsigned long *older_than_this) { +int moved;assert_spin_locked (&wb->list_lock);list_splice_init (&wb->b_more_io, &wb->b_io); -move_expired_inodes (&wb->b_dirty, &wb->b_io, older_than_this); + moved =move_expired_inodes (&wb->b_dirty, &wb->b_io, older_than_this); +trace_writeback_queue_io (wb, older_than_this, moved); }static int write_inode (struct inode *inode,struct writeback_control *wbc)
include/trace/events/writeback.h +25 -0
@@ -158,6 +158,31 @@ DEFINE_WBC_EVENT(wbc_balance_dirty_writtDEFINE_WBC_EVENT (wbc_balance_dirty_wait);DEFINE_WBC_EVENT (wbc_writepage); +TRACE_EVENT (writeback_queue_io, + TP_PROTO(struct bdi_writeback *wb, + unsigned long *older_than_this, + int moved), +TP_ARGS (wb, older_than_this, moved), +TP_STRUCT__entry ( + __array(char, name, 32) + __field(unsigned long, older) + __field(long, age) +__field (int, moved) + ), +TP_fast_assign ( + strncpy(__entry->name, dev_name(wb->bdi->dev), 32); + __entry->older = older_than_this ? *older_than_this : 0; + __entry->age = older_than_this ? + (jiffies - *older_than_this) * 1000 / HZ : -1; + __entry->moved = moved; + ), +TP_printk ("bdi %s: older=%lu age=%ld enqueue=%d" , + __entry->name, + __entry->older,/* older_than_this in jiffies */ + __entry->age,/* older_than_this in relative milliseconds */ + __entry->moved) +); + DECLARE_EVENT_CLASS(writeback_congest_waited_template, TP_PROTO(unsigned int usec_timeout, unsigned int usec_delayed),
Jan Kara
maintainerRe: [PATCH 5/6] writeback: trace event writeback_queue_io (v2)
Quoted 8 lines · Wu Fengguang
[changes from v1: due to concerns about the relative
older_than_this, I added the rawolder_than_thisto the trace output. Hope the added overheads are not a big concern.]Note that it adds a little overheads to account the moved/enqueued inodes from
b_dirtytob_io. The "moved" accounting may be later used to limit the number of inodes that can be moved in one shot, in order to keep spinlock hold time under control.
This is fine by me.
Honza
Quoted 97 lines
--- fs/fs-writeback.c | 14 ++++++++++---- include/trace/events/writeback.h | 25 +++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-)fs/fs-writeback.c +10
-4@@ -258,15 +258,16 @@ static bool inode_dirtied_after(struct i/* * Move expired dirty inodes from @delaying_queue to @dispatch_queue. */ -static void move_expired_inodes (struct list_head *delaying_queue, +static int move_expired_inodes (struct list_head *delaying_queue,struct list_head *dispatch_queue, -unsigned long *older_than_this) +unsigned long *older_than_this) {LIST_HEAD (tmp);struct list_head *pos, *node;struct super_block *sb =NULL ;struct inode *inode;int do_sb_sort =0 ; +int moved =0 ;while (!list_empty (delaying_queue)) { inode =wb_inode (delaying_queue->prev);@@ -277,12 +278,13 @@ static void move_expired_inodes(struct l do_sb_sort =1 ; sb = inode->i_sb;list_move (&inode->i_wb_list, &tmp); + moved++; }/* just one sb in list, splice to dispatch_queue and we're done */ if (!do_sb_sort) {list_splice (&tmp, dispatch_queue); -return ; + goto out; }/* Move inodes from one superblock together */ @@ -294,6 +296,8 @@ static void move_expired_inodes(struct llist_move (&inode->i_wb_list, dispatch_queue); } } +out: +return moved; }/* @@ -309,9 +313,11 @@ static void move_expired_inodes(struct l*/ static void queue_io (struct bdi_writeback *wb,unsigned long *older_than_this) { +int moved;assert_spin_locked (&wb->list_lock);list_splice_init (&wb->b_more_io, &wb->b_io); -move_expired_inodes (&wb->b_dirty, &wb->b_io, older_than_this); + moved =move_expired_inodes (&wb->b_dirty, &wb->b_io, older_than_this); +trace_writeback_queue_io (wb, older_than_this, moved); }static int write_inode (struct inode *inode,struct writeback_control *wbc)include/trace/events/writeback.h +25
-0@@ -158,6 +158,31 @@ DEFINE_WBC_EVENT(wbc_balance_dirty_writtDEFINE_WBC_EVENT (wbc_balance_dirty_wait);DEFINE_WBC_EVENT (wbc_writepage); +TRACE_EVENT (writeback_queue_io, + TP_PROTO(struct bdi_writeback *wb, + unsigned long *older_than_this, + int moved), +TP_ARGS (wb, older_than_this, moved), +TP_STRUCT__entry ( + __array(char, name, 32) + __field(unsigned long, older) + __field(long, age) +__field (int, moved) + ), +TP_fast_assign ( + strncpy(__entry->name, dev_name(wb->bdi->dev), 32); + __entry->older = older_than_this ? *older_than_this : 0; + __entry->age = older_than_this ? + (jiffies - *older_than_this) * 1000 / HZ : -1; + __entry->moved = moved; + ), +TP_printk ("bdi %s: older=%lu age=%ld enqueue=%d" , + __entry->name, + __entry->older,/* older_than_this in jiffies */ + __entry->age,/* older_than_this in relative milliseconds */ + __entry->moved) +); + DECLARE_EVENT_CLASS(writeback_congest_waited_template, TP_PROTO(unsigned int usec_timeout, unsigned int usec_delayed),
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
PATCH 6/6 writeback: convert to relative older_than_this in trace events
In reply to Wu FengguangThis is a format change that could break established scripts.
But hopefully it's more human friendly to show the relative values like "30000" (30s old) or "0" (all fresh ones) than long absolute jiffy numbers.
Note that the initial 30000 or 0 may increase over time as they are now relative numbers.
--- include/trace/events/writeback.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
include/trace/events/writeback.h +6 -5
@@ -115,7 +115,7 @@ DECLARE_EVENT_CLASS(wbc_class, __field(int, for_reclaim) __field(int, range_cyclic) __field(int, more_io) - __field(unsigned long, older_than_this) + __field(int, older) __field(long, range_start)__field (long, range_end) ),
@@ -130,14 +130,15 @@ DECLARE_EVENT_CLASS(wbc_class, __entry->for_reclaim = wbc->for_reclaim; __entry->range_cyclic = wbc->range_cyclic; __entry->more_io = wbc->more_io; - __entry->older_than_this = wbc->older_than_this ? - *wbc->older_than_this :0 ; + __entry->older = wbc->older_than_this ? + (jiffies - *wbc->older_than_this) *1000 /HZ + :-1 ; __entry->range_start = (long )wbc->range_start; __entry->range_end = (long )wbc->range_end; ), TP_printk("bdi %s: towrt=%ld skip=%ld mode=%d kupd=%d " - "bgrd=%d reclm=%d cyclic=%d more=%d older=0x%lx " + "bgrd=%d reclm=%d cyclic=%d more=%d older=%d " "start=0x%lx end=0x%lx", __entry->name, __entry->nr_to_write,
@@ -148,7 +149,7 @@ DECLARE_EVENT_CLASS(wbc_class, __entry->for_reclaim, __entry->range_cyclic, __entry->more_io, - __entry->older_than_this, + __entry->older, __entry->range_start, __entry->range_end) )
Jan Kara
maintainerRe: [PATCH 6/6] writeback: convert to relative older_than_this in trace events
Quoted 5 lines · Wu Fengguang
This is a format change that could break established scripts.
But hopefully it's more human friendly to show the relative values like "30000" (30s old) or "0" (all fresh ones) than long absolute jiffy numbers.
It's possibly more human friendly but probably also harder to handle in
scripts. I especially don't like the fact that the number will be changing at different tracepoints - you'll then have to do some comparisons to find out whether just the timestamp has been changed or whether just the time passes by. It's not that hard to write a script doing absolute->relative conversion (since you have time stamps in the traces) for those cases where relative time is really what interests you. So do you have any particular usecase in mind for the relative value?
Honza
Quoted 52 lines
Note that the initial 30000 or 0 may increase over time as they are now relative numbers.
CC Jan Kara <[email protected]>CC Dave Chinner <[email protected]>Signed-off-by Wu Fengguang <[email protected]>--- include/trace/events/writeback.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)include/trace/events/writeback.h +6
-5@@ -115,7 +115,7 @@ DECLARE_EVENT_CLASS(wbc_class, __field(int, for_reclaim) __field(int, range_cyclic) __field(int, more_io) - __field(unsigned long, older_than_this) + __field(int, older) __field(long, range_start)__field (long, range_end) ),@@ -130,14 +130,15 @@ DECLARE_EVENT_CLASS(wbc_class, __entry->for_reclaim = wbc->for_reclaim; __entry->range_cyclic = wbc->range_cyclic; __entry->more_io = wbc->more_io; - __entry->older_than_this = wbc->older_than_this ? - *wbc->older_than_this :0 ; + __entry->older = wbc->older_than_this ? + (jiffies - *wbc->older_than_this) *1000 /HZ + :-1 ; __entry->range_start = (long )wbc->range_start; __entry->range_end = (long )wbc->range_end; ), TP_printk("bdi %s: towrt=%ld skip=%ld mode=%d kupd=%d " - "bgrd=%d reclm=%d cyclic=%d more=%d older=0x%lx " + "bgrd=%d reclm=%d cyclic=%d more=%d older=%d " "start=0x%lx end=0x%lx", __entry->name, __entry->nr_to_write,@@ -148,7 +149,7 @@ DECLARE_EVENT_CLASS(wbc_class, __entry->for_reclaim, __entry->range_cyclic, __entry->more_io, - __entry->older_than_this, + __entry->older, __entry->range_start, __entry->range_end) )
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Re: [PATCH 6/6] writeback: convert to relative older_than_this in trace events
Quoted 6 lines · Jan Kara
Quoted 5 lines · Wu Fengguang
This is a format change that could break established scripts.
But hopefully it's more human friendly to show the relative values like "30000" (30s old) or "0" (all fresh ones) than long absolute jiffy numbers.
Quoted 7 lines
It's possibly more human friendly but probably also harder to handle in
scripts. I especially don't like the fact that the number will be changing at different tracepoints - you'll then have to do some comparisons to find out whether just the timestamp has been changed or whether just the time passes by. It's not that hard to write a script doingabsolute->relativeconversion (since you have time stamps in the traces) for those cases where relative time is really what interests you.
OK.
Quoted 2 lines
So do you have any particular usecase in mind for the relative value?
I normally just look at the raw traces in which case the raw jiffies mean nothing to me. Or sometimes plot the relative dirty ages with scripts.
Thanks,
Fengguang
Quoted 54 lines
Quoted 51 lines
Note that the initial 30000 or 0 may increase over time as they are now relative numbers.
CC Jan Kara <[email protected]>CC Dave Chinner <[email protected]>Signed-off-by Wu Fengguang <[email protected]>--- include/trace/events/writeback.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)include/trace/events/writeback.h +6
-5@@ -115,7 +115,7 @@ DECLARE_EVENT_CLASS(wbc_class, __field(int, for_reclaim) __field(int, range_cyclic) __field(int, more_io) - __field(unsigned long, older_than_this) + __field(int, older) __field(long, range_start)__field (long, range_end) ),@@ -130,14 +130,15 @@ DECLARE_EVENT_CLASS(wbc_class, __entry->for_reclaim = wbc->for_reclaim; __entry->range_cyclic = wbc->range_cyclic; __entry->more_io = wbc->more_io; - __entry->older_than_this = wbc->older_than_this ? - *wbc->older_than_this :0 ; + __entry->older = wbc->older_than_this ? + (jiffies - *wbc->older_than_this) *1000 /HZ + :-1 ; __entry->range_start = (long )wbc->range_start; __entry->range_end = (long )wbc->range_end; ), TP_printk("bdi %s: towrt=%ld skip=%ld mode=%d kupd=%d " - "bgrd=%d reclm=%d cyclic=%d more=%d older=0x%lx " + "bgrd=%d reclm=%d cyclic=%d more=%d older=%d " "start=0x%lx end=0x%lx", __entry->name, __entry->nr_to_write,@@ -148,7 +149,7 @@ DECLARE_EVENT_CLASS(wbc_class, __entry->for_reclaim, __entry->range_cyclic, __entry->more_io, - __entry->older_than_this, + __entry->older, __entry->range_start, __entry->range_end) )Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 3 lines · Wu Fengguang
Andrew,
Here are one more collection of simple writeback patches.
How does this relate to the other floating writeback patches? We really need a proper writeback tree to make any sense of the patch queues..
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · Wu Fengguang
Andrew,
Here are one more collection of simple writeback patches.
How does this relate to the other floating writeback patches? We really need a proper writeback tree to make any sense of the patch queues..
These patches are based on the writeback patches already in -mm. Sorry I should have mentioned it.
Thanks,
Fengguang
Quoted 4 lines · Wu Fengguang
Quoted 3 lines · Christoph Hellwig
Quoted 2 lines · Wu Fengguang
Andrew,
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Here are one more collection of simple writeback patches.
How does this relate to the other floating writeback patches? We really need a proper writeback tree to make any sense of the patch queues..
These patches are based on the writeback patches already in -mm. Sorry I should have mentioned it.
Does anyone actaully testing filesystems use the -mm tree? I'm pretty sure that no-one in the XFS world does, and I don't think that any ext4 or btrfs folk do, either....
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 6 lines · Dave Chinner
Quoted 2 lines
These patches are based on the writeback patches already in -mm. Sorry I should have mentioned it.
Does anyone actaully testing filesystems use the -mm tree? I'm pretty sure that no-one in the XFS world does, and I don't think that any ext4 or btrfs folk do, either....
I doubt it since -mm still hasn't made it into linux-next.
Quoted 3 lines · Dave Chinner
Does anyone actaully testing filesystems use the -mm tree? I'm pretty sure that no-one in the XFS world does, and I don't think that any ext4 or btrfs folk do, either....
I don't. I agree that it really would be great if there was a separate git tree for the writeback changes, since that way it becomes possible to test just the writeback changes, and not worry about other potential stability problems introduced by changes in the -mm tree....
-- Ted
Jan Kara
maintainerQuoted 10 lines · Ted Tso
Quoted 3 lines · Dave Chinner
Does anyone actaully testing filesystems use the -mm tree? I'm pretty sure that no-one in the XFS world does, and I don't think that any ext4 or btrfs folk do, either....
I don't. I agree that it really would be great if there was a separate
git tree for the writeback changes, since that way it becomes possible to test just the writeback changes, and not worry about other potential stability problems introduced by changes in the -mm tree....
OK. We'd still push changes to Linus via Andrew but have them also
accumulated in that tree for testing. So we'd have a branch with changes sitting in -mm (likely to go to Linus in near future) and then possibly other branches with things brewing for people to try out. Does that make sense? I can setup that tree and maintain it. Or Fengguang, do you want to do it?
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR