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
Andrew Morton wrote:
> Could I just point out that this is a very very painful way of writing
> a changelog? All these new revelations are important and relevant and
> should have been there!
>

Agreed, some of this was bounced around only in the email thread.
> On Tue, 19 Aug 2008 14:08:31 -0400
> Ric Wheeler <rwheeler@redhat.com> wrote:
>
>
>> Andrew Morton wrote:
>>
>>> On Tue, 19 Aug 2008 07:01:11 -0400 Ric Wheeler <rwheeler@redhat.com> wrote:
>>>
>>>
>>>
>>>> It would be great to be able to use this batching technique for faster
>>>> devices, but we currently sleep 3-4 times longer waiting to batch for an
>>>> array than it takes to complete the transaction.
>>>>
>>>>
>>> Obviously, tuning that delay down to the minimum necessary is a good
>>> thing. But doing it based on commit-time seems indirect at best. What
>>> happens on a slower disk when commit times are in the tens of
>>> milliseconds? When someone runs a concurrent `dd if=/dev/zero of=foo'
>>> when commit times go up to seconds?
>>>
>>>
>> Transactions on that busier drive would take longer, we would sleep
>> longer which would allow us to batch up more into one transaction. That
>> should be a good result and it should reset when the drive gets less
>> busy (and transactions shorter) to a shorter sleep time.
>>
>
> Has this been empirically confirmed?
>

No, this was just me thinking about how a shared disk can become
sluggish. It is a good experiment to try (run the test on one host,
inject a heavy load from a second, watch the behaviour).

> Commits can takes tens of seconds and causing an fsync() to block
> itself for such periods could have quite bad effects. How do we (ie:
> I) know that there are no such scenarios with this change?
>

A very good point - I like your suggestion to do the minimum of avg
sleep time vs 1 jiffie in the follow up message.

>
>>> 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?

>
>>> 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.

>
>>> - 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).

>
>>> - 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...

ric





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