Re: [PATCH v2 11/14] fs: xfs: Support FS_XFLAG_ATOMICWRITES for forcealign

[John Garry]

> >   #define XFS_SB_FEAT_RO_COMPAT_ALL \
> >   		(XFS_SB_FEAT_RO_COMPAT_FINOBT | \
> >   		 XFS_SB_FEAT_RO_COMPAT_RMAPBT | \
> >   		 XFS_SB_FEAT_RO_COMPAT_REFLINK| \
> > -		 XFS_SB_FEAT_RO_COMPAT_INOBTCNT | \
> > -		 XFS_SB_FEAT_RO_COMPAT_FORCEALIGN)
> > +		 XFS_SB_FEAT_RO_COMPAT_INOBTCNT| \
> > +		 XFS_SB_FEAT_RO_COMPAT_FORCEALIGN| \
>
> Please leave a spave between the feature name and the '| \'.

ok

> > +		 XFS_SB_FEAT_RO_COMPAT_ATOMICWRITES)
> > +

..

> >   }
> >   
> > +static inline bool xfs_inode_atomicwrites(struct xfs_inode *ip)
> > +{
> > +	return ip->i_diflags2 & XFS_DIFLAG2_ATOMICWRITES;
> > +}
>
> I'd really like this to be more readable:
> xfs_inode_has_atomic_writes().
>
> Same for the force align check, now that I notice it:
> xfs_inode_has_force_align().

ok, will change

> > +
> >   /*
> >    * Return the buftarg used for data allocations on a given inode.
> >    */
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 867d8d51a3d0..f118a1ae39b5 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1112,6 +1112,8 @@ xfs_flags2diflags2(
> >   		di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
> >   	if (xflags & FS_XFLAG_FORCEALIGN)
> >   		di_flags2 |= XFS_DIFLAG2_FORCEALIGN;
> > +	if (xflags & FS_XFLAG_ATOMICWRITES)
> > +		di_flags2 |= XFS_DIFLAG2_ATOMICWRITES;
> >   
> >   	return di_flags2;
> >   }
> > @@ -1124,10 +1126,12 @@ xfs_ioctl_setattr_xflags(
> >   {
> >   	struct xfs_mount	*mp = ip->i_mount;
> >   	bool			rtflag = (fa->fsx_xflags & FS_XFLAG_REALTIME);
> > +	bool			atomic_writes = fa->fsx_xflags & FS_XFLAG_ATOMICWRITES;
> >   	uint64_t		i_flags2;
> >   
> > -	if (rtflag != XFS_IS_REALTIME_INODE(ip)) {
> > -		/* Can't change realtime flag if any extents are allocated. */
> > +	/* Can't change RT or atomic flags if any extents are allocated. */
> > +	if (rtflag != XFS_IS_REALTIME_INODE(ip) ||
> > +	    atomic_writes != xfs_inode_atomicwrites(ip)) {
> >   		if (ip->i_df.if_nextents || ip->i_delayed_blks)
> >   			return -EINVAL;
> >   	}
> > @@ -1164,6 +1168,13 @@ xfs_ioctl_setattr_xflags(
> >   			return -EINVAL;
> >   	}
> >   
> > +	if (atomic_writes) {
> > +		if (!xfs_has_atomicwrites(mp))
> > +			return -EINVAL;
>
> That looks wrong - if we are trying to turn on atomic writes, then
> shouldn't this be returning an error if atomic writes are already
> configured?

I think that you are talking about a xfs_inode_atomicwrites() check.


> > +		if (!(fa->fsx_xflags & FS_XFLAG_FORCEALIGN))
> > +			return -EINVAL;

> Where's the check for xfs_has_atomicwrites(mp) here? 

please see above

> We can't allow
> this inode flag to be set if the superblock does not have the
> feature bit that says it's a known feature bit set.
>
> Which reminds me: both the forcealign and the atomicwrite inode flag
> need explicit checking in the inode verifier. i.e. checking that if
> the inode flag bit is set, the relevant superblock feature bit is
> set.

We do have that in the xfs_has_atomicwrites() and xfs_has_forcealign() 
checks - is that ok?

> ....
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 74dcafddf6a9..efe4b4234b2e 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1712,6 +1712,10 @@ xfs_fs_fill_super(
> >   		xfs_warn(mp,
> >   "EXPERIMENTAL forced data extent alignment feature in use. Use at your own risk!");
> >   
> > +	if (xfs_has_atomicwrites(mp))
> > +		xfs_warn(mp,
> > +"EXPERIMENTAL atomicwrites feature in use. Use at your own risk!");
>
> "EXPERIMENTAL atomic write IO feature is in use. Use at your own risk!");

ok

Thanks,
John