Re: [PATCH v2] fs: inode: return proper error code in bmap()

[Dave Chinner]

On Sat, Jul 15, 2023 at 05:22:04PM +0900, Leesoo Ahn wrote:
> Return -EOPNOTSUPP instead of -EINVAL which has the meaning of
> the argument is an inappropriate value. The current error code doesn't
> make sense to represent that a file system doesn't support bmap operation.
> 
> Signed-off-by: Leesoo Ahn <lsahn@xxxxxxxxxxxxxx>
> ---
> Changes since v1:
> - Modify the comments of bmap()
> - Modify subject and description requested by Markus Elfring
> https://lore.kernel.org/lkml/20230715060217.1469690-1-lsahn@xxxxxxxxxxxxxx/
> 
>  fs/inode.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 8fefb69e1f84..697c51ed226a 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1831,13 +1831,13 @@ EXPORT_SYMBOL(iput);
>   *	4 in ``*block``, with disk block relative to the disk start that holds that
>   *	block of the file.
>   *
> - *	Returns -EINVAL in case of error, 0 otherwise. If mapping falls into a
> + *	Returns -EOPNOTSUPP in case of error, 0 otherwise. If mapping falls into a
>   *	hole, returns 0 and ``*block`` is also set to 0.
>   */
>  int bmap(struct inode *inode, sector_t *block)
>  {
>  	if (!inode->i_mapping->a_ops->bmap)
> -		return -EINVAL;
> +		return -EOPNOTSUPP;
>  
>  	*block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
>  	return 0;

What about the CONFIG_BLOCK=n wrapper?

Also, all the in kernel consumers squash this error back to 0, -EIO
or -EINVAL, so this change only ever propagates out to userspace via
the return from ioctl(FIBMAP).  Do we really need to change this and
risk breaking userspace that handles -EINVAL correctly but not
-EOPNOTSUPP?

-Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx