From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:52832 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756034AbdGLBpp (ORCPT ); Tue, 11 Jul 2017 21:45:45 -0400 Subject: Re: [PATCH] bcache: only recovery I/O error for writethrough mode To: tang.junhui@zte.com.cn Cc: linux-bcache@vger.kernel.org, linux-bcache-owner@vger.kernel.org, linux-block@vger.kernel.org References: <20170710111828.97918-1-colyli@suse.de> From: Coly Li Message-ID: <45926957-5990-6520-7bd7-6de8c2bae7b8@suse.de> Date: Wed, 12 Jul 2017 09:45:34 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Sender: linux-block-owner@vger.kernel.org List-Id: linux-block@vger.kernel.org On 2017/7/12 上午9:43, tang.junhui@zte.com.cn wrote: >>>> If a read bio to cache device gets failed, bcache will try to > recovery it >>>> by forward the read bio to backing device. If backing device responses >>>> read request successfully then the bio contains data from backing > device >>>> will be returned to uppper layer. >>>> >>>> The recovery effort in cached_dev_read_error() is not correct, and > there >>>> is report that corrupted data may returned when a dirty cache device > goes >>>> offline during reading I/O. >>>> >>>> For writeback cache mode, before dirty data are wrote back to backing >>>> device, data blocks on backing device are not updated and > consistent. If >>>> a dirty cache device dropped and a read bio gets failed, bcache will >>>> return its stale version from backing device. This is mistaken behavior >>>> that applications don't expected, especially for data base workload. >>>> >>>> This patch fixes the issue by only permit recoverable I/O when cached >>>> device is in writethough mode, and s->recoverable is set. For other > cache >>>> mode, recovery I/O failure by reading backing device does not make > sense, >>>> bache just simply returns -EIO immediately. >>>> >>>> Reported-by: Arne Wolf >>>> Signed-off-by: Coly Li >>>> --- >>>> drivers/md/bcache/request.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c >>>> index 019b3df9f1c6..6edacac9b00d 100644 >>>> --- a/drivers/md/bcache/request.c >>>> +++ b/drivers/md/bcache/request.c >>>> @@ -702,8 +702,11 @@ static void cached_dev_read_error(struct closure >>> *cl) >>>> { >>>> struct search *s = container_of(cl, struct search, > cl); >>>> struct bio *bio = &s->bio.bio; >>>> + struct cached_dev *dc = container_of(s->d, struct >>> cached_dev, disk); >>>> + unsigned mode = cache_mode(dc, NULL); >>>> >>>> - if (s->recoverable) { >>>> + if (s->recoverable && >>>> + (mode == CACHE_MODE_WRITETHROUGH)) { >>>> /* Retry from the backing device: */ >>>> > trace_bcache_read_retry(s->orig_bio); >>>> >>>> -- >>> >>> No, Poly, >>> Cache mode can change dynamically, >>> So maybe the bcache device is still dirty >>> when the cache mode is changed to CACHE_MODE_WRITETHROUGH, >>> so I think this patch can not solve this question fundamentally. >> >>Hi Junnhui, >> >>Yes I agree with you, if cache mode is switched from writeback to >>writethrough or writearound mode, and then the cache device is >>disconnected, this patch does not help us. >> >>When I replied question from Kai Krakow, I expressed similar opinion as >>yours. This patch is a best-effort fix, it is very necessary for data >>base applications which always use writeback mode and not switch to >>other mode during all their online time. > > Since this issue is important, we should find out a solution > to solve it completely, not just do best-effort. > I meant "it is very necessary for data base applications which always use *writeback* mode and not switch to other mode during all their online time." ^_^ Thanks. Coly