diff mbox series

[39/45] libxfs: remove pointless *XFS_MOUNT* flags

Message ID 164263805814.860211.18062742237091017727.stgit@magnolia (mailing list archive)
State New, archived
Headers show
Series xfsprogs: sync libxfs with 5.15 | expand

Commit Message

Darrick J. Wong Jan. 20, 2022, 12:20 a.m. UTC 
From: Darrick J. Wong <djwong@kernel.org>

Get rid of these flags and the m_flags field, since none of them do
anything anymore.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 db/attrset.c              |    4 ----
 include/xfs_mount.h       |    9 ++-------
 libxfs/init.c             |   13 +++----------
 libxfs/libxfs_priv.h      |   10 ----------
 libxlog/xfs_log_recover.c |    1 -
 5 files changed, 5 insertions(+), 32 deletions(-)

Comments

Eric Sandeen Jan. 27, 2022, 11:03 p.m. UTC | #1 
On 1/19/22 6:20 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Get rid of these flags and the m_flags field, since none of them do
> anything anymore.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

...

> diff --git a/libxfs/init.c b/libxfs/init.c
> index e9235a35..093ce878 100644
> --- a/libxfs/init.c
> +++ b/libxfs/init.c
> @@ -540,13 +540,10 @@ xfs_set_inode_alloc(
>   	 * sufficiently large, set XFS_MOUNT_32BITINODES if we must alter
>   	 * the allocator to accommodate the request.
>   	 */
> -	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > XFS_MAXINUMBER_32) {
> +	if (ino > XFS_MAXINUMBER_32)
>   		xfs_set_inode32(mp);
> -		mp->m_flags |= XFS_MOUNT_32BITINODES;
> -	} else {
> +	else
>   		xfs_clear_inode32(mp);
> -		mp->m_flags &= ~XFS_MOUNT_32BITINODES;
> -	}

Hm, so this just removes the "XFS_MOUNT_SMALL_INUMS" test. In the last
release, nothing ever set this flag in userspace, so the first part of
the conditional was always false, so we always cleared the 32bitinode
setting.

So I think this is a change in behavior, and if we get a request for a
large inode, we'll enable inode32, at least for this session?

But maybe that's ok, since there is no "inode32 mount option" in
userspace, and maybe we *shouldn't* be allocating 64-bit inodes in
userspace?  <thinking>

<thinking some more>

I'll get back to this :)

-Eric
Darrick J. Wong Jan. 28, 2022, 12:53 a.m. UTC | #2 
On Thu, Jan 27, 2022 at 05:03:22PM -0600, Eric Sandeen wrote:
> On 1/19/22 6:20 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Get rid of these flags and the m_flags field, since none of them do
> > anything anymore.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> 
> ...
> 
> > diff --git a/libxfs/init.c b/libxfs/init.c
> > index e9235a35..093ce878 100644
> > --- a/libxfs/init.c
> > +++ b/libxfs/init.c
> > @@ -540,13 +540,10 @@ xfs_set_inode_alloc(
> >   	 * sufficiently large, set XFS_MOUNT_32BITINODES if we must alter
> >   	 * the allocator to accommodate the request.
> >   	 */
> > -	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > XFS_MAXINUMBER_32) {
> > +	if (ino > XFS_MAXINUMBER_32)
> >   		xfs_set_inode32(mp);
> > -		mp->m_flags |= XFS_MOUNT_32BITINODES;
> > -	} else {
> > +	else
> >   		xfs_clear_inode32(mp);
> > -		mp->m_flags &= ~XFS_MOUNT_32BITINODES;
> > -	}
> 
> Hm, so this just removes the "XFS_MOUNT_SMALL_INUMS" test. In the last
> release, nothing ever set this flag in userspace, so the first part of
> the conditional was always false, so we always cleared the 32bitinode
> setting.
> 
> So I think this is a change in behavior, and if we get a request for a
> large inode, we'll enable inode32, at least for this session?
> 
> But maybe that's ok, since there is no "inode32 mount option" in
> userspace, and maybe we *shouldn't* be allocating 64-bit inodes in
> userspace?  <thinking>
> 
> <thinking some more>
> 
> I'll get back to this :)

Ooh, good catch.  MOUNT_SMALL_INUMS was zero, so the first part of the
if test was alway zero, which means that we always call the else clause.
IOWs, inode64 should always be enabled, not inode32.

Maybe there's an argument for only using inode32 mode in userspace
(though userspace never adds user-visible files) so that a system that
has inode32 in fstab will never ever have big inodes?

Dunno, but that's a separate patch if people really want that.

I'll respin this patch with fixed logic.  I'll also fix the comments.

--D

> -Eric
> 
>
Eric Sandeen Jan. 28, 2022, 8:01 p.m. UTC | #3 
On 1/19/22 6:20 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Get rid of these flags and the m_flags field, since none of them do
> anything anymore.

> diff --git a/db/attrset.c b/db/attrset.c
> index 98a08a49..6441809a 100644
> --- a/db/attrset.c
> +++ b/db/attrset.c
> @@ -107,7 +107,6 @@ attr_set_f(
>   			break;
>   
>   		case 'n':
> -			mp->m_flags |= LIBXFS_MOUNT_COMPAT_ATTR;
>   			break;

That leaves an interesting no-op! I think IRC discussion ended at
"haha it was a no-op before, too!" but maybe a comment to note the
weird wart would be reasonable while we work out what to do with
it, since it's so blazingly obvious now.

>   
>   		/* value length */
> @@ -169,7 +168,6 @@ attr_set_f(
>   	set_cur_inode(iocur_top->ino);
>   
>   out:
> -	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
>   	if (args.dp)
>   		libxfs_irele(args.dp);
>   	if (args.value)
> @@ -211,7 +209,6 @@ attr_remove_f(
>   			break;
>   
>   		case 'n':
> -			mp->m_flags |= LIBXFS_MOUNT_COMPAT_ATTR;
>   			break;

here as well

...

> diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c
> index 592e4502..d43914b9 100644
> --- a/libxlog/xfs_log_recover.c
> +++ b/libxlog/xfs_log_recover.c
> @@ -827,7 +827,6 @@ xlog_find_tail(
>   			 * superblock counters from the perag headers if we
>   			 * have a filesystem using non-persistent counters.
>   			 */
> -			log->l_mp->m_flags |= XFS_MOUNT_WAS_CLEAN;
>   		}
>   	}

Preceeding comment should go too, then, or maybe we should leave the
equivalent opstate in place there? I'm not sure if the libxlog copies are
supposed to stay as true as possible to kernelspace, or if we expect them
to diverge and remove the pointless bits.  Probably the latter, right?
So probably just nuke the comment.

-Eric
Darrick J. Wong Jan. 28, 2022, 9:59 p.m. UTC | #4 
On Fri, Jan 28, 2022 at 02:01:53PM -0600, Eric Sandeen wrote:
> On 1/19/22 6:20 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Get rid of these flags and the m_flags field, since none of them do
> > anything anymore.
> 
> > diff --git a/db/attrset.c b/db/attrset.c
> > index 98a08a49..6441809a 100644
> > --- a/db/attrset.c
> > +++ b/db/attrset.c
> > @@ -107,7 +107,6 @@ attr_set_f(
> >   			break;
> >   		case 'n':
> > -			mp->m_flags |= LIBXFS_MOUNT_COMPAT_ATTR;
> >   			break;
> 
> That leaves an interesting no-op! I think IRC discussion ended at
> "haha it was a no-op before, too!" but maybe a comment to note the
> weird wart would be reasonable while we work out what to do with
> it, since it's so blazingly obvious now.

Ok, I'll go add a comment.

> 
> >   		/* value length */
> > @@ -169,7 +168,6 @@ attr_set_f(
> >   	set_cur_inode(iocur_top->ino);
> >   out:
> > -	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
> >   	if (args.dp)
> >   		libxfs_irele(args.dp);
> >   	if (args.value)
> > @@ -211,7 +209,6 @@ attr_remove_f(
> >   			break;
> >   		case 'n':
> > -			mp->m_flags |= LIBXFS_MOUNT_COMPAT_ATTR;
> >   			break;
> 
> here as well
> 
> ...
> 
> > diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c
> > index 592e4502..d43914b9 100644
> > --- a/libxlog/xfs_log_recover.c
> > +++ b/libxlog/xfs_log_recover.c
> > @@ -827,7 +827,6 @@ xlog_find_tail(
> >   			 * superblock counters from the perag headers if we
> >   			 * have a filesystem using non-persistent counters.
> >   			 */
> > -			log->l_mp->m_flags |= XFS_MOUNT_WAS_CLEAN;
> >   		}
> >   	}
> 
> Preceeding comment should go too, then, or maybe we should leave the
> equivalent opstate in place there? I'm not sure if the libxlog copies are
> supposed to stay as true as possible to kernelspace, or if we expect them
> to diverge and remove the pointless bits.  Probably the latter, right?
> So probably just nuke the comment.

The comment doesn't make sense anymore so I'll remove it.

As for libxlog, the kernel has diverged so far from userspace at this
point that it probably doesn't matter.

--D

> -Eric
diff mbox series

Patch

diff --git a/db/attrset.c b/db/attrset.c
index 98a08a49..6441809a 100644
--- a/db/attrset.c
+++ b/db/attrset.c
@@ -107,7 +107,6 @@  attr_set_f(
 			break;
 
 		case 'n':
-			mp->m_flags |= LIBXFS_MOUNT_COMPAT_ATTR;
 			break;
 
 		/* value length */
@@ -169,7 +168,6 @@  attr_set_f(
 	set_cur_inode(iocur_top->ino);
 
 out:
-	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
 	if (args.dp)
 		libxfs_irele(args.dp);
 	if (args.value)
@@ -211,7 +209,6 @@  attr_remove_f(
 			break;
 
 		case 'n':
-			mp->m_flags |= LIBXFS_MOUNT_COMPAT_ATTR;
 			break;
 
 		default:
@@ -254,7 +251,6 @@  attr_remove_f(
 	set_cur_inode(iocur_top->ino);
 
 out:
-	mp->m_flags &= ~LIBXFS_MOUNT_COMPAT_ATTR;
 	if (args.dp)
 		libxfs_irele(args.dp);
 	return 0;
diff --git a/include/xfs_mount.h b/include/xfs_mount.h
index 52b699f1..37398fd3 100644
--- a/include/xfs_mount.h
+++ b/include/xfs_mount.h
@@ -79,7 +79,6 @@  typedef struct xfs_mount {
 	uint			m_alloc_set_aside; /* space we can't use */
 	uint			m_ag_max_usable; /* max space per AG */
 	struct radix_tree_root	m_perag_tree;
-	uint			m_flags;	/* global mount flags */
 	uint64_t		m_features;	/* active filesystem features */
 	unsigned long		m_opstate;	/* dynamic state flags */
 	bool			m_finobt_nores; /* no per-AG finobt resv. */
@@ -250,16 +249,12 @@  __XFS_UNSUPP_OPSTATE(readonly)
 __XFS_UNSUPP_OPSTATE(shutdown)
 
 #define LIBXFS_MOUNT_DEBUGGER		0x0001
-#define LIBXFS_MOUNT_32BITINODES	0x0002
-#define LIBXFS_MOUNT_32BITINOOPT	0x0004
-#define LIBXFS_MOUNT_COMPAT_ATTR	0x0008
-#define LIBXFS_MOUNT_ATTR2		0x0010
 #define LIBXFS_MOUNT_WANT_CORRUPTED	0x0020
 
 #define LIBXFS_BHASHSIZE(sbp) 		(1<<10)
 
-extern xfs_mount_t	*libxfs_mount (xfs_mount_t *, xfs_sb_t *,
-				dev_t, dev_t, dev_t, int);
+struct xfs_mount *libxfs_mount(struct xfs_mount *mp, struct xfs_sb *sb,
+		dev_t dev, dev_t logdev, dev_t rtdev, unsigned int flags);
 int libxfs_flush_mount(struct xfs_mount *mp);
 int		libxfs_umount(struct xfs_mount *mp);
 extern void	libxfs_rtmount_destroy (xfs_mount_t *);
diff --git a/libxfs/init.c b/libxfs/init.c
index e9235a35..093ce878 100644
--- a/libxfs/init.c
+++ b/libxfs/init.c
@@ -540,13 +540,10 @@  xfs_set_inode_alloc(
 	 * sufficiently large, set XFS_MOUNT_32BITINODES if we must alter
 	 * the allocator to accommodate the request.
 	 */
-	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) && ino > XFS_MAXINUMBER_32) {
+	if (ino > XFS_MAXINUMBER_32)
 		xfs_set_inode32(mp);
-		mp->m_flags |= XFS_MOUNT_32BITINODES;
-	} else {
+	else
 		xfs_clear_inode32(mp);
-		mp->m_flags &= ~XFS_MOUNT_32BITINODES;
-	}
 
 	for (index = 0; index < agcount; index++) {
 		struct xfs_perag	*pag;
@@ -718,7 +715,7 @@  libxfs_mount(
 	dev_t			dev,
 	dev_t			logdev,
 	dev_t			rtdev,
-	int			flags)
+	unsigned int		flags)
 {
 	struct xfs_buf		*bp;
 	struct xfs_sb		*sbp;
@@ -733,7 +730,6 @@  libxfs_mount(
 	libxfs_buftarg_init(mp, dev, logdev, rtdev);
 
 	mp->m_finobt_nores = true;
-	mp->m_flags = (LIBXFS_MOUNT_32BITINODES|LIBXFS_MOUNT_32BITINOOPT);
 	xfs_set_inode32(mp);
 	mp->m_sb = *sb;
 	INIT_RADIX_TREE(&mp->m_perag_tree, GFP_KERNEL);
@@ -799,9 +795,6 @@  libxfs_mount(
 
 	xfs_da_mount(mp);
 
-	if (xfs_has_attr2(mp))
-		mp->m_flags |= LIBXFS_MOUNT_ATTR2;
-
 	/* Initialize the precomputed transaction reservations values */
 	xfs_trans_init(mp);
 
diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index 2b72751d..b94ff41e 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -442,16 +442,6 @@  void __xfs_buf_mark_corrupt(struct xfs_buf *bp, xfs_failaddr_t fa);
 #define xfs_buf_mark_corrupt(bp) __xfs_buf_mark_corrupt((bp), __this_address)
 
 /* mount stuff */
-#define XFS_MOUNT_32BITINODES		LIBXFS_MOUNT_32BITINODES
-#define XFS_MOUNT_ATTR2			LIBXFS_MOUNT_ATTR2
-#define XFS_MOUNT_SMALL_INUMS		0	/* ignored in userspace */
-#define XFS_MOUNT_WSYNC			0	/* ignored in userspace */
-#define XFS_MOUNT_NOALIGN		0	/* ignored in userspace */
-#define XFS_MOUNT_IKEEP			0	/* ignored in userspace */
-#define XFS_MOUNT_SWALLOC		0	/* ignored in userspace */
-#define XFS_MOUNT_RDONLY		0	/* ignored in userspace */
-#define XFS_MOUNT_BAD_SUMMARY		0	/* ignored in userspace */
-
 #define xfs_trans_set_sync(tp)		((void) 0)
 #define xfs_trans_buf_set_type(tp, bp, t)	({	\
 	int __t = (t);					\
diff --git a/libxlog/xfs_log_recover.c b/libxlog/xfs_log_recover.c
index 592e4502..d43914b9 100644
--- a/libxlog/xfs_log_recover.c
+++ b/libxlog/xfs_log_recover.c
@@ -827,7 +827,6 @@  xlog_find_tail(
 			 * superblock counters from the perag headers if we
 			 * have a filesystem using non-persistent counters.
 			 */
-			log->l_mp->m_flags |= XFS_MOUNT_WAS_CLEAN;
 		}
 	}