Message-ID: <20240724000411.GV612460@frogsfrogsfrogs>
Date: Tue, 23 Jul 2024 17:04:11 -0700
From: "Darrick J. Wong" <djwong@...nel.org>
To: Dave Chinner <david@...morbit.com>
Cc: John Garry <john.g.garry@...cle.com>, 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 Wed, Jul 24, 2024 at 09:38:18AM +1000, Dave Chinner wrote:
> On Thu, Jul 18, 2024 at 09:53:14AM +0100, 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.
> > > 
> > > 2. prevent forcealign from being set if the shared extent flag is
> > > set on the inode.
> > > 
> > > 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).
> > > 
> > > 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?
> > 
> > We want to support this config in future, but the current implementation
> > will not support it.
> 
> What's the problem with supporting it right from the start? We
> already support forcealign for RT, just it's a global config 
> under the "big rt alloc" moniker rather than a per-inode flag.
> 
> Like all on-disk format change based features,
> forcealign should add the EXPERIMENTAL flag to the filesystem for a
> couple of releases after merge, so there will be plenty of time to
> test both data and rt dev functionality before removing the
> EXPERIMENTAL flag from it.
> 
> So why not just enable the per-inode flag with RT right from the
> start given that this functionality is supposed to work and be
> globally supported by the rtdev right now? It seems like a whole lot
> less work to just enable it for RT now than it is to disable it...

What needs to be done to the rt allocator, anyway?

I think it's mostly turning off the fallback to unaligned allocation,
just like what was done for the data device allocator, right?  And
possibly tweaking whatever this does:

	/*
	 * Only bother calculating a real prod factor if offset & length are
	 * perfectly aligned, otherwise it will just get us in trouble.
	 */
	div_u64_rem(ap->offset, align, &mod);
	if (mod || ap->length % align) {
		prod = 1;
	} else {
		prod = xfs_extlen_to_rtxlen(mp, align);
		if (prod > 1)
			xfs_rtalloc_align_minmax(&raminlen, &ralen, &prod);
	}


> > 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;
> > 
> > 	return NULL;
> > }
> 
> I suspect the logic needs tweaking, but why not just do this right
> from the start?

Do we even allow (i_extsize % mp->m_sb.sb_rextsize) != 0 for realtime
files?  I didn't think we did.

--D

> 
> -Dave.
> -- 
> Dave Chinner
> david@...morbit.com
> 

Powered by blists - more mailing lists