| Message ID | 4c687c1c5322b4eaf0bb173f0b5d58b38fdaa847.1709361537.git.ritesh.list@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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.
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 --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 */