On 2011-01-05, at 13:29, Ted Ts'o wrote: > On Wed, Jan 05, 2011 at 11:43:08AM -0700, Andreas Dilger wrote: >> >> It looks like you have compensated for the above by changing the >> code here, but I think it is risky/confusing if clearing the state >> flags has a side-effect on 64-bit arches, that doesn't exist on >> 32-bit arches. It looks like a bug waiting to happen... > > Yeah, I did think of this, but it seemed like extra/needless work that > I was trying to optimize away. It's still not safe to do: > > #define EXT4_CLEAR_STATE_FLAGS(ei) (ei)->i_flags &= 0xffffffffULL; > > ... since we're not atomically updating i_flags. This code would only be used on a 64-bit arch, so it should be updating the whole word at once (unlike a 32-bit arch). > So if anyone tries using EXT4_CLEAR_STATE_FLAGS() aside from the two > allocation contexts, they need to be careful anyway. > > I did think about putting the #ifdef BITS_PER_LONG < 64 inline in the > code, but that's ugly. I'm missing the point of that - isn't the EXT4_CLEAR_STATE_FLAGS() macro masking already conditional on 64-bit architectures? The one call in ext4_do_update_inode() that is masking i_flags is redundant, since the cpu_to_le32() macro is itself either masking the value before swabbing, and/or it is truncated by the assignment to i_flags. > Maybe the best thing to do is to clearly document this pitfall, and > then leave things as-is? There aren't a lot of great solutions. It doesn't matter so much to me in the end. At least documenting this anomaly is useful, and I don't think that doing the masking is harmful. Cheers, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
