Re: [PATCH v2] writeback: Do not sync data dirtied after sync start

[Jan Kara <jack@suse.cz>]

On Fri 27-09-13 10:55:53, Dave Chinner wrote:
> On Thu, Sep 26, 2013 at 09:23:58PM +0200, Jan Kara wrote:
> > When there are processes heavily creating small files while sync(2) is
> > running, it can easily happen that quite some new files are created
> > between WB_SYNC_NONE and WB_SYNC_ALL pass of sync(2). That can happen
> > especially if there are several busy filesystems (remember that sync
> > traverses filesystems sequentially and waits in WB_SYNC_ALL phase on one
> > fs before starting it on another fs). Because WB_SYNC_ALL pass is slow
> > (e.g. causes a transaction commit and cache flush for each inode in
> > ext3), resulting sync(2) times are rather large.
> 
> Yup, that can be a problem.
> 
> Build warning form the patch:
> 
> In file included from include/trace/ftrace.h:575:0,
>                  from include/trace/define_trace.h:90,
>                  from include/trace/events/writeback.h:603,
>                  from fs/fs-writeback.c:89:
> include/trace/events/writeback.h: In function ¿ftrace_raw_event_writeback_queue_io¿:
> include/trace/events/writeback.h:277:1: warning: initialization makes pointer from integer without a cast [enabled by default]
> In file included from include/trace/ftrace.h:711:0,
>                  from include/trace/define_trace.h:90,
>                  from include/trace/events/writeback.h:603,
>                  from fs/fs-writeback.c:89:
> include/trace/events/writeback.h: In function ¿perf_trace_writeback_queue_io¿:
> include/trace/events/writeback.h:277:1: warning: initialization makes pointer from integer without a cast [enabled by default]
  Thanks for catching this. I'll send v3 in a minute.

> > The following script reproduces the problem:
> > 
> > function run_writers
> > {
> >   for (( i = 0; i < 10; i++ )); do
> >     mkdir $1/dir$i
> >     for (( j = 0; j < 40000; j++ )); do
> >       dd if=/dev/zero of=$1/dir$i/$j bs=4k count=4 &>/dev/null
> >     done &
> >   done
> > }
> > 
> > for dir in "$@"; do
> >   run_writers $dir
> > done
> > 
> > sleep 40
> > time sync
> > ======
> > 
> > Fix the problem by disregarding inodes dirtied after sync(2) was called
> > in the WB_SYNC_ALL pass. To allow for this, sync_inodes_sb() now takes a
> > time stamp when sync has started which is used for setting up work for
> > flusher threads.
> > 
> > To give some numbers, when above script is run on two ext4 filesystems on
> > simple SATA drive, the average sync time from 10 runs is 267.549 seconds
> > with standard deviation 104.799426. With the patched kernel, the average
> > sync time from 10 runs is 2.995 seconds with standard deviation 0.096.
> 
> Hmmmm. 2.8 seconds on my XFS perf VM without the patch. Ok, try a
> smaller VM backed by single spindle of spinning rust rather than
> SSDs. Over 10 runs I see:
> 
> kernel		min	max	av
> vanilla		0.18s	4.46s	1.63s
> patched		0.14s	0.45s	0.28s
> 
> Definitely an improvement, but nowhere near the numbers you are
> seeing for ext4 - maybe XFS isn't as susceptible to this problem
> as ext4. Nope, ext4 on an unpatched kernel gives 1.66/6.81/3.12s,
> (which is less than your patched kernel results :) but means
> so it must be something else configuration/hardware related.
  Have you really used *two* (or more) busy filesystems? That makes the
problem an order of magnitude worse for me. The numbers I've posted are for
such situation...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html