diff mbox series

[RFC,2/8] fs: Reserve inode flag FS_ATOMICWRITES_FL for atomic writes

Message ID 4c687c1c5322b4eaf0bb173f0b5d58b38fdaa847.1709361537.git.ritesh.list@gmail.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Ritesh Harjani (IBM) March 2, 2024, 7:41 a.m. UTC
This reserves FS_ATOMICWRITES_FL for flags and adds support in
fileattr to support atomic writes flag & xflag needed for ext4
and xfs.

Co-developed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 fs/ioctl.c               | 4 ++++
 include/linux/fileattr.h | 4 ++--
 include/uapi/linux/fs.h  | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

Comments

Dave Chinner March 4, 2024, 12:59 a.m. UTC | #1
On Sat, Mar 02, 2024 at 01:11:59PM +0530, Ritesh Harjani (IBM) wrote:
> This reserves FS_ATOMICWRITES_FL for flags and adds support in
> fileattr to support atomic writes flag & xflag needed for ext4
> and xfs.
> 
> Co-developed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>  fs/ioctl.c               | 4 ++++
>  include/linux/fileattr.h | 4 ++--
>  include/uapi/linux/fs.h  | 1 +
>  3 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 76cf22ac97d7..e0f7fae4777e 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -481,6 +481,8 @@ void fileattr_fill_xflags(struct fileattr *fa, u32 xflags)
>  		fa->flags |= FS_DAX_FL;
>  	if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
>  		fa->flags |= FS_PROJINHERIT_FL;
> +	if (fa->fsx_xflags & FS_XFLAG_ATOMICWRITES)
> +		fa->flags |= FS_ATOMICWRITES_FL;
>  }
>  EXPORT_SYMBOL(fileattr_fill_xflags);
>  
> @@ -511,6 +513,8 @@ void fileattr_fill_flags(struct fileattr *fa, u32 flags)
>  		fa->fsx_xflags |= FS_XFLAG_DAX;
>  	if (fa->flags & FS_PROJINHERIT_FL)
>  		fa->fsx_xflags |= FS_XFLAG_PROJINHERIT;
> +	if (fa->flags & FS_ATOMICWRITES_FL)
> +		fa->fsx_xflags |= FS_XFLAG_ATOMICWRITES;
>  }
>  EXPORT_SYMBOL(fileattr_fill_flags);
>  
> diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
> index 47c05a9851d0..ae9329afa46b 100644
> --- a/include/linux/fileattr.h
> +++ b/include/linux/fileattr.h
> @@ -7,12 +7,12 @@
>  #define FS_COMMON_FL \
>  	(FS_SYNC_FL | FS_IMMUTABLE_FL | FS_APPEND_FL | \
>  	 FS_NODUMP_FL |	FS_NOATIME_FL | FS_DAX_FL | \
> -	 FS_PROJINHERIT_FL)
> +	 FS_PROJINHERIT_FL | FS_ATOMICWRITES_FL)
>  
>  #define FS_XFLAG_COMMON \
>  	(FS_XFLAG_SYNC | FS_XFLAG_IMMUTABLE | FS_XFLAG_APPEND | \
>  	 FS_XFLAG_NODUMP | FS_XFLAG_NOATIME | FS_XFLAG_DAX | \
> -	 FS_XFLAG_PROJINHERIT)
> +	 FS_XFLAG_PROJINHERIT | FS_XFLAG_ATOMICWRITES)

I'd much prefer that we only use a single user API to set/clear this
flag.

This functionality is going to be tied to using extent size hints on
XFS to indicate preferred atomic IO alignment/size, so applications
are going to have to use the FS_IOC_FS{G,S}ETXATTR APIs regardless
of whether it's added to the FS_IOC_{G,S}ETFLAGS API.

Also, there are relatively few flags left in the SETFLAGS 32-bit
space, so this duplication seems like a waste of the few flags
that are remaining.

-Dave.
Ojaswin Mujoo March 8, 2024, 7:19 a.m. UTC | #2
On Mon, Mar 04, 2024 at 11:59:02AM +1100, Dave Chinner wrote:
> On Sat, Mar 02, 2024 at 01:11:59PM +0530, Ritesh Harjani (IBM) wrote:
> > This reserves FS_ATOMICWRITES_FL for flags and adds support in
> > fileattr to support atomic writes flag & xflag needed for ext4
> > and xfs.
> > 
> > Co-developed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > Signed-off-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>
> > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> > ---
> >  fs/ioctl.c               | 4 ++++
> >  include/linux/fileattr.h | 4 ++--
> >  include/uapi/linux/fs.h  | 1 +
> >  3 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 76cf22ac97d7..e0f7fae4777e 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -481,6 +481,8 @@ void fileattr_fill_xflags(struct fileattr *fa, u32 xflags)
> >  		fa->flags |= FS_DAX_FL;
> >  	if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
> >  		fa->flags |= FS_PROJINHERIT_FL;
> > +	if (fa->fsx_xflags & FS_XFLAG_ATOMICWRITES)
> > +		fa->flags |= FS_ATOMICWRITES_FL;
> >  }
> >  EXPORT_SYMBOL(fileattr_fill_xflags);
> >  
> > @@ -511,6 +513,8 @@ void fileattr_fill_flags(struct fileattr *fa, u32 flags)
> >  		fa->fsx_xflags |= FS_XFLAG_DAX;
> >  	if (fa->flags & FS_PROJINHERIT_FL)
> >  		fa->fsx_xflags |= FS_XFLAG_PROJINHERIT;
> > +	if (fa->flags & FS_ATOMICWRITES_FL)
> > +		fa->fsx_xflags |= FS_XFLAG_ATOMICWRITES;
> >  }
> >  EXPORT_SYMBOL(fileattr_fill_flags);
> >  
> > diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
> > index 47c05a9851d0..ae9329afa46b 100644
> > --- a/include/linux/fileattr.h
> > +++ b/include/linux/fileattr.h
> > @@ -7,12 +7,12 @@
> >  #define FS_COMMON_FL \
> >  	(FS_SYNC_FL | FS_IMMUTABLE_FL | FS_APPEND_FL | \
> >  	 FS_NODUMP_FL |	FS_NOATIME_FL | FS_DAX_FL | \
> > -	 FS_PROJINHERIT_FL)
> > +	 FS_PROJINHERIT_FL | FS_ATOMICWRITES_FL)
> >  
> >  #define FS_XFLAG_COMMON \
> >  	(FS_XFLAG_SYNC | FS_XFLAG_IMMUTABLE | FS_XFLAG_APPEND | \
> >  	 FS_XFLAG_NODUMP | FS_XFLAG_NOATIME | FS_XFLAG_DAX | \
> > -	 FS_XFLAG_PROJINHERIT)
> > +	 FS_XFLAG_PROJINHERIT | FS_XFLAG_ATOMICWRITES)
> 
> I'd much prefer that we only use a single user API to set/clear this
> flag.

Hi Dave,

So right now we have 2 ways to mark this flag in ext4:

1. SETFLAGS ioctl() w/ FS_ATOMICWRITES_FL -> set EXT4_ATOMICWRITES_FL on inode
2. SETXFLAGS ioctl() w/ FS_XFLAG_ATOMICWRITES -> translate to FS_ATOMICWRITES_FL -> set EXT4_ATOMICWRITES_FL on inode

IIUC you want to only keep 2. and not support 1. so the user space only
has a single ioctl to use, correct?

One thing I see is that the ext4_fileattr_set() is not XFLAGS aware
at all and right now it expects the XFLAGS to already be translated to 
SETFLAG equivalent before setting it in the inode. Maybe we'll need
to add that logic however it'll be more of an exception than the usual 
pattern.

> 
> This functionality is going to be tied to using extent size hints on
> XFS to indicate preferred atomic IO alignment/size, so applications
> are going to have to use the FS_IOC_FS{G,S}ETXATTR APIs regardless
> of whether it's added to the FS_IOC_{G,S}ETFLAGS API.

Hmm that's right, I'm not sure how we'll handle it in ext4 yet since we
don't have a per file extent size hint, the closest we have is bigalloc
that is more of an mkfs time, FS wide feature. 

Regards,
ojasw
> 
> Also, there are relatively few flags left in the SETFLAGS 32-bit
> space, so this duplication seems like a waste of the few flags
> that are remaining.

> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
diff mbox series

Patch

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 76cf22ac97d7..e0f7fae4777e 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -481,6 +481,8 @@  void fileattr_fill_xflags(struct fileattr *fa, u32 xflags)
 		fa->flags |= FS_DAX_FL;
 	if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
 		fa->flags |= FS_PROJINHERIT_FL;
+	if (fa->fsx_xflags & FS_XFLAG_ATOMICWRITES)
+		fa->flags |= FS_ATOMICWRITES_FL;
 }
 EXPORT_SYMBOL(fileattr_fill_xflags);
 
@@ -511,6 +513,8 @@  void fileattr_fill_flags(struct fileattr *fa, u32 flags)
 		fa->fsx_xflags |= FS_XFLAG_DAX;
 	if (fa->flags & FS_PROJINHERIT_FL)
 		fa->fsx_xflags |= FS_XFLAG_PROJINHERIT;
+	if (fa->flags & FS_ATOMICWRITES_FL)
+		fa->fsx_xflags |= FS_XFLAG_ATOMICWRITES;
 }
 EXPORT_SYMBOL(fileattr_fill_flags);
 
diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
index 47c05a9851d0..ae9329afa46b 100644
--- a/include/linux/fileattr.h
+++ b/include/linux/fileattr.h
@@ -7,12 +7,12 @@ 
 #define FS_COMMON_FL \
 	(FS_SYNC_FL | FS_IMMUTABLE_FL | FS_APPEND_FL | \
 	 FS_NODUMP_FL |	FS_NOATIME_FL | FS_DAX_FL | \
-	 FS_PROJINHERIT_FL)
+	 FS_PROJINHERIT_FL | FS_ATOMICWRITES_FL)
 
 #define FS_XFLAG_COMMON \
 	(FS_XFLAG_SYNC | FS_XFLAG_IMMUTABLE | FS_XFLAG_APPEND | \
 	 FS_XFLAG_NODUMP | FS_XFLAG_NOATIME | FS_XFLAG_DAX | \
-	 FS_XFLAG_PROJINHERIT)
+	 FS_XFLAG_PROJINHERIT | FS_XFLAG_ATOMICWRITES)
 
 /*
  * Merged interface for miscellaneous file attributes.  'flags' originates from
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index b5b4e1db9576..17f52530f9c8 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -264,6 +264,7 @@  struct fsxattr {
 #define FS_EA_INODE_FL			0x00200000 /* Inode used for large EA */
 #define FS_EOFBLOCKS_FL			0x00400000 /* Reserved for ext4 */
 #define FS_NOCOW_FL			0x00800000 /* Do not cow file */
+#define FS_ATOMICWRITES_FL		0x01000000 /* Inode supports atomic writes */
 #define FS_DAX_FL			0x02000000 /* Inode is DAX */
 #define FS_INLINE_DATA_FL		0x10000000 /* Reserved for ext4 */
 #define FS_PROJINHERIT_FL		0x20000000 /* Create with parents projid */