| Message ID | 20190901200836.14959-2-rgoldwyn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Btrfs iomap | expand |
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 >
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.
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: */
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;
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
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: */