linux-kernel - Re: [PATCH] md: use BLK_STS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <930e7a23-a22e-409f-e058-0b1576c5d9d0@suse.de>
Date:   Wed, 23 Jun 2021 00:10:23 +0800
From:   Coly Li <colyli@...e.de>
To:     Xianting Tian <xianting_tian@....com>
Cc:     linux-bcache@...r.kernel.org, agk@...hat.com, snitzer@...hat.com,
        kent.overstreet@...il.com, linux-kernel@...r.kernel.org,
        linux-raid@...r.kernel.org,
        Xianting Tian <xianting.tian@...ux.alibaba.com>,
        song@...nel.org, dm-devel@...hat.com
Subject: Re: [PATCH] md: use BLK_STS_OK instead of hardcode

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