From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shaohua Li Subject: Re: [PATCH 2/3] md/raid5-cache: bump flush stripe batch size Date: Tue, 7 Mar 2017 12:50:13 -0800 Message-ID: <20170307205013.lcy5lqk5ul4lmp27@kernel.org> References: <52a5e6f8924b9a19d4ec4ad73f6e7087f36f139d.1487373517.git.shli@fb.com> <87mvd3m58c.fsf@notabene.neil.brown.name> <20170303174138.iecwisogdkkdh6iy@kernel.org> <87efyakjp7.fsf@notabene.neil.brown.name> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <87efyakjp7.fsf@notabene.neil.brown.name> Sender: linux-raid-owner@vger.kernel.org To: NeilBrown Cc: linux-raid@vger.kernel.org, songliubraving@fb.com List-Id: linux-raid.ids On Mon, Mar 06, 2017 at 05:23:00PM +1100, Neil Brown wrote: > On Fri, Mar 03 2017, Shaohua Li wrote: > > > On Fri, Mar 03, 2017 at 02:03:31PM +1100, Neil Brown wrote: > >> On Fri, Feb 17 2017, Shaohua Li wrote: > >> > >> > Bump the flush stripe batch size to 2048. For my 12 disks raid > >> > array, the stripes takes: > >> > 12 * 4k * 2048 = 96MB > >> > > >> > This is still quite small. A hardware raid card generally has 1GB size, > >> > which we suggest the raid5-cache has similar cache size. > >> > > >> > The advantage of a big batch size is we can dispatch a lot of IO in the > >> > same time, then we can do some scheduling to make better IO pattern. > >> > > >> > Last patch prioritizes stripes, so we don't worry about a big flush > >> > stripe batch will starve normal stripes. > >> > > >> > Signed-off-by: Shaohua Li > >> > --- > >> > drivers/md/raid5-cache.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c > >> > index 3f307be..b25512c 100644 > >> > --- a/drivers/md/raid5-cache.c > >> > +++ b/drivers/md/raid5-cache.c > >> > @@ -43,7 +43,7 @@ > >> > /* wake up reclaim thread periodically */ > >> > #define R5C_RECLAIM_WAKEUP_INTERVAL (30 * HZ) > >> > /* start flush with these full stripes */ > >> > -#define R5C_FULL_STRIPE_FLUSH_BATCH 256 > >> > +#define R5C_FULL_STRIPE_FLUSH_BATCH 2048 > >> > >> Fixed numbers are warning signs... I wonder if there is something better > >> we could do? "conf->max_nr_stripes / 4" maybe? We use that sort of > >> number elsewhere. > >> Would that make sense? > > > > The code where we check the batch size (in r5c_do_reclaim) already a check: > > total_cached > conf->min_nr_stripes * 1 / 2 > > so I think that's ok, no? > > I'm not sure what you are saying. > > I'm suggesting that we get rid of R5C_FULL_STRIPE_FLUSH_BATCH and use a > number like "conf->max_nr_stripes / 4" > Are you agreeing, or are you saying that you don't think we need to get > rid of R5C_FULL_STRIPE_FLUSH_BATCH?? What I mean is we already check the min_nr_stripes which is related to max_nr_stripes, so we don't need check max_nr_stripes again. Thinking this more, max_nr_stripes / 4 does make more sense if the cache is very big. I'll change R5C_FULL_STRIPE_FLUSH_BATCH to 'conf->max_nr_stripes / 4'. Thanks, Shaohua