Re: [PATCH] [RFC]: fs: claw back a few FMODE_* bits

[Dave Chinner <david@fromorbit.com>]

On Thu, Mar 28, 2024 at 09:06:43AM +0100, Christian Brauner wrote:
> On Thu, Mar 28, 2024 at 12:18:06PM +1100, Dave Chinner wrote:
> > On Wed, Mar 27, 2024 at 05:45:09PM +0100, Christian Brauner wrote:
> > > There's a bunch of flags that are purely based on what the file
> > > operations support while also never being conditionally set or unset.
> > > IOW, they're not subject to change for individual file opens. Imho, such
> > > flags don't need to live in f_mode they might as well live in the fops
> > > structs itself. And the fops struct already has that lonely
> > > mmap_supported_flags member. We might as well turn that into a generic
> > > fops_flags member and move a few flags from FMODE_* space into FOP_*
> > > space. That gets us four FMODE_* bits back and the ability for new
> > > static flags that are about file ops to not have to live in FMODE_*
> > > space but in their own FOP_* space. It's not the most beautiful thing
> > > ever but it gets the job done. Yes, there'll be an additional pointer
> > > chase but hopefully that won't matter for these flags.
> > > 
> > > If this is palatable I suspect there's a few more we can move into there
> > > and that we can also redirect new flag suggestions that follow this
> > > pattern into the fops_flags field instead of f_mode. As of yet untested.
> > > 
> > > (Fwiw, FMODE_NOACCOUNT and FMODE_BACKING could live in fops_flags as
> > >  well because they're also completely static but they aren't really
> > >  about file operations so they're better suited for FMODE_* imho.)
> > > 
> > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > .....
> > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > index 632653e00906..d13e21eb9a3c 100644
> > > --- a/fs/xfs/xfs_file.c
> > > +++ b/fs/xfs/xfs_file.c
> > > @@ -1230,8 +1230,7 @@ xfs_file_open(
> > >  {
> > >  	if (xfs_is_shutdown(XFS_M(inode->i_sb)))
> > >  		return -EIO;
> > > -	file->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC | FMODE_BUF_WASYNC |
> > > -			FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT;
> > > +	file->f_mode |= FMODE_NOWAIT | FMODE_CAN_ODIRECT;
> > >  	return generic_file_open(inode, file);
> > >  }
> > >  
> > > @@ -1490,7 +1489,6 @@ const struct file_operations xfs_file_operations = {
> > >  	.compat_ioctl	= xfs_file_compat_ioctl,
> > >  #endif
> > >  	.mmap		= xfs_file_mmap,
> > > -	.mmap_supported_flags = MAP_SYNC,
> > >  	.open		= xfs_file_open,
> > >  	.release	= xfs_file_release,
> > >  	.fsync		= xfs_file_fsync,
> > > @@ -1498,6 +1496,8 @@ const struct file_operations xfs_file_operations = {
> > >  	.fallocate	= xfs_file_fallocate,
> > >  	.fadvise	= xfs_file_fadvise,
> > >  	.remap_file_range = xfs_file_remap_range,
> > > +	.fops_flags	= FOP_MMAP_SYNC | FOP_BUF_RASYNC | FOP_BUF_WASYNC |
> > > +			  FOP_DIO_PARALLEL_WRITE,
> > >  };
> > >  
> > >  const struct file_operations xfs_dir_file_operations = {
> > > @@ -1510,4 +1510,6 @@ const struct file_operations xfs_dir_file_operations = {
> > >  	.compat_ioctl	= xfs_file_compat_ioctl,
> > >  #endif
> > >  	.fsync		= xfs_dir_fsync,
> > > +	.fops_flags	= FOP_MMAP_SYNC | FOP_BUF_RASYNC | FOP_BUF_WASYNC |
> > > +			  FOP_DIO_PARALLEL_WRITE,
> > >  };
> > 
> > Why do we need to set any of these for directory operations now that
> > we have a clear choice? i.e. we can't mmap directories, and the rest
> > of these flags are for read() and write() operations which we also
> > can't do on directories...
> 
> Yeah, I know but since your current implementation raises them for both
> I just did it 1:1:

Sure, that's fine. I asked the question because your patch made the
issue obvious, regardless of the current state of the code.

i.e. a patch adding a flag to xfs_file_open() (e.g.  adding
FMODE_DIO_PARALLEL_WRITE had nothing to do with XFS) does not touch
xfs_dir_open() and so it is not immediately clear from the diff that
it also affects directories as well. So it's not until someone
actually notices that flags only relevant to regular files are being
set on directories that someone comments and it becomes clear we
need to fix this...

As Christoph said, it doesn't need to be fixed in this patch, but we
should address this properly. The next question is this: how many
other filesystem implementations do the same thing?

-Dave.
-- 
Dave Chinner
david@fromorbit.com