Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] md: use BLK_STS_OK instead of hardcode
To:     Xianting Tian <xianting_tian@126.com>
Cc:     linux-bcache@vger.kernel.org, agk@redhat.com, snitzer@redhat.com,
        kent.overstreet@gmail.com, linux-kernel@vger.kernel.org,
        linux-raid@vger.kernel.org,
        Xianting Tian <xianting.tian@linux.alibaba.com>,
        song@kernel.org, dm-devel@redhat.com
References: <1624377241-8642-1-git-send-email-xianting_tian@126.com>
From:   Coly Li <colyli@suse.de>
Message-ID: <930e7a23-a22e-409f-e058-0b1576c5d9d0@suse.de>
Date:   Wed, 23 Jun 2021 00:10:23 +0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <1624377241-8642-1-git-send-email-xianting_tian@126.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

On 6/22/21 11:54 PM, Xianting Tian wrote:
> When setting io status, sometimes it uses BLK_STS_*, sometimes,
> it uses hardcode 0.
> Use the macro to replace hardcode in multiple places.
>
> Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
> ---
>  drivers/md/bcache/request.c  | 2 +-
>  drivers/md/dm-clone-target.c | 2 +-
>  drivers/md/dm-integrity.c    | 2 +-
>  drivers/md/dm-mpath.c        | 2 +-
>  drivers/md/dm-raid1.c        | 2 +-
>  drivers/md/dm.c              | 2 +-
>  drivers/md/raid1.c           | 4 ++--
>  drivers/md/raid10.c          | 2 +-
>  8 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index 6d1de88..73ba5a6 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -790,7 +790,7 @@ static void cached_dev_read_error(struct closure *cl)
>  		/* Retry from the backing device: */
>  		trace_bcache_read_retry(s->orig_bio);
>  
> -		s->iop.status = 0;
> +		s->iop.status = BLK_STS_OK;
>  		do_bio_hook(s, s->orig_bio, backing_request_endio);
>  
>  		/* XXX: invalidate cache */

Hi Xianting,

NACK for bcache part.

The change is incomplete, if you want to replace 0 by BLK_STS_OK, you
should check all locations
where s->iop.status is checked and replace with BLK_STS_OK when necessary.

One but not the only one example is,
871         if (s->iop.status)
872                 continue_at_nobarrier(cl, cached_dev_read_error,
bcache_wq);
Maybe you should change to
        if (s->iop.status != BLK_STS_OK)
                    continue_at_nobarrier(cl, cached_dev_read_error,
bcache_wq);


Just FYI.

Coly Li