On Tue, Jun 12, 2018 at 7:31 PM, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > On Tue, Jun 12, 2018 at 7:38 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: >> >> That's what I suspected. However, if using 1-byte access for i_write_hint >> is non controversial(?) then by re-arranging i_write_hint, I get the 4-byte >> hole that I need to move i_generation into (in place of i_write_hint): >> >> unsigned short i_bytes; >> + u8 i_write_hint; >> unsigned int i_blkbits; >> - enum rw_hint i_write_hint; >> blkcnt_t i_blocks; > > Yeah, we shouldn't use 'enum' in structs anyway, since the size isn't > well-defined. It depends on compiler flags etc. > > But using "__attribute__ ((__packed__))" for an enum like this is > actually fine. Then you still get the type checking (not that there's > much of it for enums, really), but I think gcc will act as if > "-fshort-enums" was passed for that one case. > > In this case, I think "u8" is fine. And yes, byte accesses could be a > cycle longer due to extra needed zero extension (or masking/shifting, > for crazy architectures), but honestly, we'll never see that. Making > 'struct inode' smaller is likely a bigger win (even if it might not be > very noticeable _either, since allocation issues etc might make it not > very noticeable). > Just to make sure I understand correctly, do you agree with Jan w.r.t original patch that we should leave i_blkbits int? Or do you think that shaving 8 bytes off of struct inode for both 64bit and 32bit arch is a bigger win? If latter, then I guess you can just take the patch as is. Thanks, Amir.
