lkml.org 
[lkml]   [2008]   [Aug]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] improve ext3 fsync batching
On Tue, 19 Aug 2008 16:55:47 -0400
Ric Wheeler <rwheeler@redhat.com> wrote:

> >
> >>> Perhaps a better scheme would be to tune it based on how many other
> >>> processes are joining that transaction. If it's "zero" then decrease
> >>> the timeout. But one would need to work out how to increase it, which
> >>> perhaps could be done by detecting the case where process A runs an
> >>> fsync when a commit is currently in progress, and that commit was
> >>> caused by process B's fsync.
> >>>
> >>>
> >> This is really, really a property of the device's latency at any given
> >> point in time. If there are no other processes running, we could do an
> >> optimization and not wait.
> >>
> >
> > well yes. This represents yet another attempt to predict future
> > application behaviour. The way in which we _usually_ do that is by
> > monitoring past application behaviour.
> >
> This whole area is very similar to the IO elevator area where some of
> the same device characteristics are measured.
>
>
> > Only this patch didn't do that (directly) and I'm wondering why not.
> >
>
> The average transaction commit time is a direct measurement of the past
> behaviour, right?

Of commit time, yes. Somewhat - it has obvious failures during
transients.

But there's an assumption here that commit time is usefully correlated
with "mean time between fsync()s", which might introduce further
inaccuracies.

> >
> >>> But before doing all that I would recommend/ask that the following be
> >>> investigated:
> >>>
> >>> - How effective is the present code?
> >>>
> >>>
> >> It causes the most expensive storage (arrays) to run 3-4 times slower
> >> than they should on a synchronous write workload (NFS server, mail
> >> server?) with more than 1 thread. For example, against a small EMC
> >> array, I saw single threaded write rates of 720 files/sec against ext3
> >> with 1 thread, 225 (if I remember correctly) with 2 ;-)
> >>
> >
> > Current code has:
> >
> > /*
> > * Implement synchronous transaction batching. If the handle
> > * was synchronous, don't force a commit immediately. Let's
> > * yield and let another thread piggyback onto this transaction.
> > * Keep doing that while new threads continue to arrive.
> > * It doesn't cost much - we're about to run a commit and sleep
> > * on IO anyway. Speeds up many-threaded, many-dir operations
> > * by 30x or more...
> > *
> > * But don't do this if this process was the most recent one to
> > * perform a synchronous write. We do this to detect the case where a
> > * single process is doing a stream of sync writes. No point in waiting
> > * for joiners in that case.
> > */
> >
> > has the 30x been reproduced? If not, what broke? If so, what effect
> > did the proposed change have upon it?
> >
>
> The huge gain was only in the case of a RAM disk test which I assume was
> not tested against the original patch early on. Against an array (with a
> 250HZ kernel), we saw a 2.5x speedup with the new code.

You only answered question 3/3 ;)

I am concerned that the current code is no longer working correctly.
If so then apart from the obvious we-should-fix-that issue, there's
also the possibility that we're adding more stuff on top of the broken
stuff rather than fixing the broken stuff.

> >
> >>> - What happens when it is simply removed?
> >>>
> >>>
> >> If you remove the code, you will not see the throughput rise when you go
> >> multithreaded on existing slow devices (S-ATA/ATA for example). Faster
> >> devices will not see that 2 threaded drop.
> >>
> >
> > See above - has this been tested and confirmed?
> >
>
> Yes - we (back at EMC) did remove the logic and the fast devices will
> write at least at their starting rate (700+ files/sec).

Did you observe the effect upon slower devices?

> >
> >>> - Add instrumentation (a counter and a printk) to work out how
> >>> many other tasks are joining this task's transaction.
> >>>
> >>> - If the answer is "zero" or "small", work out why.
> >>>
> >>> - See if we can increase its effectiveness.
> >>>
> >>> Because it could be that the code broke. There might be issues with
> >>> higher-level locks which are preventing the batching. For example, if
> >>> all the files which the test app is syncing are in the same directory,
> >>> perhaps all the tasks are piling up on that directory's i_mutex?
> >>>
> >>>
> >> I have to admit that I don't see the down side here - we have shown a
> >> huge increase for arrays (embarrassingly huge increase for RAM disks)
> >> and see no degradation for the S-ATA/ATA case.
> >>
> >> The code is not broken (having been there and done the performance
> >> tuning on the original code), it just did not account for the widely
> >> varying average response times for different classes of storage ;-)
> >>
> >
> > Well, as I said - last time I checked, it did seem to be broken. By
> > what means did you confirm that it is still effective, and what were
> > the results?
> >
> >
>
> I think Josef posted those results for S-ATA earlier in the thread and
> they were still working. We can repost/rerun to give more detail...

Yes please.


\
 
 \ /
  Last update: 2008-08-19 23:21    [W:0.048 / U:20.956 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site