On Mon, Aug 18, 2025 at 03:22:02PM -0500, Eric Sandeen wrote:
> We had a report that a failing scsi disk was oopsing XFS when an xattr
> read encountered a media error. This is because the media error returned
> -ENODATA, which we map in xattr code to -ENOATTR and treat specially.
>
> In this particular case, it looked like:
>
> xfs_attr_leaf_get()
> error = xfs_attr_leaf_hasname(args, &bp);
> // here bp is NULL, error == -ENODATA from disk failure
> // but we define ENOATTR as ENODATA, so ...
> if (error == -ENOATTR) {
> // whoops, surprise! bp is NULL, OOPS here
> xfs_trans_brelse(args->trans, bp);
> return error;
> } ...
>
> To avoid whack-a-mole "test for null bp" or "which -ENODATA do we really
> mean in this function?" throughout the xattr code, my first thought is
> that we should simply map -ENODATA in lower level read functions back to
> -EIO, which is unambiguous, even if we lose the nuance of the underlying
> error code. (The block device probably already squawked.) Thoughts?
>
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index f9ef3b2a332a..6ba57ccaa25f 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -747,6 +747,9 @@ xfs_buf_read_map(
> /* bad CRC means corrupted metadata */
> if (error == -EFSBADCRC)
> error = -EFSCORRUPTED;
> + /* ENODATA == ENOATTR which confuses xattr layers */
> + if (error == -ENODATA)
> + error = -EIO;
Not sure this is the right place to map this. It is only relevant to
the XFS xattr layer in the kernel, so mapping it for everything
seems like overkill.
I suspect that this error mapping should really be in
xfs_attr3_leaf_read() and xfs_da_read_buf() so it is done for
xattr fork metadata read IO only...
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
