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

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

[Leesoo Ahn]

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;
-- 
2.34.1

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

[Leesoo Ahn]

2023-07-15 오후 9:50에 Markus Elfring 이(가) 쓴 글:
> 
> * How do you think about to add the tag “Fixes”?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc1#n591I think the description is enough, but still open to listen for that 
from others.

> 
> * Would you like to avoid a checkpatch warning by the specification
> of the tag “From” before your improved change description?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc1#n499
I use both emails and don't think that's a big deal. But if that 
matters, I will post v3 for that or maintainers could sync up the email 
of author by hands when they merge it.

thank you for your opinion,
Leesoo

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

[Matthew Wilcox]

On Sat, Jul 15, 2023 at 05:22:04PM +0900, Leesoo Ahn wrote:
> Return -EOPNOTSUPP instead of -EINVAL which has the meaning of

EOPNOTSUPP is the wrong errno.  ENOTTY is "more" correct.

By the way, don't send v2 just because Markus has picked at a nit.
Wait for substantive feedback.

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@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

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

[Leesoo Ahn]

23. 7. 16. 08:36에 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@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?
How does it work? Could you explain that in details, pls?
However, as far as I understand, bmap operation could be NULL even 
though CONFIG_BLOCK is enabled. It totally depends on the implementation 
of file systems.

> 
> 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?
That's a consideration and we must carefully modify the APIs which 
communicate to users. But -EINVAL could be interpreted by two cases at 
this point that the first, for sure an argument from user to kernel is 
inappropriate, on the other hand, the second case would be that a file 
system doesn't support bmap operation. However, I don't think there is a 
proper way to know which one is right from user.

For me, the big problem is that user could get confused by these two 
cases with the same error code.

Best regards,
Leesoo

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

[Leesoo Ahn]

23. 7. 15. 23:56에 Matthew Wilcox 이(가) 쓴 글:
> On Sat, Jul 15, 2023 at 05:22:04PM +0900, Leesoo Ahn wrote:
>  > Return -EOPNOTSUPP instead of -EINVAL which has the meaning of
> 
> EOPNOTSUPP is the wrong errno. ENOTTY is "more" correct.
Thank you for the feedback, just figured out that ioctl never returns 
the error code.

Best regards,
Leesoo

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

[Dave Chinner]

On Tue, Jul 18, 2023 at 12:08:07AM +0900, Leesoo Ahn wrote:
> 23. 7. 16. 08:36에 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@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?
> How does it work? Could you explain that in details, pls?
> However, as far as I understand, bmap operation could be NULL even though
> CONFIG_BLOCK is enabled. It totally depends on the implementation of file
> systems.

That wrapper returns -EINVAL unconditionally. If CONFIG_BLOCK=n,
then by your reasoning it should return -EOPNOTSUPP, not -EINVAL,
so you need to fix that as well.

> > 
> > 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?
> That's a consideration and we must carefully modify the APIs which
> communicate to users. But -EINVAL could be interpreted by two cases at this
> point that the first, for sure an argument from user to kernel is
> inappropriate, on the other hand, the second case would be that a file
> system doesn't support bmap operation. However, I don't think there is a
> proper way to know which one is right from user.

That doesn't matter a whole lot - what matters is if the change of
return value breaks existing userspace binaries. That's on you to
audit all known userspace users (e.g. via debian codesearch) to
determine that nothing in userspace using FIBMAP cares about the
change of return value.

> For me, the big problem is that user could get confused by these two cases
> with the same error code.

So you're talking about a theoretical problem, not an actual real
world issue that is causing an application to do the wrong thing?

-Dave.
-- 
Dave Chinner
david@fromorbit.com