Linux-EROFS Archive on lore.kernel.org
 help / color / Atom feed
* [RFC] errno.h: Provide EFSCORRUPTED for everybody
@ 2019-10-31  1:07 Valdis Kletnieks
  2019-10-31  3:04 ` Darrick J. Wong
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Valdis Kletnieks @ 2019-10-31  1:07 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: devel, linux-arch, Theodore Ts'o, Arnd Bergmann,
	Darrick J. Wong, linux-kernel, linux-f2fs-devel, linux-xfs,
	Andreas Dilger, Jaegeuk Kim, linux-fsdevel, Valdis Kletnieks,
	Gao Xiang, linux-ext4, linux-erofs, Jan Kara

Three questions: (a) ACK/NAK on this patch, (b) should it be all in one
patch, or one to add to errno.h and 6 patches for 6 filesystems?), and
(c) if one patch, who gets to shepherd it through?


There's currently 6 filesystems that have the same #define. Move it
into errno.h so it's defined in just one place.

Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
---
 drivers/staging/exfat/exfat.h    | 2 --
 fs/erofs/internal.h              | 2 --
 fs/ext4/ext4.h                   | 1 -
 fs/f2fs/f2fs.h                   | 1 -
 fs/xfs/xfs_linux.h               | 1 -
 include/linux/jbd2.h             | 1 -
 include/uapi/asm-generic/errno.h | 1 +
 7 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/staging/exfat/exfat.h b/drivers/staging/exfat/exfat.h
index 84de1123e178..3cf7e54af0b7 100644
--- a/drivers/staging/exfat/exfat.h
+++ b/drivers/staging/exfat/exfat.h
@@ -30,8 +30,6 @@
 #undef DEBUG
 #endif
 
-#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
-
 #define DENTRY_SIZE		32	/* dir entry size */
 #define DENTRY_SIZE_BITS	5
 
diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 544a453f3076..3980026a8882 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -425,7 +425,5 @@ static inline int z_erofs_init_zip_subsystem(void) { return 0; }
 static inline void z_erofs_exit_zip_subsystem(void) {}
 #endif	/* !CONFIG_EROFS_FS_ZIP */
 
-#define EFSCORRUPTED    EUCLEAN         /* Filesystem is corrupted */
-
 #endif	/* __EROFS_INTERNAL_H */
 
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 03db3e71676c..a86c2585457d 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3396,6 +3396,5 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)
 #endif	/* __KERNEL__ */
 
 #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
-#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
 
 #endif	/* _EXT4_H */
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 4024790028aa..04ebe77569a3 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3752,6 +3752,5 @@ static inline bool is_journalled_quota(struct f2fs_sb_info *sbi)
 }
 
 #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
-#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
 
 #endif /* _LINUX_F2FS_H */
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index ca15105681ca..3409d02a7d21 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -123,7 +123,6 @@ typedef __u32			xfs_nlink_t;
 
 #define ENOATTR		ENODATA		/* Attribute not found */
 #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
-#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
 #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
 
 #define SYNCHRONIZE()	barrier()
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 564793c24d12..1ecd3859d040 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1657,6 +1657,5 @@ static inline tid_t  jbd2_get_latest_transaction(journal_t *journal)
 #endif	/* __KERNEL__ */
 
 #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
-#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
 
 #endif	/* _LINUX_JBD2_H */
diff --git a/include/uapi/asm-generic/errno.h b/include/uapi/asm-generic/errno.h
index cf9c51ac49f9..1d5ffdf54cb0 100644
--- a/include/uapi/asm-generic/errno.h
+++ b/include/uapi/asm-generic/errno.h
@@ -98,6 +98,7 @@
 #define	EINPROGRESS	115	/* Operation now in progress */
 #define	ESTALE		116	/* Stale file handle */
 #define	EUCLEAN		117	/* Structure needs cleaning */
+#define	EFSCORRUPTED	EUCLEAN
 #define	ENOTNAM		118	/* Not a XENIX named type file */
 #define	ENAVAIL		119	/* No XENIX semaphores available */
 #define	EISNAM		120	/* Is a named type file */
-- 
2.24.0.rc1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] errno.h: Provide EFSCORRUPTED for everybody
  2019-10-31  1:07 [RFC] errno.h: Provide EFSCORRUPTED for everybody Valdis Kletnieks
@ 2019-10-31  3:04 ` Darrick J. Wong
  2019-10-31  3:16   ` Valdis Klētnieks
  2019-10-31 16:43 ` Jan Kara
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Darrick J. Wong @ 2019-10-31  3:04 UTC (permalink / raw)
  To: Valdis Kletnieks
  Cc: devel, linux-arch, Theodore Ts'o, Arnd Bergmann,
	Greg Kroah-Hartman, linux-kernel, linux-f2fs-devel, linux-xfs,
	Andreas Dilger, Jan Kara, linux-fsdevel, Jaegeuk Kim, Gao Xiang,
	linux-ext4, linux-erofs

On Wed, Oct 30, 2019 at 09:07:33PM -0400, Valdis Kletnieks wrote:
> Three questions: (a) ACK/NAK on this patch,

Acked-by: Darrick J. Wong <darrick.wong@oracle.com>

> (b) should it be all in one patch, or one to add to errno.h and 6
> patches for 6 filesystems?), and

I don't particularly care, but I've a slight preference for changing it
all at once so that it's obvious as a move.

> (c) if one patch, who gets to shepherd it through?

Heh. :)

I would add (d) can we do the same to EFSBADCRC, seeing as f2fs,
ext4, xfs, and jbd2 all define it the same way?

--D

> There's currently 6 filesystems that have the same #define. Move it
> into errno.h so it's defined in just one place.
> 
> Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
> ---
>  drivers/staging/exfat/exfat.h    | 2 --
>  fs/erofs/internal.h              | 2 --
>  fs/ext4/ext4.h                   | 1 -
>  fs/f2fs/f2fs.h                   | 1 -
>  fs/xfs/xfs_linux.h               | 1 -
>  include/linux/jbd2.h             | 1 -
>  include/uapi/asm-generic/errno.h | 1 +
>  7 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/exfat/exfat.h b/drivers/staging/exfat/exfat.h
> index 84de1123e178..3cf7e54af0b7 100644
> --- a/drivers/staging/exfat/exfat.h
> +++ b/drivers/staging/exfat/exfat.h
> @@ -30,8 +30,6 @@
>  #undef DEBUG
>  #endif
>  
> -#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
> -
>  #define DENTRY_SIZE		32	/* dir entry size */
>  #define DENTRY_SIZE_BITS	5
>  
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 544a453f3076..3980026a8882 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -425,7 +425,5 @@ static inline int z_erofs_init_zip_subsystem(void) { return 0; }
>  static inline void z_erofs_exit_zip_subsystem(void) {}
>  #endif	/* !CONFIG_EROFS_FS_ZIP */
>  
> -#define EFSCORRUPTED    EUCLEAN         /* Filesystem is corrupted */
> -
>  #endif	/* __EROFS_INTERNAL_H */
>  
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 03db3e71676c..a86c2585457d 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3396,6 +3396,5 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)
>  #endif	/* __KERNEL__ */
>  
>  #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
> -#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
>  
>  #endif	/* _EXT4_H */
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 4024790028aa..04ebe77569a3 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3752,6 +3752,5 @@ static inline bool is_journalled_quota(struct f2fs_sb_info *sbi)
>  }
>  
>  #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
> -#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
>  
>  #endif /* _LINUX_F2FS_H */
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index ca15105681ca..3409d02a7d21 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -123,7 +123,6 @@ typedef __u32			xfs_nlink_t;
>  
>  #define ENOATTR		ENODATA		/* Attribute not found */
>  #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
> -#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
>  #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
>  
>  #define SYNCHRONIZE()	barrier()
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 564793c24d12..1ecd3859d040 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1657,6 +1657,5 @@ static inline tid_t  jbd2_get_latest_transaction(journal_t *journal)
>  #endif	/* __KERNEL__ */
>  
>  #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
> -#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
>  
>  #endif	/* _LINUX_JBD2_H */
> diff --git a/include/uapi/asm-generic/errno.h b/include/uapi/asm-generic/errno.h
> index cf9c51ac49f9..1d5ffdf54cb0 100644
> --- a/include/uapi/asm-generic/errno.h
> +++ b/include/uapi/asm-generic/errno.h
> @@ -98,6 +98,7 @@
>  #define	EINPROGRESS	115	/* Operation now in progress */
>  #define	ESTALE		116	/* Stale file handle */
>  #define	EUCLEAN		117	/* Structure needs cleaning */
> +#define	EFSCORRUPTED	EUCLEAN
>  #define	ENOTNAM		118	/* Not a XENIX named type file */
>  #define	ENAVAIL		119	/* No XENIX semaphores available */
>  #define	EISNAM		120	/* Is a named type file */
> -- 
> 2.24.0.rc1
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] errno.h: Provide EFSCORRUPTED for everybody
  2019-10-31  3:04 ` Darrick J. Wong
@ 2019-10-31  3:16   ` Valdis Klētnieks
  0 siblings, 0 replies; 9+ messages in thread
From: Valdis Klētnieks @ 2019-10-31  3:16 UTC (permalink / raw)
  To: Darrick J. Wong
  Cc: devel, linux-arch, Theodore Ts'o, Arnd Bergmann,
	Greg Kroah-Hartman, linux-kernel, linux-f2fs-devel, linux-xfs,
	Andreas Dilger, Jan Kara, linux-fsdevel, Jaegeuk Kim, Gao Xiang,
	linux-ext4, linux-erofs

[-- Attachment #1: Type: text/plain, Size: 245 bytes --]

On Wed, 30 Oct 2019 20:04:49 -0700, "Darrick J. Wong" said:

> I would add (d) can we do the same to EFSBADCRC, seeing as f2fs,
> ext4, xfs, and jbd2 all define it the same way?

If this one flies, that's a good candidate for a second patch....

[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] errno.h: Provide EFSCORRUPTED for everybody
  2019-10-31  1:07 [RFC] errno.h: Provide EFSCORRUPTED for everybody Valdis Kletnieks
  2019-10-31  3:04 ` Darrick J. Wong
@ 2019-10-31 16:43 ` Jan Kara
  2019-10-31 21:23 ` Theodore Y. Ts'o
  2019-11-01 20:57 ` Geert Uytterhoeven
  3 siblings, 0 replies; 9+ messages in thread
From: Jan Kara @ 2019-10-31 16:43 UTC (permalink / raw)
  To: Valdis Kletnieks
  Cc: devel, linux-arch, Theodore Ts'o, Arnd Bergmann,
	Darrick J. Wong, Greg Kroah-Hartman, linux-kernel,
	linux-f2fs-devel, linux-xfs, Andreas Dilger, Jan Kara,
	linux-fsdevel, Jaegeuk Kim, Gao Xiang, linux-ext4, linux-erofs

On Wed 30-10-19 21:07:33, Valdis Kletnieks wrote:
> Three questions: (a) ACK/NAK on this patch, (b) should it be all in one
> patch, or one to add to errno.h and 6 patches for 6 filesystems?), and
> (c) if one patch, who gets to shepherd it through?
> 
> 
> There's currently 6 filesystems that have the same #define. Move it
> into errno.h so it's defined in just one place.
> 
> Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  drivers/staging/exfat/exfat.h    | 2 --
>  fs/erofs/internal.h              | 2 --
>  fs/ext4/ext4.h                   | 1 -
>  fs/f2fs/f2fs.h                   | 1 -
>  fs/xfs/xfs_linux.h               | 1 -
>  include/linux/jbd2.h             | 1 -
>  include/uapi/asm-generic/errno.h | 1 +
>  7 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/exfat/exfat.h b/drivers/staging/exfat/exfat.h
> index 84de1123e178..3cf7e54af0b7 100644
> --- a/drivers/staging/exfat/exfat.h
> +++ b/drivers/staging/exfat/exfat.h
> @@ -30,8 +30,6 @@
>  #undef DEBUG
>  #endif
>  
> -#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
> -
>  #define DENTRY_SIZE		32	/* dir entry size */
>  #define DENTRY_SIZE_BITS	5
>  
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
> index 544a453f3076..3980026a8882 100644
> --- a/fs/erofs/internal.h
> +++ b/fs/erofs/internal.h
> @@ -425,7 +425,5 @@ static inline int z_erofs_init_zip_subsystem(void) { return 0; }
>  static inline void z_erofs_exit_zip_subsystem(void) {}
>  #endif	/* !CONFIG_EROFS_FS_ZIP */
>  
> -#define EFSCORRUPTED    EUCLEAN         /* Filesystem is corrupted */
> -
>  #endif	/* __EROFS_INTERNAL_H */
>  
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 03db3e71676c..a86c2585457d 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3396,6 +3396,5 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)
>  #endif	/* __KERNEL__ */
>  
>  #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
> -#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
>  
>  #endif	/* _EXT4_H */
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 4024790028aa..04ebe77569a3 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3752,6 +3752,5 @@ static inline bool is_journalled_quota(struct f2fs_sb_info *sbi)
>  }
>  
>  #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
> -#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
>  
>  #endif /* _LINUX_F2FS_H */
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index ca15105681ca..3409d02a7d21 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -123,7 +123,6 @@ typedef __u32			xfs_nlink_t;
>  
>  #define ENOATTR		ENODATA		/* Attribute not found */
>  #define EWRONGFS	EINVAL		/* Mount with wrong filesystem type */
> -#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
>  #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
>  
>  #define SYNCHRONIZE()	barrier()
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 564793c24d12..1ecd3859d040 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1657,6 +1657,5 @@ static inline tid_t  jbd2_get_latest_transaction(journal_t *journal)
>  #endif	/* __KERNEL__ */
>  
>  #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
> -#define EFSCORRUPTED	EUCLEAN		/* Filesystem is corrupted */
>  
>  #endif	/* _LINUX_JBD2_H */
> diff --git a/include/uapi/asm-generic/errno.h b/include/uapi/asm-generic/errno.h
> index cf9c51ac49f9..1d5ffdf54cb0 100644
> --- a/include/uapi/asm-generic/errno.h
> +++ b/include/uapi/asm-generic/errno.h
> @@ -98,6 +98,7 @@
>  #define	EINPROGRESS	115	/* Operation now in progress */
>  #define	ESTALE		116	/* Stale file handle */
>  #define	EUCLEAN		117	/* Structure needs cleaning */
> +#define	EFSCORRUPTED	EUCLEAN
>  #define	ENOTNAM		118	/* Not a XENIX named type file */
>  #define	ENAVAIL		119	/* No XENIX semaphores available */
>  #define	EISNAM		120	/* Is a named type file */
> -- 
> 2.24.0.rc1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] errno.h: Provide EFSCORRUPTED for everybody
  2019-10-31  1:07 [RFC] errno.h: Provide EFSCORRUPTED for everybody Valdis Kletnieks
  2019-10-31  3:04 ` Darrick J. Wong
  2019-10-31 16:43 ` Jan Kara
@ 2019-10-31 21:23 ` Theodore Y. Ts'o
  2019-11-01 20:57 ` Geert Uytterhoeven
  3 siblings, 0 replies; 9+ messages in thread
From: Theodore Y. Ts'o @ 2019-10-31 21:23 UTC (permalink / raw)
  To: Valdis Kletnieks
  Cc: devel, linux-arch, Arnd Bergmann, Darrick J. Wong,
	Greg Kroah-Hartman, linux-kernel, linux-f2fs-devel, linux-xfs,
	Andreas Dilger, Jan Kara, linux-fsdevel, Jaegeuk Kim, Gao Xiang,
	linux-ext4, linux-erofs

On Wed, Oct 30, 2019 at 09:07:33PM -0400, Valdis Kletnieks wrote:
> Three questions: (a) ACK/NAK on this patch, (b) should it be all in one
> patch, or one to add to errno.h and 6 patches for 6 filesystems?), and
> (c) if one patch, who gets to shepherd it through?

Acked-by: Theodore Ts'o <tytso@mit.edu>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] errno.h: Provide EFSCORRUPTED for everybody
  2019-10-31  1:07 [RFC] errno.h: Provide EFSCORRUPTED for everybody Valdis Kletnieks
                   ` (2 preceding siblings ...)
  2019-10-31 21:23 ` Theodore Y. Ts'o
@ 2019-11-01 20:57 ` Geert Uytterhoeven
  2019-11-01 21:38   ` Dave Chinner
  3 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2019-11-01 20:57 UTC (permalink / raw)
  To: Valdis Kletnieks
  Cc: driverdevel, Linux-Arch, Theodore Ts'o, Arnd Bergmann,
	Darrick J. Wong, Greg Kroah-Hartman, Linux Kernel Mailing List,
	linux-f2fs-devel, linux-xfs, Andreas Dilger, Jan Kara,
	Linux FS Devel, Jaegeuk Kim, Gao Xiang, Ext4 Developers List,
	linux-erofs

Hi Valdis,

On Thu, Oct 31, 2019 at 2:11 AM Valdis Kletnieks
<valdis.kletnieks@vt.edu> wrote:
> Three questions: (a) ACK/NAK on this patch, (b) should it be all in one
> patch, or one to add to errno.h and 6 patches for 6 filesystems?), and
> (c) if one patch, who gets to shepherd it through?
>
> There's currently 6 filesystems that have the same #define. Move it
> into errno.h so it's defined in just one place.
>
> Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>

Thanks for your patch!

> --- a/include/uapi/asm-generic/errno.h
> +++ b/include/uapi/asm-generic/errno.h
> @@ -98,6 +98,7 @@
>  #define        EINPROGRESS     115     /* Operation now in progress */
>  #define        ESTALE          116     /* Stale file handle */
>  #define        EUCLEAN         117     /* Structure needs cleaning */
> +#define        EFSCORRUPTED    EUCLEAN

I have two questions:
a) Why not use EUCLEAN everywhere instead?
    Having two different names for the same errno complicates grepping.
b) Perhaps both errors should use different values? Do they have the
   same semantics? I'm not a fs developer, so this is a bit fuzzy to me.
   According to Documentation/, one seems to originate in mtd, the
   other in xfs.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] errno.h: Provide EFSCORRUPTED for everybody
  2019-11-01 20:57 ` Geert Uytterhoeven
@ 2019-11-01 21:38   ` Dave Chinner
  2019-11-05 15:15     ` David Sterba
  0 siblings, 1 reply; 9+ messages in thread
From: Dave Chinner @ 2019-11-01 21:38 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: driverdevel, Linux-Arch, Valdis Kletnieks, Arnd Bergmann,
	Darrick J. Wong, Greg Kroah-Hartman, Linux Kernel Mailing List,
	linux-f2fs-devel, linux-xfs, Andreas Dilger, Jaegeuk Kim,
	Linux FS Devel, Theodore Ts'o, Gao Xiang,
	Ext4 Developers List, linux-erofs, Jan Kara

On Fri, Nov 01, 2019 at 09:57:31PM +0100, Geert Uytterhoeven wrote:
> Hi Valdis,
> 
> On Thu, Oct 31, 2019 at 2:11 AM Valdis Kletnieks
> <valdis.kletnieks@vt.edu> wrote:
> > Three questions: (a) ACK/NAK on this patch, (b) should it be all in one
> > patch, or one to add to errno.h and 6 patches for 6 filesystems?), and
> > (c) if one patch, who gets to shepherd it through?
> >
> > There's currently 6 filesystems that have the same #define. Move it
> > into errno.h so it's defined in just one place.
> >
> > Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
> 
> Thanks for your patch!
> 
> > --- a/include/uapi/asm-generic/errno.h
> > +++ b/include/uapi/asm-generic/errno.h
> > @@ -98,6 +98,7 @@
> >  #define        EINPROGRESS     115     /* Operation now in progress */
> >  #define        ESTALE          116     /* Stale file handle */
> >  #define        EUCLEAN         117     /* Structure needs cleaning */
> > +#define        EFSCORRUPTED    EUCLEAN
> 
> I have two questions:
> a) Why not use EUCLEAN everywhere instead?
>     Having two different names for the same errno complicates grepping.

Because:
	a) EUCLEAN is horrible for code documentation. EFSCORRUPTED
	describes exactly the error being returned and/or checked for.

	b) we've used EFSCORRUPTED in XFS since 1993. i.e. it was an
	official, published error value on Irix, and we've kept it
	in the linux code for the past ~20 years because of a)

	c) Userspace programs that include filesystem specific
	headers have already been exposed to and use EFSCORRUPTED,
	so we can't remove/change it without breaking userspace.

	d) EUCLEAN has a convenient userspace string description
	that is appropriate for filesystem corruption: "Structure
	needs cleaning" is precisely what needs to happen. Repair of
	the filesystem (i.e. recovery to a clean state) is what is
	required to fix the error....

> b) Perhaps both errors should use different values?

That horse bolted to userspace years ago - this is just formalising
the practice that has spread across multiple linux filesystems from
XFS over the past ~10 years..

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] errno.h: Provide EFSCORRUPTED for everybody
  2019-11-01 21:38   ` Dave Chinner
@ 2019-11-05 15:15     ` David Sterba
  2019-11-06  6:39       ` Dave Chinner
  0 siblings, 1 reply; 9+ messages in thread
From: David Sterba @ 2019-11-05 15:15 UTC (permalink / raw)
  To: Dave Chinner
  Cc: driverdevel, Linux-Arch, Valdis Kletnieks, Arnd Bergmann,
	Darrick J. Wong, Greg Kroah-Hartman, Andreas Dilger,
	Linux Kernel Mailing List, linux-f2fs-devel, linux-xfs,
	Geert Uytterhoeven, Jaegeuk Kim, Linux FS Devel,
	Theodore Ts'o, Gao Xiang, Ext4 Developers List, linux-erofs,
	Jan Kara

On Sat, Nov 02, 2019 at 08:38:23AM +1100, Dave Chinner wrote:
> On Fri, Nov 01, 2019 at 09:57:31PM +0100, Geert Uytterhoeven wrote:
> > Hi Valdis,
> > 
> > On Thu, Oct 31, 2019 at 2:11 AM Valdis Kletnieks
> > <valdis.kletnieks@vt.edu> wrote:
> > > Three questions: (a) ACK/NAK on this patch, (b) should it be all in one
> > > patch, or one to add to errno.h and 6 patches for 6 filesystems?), and
> > > (c) if one patch, who gets to shepherd it through?
> > >
> > > There's currently 6 filesystems that have the same #define. Move it
> > > into errno.h so it's defined in just one place.
> > >
> > > Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
> > 
> > Thanks for your patch!
> > 
> > > --- a/include/uapi/asm-generic/errno.h
> > > +++ b/include/uapi/asm-generic/errno.h
> > > @@ -98,6 +98,7 @@
> > >  #define        EINPROGRESS     115     /* Operation now in progress */
> > >  #define        ESTALE          116     /* Stale file handle */
> > >  #define        EUCLEAN         117     /* Structure needs cleaning */
> > > +#define        EFSCORRUPTED    EUCLEAN
> > 
> > I have two questions:
> > a) Why not use EUCLEAN everywhere instead?
> >     Having two different names for the same errno complicates grepping.
> 
> Because:
> 	a) EUCLEAN is horrible for code documentation. EFSCORRUPTED
> 	describes exactly the error being returned and/or checked for.
> 
> 	b) we've used EFSCORRUPTED in XFS since 1993. i.e. it was an
> 	official, published error value on Irix, and we've kept it
> 	in the linux code for the past ~20 years because of a)
> 
> 	c) Userspace programs that include filesystem specific
> 	headers have already been exposed to and use EFSCORRUPTED,
> 	so we can't remove/change it without breaking userspace.
> 
> 	d) EUCLEAN has a convenient userspace string description
> 	that is appropriate for filesystem corruption: "Structure
> 	needs cleaning" is precisely what needs to happen. Repair of
> 	the filesystem (i.e. recovery to a clean state) is what is
> 	required to fix the error....

The description is very confusing to users that are also not filesystem
developers. "Structure needs cleaning" says what needs to be done but
not what happened. Unlike other error codes like "not enough memory",
"IO error" etc. We don't have EBUYMEM / "Buy more memory" instead of
ENOMEM.

Fuzzing tests and crafted images produce most of the EUCLEAN errors and
in this context "structure needs cleaning" makes even less sense.

> > b) Perhaps both errors should use different values?
> 
> That horse bolted to userspace years ago - this is just formalising
> the practice that has spread across multiple linux filesystems from
> XFS over the past ~10 years..

EFSCORRUPTED is a appropriate name but to share the number with EUCLEAN
was a terrible decision and now every filesystem has to suffer and
explain to users what the code really means and point to the the sad
story when asked "So why don't you have a separate code?".

I wonder what userspace package really depends on the value, that would
eg. change code flow. Uncommon error values usually lead to a message
and exit.

Debian code search shows only jython, e2fsprogs, xfsprogs, python2.7,
pypy, linux, partclone using EFSCORRUPTED. So 2 of them are filesystem
packages that can handle that, python/jython only defines the value for
IRIX platform. The rest is linux kernel, not relevant.

So please give me one example where userspace will break.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC] errno.h: Provide EFSCORRUPTED for everybody
  2019-11-05 15:15     ` David Sterba
@ 2019-11-06  6:39       ` Dave Chinner
  0 siblings, 0 replies; 9+ messages in thread
From: Dave Chinner @ 2019-11-06  6:39 UTC (permalink / raw)
  To: dsterba, Geert Uytterhoeven, Valdis Kletnieks,
	Greg Kroah-Hartman, Gao Xiang, Chao Yu, Theodore Ts'o,
	Andreas Dilger, Jaegeuk Kim, Darrick J. Wong, linux-xfs,
	Jan Kara, Arnd Bergmann, Linux FS Devel, driverdevel,
	Linux Kernel Mailing List, linux-erofs, Ext4 Developers List,
	linux-f2fs-devel, Linux-Arch

On Tue, Nov 05, 2019 at 04:15:50PM +0100, David Sterba wrote:
> On Sat, Nov 02, 2019 at 08:38:23AM +1100, Dave Chinner wrote:
> > On Fri, Nov 01, 2019 at 09:57:31PM +0100, Geert Uytterhoeven wrote:
> > > Hi Valdis,
> > > 
> > > On Thu, Oct 31, 2019 at 2:11 AM Valdis Kletnieks
> > > <valdis.kletnieks@vt.edu> wrote:
> > > > Three questions: (a) ACK/NAK on this patch, (b) should it be all in one
> > > > patch, or one to add to errno.h and 6 patches for 6 filesystems?), and
> > > > (c) if one patch, who gets to shepherd it through?
> > > >
> > > > There's currently 6 filesystems that have the same #define. Move it
> > > > into errno.h so it's defined in just one place.
> > > >
> > > > Signed-off-by: Valdis Kletnieks <Valdis.Kletnieks@vt.edu>
> > > 
> > > Thanks for your patch!
> > > 
> > > > --- a/include/uapi/asm-generic/errno.h
> > > > +++ b/include/uapi/asm-generic/errno.h
> > > > @@ -98,6 +98,7 @@
> > > >  #define        EINPROGRESS     115     /* Operation now in progress */
> > > >  #define        ESTALE          116     /* Stale file handle */
> > > >  #define        EUCLEAN         117     /* Structure needs cleaning */
> > > > +#define        EFSCORRUPTED    EUCLEAN
> > > 
> > > I have two questions:
> > > a) Why not use EUCLEAN everywhere instead?
> > >     Having two different names for the same errno complicates grepping.
> > 
> > Because:
> > 	a) EUCLEAN is horrible for code documentation. EFSCORRUPTED
> > 	describes exactly the error being returned and/or checked for.
> > 
> > 	b) we've used EFSCORRUPTED in XFS since 1993. i.e. it was an
> > 	official, published error value on Irix, and we've kept it
> > 	in the linux code for the past ~20 years because of a)
> > 
> > 	c) Userspace programs that include filesystem specific
> > 	headers have already been exposed to and use EFSCORRUPTED,
> > 	so we can't remove/change it without breaking userspace.
> > 
> > 	d) EUCLEAN has a convenient userspace string description
> > 	that is appropriate for filesystem corruption: "Structure
> > 	needs cleaning" is precisely what needs to happen. Repair of
> > 	the filesystem (i.e. recovery to a clean state) is what is
> > 	required to fix the error....
> 
> The description is very confusing to users that are also not filesystem
> developers.

That's a pretty good description of most error messages for modern
software packages. Anything that is a non-trivial error requires web
searching to understand and diagnose these days.

Google knows exactly what you are looking for if you search for
"mount structure needs cleaning" or other similar error messages.
That means users also know what it means and what they need to
do to address it.  i.e. it's been around long enough that there's a
deep history in internet search engines and question forums like
stackexchange.

> "Structure needs cleaning" says what needs to be done but
> not what happened. Unlike other error codes like "not enough memory",
> "IO error" etc. We don't have EBUYMEM / "Buy more memory" instead of
> ENOMEM.

Message grammar is largely irrelevant. And not unique to EUCLEAN. e.g.
EAGAIN = "Try again".

> Fuzzing tests and crafted images produce most of the EUCLEAN errors and
> in this context "structure needs cleaning" makes even less sense.

It's pretty silly to argue that a developer crafting and/or fuzzing
corrupted filesystem images is not going to understand what the
error message returned when they successfully corrupt a filesystem
actually means....

> > > b) Perhaps both errors should use different values?
> > 
> > That horse bolted to userspace years ago - this is just
> > formalising the practice that has spread across multiple linux
> > filesystems from XFS over the past ~10 years..
> 
> EFSCORRUPTED is a appropriate name but to share the number with
> EUCLEAN was a terrible decision and now every filesystem has to
> suffer and explain to users what the code really means and point
> to the the sad story when asked "So why don't you have a separate
> code?".

Damned if you do, damned if you don't.

Back in 2005-2006, XFS developers tried to make EFSCORRUPTED a
proper system wide errno (like we had on Irix). The NIH was strong
in the linux community back then, and we were largely told to go
away as the superior Linux filesystems would never need to report
corruption to userspace so we don't need a special errno just
because some shitty Irix filesystem port needs it.

And so we didn't get a new errno and went with the second choice:
precedence set by older unix systems....

commit 9e1fd551aba7291564d5d6c28948405142d5e2ca
Author: Nathan Scott <nathans@sgi.com>
Date:   Tue Jun 20 03:49:47 2006 +0000

    Map EFSCORRUPTED to an actual error code, not just a made up one
    (990).  Turns out some ye-olde unices used EUCLEAN as
    Filesystem-needs-cleaning, so now we use that too.
    Merge of xfs-linux-melb:xfs-kern:26286a by kenmcd.

diff --git a/fs/xfs/linux-2.6/xfs_linux.h b/fs/xfs/linux-2.6/xfs_linux.h
index 3910afa7..b4083f8c 100644
--- a/fs/xfs/linux-2.6/xfs_linux.h
+++ b/fs/xfs/linux-2.6/xfs_linux.h
@@ -197,25 +197,9 @@ BUFFER_FNS(PrivateStart, unwritten);
 /* bytes to clicks */
 #define btoc(x)         (((__psunsigned_t)(x)+(NBPC-1))>>BPCSHIFT)
 
-#ifndef ENOATTR
 #define ENOATTR                ENODATA         /* Attribute not found */
-#endif
-
-/* Note: EWRONGFS never visible outside the kernel */
-#define        EWRONGFS        EINVAL          /* Mount with wrong filesystem type */
-
-/*
- * XXX EFSCORRUPTED needs a real value in errno.h. asm-i386/errno.h won't
- *     return codes out of its known range in errno.
- * XXX Also note: needs to be < 1000 and fairly unique on Linux (mustn't
- *     conflict with any code we use already or any code a driver may use)
- * XXX Some options (currently we do #2):
- *     1/ New error code ["Filesystem is corrupted", _after_ glibc updated]
- *     2/ 990 ["Unknown error 990"]
- *     3/ EUCLEAN ["Structure needs cleaning"]
- *     4/ Convert EFSCORRUPTED to EIO [just prior to return into userspace]
- */
-#define EFSCORRUPTED    990            /* Filesystem is corrupted */
+#define EWRONGFS       EINVAL          /* Mount with wrong filesystem type */
+#define EFSCORRUPTED   EUCLEAN         /* Filesystem is corrupted */
 
 #define SYNCHRONIZE()  barrier()
 #define __return_address __builtin_return_address(0)

Perhaps you should learn the history of why certain decisions were
made before letting go with both barrels, hmmm?

> I wonder what userspace package really depends on the value, that
> would eg. change code flow. Uncommon error values usually lead to
> a message and exit.
> 
> Debian code search shows only jython, e2fsprogs, xfsprogs,
> python2.7, pypy, linux, partclone using EFSCORRUPTED. So 2 of them
> are filesystem packages that can handle that, python/jython only
> defines the value for IRIX platform. The rest is linux kernel, not
> relevant.

You didn't search for EUCLEAN, did you?

> So please give me one example where userspace will break.

ABI changes cannot be justified this way and you should damn well
know it. Especially as EUCLEAN/EFSCORRUPTED is documented in quite a
few man pages out there:

$ git grep -l EFSCORRUPTED man/
man/man2/ioctl_xfs_ag_geometry.2
man/man2/ioctl_xfs_bulkstat.2
man/man2/ioctl_xfs_fsbulkstat.2
man/man2/ioctl_xfs_fscounts.2
man/man2/ioctl_xfs_fsgetxattr.2
man/man2/ioctl_xfs_fsinumbers.2
man/man2/ioctl_xfs_fsop_geometry.2
man/man2/ioctl_xfs_getbmapx.2
man/man2/ioctl_xfs_getresblks.2
man/man2/ioctl_xfs_goingdown.2
man/man2/ioctl_xfs_inumbers.2
man/man2/ioctl_xfs_scrub_metadata.2

And EUCLEAN is in a few, too. e.g:

$ man ioctl_getfsmap |grep -A 1 EUCLEAN
       EUCLEAN
              The filesystem metadata is corrupt and needs repair.

So, yeah, the EFSCORRUPTED = EUCLEAN horse bolted long ago, and no
amount of retconning will put the genie back in the bottle.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-31  1:07 [RFC] errno.h: Provide EFSCORRUPTED for everybody Valdis Kletnieks
2019-10-31  3:04 ` Darrick J. Wong
2019-10-31  3:16   ` Valdis Klētnieks
2019-10-31 16:43 ` Jan Kara
2019-10-31 21:23 ` Theodore Y. Ts'o
2019-11-01 20:57 ` Geert Uytterhoeven
2019-11-01 21:38   ` Dave Chinner
2019-11-05 15:15     ` David Sterba
2019-11-06  6:39       ` Dave Chinner

Linux-EROFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-erofs/0 linux-erofs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-erofs linux-erofs/ https://lore.kernel.org/linux-erofs \
		linux-erofs@lists.ozlabs.org linux-erofs@ozlabs.org
	public-inbox-index linux-erofs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linux-erofs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git