[PATCH v2 1/3] xfs: don't set bt_nr_sectors to a negative number

[PATCH v2 1/3] xfs: don't set bt_nr_sectors to a negative number

[Darrick J. Wong]

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

xfs_daddr_t is a signed type, which means that xfs_buf_map_verify is
using a signed comparison.  This causes problems if bt_nr_sectors is
never overridden (e.g. in the case of an xfbtree for rmap btree repairs)
because even daddr 0 can't pass the verifier test in that case.

Define an explicit max constant and set the initial bt_nr_sectors to a
positive value.

Found by xfs/422.

Cc: <stable@vger.kernel.org> # v6.18-rc1
Fixes: 42852fe57c6d2a ("xfs: track the number of blocks in each buftarg")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_buf.h |    1 +
 fs/xfs/xfs_buf.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 8fa7bdf59c9110..e25cd2a160f31c 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -22,6 +22,7 @@ extern struct kmem_cache *xfs_buf_cache;
  */
 struct xfs_buf;
 
+#define XFS_BUF_DADDR_MAX	((xfs_daddr_t) S64_MAX)
 #define XFS_BUF_DADDR_NULL	((xfs_daddr_t) (-1LL))
 
 #define XBF_READ	 (1u << 0) /* buffer intended for reading from device */
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 773d959965dc29..47edf3041631bb 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1751,7 +1751,7 @@ xfs_init_buftarg(
 	const char			*descr)
 {
 	/* The maximum size of the buftarg is only known once the sb is read. */
-	btp->bt_nr_sectors = (xfs_daddr_t)-1;
+	btp->bt_nr_sectors = XFS_BUF_DADDR_MAX;
 
 	/* Set up device logical sector size mask */
 	btp->bt_logical_sectorsize = logical_sectorsize;

[PATCH v2 2/3] xfs: always warn about deprecated mount options

[Darrick J. Wong]

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

The deprecation of the 'attr2' mount option in 6.18 wasn't entirely
successful because nobody noticed that the kernel never printed a
warning about attr2 being set in fstab if the only xfs filesystem is the
root fs; the initramfs mounts the root fs with no mount options; and the
init scripts only conveyed the fstab options by remounting the root fs.

Fix this by making it complain all the time.

Cc: <stable@vger.kernel.org> # v5.13
Fixes: 92cf7d36384b99 ("xfs: Skip repetitive warnings about mount options")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_super.c |   25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index e85a156dc17d16..ae9b17730eaf41 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1373,16 +1373,25 @@ suffix_kstrtoull(
 static inline void
 xfs_fs_warn_deprecated(
 	struct fs_context	*fc,
-	struct fs_parameter	*param,
-	uint64_t		flag,
-	bool			value)
+	struct fs_parameter	*param)
 {
-	/* Don't print the warning if reconfiguring and current mount point
-	 * already had the flag set
+	/*
+	 * Always warn about someone passing in a deprecated mount option.
+	 * Previously we wouldn't print the warning if we were reconfiguring
+	 * and current mount point already had the flag set, but that was not
+	 * the right thing to do.
+	 *
+	 * Many distributions mount the root filesystem with no options in the
+	 * initramfs and rely on mount -a to remount the root fs with the
+	 * options in fstab.  However, the old behavior meant that there would
+	 * never be a warning about deprecated mount options for the root fs in
+	 * /etc/fstab.  On a single-fs system, that means no warning at all.
+	 *
+	 * Compounding this problem are distribution scripts that copy
+	 * /proc/mounts to fstab, which means that we can't remove mount
+	 * options unless we're 100% sure they have only ever been advertised
+	 * in /proc/mounts in response to explicitly provided mount options.
 	 */
-	if ((fc->purpose & FS_CONTEXT_FOR_RECONFIGURE) &&
-            !!(XFS_M(fc->root->d_sb)->m_features & flag) == value)
-		return;
 	xfs_warn(fc->s_fs_info, "%s mount option is deprecated.", param->key);
 }
 

[PATCH v2 3/3] xfs: quietly ignore deprecated mount options

[Darrick J. Wong]

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

Apparently we can never deprecate mount options in this project, because
it will invariably turn out that some foolish userspace depends on some
behavior and break.  From Oleksandr Natalenko:

> In v6.18, the attr2 XFS mount option is removed. This may silently
> break system boot if the attr2 option is still present in /etc/fstab
> for rootfs.
>
> Consider Arch Linux that is being set up from scratch with / being
> formatted as XFS. The genfstab command that is used to generate
> /etc/fstab produces something like this by default:
>
> /dev/sda2 on / type xfs (rw,relatime,attr2,discard,inode64,logbufs=8,logbsize=32k,noquota)
>
> Once the system is set up and rebooted, there's no deprecation warning
> seen in the kernel log:
>
> # cat /proc/cmdline
> root=UUID=77b42de2-397e-47ee-a1ef-4dfd430e47e9 rootflags=discard rd.luks.options=discard quiet
>
> # dmesg | grep -i xfs
> [    2.409818] SGI XFS with ACLs, security attributes, realtime, scrub, repair, quota, no debug enabled
> [    2.415341] XFS (sda2): Mounting V5 Filesystem 77b42de2-397e-47ee-a1ef-4dfd430e47e9
> [    2.442546] XFS (sda2): Ending clean mount
>
> Although as per the deprecation intention, it should be there.
>
> Vlastimil (in Cc) suggests this is because xfs_fs_warn_deprecated()
> doesn't produce any warning by design if the XFS FS is set to be
> rootfs and gets remounted read-write during boot. This imposes two
> problems:
>
> 1) a user doesn't see the deprecation warning; and
> 2) with v6.18 kernel, the read-write remount fails because of unknown
>    attr2 option rendering system unusable:
>
> systemd[1]: Switching root.
> systemd-remount-fs[225]: /usr/bin/mount for / exited with exit status 32.
>
> # mount -o rw /
> mount: /: fsconfig() failed: xfs: Unknown parameter 'attr2'.
>
> Thorsten (in Cc) suggested reporting this as a user-visible regression.
>
> From my PoV, although the deprecation is in place for 5 years already,
> it may not be visible enough as the warning is not emitted for rootfs.
> Considering the amount of systems set up with XFS on /, this may
> impose a mass problem for users.
>
> Vlastimil suggested making attr2 option a complete noop instead of
> removing it.

IOWs, the initrd mounts the root fs with (I assume) no mount options,
and mount -a remounts with whatever options are in fstab.  However,
XFS doesn't complain about deprecated mount options during a remount, so
technically speaking we were not warning all users in all combinations
that they were heading for a cliff.

Gotcha!!

Now, how did 'attr2' get slurped up on so many systems?  The old code
would put that in /proc/mounts if the filesystem happened to be in attr2
mode, even if user hadn't mounted with any such option.  IOWs, this is
because someone thought it would be a good idea to advertise system
state via /proc/mounts.

The easy way to fix this is to reintroduce the four mount options but
map them to a no-op option that ignores them, and hope that nobody's
depending on attr2 to appear in /proc/mounts.  (Hint: use the fsgeometry
ioctl).  But we've learned our lesson, so complain as LOUDLY as possible
about the deprecation.

Lessons learned:

 1. Don't expose system state via /proc/mounts; the only strings that
    ought to be there are options *explicitly* provided by the user.
 2. Never tidy, it's not worth the stress and irritation.

Reported-by: oleksandr@natalenko.name
Reported-by: vbabka@suse.cz
Cc: <stable@vger.kernel.org> # v6.18-rc1
Fixes: b9a176e54162f8 ("xfs: remove deprecated mount options")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/xfs_super.c |   20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index ae9b17730eaf41..d8f326d8838036 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -102,7 +102,7 @@ static const struct constant_table dax_param_enums[] = {
  * Table driven mount option parser.
  */
 enum {
-	Opt_logbufs, Opt_logbsize, Opt_logdev, Opt_rtdev,
+	Op_deprecated, Opt_logbufs, Opt_logbsize, Opt_logdev, Opt_rtdev,
 	Opt_wsync, Opt_noalign, Opt_swalloc, Opt_sunit, Opt_swidth, Opt_nouuid,
 	Opt_grpid, Opt_nogrpid, Opt_bsdgroups, Opt_sysvgroups,
 	Opt_allocsize, Opt_norecovery, Opt_inode64, Opt_inode32,
@@ -114,7 +114,21 @@ enum {
 	Opt_lifetime, Opt_nolifetime, Opt_max_atomic_write,
 };
 
+#define fsparam_dead(NAME) \
+	__fsparam(NULL, (NAME), Op_deprecated, fs_param_deprecated, NULL)
+
 static const struct fs_parameter_spec xfs_fs_parameters[] = {
+	/*
+	 * These mount options were supposed to be deprecated in September 2025
+	 * but the deprecation warning was buggy, so not all users were
+	 * notified.  The deprecation is now obnoxiously loud and postponed to
+	 * September 2030.
+	 */
+	fsparam_dead("attr2"),
+	fsparam_dead("noattr2"),
+	fsparam_dead("ikeep"),
+	fsparam_dead("noikeep"),
+
 	fsparam_u32("logbufs",		Opt_logbufs),
 	fsparam_string("logbsize",	Opt_logbsize),
 	fsparam_string("logdev",	Opt_logdev),
@@ -1417,6 +1431,9 @@ xfs_fs_parse_param(
 		return opt;
 
 	switch (opt) {
+	case Op_deprecated:
+		xfs_fs_warn_deprecated(fc, param);
+		return 0;
 	case Opt_logbufs:
 		parsing_mp->m_logbufs = result.uint_32;
 		return 0;
@@ -1537,7 +1554,6 @@ xfs_fs_parse_param(
 		xfs_mount_set_dax_mode(parsing_mp, result.uint_32);
 		return 0;
 #endif
-	/* Following mount options will be removed in September 2025 */
 	case Opt_max_open_zones:
 		parsing_mp->m_max_open_zones = result.uint_32;
 		return 0;

Re: [PATCH v2 3/3] xfs: quietly ignore deprecated mount options

[Vlastimil Babka]

On 10/15/25 07:04, Darrick J. Wong wrote:
>  static const struct fs_parameter_spec xfs_fs_parameters[] = {
> +	/*
> +	 * These mount options were supposed to be deprecated in September 2025
> +	 * but the deprecation warning was buggy, so not all users were
> +	 * notified.  The deprecation is now obnoxiously loud and postponed to
> +	 * September 2030.
> +	 */

FWIW, this seems at odds with the subject "quietly ignore" ;)
"loudly ignore"? ;)
"warn about but otherwise ignore"?

Also there's maybe a difference of ignoring "attr2" because it's enabled
anyway, and ignoring "noattr2" because it's going to be enabled regardless.
AFAIK prior to b9a176e54162f8 "noattr2" still prevented the enabling? But
maybe it's not important. (I don't know how (no)ikeep works.)

Hypothetically someone might complaing after taking a disk out of very old
system without attr2, booting it on 6.18 that will enable attr2, and not
being able to use it again in the old system. (Funnily enough similar issue
recently happened to me with btrfs from Turris 1.0 router's microSD). But
maybe there are other things besides attr2 that can cause it anyway.

Anyway I think even in 2030 it will be the best to just keep warning instead
of refusing to mount.

> +	fsparam_dead("attr2"),
> +	fsparam_dead("noattr2"),
> +	fsparam_dead("ikeep"),
> +	fsparam_dead("noikeep"),
> +
>  	fsparam_u32("logbufs",		Opt_logbufs),
>  	fsparam_string("logbsize",	Opt_logbsize),
>  	fsparam_string("logdev",	Opt_logdev),
> @@ -1417,6 +1431,9 @@ xfs_fs_parse_param(
>  		return opt;
>  
>  	switch (opt) {
> +	case Op_deprecated:
> +		xfs_fs_warn_deprecated(fc, param);
> +		return 0;
>  	case Opt_logbufs:
>  		parsing_mp->m_logbufs = result.uint_32;
>  		return 0;
> @@ -1537,7 +1554,6 @@ xfs_fs_parse_param(
>  		xfs_mount_set_dax_mode(parsing_mp, result.uint_32);
>  		return 0;
>  #endif
> -	/* Following mount options will be removed in September 2025 */
>  	case Opt_max_open_zones:
>  		parsing_mp->m_max_open_zones = result.uint_32;
>  		return 0;

Re: [PATCH v2 3/3] xfs: quietly ignore deprecated mount options

[Darrick J. Wong]

On Wed, Oct 15, 2025 at 10:07:54AM +0200, Vlastimil Babka wrote:
> On 10/15/25 07:04, Darrick J. Wong wrote:
> >  static const struct fs_parameter_spec xfs_fs_parameters[] = {
> > +	/*
> > +	 * These mount options were supposed to be deprecated in September 2025
> > +	 * but the deprecation warning was buggy, so not all users were
> > +	 * notified.  The deprecation is now obnoxiously loud and postponed to
> > +	 * September 2030.
> > +	 */
> 
> FWIW, this seems at odds with the subject "quietly ignore" ;)
> "loudly ignore"? ;)
> "warn about but otherwise ignore"?

Ugh, yeah, I forgot to update the subject line.

"xfs: loudly complain about defunct mount options", then.

> Also there's maybe a difference of ignoring "attr2" because it's enabled
> anyway, and ignoring "noattr2" because it's going to be enabled regardless.
> AFAIK prior to b9a176e54162f8 "noattr2" still prevented the enabling? But
> maybe it's not important. (I don't know how (no)ikeep works.)

Old filesystems will be automatically upgraded to attr2 the next time
anyone writes to an xattr structure.  After V4 is removed in 2030, xfs
won't have to deal with attr1 structures anymore, because V5 always has
attr2.

noikeep was (and still is) the default; it means that inode blocks are
deleted when they are no longer in use.

> Hypothetically someone might complaing after taking a disk out of very old
> system without attr2, booting it on 6.18 that will enable attr2, and not
> being able to use it again in the old system. (Funnily enough similar issue
> recently happened to me with btrfs from Turris 1.0 router's microSD). But
> maybe there are other things besides attr2 that can cause it anyway.
> 
> Anyway I think even in 2030 it will be the best to just keep warning instead
> of refusing to mount.

I think you're right, we should keep this forever.  It's not that big of
a deal to accumulate all the dead mount options via fsparam_dead, and
probably a good tombstone to prevent accidental reuse in the future.

--D

> > +	fsparam_dead("attr2"),
> > +	fsparam_dead("noattr2"),
> > +	fsparam_dead("ikeep"),
> > +	fsparam_dead("noikeep"),
> > +
> >  	fsparam_u32("logbufs",		Opt_logbufs),
> >  	fsparam_string("logbsize",	Opt_logbsize),
> >  	fsparam_string("logdev",	Opt_logdev),
> > @@ -1417,6 +1431,9 @@ xfs_fs_parse_param(
> >  		return opt;
> >  
> >  	switch (opt) {
> > +	case Op_deprecated:
> > +		xfs_fs_warn_deprecated(fc, param);
> > +		return 0;
> >  	case Opt_logbufs:
> >  		parsing_mp->m_logbufs = result.uint_32;
> >  		return 0;
> > @@ -1537,7 +1554,6 @@ xfs_fs_parse_param(
> >  		xfs_mount_set_dax_mode(parsing_mp, result.uint_32);
> >  		return 0;
> >  #endif
> > -	/* Following mount options will be removed in September 2025 */
> >  	case Opt_max_open_zones:
> >  		parsing_mp->m_max_open_zones = result.uint_32;
> >  		return 0;
> 
> 

[PATCH 4/3] xfs: fix locking in xchk_nlinks_collect_dir

[Darrick J. Wong]

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

On a filesystem with parent pointers, xchk_nlinks_collect_dir walks both
the directory entries (data fork) and the parent pointers (attr fork) to
determine the correct link count.  Unfortunately I forgot to update the
lock mode logic to handle the case of a directory whose attr fork is in
btree format and has not yet been loaded *and* whose data fork doesn't
need loading.

This leads to a bunch of assertions from xfs/286 in xfs_iread_extents
because we only took ILOCK_SHARED, not ILOCK_EXCL.  You'd need the rare
happenstance of a directory with a large number of non-pptr extended
attributes set and enough memory pressure to cause the directory to be
evicted and partially reloaded from disk.

I /think/ this only started in 6.18-rc1 because I've started seeing OOM
errors with the maple tree slab using 70% of memory, and this didn't
happen in 6.17.  Yay dynamic systems!

Cc: <stable@vger.kernel.org> # v6.10
Fixes: 77ede5f44b0d86 ("xfs: walk directory parent pointers to determine backref count")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/scrub/nlinks.c |   22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/scrub/nlinks.c b/fs/xfs/scrub/nlinks.c
index 26721fab5cab42..dcd57c3f65dfdc 100644
--- a/fs/xfs/scrub/nlinks.c
+++ b/fs/xfs/scrub/nlinks.c
@@ -376,6 +376,24 @@ xchk_nlinks_collect_pptr(
 	return error;
 }
 
+static uint
+xchk_nlinks_ilock_dir(
+	struct xfs_inode	*ip)
+{
+	uint			lock_mode = XFS_ILOCK_SHARED;
+
+	if (xfs_need_iread_extents(&ip->i_df))
+		lock_mode = XFS_ILOCK_EXCL;
+
+	if (xfs_has_parent(ip->i_mount) && xfs_inode_has_attr_fork(ip) &&
+	    xfs_need_iread_extents(&ip->i_af))
+		lock_mode = XFS_ILOCK_EXCL;
+
+	lock_mode |= XFS_IOLOCK_SHARED;
+	xfs_ilock(ip, lock_mode);
+	return lock_mode;
+}
+
 /* Walk a directory to bump the observed link counts of the children. */
 STATIC int
 xchk_nlinks_collect_dir(
@@ -394,8 +412,7 @@ xchk_nlinks_collect_dir(
 		return 0;
 
 	/* Prevent anyone from changing this directory while we walk it. */
-	xfs_ilock(dp, XFS_IOLOCK_SHARED);
-	lock_mode = xfs_ilock_data_map_shared(dp);
+	lock_mode = xchk_nlinks_ilock_dir(dp);
 
 	/*
 	 * The dotdot entry of an unlinked directory still points to the last
@@ -452,7 +469,6 @@ xchk_nlinks_collect_dir(
 	xchk_iscan_abort(&xnc->collect_iscan);
 out_unlock:
 	xfs_iunlock(dp, lock_mode);
-	xfs_iunlock(dp, XFS_IOLOCK_SHARED);
 	return error;
 }
 

Re: [PATCH v2 3/3] xfs: quietly ignore deprecated mount options

[Christoph Hellwig]

On Wed, Oct 15, 2025 at 08:37:02AM -0700, Darrick J. Wong wrote:
> won't have to deal with attr1 structures anymore, because V5 always has
> attr2.

There's isn't really any attr1 structures anyway, it's really just
lex flexible (aka) dump partitioning of the space in the inode.

> I think you're right, we should keep this forever.  It's not that big of
> a deal to accumulate all the dead mount options via fsparam_dead, and
> probably a good tombstone to prevent accidental reuse in the future.

Agreed.

Re: [PATCH v2 3/3] xfs: quietly ignore deprecated mount options

[Christoph Hellwig]

On Tue, Oct 14, 2025 at 10:04:31PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Apparently we can never deprecate mount options in this project, because
> it will invariably turn out that some foolish userspace depends on some
> behavior and break.  From Oleksandr Natalenko:

Assuming you are redoing this anyway, could you format the quotes
without the mail-style "> " quoting?  It reads really ugly in git log
output.

> Reported-by: oleksandr@natalenko.name
> Reported-by: vbabka@suse.cz

Normally Reported-by: has full names.

Re: [PATCH 4/3] xfs: fix locking in xchk_nlinks_collect_dir

[Christoph Hellwig]

> +static uint
> +xchk_nlinks_ilock_dir(
> +	struct xfs_inode	*ip)
> +{
> +	uint			lock_mode = XFS_ILOCK_SHARED;
> +
> +	if (xfs_need_iread_extents(&ip->i_df))
> +		lock_mode = XFS_ILOCK_EXCL;
> +
> +	if (xfs_has_parent(ip->i_mount) && xfs_inode_has_attr_fork(ip) &&
> +	    xfs_need_iread_extents(&ip->i_af))
> +		lock_mode = XFS_ILOCK_EXCL;

Please add a comment explaining the need for the conditions, this is too
much black magic otherwise.

Re: [PATCH 4/3] xfs: fix locking in xchk_nlinks_collect_dir

[Darrick J. Wong]

On Thu, Oct 16, 2025 at 10:48:07PM -0700, Christoph Hellwig wrote:
> > +static uint
> > +xchk_nlinks_ilock_dir(
> > +	struct xfs_inode	*ip)
> > +{
> > +	uint			lock_mode = XFS_ILOCK_SHARED;
> > +
> > +	if (xfs_need_iread_extents(&ip->i_df))
> > +		lock_mode = XFS_ILOCK_EXCL;
> > +
> > +	if (xfs_has_parent(ip->i_mount) && xfs_inode_has_attr_fork(ip) &&
> > +	    xfs_need_iread_extents(&ip->i_af))
> > +		lock_mode = XFS_ILOCK_EXCL;
> 
> Please add a comment explaining the need for the conditions, this is too
> much black magic otherwise.

Done.

--D