Message-ID: <bdad6bae-3baf-41de-9359-39024dba3268@oracle.com>
Date: Tue, 23 Jul 2024 11:11:28 +0100
From: John Garry <john.g.garry@...cle.com>
To: Dave Chinner <david@...morbit.com>, "Darrick J. Wong" <djwong@...nel.org>
Cc: chandan.babu@...cle.com, dchinner@...hat.com, hch@....de,
        viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz,
        linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, catherine.hoang@...cle.com,
        martin.petersen@...cle.com
Subject: Re: [PATCH v2 07/13] xfs: Introduce FORCEALIGN inode flag

On 18/07/2024 09:53, John Garry wrote:
> On 12/07/2024 00:20, Dave Chinner wrote:
>>>> /* Reflink'ed disallowed */
>>>> +    if (flags2 & XFS_DIFLAG2_REFLINK)
>>>> +        return __this_address;
>>> Hmm.  If we don't support reflink + forcealign ATM, then shouldn't the
>>> superblock verifier or xfs_fs_fill_super fail the mount so that old
>>> kernels won't abruptly emit EFSCORRUPTED errors if a future kernel adds
>>> support for forcealign'd cow and starts writing out files with both
>>> iflags set?
>> I don't think we should error out the mount because reflink and
>> forcealign are enabled - that's going to be the common configuration
>> for every user of forcealign, right? I also don't think we should
>> throw a corruption error if both flags are set, either.
>>
>> We're making an initial*implementation choice*  not to implement the
>> two features on the same inode at the same time. We are not making a
>> an on-disk format design decision that says "these two on-disk flags
>> are incompatible".
>>
>> IOWs, if both are set on a current kernel, it's not corruption but a
>> more recent kernel that supports both flags has modified this inode.
>> Put simply, we have detected a ro-compat situation for this specific
>> inode.
>>
>> Looking at it as a ro-compat situation rather then corruption,
>> what I would suggest we do is this:
>>
>> 1. Warn at mount that reflink+force align inodes will be treated
>> as ro-compat inodes. i.e. read-only.

I am looking at something like this to implement read-only for those inodes:

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 07f736c42460..444a44ccc11c 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1132,6 +1132,17 @@ xfs_vn_tmpfile(
  	return finish_open_simple(file, err);
  }

+static int xfs_permission(struct mnt_idmap *d, struct inode *inode, int 
mask)
+{
+	struct xfs_inode	*ip = XFS_I(inode);
+
+	if (mask & MAY_WRITE) {
+		if (xfs_is_reflink_inode(ip) && xfs_inode_has_forcealign(ip))
+			return -EACCES;
+	}
+	return generic_permission(d, inode, mask);
+}
+
  static const struct inode_operations xfs_inode_operations = {
  	.get_inode_acl		= xfs_get_acl,
  	.set_acl		= xfs_set_acl,
@@ -1142,6 +1153,7 @@ static const struct inode_operations 
xfs_inode_operations = {
  	.update_time		= xfs_vn_update_time,
  	.fileattr_get		= xfs_fileattr_get,
  	.fileattr_set		= xfs_fileattr_set,
+	.permission		= xfs_permission,
  };

Or how else could this be done? I guess that we have something else in 
the xfs code to implement the equivalent of this, but I did not find it.

>>
>> 2. prevent forcealign from being set if the shared extent flag is
>> set on the inode.

This is just XFS_DIFLAG2_REFLINK flag, right?

>>
>> 3. prevent shared extents from being created if the force align flag
>> is set (i.e. ->remap_file_range() and anything else that relies on
>> shared extents will fail on forcealign inodes).

In this series version I extend the RT check in xfs_reflink_remap_prep() 
to cover forcealign - is that good enough?

>>
>> 4. if we read an inode with both set, we emit a warning and force
>> the inode to be read only so we don't screw up the force alignment
>> of the file (i.e. that inode operates in ro-compat mode.)
>>
>> #1 is the mount time warning of potential ro-compat behaviour.
>>
>> #2 and #3 prevent both from getting set on existing kernels.
>>
>> #4 is the ro-compat behaviour that would occur from taking a
>> filesystem that ran on a newer kernel that supports force-align+COW.
>> This avoids corruption shutdowns and modifications that would screw
>> up the alignment of the shared and COW'd extents.
>>
> 
> This seems fine for dealing with forcealign and reflink.
> 
> So what about forcealign and RT?

Any opinion on this?

> 
> We want to support this config in future, but the current implementation 
> will not support it.
> 
> In this v2 series, I just disallow a mount for forcealign and RT, 
> similar to reflink and RT together.
> 
> Furthermore, I am also saying here that still forcealign and RT bits set 
> is a valid inode on-disk format and we just have to enforce a 
> sb_rextsize to extsize relationship:
> 
> xfs_inode_validate_forcealign(
>      struct xfs_mount    *mp,
>      uint32_t        extsize,
>      uint32_t        cowextsize,
>      uint16_t        mode,
>      uint16_t        flags,
>      uint64_t        flags2)
> {
>      bool            rt =  flags & XFS_DIFLAG_REALTIME;
> ...
> 
> 
>      /* extsize must be a multiple of sb_rextsize for RT */
>      if (rt && mp->m_sb.sb_rextsize && extsize % mp->m_sb.sb_rextsize)
>          return __this_address;

And this? If not, I'll just send v3 with this code as-is.

> 
>      return NULL;
> }
> 

Powered by blists - more mailing lists