From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Chinner Subject: Re: [PATCH v2] writeback: Do not sync data dirtied after sync start Date: Fri, 27 Sep 2013 10:55:53 +1000 Message-ID: <20130927005553.GV26872@dastard> References: <1380223438-26381-1-git-send-email-jack@suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Al Viro , Wu Fengguang , linux-fsdevel@vger.kernel.org To: Jan Kara Return-path: Received: from ipmail07.adl2.internode.on.net ([150.101.137.131]:52626 "EHLO ipmail07.adl2.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750972Ab3I0A4C (ORCPT ); Thu, 26 Sep 2013 20:56:02 -0400 Content-Disposition: inline In-Reply-To: <1380223438-26381-1-git-send-email-jack@suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Sep 26, 2013 at 09:23:58PM +0200, Jan Kara wrote: > When there are processes heavily creating small files while sync(2) i= s > 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 slo= w > (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 =BFftrace_raw_event_write= back_queue_io=BF: include/trace/events/writeback.h:277:1: warning: initialization makes p= ointer 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 =BFperf_trace_writeback_q= ueue_io=BF: include/trace/events/writeback.h:277:1: warning: initialization makes p= ointer from integer without a cast [enabled by default] > The following script reproduces the problem: >=20 > function run_writers > { > for (( i =3D 0; i < 10; i++ )); do > mkdir $1/dir$i > for (( j =3D 0; j < 40000; j++ )); do > dd if=3D/dev/zero of=3D$1/dir$i/$j bs=3D4k count=3D4 &>/dev/nul= l > done & > done > } >=20 > for dir in "$@"; do > run_writers $dir > done >=20 > sleep 40 > time sync > =3D=3D=3D=3D=3D=3D >=20 > Fix the problem by disregarding inodes dirtied after sync(2) was call= ed > in the WB_SYNC_ALL pass. To allow for this, sync_inodes_sb() now take= s a > time stamp when sync has started which is used for setting up work fo= r > flusher threads. >=20 > To give some numbers, when above script is run on two ext4 filesystem= s on > simple SATA drive, the average sync time from 10 runs is 267.549 seco= nds > with standard deviation 104.799426. With the patched kernel, the aver= age > sync time from 10 runs is 2.995 seconds with standard deviation 0.096= =2E 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. Anyway, the change looks good, it just needs the above warning fixed... Cheers, Dave. --=20 Dave Chinner david@fromorbit.com -- 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