Re: [PATCH v2 04/16] fs: Increase fmode_t size

John Garry
2023-12-13T13:04:10Z

On 13/12/2023 11:20, Jan Kara wrote:
>> To allow for further expansion, increase from unsigned int to unsigned
>> long.
>>
>> Since the dma-buf driver prints the file->f_mode member, change the print
>> as necessary to deal with the larger size.
>>
>> Signed-off-by: John Garry<john.g.garry@oracle.com>
> Uh, Al has more experience with fmode_t changes so I'd defer final decision
> to him but to me this seems dangerous.

Ack

> Firstly, this breaks packing of
> struct file on 64-bit architectures and struct file is highly optimized for
> cache efficiency (see the comment before the struct definition).

From pahole, I think that we still fit on the same 64B cacheline
(x86_64), but some padding has been added.

Before:
struct file {
union {
struct llist_node f_llist; /* 0 8 */
struct callback_head f_rcuhead
__attribute__((__aligned__(8))); /* 0 16 */
unsigned int f_iocb_flags; /* 0 4 */
} __attribute__((__aligned__(8))); /* 0 16 */
spinlock_t f_lock; /* 16 4 */
fmode_t f_mode; /* 20 4 */
atomic_long_t f_count; /* 24 8 */
struct mutex f_pos_lock; /* 32 32 */
/* --- cacheline 1 boundary (64 bytes) --- */

After:

struct file {
union {
struct llist_node f_llist /* 0 8 */
struct callback_head f_rcuhead
__attribute__((__aligned__(8))); /* 0 16 */
unsigned int f_iocb_flags; /* 0 4 */
} __attribute__((__aligned__(8))); /* 0 16 */
spinlock_t f_lock; /* 16 4 */

/* XXX 4 bytes hole, try to pack */

fmode_t f_mode; /* 24 8 */
atomic_long_t f_count; /* 32 8 */
struct mutex f_pos_lock; /* 40 32 */
/* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */

> Secondly
> this will probably generate warnings on 32-bit architectures as there
> sizeof(unsigned long) == sizeof(unsigned int) and so your new flags won't
> fit anyway?

Right, it would then need to be unsigned long long. Or add another 32b
member for extended modes. There were no i386 build warnings.

Thanks,
John