[PATCHSET 0/4] mkfs: various bug fixes

[PATCHSET 0/4] mkfs: various bug fixes

[Darrick J. Wong]

Hi all,

This series fixes numerous bugs in mkfs with the validation and stripe
aligning behavior of internal log size computations.  After this, we
don't allow overly large logs to bump the root inode chunk into AG 1,
nor do we allow stripe unit roundup to cause the format to fail.
There's also a fix for mkfs ignoring the gid in the user's protofile
when the parent is setgid.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=mkfs-fixes
---
 include/xfs_inode.h |   11 ++++--
 libxfs/util.c       |    3 +-
 mkfs/proto.c        |    3 +-
 mkfs/xfs_mkfs.c     |  102 ++++++++++++++++++++++++++++++++++++++++-----------
 4 files changed, 91 insertions(+), 28 deletions(-)

[PATCH 1/4] mkfs: fix missing validation of -l size against maximum internal log size

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If a sysadmin specifies a log size explicitly, we don't actually check
that against the maximum internal log size that we compute for the
default log size computation.  We're going to add more validation soon,
so refactor the max internal log blocks into a common variable and
add a check.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |   36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 6d0af86d..e11b39d7 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3275,6 +3275,7 @@ calculate_log_size(
 {
 	struct xfs_sb		*sbp = &mp->m_sb;
 	int			min_logblocks;	/* absolute minimum */
+	int			max_logblocks;	/* absolute max for this AG */
 	struct xfs_mount	mount;
 
 	/* we need a temporary mount to calculate the minimum log size. */
@@ -3314,6 +3315,18 @@ _("external log device size %lld blocks too small, must be at least %lld blocks\
 		return;
 	}
 
+	/*
+	 * Make sure the log fits wholly within an AG
+	 *
+	 * XXX: If agf->freeblks ends up as 0 because the log uses all
+	 * the free space, it causes the kernel all sorts of problems
+	 * with per-ag reservations. Right now just back it off one
+	 * block, but there's a whole can of worms here that needs to be
+	 * opened to decide what is the valid maximum size of a log in
+	 * an AG.
+	 */
+	max_logblocks = libxfs_alloc_ag_max_usable(mp) - 1;
+
 	/* internal log - if no size specified, calculate automatically */
 	if (!cfg->logblocks) {
 		/* Use a 2048:1 fs:log ratio for most filesystems */
@@ -3328,21 +3341,9 @@ _("external log device size %lld blocks too small, must be at least %lld blocks\
 		if (cfg->dblocks < MEGABYTES(300, cfg->blocklog))
 			cfg->logblocks = min_logblocks;
 
-		/* Ensure the chosen size meets minimum log size requirements */
+		/* Ensure the chosen size fits within log size requirements */
 		cfg->logblocks = max(min_logblocks, cfg->logblocks);
-
-		/*
-		 * Make sure the log fits wholly within an AG
-		 *
-		 * XXX: If agf->freeblks ends up as 0 because the log uses all
-		 * the free space, it causes the kernel all sorts of problems
-		 * with per-ag reservations. Right now just back it off one
-		 * block, but there's a whole can of worms here that needs to be
-		 * opened to decide what is the valid maximum size of a log in
-		 * an AG.
-		 */
-		cfg->logblocks = min(cfg->logblocks,
-				     libxfs_alloc_ag_max_usable(mp) - 1);
+		cfg->logblocks = min(cfg->logblocks, max_logblocks);
 
 		/* and now clamp the size to the maximum supported size */
 		cfg->logblocks = min(cfg->logblocks, XFS_MAX_LOG_BLOCKS);
@@ -3350,6 +3351,13 @@ _("external log device size %lld blocks too small, must be at least %lld blocks\
 			cfg->logblocks = XFS_MAX_LOG_BYTES >> cfg->blocklog;
 
 		validate_log_size(cfg->logblocks, cfg->blocklog, min_logblocks);
+	} else if (cfg->logblocks > max_logblocks) {
+		/* check specified log size */
+		fprintf(stderr,
+_("internal log size %lld too large, must be less than %d\n"),
+			(long long)cfg->logblocks,
+			max_logblocks);
+		usage();
 	}
 
 	if (cfg->logblocks > sbp->sb_agblocks - libxfs_prealloc_blocks(mp)) {

Re: [PATCH 1/4] mkfs: fix missing validation of -l size against maximum internal log size

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/4] mkfs: reduce internal log size when log stripe units are in play

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Currently, one can feed mkfs a combination of options like this:

$ truncate -s 6366g /tmp/a ; mkfs.xfs -f /tmp/a -d agcount=3200 -d su=256k,sw=4
meta-data=/tmp/a                 isize=512    agcount=3200, agsize=521536 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=0
         =                       reflink=1    bigtime=0 inobtcount=0
data     =                       bsize=4096   blocks=1668808704, imaxpct=5
         =                       sunit=64     swidth=256 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=521536, version=2
         =                       sectsz=512   sunit=64 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
Metadata corruption detected at 0x55e88052c6b6, xfs_agf block 0x1/0x200
libxfs_bwrite: write verifier failed on xfs_agf bno 0x1/0x1
mkfs.xfs: writing AG headers failed, err=117

The format fails because the internal log size sizing algorithm
specifies a log size of 521492 blocks to avoid taking all the space in
the AG, but align_log_size sees the stripe unit and rounds that up to
the next stripe unit, which is 521536 blocks.

Fix this problem by rounding the log size down if rounding up would
result in a log that consumes more space in the AG than we allow.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index e11b39d7..eb4d7fa9 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3180,9 +3180,10 @@ sb_set_features(
 static void
 align_log_size(
 	struct mkfs_params	*cfg,
-	int			sunit)
+	int			sunit,
+	int			max_logblocks)
 {
-	uint64_t	tmp_logblocks;
+	uint64_t		tmp_logblocks;
 
 	/* nothing to do if it's already aligned. */
 	if ((cfg->logblocks % sunit) == 0)
@@ -3199,7 +3200,8 @@ _("log size %lld is not a multiple of the log stripe unit %d\n"),
 
 	/* If the log is too large, round down instead of round up */
 	if ((tmp_logblocks > XFS_MAX_LOG_BLOCKS) ||
-	    ((tmp_logblocks << cfg->blocklog) > XFS_MAX_LOG_BYTES)) {
+	    ((tmp_logblocks << cfg->blocklog) > XFS_MAX_LOG_BYTES) ||
+	    tmp_logblocks > max_logblocks) {
 		tmp_logblocks = (cfg->logblocks / sunit) * sunit;
 	}
 	cfg->logblocks = tmp_logblocks;
@@ -3213,7 +3215,8 @@ static void
 align_internal_log(
 	struct mkfs_params	*cfg,
 	struct xfs_mount	*mp,
-	int			sunit)
+	int			sunit,
+	int			max_logblocks)
 {
 	uint64_t		logend;
 
@@ -3231,7 +3234,7 @@ _("Due to stripe alignment, the internal log start (%lld) cannot be aligned\n"
 	}
 
 	/* round up/down the log size now */
-	align_log_size(cfg, sunit);
+	align_log_size(cfg, sunit, max_logblocks);
 
 	/* check the aligned log still starts and ends in the same AG. */
 	logend = cfg->logstart + cfg->logblocks - 1;
@@ -3309,7 +3312,7 @@ _("external log device size %lld blocks too small, must be at least %lld blocks\
 		cfg->logstart = 0;
 		cfg->logagno = 0;
 		if (cfg->lsunit)
-			align_log_size(cfg, cfg->lsunit);
+			align_log_size(cfg, cfg->lsunit, XFS_MAX_LOG_BLOCKS);
 
 		validate_log_size(cfg->logblocks, cfg->blocklog, min_logblocks);
 		return;
@@ -3386,9 +3389,9 @@ _("log ag number %lld too large, must be less than %lld\n"),
 	 * Align the logstart at stripe unit boundary.
 	 */
 	if (cfg->lsunit) {
-		align_internal_log(cfg, mp, cfg->lsunit);
+		align_internal_log(cfg, mp, cfg->lsunit, max_logblocks);
 	} else if (cfg->dsunit) {
-		align_internal_log(cfg, mp, cfg->dsunit);
+		align_internal_log(cfg, mp, cfg->dsunit, max_logblocks);
 	}
 	validate_log_size(cfg->logblocks, cfg->blocklog, min_logblocks);
 }

Re: [PATCH 2/4] mkfs: reduce internal log size when log stripe units are in play

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/4] mkfs: don't let internal logs bump the root dir inode chunk to AG 1

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Currently, we don't let an internal log consume every last block in an
AG.  According to the comment, we're doing this to avoid tripping AGF
verifiers if freeblks==0, but on a modern filesystem this isn't
sufficient to avoid problems because we need to have enough space in the
AG to allocate an aligned root inode chunk, if it should be the case
that the log also ends up in AG 0:

$ truncate -s 6366g /tmp/a ; mkfs.xfs -f /tmp/a -d agcount=3200 -l agnum=0
meta-data=/tmp/a                 isize=512    agcount=3200, agsize=521503 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=1        finobt=1, sparse=1, rmapbt=0
         =                       reflink=1    bigtime=0 inobtcount=0
data     =                       bsize=4096   blocks=1668808704, imaxpct=5
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=521492, version=2
         =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0
mkfs.xfs: root inode created in AG 1, not AG 0

Therefore, modify the maximum internal log size calculation to constrain
the maximum internal log size so that the aligned inode chunk allocation
will always succeed.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index eb4d7fa9..0b1fb746 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3270,6 +3270,49 @@ validate_log_size(uint64_t logblocks, int blocklog, int min_logblocks)
 	}
 }
 
+static void
+adjust_ag0_internal_logblocks(
+	struct mkfs_params	*cfg,
+	struct xfs_mount	*mp,
+	int			min_logblocks,
+	int			*max_logblocks)
+{
+	int			backoff = 0;
+	int			ichunk_blocks;
+
+	/*
+	 * mkfs will trip over the write verifiers if the log is allocated in
+	 * AG 0 and consumes enough space that we cannot allocate a non-sparse
+	 * inode chunk for the root directory.  The inode allocator requires
+	 * that the AG have enough free space for the chunk itself plus enough
+	 * to fix up the freelist with aligned blocks if we need to fill the
+	 * allocation from the AGFL.
+	 */
+	ichunk_blocks = XFS_INODES_PER_CHUNK * cfg->inodesize >> cfg->blocklog;
+	backoff = ichunk_blocks * 4;
+
+	/*
+	 * We try to align inode allocations to the data device stripe unit,
+	 * so ensure there's enough space to perform an aligned allocation.
+	 * The inode geometry structure isn't set up yet, so compute this by
+	 * hand.
+	 */
+	backoff = max(backoff, cfg->dsunit * 2);
+
+	*max_logblocks -= backoff;
+
+	/* If the specified log size is too big, complain. */
+	if (cli_opt_set(&lopts, L_SIZE) && cfg->logblocks > *max_logblocks) {
+		fprintf(stderr,
+_("internal log size %lld too large, must be less than %d\n"),
+			(long long)cfg->logblocks,
+			*max_logblocks);
+		usage();
+	}
+
+	cfg->logblocks = min(cfg->logblocks, *max_logblocks);
+}
+
 static void
 calculate_log_size(
 	struct mkfs_params	*cfg,
@@ -3382,6 +3425,10 @@ _("log ag number %lld too large, must be less than %lld\n"),
 	} else
 		cfg->logagno = (xfs_agnumber_t)(sbp->sb_agcount / 2);
 
+	if (cfg->logagno == 0)
+		adjust_ag0_internal_logblocks(cfg, mp, min_logblocks,
+				&max_logblocks);
+
 	cfg->logstart = XFS_AGB_TO_FSB(mp, cfg->logagno,
 				       libxfs_prealloc_blocks(mp));
 

Re: [PATCH 3/4] mkfs: don't let internal logs bump the root dir inode chunk to AG 1

[Christoph Hellwig]

On Thu, Apr 14, 2022 at 11:49:14AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Currently, we don't let an internal log consume every last block in an
> AG.  According to the comment, we're doing this to avoid tripping AGF
> verifiers if freeblks==0, but on a modern filesystem this isn't
> sufficient to avoid problems because we need to have enough space in the
> AG to allocate an aligned root inode chunk, if it should be the case
> that the log also ends up in AG 0:

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 4/4] mkfs: don't trample the gid set in the protofile

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Catherine's recent changes to xfs/019 exposed a bug in how libxfs
handles setgid bits.  mkfs reads the desired gid in from the protofile,
but if the parent directory is setgid, it will override the user's
setting and (re)set the child's gid to the parent's gid.  Overriding
user settings is (probably) not the desired mode of operation, so create
a flag to struct cred to force the gid in the protofile.

It looks like this has been broken since ~2005.

Cc: Catherine Hoang <catherine.hoang@oracle.com>
Fixes: 9f064b7e ("Provide mkfs options to easily exercise all inheritable attributes, esp. the extsize allocator hint. Merge of master-melb:xfs-cmds:24370a by kenmcd.")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 include/xfs_inode.h |   11 +++++++----
 libxfs/util.c       |    3 ++-
 mkfs/proto.c        |    3 ++-
 3 files changed, 11 insertions(+), 6 deletions(-)


diff --git a/include/xfs_inode.h b/include/xfs_inode.h
index 08a62d83..db9faa57 100644
--- a/include/xfs_inode.h
+++ b/include/xfs_inode.h
@@ -164,10 +164,13 @@ static inline bool xfs_inode_has_bigtime(struct xfs_inode *ip)
 	return ip->i_diflags2 & XFS_DIFLAG2_BIGTIME;
 }
 
-typedef struct cred {
-	uid_t	cr_uid;
-	gid_t	cr_gid;
-} cred_t;
+/* Always set the child's GID to this value, even if the parent is setgid. */
+#define CRED_FORCE_GID	(1U << 0)
+struct cred {
+	uid_t		cr_uid;
+	gid_t		cr_gid;
+	unsigned int	cr_flags;
+};
 
 extern int	libxfs_dir_ialloc (struct xfs_trans **, struct xfs_inode *,
 				mode_t, nlink_t, xfs_dev_t, struct cred *,
diff --git a/libxfs/util.c b/libxfs/util.c
index 9f1ca907..655a7bd3 100644
--- a/libxfs/util.c
+++ b/libxfs/util.c
@@ -271,7 +271,8 @@ libxfs_init_new_inode(
 	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG | XFS_ICHGTIME_MOD);
 
 	if (pip && (VFS_I(pip)->i_mode & S_ISGID)) {
-		VFS_I(ip)->i_gid = VFS_I(pip)->i_gid;
+		if (!(cr->cr_flags & CRED_FORCE_GID))
+			VFS_I(ip)->i_gid = VFS_I(pip)->i_gid;
 		if ((VFS_I(pip)->i_mode & S_ISGID) && (mode & S_IFMT) == S_IFDIR)
 			VFS_I(ip)->i_mode |= S_ISGID;
 	}
diff --git a/mkfs/proto.c b/mkfs/proto.c
index ef130ed6..127d87dd 100644
--- a/mkfs/proto.c
+++ b/mkfs/proto.c
@@ -378,7 +378,7 @@ parseproto(
 	xfs_trans_t	*tp;
 	int		val;
 	int		isroot = 0;
-	cred_t		creds;
+	struct cred	creds;
 	char		*value;
 	struct xfs_name	xname;
 
@@ -446,6 +446,7 @@ parseproto(
 	mode |= val;
 	creds.cr_uid = (int)getnum(getstr(pp), 0, 0, false);
 	creds.cr_gid = (int)getnum(getstr(pp), 0, 0, false);
+	creds.cr_flags = CRED_FORCE_GID;
 	xname.name = (unsigned char *)name;
 	xname.len = name ? strlen(name) : 0;
 	xname.type = 0;

Re: [PATCH 4/4] mkfs: don't trample the gid set in the protofile

[Catherine Hoang]

> On Apr 14, 2022, at 11:49 AM, Darrick J. Wong <djwong@kernel.org> wrote:
> 
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Catherine's recent changes to xfs/019 exposed a bug in how libxfs
> handles setgid bits.  mkfs reads the desired gid in from the protofile,
> but if the parent directory is setgid, it will override the user's
> setting and (re)set the child's gid to the parent's gid.  Overriding
> user settings is (probably) not the desired mode of operation, so create
> a flag to struct cred to force the gid in the protofile.
> 
> It looks like this has been broken since ~2005.

Looks good to me
Reviewed-by: Catherine Hoang <catherine.hoang@oracle.com>
> 
> Cc: Catherine Hoang <catherine.hoang@oracle.com>
> Fixes: 9f064b7e ("Provide mkfs options to easily exercise all inheritable attributes, esp. the extsize allocator hint. Merge of master-melb:xfs-cmds:24370a by kenmcd.")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> include/xfs_inode.h |   11 +++++++----
> libxfs/util.c       |    3 ++-
> mkfs/proto.c        |    3 ++-
> 3 files changed, 11 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/include/xfs_inode.h b/include/xfs_inode.h
> index 08a62d83..db9faa57 100644
> --- a/include/xfs_inode.h
> +++ b/include/xfs_inode.h
> @@ -164,10 +164,13 @@ static inline bool xfs_inode_has_bigtime(struct xfs_inode *ip)
> 	return ip->i_diflags2 & XFS_DIFLAG2_BIGTIME;
> }
> 
> -typedef struct cred {
> -	uid_t	cr_uid;
> -	gid_t	cr_gid;
> -} cred_t;
> +/* Always set the child's GID to this value, even if the parent is setgid. */
> +#define CRED_FORCE_GID	(1U << 0)
> +struct cred {
> +	uid_t		cr_uid;
> +	gid_t		cr_gid;
> +	unsigned int	cr_flags;
> +};
> 
> extern int	libxfs_dir_ialloc (struct xfs_trans **, struct xfs_inode *,
> 				mode_t, nlink_t, xfs_dev_t, struct cred *,
> diff --git a/libxfs/util.c b/libxfs/util.c
> index 9f1ca907..655a7bd3 100644
> --- a/libxfs/util.c
> +++ b/libxfs/util.c
> @@ -271,7 +271,8 @@ libxfs_init_new_inode(
> 	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG | XFS_ICHGTIME_MOD);
> 
> 	if (pip && (VFS_I(pip)->i_mode & S_ISGID)) {
> -		VFS_I(ip)->i_gid = VFS_I(pip)->i_gid;
> +		if (!(cr->cr_flags & CRED_FORCE_GID))
> +			VFS_I(ip)->i_gid = VFS_I(pip)->i_gid;
> 		if ((VFS_I(pip)->i_mode & S_ISGID) && (mode & S_IFMT) == S_IFDIR)
> 			VFS_I(ip)->i_mode |= S_ISGID;
> 	}
> diff --git a/mkfs/proto.c b/mkfs/proto.c
> index ef130ed6..127d87dd 100644
> --- a/mkfs/proto.c
> +++ b/mkfs/proto.c
> @@ -378,7 +378,7 @@ parseproto(
> 	xfs_trans_t	*tp;
> 	int		val;
> 	int		isroot = 0;
> -	cred_t		creds;
> +	struct cred	creds;
> 	char		*value;
> 	struct xfs_name	xname;
> 
> @@ -446,6 +446,7 @@ parseproto(
> 	mode |= val;
> 	creds.cr_uid = (int)getnum(getstr(pp), 0, 0, false);
> 	creds.cr_gid = (int)getnum(getstr(pp), 0, 0, false);
> +	creds.cr_flags = CRED_FORCE_GID;
> 	xname.name = (unsigned char *)name;
> 	xname.len = name ? strlen(name) : 0;
> 	xname.type = 0;
> 

Re: [PATCH 4/4] mkfs: don't trample the gid set in the protofile

[Christoph Hellwig]

Maybe it's time to finally kill the protofile feature?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 5/4] mkfs: improve log extent validation

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Use the standard libxfs fsblock verifiers to check the start and end of
the internal log.  The current code does not catch the case of a
(segmented) fsblock that is beyond agf_blocks but not so large to change
the agno part of the segmented fsblock.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 libxfs/libxfs_api_defs.h |    1 +
 mkfs/xfs_mkfs.c          |   10 ++++------
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 064fb48c..f34a0483 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -207,6 +207,7 @@
 #define xfs_verify_agino		libxfs_verify_agino
 #define xfs_verify_cksum		libxfs_verify_cksum
 #define xfs_verify_dir_ino		libxfs_verify_dir_ino
+#define xfs_verify_fsbext		libxfs_verify_fsbext
 #define xfs_verify_fsbno		libxfs_verify_fsbno
 #define xfs_verify_ino			libxfs_verify_ino
 #define xfs_verify_rtbno		libxfs_verify_rtbno
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 0b1fb746..b932acaa 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3218,15 +3218,13 @@ align_internal_log(
 	int			sunit,
 	int			max_logblocks)
 {
-	uint64_t		logend;
-
 	/* round up log start if necessary */
 	if ((cfg->logstart % sunit) != 0)
 		cfg->logstart = ((cfg->logstart + (sunit - 1)) / sunit) * sunit;
 
 	/* If our log start overlaps the next AG's metadata, fail. */
-	if (XFS_FSB_TO_AGBNO(mp, cfg->logstart) <= XFS_AGFL_BLOCK(mp)) {
-			fprintf(stderr,
+	if (!libxfs_verify_fsbno(mp, cfg->logstart)) {
+		fprintf(stderr,
 _("Due to stripe alignment, the internal log start (%lld) cannot be aligned\n"
   "within an allocation group.\n"),
 			(long long) cfg->logstart);
@@ -3237,8 +3235,7 @@ _("Due to stripe alignment, the internal log start (%lld) cannot be aligned\n"
 	align_log_size(cfg, sunit, max_logblocks);
 
 	/* check the aligned log still starts and ends in the same AG. */
-	logend = cfg->logstart + cfg->logblocks - 1;
-	if (XFS_FSB_TO_AGNO(mp, cfg->logstart) != XFS_FSB_TO_AGNO(mp, logend)) {
+	if (!libxfs_verify_fsbext(mp, cfg->logstart, cfg->logblocks)) {
 		fprintf(stderr,
 _("Due to stripe alignment, the internal log size (%lld) is too large.\n"
   "Must fit within an allocation group.\n"),
@@ -3465,6 +3462,7 @@ start_superblock_setup(
 	sbp->sb_agblocks = (xfs_agblock_t)cfg->agsize;
 	sbp->sb_agblklog = (uint8_t)log2_roundup(cfg->agsize);
 	sbp->sb_agcount = (xfs_agnumber_t)cfg->agcount;
+	sbp->sb_dblocks = (xfs_rfsblock_t)cfg->dblocks;
 
 	sbp->sb_inodesize = (uint16_t)cfg->inodesize;
 	sbp->sb_inodelog = (uint8_t)cfg->inodelog;

Re: [PATCH 5/4] mkfs: improve log extent validation

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 6/4] mkfs: round log size down if rounding log start up causes overflow

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If rounding the log start up to the next stripe unit would cause the log
to overrun the end of the AG, round the log size down by a stripe unit.
We already ensured that logblocks was small enough to fit inside the AG,
so the minor adjustment should suffice.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index b932acaa..cfa38f17 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3219,9 +3219,19 @@ align_internal_log(
 	int			max_logblocks)
 {
 	/* round up log start if necessary */
-	if ((cfg->logstart % sunit) != 0)
+	if ((cfg->logstart % sunit) != 0) {
 		cfg->logstart = ((cfg->logstart + (sunit - 1)) / sunit) * sunit;
 
+		/*
+		 * If rounding up logstart to a stripe boundary moves the end
+		 * of the log past the end of the AG, reduce logblocks to get
+		 * it back under EOAG.
+		 */
+		if (!libxfs_verify_fsbext(mp, cfg->logstart, cfg->logblocks) &&
+		    cfg->logblocks > sunit)
+			cfg->logblocks -= sunit;
+	}
+
 	/* If our log start overlaps the next AG's metadata, fail. */
 	if (!libxfs_verify_fsbno(mp, cfg->logstart)) {
 		fprintf(stderr,

Re: [PATCH 6/4] mkfs: round log size down if rounding log start up causes overflow

[Darrick J. Wong]

On Fri, Apr 15, 2022 at 04:57:58PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If rounding the log start up to the next stripe unit would cause the log
> to overrun the end of the AG, round the log size down by a stripe unit.
> We already ensured that logblocks was small enough to fit inside the AG,
> so the minor adjustment should suffice.

Self NAK, Eric pointed out more ways this can fail.  New patch coming
soon.

--D

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  mkfs/xfs_mkfs.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index b932acaa..cfa38f17 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3219,9 +3219,19 @@ align_internal_log(
>  	int			max_logblocks)
>  {
>  	/* round up log start if necessary */
> -	if ((cfg->logstart % sunit) != 0)
> +	if ((cfg->logstart % sunit) != 0) {
>  		cfg->logstart = ((cfg->logstart + (sunit - 1)) / sunit) * sunit;
>  
> +		/*
> +		 * If rounding up logstart to a stripe boundary moves the end
> +		 * of the log past the end of the AG, reduce logblocks to get
> +		 * it back under EOAG.
> +		 */
> +		if (!libxfs_verify_fsbext(mp, cfg->logstart, cfg->logblocks) &&
> +		    cfg->logblocks > sunit)
> +			cfg->logblocks -= sunit;
> +	}
> +
>  	/* If our log start overlaps the next AG's metadata, fail. */
>  	if (!libxfs_verify_fsbno(mp, cfg->logstart)) {
>  		fprintf(stderr,

[PATCH v1.1 6/4] mkfs: round log size down if rounding log start up causes overflow

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

If rounding the log start up to the next stripe unit would cause the log
to overrun the end of the AG, round the log size down by a stripe unit.
We already ensured that logblocks was small enough to fit inside the AG,
so the minor adjustment should suffice.

This can be reproduced with:
mkfs.xfs -dsu=44k,sw=1,size=300m,file,name=fsfile -m rmapbt=0
and:
mkfs.xfs -dsu=48k,sw=1,size=512m,file,name=fsfile -m rmapbt=0

Reported-by: Eric Sandeen <sandeen@sandeen.net>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index b932acaa..01d2e8ca 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3234,6 +3234,15 @@ _("Due to stripe alignment, the internal log start (%lld) cannot be aligned\n"
 	/* round up/down the log size now */
 	align_log_size(cfg, sunit, max_logblocks);
 
+	/*
+	 * If the end of the log has been rounded up past the end of the AG,
+	 * reduce logblocks by a stripe unit to try to get it back under EOAG.
+	 */
+	if (!libxfs_verify_fsbext(mp, cfg->logstart, cfg->logblocks) &&
+	    cfg->logblocks > sunit) {
+		cfg->logblocks -= sunit;
+	}
+
 	/* check the aligned log still starts and ends in the same AG. */
 	if (!libxfs_verify_fsbext(mp, cfg->logstart, cfg->logblocks)) {
 		fprintf(stderr,