On Tue, 29 Apr 2014 19:13:58 +0800 Shaohua Li <shli@xxxxxxxxxx> wrote: > On Tue, Apr 29, 2014 at 05:07:25PM +1000, NeilBrown wrote: > > On Tue, 29 Apr 2014 10:01:24 +0800 Shaohua Li <shli@xxxxxxxxxx> wrote: > > > > > On Mon, Apr 28, 2014 at 08:44:07PM +1000, NeilBrown wrote: > > > > On Mon, 28 Apr 2014 03:17:48 -0700 Christoph Hellwig <hch@xxxxxxxxxxxxx> > > > > wrote: > > > > > > > > > On Mon, Apr 28, 2014 at 02:58:41PM +0800, Shaohua Li wrote: > > > > > > > > > > > > The stripe cache has two goals: > > > > > > 1. cache data, so next time if data can be found in stripe cache, disk access > > > > > > can be avoided. > > > > > > > > > > I think this is mostly a side effect. We have a much larger and better > > > > > tuned page cache to take care of this. > > > > > > > > > > > 2. stable data. data is copied from bio to stripe cache and calculated parity. > > > > > > data written to disk is from stripe cache, so if upper layer changes bio data, > > > > > > data written to disk isn't impacted. > > > > > > > > > > > > In my environment, I can guarantee 2 will not happen. > > > > > > > > > > Why just in your environment? Now that we got stable pages in the page > > > > > cache this should always be the case. > > > > > > > > Hmm... I hadn't realised that we were guaranteed stabled pages always (if > > > > requested). It seems that we are. > > > > > > > > > > > > > > > Of course, this shouldn't be enabled by default, so I added an option to > > > > > > control it. > > > > > > > > > > Unless careful benchmarking in various scenarious shows adverse effects > > > > > this should be the default. And if we can find adverse effects we need > > > > > to look into them. > > > > > > > > Certainly some benchmarking is needed. > > > > > > > > We should set > > > > > > > > mddev->queue->backing_dev_info.capabilities |= BDI_CAP_STABLE_WRITES > > > > > > > > if and only iff 'skip_copy' is set. Then test various cases just to confirm > > > > that it is generally an improvement. > > > > > > IIRC, we switched from 'force wait page writeback' to 'wait page writeback if > > > required' because of performance issues reported, so we shoudn't always enable > > > BDI_CAP_STABLE_WRITES. Is it safe to set/clear BDI_CAP_STABLE_WRITES at > > > runtime, if we use 'skip_copy' to control it? Ofcourse, we don't need runtime > > > changing the setting, but we need a mechanism to setup it before array runs. > > > > So for md/RAID5 the trade off is: > > - If we set BDI_CAP_STABLE_WRITES then processes might sometimes have to wait > > for the writeout to complete where otherwise they would not > > - If we don't then RAID5 *always* has to copy the page into the stripe cache. > > > > It isn't at all clear to me which is best. It is very possible that copying > > costs a lot. But then waiting for read-modify-write cycles can be a real > > cost too.... > > > > I think it is perfectly safe to change BDI_CAP_STABLE_WRITES while the array > > is suspended. So > > mddev_suspend(mddev); > > change BDI_CAP_STABLE_WRITES > > mddev_resume(mddev); > > > > should be safe. > > sounds good. > > > > > > > As of performance, the 'skip_copy' is very helpful (> 30% boost) for my raid5 > > > array (with 6 fast PCIe SSD) for 1M request size workload. Nothing changed for > > > 4k randwrite workload. > > > > It would be really good to see comparison for sequential and random loads on > > various filesytems with both rotating and SSD devices, in RAID5 and RAID6, > > with various numbers of devices. > > :-) > > > > If you'd like to update your patch to adjust BDI_CAP_STABLE_WRITES when > > skip_copy is changed, I'll apply it so that people can test it. > > Here it is. I've removed this patch for now. It causes a nasty crash when running the 07changelevels test in the mdadm test suite. First we get kernel: [ 8282.822194] WARNING: CPU: 0 PID: 16377 at /home/git/md/drivers/md/raid5.c:1404 raid_run which is + BUG_ON(sh->dev[i].page != sh->dev[i].orig_page); which I changed to WARN_ON, then kernel: [ 8284.116364] BUG: sleeping function called from invalid context at /home/git/md/kernel/locking/rwsem.c:20 kernel: [ 8284.116369] in_atomic(): 1, irqs_disabled(): 0, pid: 16377, name: md0_raid5 kernel: [ 8284.116372] INFO: lockdep is turned off. kernel: [ 8284.116379] Preemption disabled at:[<ffffffff81a63de4>] handle_stripe_expansion+0x134/0x1e0 kernel: [ 8284.116380] kernel: [ 8284.116385] CPU: 0 PID: 16377 Comm: md0_raid5 Tainted: G D W 3.15.0-rc5+ #855 kernel: [ 8284.116388] Hardware name: HP ProLiant ML310 G3, BIOS W02 04/17/2006 kernel: [ 8284.116400] ffff8800d25641d0 ffff8800b7403888 ffffffff81c62893 0000000000000000 kernel: [ 8284.116409] ffff8800b74038b0 ffffffff810c4dea ffff88014091d410 ffff88014091d470 kernel: [ 8284.116415] ffff8800d25641d0 ffff8800b74038d8 ffffffff81c716e5 ffff8800d25641d0 kernel: [ 8284.116416] Call Trace: kernel: [ 8284.116422] [<ffffffff81c62893>] dump_stack+0x4e/0x7a kernel: [ 8284.116429] [<ffffffff810c4dea>] __might_sleep+0x15a/0x250 kernel: [ 8284.116436] [<ffffffff81c716e5>] down_read+0x25/0xa0 kernel: [ 8284.116445] [<ffffffff810a6dcf>] exit_signals+0x1f/0x120 kernel: [ 8284.116453] [<ffffffff81093d35>] do_exit+0xb5/0xc70 kernel: [ 8284.116462] [<ffffffff810f7bcd>] ? kmsg_dump+0x1ad/0x220 kernel: [ 8284.116465] [<ffffffff810f7a40>] ? kmsg_dump+0x20/0x220 kernel: [ 8284.116473] [<ffffffff81055515>] oops_end+0x85/0xc0 kernel: [ 8284.116480] [<ffffffff81055686>] die+0x46/0x70 kernel: [ 8284.116487] [<ffffffff8105250a>] do_general_protection+0xca/0x150 kernel: [ 8284.116494] [<ffffffff81c739d2>] general_protection+0x22/0x30 kernel: [ 8284.116501] [<ffffffff8166a0a9>] ? memcpy+0x29/0x110 kernel: [ 8284.116508] [<ffffffff81638275>] ? async_memcpy+0xc5/0x160 kernel: [ 8284.116516] [<ffffffff81a63de4>] handle_stripe_expansion+0x134/0x1e0 kernel: [ 8284.116522] [<ffffffff81a6496e>] handle_stripe+0xade/0x23e0 I've haven't looked at why this might be. And re the other patch you send, I meant to also say to please use __test_and_clear_bit(). This version is sufficient when the variable is only used by one thread, and it is slightly more efficient. NeilBrown > > > Subject: raid5: add an option to avoid copy data from bio to stripe cache > > The stripe cache has two goals: > 1. cache data, so next time if data can be found in stripe cache, disk access > can be avoided. > 2. stable data. data is copied from bio to stripe cache and calculated parity. > data written to disk is from stripe cache, so if upper layer changes bio data, > data written to disk isn't impacted. > > In my environment, I can guarantee 2 will not happen. And BDI_CAP_STABLE_WRITES > can guarantee 2 too. For 1, it's not common too. block plug mechanism will > dispatch a bunch of sequentail small requests together. And since I'm using > SSD, I'm using small chunk size. It's rare case stripe cache is really useful. > > So I'd like to avoid the copy from bio to stripe cache and it's very helpful > for performance. In my 1M randwrite tests, avoid the copy can increase the > performance more than 30%. > > Of course, this shouldn't be enabled by default. It's reported enabling > BDI_CAP_STABLE_WRITES can harm some workloads before, so I added an option to > control it. > > Signed-off-by: Shaohua Li <shli@xxxxxxxxxxxx> > --- > drivers/md/raid5.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++------- > drivers/md/raid5.h | 4 +- > 2 files changed, 94 insertions(+), 14 deletions(-) >
Attachment:
signature.asc
Description: PGP signature
