| Message ID | 1603098158-30406-1-git-send-email-brookxu@tencent.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/8] ext4: use ASSERT() to replace J_ASSERT() | expand |
On Oct 19, 2020, at 3:02 AM, Chunguang Xu <brookxu.cn@gmail.com> wrote: > > There are currently multiple forms of assertion, such as J_ASSERT(). > J_ASEERT is provided for the jbd module, which is a public module. (typo) "J_ASSERT()" > Maybe we should use custom ASSERT() like other file systems, such as > xfs, which would be better. My one minor complaint is that "ASSERT()" is a very generic name and is likely to cause conflicts with ASSERT in other headers. That said, I also see many other filesystems using their own ASSERT() macro, so I guess they are all in private headers only? Some minor comments/questions below, but not worth changing the patch unless you think they are important... You can add: Reviewed-by: Andreas Dilger <adilger@dilger.ca> > @@ -185,7 +185,7 @@ static int ext4_init_block_bitmap(struct super_block *sb, > struct ext4_sb_info *sbi = EXT4_SB(sb); > ext4_fsblk_t start, tmp; > > - J_ASSERT_BH(bh, buffer_locked(bh)); > + ASSERT(buffer_locked(bh)); I thought J_ASSERT_BH() did something useful, but J_ASSERT_BH() just maps to J_ASSERT() internally anyway. > +#define ASSERT(assert) \ > +do { \ > + if (unlikely(!(assert))) { \ > + printk(KERN_EMERG \ > + "Assertion failure in %s() at %s:%d: \"%s\"\n", \ (style) better to use single quotes '%s' to avoid the need to escape \". Cheers, Andreas
Thanks for your reply. Andreas Dilger wrote on 2020/10/20 11:37: > On Oct 19, 2020, at 3:02 AM, Chunguang Xu <brookxu.cn@gmail.com> wrote: >> >> There are currently multiple forms of assertion, such as J_ASSERT(). >> J_ASEERT is provided for the jbd module, which is a public module. > > (typo) "J_ASSERT()" Thanks, I will Fixed that. >> Maybe we should use custom ASSERT() like other file systems, such as >> xfs, which would be better. > > My one minor complaint is that "ASSERT()" is a very generic name and is > likely to cause conflicts with ASSERT in other headers. That said, I > also see many other filesystems using their own ASSERT() macro, so I > guess they are all in private headers only? I also thought about this before, but even if we define it in a private header file, because we still include several header files in a certain file, it seems that the conflict cannot be resolved. However, maybe it is safest to use a name with ext4 prefix. I will try to fix it in next version. Thanks. > Some minor comments/questions below, but not worth changing the patch > unless you think they are important... > > You can add: > > Reviewed-by: Andreas Dilger <adilger@dilger.ca> > >> @@ -185,7 +185,7 @@ static int ext4_init_block_bitmap(struct super_block *sb, >> struct ext4_sb_info *sbi = EXT4_SB(sb); >> ext4_fsblk_t start, tmp; >> >> - J_ASSERT_BH(bh, buffer_locked(bh)); >> + ASSERT(buffer_locked(bh)); > > I thought J_ASSERT_BH() did something useful, but J_ASSERT_BH() just maps > to J_ASSERT() internally anyway. > >> +#define ASSERT(assert) \ >> +do { \ >> + if (unlikely(!(assert))) { \ >> + printk(KERN_EMERG \ >> + "Assertion failure in %s() at %s:%d: \"%s\"\n", \ > > (style) better to use single quotes '%s' to avoid the need to escape \". Thanks, this is a good suggestion, I will fix it next version. > Cheers, Andreas > > > > >
diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c index dea738b..386cfc3 100644 --- a/fs/ext4/balloc.c +++ b/fs/ext4/balloc.c @@ -185,7 +185,7 @@ static int ext4_init_block_bitmap(struct super_block *sb, struct ext4_sb_info *sbi = EXT4_SB(sb); ext4_fsblk_t start, tmp; - J_ASSERT_BH(bh, buffer_locked(bh)); + ASSERT(buffer_locked(bh)); /* If checksum is bad mark all blocks used to prevent allocation * essentially implementing a per-group read-only flag. */ diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 250e905..512f833 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -98,6 +98,16 @@ #define ext_debug(ino, fmt, ...) no_printk(fmt, ##__VA_ARGS__) #endif +#define ASSERT(assert) \ +do { \ + if (unlikely(!(assert))) { \ + printk(KERN_EMERG \ + "Assertion failure in %s() at %s:%d: \"%s\"\n", \ + __func__, __FILE__, __LINE__, #assert); \ + BUG(); \ + } \ +} while (0) + /* data type for block offset of block group */ typedef int ext4_grpblk_t; diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c index 6476994..f9e33c7 100644 --- a/fs/ext4/fsync.c +++ b/fs/ext4/fsync.c @@ -136,7 +136,7 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync) if (unlikely(ext4_forced_shutdown(sbi))) return -EIO; - J_ASSERT(ext4_journal_current_handle() == NULL); + ASSERT(ext4_journal_current_handle() == NULL); trace_ext4_sync_file_enter(file, datasync); diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c index 05efa682..1223a18 100644 --- a/fs/ext4/indirect.c +++ b/fs/ext4/indirect.c @@ -534,8 +534,8 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode, ext4_fsblk_t first_block = 0; trace_ext4_ind_map_blocks_enter(inode, map->m_lblk, map->m_len, flags); - J_ASSERT(!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))); - J_ASSERT(handle != NULL || (flags & EXT4_GET_BLOCKS_CREATE) == 0); + ASSERT(!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))); + ASSERT(handle != NULL || (flags & EXT4_GET_BLOCKS_CREATE) == 0); depth = ext4_block_to_path(inode, map->m_lblk, offsets, &blocks_to_boundary); diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 09096fe..8113898 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -825,7 +825,7 @@ struct buffer_head *ext4_getblk(handle_t *handle, struct inode *inode, int create = map_flags & EXT4_GET_BLOCKS_CREATE; int err; - J_ASSERT(handle != NULL || create == 0); + ASSERT(handle != NULL || create == 0); map.m_lblk = block; map.m_len = 1; @@ -840,8 +840,8 @@ struct buffer_head *ext4_getblk(handle_t *handle, struct inode *inode, if (unlikely(!bh)) return ERR_PTR(-ENOMEM); if (map.m_flags & EXT4_MAP_NEW) { - J_ASSERT(create != 0); - J_ASSERT(handle != NULL); + ASSERT(create != 0); + ASSERT(handle != NULL); /* * Now that we do not always journal data, we should diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index b735372..1cb9424 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -182,10 +182,6 @@ static struct buffer_head *__ext4_read_dirblock(struct inode *inode, return bh; } -#ifndef assert -#define assert(test) J_ASSERT(test) -#endif - #ifdef DX_DEBUG #define dxtrace(command) command #else @@ -849,7 +845,7 @@ struct stats dx_show_entries(struct dx_hash_info *hinfo, struct inode *dir, break; } } - assert (at == p - 1); + ASSERT(at == p - 1); } at = p - 1; @@ -1265,8 +1261,8 @@ static void dx_insert_block(struct dx_frame *frame, u32 hash, ext4_lblk_t block) struct dx_entry *old = frame->at, *new = old + 1; int count = dx_get_count(entries); - assert(count < dx_get_limit(entries)); - assert(old < entries + count); + ASSERT(count < dx_get_limit(entries)); + ASSERT(old < entries + count); memmove(new + 1, new, (char *)(entries + count) - (char *)(new)); dx_set_hash(new, hash); dx_set_block(new, block); @@ -2959,7 +2955,7 @@ int ext4_orphan_add(handle_t *handle, struct inode *inode) * hold i_mutex, or the inode can not be referenced from outside, * so i_nlink should not be bumped due to race */ - J_ASSERT((S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) || + ASSERT((S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode)) || inode->i_nlink == 0); BUFFER_TRACE(sbi->s_sbh, "get_write_access"); diff --git a/fs/ext4/super.c b/fs/ext4/super.c index 9d01318..8aa163a 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -1251,7 +1251,7 @@ static void ext4_put_super(struct super_block *sb) * in-memory list had better be clean by this point. */ if (!list_empty(&sbi->s_orphan)) dump_orphan_list(sb, sbi); - J_ASSERT(list_empty(&sbi->s_orphan)); + ASSERT(list_empty(&sbi->s_orphan)); sync_blockdev(sb->s_bdev); invalidate_bdev(sb->s_bdev);
There are currently multiple forms of assertion, such as J_ASSERT(). J_ASEERT is provided for the jbd module, which is a public module. Maybe we should use custom ASSERT() like other file systems, such as xfs, which would be better. Signed-off-by: Chunguang Xu <brookxu@tencent.com> --- fs/ext4/balloc.c | 2 +- fs/ext4/ext4.h | 10 ++++++++++ fs/ext4/fsync.c | 2 +- fs/ext4/indirect.c | 4 ++-- fs/ext4/inode.c | 6 +++--- fs/ext4/namei.c | 12 ++++-------- fs/ext4/super.c | 2 +- 7 files changed, 22 insertions(+), 16 deletions(-)