diff mbox series

[v2,04/16] fs: Increase fmode_t size

Message ID 20231212110844.19698-5-john.g.garry@oracle.com (mailing list archive)
State Not Applicable
Headers show
Series block atomic writes | expand

Commit Message

John Garry Dec. 12, 2023, 11:08 a.m. UTC
Currently all bits are being used in fmode_t.

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>
---
 drivers/dma-buf/dma-buf.c | 2 +-
 include/linux/types.h     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Kara Dec. 13, 2023, 11:20 a.m. UTC | #1
On Tue 12-12-23 11:08:32, John Garry wrote:
> Currently all bits are being used in fmode_t.
> 
> 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. 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). 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?

								Honza

> ---
>  drivers/dma-buf/dma-buf.c | 2 +-
>  include/linux/types.h     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 21916bba77d5..a5227ae3d637 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -1628,7 +1628,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused)
>  
>  
>  		spin_lock(&buf_obj->name_lock);
> -		seq_printf(s, "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n",
> +		seq_printf(s, "%08zu\t%08x\t%08lx\t%08ld\t%s\t%08lu\t%s\n",
>  				buf_obj->size,
>  				buf_obj->file->f_flags, buf_obj->file->f_mode,
>  				file_count(buf_obj->file),
> diff --git a/include/linux/types.h b/include/linux/types.h
> index 253168bb3fe1..49c754fde1d6 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -153,7 +153,7 @@ typedef u32 dma_addr_t;
>  
>  typedef unsigned int __bitwise gfp_t;
>  typedef unsigned int __bitwise slab_flags_t;
> -typedef unsigned int __bitwise fmode_t;
> +typedef unsigned long __bitwise fmode_t;
>  
>  #ifdef CONFIG_PHYS_ADDR_T_64BIT
>  typedef u64 phys_addr_t;
> -- 
> 2.35.3
>
Christian Brauner Dec. 13, 2023, 1:02 p.m. UTC | #2
On Tue, Dec 12, 2023 at 11:08:32AM +0000, John Garry wrote:
> Currently all bits are being used in fmode_t.
> 
> 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>
> ---
>  drivers/dma-buf/dma-buf.c | 2 +-
>  include/linux/types.h     | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 21916bba77d5..a5227ae3d637 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -1628,7 +1628,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused)
>  
>  
>  		spin_lock(&buf_obj->name_lock);
> -		seq_printf(s, "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n",
> +		seq_printf(s, "%08zu\t%08x\t%08lx\t%08ld\t%s\t%08lu\t%s\n",
>  				buf_obj->size,
>  				buf_obj->file->f_flags, buf_obj->file->f_mode,
>  				file_count(buf_obj->file),
> diff --git a/include/linux/types.h b/include/linux/types.h
> index 253168bb3fe1..49c754fde1d6 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -153,7 +153,7 @@ typedef u32 dma_addr_t;
>  
>  typedef unsigned int __bitwise gfp_t;
>  typedef unsigned int __bitwise slab_flags_t;
> -typedef unsigned int __bitwise fmode_t;
> +typedef unsigned long __bitwise fmode_t;

As Jan said, that's likely a bad idea. There's a bunch of places that
assume fmode_t is 32bit. So not really a change we want to make if we
can avoid it.
John Garry Dec. 13, 2023, 1:03 p.m. UTC | #3
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
John Garry Dec. 13, 2023, 1:15 p.m. UTC | #4
On 13/12/2023 13:02, Christian Brauner wrote:
> On Tue, Dec 12, 2023 at 11:08:32AM +0000, John Garry wrote:
>> Currently all bits are being used in fmode_t.
>>
>> 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>
>> ---
>>   drivers/dma-buf/dma-buf.c | 2 +-
>>   include/linux/types.h     | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index 21916bba77d5..a5227ae3d637 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -1628,7 +1628,7 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused)
>>   
>>   
>>   		spin_lock(&buf_obj->name_lock);
>> -		seq_printf(s, "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n",
>> +		seq_printf(s, "%08zu\t%08x\t%08lx\t%08ld\t%s\t%08lu\t%s\n",
>>   				buf_obj->size,
>>   				buf_obj->file->f_flags, buf_obj->file->f_mode,
>>   				file_count(buf_obj->file),
>> diff --git a/include/linux/types.h b/include/linux/types.h
>> index 253168bb3fe1..49c754fde1d6 100644
>> --- a/include/linux/types.h
>> +++ b/include/linux/types.h
>> @@ -153,7 +153,7 @@ typedef u32 dma_addr_t;
>>   
>>   typedef unsigned int __bitwise gfp_t;
>>   typedef unsigned int __bitwise slab_flags_t;
>> -typedef unsigned int __bitwise fmode_t;
>> +typedef unsigned long __bitwise fmode_t;
> 
> As Jan said, that's likely a bad idea. There's a bunch of places that
> assume fmode_t is 32bit. So not really a change we want to make if we
> can avoid it.

ok, understood.

Some strictly unnecessary bits in f_mode could be recycled (if there 
were any), but this issue will prob come up again.

Could it be considered to add an extended fmode_t member in struct file?

Thanks,
John
Christoph Hellwig Dec. 13, 2023, 4:03 p.m. UTC | #5
On Wed, Dec 13, 2023 at 02:02:31PM +0100, Christian Brauner wrote:
> >  typedef unsigned int __bitwise gfp_t;
> >  typedef unsigned int __bitwise slab_flags_t;
> > -typedef unsigned int __bitwise fmode_t;
> > +typedef unsigned long __bitwise fmode_t;
> 
> As Jan said, that's likely a bad idea. There's a bunch of places that
> assume fmode_t is 32bit. So not really a change we want to make if we
> can avoid it.

Oh well, let me dust of my series to move the fairly static flags out
of it.  But even without that do we even need to increase it?  There's
still quite a lot of space after FMODE_EXEC for example.
John Garry Dec. 14, 2023, 8:56 a.m. UTC | #6
> But even without that do we even need to increase it?  There's
> still quite a lot of space after FMODE_EXEC for example.

Right, I can use the space after FMODE_EXEC, which came free after 
removal of FMODE_NDELAY, FMODE_EXCL, and FMODE_WRITE_IOCTL in v6.5.

Thanks,
John
diff mbox series

Patch

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 21916bba77d5..a5227ae3d637 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1628,7 +1628,7 @@  static int dma_buf_debug_show(struct seq_file *s, void *unused)
 
 
 		spin_lock(&buf_obj->name_lock);
-		seq_printf(s, "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n",
+		seq_printf(s, "%08zu\t%08x\t%08lx\t%08ld\t%s\t%08lu\t%s\n",
 				buf_obj->size,
 				buf_obj->file->f_flags, buf_obj->file->f_mode,
 				file_count(buf_obj->file),
diff --git a/include/linux/types.h b/include/linux/types.h
index 253168bb3fe1..49c754fde1d6 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -153,7 +153,7 @@  typedef u32 dma_addr_t;
 
 typedef unsigned int __bitwise gfp_t;
 typedef unsigned int __bitwise slab_flags_t;
-typedef unsigned int __bitwise fmode_t;
+typedef unsigned long __bitwise fmode_t;
 
 #ifdef CONFIG_PHYS_ADDR_T_64BIT
 typedef u64 phys_addr_t;