On Fri, May 7, 2021 at 8:56 AM Theodore Ts'o <tytso@xxxxxxx> wrote: > > On Thu, May 06, 2021 at 11:45:09PM -0700, Eric Biggers wrote: > > Just to be clear (looking at the latest patches on the list which are copying > > whole structs), by "the memcpy() approach does get optimized properly", I meant > > that it gets optimized properly in implementations of get_unaligned_le16(), > > get_unaligned_le32(), put_unaligned_le32(), etc., where a single word (or less > > than a word) is loaded or stored. I don't know how reliably the compilers will > > optimize out the copy if you memcpy() a whole struct instead of a single word. > > > > Even if they don't optimize it out, I don't expect that it would be a > > performance problem in this context, so it's probably still fine to solve the > > problem. But I just wanted to clarify what I meant here. > > For the most recent patch that sent out, we really needed to copy out > the whole structure since we're then passing it to ext2fs library > functions. I agree that it's not likely going to be a performance > problem, and at this point, I'm more concerned about code clarity and > correctness. > > Especially since apparently the problems which Harshad's change and my > most recent commit addressed were not picked up by UBSAN (either using > gcc or clang), --- and IMHO they really should have. So we can't > count on UBSAN to find all possible alignment problems. > > Lesson learned, before I do future releases, I should do a build and > "make check" on a armhf chroot running on a arm-64 machine, as well as > on a sparc64 machine, since these seem to be the most sensitive to > alignment issues. And if I miss anything, fortunately Debian's > autobuilders on a large cross-section of architectures will catch them > since we run the regression test suite as part of the package build. > > - Ted > > P.S. Harshad, could you prepare patches to kernel files in ext4 and > jbd2 to make similar alignment portability fixes? Thanks!! Sure, I'll take care of that, thanks! - Harshad
