[01/15] iomap: Introduce CONFIG_FS_IOMAP_DEBUG
diff mbox series

Message ID 20190901200836.14959-2-rgoldwyn@suse.de
State New
Headers show
Series
  • Btrfs iomap
Related show

Commit Message

Goldwyn Rodrigues Sept. 1, 2019, 8:08 p.m. UTC
From: Goldwyn Rodrigues <rgoldwyn@suse.com>

To improve debugging abilities, especially invalid option
asserts.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/Kconfig            |  3 +++
 include/linux/iomap.h | 11 +++++++++++
 2 files changed, 14 insertions(+)

Comments

Darrick J. Wong Sept. 1, 2019, 8:50 p.m. UTC | #1
On Sun, Sep 01, 2019 at 03:08:22PM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> To improve debugging abilities, especially invalid option
> asserts.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/Kconfig            |  3 +++
>  include/linux/iomap.h | 11 +++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/fs/Kconfig b/fs/Kconfig
> index bfb1c6095c7a..4bed5df9b55f 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -19,6 +19,9 @@ if BLOCK
>  
>  config FS_IOMAP
>  	bool
> +config FS_IOMAP_DEBUG
> +	bool "Debugging for the iomap code"
> +	depends on FS_IOMAP
>  
>  source "fs/ext2/Kconfig"
>  source "fs/ext4/Kconfig"
> diff --git a/include/linux/iomap.h b/include/linux/iomap.h
> index bc499ceae392..209b6c93674e 100644
> --- a/include/linux/iomap.h
> +++ b/include/linux/iomap.h
> @@ -18,6 +18,17 @@ struct page;
>  struct vm_area_struct;
>  struct vm_fault;
>  
> +#ifdef CONFIG_FS_IOMAP_DEBUG
> +#define iomap_assert(expr) \
> +	if(!(expr)) { \
> +		printk( "Assertion failed! %s,%s,%s,line=%d\n",\
> +#expr,__FILE__,__func__,__LINE__); \
> +		BUG(); \

Hmm, this log message ought to have a priority level, right?

#define IOMAP_ASSERT(expr) do { WARN_ON(!(expr)); } while(0)

(or crib ASSERT/ass{fail,warn} from XFS? :D)

> +	}
> +#else
> +#define iomap_assert(expr)

((void)0)

So we don't just have stray semicolons in the code stream?

--D

> +#endif
> +
>  /*
>   * Types of block ranges for iomap mappings:
>   */
> -- 
> 2.16.4
>
Christoph Hellwig Sept. 2, 2019, 4:29 p.m. UTC | #2
On Sun, Sep 01, 2019 at 03:08:22PM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> To improve debugging abilities, especially invalid option
> asserts.

Looking at the code I'd much rather have unconditional WARN_ON_ONCE
statements in most places.  Including returning an error when we see
something invalid in most cases.
Darrick J. Wong Sept. 2, 2019, 5:09 p.m. UTC | #3
On Mon, Sep 02, 2019 at 06:29:34PM +0200, Christoph Hellwig wrote:
> On Sun, Sep 01, 2019 at 03:08:22PM -0500, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > To improve debugging abilities, especially invalid option
> > asserts.
> 
> Looking at the code I'd much rather have unconditional WARN_ON_ONCE
> statements in most places.  Including returning an error when we see
> something invalid in most cases.

Yeah, I was thinking something like this, which has the advantage that
the report format is familiar to XFS developers and will get picked up
by the automated error collection stuff I put in xfstests to complain
about any XFS assertion failures:

iomap: Introduce CONFIG_FS_IOMAP_DEBUG

To improve debugging abilities, especially invalid option
asserts.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
[darrick: restructure it to follow what xfs does]
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/Kconfig            |    3 +++
 fs/iomap/apply.c      |   10 ++++++++++
 include/linux/iomap.h |   14 ++++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/fs/Kconfig b/fs/Kconfig
index bfb1c6095c7a..4bed5df9b55f 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -19,6 +19,9 @@ if BLOCK
 
 config FS_IOMAP
 	bool
+config FS_IOMAP_DEBUG
+	bool "Debugging for the iomap code"
+	depends on FS_IOMAP
 
 source "fs/ext2/Kconfig"
 source "fs/ext4/Kconfig"
diff --git a/fs/iomap/apply.c b/fs/iomap/apply.c
index 54c02aecf3cd..95cc3a2cadd5 100644
--- a/fs/iomap/apply.c
+++ b/fs/iomap/apply.c
@@ -8,6 +8,16 @@
 #include <linux/fs.h>
 #include <linux/iomap.h>
 
+#ifdef CONFIG_FS_IOMAP_DEBUG
+void
+iomap_assertion_failed(const char *expr, const char *file, int line)
+{
+	printk(KERN_EMERG "IOMAP: Assertion failed: %s, file: %s, line: %d",
+	       expr, file, line);
+	WARN_ON_ONCE(1);
+}
+#endif
+
 /*
  * Execute a iomap write on a segment of the mapping that spans a
  * contiguous range of pages that have identical block mapping state.
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index 834d3923e2f2..b3d5d6f323cf 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -20,6 +20,20 @@ struct page;
 struct vm_area_struct;
 struct vm_fault;
 
+#ifdef CONFIG_FS_IOMAP_DEBUG
+
+extern void iomap_assertion_failed(const char *expr, const char *f, int l);
+
+#define IOMAP_ASSERT(expr) \
+	do { \
+		if (unlikely(!(expr))) \
+			iomap_assertion_failed(#expr, __FILE__, __LINE__); \
+	} while(0)
+
+#else
+#define IOMAP_ASSERT(expr)	((void)0)
+#endif
+
 /*
  * Types of block ranges for iomap mappings:
  */
Christoph Hellwig Sept. 2, 2019, 5:18 p.m. UTC | #4
On Mon, Sep 02, 2019 at 10:09:16AM -0700, Darrick J. Wong wrote:
> On Mon, Sep 02, 2019 at 06:29:34PM +0200, Christoph Hellwig wrote:
> > On Sun, Sep 01, 2019 at 03:08:22PM -0500, Goldwyn Rodrigues wrote:
> > > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > > 
> > > To improve debugging abilities, especially invalid option
> > > asserts.
> > 
> > Looking at the code I'd much rather have unconditional WARN_ON_ONCE
> > statements in most places.  Including returning an error when we see
> > something invalid in most cases.
> 
> Yeah, I was thinking something like this, which has the advantage that
> the report format is familiar to XFS developers and will get picked up
> by the automated error collection stuff I put in xfstests to complain
> about any XFS assertion failures:
> 
> iomap: Introduce CONFIG_FS_IOMAP_DEBUG
> 
> To improve debugging abilities, especially invalid option
> asserts.

I'd actually just rather have more unconditional WARN_ON_ONCE calls,
including actually recovering from the situation by return an actual
error code.  That is more

	if (WARN_ON_ONCE(some_impossible_condition))
		return -EIO;
Darrick J. Wong Sept. 3, 2019, 3:06 p.m. UTC | #5
On Mon, Sep 02, 2019 at 07:18:00PM +0200, Christoph Hellwig wrote:
> On Mon, Sep 02, 2019 at 10:09:16AM -0700, Darrick J. Wong wrote:
> > On Mon, Sep 02, 2019 at 06:29:34PM +0200, Christoph Hellwig wrote:
> > > On Sun, Sep 01, 2019 at 03:08:22PM -0500, Goldwyn Rodrigues wrote:
> > > > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > > > 
> > > > To improve debugging abilities, especially invalid option
> > > > asserts.
> > > 
> > > Looking at the code I'd much rather have unconditional WARN_ON_ONCE
> > > statements in most places.  Including returning an error when we see
> > > something invalid in most cases.
> > 
> > Yeah, I was thinking something like this, which has the advantage that
> > the report format is familiar to XFS developers and will get picked up
> > by the automated error collection stuff I put in xfstests to complain
> > about any XFS assertion failures:
> > 
> > iomap: Introduce CONFIG_FS_IOMAP_DEBUG
> > 
> > To improve debugging abilities, especially invalid option
> > asserts.
> 
> I'd actually just rather have more unconditional WARN_ON_ONCE calls,
> including actually recovering from the situation by return an actual
> error code.  That is more
> 
> 	if (WARN_ON_ONCE(some_impossible_condition))
> 		return -EIO;

Oh, right, WARNings actually do spit out the file and line number.
Let's do that. :)

--D

Patch
diff mbox series

diff --git a/fs/Kconfig b/fs/Kconfig
index bfb1c6095c7a..4bed5df9b55f 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -19,6 +19,9 @@  if BLOCK
 
 config FS_IOMAP
 	bool
+config FS_IOMAP_DEBUG
+	bool "Debugging for the iomap code"
+	depends on FS_IOMAP
 
 source "fs/ext2/Kconfig"
 source "fs/ext4/Kconfig"
diff --git a/include/linux/iomap.h b/include/linux/iomap.h
index bc499ceae392..209b6c93674e 100644
--- a/include/linux/iomap.h
+++ b/include/linux/iomap.h
@@ -18,6 +18,17 @@  struct page;
 struct vm_area_struct;
 struct vm_fault;
 
+#ifdef CONFIG_FS_IOMAP_DEBUG
+#define iomap_assert(expr) \
+	if(!(expr)) { \
+		printk( "Assertion failed! %s,%s,%s,line=%d\n",\
+#expr,__FILE__,__func__,__LINE__); \
+		BUG(); \
+	}
+#else
+#define iomap_assert(expr)
+#endif
+
 /*
  * Types of block ranges for iomap mappings:
  */