Re: [RFC]raid5: add an option to avoid copy data from bio to stripe cache

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3334 bytes --]

On Tue, 29 Apr 2014 10:01:24 +0800 Shaohua Li <shli@kernel.org> 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@infradead.org>
> > 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.

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

Thanks,
NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]