Re: [PATCH 1/2] ext4: Use bitops to read/modify EXT4_I(inode)->i_flags

[tytso@mit.edu]

On Mon, Apr 19, 2010 at 06:32:16PM +0400, Dmitry Monakhov wrote:
> At several places we modify EXT4_I(inode)->i_flags without holding
> i_mutex (ext4_do_update_inode, ...). These modifications are racy and we can
> lose updates to i_flags. So convert handling of i_flags to use bitops
> which are atomic.
> https://bugzilla.kernel.org/show_bug.cgi?id=15792

I don't think it would cause any harm, since those flags aren't used
(yet) but this patch is buggy; The values for EXT4_INODE_EA_INODE, and
EXT4_INODE_RESERVED are wrong.  Fortunately I'm a paranoid b*stard, so
I caught it by *not* deleting the explicit EXT4_XXX_FL flags, and
using this:

#define TEST_FLAG_VALUE(FLAG) (EXT4_##FLAG##_FL == (1 << EXT4_INODE_##FLAG))
#define CHECK_FLAG_VALUE(FLAG) if (!TEST_FLAG_VALUE(FLAG)) { \
	printk("EXT4 flag fail: " #FLAG ": %d %d\n", EXT4_##FLAG##_FL, \
	       EXT4_INODE_##FLAG); BUG_ON(1); }

/*
 * Since it's pretty easy to mix up bit numbers and hex values, and we
 * can't do a compile-time test for ENUM values, we use a run-time
 * test to make sure that EXT4_XXX_FL is consistent with respect to
 * EXT4_INODE_XXX.  If all is well the printk and BUG_ON will all drop
 * out so it won't cost any extra space in the compiled kernel image.
 * But it's important that these values are the same, since we are
 * using EXT4_INODE_XXX to test for the flag values, but EXT4_XX_FL
 * must be consistent with the values of FS_XXX_FL defined in
 * include/linux/fs.h and the on-disk values found in ext2, ext3, and
 * ext4 filesystems, and of course the values defined in e2fsprogs.
 *
 * It's not paranoia if Murphy's Law really *is* out to get you.  :-)
 */
static inline void ext4_check_flag_values(void)
{
	CHECK_FLAG_VALUE(SECRM);
	CHECK_FLAG_VALUE(UNRM);
	CHECK_FLAG_VALUE(COMPR);
	CHECK_FLAG_VALUE(SYNC);
	CHECK_FLAG_VALUE(IMMUTABLE);
	CHECK_FLAG_VALUE(APPEND);
	CHECK_FLAG_VALUE(NODUMP);
	CHECK_FLAG_VALUE(NOATIME);
	CHECK_FLAG_VALUE(DIRTY);
	CHECK_FLAG_VALUE(COMPRBLK);
	CHECK_FLAG_VALUE(NOCOMPR);
	CHECK_FLAG_VALUE(ECOMPR);
	CHECK_FLAG_VALUE(INDEX);
	CHECK_FLAG_VALUE(IMAGIC);
	CHECK_FLAG_VALUE(JOURNAL_DATA);
	CHECK_FLAG_VALUE(NOTAIL);
	CHECK_FLAG_VALUE(DIRSYNC);
	CHECK_FLAG_VALUE(TOPDIR);
	CHECK_FLAG_VALUE(HUGE_FILE);
	CHECK_FLAG_VALUE(EXTENTS);
	CHECK_FLAG_VALUE(EA_INODE);
	CHECK_FLAG_VALUE(EOFBLOCKS);
	CHECK_FLAG_VALUE(RESERVED);
}

I'll send the full patch after I finish doing some testing, but I
thought I'd just point this out before I hit the sack...

	    	       	    	       	 - Ted