[PATCH 0/3 v3] xfs: simplify superblock logging

[PATCH 0/3 v3] xfs: simplify superblock logging

[Dave Chinner]

Hi folks,

This patchset removes all the field-flag based logging and writeback
updates from the superblock and replaces it with "whole structure"
updates and logging as done for other buffer updates. Previous
versions:

http://oss.sgi.com/archives/xfs/2014-07/msg00465.html
http://oss.sgi.com/archives/xfs/2014-09/msg00397.html

Comments welcome.

-Dave.

Version 3:
- rebased on current for-next
- dropped xfs_fs_writeable changes as that's already been merged.
- fixed endian conversion in quota flags updates
- updated transaction type definitions

Version 2:
- added initial patch to sanitse xfs_fs_writeable
- updated comments to describe caller requirements for
  xfs_sync_sb()


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/3] xfs: remove bitfield based superblock updates

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When we log changes to the superblock, we first have to write them
to the on-disk buffer, and then log that. Right now we have a
complex bitfield based arrangement to only write the modified field
to the buffer before we log it.

This used to be necessary as a performance optimisation because we
logged the superblock buffer in every extent or inode allocation or
freeing, and so performance was extremely important. We haven't done
this for years, however, ever since the lazy superblock counters
pulled the superblock logging out of the transaction commit
fast path.

Hence we have a bunch of complexity that is not necessary that makes
writing the in-core superblock to disk much more complex than it
needs to be. We only need to log the superblock now during
management operations (e.g. during mount, unmount or quota control
operations) so it is not a performance critical path anymore.

As such, remove the complex field based logging mechanism and
replace it with a simple conversion function similar to what we use
for all other on-disk structures.

This means we always log the entirity of the superblock, but again
because we rarely modify the superblock this is not an issue for log
bandwidth or CPU time. Indeed, if we do log the superblock
frequently, delayed logging will minimise the impact of this
overhead.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c |   2 +-
 fs/xfs/libxfs/xfs_bmap.c      |  14 +-
 fs/xfs/libxfs/xfs_sb.c        | 290 ++++++++++++++----------------------------
 fs/xfs/libxfs/xfs_sb.h        |  10 +-
 fs/xfs/xfs_fsops.c            |   6 +-
 fs/xfs/xfs_mount.c            |  21 ++-
 fs/xfs/xfs_mount.h            |   2 +-
 fs/xfs/xfs_qm.c               |  26 +---
 fs/xfs/xfs_qm.h               |   2 +-
 fs/xfs/xfs_qm_syscalls.c      |  13 +-
 fs/xfs/xfs_super.c            |   2 +-
 11 files changed, 132 insertions(+), 256 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 5d38e8b..c914422 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -403,7 +403,7 @@ xfs_sbversion_add_attr2(xfs_mount_t *mp, xfs_trans_t *tp)
 		if (!xfs_sb_version_hasattr2(&mp->m_sb)) {
 			xfs_sb_version_addattr2(&mp->m_sb);
 			spin_unlock(&mp->m_sb_lock);
-			xfs_mod_sb(tp, XFS_SB_VERSIONNUM | XFS_SB_FEATURES2);
+			xfs_mod_sb(tp);
 		} else
 			spin_unlock(&mp->m_sb_lock);
 	}
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index b5eb474..8c39cc8 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1221,22 +1221,20 @@ xfs_bmap_add_attrfork(
 		goto bmap_cancel;
 	if (!xfs_sb_version_hasattr(&mp->m_sb) ||
 	   (!xfs_sb_version_hasattr2(&mp->m_sb) && version == 2)) {
-		__int64_t sbfields = 0;
+		bool mod_sb = false;
 
 		spin_lock(&mp->m_sb_lock);
 		if (!xfs_sb_version_hasattr(&mp->m_sb)) {
 			xfs_sb_version_addattr(&mp->m_sb);
-			sbfields |= XFS_SB_VERSIONNUM;
+			mod_sb = true;
 		}
 		if (!xfs_sb_version_hasattr2(&mp->m_sb) && version == 2) {
 			xfs_sb_version_addattr2(&mp->m_sb);
-			sbfields |= (XFS_SB_VERSIONNUM | XFS_SB_FEATURES2);
+			mod_sb = true;
 		}
-		if (sbfields) {
-			spin_unlock(&mp->m_sb_lock);
-			xfs_mod_sb(tp, sbfields);
-		} else
-			spin_unlock(&mp->m_sb_lock);
+		spin_unlock(&mp->m_sb_lock);
+		if (mod_sb)
+			xfs_mod_sb(tp);
 	}
 
 	error = xfs_bmap_finish(&tp, &flist, &committed);
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 752915f..4afbbce 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -40,69 +40,6 @@
  * Physical superblock buffer manipulations. Shared with libxfs in userspace.
  */
 
-static const struct {
-	short offset;
-	short type;	/* 0 = integer
-			 * 1 = binary / string (no translation)
-			 */
-} xfs_sb_info[] = {
-	{ offsetof(xfs_sb_t, sb_magicnum),	0 },
-	{ offsetof(xfs_sb_t, sb_blocksize),	0 },
-	{ offsetof(xfs_sb_t, sb_dblocks),	0 },
-	{ offsetof(xfs_sb_t, sb_rblocks),	0 },
-	{ offsetof(xfs_sb_t, sb_rextents),	0 },
-	{ offsetof(xfs_sb_t, sb_uuid),		1 },
-	{ offsetof(xfs_sb_t, sb_logstart),	0 },
-	{ offsetof(xfs_sb_t, sb_rootino),	0 },
-	{ offsetof(xfs_sb_t, sb_rbmino),	0 },
-	{ offsetof(xfs_sb_t, sb_rsumino),	0 },
-	{ offsetof(xfs_sb_t, sb_rextsize),	0 },
-	{ offsetof(xfs_sb_t, sb_agblocks),	0 },
-	{ offsetof(xfs_sb_t, sb_agcount),	0 },
-	{ offsetof(xfs_sb_t, sb_rbmblocks),	0 },
-	{ offsetof(xfs_sb_t, sb_logblocks),	0 },
-	{ offsetof(xfs_sb_t, sb_versionnum),	0 },
-	{ offsetof(xfs_sb_t, sb_sectsize),	0 },
-	{ offsetof(xfs_sb_t, sb_inodesize),	0 },
-	{ offsetof(xfs_sb_t, sb_inopblock),	0 },
-	{ offsetof(xfs_sb_t, sb_fname[0]),	1 },
-	{ offsetof(xfs_sb_t, sb_blocklog),	0 },
-	{ offsetof(xfs_sb_t, sb_sectlog),	0 },
-	{ offsetof(xfs_sb_t, sb_inodelog),	0 },
-	{ offsetof(xfs_sb_t, sb_inopblog),	0 },
-	{ offsetof(xfs_sb_t, sb_agblklog),	0 },
-	{ offsetof(xfs_sb_t, sb_rextslog),	0 },
-	{ offsetof(xfs_sb_t, sb_inprogress),	0 },
-	{ offsetof(xfs_sb_t, sb_imax_pct),	0 },
-	{ offsetof(xfs_sb_t, sb_icount),	0 },
-	{ offsetof(xfs_sb_t, sb_ifree),		0 },
-	{ offsetof(xfs_sb_t, sb_fdblocks),	0 },
-	{ offsetof(xfs_sb_t, sb_frextents),	0 },
-	{ offsetof(xfs_sb_t, sb_uquotino),	0 },
-	{ offsetof(xfs_sb_t, sb_gquotino),	0 },
-	{ offsetof(xfs_sb_t, sb_qflags),	0 },
-	{ offsetof(xfs_sb_t, sb_flags),		0 },
-	{ offsetof(xfs_sb_t, sb_shared_vn),	0 },
-	{ offsetof(xfs_sb_t, sb_inoalignmt),	0 },
-	{ offsetof(xfs_sb_t, sb_unit),		0 },
-	{ offsetof(xfs_sb_t, sb_width),		0 },
-	{ offsetof(xfs_sb_t, sb_dirblklog),	0 },
-	{ offsetof(xfs_sb_t, sb_logsectlog),	0 },
-	{ offsetof(xfs_sb_t, sb_logsectsize),	0 },
-	{ offsetof(xfs_sb_t, sb_logsunit),	0 },
-	{ offsetof(xfs_sb_t, sb_features2),	0 },
-	{ offsetof(xfs_sb_t, sb_bad_features2),	0 },
-	{ offsetof(xfs_sb_t, sb_features_compat),	0 },
-	{ offsetof(xfs_sb_t, sb_features_ro_compat),	0 },
-	{ offsetof(xfs_sb_t, sb_features_incompat),	0 },
-	{ offsetof(xfs_sb_t, sb_features_log_incompat),	0 },
-	{ offsetof(xfs_sb_t, sb_crc),		0 },
-	{ offsetof(xfs_sb_t, sb_pad),		0 },
-	{ offsetof(xfs_sb_t, sb_pquotino),	0 },
-	{ offsetof(xfs_sb_t, sb_lsn),		0 },
-	{ sizeof(xfs_sb_t),			0 }
-};
-
 /*
  * Reference counting access wrappers to the perag structures.
  * Because we never free per-ag structures, the only thing we
@@ -461,128 +398,119 @@ xfs_sb_from_disk(
 	__xfs_sb_from_disk(to, from, true);
 }
 
-static inline void
+static void
 xfs_sb_quota_to_disk(
-	xfs_dsb_t	*to,
-	xfs_sb_t	*from,
-	__int64_t	*fields)
+	struct xfs_dsb	*to,
+	struct xfs_sb	*from)
 {
 	__uint16_t	qflags = from->sb_qflags;
 
+	to->sb_uquotino = cpu_to_be64(from->sb_uquotino);
+	if (xfs_sb_version_has_pquotino(from)) {
+		to->sb_qflags = cpu_to_be16(from->sb_qflags);
+		to->sb_gquotino = cpu_to_be64(from->sb_gquotino);
+		to->sb_pquotino = cpu_to_be64(from->sb_pquotino);
+		return;
+	}
+
 	/*
-	 * We need to do these manipilations only if we are working
-	 * with an older version of on-disk superblock.
+	 * The in-core version of sb_qflags do not have XFS_OQUOTA_*
+	 * flags, whereas the on-disk version does.  So, convert incore
+	 * XFS_{PG}QUOTA_* flags to on-disk XFS_OQUOTA_* flags.
 	 */
-	if (xfs_sb_version_has_pquotino(from))
-		return;
+	qflags &= ~(XFS_PQUOTA_ENFD | XFS_PQUOTA_CHKD |
+			XFS_GQUOTA_ENFD | XFS_GQUOTA_CHKD);
 
-	if (*fields & XFS_SB_QFLAGS) {
-		/*
-		 * The in-core version of sb_qflags do not have
-		 * XFS_OQUOTA_* flags, whereas the on-disk version
-		 * does.  So, convert incore XFS_{PG}QUOTA_* flags
-		 * to on-disk XFS_OQUOTA_* flags.
-		 */
-		qflags &= ~(XFS_PQUOTA_ENFD | XFS_PQUOTA_CHKD |
-				XFS_GQUOTA_ENFD | XFS_GQUOTA_CHKD);
-
-		if (from->sb_qflags &
-				(XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD))
-			qflags |= XFS_OQUOTA_ENFD;
-		if (from->sb_qflags &
-				(XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD))
-			qflags |= XFS_OQUOTA_CHKD;
-		to->sb_qflags = cpu_to_be16(qflags);
-		*fields &= ~XFS_SB_QFLAGS;
-	}
+	if (from->sb_qflags &
+			(XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD))
+		qflags |= XFS_OQUOTA_ENFD;
+	if (from->sb_qflags &
+			(XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD))
+		qflags |= XFS_OQUOTA_CHKD;
+	to->sb_qflags = cpu_to_be16(qflags);
 
 	/*
-	 * GQUOTINO and PQUOTINO cannot be used together in versions of
-	 * superblock that do not have pquotino. from->sb_flags tells us which
-	 * quota is active and should be copied to disk. If neither are active,
-	 * make sure we write NULLFSINO to the sb_gquotino field as a quota
-	 * inode value of "0" is invalid when the XFS_SB_VERSION_QUOTA feature
-	 * bit is set.
+	 * GQUOTINO and PQUOTINO cannot be used together in versions
+	 * of superblock that do not have pquotino. from->sb_flags
+	 * tells us which quota is active and should be copied to
+	 * disk. If neither are active, we should NULL the inode.
 	 *
-	 * Note that we don't need to handle the sb_uquotino or sb_pquotino here
-	 * as they do not require any translation. Hence the main sb field loop
-	 * will write them appropriately from the in-core superblock.
+	 * In all cases, the separate pquotino must remain 0 because it
+	 * it beyond the "end" of the valid non-pquotino superblock.
 	 */
-	if ((*fields & XFS_SB_GQUOTINO) &&
-				(from->sb_qflags & XFS_GQUOTA_ACCT))
+	if (from->sb_qflags & XFS_GQUOTA_ACCT)
 		to->sb_gquotino = cpu_to_be64(from->sb_gquotino);
-	else if ((*fields & XFS_SB_PQUOTINO) &&
-				(from->sb_qflags & XFS_PQUOTA_ACCT))
+	else if (from->sb_qflags & XFS_PQUOTA_ACCT)
 		to->sb_gquotino = cpu_to_be64(from->sb_pquotino);
-	else {
-		/*
-		 * We can't rely on just the fields being logged to tell us
-		 * that it is safe to write NULLFSINO - we should only do that
-		 * if quotas are not actually enabled. Hence only write
-		 * NULLFSINO if both in-core quota inodes are NULL.
-		 */
-		if (from->sb_gquotino == NULLFSINO &&
-		    from->sb_pquotino == NULLFSINO)
-			to->sb_gquotino = cpu_to_be64(NULLFSINO);
-	}
+	else
+		to->sb_gquotino = cpu_to_be64(NULLFSINO);
 
-	*fields &= ~(XFS_SB_PQUOTINO | XFS_SB_GQUOTINO);
+	to->sb_pquotino = 0;
 }
 
-/*
- * Copy in core superblock to ondisk one.
- *
- * The fields argument is mask of superblock fields to copy.
- */
 void
 xfs_sb_to_disk(
-	xfs_dsb_t	*to,
-	xfs_sb_t	*from,
-	__int64_t	fields)
+	struct xfs_dsb	*to,
+	struct xfs_sb	*from)
 {
-	xfs_caddr_t	to_ptr = (xfs_caddr_t)to;
-	xfs_caddr_t	from_ptr = (xfs_caddr_t)from;
-	xfs_sb_field_t	f;
-	int		first;
-	int		size;
-
-	ASSERT(fields);
-	if (!fields)
-		return;
+	xfs_sb_quota_to_disk(to, from);
 
-	/* We should never write the crc here, it's updated in the IO path */
-	fields &= ~XFS_SB_CRC;
-
-	xfs_sb_quota_to_disk(to, from, &fields);
-	while (fields) {
-		f = (xfs_sb_field_t)xfs_lowbit64((__uint64_t)fields);
-		first = xfs_sb_info[f].offset;
-		size = xfs_sb_info[f + 1].offset - first;
-
-		ASSERT(xfs_sb_info[f].type == 0 || xfs_sb_info[f].type == 1);
-
-		if (size == 1 || xfs_sb_info[f].type == 1) {
-			memcpy(to_ptr + first, from_ptr + first, size);
-		} else {
-			switch (size) {
-			case 2:
-				*(__be16 *)(to_ptr + first) =
-				      cpu_to_be16(*(__u16 *)(from_ptr + first));
-				break;
-			case 4:
-				*(__be32 *)(to_ptr + first) =
-				      cpu_to_be32(*(__u32 *)(from_ptr + first));
-				break;
-			case 8:
-				*(__be64 *)(to_ptr + first) =
-				      cpu_to_be64(*(__u64 *)(from_ptr + first));
-				break;
-			default:
-				ASSERT(0);
-			}
-		}
+	to->sb_magicnum = cpu_to_be32(from->sb_magicnum);
+	to->sb_blocksize = cpu_to_be32(from->sb_blocksize);
+	to->sb_dblocks = cpu_to_be64(from->sb_dblocks);
+	to->sb_rblocks = cpu_to_be64(from->sb_rblocks);
+	to->sb_rextents = cpu_to_be64(from->sb_rextents);
+	memcpy(&to->sb_uuid, &from->sb_uuid, sizeof(to->sb_uuid));
+	to->sb_logstart = cpu_to_be64(from->sb_logstart);
+	to->sb_rootino = cpu_to_be64(from->sb_rootino);
+	to->sb_rbmino = cpu_to_be64(from->sb_rbmino);
+	to->sb_rsumino = cpu_to_be64(from->sb_rsumino);
+	to->sb_rextsize = cpu_to_be32(from->sb_rextsize);
+	to->sb_agblocks = cpu_to_be32(from->sb_agblocks);
+	to->sb_agcount = cpu_to_be32(from->sb_agcount);
+	to->sb_rbmblocks = cpu_to_be32(from->sb_rbmblocks);
+	to->sb_logblocks = cpu_to_be32(from->sb_logblocks);
+	to->sb_versionnum = cpu_to_be16(from->sb_versionnum);
+	to->sb_sectsize = cpu_to_be16(from->sb_sectsize);
+	to->sb_inodesize = cpu_to_be16(from->sb_inodesize);
+	to->sb_inopblock = cpu_to_be16(from->sb_inopblock);
+	memcpy(&to->sb_fname, &from->sb_fname, sizeof(to->sb_fname));
+	to->sb_blocklog = from->sb_blocklog;
+	to->sb_sectlog = from->sb_sectlog;
+	to->sb_inodelog = from->sb_inodelog;
+	to->sb_inopblog = from->sb_inopblog;
+	to->sb_agblklog = from->sb_agblklog;
+	to->sb_rextslog = from->sb_rextslog;
+	to->sb_inprogress = from->sb_inprogress;
+	to->sb_imax_pct = from->sb_imax_pct;
+	to->sb_icount = cpu_to_be64(from->sb_icount);
+	to->sb_ifree = cpu_to_be64(from->sb_ifree);
+	to->sb_fdblocks = cpu_to_be64(from->sb_fdblocks);
+	to->sb_frextents = cpu_to_be64(from->sb_frextents);
 
-		fields &= ~(1LL << f);
+
+	to->sb_flags = from->sb_flags;
+	to->sb_shared_vn = from->sb_shared_vn;
+	to->sb_inoalignmt = cpu_to_be32(from->sb_inoalignmt);
+	to->sb_unit = cpu_to_be32(from->sb_unit);
+	to->sb_width = cpu_to_be32(from->sb_width);
+	to->sb_dirblklog = from->sb_dirblklog;
+	to->sb_logsectlog = from->sb_logsectlog;
+	to->sb_logsectsize = cpu_to_be16(from->sb_logsectsize);
+	to->sb_logsunit = cpu_to_be32(from->sb_logsunit);
+	to->sb_features2 = cpu_to_be32(from->sb_features2);
+	to->sb_bad_features2 = cpu_to_be32(from->sb_bad_features2);
+
+	if (xfs_sb_version_hascrc(from)) {
+		to->sb_features_compat = cpu_to_be32(from->sb_features_compat);
+		to->sb_features_ro_compat =
+				cpu_to_be32(from->sb_features_ro_compat);
+		to->sb_features_incompat =
+				cpu_to_be32(from->sb_features_incompat);
+		to->sb_features_log_incompat =
+				cpu_to_be32(from->sb_features_log_incompat);
+		to->sb_pad = 0;
+		to->sb_lsn = cpu_to_be64(from->sb_lsn);
 	}
 }
 
@@ -823,35 +751,13 @@ xfs_initialize_perag_data(
  * access.
  */
 void
-xfs_mod_sb(xfs_trans_t *tp, __int64_t fields)
+xfs_mod_sb(
+	struct xfs_trans	*tp)
 {
-	xfs_buf_t	*bp;
-	int		first;
-	int		last;
-	xfs_mount_t	*mp;
-	xfs_sb_field_t	f;
-
-	ASSERT(fields);
-	if (!fields)
-		return;
-	mp = tp->t_mountp;
-	bp = xfs_trans_getsb(tp, mp, 0);
-	first = sizeof(xfs_sb_t);
-	last = 0;
-
-	/* translate/copy */
-
-	xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb, fields);
-
-	/* find modified range */
-	f = (xfs_sb_field_t)xfs_highbit64((__uint64_t)fields);
-	ASSERT((1LL << f) & XFS_SB_MOD_BITS);
-	last = xfs_sb_info[f + 1].offset - 1;
-
-	f = (xfs_sb_field_t)xfs_lowbit64((__uint64_t)fields);
-	ASSERT((1LL << f) & XFS_SB_MOD_BITS);
-	first = xfs_sb_info[f].offset;
+	struct xfs_mount	*mp = tp->t_mountp;
+	struct xfs_buf		*bp = xfs_trans_getsb(tp, mp, 0);
 
+	xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb);
 	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_SB_BUF);
-	xfs_trans_log_buf(tp, bp, first, last);
+	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsb));
 }
diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
index 8eb1c54..e193caa 100644
--- a/fs/xfs/libxfs/xfs_sb.h
+++ b/fs/xfs/libxfs/xfs_sb.h
@@ -27,11 +27,11 @@ extern struct xfs_perag *xfs_perag_get_tag(struct xfs_mount *, xfs_agnumber_t,
 extern void	xfs_perag_put(struct xfs_perag *pag);
 extern int	xfs_initialize_perag_data(struct xfs_mount *, xfs_agnumber_t);
 
-extern void	xfs_sb_calc_crc(struct xfs_buf	*);
-extern void	xfs_mod_sb(struct xfs_trans *, __int64_t);
-extern void	xfs_sb_mount_common(struct xfs_mount *, struct xfs_sb *);
-extern void	xfs_sb_from_disk(struct xfs_sb *, struct xfs_dsb *);
-extern void	xfs_sb_to_disk(struct xfs_dsb *, struct xfs_sb *, __int64_t);
+extern void	xfs_sb_calc_crc(struct xfs_buf *bp);
+extern void	xfs_mod_sb(struct xfs_trans *tp);
+extern void	xfs_sb_mount_common(struct xfs_mount *mp, struct xfs_sb *sbp);
+extern void	xfs_sb_from_disk(struct xfs_sb *to, struct xfs_dsb *from);
+extern void	xfs_sb_to_disk(struct xfs_dsb *to, struct xfs_sb *from);
 extern void	xfs_sb_quota_from_disk(struct xfs_sb *sbp);
 
 #endif	/* __XFS_SB_H__ */
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index fdc6422..82af857 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -541,7 +541,7 @@ xfs_growfs_data_private(
 			saved_error = error;
 			continue;
 		}
-		xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb, XFS_SB_ALL_BITS);
+		xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb);
 
 		error = xfs_bwrite(bp);
 		xfs_buf_relse(bp);
@@ -780,9 +780,7 @@ xfs_fs_log_dummy(
 		xfs_trans_cancel(tp, 0);
 		return error;
 	}
-
-	/* log the UUID because it is an unchanging field */
-	xfs_mod_sb(tp, XFS_SB_UUID);
+	xfs_mod_sb(tp);
 	xfs_trans_set_sync(tp);
 	return xfs_trans_commit(tp, 0);
 }
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 71d2c97..defcf69 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -613,7 +613,7 @@ xfs_mount_reset_sbqflags(
 		return error;
 	}
 
-	xfs_mod_sb(tp, XFS_SB_QFLAGS);
+	xfs_mod_sb(tp);
 	return xfs_trans_commit(tp, 0);
 }
 
@@ -896,7 +896,7 @@ xfs_mountfs(
 	 * perform the update e.g. for the root filesystem.
 	 */
 	if (mp->m_update_flags && !(mp->m_flags & XFS_MOUNT_RDONLY)) {
-		error = xfs_mount_log_sb(mp, mp->m_update_flags);
+		error = xfs_mount_log_sb(mp);
 		if (error) {
 			xfs_warn(mp, "failed to write sb changes");
 			goto out_rtunmount;
@@ -1126,7 +1126,7 @@ xfs_log_sbcount(xfs_mount_t *mp)
 		return error;
 	}
 
-	xfs_mod_sb(tp, XFS_SB_IFREE | XFS_SB_ICOUNT | XFS_SB_FDBLOCKS);
+	xfs_mod_sb(tp);
 	xfs_trans_set_sync(tp);
 	error = xfs_trans_commit(tp, 0);
 	return error;
@@ -1429,14 +1429,10 @@ xfs_freesb(
  */
 int
 xfs_mount_log_sb(
-	xfs_mount_t	*mp,
-	__int64_t	fields)
+	struct xfs_mount	*mp)
 {
-	xfs_trans_t	*tp;
-	int		error;
-
-	ASSERT(fields & (XFS_SB_UNIT | XFS_SB_WIDTH | XFS_SB_UUID |
-			 XFS_SB_FEATURES2 | XFS_SB_VERSIONNUM));
+	struct xfs_trans	*tp;
+	int			error;
 
 	tp = xfs_trans_alloc(mp, XFS_TRANS_SB_UNIT);
 	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_sb, 0, 0);
@@ -1444,9 +1440,8 @@ xfs_mount_log_sb(
 		xfs_trans_cancel(tp, 0);
 		return error;
 	}
-	xfs_mod_sb(tp, fields);
-	error = xfs_trans_commit(tp, 0);
-	return error;
+	xfs_mod_sb(tp);
+	return xfs_trans_commit(tp, 0);
 }
 
 /*
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 22ccf69..28b341b 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -378,7 +378,7 @@ extern void	xfs_unmountfs(xfs_mount_t *);
 extern int	xfs_mod_incore_sb(xfs_mount_t *, xfs_sb_field_t, int64_t, int);
 extern int	xfs_mod_incore_sb_batch(xfs_mount_t *, xfs_mod_sb_t *,
 			uint, int);
-extern int	xfs_mount_log_sb(xfs_mount_t *, __int64_t);
+extern int	xfs_mount_log_sb(xfs_mount_t *);
 extern struct xfs_buf *xfs_getsb(xfs_mount_t *, int);
 extern int	xfs_readsb(xfs_mount_t *, int);
 extern void	xfs_freesb(xfs_mount_t *);
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 79fb19d..c815a80 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -714,7 +714,6 @@ STATIC int
 xfs_qm_qino_alloc(
 	xfs_mount_t	*mp,
 	xfs_inode_t	**ip,
-	__int64_t	sbfields,
 	uint		flags)
 {
 	xfs_trans_t	*tp;
@@ -777,11 +776,6 @@ xfs_qm_qino_alloc(
 	spin_lock(&mp->m_sb_lock);
 	if (flags & XFS_QMOPT_SBVERSION) {
 		ASSERT(!xfs_sb_version_hasquota(&mp->m_sb));
-		ASSERT((sbfields & (XFS_SB_VERSIONNUM | XFS_SB_UQUOTINO |
-			XFS_SB_GQUOTINO | XFS_SB_PQUOTINO | XFS_SB_QFLAGS)) ==
-				(XFS_SB_VERSIONNUM | XFS_SB_UQUOTINO |
-				 XFS_SB_GQUOTINO | XFS_SB_PQUOTINO |
-				 XFS_SB_QFLAGS));
 
 		xfs_sb_version_addquota(&mp->m_sb);
 		mp->m_sb.sb_uquotino = NULLFSINO;
@@ -798,7 +792,7 @@ xfs_qm_qino_alloc(
 	else
 		mp->m_sb.sb_pquotino = (*ip)->i_ino;
 	spin_unlock(&mp->m_sb_lock);
-	xfs_mod_sb(tp, sbfields);
+	xfs_mod_sb(tp);
 
 	if ((error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES))) {
 		xfs_alert(mp, "%s failed (error %d)!", __func__, error);
@@ -1451,7 +1445,7 @@ xfs_qm_mount_quotas(
 	spin_unlock(&mp->m_sb_lock);
 
 	if (sbf != (mp->m_qflags & XFS_MOUNT_QUOTA_ALL)) {
-		if (xfs_qm_write_sb_changes(mp, XFS_SB_QFLAGS)) {
+		if (xfs_qm_write_sb_changes(mp)) {
 			/*
 			 * We could only have been turning quotas off.
 			 * We aren't in very good shape actually because
@@ -1482,7 +1476,6 @@ xfs_qm_init_quotainos(
 	struct xfs_inode	*gip = NULL;
 	struct xfs_inode	*pip = NULL;
 	int			error;
-	__int64_t		sbflags = 0;
 	uint			flags = 0;
 
 	ASSERT(mp->m_quotainfo);
@@ -1517,9 +1510,6 @@ xfs_qm_init_quotainos(
 		}
 	} else {
 		flags |= XFS_QMOPT_SBVERSION;
-		sbflags |= (XFS_SB_VERSIONNUM | XFS_SB_UQUOTINO |
-			    XFS_SB_GQUOTINO | XFS_SB_PQUOTINO |
-			    XFS_SB_QFLAGS);
 	}
 
 	/*
@@ -1530,7 +1520,6 @@ xfs_qm_init_quotainos(
 	 */
 	if (XFS_IS_UQUOTA_ON(mp) && uip == NULL) {
 		error = xfs_qm_qino_alloc(mp, &uip,
-					      sbflags | XFS_SB_UQUOTINO,
 					      flags | XFS_QMOPT_UQUOTA);
 		if (error)
 			goto error_rele;
@@ -1539,7 +1528,6 @@ xfs_qm_init_quotainos(
 	}
 	if (XFS_IS_GQUOTA_ON(mp) && gip == NULL) {
 		error = xfs_qm_qino_alloc(mp, &gip,
-					  sbflags | XFS_SB_GQUOTINO,
 					  flags | XFS_QMOPT_GQUOTA);
 		if (error)
 			goto error_rele;
@@ -1548,7 +1536,6 @@ xfs_qm_init_quotainos(
 	}
 	if (XFS_IS_PQUOTA_ON(mp) && pip == NULL) {
 		error = xfs_qm_qino_alloc(mp, &pip,
-					  sbflags | XFS_SB_PQUOTINO,
 					  flags | XFS_QMOPT_PQUOTA);
 		if (error)
 			goto error_rele;
@@ -1593,8 +1580,7 @@ xfs_qm_dqfree_one(
  */
 int
 xfs_qm_write_sb_changes(
-	xfs_mount_t	*mp,
-	__int64_t	flags)
+	struct xfs_mount *mp)
 {
 	xfs_trans_t	*tp;
 	int		error;
@@ -1606,10 +1592,8 @@ xfs_qm_write_sb_changes(
 		return error;
 	}
 
-	xfs_mod_sb(tp, flags);
-	error = xfs_trans_commit(tp, 0);
-
-	return error;
+	xfs_mod_sb(tp);
+	return xfs_trans_commit(tp, 0);
 }
 
 
diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
index 3a07a93..bddd23f 100644
--- a/fs/xfs/xfs_qm.h
+++ b/fs/xfs/xfs_qm.h
@@ -157,7 +157,7 @@ struct xfs_dquot_acct {
 #define XFS_QM_RTBWARNLIMIT	5
 
 extern void		xfs_qm_destroy_quotainfo(struct xfs_mount *);
-extern int		xfs_qm_write_sb_changes(struct xfs_mount *, __int64_t);
+extern int		xfs_qm_write_sb_changes(struct xfs_mount *);
 
 /* dquot stuff */
 extern void		xfs_qm_dqpurge_all(struct xfs_mount *, uint);
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 74fca68..8d7e5f0 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -92,8 +92,7 @@ xfs_qm_scall_quotaoff(
 		mutex_unlock(&q->qi_quotaofflock);
 
 		/* XXX what to do if error ? Revert back to old vals incore ? */
-		error = xfs_qm_write_sb_changes(mp, XFS_SB_QFLAGS);
-		return error;
+		return xfs_qm_write_sb_changes(mp);
 	}
 
 	dqtype = 0;
@@ -314,7 +313,6 @@ xfs_qm_scall_quotaon(
 {
 	int		error;
 	uint		qf;
-	__int64_t	sbflags;
 
 	flags &= (XFS_ALL_QUOTA_ACCT | XFS_ALL_QUOTA_ENFD);
 	/*
@@ -322,8 +320,6 @@ xfs_qm_scall_quotaon(
 	 */
 	flags &= ~(XFS_ALL_QUOTA_ACCT);
 
-	sbflags = 0;
-
 	if (flags == 0) {
 		xfs_debug(mp, "%s: zero flags, m_qflags=%x",
 			__func__, mp->m_qflags);
@@ -370,11 +366,10 @@ xfs_qm_scall_quotaon(
 	/*
 	 * There's nothing to change if it's the same.
 	 */
-	if ((qf & flags) == flags && sbflags == 0)
+	if ((qf & flags) == flags)
 		return -EEXIST;
-	sbflags |= XFS_SB_QFLAGS;
 
-	if ((error = xfs_qm_write_sb_changes(mp, sbflags)))
+	if ((error = xfs_qm_write_sb_changes(mp)))
 		return error;
 	/*
 	 * If we aren't trying to switch on quota enforcement, we are done.
@@ -801,7 +796,7 @@ xfs_qm_log_quotaoff(
 	mp->m_sb.sb_qflags = (mp->m_qflags & ~(flags)) & XFS_MOUNT_QUOTA_ALL;
 	spin_unlock(&mp->m_sb_lock);
 
-	xfs_mod_sb(tp, XFS_SB_QFLAGS);
+	xfs_mod_sb(tp);
 
 	/*
 	 * We have to make sure that the transaction is secure on disk before we
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 40bad75..3797a03 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1258,7 +1258,7 @@ xfs_fs_remount(
 		 * might have some superblock changes to update.
 		 */
 		if (mp->m_update_flags) {
-			error = xfs_mount_log_sb(mp, mp->m_update_flags);
+			error = xfs_mount_log_sb(mp);
 			if (error) {
 				xfs_warn(mp, "failed to write sb changes");
 				return error;
-- 
2.0.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/3] xfs: remove bitfield based superblock updates

[Brian Foster]

On Thu, Jan 08, 2015 at 08:51:03AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> When we log changes to the superblock, we first have to write them
> to the on-disk buffer, and then log that. Right now we have a
> complex bitfield based arrangement to only write the modified field
> to the buffer before we log it.
> 
> This used to be necessary as a performance optimisation because we
> logged the superblock buffer in every extent or inode allocation or
> freeing, and so performance was extremely important. We haven't done
> this for years, however, ever since the lazy superblock counters
> pulled the superblock logging out of the transaction commit
> fast path.
> 
> Hence we have a bunch of complexity that is not necessary that makes
> writing the in-core superblock to disk much more complex than it
> needs to be. We only need to log the superblock now during
> management operations (e.g. during mount, unmount or quota control
> operations) so it is not a performance critical path anymore.
> 
> As such, remove the complex field based logging mechanism and
> replace it with a simple conversion function similar to what we use
> for all other on-disk structures.
> 
> This means we always log the entirity of the superblock, but again
> because we rarely modify the superblock this is not an issue for log
> bandwidth or CPU time. Indeed, if we do log the superblock
> frequently, delayed logging will minimise the impact of this
> overhead.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c |   2 +-
>  fs/xfs/libxfs/xfs_bmap.c      |  14 +-
>  fs/xfs/libxfs/xfs_sb.c        | 290 ++++++++++++++----------------------------
>  fs/xfs/libxfs/xfs_sb.h        |  10 +-
>  fs/xfs/xfs_fsops.c            |   6 +-
>  fs/xfs/xfs_mount.c            |  21 ++-
>  fs/xfs/xfs_mount.h            |   2 +-
>  fs/xfs/xfs_qm.c               |  26 +---
>  fs/xfs/xfs_qm.h               |   2 +-
>  fs/xfs/xfs_qm_syscalls.c      |  13 +-
>  fs/xfs/xfs_super.c            |   2 +-
>  11 files changed, 132 insertions(+), 256 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 5d38e8b..c914422 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -403,7 +403,7 @@ xfs_sbversion_add_attr2(xfs_mount_t *mp, xfs_trans_t *tp)
>  		if (!xfs_sb_version_hasattr2(&mp->m_sb)) {
>  			xfs_sb_version_addattr2(&mp->m_sb);
>  			spin_unlock(&mp->m_sb_lock);
> -			xfs_mod_sb(tp, XFS_SB_VERSIONNUM | XFS_SB_FEATURES2);
> +			xfs_mod_sb(tp);
>  		} else
>  			spin_unlock(&mp->m_sb_lock);
>  	}
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index b5eb474..8c39cc8 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1221,22 +1221,20 @@ xfs_bmap_add_attrfork(
>  		goto bmap_cancel;
>  	if (!xfs_sb_version_hasattr(&mp->m_sb) ||
>  	   (!xfs_sb_version_hasattr2(&mp->m_sb) && version == 2)) {
> -		__int64_t sbfields = 0;
> +		bool mod_sb = false;
>  
>  		spin_lock(&mp->m_sb_lock);
>  		if (!xfs_sb_version_hasattr(&mp->m_sb)) {
>  			xfs_sb_version_addattr(&mp->m_sb);
> -			sbfields |= XFS_SB_VERSIONNUM;
> +			mod_sb = true;
>  		}
>  		if (!xfs_sb_version_hasattr2(&mp->m_sb) && version == 2) {
>  			xfs_sb_version_addattr2(&mp->m_sb);
> -			sbfields |= (XFS_SB_VERSIONNUM | XFS_SB_FEATURES2);
> +			mod_sb = true;
>  		}
> -		if (sbfields) {
> -			spin_unlock(&mp->m_sb_lock);
> -			xfs_mod_sb(tp, sbfields);
> -		} else
> -			spin_unlock(&mp->m_sb_lock);
> +		spin_unlock(&mp->m_sb_lock);
> +		if (mod_sb)
> +			xfs_mod_sb(tp);
>  	}
>  
>  	error = xfs_bmap_finish(&tp, &flist, &committed);
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 752915f..4afbbce 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -40,69 +40,6 @@
>   * Physical superblock buffer manipulations. Shared with libxfs in userspace.
>   */
>  
> -static const struct {
> -	short offset;
> -	short type;	/* 0 = integer
> -			 * 1 = binary / string (no translation)
> -			 */
> -} xfs_sb_info[] = {
> -	{ offsetof(xfs_sb_t, sb_magicnum),	0 },
> -	{ offsetof(xfs_sb_t, sb_blocksize),	0 },
> -	{ offsetof(xfs_sb_t, sb_dblocks),	0 },
> -	{ offsetof(xfs_sb_t, sb_rblocks),	0 },
> -	{ offsetof(xfs_sb_t, sb_rextents),	0 },
> -	{ offsetof(xfs_sb_t, sb_uuid),		1 },
> -	{ offsetof(xfs_sb_t, sb_logstart),	0 },
> -	{ offsetof(xfs_sb_t, sb_rootino),	0 },
> -	{ offsetof(xfs_sb_t, sb_rbmino),	0 },
> -	{ offsetof(xfs_sb_t, sb_rsumino),	0 },
> -	{ offsetof(xfs_sb_t, sb_rextsize),	0 },
> -	{ offsetof(xfs_sb_t, sb_agblocks),	0 },
> -	{ offsetof(xfs_sb_t, sb_agcount),	0 },
> -	{ offsetof(xfs_sb_t, sb_rbmblocks),	0 },
> -	{ offsetof(xfs_sb_t, sb_logblocks),	0 },
> -	{ offsetof(xfs_sb_t, sb_versionnum),	0 },
> -	{ offsetof(xfs_sb_t, sb_sectsize),	0 },
> -	{ offsetof(xfs_sb_t, sb_inodesize),	0 },
> -	{ offsetof(xfs_sb_t, sb_inopblock),	0 },
> -	{ offsetof(xfs_sb_t, sb_fname[0]),	1 },
> -	{ offsetof(xfs_sb_t, sb_blocklog),	0 },
> -	{ offsetof(xfs_sb_t, sb_sectlog),	0 },
> -	{ offsetof(xfs_sb_t, sb_inodelog),	0 },
> -	{ offsetof(xfs_sb_t, sb_inopblog),	0 },
> -	{ offsetof(xfs_sb_t, sb_agblklog),	0 },
> -	{ offsetof(xfs_sb_t, sb_rextslog),	0 },
> -	{ offsetof(xfs_sb_t, sb_inprogress),	0 },
> -	{ offsetof(xfs_sb_t, sb_imax_pct),	0 },
> -	{ offsetof(xfs_sb_t, sb_icount),	0 },
> -	{ offsetof(xfs_sb_t, sb_ifree),		0 },
> -	{ offsetof(xfs_sb_t, sb_fdblocks),	0 },
> -	{ offsetof(xfs_sb_t, sb_frextents),	0 },
> -	{ offsetof(xfs_sb_t, sb_uquotino),	0 },
> -	{ offsetof(xfs_sb_t, sb_gquotino),	0 },
> -	{ offsetof(xfs_sb_t, sb_qflags),	0 },
> -	{ offsetof(xfs_sb_t, sb_flags),		0 },
> -	{ offsetof(xfs_sb_t, sb_shared_vn),	0 },
> -	{ offsetof(xfs_sb_t, sb_inoalignmt),	0 },
> -	{ offsetof(xfs_sb_t, sb_unit),		0 },
> -	{ offsetof(xfs_sb_t, sb_width),		0 },
> -	{ offsetof(xfs_sb_t, sb_dirblklog),	0 },
> -	{ offsetof(xfs_sb_t, sb_logsectlog),	0 },
> -	{ offsetof(xfs_sb_t, sb_logsectsize),	0 },
> -	{ offsetof(xfs_sb_t, sb_logsunit),	0 },
> -	{ offsetof(xfs_sb_t, sb_features2),	0 },
> -	{ offsetof(xfs_sb_t, sb_bad_features2),	0 },
> -	{ offsetof(xfs_sb_t, sb_features_compat),	0 },
> -	{ offsetof(xfs_sb_t, sb_features_ro_compat),	0 },
> -	{ offsetof(xfs_sb_t, sb_features_incompat),	0 },
> -	{ offsetof(xfs_sb_t, sb_features_log_incompat),	0 },
> -	{ offsetof(xfs_sb_t, sb_crc),		0 },
> -	{ offsetof(xfs_sb_t, sb_pad),		0 },
> -	{ offsetof(xfs_sb_t, sb_pquotino),	0 },
> -	{ offsetof(xfs_sb_t, sb_lsn),		0 },
> -	{ sizeof(xfs_sb_t),			0 }
> -};
> -
>  /*
>   * Reference counting access wrappers to the perag structures.
>   * Because we never free per-ag structures, the only thing we
> @@ -461,128 +398,119 @@ xfs_sb_from_disk(
>  	__xfs_sb_from_disk(to, from, true);
>  }
>  
> -static inline void
> +static void
>  xfs_sb_quota_to_disk(
> -	xfs_dsb_t	*to,
> -	xfs_sb_t	*from,
> -	__int64_t	*fields)
> +	struct xfs_dsb	*to,
> +	struct xfs_sb	*from)
>  {
>  	__uint16_t	qflags = from->sb_qflags;
>  
> +	to->sb_uquotino = cpu_to_be64(from->sb_uquotino);
> +	if (xfs_sb_version_has_pquotino(from)) {
> +		to->sb_qflags = cpu_to_be16(from->sb_qflags);
> +		to->sb_gquotino = cpu_to_be64(from->sb_gquotino);
> +		to->sb_pquotino = cpu_to_be64(from->sb_pquotino);
> +		return;
> +	}
> +
>  	/*
> -	 * We need to do these manipilations only if we are working
> -	 * with an older version of on-disk superblock.
> +	 * The in-core version of sb_qflags do not have XFS_OQUOTA_*
> +	 * flags, whereas the on-disk version does.  So, convert incore
> +	 * XFS_{PG}QUOTA_* flags to on-disk XFS_OQUOTA_* flags.
>  	 */
> -	if (xfs_sb_version_has_pquotino(from))
> -		return;
> +	qflags &= ~(XFS_PQUOTA_ENFD | XFS_PQUOTA_CHKD |
> +			XFS_GQUOTA_ENFD | XFS_GQUOTA_CHKD);
>  
> -	if (*fields & XFS_SB_QFLAGS) {
> -		/*
> -		 * The in-core version of sb_qflags do not have
> -		 * XFS_OQUOTA_* flags, whereas the on-disk version
> -		 * does.  So, convert incore XFS_{PG}QUOTA_* flags
> -		 * to on-disk XFS_OQUOTA_* flags.
> -		 */
> -		qflags &= ~(XFS_PQUOTA_ENFD | XFS_PQUOTA_CHKD |
> -				XFS_GQUOTA_ENFD | XFS_GQUOTA_CHKD);
> -
> -		if (from->sb_qflags &
> -				(XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD))
> -			qflags |= XFS_OQUOTA_ENFD;
> -		if (from->sb_qflags &
> -				(XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD))
> -			qflags |= XFS_OQUOTA_CHKD;
> -		to->sb_qflags = cpu_to_be16(qflags);
> -		*fields &= ~XFS_SB_QFLAGS;
> -	}
> +	if (from->sb_qflags &
> +			(XFS_PQUOTA_ENFD | XFS_GQUOTA_ENFD))
> +		qflags |= XFS_OQUOTA_ENFD;
> +	if (from->sb_qflags &
> +			(XFS_PQUOTA_CHKD | XFS_GQUOTA_CHKD))
> +		qflags |= XFS_OQUOTA_CHKD;
> +	to->sb_qflags = cpu_to_be16(qflags);
>  
>  	/*
> -	 * GQUOTINO and PQUOTINO cannot be used together in versions of
> -	 * superblock that do not have pquotino. from->sb_flags tells us which
> -	 * quota is active and should be copied to disk. If neither are active,
> -	 * make sure we write NULLFSINO to the sb_gquotino field as a quota
> -	 * inode value of "0" is invalid when the XFS_SB_VERSION_QUOTA feature
> -	 * bit is set.
> +	 * GQUOTINO and PQUOTINO cannot be used together in versions
> +	 * of superblock that do not have pquotino. from->sb_flags
> +	 * tells us which quota is active and should be copied to
> +	 * disk. If neither are active, we should NULL the inode.
>  	 *
> -	 * Note that we don't need to handle the sb_uquotino or sb_pquotino here
> -	 * as they do not require any translation. Hence the main sb field loop
> -	 * will write them appropriately from the in-core superblock.
> +	 * In all cases, the separate pquotino must remain 0 because it
> +	 * it beyond the "end" of the valid non-pquotino superblock.
>  	 */
> -	if ((*fields & XFS_SB_GQUOTINO) &&
> -				(from->sb_qflags & XFS_GQUOTA_ACCT))
> +	if (from->sb_qflags & XFS_GQUOTA_ACCT)
>  		to->sb_gquotino = cpu_to_be64(from->sb_gquotino);
> -	else if ((*fields & XFS_SB_PQUOTINO) &&
> -				(from->sb_qflags & XFS_PQUOTA_ACCT))
> +	else if (from->sb_qflags & XFS_PQUOTA_ACCT)
>  		to->sb_gquotino = cpu_to_be64(from->sb_pquotino);
> -	else {
> -		/*
> -		 * We can't rely on just the fields being logged to tell us
> -		 * that it is safe to write NULLFSINO - we should only do that
> -		 * if quotas are not actually enabled. Hence only write
> -		 * NULLFSINO if both in-core quota inodes are NULL.
> -		 */
> -		if (from->sb_gquotino == NULLFSINO &&
> -		    from->sb_pquotino == NULLFSINO)
> -			to->sb_gquotino = cpu_to_be64(NULLFSINO);
> -	}
> +	else
> +		to->sb_gquotino = cpu_to_be64(NULLFSINO);

FYI... it looks like the above hunk causes a regression due to resetting
sb_gquotaino when one of the in-core inodes is set. I'm seeing
disconnected quota inode messages on some xfstests (e.g., xfs/108) on v4
filesystems. I'm about to put a patch on the list to go back to the
original logic...

Brian

>  
> -	*fields &= ~(XFS_SB_PQUOTINO | XFS_SB_GQUOTINO);
> +	to->sb_pquotino = 0;
>  }
>  
> -/*
> - * Copy in core superblock to ondisk one.
> - *
> - * The fields argument is mask of superblock fields to copy.
> - */
>  void
>  xfs_sb_to_disk(
> -	xfs_dsb_t	*to,
> -	xfs_sb_t	*from,
> -	__int64_t	fields)
> +	struct xfs_dsb	*to,
> +	struct xfs_sb	*from)
>  {
> -	xfs_caddr_t	to_ptr = (xfs_caddr_t)to;
> -	xfs_caddr_t	from_ptr = (xfs_caddr_t)from;
> -	xfs_sb_field_t	f;
> -	int		first;
> -	int		size;
> -
> -	ASSERT(fields);
> -	if (!fields)
> -		return;
> +	xfs_sb_quota_to_disk(to, from);
>  
> -	/* We should never write the crc here, it's updated in the IO path */
> -	fields &= ~XFS_SB_CRC;
> -
> -	xfs_sb_quota_to_disk(to, from, &fields);
> -	while (fields) {
> -		f = (xfs_sb_field_t)xfs_lowbit64((__uint64_t)fields);
> -		first = xfs_sb_info[f].offset;
> -		size = xfs_sb_info[f + 1].offset - first;
> -
> -		ASSERT(xfs_sb_info[f].type == 0 || xfs_sb_info[f].type == 1);
> -
> -		if (size == 1 || xfs_sb_info[f].type == 1) {
> -			memcpy(to_ptr + first, from_ptr + first, size);
> -		} else {
> -			switch (size) {
> -			case 2:
> -				*(__be16 *)(to_ptr + first) =
> -				      cpu_to_be16(*(__u16 *)(from_ptr + first));
> -				break;
> -			case 4:
> -				*(__be32 *)(to_ptr + first) =
> -				      cpu_to_be32(*(__u32 *)(from_ptr + first));
> -				break;
> -			case 8:
> -				*(__be64 *)(to_ptr + first) =
> -				      cpu_to_be64(*(__u64 *)(from_ptr + first));
> -				break;
> -			default:
> -				ASSERT(0);
> -			}
> -		}
> +	to->sb_magicnum = cpu_to_be32(from->sb_magicnum);
> +	to->sb_blocksize = cpu_to_be32(from->sb_blocksize);
> +	to->sb_dblocks = cpu_to_be64(from->sb_dblocks);
> +	to->sb_rblocks = cpu_to_be64(from->sb_rblocks);
> +	to->sb_rextents = cpu_to_be64(from->sb_rextents);
> +	memcpy(&to->sb_uuid, &from->sb_uuid, sizeof(to->sb_uuid));
> +	to->sb_logstart = cpu_to_be64(from->sb_logstart);
> +	to->sb_rootino = cpu_to_be64(from->sb_rootino);
> +	to->sb_rbmino = cpu_to_be64(from->sb_rbmino);
> +	to->sb_rsumino = cpu_to_be64(from->sb_rsumino);
> +	to->sb_rextsize = cpu_to_be32(from->sb_rextsize);
> +	to->sb_agblocks = cpu_to_be32(from->sb_agblocks);
> +	to->sb_agcount = cpu_to_be32(from->sb_agcount);
> +	to->sb_rbmblocks = cpu_to_be32(from->sb_rbmblocks);
> +	to->sb_logblocks = cpu_to_be32(from->sb_logblocks);
> +	to->sb_versionnum = cpu_to_be16(from->sb_versionnum);
> +	to->sb_sectsize = cpu_to_be16(from->sb_sectsize);
> +	to->sb_inodesize = cpu_to_be16(from->sb_inodesize);
> +	to->sb_inopblock = cpu_to_be16(from->sb_inopblock);
> +	memcpy(&to->sb_fname, &from->sb_fname, sizeof(to->sb_fname));
> +	to->sb_blocklog = from->sb_blocklog;
> +	to->sb_sectlog = from->sb_sectlog;
> +	to->sb_inodelog = from->sb_inodelog;
> +	to->sb_inopblog = from->sb_inopblog;
> +	to->sb_agblklog = from->sb_agblklog;
> +	to->sb_rextslog = from->sb_rextslog;
> +	to->sb_inprogress = from->sb_inprogress;
> +	to->sb_imax_pct = from->sb_imax_pct;
> +	to->sb_icount = cpu_to_be64(from->sb_icount);
> +	to->sb_ifree = cpu_to_be64(from->sb_ifree);
> +	to->sb_fdblocks = cpu_to_be64(from->sb_fdblocks);
> +	to->sb_frextents = cpu_to_be64(from->sb_frextents);
>  
> -		fields &= ~(1LL << f);
> +
> +	to->sb_flags = from->sb_flags;
> +	to->sb_shared_vn = from->sb_shared_vn;
> +	to->sb_inoalignmt = cpu_to_be32(from->sb_inoalignmt);
> +	to->sb_unit = cpu_to_be32(from->sb_unit);
> +	to->sb_width = cpu_to_be32(from->sb_width);
> +	to->sb_dirblklog = from->sb_dirblklog;
> +	to->sb_logsectlog = from->sb_logsectlog;
> +	to->sb_logsectsize = cpu_to_be16(from->sb_logsectsize);
> +	to->sb_logsunit = cpu_to_be32(from->sb_logsunit);
> +	to->sb_features2 = cpu_to_be32(from->sb_features2);
> +	to->sb_bad_features2 = cpu_to_be32(from->sb_bad_features2);
> +
> +	if (xfs_sb_version_hascrc(from)) {
> +		to->sb_features_compat = cpu_to_be32(from->sb_features_compat);
> +		to->sb_features_ro_compat =
> +				cpu_to_be32(from->sb_features_ro_compat);
> +		to->sb_features_incompat =
> +				cpu_to_be32(from->sb_features_incompat);
> +		to->sb_features_log_incompat =
> +				cpu_to_be32(from->sb_features_log_incompat);
> +		to->sb_pad = 0;
> +		to->sb_lsn = cpu_to_be64(from->sb_lsn);
>  	}
>  }
>  
> @@ -823,35 +751,13 @@ xfs_initialize_perag_data(
>   * access.
>   */
>  void
> -xfs_mod_sb(xfs_trans_t *tp, __int64_t fields)
> +xfs_mod_sb(
> +	struct xfs_trans	*tp)
>  {
> -	xfs_buf_t	*bp;
> -	int		first;
> -	int		last;
> -	xfs_mount_t	*mp;
> -	xfs_sb_field_t	f;
> -
> -	ASSERT(fields);
> -	if (!fields)
> -		return;
> -	mp = tp->t_mountp;
> -	bp = xfs_trans_getsb(tp, mp, 0);
> -	first = sizeof(xfs_sb_t);
> -	last = 0;
> -
> -	/* translate/copy */
> -
> -	xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb, fields);
> -
> -	/* find modified range */
> -	f = (xfs_sb_field_t)xfs_highbit64((__uint64_t)fields);
> -	ASSERT((1LL << f) & XFS_SB_MOD_BITS);
> -	last = xfs_sb_info[f + 1].offset - 1;
> -
> -	f = (xfs_sb_field_t)xfs_lowbit64((__uint64_t)fields);
> -	ASSERT((1LL << f) & XFS_SB_MOD_BITS);
> -	first = xfs_sb_info[f].offset;
> +	struct xfs_mount	*mp = tp->t_mountp;
> +	struct xfs_buf		*bp = xfs_trans_getsb(tp, mp, 0);
>  
> +	xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb);
>  	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_SB_BUF);
> -	xfs_trans_log_buf(tp, bp, first, last);
> +	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsb));
>  }
> diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
> index 8eb1c54..e193caa 100644
> --- a/fs/xfs/libxfs/xfs_sb.h
> +++ b/fs/xfs/libxfs/xfs_sb.h
> @@ -27,11 +27,11 @@ extern struct xfs_perag *xfs_perag_get_tag(struct xfs_mount *, xfs_agnumber_t,
>  extern void	xfs_perag_put(struct xfs_perag *pag);
>  extern int	xfs_initialize_perag_data(struct xfs_mount *, xfs_agnumber_t);
>  
> -extern void	xfs_sb_calc_crc(struct xfs_buf	*);
> -extern void	xfs_mod_sb(struct xfs_trans *, __int64_t);
> -extern void	xfs_sb_mount_common(struct xfs_mount *, struct xfs_sb *);
> -extern void	xfs_sb_from_disk(struct xfs_sb *, struct xfs_dsb *);
> -extern void	xfs_sb_to_disk(struct xfs_dsb *, struct xfs_sb *, __int64_t);
> +extern void	xfs_sb_calc_crc(struct xfs_buf *bp);
> +extern void	xfs_mod_sb(struct xfs_trans *tp);
> +extern void	xfs_sb_mount_common(struct xfs_mount *mp, struct xfs_sb *sbp);
> +extern void	xfs_sb_from_disk(struct xfs_sb *to, struct xfs_dsb *from);
> +extern void	xfs_sb_to_disk(struct xfs_dsb *to, struct xfs_sb *from);
>  extern void	xfs_sb_quota_from_disk(struct xfs_sb *sbp);
>  
>  #endif	/* __XFS_SB_H__ */
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index fdc6422..82af857 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -541,7 +541,7 @@ xfs_growfs_data_private(
>  			saved_error = error;
>  			continue;
>  		}
> -		xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb, XFS_SB_ALL_BITS);
> +		xfs_sb_to_disk(XFS_BUF_TO_SBP(bp), &mp->m_sb);
>  
>  		error = xfs_bwrite(bp);
>  		xfs_buf_relse(bp);
> @@ -780,9 +780,7 @@ xfs_fs_log_dummy(
>  		xfs_trans_cancel(tp, 0);
>  		return error;
>  	}
> -
> -	/* log the UUID because it is an unchanging field */
> -	xfs_mod_sb(tp, XFS_SB_UUID);
> +	xfs_mod_sb(tp);
>  	xfs_trans_set_sync(tp);
>  	return xfs_trans_commit(tp, 0);
>  }
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 71d2c97..defcf69 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -613,7 +613,7 @@ xfs_mount_reset_sbqflags(
>  		return error;
>  	}
>  
> -	xfs_mod_sb(tp, XFS_SB_QFLAGS);
> +	xfs_mod_sb(tp);
>  	return xfs_trans_commit(tp, 0);
>  }
>  
> @@ -896,7 +896,7 @@ xfs_mountfs(
>  	 * perform the update e.g. for the root filesystem.
>  	 */
>  	if (mp->m_update_flags && !(mp->m_flags & XFS_MOUNT_RDONLY)) {
> -		error = xfs_mount_log_sb(mp, mp->m_update_flags);
> +		error = xfs_mount_log_sb(mp);
>  		if (error) {
>  			xfs_warn(mp, "failed to write sb changes");
>  			goto out_rtunmount;
> @@ -1126,7 +1126,7 @@ xfs_log_sbcount(xfs_mount_t *mp)
>  		return error;
>  	}
>  
> -	xfs_mod_sb(tp, XFS_SB_IFREE | XFS_SB_ICOUNT | XFS_SB_FDBLOCKS);
> +	xfs_mod_sb(tp);
>  	xfs_trans_set_sync(tp);
>  	error = xfs_trans_commit(tp, 0);
>  	return error;
> @@ -1429,14 +1429,10 @@ xfs_freesb(
>   */
>  int
>  xfs_mount_log_sb(
> -	xfs_mount_t	*mp,
> -	__int64_t	fields)
> +	struct xfs_mount	*mp)
>  {
> -	xfs_trans_t	*tp;
> -	int		error;
> -
> -	ASSERT(fields & (XFS_SB_UNIT | XFS_SB_WIDTH | XFS_SB_UUID |
> -			 XFS_SB_FEATURES2 | XFS_SB_VERSIONNUM));
> +	struct xfs_trans	*tp;
> +	int			error;
>  
>  	tp = xfs_trans_alloc(mp, XFS_TRANS_SB_UNIT);
>  	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_sb, 0, 0);
> @@ -1444,9 +1440,8 @@ xfs_mount_log_sb(
>  		xfs_trans_cancel(tp, 0);
>  		return error;
>  	}
> -	xfs_mod_sb(tp, fields);
> -	error = xfs_trans_commit(tp, 0);
> -	return error;
> +	xfs_mod_sb(tp);
> +	return xfs_trans_commit(tp, 0);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 22ccf69..28b341b 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -378,7 +378,7 @@ extern void	xfs_unmountfs(xfs_mount_t *);
>  extern int	xfs_mod_incore_sb(xfs_mount_t *, xfs_sb_field_t, int64_t, int);
>  extern int	xfs_mod_incore_sb_batch(xfs_mount_t *, xfs_mod_sb_t *,
>  			uint, int);
> -extern int	xfs_mount_log_sb(xfs_mount_t *, __int64_t);
> +extern int	xfs_mount_log_sb(xfs_mount_t *);
>  extern struct xfs_buf *xfs_getsb(xfs_mount_t *, int);
>  extern int	xfs_readsb(xfs_mount_t *, int);
>  extern void	xfs_freesb(xfs_mount_t *);
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 79fb19d..c815a80 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -714,7 +714,6 @@ STATIC int
>  xfs_qm_qino_alloc(
>  	xfs_mount_t	*mp,
>  	xfs_inode_t	**ip,
> -	__int64_t	sbfields,
>  	uint		flags)
>  {
>  	xfs_trans_t	*tp;
> @@ -777,11 +776,6 @@ xfs_qm_qino_alloc(
>  	spin_lock(&mp->m_sb_lock);
>  	if (flags & XFS_QMOPT_SBVERSION) {
>  		ASSERT(!xfs_sb_version_hasquota(&mp->m_sb));
> -		ASSERT((sbfields & (XFS_SB_VERSIONNUM | XFS_SB_UQUOTINO |
> -			XFS_SB_GQUOTINO | XFS_SB_PQUOTINO | XFS_SB_QFLAGS)) ==
> -				(XFS_SB_VERSIONNUM | XFS_SB_UQUOTINO |
> -				 XFS_SB_GQUOTINO | XFS_SB_PQUOTINO |
> -				 XFS_SB_QFLAGS));
>  
>  		xfs_sb_version_addquota(&mp->m_sb);
>  		mp->m_sb.sb_uquotino = NULLFSINO;
> @@ -798,7 +792,7 @@ xfs_qm_qino_alloc(
>  	else
>  		mp->m_sb.sb_pquotino = (*ip)->i_ino;
>  	spin_unlock(&mp->m_sb_lock);
> -	xfs_mod_sb(tp, sbfields);
> +	xfs_mod_sb(tp);
>  
>  	if ((error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES))) {
>  		xfs_alert(mp, "%s failed (error %d)!", __func__, error);
> @@ -1451,7 +1445,7 @@ xfs_qm_mount_quotas(
>  	spin_unlock(&mp->m_sb_lock);
>  
>  	if (sbf != (mp->m_qflags & XFS_MOUNT_QUOTA_ALL)) {
> -		if (xfs_qm_write_sb_changes(mp, XFS_SB_QFLAGS)) {
> +		if (xfs_qm_write_sb_changes(mp)) {
>  			/*
>  			 * We could only have been turning quotas off.
>  			 * We aren't in very good shape actually because
> @@ -1482,7 +1476,6 @@ xfs_qm_init_quotainos(
>  	struct xfs_inode	*gip = NULL;
>  	struct xfs_inode	*pip = NULL;
>  	int			error;
> -	__int64_t		sbflags = 0;
>  	uint			flags = 0;
>  
>  	ASSERT(mp->m_quotainfo);
> @@ -1517,9 +1510,6 @@ xfs_qm_init_quotainos(
>  		}
>  	} else {
>  		flags |= XFS_QMOPT_SBVERSION;
> -		sbflags |= (XFS_SB_VERSIONNUM | XFS_SB_UQUOTINO |
> -			    XFS_SB_GQUOTINO | XFS_SB_PQUOTINO |
> -			    XFS_SB_QFLAGS);
>  	}
>  
>  	/*
> @@ -1530,7 +1520,6 @@ xfs_qm_init_quotainos(
>  	 */
>  	if (XFS_IS_UQUOTA_ON(mp) && uip == NULL) {
>  		error = xfs_qm_qino_alloc(mp, &uip,
> -					      sbflags | XFS_SB_UQUOTINO,
>  					      flags | XFS_QMOPT_UQUOTA);
>  		if (error)
>  			goto error_rele;
> @@ -1539,7 +1528,6 @@ xfs_qm_init_quotainos(
>  	}
>  	if (XFS_IS_GQUOTA_ON(mp) && gip == NULL) {
>  		error = xfs_qm_qino_alloc(mp, &gip,
> -					  sbflags | XFS_SB_GQUOTINO,
>  					  flags | XFS_QMOPT_GQUOTA);
>  		if (error)
>  			goto error_rele;
> @@ -1548,7 +1536,6 @@ xfs_qm_init_quotainos(
>  	}
>  	if (XFS_IS_PQUOTA_ON(mp) && pip == NULL) {
>  		error = xfs_qm_qino_alloc(mp, &pip,
> -					  sbflags | XFS_SB_PQUOTINO,
>  					  flags | XFS_QMOPT_PQUOTA);
>  		if (error)
>  			goto error_rele;
> @@ -1593,8 +1580,7 @@ xfs_qm_dqfree_one(
>   */
>  int
>  xfs_qm_write_sb_changes(
> -	xfs_mount_t	*mp,
> -	__int64_t	flags)
> +	struct xfs_mount *mp)
>  {
>  	xfs_trans_t	*tp;
>  	int		error;
> @@ -1606,10 +1592,8 @@ xfs_qm_write_sb_changes(
>  		return error;
>  	}
>  
> -	xfs_mod_sb(tp, flags);
> -	error = xfs_trans_commit(tp, 0);
> -
> -	return error;
> +	xfs_mod_sb(tp);
> +	return xfs_trans_commit(tp, 0);
>  }
>  
>  
> diff --git a/fs/xfs/xfs_qm.h b/fs/xfs/xfs_qm.h
> index 3a07a93..bddd23f 100644
> --- a/fs/xfs/xfs_qm.h
> +++ b/fs/xfs/xfs_qm.h
> @@ -157,7 +157,7 @@ struct xfs_dquot_acct {
>  #define XFS_QM_RTBWARNLIMIT	5
>  
>  extern void		xfs_qm_destroy_quotainfo(struct xfs_mount *);
> -extern int		xfs_qm_write_sb_changes(struct xfs_mount *, __int64_t);
> +extern int		xfs_qm_write_sb_changes(struct xfs_mount *);
>  
>  /* dquot stuff */
>  extern void		xfs_qm_dqpurge_all(struct xfs_mount *, uint);
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index 74fca68..8d7e5f0 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -92,8 +92,7 @@ xfs_qm_scall_quotaoff(
>  		mutex_unlock(&q->qi_quotaofflock);
>  
>  		/* XXX what to do if error ? Revert back to old vals incore ? */
> -		error = xfs_qm_write_sb_changes(mp, XFS_SB_QFLAGS);
> -		return error;
> +		return xfs_qm_write_sb_changes(mp);
>  	}
>  
>  	dqtype = 0;
> @@ -314,7 +313,6 @@ xfs_qm_scall_quotaon(
>  {
>  	int		error;
>  	uint		qf;
> -	__int64_t	sbflags;
>  
>  	flags &= (XFS_ALL_QUOTA_ACCT | XFS_ALL_QUOTA_ENFD);
>  	/*
> @@ -322,8 +320,6 @@ xfs_qm_scall_quotaon(
>  	 */
>  	flags &= ~(XFS_ALL_QUOTA_ACCT);
>  
> -	sbflags = 0;
> -
>  	if (flags == 0) {
>  		xfs_debug(mp, "%s: zero flags, m_qflags=%x",
>  			__func__, mp->m_qflags);
> @@ -370,11 +366,10 @@ xfs_qm_scall_quotaon(
>  	/*
>  	 * There's nothing to change if it's the same.
>  	 */
> -	if ((qf & flags) == flags && sbflags == 0)
> +	if ((qf & flags) == flags)
>  		return -EEXIST;
> -	sbflags |= XFS_SB_QFLAGS;
>  
> -	if ((error = xfs_qm_write_sb_changes(mp, sbflags)))
> +	if ((error = xfs_qm_write_sb_changes(mp)))
>  		return error;
>  	/*
>  	 * If we aren't trying to switch on quota enforcement, we are done.
> @@ -801,7 +796,7 @@ xfs_qm_log_quotaoff(
>  	mp->m_sb.sb_qflags = (mp->m_qflags & ~(flags)) & XFS_MOUNT_QUOTA_ALL;
>  	spin_unlock(&mp->m_sb_lock);
>  
> -	xfs_mod_sb(tp, XFS_SB_QFLAGS);
> +	xfs_mod_sb(tp);
>  
>  	/*
>  	 * We have to make sure that the transaction is secure on disk before we
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 40bad75..3797a03 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1258,7 +1258,7 @@ xfs_fs_remount(
>  		 * might have some superblock changes to update.
>  		 */
>  		if (mp->m_update_flags) {
> -			error = xfs_mount_log_sb(mp, mp->m_update_flags);
> +			error = xfs_mount_log_sb(mp);
>  			if (error) {
>  				xfs_warn(mp, "failed to write sb changes");
>  				return error;
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/3] xfs: remove bitfield based superblock updates

[Dave Chinner]

On Fri, Jan 09, 2015 at 03:35:01PM -0500, Brian Foster wrote:
> On Thu, Jan 08, 2015 at 08:51:03AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > When we log changes to the superblock, we first have to write them
> > to the on-disk buffer, and then log that. Right now we have a
> > complex bitfield based arrangement to only write the modified field
> > to the buffer before we log it.
> > 
> > This used to be necessary as a performance optimisation because we
> > logged the superblock buffer in every extent or inode allocation or
> > freeing, and so performance was extremely important. We haven't done
> > this for years, however, ever since the lazy superblock counters
> > pulled the superblock logging out of the transaction commit
> > fast path.
> > 
> > Hence we have a bunch of complexity that is not necessary that makes
> > writing the in-core superblock to disk much more complex than it
> > needs to be. We only need to log the superblock now during
> > management operations (e.g. during mount, unmount or quota control
> > operations) so it is not a performance critical path anymore.
> > 
> > As such, remove the complex field based logging mechanism and
> > replace it with a simple conversion function similar to what we use
> > for all other on-disk structures.
> > 
> > This means we always log the entirity of the superblock, but again
> > because we rarely modify the superblock this is not an issue for log
> > bandwidth or CPU time. Indeed, if we do log the superblock
> > frequently, delayed logging will minimise the impact of this
> > overhead.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
....
> >  	/*
> > -	 * GQUOTINO and PQUOTINO cannot be used together in versions of
> > -	 * superblock that do not have pquotino. from->sb_flags tells us which
> > -	 * quota is active and should be copied to disk. If neither are active,
> > -	 * make sure we write NULLFSINO to the sb_gquotino field as a quota
> > -	 * inode value of "0" is invalid when the XFS_SB_VERSION_QUOTA feature
> > -	 * bit is set.
> > +	 * GQUOTINO and PQUOTINO cannot be used together in versions
> > +	 * of superblock that do not have pquotino. from->sb_flags
> > +	 * tells us which quota is active and should be copied to
> > +	 * disk. If neither are active, we should NULL the inode.
> >  	 *
> > -	 * Note that we don't need to handle the sb_uquotino or sb_pquotino here
> > -	 * as they do not require any translation. Hence the main sb field loop
> > -	 * will write them appropriately from the in-core superblock.
> > +	 * In all cases, the separate pquotino must remain 0 because it
> > +	 * it beyond the "end" of the valid non-pquotino superblock.
> >  	 */
> > -	if ((*fields & XFS_SB_GQUOTINO) &&
> > -				(from->sb_qflags & XFS_GQUOTA_ACCT))
> > +	if (from->sb_qflags & XFS_GQUOTA_ACCT)
> >  		to->sb_gquotino = cpu_to_be64(from->sb_gquotino);
> > -	else if ((*fields & XFS_SB_PQUOTINO) &&
> > -				(from->sb_qflags & XFS_PQUOTA_ACCT))
> > +	else if (from->sb_qflags & XFS_PQUOTA_ACCT)
> >  		to->sb_gquotino = cpu_to_be64(from->sb_pquotino);
> > -	else {
> > -		/*
> > -		 * We can't rely on just the fields being logged to tell us
> > -		 * that it is safe to write NULLFSINO - we should only do that
> > -		 * if quotas are not actually enabled. Hence only write
> > -		 * NULLFSINO if both in-core quota inodes are NULL.
> > -		 */
> > -		if (from->sb_gquotino == NULLFSINO &&
> > -		    from->sb_pquotino == NULLFSINO)
> > -			to->sb_gquotino = cpu_to_be64(NULLFSINO);
> > -	}
> > +	else
> > +		to->sb_gquotino = cpu_to_be64(NULLFSINO);
> 
> FYI... it looks like the above hunk causes a regression due to resetting
> sb_gquotaino when one of the in-core inodes is set. I'm seeing
> disconnected quota inode messages on some xfstests (e.g., xfs/108) on v4
> filesystems. I'm about to put a patch on the list to go back to the
> original logic...

Interesting, I'm not seeing that in any of my v4 testing. hmmm - I
wonder if that's a result of using config sections and the test
device is not being remade with the changing format. I'll look into
it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/3] xfs: consolidate superblock logging functions

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

We now have several superblock loggin functions that are identical
except for the transaction reservation and whether it shoul dbe a
synchronous transaction or not. Consolidate these all into a single
function, a single reserveration and a sync flag and call it
xfs_sync_sb().

Also, xfs_mod_sb() is not really a modification function - it's the
operation of logging the superblock buffer. hence change the name of
it to reflect this.

Note that we have to change the mp->m_update_flags that are passed
around at mount time to a boolean simply to indicate a superblock
update is needed.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c  |  2 +-
 fs/xfs/libxfs/xfs_bmap.c       | 10 +++---
 fs/xfs/libxfs/xfs_sb.c         | 43 +++++++++++++++++++----
 fs/xfs/libxfs/xfs_sb.h         |  3 +-
 fs/xfs/libxfs/xfs_shared.h     | 33 ++++++++----------
 fs/xfs/libxfs/xfs_trans_resv.c | 14 --------
 fs/xfs/libxfs/xfs_trans_resv.h |  1 -
 fs/xfs/xfs_fsops.c             | 29 ----------------
 fs/xfs/xfs_log.c               | 18 ++++++++--
 fs/xfs/xfs_mount.c             | 78 +++++++-----------------------------------
 fs/xfs/xfs_mount.h             |  3 +-
 fs/xfs/xfs_qm.c                | 27 ++-------------
 fs/xfs/xfs_qm_syscalls.c       |  7 ++--
 fs/xfs/xfs_super.c             | 13 +++----
 14 files changed, 101 insertions(+), 180 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index c914422..15105db 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -403,7 +403,7 @@ xfs_sbversion_add_attr2(xfs_mount_t *mp, xfs_trans_t *tp)
 		if (!xfs_sb_version_hasattr2(&mp->m_sb)) {
 			xfs_sb_version_addattr2(&mp->m_sb);
 			spin_unlock(&mp->m_sb_lock);
-			xfs_mod_sb(tp);
+			xfs_log_sb(tp);
 		} else
 			spin_unlock(&mp->m_sb_lock);
 	}
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 8c39cc8..63a5bb9 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1221,20 +1221,20 @@ xfs_bmap_add_attrfork(
 		goto bmap_cancel;
 	if (!xfs_sb_version_hasattr(&mp->m_sb) ||
 	   (!xfs_sb_version_hasattr2(&mp->m_sb) && version == 2)) {
-		bool mod_sb = false;
+		bool log_sb = false;
 
 		spin_lock(&mp->m_sb_lock);
 		if (!xfs_sb_version_hasattr(&mp->m_sb)) {
 			xfs_sb_version_addattr(&mp->m_sb);
-			mod_sb = true;
+			log_sb = true;
 		}
 		if (!xfs_sb_version_hasattr2(&mp->m_sb) && version == 2) {
 			xfs_sb_version_addattr2(&mp->m_sb);
-			mod_sb = true;
+			log_sb = true;
 		}
 		spin_unlock(&mp->m_sb_lock);
-		if (mod_sb)
-			xfs_mod_sb(tp);
+		if (log_sb)
+			xfs_log_sb(tp);
 	}
 
 	error = xfs_bmap_finish(&tp, &flist, &committed);
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 4afbbce..6ee3af6 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -744,14 +744,13 @@ xfs_initialize_perag_data(
 }
 
 /*
- * xfs_mod_sb() can be used to copy arbitrary changes to the
- * in-core superblock into the superblock buffer to be logged.
- * It does not provide the higher level of locking that is
- * needed to protect the in-core superblock from concurrent
- * access.
+ * xfs_log_sb() can be used to copy arbitrary changes to the in-core superblock
+ * into the superblock buffer to be logged.  It does not provide the higher
+ * level of locking that is needed to protect the in-core superblock from
+ * concurrent access.
  */
 void
-xfs_mod_sb(
+xfs_log_sb(
 	struct xfs_trans	*tp)
 {
 	struct xfs_mount	*mp = tp->t_mountp;
@@ -761,3 +760,35 @@ xfs_mod_sb(
 	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_SB_BUF);
 	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsb));
 }
+
+/*
+ * xfs_sync_sb
+ *
+ * Sync the superblock to disk.
+ *
+ * Note that the caller is responsible for checking the frozen state of the
+ * filesystem. This procedure uses the non-blocking transaction allocator and
+ * thus will allow modifications to a frozen fs. This is required because this
+ * code can be called during the process of freezing where use of the high-level
+ * allocator would deadlock.
+ */
+int
+xfs_sync_sb(
+	struct xfs_mount	*mp,
+	bool			wait)
+{
+	struct xfs_trans	*tp;
+	int			error;
+
+	tp = _xfs_trans_alloc(mp, XFS_TRANS_SB_CHANGE, KM_SLEEP);
+	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_sb, 0, 0);
+	if (error) {
+		xfs_trans_cancel(tp, 0);
+		return error;
+	}
+
+	xfs_log_sb(tp);
+	if (wait)
+		xfs_trans_set_sync(tp);
+	return xfs_trans_commit(tp, 0);
+}
diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
index e193caa..b25bb9a 100644
--- a/fs/xfs/libxfs/xfs_sb.h
+++ b/fs/xfs/libxfs/xfs_sb.h
@@ -28,7 +28,8 @@ extern void	xfs_perag_put(struct xfs_perag *pag);
 extern int	xfs_initialize_perag_data(struct xfs_mount *, xfs_agnumber_t);
 
 extern void	xfs_sb_calc_crc(struct xfs_buf *bp);
-extern void	xfs_mod_sb(struct xfs_trans *tp);
+extern void	xfs_log_sb(struct xfs_trans *tp);
+extern int	xfs_sync_sb(struct xfs_mount *mp, bool wait);
 extern void	xfs_sb_mount_common(struct xfs_mount *mp, struct xfs_sb *sbp);
 extern void	xfs_sb_from_disk(struct xfs_sb *to, struct xfs_dsb *from);
 extern void	xfs_sb_to_disk(struct xfs_dsb *to, struct xfs_sb *from);
diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
index 82404da..8dda4b3 100644
--- a/fs/xfs/libxfs/xfs_shared.h
+++ b/fs/xfs/libxfs/xfs_shared.h
@@ -82,7 +82,7 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
 #define	XFS_TRANS_ATTR_RM		23
 #define	XFS_TRANS_ATTR_FLAG		24
 #define	XFS_TRANS_CLEAR_AGI_BUCKET	25
-#define XFS_TRANS_QM_SBCHANGE		26
+#define XFS_TRANS_SB_CHANGE		26
 /*
  * Dummy entries since we use the transaction type to index into the
  * trans_type[] in xlog_recover_print_trans_head()
@@ -95,17 +95,15 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
 #define XFS_TRANS_QM_DQCLUSTER		32
 #define XFS_TRANS_QM_QINOCREATE		33
 #define XFS_TRANS_QM_QUOTAOFF_END	34
-#define XFS_TRANS_SB_UNIT		35
-#define XFS_TRANS_FSYNC_TS		36
-#define	XFS_TRANS_GROWFSRT_ALLOC	37
-#define	XFS_TRANS_GROWFSRT_ZERO		38
-#define	XFS_TRANS_GROWFSRT_FREE		39
-#define	XFS_TRANS_SWAPEXT		40
-#define	XFS_TRANS_SB_COUNT		41
-#define	XFS_TRANS_CHECKPOINT		42
-#define	XFS_TRANS_ICREATE		43
-#define	XFS_TRANS_CREATE_TMPFILE	44
-#define	XFS_TRANS_TYPE_MAX		44
+#define XFS_TRANS_FSYNC_TS		35
+#define	XFS_TRANS_GROWFSRT_ALLOC	36
+#define	XFS_TRANS_GROWFSRT_ZERO		37
+#define	XFS_TRANS_GROWFSRT_FREE		38
+#define	XFS_TRANS_SWAPEXT		39
+#define	XFS_TRANS_CHECKPOINT		40
+#define	XFS_TRANS_ICREATE		41
+#define	XFS_TRANS_CREATE_TMPFILE	42
+#define	XFS_TRANS_TYPE_MAX		43
 /* new transaction types need to be reflected in xfs_logprint(8) */
 
 #define XFS_TRANS_TYPES \
@@ -113,7 +111,6 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
 	{ XFS_TRANS_SETATTR_SIZE,	"SETATTR_SIZE" }, \
 	{ XFS_TRANS_INACTIVE,		"INACTIVE" }, \
 	{ XFS_TRANS_CREATE,		"CREATE" }, \
-	{ XFS_TRANS_CREATE_TMPFILE,	"CREATE_TMPFILE" }, \
 	{ XFS_TRANS_CREATE_TRUNC,	"CREATE_TRUNC" }, \
 	{ XFS_TRANS_TRUNCATE_FILE,	"TRUNCATE_FILE" }, \
 	{ XFS_TRANS_REMOVE,		"REMOVE" }, \
@@ -134,23 +131,23 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
 	{ XFS_TRANS_ATTR_RM,		"ATTR_RM" }, \
 	{ XFS_TRANS_ATTR_FLAG,		"ATTR_FLAG" }, \
 	{ XFS_TRANS_CLEAR_AGI_BUCKET,	"CLEAR_AGI_BUCKET" }, \
-	{ XFS_TRANS_QM_SBCHANGE,	"QM_SBCHANGE" }, \
+	{ XFS_TRANS_SB_CHANGE,		"SBCHANGE" }, \
+	{ XFS_TRANS_DUMMY1,		"DUMMY1" }, \
+	{ XFS_TRANS_DUMMY2,		"DUMMY2" }, \
 	{ XFS_TRANS_QM_QUOTAOFF,	"QM_QUOTAOFF" }, \
 	{ XFS_TRANS_QM_DQALLOC,		"QM_DQALLOC" }, \
 	{ XFS_TRANS_QM_SETQLIM,		"QM_SETQLIM" }, \
 	{ XFS_TRANS_QM_DQCLUSTER,	"QM_DQCLUSTER" }, \
 	{ XFS_TRANS_QM_QINOCREATE,	"QM_QINOCREATE" }, \
 	{ XFS_TRANS_QM_QUOTAOFF_END,	"QM_QOFF_END" }, \
-	{ XFS_TRANS_SB_UNIT,		"SB_UNIT" }, \
 	{ XFS_TRANS_FSYNC_TS,		"FSYNC_TS" }, \
 	{ XFS_TRANS_GROWFSRT_ALLOC,	"GROWFSRT_ALLOC" }, \
 	{ XFS_TRANS_GROWFSRT_ZERO,	"GROWFSRT_ZERO" }, \
 	{ XFS_TRANS_GROWFSRT_FREE,	"GROWFSRT_FREE" }, \
 	{ XFS_TRANS_SWAPEXT,		"SWAPEXT" }, \
-	{ XFS_TRANS_SB_COUNT,		"SB_COUNT" }, \
 	{ XFS_TRANS_CHECKPOINT,		"CHECKPOINT" }, \
-	{ XFS_TRANS_DUMMY1,		"DUMMY1" }, \
-	{ XFS_TRANS_DUMMY2,		"DUMMY2" }, \
+	{ XFS_TRANS_ICREATE,		"ICREATE" }, \
+	{ XFS_TRANS_CREATE_TMPFILE,	"CREATE_TMPFILE" }, \
 	{ XLOG_UNMOUNT_REC_TYPE,	"UNMOUNT" }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 6c1330f..68cb1e7 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -716,17 +716,6 @@ xfs_calc_clear_agi_bucket_reservation(
 }
 
 /*
- * Clearing the quotaflags in the superblock.
- *	the super block for changing quota flags: sector size
- */
-STATIC uint
-xfs_calc_qm_sbchange_reservation(
-	struct xfs_mount	*mp)
-{
-	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
-}
-
-/*
  * Adjusting quota limits.
  *    the xfs_disk_dquot_t: sizeof(struct xfs_disk_dquot)
  */
@@ -864,9 +853,6 @@ xfs_trans_resv_calc(
 	 * The following transactions are logged in logical format with
 	 * a default log count.
 	 */
-	resp->tr_qm_sbchange.tr_logres = xfs_calc_qm_sbchange_reservation(mp);
-	resp->tr_qm_sbchange.tr_logcount = XFS_DEFAULT_LOG_COUNT;
-
 	resp->tr_qm_setqlim.tr_logres = xfs_calc_qm_setqlim_reservation(mp);
 	resp->tr_qm_setqlim.tr_logcount = XFS_DEFAULT_LOG_COUNT;
 
diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
index 1097d14..2d5bdfc 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.h
+++ b/fs/xfs/libxfs/xfs_trans_resv.h
@@ -56,7 +56,6 @@ struct xfs_trans_resv {
 	struct xfs_trans_res	tr_growrtalloc;	/* grow realtime allocations */
 	struct xfs_trans_res	tr_growrtzero;	/* grow realtime zeroing */
 	struct xfs_trans_res	tr_growrtfree;	/* grow realtime freeing */
-	struct xfs_trans_res	tr_qm_sbchange;	/* change quota flags */
 	struct xfs_trans_res	tr_qm_setqlim;	/* adjust quota limits */
 	struct xfs_trans_res	tr_qm_dqalloc;	/* allocate quota on disk */
 	struct xfs_trans_res	tr_qm_quotaoff;	/* turn quota off */
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 82af857..f711452 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -756,35 +756,6 @@ out:
 	return 0;
 }
 
-/*
- * Dump a transaction into the log that contains no real change. This is needed
- * to be able to make the log dirty or stamp the current tail LSN into the log
- * during the covering operation.
- *
- * We cannot use an inode here for this - that will push dirty state back up
- * into the VFS and then periodic inode flushing will prevent log covering from
- * making progress. Hence we log a field in the superblock instead and use a
- * synchronous transaction to ensure the superblock is immediately unpinned
- * and can be written back.
- */
-int
-xfs_fs_log_dummy(
-	xfs_mount_t	*mp)
-{
-	xfs_trans_t	*tp;
-	int		error;
-
-	tp = _xfs_trans_alloc(mp, XFS_TRANS_DUMMY1, KM_SLEEP);
-	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_sb, 0, 0);
-	if (error) {
-		xfs_trans_cancel(tp, 0);
-		return error;
-	}
-	xfs_mod_sb(tp);
-	xfs_trans_set_sync(tp);
-	return xfs_trans_commit(tp, 0);
-}
-
 int
 xfs_fs_goingdown(
 	xfs_mount_t	*mp,
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 8fbbfb2..bcc7cfa 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -33,6 +33,7 @@
 #include "xfs_fsops.h"
 #include "xfs_cksum.h"
 #include "xfs_sysfs.h"
+#include "xfs_sb.h"
 
 kmem_zone_t	*xfs_log_ticket_zone;
 
@@ -1290,9 +1291,20 @@ xfs_log_worker(
 	struct xfs_mount	*mp = log->l_mp;
 
 	/* dgc: errors ignored - not fatal and nowhere to report them */
-	if (xfs_log_need_covered(mp))
-		xfs_fs_log_dummy(mp);
-	else
+	if (xfs_log_need_covered(mp)) {
+		/*
+		 * Dump a transaction into the log that contains no real change.
+		 * This is needed to stamp the current tail LSN into the log
+		 * during the covering operation.
+		 *
+		 * We cannot use an inode here for this - that will push dirty
+		 * state back up into the VFS and then periodic inode flushing
+		 * will prevent log covering from making progress. Hence we
+		 * synchronously log the superblock instead to ensure the
+		 * superblock is immediately unpinned and can be written back.
+		 */
+		xfs_sync_sb(mp, true);
+	} else
 		xfs_log_force(mp, 0);
 
 	/* start pushing all the metadata that is currently dirty */
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index defcf69..5ef9aa2 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -408,11 +408,11 @@ xfs_update_alignment(xfs_mount_t *mp)
 		if (xfs_sb_version_hasdalign(sbp)) {
 			if (sbp->sb_unit != mp->m_dalign) {
 				sbp->sb_unit = mp->m_dalign;
-				mp->m_update_flags |= XFS_SB_UNIT;
+				mp->m_update_sb = true;
 			}
 			if (sbp->sb_width != mp->m_swidth) {
 				sbp->sb_width = mp->m_swidth;
-				mp->m_update_flags |= XFS_SB_WIDTH;
+				mp->m_update_sb = true;
 			}
 		} else {
 			xfs_warn(mp,
@@ -583,38 +583,19 @@ int
 xfs_mount_reset_sbqflags(
 	struct xfs_mount	*mp)
 {
-	int			error;
-	struct xfs_trans	*tp;
-
 	mp->m_qflags = 0;
 
-	/*
-	 * It is OK to look at sb_qflags here in mount path,
-	 * without m_sb_lock.
-	 */
+	/* It is OK to look at sb_qflags in the mount path without m_sb_lock. */
 	if (mp->m_sb.sb_qflags == 0)
 		return 0;
 	spin_lock(&mp->m_sb_lock);
 	mp->m_sb.sb_qflags = 0;
 	spin_unlock(&mp->m_sb_lock);
 
-	/*
-	 * If the fs is readonly, let the incore superblock run
-	 * with quotas off but don't flush the update out to disk
-	 */
-	if (mp->m_flags & XFS_MOUNT_RDONLY)
+	if (!xfs_fs_writable(mp, SB_FREEZE_WRITE))
 		return 0;
 
-	tp = xfs_trans_alloc(mp, XFS_TRANS_QM_SBCHANGE);
-	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_sbchange, 0, 0);
-	if (error) {
-		xfs_trans_cancel(tp, 0);
-		xfs_alert(mp, "%s: Superblock update failed!", __func__);
-		return error;
-	}
-
-	xfs_mod_sb(tp);
-	return xfs_trans_commit(tp, 0);
+	return xfs_sync_sb(mp, false);
 }
 
 __uint64_t
@@ -678,7 +659,7 @@ xfs_mountfs(
 		xfs_warn(mp, "correcting sb_features alignment problem");
 		sbp->sb_features2 |= sbp->sb_bad_features2;
 		sbp->sb_bad_features2 = sbp->sb_features2;
-		mp->m_update_flags |= XFS_SB_FEATURES2;
+		mp->m_update_sb = true;
 
 		/*
 		 * Re-check for ATTR2 in case it was found in bad_features2
@@ -692,17 +673,17 @@ xfs_mountfs(
 	if (xfs_sb_version_hasattr2(&mp->m_sb) &&
 	   (mp->m_flags & XFS_MOUNT_NOATTR2)) {
 		xfs_sb_version_removeattr2(&mp->m_sb);
-		mp->m_update_flags |= XFS_SB_FEATURES2;
+		mp->m_update_sb = true;
 
 		/* update sb_versionnum for the clearing of the morebits */
 		if (!sbp->sb_features2)
-			mp->m_update_flags |= XFS_SB_VERSIONNUM;
+			mp->m_update_sb = true;
 	}
 
 	/* always use v2 inodes by default now */
 	if (!(mp->m_sb.sb_versionnum & XFS_SB_VERSION_NLINKBIT)) {
 		mp->m_sb.sb_versionnum |= XFS_SB_VERSION_NLINKBIT;
-		mp->m_update_flags |= XFS_SB_VERSIONNUM;
+		mp->m_update_sb = true;
 	}
 
 	/*
@@ -895,8 +876,8 @@ xfs_mountfs(
 	 * the next remount into writeable mode.  Otherwise we would never
 	 * perform the update e.g. for the root filesystem.
 	 */
-	if (mp->m_update_flags && !(mp->m_flags & XFS_MOUNT_RDONLY)) {
-		error = xfs_mount_log_sb(mp);
+	if (mp->m_update_sb && !(mp->m_flags & XFS_MOUNT_RDONLY)) {
+		error = xfs_sync_sb(mp, false);
 		if (error) {
 			xfs_warn(mp, "failed to write sb changes");
 			goto out_rtunmount;
@@ -1103,9 +1084,6 @@ xfs_fs_writable(
 int
 xfs_log_sbcount(xfs_mount_t *mp)
 {
-	xfs_trans_t	*tp;
-	int		error;
-
 	/* allow this to proceed during the freeze sequence... */
 	if (!xfs_fs_writable(mp, SB_FREEZE_COMPLETE))
 		return 0;
@@ -1119,17 +1097,7 @@ xfs_log_sbcount(xfs_mount_t *mp)
 	if (!xfs_sb_version_haslazysbcount(&mp->m_sb))
 		return 0;
 
-	tp = _xfs_trans_alloc(mp, XFS_TRANS_SB_COUNT, KM_SLEEP);
-	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_sb, 0, 0);
-	if (error) {
-		xfs_trans_cancel(tp, 0);
-		return error;
-	}
-
-	xfs_mod_sb(tp);
-	xfs_trans_set_sync(tp);
-	error = xfs_trans_commit(tp, 0);
-	return error;
+	return xfs_sync_sb(mp, true);
 }
 
 /*
@@ -1423,28 +1391,6 @@ xfs_freesb(
 }
 
 /*
- * Used to log changes to the superblock unit and width fields which could
- * be altered by the mount options, as well as any potential sb_features2
- * fixup. Only the first superblock is updated.
- */
-int
-xfs_mount_log_sb(
-	struct xfs_mount	*mp)
-{
-	struct xfs_trans	*tp;
-	int			error;
-
-	tp = xfs_trans_alloc(mp, XFS_TRANS_SB_UNIT);
-	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_sb, 0, 0);
-	if (error) {
-		xfs_trans_cancel(tp, 0);
-		return error;
-	}
-	xfs_mod_sb(tp);
-	return xfs_trans_commit(tp, 0);
-}
-
-/*
  * If the underlying (data/log/rt) device is readonly, there are some
  * operations that cannot proceed.
  */
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 28b341b..a5b2ff8 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -162,8 +162,7 @@ typedef struct xfs_mount {
 	struct delayed_work	m_reclaim_work;	/* background inode reclaim */
 	struct delayed_work	m_eofblocks_work; /* background eof blocks
 						     trimming */
-	__int64_t		m_update_flags;	/* sb flags we need to update
-						   on the next remount,rw */
+	bool			m_update_sb;	/* sb needs update in mount */
 	int64_t			m_low_space[XFS_LOWSP_MAX];
 						/* low free space thresholds */
 	struct xfs_kobj		m_kobj;
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index c815a80..3e81862 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -792,7 +792,7 @@ xfs_qm_qino_alloc(
 	else
 		mp->m_sb.sb_pquotino = (*ip)->i_ino;
 	spin_unlock(&mp->m_sb_lock);
-	xfs_mod_sb(tp);
+	xfs_log_sb(tp);
 
 	if ((error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES))) {
 		xfs_alert(mp, "%s failed (error %d)!", __func__, error);
@@ -1445,7 +1445,7 @@ xfs_qm_mount_quotas(
 	spin_unlock(&mp->m_sb_lock);
 
 	if (sbf != (mp->m_qflags & XFS_MOUNT_QUOTA_ALL)) {
-		if (xfs_qm_write_sb_changes(mp)) {
+		if (xfs_sync_sb(mp, false)) {
 			/*
 			 * We could only have been turning quotas off.
 			 * We aren't in very good shape actually because
@@ -1574,29 +1574,6 @@ xfs_qm_dqfree_one(
 	xfs_qm_dqdestroy(dqp);
 }
 
-/*
- * Start a transaction and write the incore superblock changes to
- * disk. flags parameter indicates which fields have changed.
- */
-int
-xfs_qm_write_sb_changes(
-	struct xfs_mount *mp)
-{
-	xfs_trans_t	*tp;
-	int		error;
-
-	tp = xfs_trans_alloc(mp, XFS_TRANS_QM_SBCHANGE);
-	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_sbchange, 0, 0);
-	if (error) {
-		xfs_trans_cancel(tp, 0);
-		return error;
-	}
-
-	xfs_mod_sb(tp);
-	return xfs_trans_commit(tp, 0);
-}
-
-
 /* --------------- utility functions for vnodeops ---------------- */
 
 
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index 8d7e5f0..b8a565e 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -92,7 +92,7 @@ xfs_qm_scall_quotaoff(
 		mutex_unlock(&q->qi_quotaofflock);
 
 		/* XXX what to do if error ? Revert back to old vals incore ? */
-		return xfs_qm_write_sb_changes(mp);
+		return xfs_sync_sb(mp, false);
 	}
 
 	dqtype = 0;
@@ -369,7 +369,8 @@ xfs_qm_scall_quotaon(
 	if ((qf & flags) == flags)
 		return -EEXIST;
 
-	if ((error = xfs_qm_write_sb_changes(mp)))
+	error = xfs_sync_sb(mp, false);
+	if (error)
 		return error;
 	/*
 	 * If we aren't trying to switch on quota enforcement, we are done.
@@ -796,7 +797,7 @@ xfs_qm_log_quotaoff(
 	mp->m_sb.sb_qflags = (mp->m_qflags & ~(flags)) & XFS_MOUNT_QUOTA_ALL;
 	spin_unlock(&mp->m_sb_lock);
 
-	xfs_mod_sb(tp);
+	xfs_log_sb(tp);
 
 	/*
 	 * We have to make sure that the transaction is secure on disk before we
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 3797a03..afd6bae 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1257,13 +1257,13 @@ xfs_fs_remount(
 		 * If this is the first remount to writeable state we
 		 * might have some superblock changes to update.
 		 */
-		if (mp->m_update_flags) {
-			error = xfs_mount_log_sb(mp);
+		if (mp->m_update_sb) {
+			error = xfs_sync_sb(mp, false);
 			if (error) {
 				xfs_warn(mp, "failed to write sb changes");
 				return error;
 			}
-			mp->m_update_flags = 0;
+			mp->m_update_sb = false;
 		}
 
 		/*
@@ -1293,8 +1293,9 @@ xfs_fs_remount(
 
 /*
  * Second stage of a freeze. The data is already frozen so we only
- * need to take care of the metadata. Once that's done write a dummy
- * record to dirty the log in case of a crash while frozen.
+ * need to take care of the metadata. Once that's done sync the superblock
+ * to the log to dirty it in case of a crash while frozen. This ensures that we
+ * will recover the unlinked inode lists on the next mount.
  */
 STATIC int
 xfs_fs_freeze(
@@ -1304,7 +1305,7 @@ xfs_fs_freeze(
 
 	xfs_save_resvblks(mp);
 	xfs_quiesce_attr(mp);
-	return xfs_fs_log_dummy(mp);
+	return xfs_sync_sb(mp, true);
 }
 
 STATIC int
-- 
2.0.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/3] xfs: consolidate superblock logging functions

[Brian Foster]

On Thu, Jan 08, 2015 at 08:51:04AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> We now have several superblock loggin functions that are identical
> except for the transaction reservation and whether it shoul dbe a
> synchronous transaction or not. Consolidate these all into a single
> function, a single reserveration and a sync flag and call it
> xfs_sync_sb().
> 
> Also, xfs_mod_sb() is not really a modification function - it's the
> operation of logging the superblock buffer. hence change the name of
> it to reflect this.
> 
> Note that we have to change the mp->m_update_flags that are passed
> around at mount time to a boolean simply to indicate a superblock
> update is needed.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

A question and one other little thing...

>  fs/xfs/libxfs/xfs_attr_leaf.c  |  2 +-
>  fs/xfs/libxfs/xfs_bmap.c       | 10 +++---
>  fs/xfs/libxfs/xfs_sb.c         | 43 +++++++++++++++++++----
>  fs/xfs/libxfs/xfs_sb.h         |  3 +-
>  fs/xfs/libxfs/xfs_shared.h     | 33 ++++++++----------
>  fs/xfs/libxfs/xfs_trans_resv.c | 14 --------
>  fs/xfs/libxfs/xfs_trans_resv.h |  1 -
>  fs/xfs/xfs_fsops.c             | 29 ----------------
>  fs/xfs/xfs_log.c               | 18 ++++++++--
>  fs/xfs/xfs_mount.c             | 78 +++++++-----------------------------------
>  fs/xfs/xfs_mount.h             |  3 +-
>  fs/xfs/xfs_qm.c                | 27 ++-------------
>  fs/xfs/xfs_qm_syscalls.c       |  7 ++--
>  fs/xfs/xfs_super.c             | 13 +++----
>  14 files changed, 101 insertions(+), 180 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index c914422..15105db 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -403,7 +403,7 @@ xfs_sbversion_add_attr2(xfs_mount_t *mp, xfs_trans_t *tp)
>  		if (!xfs_sb_version_hasattr2(&mp->m_sb)) {
>  			xfs_sb_version_addattr2(&mp->m_sb);
>  			spin_unlock(&mp->m_sb_lock);
> -			xfs_mod_sb(tp);
> +			xfs_log_sb(tp);
>  		} else
>  			spin_unlock(&mp->m_sb_lock);
>  	}
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 8c39cc8..63a5bb9 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1221,20 +1221,20 @@ xfs_bmap_add_attrfork(
>  		goto bmap_cancel;
>  	if (!xfs_sb_version_hasattr(&mp->m_sb) ||
>  	   (!xfs_sb_version_hasattr2(&mp->m_sb) && version == 2)) {
> -		bool mod_sb = false;
> +		bool log_sb = false;
>  
>  		spin_lock(&mp->m_sb_lock);
>  		if (!xfs_sb_version_hasattr(&mp->m_sb)) {
>  			xfs_sb_version_addattr(&mp->m_sb);
> -			mod_sb = true;
> +			log_sb = true;
>  		}
>  		if (!xfs_sb_version_hasattr2(&mp->m_sb) && version == 2) {
>  			xfs_sb_version_addattr2(&mp->m_sb);
> -			mod_sb = true;
> +			log_sb = true;
>  		}
>  		spin_unlock(&mp->m_sb_lock);
> -		if (mod_sb)
> -			xfs_mod_sb(tp);
> +		if (log_sb)
> +			xfs_log_sb(tp);
>  	}
>  
>  	error = xfs_bmap_finish(&tp, &flist, &committed);
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 4afbbce..6ee3af6 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -744,14 +744,13 @@ xfs_initialize_perag_data(
>  }
>  
>  /*
> - * xfs_mod_sb() can be used to copy arbitrary changes to the
> - * in-core superblock into the superblock buffer to be logged.
> - * It does not provide the higher level of locking that is
> - * needed to protect the in-core superblock from concurrent
> - * access.
> + * xfs_log_sb() can be used to copy arbitrary changes to the in-core superblock
> + * into the superblock buffer to be logged.  It does not provide the higher
> + * level of locking that is needed to protect the in-core superblock from
> + * concurrent access.
>   */
>  void
> -xfs_mod_sb(
> +xfs_log_sb(
>  	struct xfs_trans	*tp)
>  {
>  	struct xfs_mount	*mp = tp->t_mountp;
> @@ -761,3 +760,35 @@ xfs_mod_sb(
>  	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_SB_BUF);
>  	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsb));
>  }
> +
> +/*
> + * xfs_sync_sb
> + *
> + * Sync the superblock to disk.
> + *
> + * Note that the caller is responsible for checking the frozen state of the
> + * filesystem. This procedure uses the non-blocking transaction allocator and
> + * thus will allow modifications to a frozen fs. This is required because this
> + * code can be called during the process of freezing where use of the high-level
> + * allocator would deadlock.
> + */
> +int
> +xfs_sync_sb(
> +	struct xfs_mount	*mp,
> +	bool			wait)
> +{
> +	struct xfs_trans	*tp;
> +	int			error;
> +
> +	tp = _xfs_trans_alloc(mp, XFS_TRANS_SB_CHANGE, KM_SLEEP);
> +	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_sb, 0, 0);
> +	if (error) {
> +		xfs_trans_cancel(tp, 0);
> +		return error;
> +	}
> +
> +	xfs_log_sb(tp);
> +	if (wait)
> +		xfs_trans_set_sync(tp);
> +	return xfs_trans_commit(tp, 0);
> +}
> diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
> index e193caa..b25bb9a 100644
> --- a/fs/xfs/libxfs/xfs_sb.h
> +++ b/fs/xfs/libxfs/xfs_sb.h
> @@ -28,7 +28,8 @@ extern void	xfs_perag_put(struct xfs_perag *pag);
>  extern int	xfs_initialize_perag_data(struct xfs_mount *, xfs_agnumber_t);
>  
>  extern void	xfs_sb_calc_crc(struct xfs_buf *bp);
> -extern void	xfs_mod_sb(struct xfs_trans *tp);
> +extern void	xfs_log_sb(struct xfs_trans *tp);
> +extern int	xfs_sync_sb(struct xfs_mount *mp, bool wait);
>  extern void	xfs_sb_mount_common(struct xfs_mount *mp, struct xfs_sb *sbp);
>  extern void	xfs_sb_from_disk(struct xfs_sb *to, struct xfs_dsb *from);
>  extern void	xfs_sb_to_disk(struct xfs_dsb *to, struct xfs_sb *from);
> diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
> index 82404da..8dda4b3 100644
> --- a/fs/xfs/libxfs/xfs_shared.h
> +++ b/fs/xfs/libxfs/xfs_shared.h
> @@ -82,7 +82,7 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
>  #define	XFS_TRANS_ATTR_RM		23
>  #define	XFS_TRANS_ATTR_FLAG		24
>  #define	XFS_TRANS_CLEAR_AGI_BUCKET	25
> -#define XFS_TRANS_QM_SBCHANGE		26
> +#define XFS_TRANS_SB_CHANGE		26
>  /*
>   * Dummy entries since we use the transaction type to index into the
>   * trans_type[] in xlog_recover_print_trans_head()
> @@ -95,17 +95,15 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
>  #define XFS_TRANS_QM_DQCLUSTER		32
>  #define XFS_TRANS_QM_QINOCREATE		33
>  #define XFS_TRANS_QM_QUOTAOFF_END	34
> -#define XFS_TRANS_SB_UNIT		35
> -#define XFS_TRANS_FSYNC_TS		36
> -#define	XFS_TRANS_GROWFSRT_ALLOC	37
> -#define	XFS_TRANS_GROWFSRT_ZERO		38
> -#define	XFS_TRANS_GROWFSRT_FREE		39
> -#define	XFS_TRANS_SWAPEXT		40
> -#define	XFS_TRANS_SB_COUNT		41
> -#define	XFS_TRANS_CHECKPOINT		42
> -#define	XFS_TRANS_ICREATE		43
> -#define	XFS_TRANS_CREATE_TMPFILE	44
> -#define	XFS_TRANS_TYPE_MAX		44
> +#define XFS_TRANS_FSYNC_TS		35
> +#define	XFS_TRANS_GROWFSRT_ALLOC	36
> +#define	XFS_TRANS_GROWFSRT_ZERO		37
> +#define	XFS_TRANS_GROWFSRT_FREE		38
> +#define	XFS_TRANS_SWAPEXT		39
> +#define	XFS_TRANS_CHECKPOINT		40
> +#define	XFS_TRANS_ICREATE		41
> +#define	XFS_TRANS_CREATE_TMPFILE	42
> +#define	XFS_TRANS_TYPE_MAX		43
>  /* new transaction types need to be reflected in xfs_logprint(8) */
>  
>  #define XFS_TRANS_TYPES \
> @@ -113,7 +111,6 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
>  	{ XFS_TRANS_SETATTR_SIZE,	"SETATTR_SIZE" }, \
>  	{ XFS_TRANS_INACTIVE,		"INACTIVE" }, \
>  	{ XFS_TRANS_CREATE,		"CREATE" }, \
> -	{ XFS_TRANS_CREATE_TMPFILE,	"CREATE_TMPFILE" }, \
>  	{ XFS_TRANS_CREATE_TRUNC,	"CREATE_TRUNC" }, \
>  	{ XFS_TRANS_TRUNCATE_FILE,	"TRUNCATE_FILE" }, \
>  	{ XFS_TRANS_REMOVE,		"REMOVE" }, \
> @@ -134,23 +131,23 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
>  	{ XFS_TRANS_ATTR_RM,		"ATTR_RM" }, \
>  	{ XFS_TRANS_ATTR_FLAG,		"ATTR_FLAG" }, \
>  	{ XFS_TRANS_CLEAR_AGI_BUCKET,	"CLEAR_AGI_BUCKET" }, \
> -	{ XFS_TRANS_QM_SBCHANGE,	"QM_SBCHANGE" }, \
> +	{ XFS_TRANS_SB_CHANGE,		"SBCHANGE" }, \
> +	{ XFS_TRANS_DUMMY1,		"DUMMY1" }, \
> +	{ XFS_TRANS_DUMMY2,		"DUMMY2" }, \

I take it we can't just remove these dummy types, for userspace
compatibility reasons, right? At least, it looks like logprint could get
confused if other transaction types inherited these values.

>  	{ XFS_TRANS_QM_QUOTAOFF,	"QM_QUOTAOFF" }, \
>  	{ XFS_TRANS_QM_DQALLOC,		"QM_DQALLOC" }, \
>  	{ XFS_TRANS_QM_SETQLIM,		"QM_SETQLIM" }, \
>  	{ XFS_TRANS_QM_DQCLUSTER,	"QM_DQCLUSTER" }, \
>  	{ XFS_TRANS_QM_QINOCREATE,	"QM_QINOCREATE" }, \
>  	{ XFS_TRANS_QM_QUOTAOFF_END,	"QM_QOFF_END" }, \
> -	{ XFS_TRANS_SB_UNIT,		"SB_UNIT" }, \
>  	{ XFS_TRANS_FSYNC_TS,		"FSYNC_TS" }, \
>  	{ XFS_TRANS_GROWFSRT_ALLOC,	"GROWFSRT_ALLOC" }, \
>  	{ XFS_TRANS_GROWFSRT_ZERO,	"GROWFSRT_ZERO" }, \
>  	{ XFS_TRANS_GROWFSRT_FREE,	"GROWFSRT_FREE" }, \
>  	{ XFS_TRANS_SWAPEXT,		"SWAPEXT" }, \
> -	{ XFS_TRANS_SB_COUNT,		"SB_COUNT" }, \
>  	{ XFS_TRANS_CHECKPOINT,		"CHECKPOINT" }, \
> -	{ XFS_TRANS_DUMMY1,		"DUMMY1" }, \
> -	{ XFS_TRANS_DUMMY2,		"DUMMY2" }, \
> +	{ XFS_TRANS_ICREATE,		"ICREATE" }, \
> +	{ XFS_TRANS_CREATE_TMPFILE,	"CREATE_TMPFILE" }, \
>  	{ XLOG_UNMOUNT_REC_TYPE,	"UNMOUNT" }
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 6c1330f..68cb1e7 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -716,17 +716,6 @@ xfs_calc_clear_agi_bucket_reservation(
>  }
>  
>  /*
> - * Clearing the quotaflags in the superblock.
> - *	the super block for changing quota flags: sector size
> - */
> -STATIC uint
> -xfs_calc_qm_sbchange_reservation(
> -	struct xfs_mount	*mp)
> -{
> -	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize);
> -}
> -
> -/*
>   * Adjusting quota limits.
>   *    the xfs_disk_dquot_t: sizeof(struct xfs_disk_dquot)
>   */
> @@ -864,9 +853,6 @@ xfs_trans_resv_calc(
>  	 * The following transactions are logged in logical format with
>  	 * a default log count.
>  	 */
> -	resp->tr_qm_sbchange.tr_logres = xfs_calc_qm_sbchange_reservation(mp);
> -	resp->tr_qm_sbchange.tr_logcount = XFS_DEFAULT_LOG_COUNT;
> -
>  	resp->tr_qm_setqlim.tr_logres = xfs_calc_qm_setqlim_reservation(mp);
>  	resp->tr_qm_setqlim.tr_logcount = XFS_DEFAULT_LOG_COUNT;
>  
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.h b/fs/xfs/libxfs/xfs_trans_resv.h
> index 1097d14..2d5bdfc 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.h
> +++ b/fs/xfs/libxfs/xfs_trans_resv.h
> @@ -56,7 +56,6 @@ struct xfs_trans_resv {
>  	struct xfs_trans_res	tr_growrtalloc;	/* grow realtime allocations */
>  	struct xfs_trans_res	tr_growrtzero;	/* grow realtime zeroing */
>  	struct xfs_trans_res	tr_growrtfree;	/* grow realtime freeing */
> -	struct xfs_trans_res	tr_qm_sbchange;	/* change quota flags */
>  	struct xfs_trans_res	tr_qm_setqlim;	/* adjust quota limits */
>  	struct xfs_trans_res	tr_qm_dqalloc;	/* allocate quota on disk */
>  	struct xfs_trans_res	tr_qm_quotaoff;	/* turn quota off */
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index 82af857..f711452 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -756,35 +756,6 @@ out:
>  	return 0;
>  }
>  
> -/*
> - * Dump a transaction into the log that contains no real change. This is needed
> - * to be able to make the log dirty or stamp the current tail LSN into the log
> - * during the covering operation.
> - *
> - * We cannot use an inode here for this - that will push dirty state back up
> - * into the VFS and then periodic inode flushing will prevent log covering from
> - * making progress. Hence we log a field in the superblock instead and use a
> - * synchronous transaction to ensure the superblock is immediately unpinned
> - * and can be written back.
> - */
> -int
> -xfs_fs_log_dummy(
> -	xfs_mount_t	*mp)
> -{
> -	xfs_trans_t	*tp;
> -	int		error;
> -
> -	tp = _xfs_trans_alloc(mp, XFS_TRANS_DUMMY1, KM_SLEEP);
> -	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_sb, 0, 0);
> -	if (error) {
> -		xfs_trans_cancel(tp, 0);
> -		return error;
> -	}
> -	xfs_mod_sb(tp);
> -	xfs_trans_set_sync(tp);
> -	return xfs_trans_commit(tp, 0);
> -}
> -
>  int
>  xfs_fs_goingdown(
>  	xfs_mount_t	*mp,
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 8fbbfb2..bcc7cfa 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -33,6 +33,7 @@
>  #include "xfs_fsops.h"
>  #include "xfs_cksum.h"
>  #include "xfs_sysfs.h"
> +#include "xfs_sb.h"
>  
>  kmem_zone_t	*xfs_log_ticket_zone;
>  
> @@ -1290,9 +1291,20 @@ xfs_log_worker(
>  	struct xfs_mount	*mp = log->l_mp;
>  
>  	/* dgc: errors ignored - not fatal and nowhere to report them */
> -	if (xfs_log_need_covered(mp))
> -		xfs_fs_log_dummy(mp);
> -	else
> +	if (xfs_log_need_covered(mp)) {
> +		/*
> +		 * Dump a transaction into the log that contains no real change.
> +		 * This is needed to stamp the current tail LSN into the log
> +		 * during the covering operation.
> +		 *
> +		 * We cannot use an inode here for this - that will push dirty
> +		 * state back up into the VFS and then periodic inode flushing
> +		 * will prevent log covering from making progress. Hence we
> +		 * synchronously log the superblock instead to ensure the
> +		 * superblock is immediately unpinned and can be written back.
> +		 */
> +		xfs_sync_sb(mp, true);
> +	} else
>  		xfs_log_force(mp, 0);
>  
>  	/* start pushing all the metadata that is currently dirty */
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index defcf69..5ef9aa2 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -408,11 +408,11 @@ xfs_update_alignment(xfs_mount_t *mp)
>  		if (xfs_sb_version_hasdalign(sbp)) {
>  			if (sbp->sb_unit != mp->m_dalign) {
>  				sbp->sb_unit = mp->m_dalign;
> -				mp->m_update_flags |= XFS_SB_UNIT;
> +				mp->m_update_sb = true;
>  			}
>  			if (sbp->sb_width != mp->m_swidth) {
>  				sbp->sb_width = mp->m_swidth;
> -				mp->m_update_flags |= XFS_SB_WIDTH;
> +				mp->m_update_sb = true;
>  			}
>  		} else {
>  			xfs_warn(mp,
> @@ -583,38 +583,19 @@ int
>  xfs_mount_reset_sbqflags(
>  	struct xfs_mount	*mp)
>  {
> -	int			error;
> -	struct xfs_trans	*tp;
> -
>  	mp->m_qflags = 0;
>  
> -	/*
> -	 * It is OK to look at sb_qflags here in mount path,
> -	 * without m_sb_lock.
> -	 */
> +	/* It is OK to look at sb_qflags in the mount path without m_sb_lock. */
>  	if (mp->m_sb.sb_qflags == 0)
>  		return 0;
>  	spin_lock(&mp->m_sb_lock);
>  	mp->m_sb.sb_qflags = 0;
>  	spin_unlock(&mp->m_sb_lock);
>  
> -	/*
> -	 * If the fs is readonly, let the incore superblock run
> -	 * with quotas off but don't flush the update out to disk
> -	 */
> -	if (mp->m_flags & XFS_MOUNT_RDONLY)
> +	if (!xfs_fs_writable(mp, SB_FREEZE_WRITE))
>  		return 0;
>  
> -	tp = xfs_trans_alloc(mp, XFS_TRANS_QM_SBCHANGE);
> -	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_sbchange, 0, 0);
> -	if (error) {
> -		xfs_trans_cancel(tp, 0);
> -		xfs_alert(mp, "%s: Superblock update failed!", __func__);
> -		return error;
> -	}
> -
> -	xfs_mod_sb(tp);
> -	return xfs_trans_commit(tp, 0);
> +	return xfs_sync_sb(mp, false);
>  }
>  
>  __uint64_t
> @@ -678,7 +659,7 @@ xfs_mountfs(
>  		xfs_warn(mp, "correcting sb_features alignment problem");
>  		sbp->sb_features2 |= sbp->sb_bad_features2;
>  		sbp->sb_bad_features2 = sbp->sb_features2;
> -		mp->m_update_flags |= XFS_SB_FEATURES2;
> +		mp->m_update_sb = true;
>  
>  		/*
>  		 * Re-check for ATTR2 in case it was found in bad_features2
> @@ -692,17 +673,17 @@ xfs_mountfs(
>  	if (xfs_sb_version_hasattr2(&mp->m_sb) &&
>  	   (mp->m_flags & XFS_MOUNT_NOATTR2)) {
>  		xfs_sb_version_removeattr2(&mp->m_sb);
> -		mp->m_update_flags |= XFS_SB_FEATURES2;
> +		mp->m_update_sb = true;
>  
>  		/* update sb_versionnum for the clearing of the morebits */
>  		if (!sbp->sb_features2)
> -			mp->m_update_flags |= XFS_SB_VERSIONNUM;
> +			mp->m_update_sb = true;
>  	}
>  
>  	/* always use v2 inodes by default now */
>  	if (!(mp->m_sb.sb_versionnum & XFS_SB_VERSION_NLINKBIT)) {
>  		mp->m_sb.sb_versionnum |= XFS_SB_VERSION_NLINKBIT;
> -		mp->m_update_flags |= XFS_SB_VERSIONNUM;
> +		mp->m_update_sb = true;
>  	}
>  
>  	/*
> @@ -895,8 +876,8 @@ xfs_mountfs(
>  	 * the next remount into writeable mode.  Otherwise we would never
>  	 * perform the update e.g. for the root filesystem.
>  	 */
> -	if (mp->m_update_flags && !(mp->m_flags & XFS_MOUNT_RDONLY)) {
> -		error = xfs_mount_log_sb(mp);
> +	if (mp->m_update_sb && !(mp->m_flags & XFS_MOUNT_RDONLY)) {
> +		error = xfs_sync_sb(mp, false);
>  		if (error) {
>  			xfs_warn(mp, "failed to write sb changes");
>  			goto out_rtunmount;
> @@ -1103,9 +1084,6 @@ xfs_fs_writable(
>  int
>  xfs_log_sbcount(xfs_mount_t *mp)
>  {
> -	xfs_trans_t	*tp;
> -	int		error;
> -
>  	/* allow this to proceed during the freeze sequence... */
>  	if (!xfs_fs_writable(mp, SB_FREEZE_COMPLETE))
>  		return 0;
> @@ -1119,17 +1097,7 @@ xfs_log_sbcount(xfs_mount_t *mp)
>  	if (!xfs_sb_version_haslazysbcount(&mp->m_sb))
>  		return 0;
>  
> -	tp = _xfs_trans_alloc(mp, XFS_TRANS_SB_COUNT, KM_SLEEP);
> -	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_sb, 0, 0);
> -	if (error) {
> -		xfs_trans_cancel(tp, 0);
> -		return error;
> -	}
> -
> -	xfs_mod_sb(tp);
> -	xfs_trans_set_sync(tp);
> -	error = xfs_trans_commit(tp, 0);
> -	return error;
> +	return xfs_sync_sb(mp, true);
>  }
>  
>  /*
> @@ -1423,28 +1391,6 @@ xfs_freesb(
>  }
>  
>  /*
> - * Used to log changes to the superblock unit and width fields which could
> - * be altered by the mount options, as well as any potential sb_features2
> - * fixup. Only the first superblock is updated.
> - */
> -int
> -xfs_mount_log_sb(
> -	struct xfs_mount	*mp)
> -{
> -	struct xfs_trans	*tp;
> -	int			error;
> -
> -	tp = xfs_trans_alloc(mp, XFS_TRANS_SB_UNIT);
> -	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_sb, 0, 0);
> -	if (error) {
> -		xfs_trans_cancel(tp, 0);
> -		return error;
> -	}
> -	xfs_mod_sb(tp);
> -	return xfs_trans_commit(tp, 0);
> -}
> -
> -/*
>   * If the underlying (data/log/rt) device is readonly, there are some
>   * operations that cannot proceed.
>   */
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index 28b341b..a5b2ff8 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -162,8 +162,7 @@ typedef struct xfs_mount {
>  	struct delayed_work	m_reclaim_work;	/* background inode reclaim */
>  	struct delayed_work	m_eofblocks_work; /* background eof blocks
>  						     trimming */
> -	__int64_t		m_update_flags;	/* sb flags we need to update
> -						   on the next remount,rw */
> +	bool			m_update_sb;	/* sb needs update in mount */
>  	int64_t			m_low_space[XFS_LOWSP_MAX];
>  						/* low free space thresholds */
>  	struct xfs_kobj		m_kobj;
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index c815a80..3e81862 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -792,7 +792,7 @@ xfs_qm_qino_alloc(
>  	else
>  		mp->m_sb.sb_pquotino = (*ip)->i_ino;
>  	spin_unlock(&mp->m_sb_lock);
> -	xfs_mod_sb(tp);
> +	xfs_log_sb(tp);
>  
>  	if ((error = xfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES))) {
>  		xfs_alert(mp, "%s failed (error %d)!", __func__, error);
> @@ -1445,7 +1445,7 @@ xfs_qm_mount_quotas(
>  	spin_unlock(&mp->m_sb_lock);
>  
>  	if (sbf != (mp->m_qflags & XFS_MOUNT_QUOTA_ALL)) {
> -		if (xfs_qm_write_sb_changes(mp)) {
> +		if (xfs_sync_sb(mp, false)) {
>  			/*
>  			 * We could only have been turning quotas off.
>  			 * We aren't in very good shape actually because
> @@ -1574,29 +1574,6 @@ xfs_qm_dqfree_one(
>  	xfs_qm_dqdestroy(dqp);
>  }
>  
> -/*
> - * Start a transaction and write the incore superblock changes to
> - * disk. flags parameter indicates which fields have changed.
> - */
> -int
> -xfs_qm_write_sb_changes(

This guy still has a function declaration in xfs_qm.h. The rest looks
good to me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> -	struct xfs_mount *mp)
> -{
> -	xfs_trans_t	*tp;
> -	int		error;
> -
> -	tp = xfs_trans_alloc(mp, XFS_TRANS_QM_SBCHANGE);
> -	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_qm_sbchange, 0, 0);
> -	if (error) {
> -		xfs_trans_cancel(tp, 0);
> -		return error;
> -	}
> -
> -	xfs_mod_sb(tp);
> -	return xfs_trans_commit(tp, 0);
> -}
> -
> -
>  /* --------------- utility functions for vnodeops ---------------- */
>  
>  
> diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
> index 8d7e5f0..b8a565e 100644
> --- a/fs/xfs/xfs_qm_syscalls.c
> +++ b/fs/xfs/xfs_qm_syscalls.c
> @@ -92,7 +92,7 @@ xfs_qm_scall_quotaoff(
>  		mutex_unlock(&q->qi_quotaofflock);
>  
>  		/* XXX what to do if error ? Revert back to old vals incore ? */
> -		return xfs_qm_write_sb_changes(mp);
> +		return xfs_sync_sb(mp, false);
>  	}
>  
>  	dqtype = 0;
> @@ -369,7 +369,8 @@ xfs_qm_scall_quotaon(
>  	if ((qf & flags) == flags)
>  		return -EEXIST;
>  
> -	if ((error = xfs_qm_write_sb_changes(mp)))
> +	error = xfs_sync_sb(mp, false);
> +	if (error)
>  		return error;
>  	/*
>  	 * If we aren't trying to switch on quota enforcement, we are done.
> @@ -796,7 +797,7 @@ xfs_qm_log_quotaoff(
>  	mp->m_sb.sb_qflags = (mp->m_qflags & ~(flags)) & XFS_MOUNT_QUOTA_ALL;
>  	spin_unlock(&mp->m_sb_lock);
>  
> -	xfs_mod_sb(tp);
> +	xfs_log_sb(tp);
>  
>  	/*
>  	 * We have to make sure that the transaction is secure on disk before we
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 3797a03..afd6bae 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1257,13 +1257,13 @@ xfs_fs_remount(
>  		 * If this is the first remount to writeable state we
>  		 * might have some superblock changes to update.
>  		 */
> -		if (mp->m_update_flags) {
> -			error = xfs_mount_log_sb(mp);
> +		if (mp->m_update_sb) {
> +			error = xfs_sync_sb(mp, false);
>  			if (error) {
>  				xfs_warn(mp, "failed to write sb changes");
>  				return error;
>  			}
> -			mp->m_update_flags = 0;
> +			mp->m_update_sb = false;
>  		}
>  
>  		/*
> @@ -1293,8 +1293,9 @@ xfs_fs_remount(
>  
>  /*
>   * Second stage of a freeze. The data is already frozen so we only
> - * need to take care of the metadata. Once that's done write a dummy
> - * record to dirty the log in case of a crash while frozen.
> + * need to take care of the metadata. Once that's done sync the superblock
> + * to the log to dirty it in case of a crash while frozen. This ensures that we
> + * will recover the unlinked inode lists on the next mount.
>   */
>  STATIC int
>  xfs_fs_freeze(
> @@ -1304,7 +1305,7 @@ xfs_fs_freeze(
>  
>  	xfs_save_resvblks(mp);
>  	xfs_quiesce_attr(mp);
> -	return xfs_fs_log_dummy(mp);
> +	return xfs_sync_sb(mp, true);
>  }
>  
>  STATIC int
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/3] xfs: consolidate superblock logging functions

[Dave Chinner]

On Thu, Jan 08, 2015 at 09:36:33AM -0500, Brian Foster wrote:
> On Thu, Jan 08, 2015 at 08:51:04AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > We now have several superblock loggin functions that are identical
> > except for the transaction reservation and whether it shoul dbe a
> > synchronous transaction or not. Consolidate these all into a single
> > function, a single reserveration and a sync flag and call it
> > xfs_sync_sb().
> > 
> > Also, xfs_mod_sb() is not really a modification function - it's the
> > operation of logging the superblock buffer. hence change the name of
> > it to reflect this.
> > 
> > Note that we have to change the mp->m_update_flags that are passed
> > around at mount time to a boolean simply to indicate a superblock
> > update is needed.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> 
> A question and one other little thing...

[snip]

> >  #define XFS_TRANS_QM_DQCLUSTER		32
> >  #define XFS_TRANS_QM_QINOCREATE		33
> >  #define XFS_TRANS_QM_QUOTAOFF_END	34
> > -#define XFS_TRANS_SB_UNIT		35
> > -#define XFS_TRANS_FSYNC_TS		36
> > -#define	XFS_TRANS_GROWFSRT_ALLOC	37
> > -#define	XFS_TRANS_GROWFSRT_ZERO		38
> > -#define	XFS_TRANS_GROWFSRT_FREE		39
> > -#define	XFS_TRANS_SWAPEXT		40
> > -#define	XFS_TRANS_SB_COUNT		41
> > -#define	XFS_TRANS_CHECKPOINT		42
> > -#define	XFS_TRANS_ICREATE		43
> > -#define	XFS_TRANS_CREATE_TMPFILE	44
> > -#define	XFS_TRANS_TYPE_MAX		44
> > +#define XFS_TRANS_FSYNC_TS		35
> > +#define	XFS_TRANS_GROWFSRT_ALLOC	36
> > +#define	XFS_TRANS_GROWFSRT_ZERO		37
> > +#define	XFS_TRANS_GROWFSRT_FREE		38
> > +#define	XFS_TRANS_SWAPEXT		39
> > +#define	XFS_TRANS_CHECKPOINT		40
> > +#define	XFS_TRANS_ICREATE		41
> > +#define	XFS_TRANS_CREATE_TMPFILE	42
> > +#define	XFS_TRANS_TYPE_MAX		43
> >  /* new transaction types need to be reflected in xfs_logprint(8) */
> >  
> >  #define XFS_TRANS_TYPES \
> > @@ -113,7 +111,6 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
> >  	{ XFS_TRANS_SETATTR_SIZE,	"SETATTR_SIZE" }, \
> >  	{ XFS_TRANS_INACTIVE,		"INACTIVE" }, \
> >  	{ XFS_TRANS_CREATE,		"CREATE" }, \
> > -	{ XFS_TRANS_CREATE_TMPFILE,	"CREATE_TMPFILE" }, \
> >  	{ XFS_TRANS_CREATE_TRUNC,	"CREATE_TRUNC" }, \
> >  	{ XFS_TRANS_TRUNCATE_FILE,	"TRUNCATE_FILE" }, \
> >  	{ XFS_TRANS_REMOVE,		"REMOVE" }, \
> > @@ -134,23 +131,23 @@ extern const struct xfs_buf_ops xfs_symlink_buf_ops;
> >  	{ XFS_TRANS_ATTR_RM,		"ATTR_RM" }, \
> >  	{ XFS_TRANS_ATTR_FLAG,		"ATTR_FLAG" }, \
> >  	{ XFS_TRANS_CLEAR_AGI_BUCKET,	"CLEAR_AGI_BUCKET" }, \
> > -	{ XFS_TRANS_QM_SBCHANGE,	"QM_SBCHANGE" }, \
> > +	{ XFS_TRANS_SB_CHANGE,		"SBCHANGE" }, \
> > +	{ XFS_TRANS_DUMMY1,		"DUMMY1" }, \
> > +	{ XFS_TRANS_DUMMY2,		"DUMMY2" }, \
> 
> I take it we can't just remove these dummy types, for userspace
> compatibility reasons, right? At least, it looks like logprint could get
> confused if other transaction types inherited these values.

With delayed logging, userspace will never see these as they never
get to disk. Userspace will only ever see checkpoints and unmount
records. My plan is to eventually remove this stuff from the kernel
(it isn['t used anymore except in tracing) and define it in
xfs_logprint in a way that works for current and legacy filesystems.
So for the moment, I'm just making the kernel/user values
consistent.

> > @@ -1574,29 +1574,6 @@ xfs_qm_dqfree_one(
> >  	xfs_qm_dqdestroy(dqp);
> >  }
> >  
> > -/*
> > - * Start a transaction and write the incore superblock changes to
> > - * disk. flags parameter indicates which fields have changed.
> > - */
> > -int
> > -xfs_qm_write_sb_changes(
> 
> This guy still has a function declaration in xfs_qm.h. The rest looks
> good to me:

Good catch, I'll kill it.

> Reviewed-by: Brian Foster <bfoster@redhat.com>

Thanks!

-Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/3] xfs: sanitise sb_bad_features2 handling

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

We currently have to ensure that every time we update sb_features2
that we update sb_bad_features2. Now that we log and format the
superblock in it's entirety we actually don't have to care because
we can simply update the sb_bad_features2 when we format it into the
buffer. This removes the need for anything but the mount and
superblock formatting code to care about sb_bad_features2, and
hence removes the possibility that we forget to update bad_features2
when necessary in the future.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_format.h | 14 +++++++-------
 fs/xfs/libxfs/xfs_sb.c     |  8 +++++++-
 fs/xfs/xfs_mount.c         | 23 +++++++++++------------
 3 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 4762732..8eb7189 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -151,10 +151,13 @@ typedef struct xfs_sb {
 	__uint32_t	sb_features2;	/* additional feature bits */
 
 	/*
-	 * bad features2 field as a result of failing to pad the sb
-	 * structure to 64 bits. Some machines will be using this field
-	 * for features2 bits. Easiest just to mark it bad and not use
-	 * it for anything else.
+	 * bad features2 field as a result of failing to pad the sb structure to
+	 * 64 bits. Some machines will be using this field for features2 bits.
+	 * Easiest just to mark it bad and not use it for anything else.
+	 *
+	 * This is not kept up to date in memory; it is always overwritten by
+	 * the value in sb_features2 when formatting the incore superblock to
+	 * the disk buffer.
 	 */
 	__uint32_t	sb_bad_features2;
 
@@ -453,13 +456,11 @@ static inline void xfs_sb_version_addattr2(struct xfs_sb *sbp)
 {
 	sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT;
 	sbp->sb_features2 |= XFS_SB_VERSION2_ATTR2BIT;
-	sbp->sb_bad_features2 |= XFS_SB_VERSION2_ATTR2BIT;
 }
 
 static inline void xfs_sb_version_removeattr2(struct xfs_sb *sbp)
 {
 	sbp->sb_features2 &= ~XFS_SB_VERSION2_ATTR2BIT;
-	sbp->sb_bad_features2 &= ~XFS_SB_VERSION2_ATTR2BIT;
 	if (!sbp->sb_features2)
 		sbp->sb_versionnum &= ~XFS_SB_VERSION_MOREBITSBIT;
 }
@@ -475,7 +476,6 @@ static inline void xfs_sb_version_addprojid32bit(struct xfs_sb *sbp)
 {
 	sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT;
 	sbp->sb_features2 |= XFS_SB_VERSION2_PROJID32BIT;
-	sbp->sb_bad_features2 |= XFS_SB_VERSION2_PROJID32BIT;
 }
 
 /*
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 6ee3af6..b440839 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -488,7 +488,6 @@ xfs_sb_to_disk(
 	to->sb_fdblocks = cpu_to_be64(from->sb_fdblocks);
 	to->sb_frextents = cpu_to_be64(from->sb_frextents);
 
-
 	to->sb_flags = from->sb_flags;
 	to->sb_shared_vn = from->sb_shared_vn;
 	to->sb_inoalignmt = cpu_to_be32(from->sb_inoalignmt);
@@ -498,6 +497,13 @@ xfs_sb_to_disk(
 	to->sb_logsectlog = from->sb_logsectlog;
 	to->sb_logsectsize = cpu_to_be16(from->sb_logsectsize);
 	to->sb_logsunit = cpu_to_be32(from->sb_logsunit);
+
+	/*
+	 * We need to ensure that bad_features2 always matches features2.
+	 * Hence we enforce that here rather than having to remember to do it
+	 * everywhere else that updates features2.
+	 */
+	from->sb_bad_features2 = from->sb_features2;
 	to->sb_features2 = cpu_to_be32(from->sb_features2);
 	to->sb_bad_features2 = cpu_to_be32(from->sb_bad_features2);
 
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 5ef9aa2..4fa80e6 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -640,25 +640,24 @@ xfs_mountfs(
 	xfs_sb_mount_common(mp, sbp);
 
 	/*
-	 * Check for a mismatched features2 values.  Older kernels
-	 * read & wrote into the wrong sb offset for sb_features2
-	 * on some platforms due to xfs_sb_t not being 64bit size aligned
-	 * when sb_features2 was added, which made older superblock
-	 * reading/writing routines swap it as a 64-bit value.
+	 * Check for a mismatched features2 values.  Older kernels read & wrote
+	 * into the wrong sb offset for sb_features2 on some platforms due to
+	 * xfs_sb_t not being 64bit size aligned when sb_features2 was added,
+	 * which made older superblock reading/writing routines swap it as a
+	 * 64-bit value.
 	 *
 	 * For backwards compatibility, we make both slots equal.
 	 *
-	 * If we detect a mismatched field, we OR the set bits into the
-	 * existing features2 field in case it has already been modified; we
-	 * don't want to lose any features.  We then update the bad location
-	 * with the ORed value so that older kernels will see any features2
-	 * flags, and mark the two fields as needing updates once the
-	 * transaction subsystem is online.
+	 * If we detect a mismatched field, we OR the set bits into the existing
+	 * features2 field in case it has already been modified; we don't want
+	 * to lose any features.  We then update the bad location with the ORed
+	 * value so that older kernels will see any features2 flags. The
+	 * superblock writeback code ensures the new sb_features2 is copied to
+	 * sb_bad_features2 before it is logged or written to disk.
 	 */
 	if (xfs_sb_has_mismatched_features2(sbp)) {
 		xfs_warn(mp, "correcting sb_features alignment problem");
 		sbp->sb_features2 |= sbp->sb_bad_features2;
-		sbp->sb_bad_features2 = sbp->sb_features2;
 		mp->m_update_sb = true;
 
 		/*
-- 
2.0.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] xfs: sanitise sb_bad_features2 handling

[Brian Foster]

On Thu, Jan 08, 2015 at 08:51:05AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> We currently have to ensure that every time we update sb_features2
> that we update sb_bad_features2. Now that we log and format the
> superblock in it's entirety we actually don't have to care because
> we can simply update the sb_bad_features2 when we format it into the
> buffer. This removes the need for anything but the mount and
> superblock formatting code to care about sb_bad_features2, and
> hence removes the possibility that we forget to update bad_features2
> when necessary in the future.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_format.h | 14 +++++++-------
>  fs/xfs/libxfs/xfs_sb.c     |  8 +++++++-
>  fs/xfs/xfs_mount.c         | 23 +++++++++++------------
>  3 files changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 4762732..8eb7189 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -151,10 +151,13 @@ typedef struct xfs_sb {
>  	__uint32_t	sb_features2;	/* additional feature bits */
>  
>  	/*
> -	 * bad features2 field as a result of failing to pad the sb
> -	 * structure to 64 bits. Some machines will be using this field
> -	 * for features2 bits. Easiest just to mark it bad and not use
> -	 * it for anything else.
> +	 * bad features2 field as a result of failing to pad the sb structure to
> +	 * 64 bits. Some machines will be using this field for features2 bits.
> +	 * Easiest just to mark it bad and not use it for anything else.
> +	 *
> +	 * This is not kept up to date in memory; it is always overwritten by
> +	 * the value in sb_features2 when formatting the incore superblock to
> +	 * the disk buffer.
>  	 */
>  	__uint32_t	sb_bad_features2;
>  
> @@ -453,13 +456,11 @@ static inline void xfs_sb_version_addattr2(struct xfs_sb *sbp)
>  {
>  	sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT;
>  	sbp->sb_features2 |= XFS_SB_VERSION2_ATTR2BIT;
> -	sbp->sb_bad_features2 |= XFS_SB_VERSION2_ATTR2BIT;
>  }
>  
>  static inline void xfs_sb_version_removeattr2(struct xfs_sb *sbp)
>  {
>  	sbp->sb_features2 &= ~XFS_SB_VERSION2_ATTR2BIT;
> -	sbp->sb_bad_features2 &= ~XFS_SB_VERSION2_ATTR2BIT;
>  	if (!sbp->sb_features2)
>  		sbp->sb_versionnum &= ~XFS_SB_VERSION_MOREBITSBIT;
>  }
> @@ -475,7 +476,6 @@ static inline void xfs_sb_version_addprojid32bit(struct xfs_sb *sbp)
>  {
>  	sbp->sb_versionnum |= XFS_SB_VERSION_MOREBITSBIT;
>  	sbp->sb_features2 |= XFS_SB_VERSION2_PROJID32BIT;
> -	sbp->sb_bad_features2 |= XFS_SB_VERSION2_PROJID32BIT;
>  }
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 6ee3af6..b440839 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -488,7 +488,6 @@ xfs_sb_to_disk(
>  	to->sb_fdblocks = cpu_to_be64(from->sb_fdblocks);
>  	to->sb_frextents = cpu_to_be64(from->sb_frextents);
>  
> -
>  	to->sb_flags = from->sb_flags;
>  	to->sb_shared_vn = from->sb_shared_vn;
>  	to->sb_inoalignmt = cpu_to_be32(from->sb_inoalignmt);
> @@ -498,6 +497,13 @@ xfs_sb_to_disk(
>  	to->sb_logsectlog = from->sb_logsectlog;
>  	to->sb_logsectsize = cpu_to_be16(from->sb_logsectsize);
>  	to->sb_logsunit = cpu_to_be32(from->sb_logsunit);
> +
> +	/*
> +	 * We need to ensure that bad_features2 always matches features2.
> +	 * Hence we enforce that here rather than having to remember to do it
> +	 * everywhere else that updates features2.
> +	 */
> +	from->sb_bad_features2 = from->sb_features2;
>  	to->sb_features2 = cpu_to_be32(from->sb_features2);
>  	to->sb_bad_features2 = cpu_to_be32(from->sb_bad_features2);
>  
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 5ef9aa2..4fa80e6 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -640,25 +640,24 @@ xfs_mountfs(
>  	xfs_sb_mount_common(mp, sbp);
>  
>  	/*
> -	 * Check for a mismatched features2 values.  Older kernels
> -	 * read & wrote into the wrong sb offset for sb_features2
> -	 * on some platforms due to xfs_sb_t not being 64bit size aligned
> -	 * when sb_features2 was added, which made older superblock
> -	 * reading/writing routines swap it as a 64-bit value.
> +	 * Check for a mismatched features2 values.  Older kernels read & wrote
> +	 * into the wrong sb offset for sb_features2 on some platforms due to
> +	 * xfs_sb_t not being 64bit size aligned when sb_features2 was added,
> +	 * which made older superblock reading/writing routines swap it as a
> +	 * 64-bit value.
>  	 *
>  	 * For backwards compatibility, we make both slots equal.
>  	 *
> -	 * If we detect a mismatched field, we OR the set bits into the
> -	 * existing features2 field in case it has already been modified; we
> -	 * don't want to lose any features.  We then update the bad location
> -	 * with the ORed value so that older kernels will see any features2
> -	 * flags, and mark the two fields as needing updates once the
> -	 * transaction subsystem is online.
> +	 * If we detect a mismatched field, we OR the set bits into the existing
> +	 * features2 field in case it has already been modified; we don't want
> +	 * to lose any features.  We then update the bad location with the ORed
> +	 * value so that older kernels will see any features2 flags. The
> +	 * superblock writeback code ensures the new sb_features2 is copied to
> +	 * sb_bad_features2 before it is logged or written to disk.
>  	 */
>  	if (xfs_sb_has_mismatched_features2(sbp)) {
>  		xfs_warn(mp, "correcting sb_features alignment problem");
>  		sbp->sb_features2 |= sbp->sb_bad_features2;
> -		sbp->sb_bad_features2 = sbp->sb_features2;
>  		mp->m_update_sb = true;
>  
>  		/*
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs