Re: [PATCH v2] btrfs: btrfs_log_dev_io_error() on all bio errors

[Anand Jain <anajain.sg@gmail.com>]

Disagree. We should only track block-device-specific errors;
specifically, only the missing MEDIUM ERROR should be added [1].

Including all error types would capture transport and fabric-related
errors, which do not justify a device replacement or permanent error state.

In my experience with storage and transport protocols, transport errors
and timeouts are often transient and far more frequent than actual block
device media failures. Including them would introduce unnecessary noise
into the device statistics.


#define BLK_STS_NOTSUPP         ((__force blk_status_t)1)
#define BLK_STS_TIMEOUT         ((__force blk_status_t)2)
#define BLK_STS_NOSPC           ((__force blk_status_t)3)
#define BLK_STS_TRANSPORT       ((__force blk_status_t)4)
#define BLK_STS_TARGET          ((__force blk_status_t)5)
#define BLK_STS_RESV_CONFLICT   ((__force blk_status_t)6)
#define BLK_STS_MEDIUM          ((__force blk_status_t)7)
#define BLK_STS_PROTECTION      ((__force blk_status_t)8)
#define BLK_STS_RESOURCE        ((__force blk_status_t)9)
#define BLK_STS_IOERR           ((__force blk_status_t)10)
<snap>
#define BLK_STS_OFFLINE         ((__force blk_status_t)16)
#define BLK_STS_DURATION_LIMIT  ((__force blk_status_t)17)
#define BLK_STS_INVAL           ((__force blk_status_t)19)



[1]


diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index 2a2a21aec817..d7af38e9ce29 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -352,7 +352,8 @@ static void btrfs_log_dev_io_error(const struct bio
*bio, struct btrfs_device *d
 {
        if (!dev || !dev->bdev)
                return;
-       if (bio->bi_status != BLK_STS_IOERR && bio->bi_status !=
BLK_STS_TARGET)
+       if (bio->bi_status != BLK_STS_IOERR && bio->bi_status !=
BLK_STS_TARGET &&
+           bio->bi_status != BLK_STS_MEDIUM)
                return;

        if (btrfs_op(bio) == BTRFS_MAP_WRITE)



Thanks, Anand

On 4/4/26 23:57, Boris Burkov wrote:
> As far as I can tell, we never intentionally constrained ourselves to
> these status codes, and it is misleading and surprising to lack the
> bdev error logging when we get a different error code from the block
> layer. This can lead to jumping to a wrong conclusion like "this
> system didn't see any bio failures but aborted with EIO".
> 
> For example on nvme devices, I observe many failures coming back as
> BLK_STS_MEDIUM. It is apparent that the nvme driver returns a variety of
> BLK_STS_* status values in nvme_error_status().
> 
> So handle the known expected errors and make some noise on the rest
> which we expect won't really happen.
> 
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
> Changelog:
> v2:
> - proper bdev err logging for expected block errors
> - btrfs_warn_rl for all other errors
> ---
>  fs/btrfs/bio.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index 2a2a21aec817..08b1d62603d0 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -352,8 +352,7 @@ static void btrfs_log_dev_io_error(const struct bio *bio, struct btrfs_device *d
>  {
>  	if (!dev || !dev->bdev)
>  		return;
> -	if (bio->bi_status != BLK_STS_IOERR && bio->bi_status != BLK_STS_TARGET)
> -		return;
> +	ASSERT(bio->bi_status);
>  
>  	if (btrfs_op(bio) == BTRFS_MAP_WRITE)
>  		btrfs_dev_stat_inc_and_print(dev, BTRFS_DEV_STAT_WRITE_ERRS);