2021-06-22 16:11:54

by Coly Li

[permalink] [raw]
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 <[email protected]>
> ---
> 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