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

[John Garry <john.g.garry@oracle.com>]

>>   #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