Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 16 Jul 2023 09:36:41 +1000
From:   Dave Chinner <david@fromorbit.com>
To:     Leesoo Ahn <lsahn@ooseel.net>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Leesoo Ahn <lsahn@wewakecorp.com>
Subject: Re: [PATCH v2] fs: inode: return proper error code in bmap()
Message-ID: <ZLMtifV5ta5VTQ2e@dread.disaster.area>
References: <20230715082204.1598206-1-lsahn@wewakecorp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230715082204.1598206-1-lsahn@wewakecorp.com>
Precedence: bulk

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@wewakecorp.com>
> ---
> 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@wewakecorp.com/
> 
>  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@fromorbit.com