Received-SPF: pass (google.com: domain of linux-ext4+bounces-1567-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 8 Mar 2024 12:49:13 +0530
From: Ojaswin Mujoo <ojaswin@linux.ibm.com>
To: Dave Chinner <david@fromorbit.com>
Cc: "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>,
        linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org,
        Jan Kara <jack@suse.cz>, "Theodore Ts'o" <tytso@mit.edu>,
        Matthew Wilcox <willy@infradead.org>,
        "Darrick J . Wong" <djwong@kernel.org>,
        Luis Chamberlain <mcgrof@kernel.org>,
        John Garry <john.g.garry@oracle.com>, linux-kernel@vger.kernel.org
Subject: Re: [RFC 2/8] fs: Reserve inode flag FS_ATOMICWRITES_FL for atomic
 writes
Message-ID: <Zeq78Ud5AJ+w2Atj@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
References: <555cc3e262efa77ee5648196362f415a1efc018d.1709361537.git.ritesh.list@gmail.com>
 <4c687c1c5322b4eaf0bb173f0b5d58b38fdaa847.1709361537.git.ritesh.list@gmail.com>
 <ZeUc1ipKMrh+pOn6@dread.disaster.area>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZeUc1ipKMrh+pOn6@dread.disaster.area>

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