diff mbox

[24/25] xfs: scrub realtime bitmap/summary

Message ID 150706340125.19351.13494381752104107446.stgit@magnolia (mailing list archive)
State Superseded
Headers show

Commit Message

Darrick J. Wong Oct. 3, 2017, 8:43 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Perform simple tests of the realtime bitmap and summary.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/Makefile            |    2 +
 fs/xfs/libxfs/xfs_format.h |    5 ++
 fs/xfs/libxfs/xfs_fs.h     |    4 +-
 fs/xfs/scrub/common.h      |    1 
 fs/xfs/scrub/rtbitmap.c    |   98 ++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/scrub/scrub.c       |   15 +++++++
 fs/xfs/scrub/scrub.h       |    2 +
 7 files changed, 126 insertions(+), 1 deletion(-)
 create mode 100644 fs/xfs/scrub/rtbitmap.c



--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Dave Chinner Oct. 9, 2017, 2:28 a.m. UTC | #1 
On Tue, Oct 03, 2017 at 01:43:21PM -0700, Darrick J. Wong wrote:
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 154c3dd..d4d9bef 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -315,6 +315,11 @@ static inline bool xfs_sb_good_version(struct xfs_sb *sbp)
>  	return false;
>  }
>  
> +static inline bool xfs_sb_version_hasrealtime(struct xfs_sb *sbp)
> +{
> +	return sbp->sb_rblocks > 0;
> +}

How much can we rely on that? do we allow a fs to mount with that
being > 0 but no rtdev= mount option?

> +/* Set us up with the realtime metadata locked. */
> +int
> +xfs_scrub_setup_rt(
> +	struct xfs_scrub_context	*sc,
> +	struct xfs_inode		*ip)
> +{
> +	struct xfs_mount		*mp = sc->mp;
> +	int				lockmode;
> +	int				error = 0;
> +
> +	if (sc->sm->sm_agno || sc->sm->sm_ino || sc->sm->sm_gen)
> +		return -EINVAL;

I've forgotten what this means already :/

> +	error = xfs_scrub_setup_fs(sc, ip);
> +	if (error)
> +		return error;
> +
> +	lockmode = XFS_ILOCK_EXCL | XFS_ILOCK_RTBITMAP;
> +	xfs_ilock(mp->m_rbmip, lockmode);
> +	xfs_trans_ijoin(sc->tp, mp->m_rbmip, lockmode);

Ok, so why do we join this inode to the transaction and not use
the sc->ilock_flags field to track how we've locked it?

> +
> +	return 0;
> +}
> +
> +/* Realtime bitmap. */
> +
> +/* Scrub a free extent record from the realtime bitmap. */
> +STATIC int
> +xfs_scrub_rtbitmap_helper(
> +	struct xfs_trans		*tp,
> +	struct xfs_rtalloc_rec		*rec,
> +	void				*priv)
> +{
> +	return 0;
> +}

Check the extent record returned is within the range of the rtdev
address space?

> +
> +/* Scrub the realtime bitmap. */
> +int
> +xfs_scrub_rtbitmap(
> +	struct xfs_scrub_context	*sc)
> +{
> +	int				error;
> +
> +	error = xfs_rtalloc_query_all(sc->tp, xfs_scrub_rtbitmap_helper, NULL);
> +	if (!xfs_scrub_fblock_op_ok(sc, XFS_DATA_FORK, 0, &error))
> +		goto out;
> +
> +out:
> +	return error;
> +}
> +
> +/* Scrub the realtime summary. */
> +int
> +xfs_scrub_rtsummary(
> +	struct xfs_scrub_context	*sc)
> +{
> +	/* XXX: implement this some day */
> +	return -ENOENT;
> +}

Alright, this is all just a stub that doesn't really do any real
scrubbing yet. I guess it's better that nothing in that it walks
the rtbitmap....

> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -241,6 +241,21 @@ static const struct xfs_scrub_meta_ops meta_scrub_ops[] = {
>  		.setup	= xfs_scrub_setup_parent,
>  		.scrub	= xfs_scrub_parent,
>  	},
> +#ifdef CONFIG_XFS_RT
> +	{ /* realtime bitmap */
> +		.setup	= xfs_scrub_setup_rt,
> +		.scrub	= xfs_scrub_rtbitmap,
> +		.has	= xfs_sb_version_hasrealtime,
> +	},
> +	{ /* realtime summary */
> +		.setup	= xfs_scrub_setup_rt,
> +		.scrub	= xfs_scrub_rtsummary,
> +		.has	= xfs_sb_version_hasrealtime,
> +	},
> +#else
> +	{ NULL },
> +	{ NULL },
> +#endif

I think I'd prefer that you supply stub functions when
CONFIG_XFS_RT=n so this table doesn't require ifdefs.

Cheers,

Dave.
Darrick J. Wong Oct. 9, 2017, 8:24 p.m. UTC | #2 
On Mon, Oct 09, 2017 at 01:28:04PM +1100, Dave Chinner wrote:
> On Tue, Oct 03, 2017 at 01:43:21PM -0700, Darrick J. Wong wrote:
> > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> > index 154c3dd..d4d9bef 100644
> > --- a/fs/xfs/libxfs/xfs_format.h
> > +++ b/fs/xfs/libxfs/xfs_format.h
> > @@ -315,6 +315,11 @@ static inline bool xfs_sb_good_version(struct xfs_sb *sbp)
> >  	return false;
> >  }
> >  
> > +static inline bool xfs_sb_version_hasrealtime(struct xfs_sb *sbp)
> > +{
> > +	return sbp->sb_rblocks > 0;
> > +}
> 
> How much can we rely on that? do we allow a fs to mount with that
> being > 0 but no rtdev= mount option?
> 
> > +/* Set us up with the realtime metadata locked. */
> > +int
> > +xfs_scrub_setup_rt(
> > +	struct xfs_scrub_context	*sc,
> > +	struct xfs_inode		*ip)
> > +{
> > +	struct xfs_mount		*mp = sc->mp;
> > +	int				lockmode;
> > +	int				error = 0;
> > +
> > +	if (sc->sm->sm_agno || sc->sm->sm_ino || sc->sm->sm_gen)
> > +		return -EINVAL;
> 
> I've forgotten what this means already :/

(I fixed all of these the first time you complained. :))

> > +	error = xfs_scrub_setup_fs(sc, ip);
> > +	if (error)
> > +		return error;
> > +
> > +	lockmode = XFS_ILOCK_EXCL | XFS_ILOCK_RTBITMAP;
> > +	xfs_ilock(mp->m_rbmip, lockmode);
> > +	xfs_trans_ijoin(sc->tp, mp->m_rbmip, lockmode);
> 
> Ok, so why do we join this inode to the transaction and not use
> the sc->ilock_flags field to track how we've locked it?

I don't know why.

It might just be a forgotten leftover from when I started tracking
inodes in the scrub context.

> > +
> > +	return 0;
> > +}
> > +
> > +/* Realtime bitmap. */
> > +
> > +/* Scrub a free extent record from the realtime bitmap. */
> > +STATIC int
> > +xfs_scrub_rtbitmap_helper(
> > +	struct xfs_trans		*tp,
> > +	struct xfs_rtalloc_rec		*rec,
> > +	void				*priv)
> > +{
> > +	return 0;
> > +}
> 
> Check the extent record returned is within the range of the rtdev
> address space?

I added:

if (rec->ar_startblock + rec->ar_blockcount <= rec->ar_startblock ||
    !xfs_verify_rtbno_ptr(sc->mp, rec->ar_startblock) ||
    !xfs_verify_rtbno_ptr(sc->mp, rec->ar_startblock +
		rec->ar_blockcount - 1))
	xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, 0);

...back when I was reworking the scrub patches to add verify_agbno_ptr and
declutter the bnobt scrubbers.

> > +
> > +/* Scrub the realtime bitmap. */
> > +int
> > +xfs_scrub_rtbitmap(
> > +	struct xfs_scrub_context	*sc)
> > +{
> > +	int				error;
> > +
> > +	error = xfs_rtalloc_query_all(sc->tp, xfs_scrub_rtbitmap_helper, NULL);
> > +	if (!xfs_scrub_fblock_op_ok(sc, XFS_DATA_FORK, 0, &error))
> > +		goto out;
> > +
> > +out:
> > +	return error;
> > +}
> > +
> > +/* Scrub the realtime summary. */
> > +int
> > +xfs_scrub_rtsummary(
> > +	struct xfs_scrub_context	*sc)
> > +{
> > +	/* XXX: implement this some day */
> > +	return -ENOENT;
> > +}
> 
> Alright, this is all just a stub that doesn't really do any real
> scrubbing yet. I guess it's better that nothing in that it walks
> the rtbitmap....
> 
> > --- a/fs/xfs/scrub/scrub.c
> > +++ b/fs/xfs/scrub/scrub.c
> > @@ -241,6 +241,21 @@ static const struct xfs_scrub_meta_ops meta_scrub_ops[] = {
> >  		.setup	= xfs_scrub_setup_parent,
> >  		.scrub	= xfs_scrub_parent,
> >  	},
> > +#ifdef CONFIG_XFS_RT
> > +	{ /* realtime bitmap */
> > +		.setup	= xfs_scrub_setup_rt,
> > +		.scrub	= xfs_scrub_rtbitmap,
> > +		.has	= xfs_sb_version_hasrealtime,
> > +	},
> > +	{ /* realtime summary */
> > +		.setup	= xfs_scrub_setup_rt,
> > +		.scrub	= xfs_scrub_rtsummary,
> > +		.has	= xfs_sb_version_hasrealtime,
> > +	},
> > +#else
> > +	{ NULL },
> > +	{ NULL },
> > +#endif
> 
> I think I'd prefer that you supply stub functions when
> CONFIG_XFS_RT=n so this table doesn't require ifdefs.

Ok.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/xfs/Makefile b/fs/xfs/Makefile
index 2193a54..9ce581e 100644
--- a/fs/xfs/Makefile
+++ b/fs/xfs/Makefile
@@ -162,4 +162,6 @@  xfs-y				+= $(addprefix scrub/, \
 				   scrub.o \
 				   symlink.o \
 				   )
+
+xfs-$(CONFIG_XFS_RT)		+= scrub/rtbitmap.o
 endif
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 154c3dd..d4d9bef 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -315,6 +315,11 @@  static inline bool xfs_sb_good_version(struct xfs_sb *sbp)
 	return false;
 }
 
+static inline bool xfs_sb_version_hasrealtime(struct xfs_sb *sbp)
+{
+	return sbp->sb_rblocks > 0;
+}
+
 /*
  * Detect a mismatched features2 field.  Older kernels read/wrote
  * this into the wrong slot, so to be safe we keep them in sync.
diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
index 7444094..f8bac92 100644
--- a/fs/xfs/libxfs/xfs_fs.h
+++ b/fs/xfs/libxfs/xfs_fs.h
@@ -502,9 +502,11 @@  struct xfs_scrub_metadata {
 #define XFS_SCRUB_TYPE_XATTR	16	/* extended attribute */
 #define XFS_SCRUB_TYPE_SYMLINK	17	/* symbolic link */
 #define XFS_SCRUB_TYPE_PARENT	18	/* parent pointers */
+#define XFS_SCRUB_TYPE_RTBITMAP	19	/* realtime bitmap */
+#define XFS_SCRUB_TYPE_RTSUM	20	/* realtime summary */
 
 /* Number of scrub subcommands. */
-#define XFS_SCRUB_TYPE_NR	19
+#define XFS_SCRUB_TYPE_NR	21
 
 /* i: Repair this metadata. */
 #define XFS_SCRUB_IFLAG_REPAIR		(1 << 0)
diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
index bde5add..f5f8d70 100644
--- a/fs/xfs/scrub/common.h
+++ b/fs/xfs/scrub/common.h
@@ -102,6 +102,7 @@  int xfs_scrub_setup_symlink(struct xfs_scrub_context *sc,
 			    struct xfs_inode *ip);
 int xfs_scrub_setup_parent(struct xfs_scrub_context *sc,
 			   struct xfs_inode *ip);
+int xfs_scrub_setup_rt(struct xfs_scrub_context *sc, struct xfs_inode *ip);
 
 void xfs_scrub_ag_free(struct xfs_scrub_context *sc, struct xfs_scrub_ag *sa);
 int xfs_scrub_ag_init(struct xfs_scrub_context *sc, xfs_agnumber_t agno,
diff --git a/fs/xfs/scrub/rtbitmap.c b/fs/xfs/scrub/rtbitmap.c
new file mode 100644
index 0000000..e15f8f3
--- /dev/null
+++ b/fs/xfs/scrub/rtbitmap.c
@@ -0,0 +1,98 @@ 
+/*
+ * Copyright (C) 2017 Oracle.  All Rights Reserved.
+ *
+ * Author: Darrick J. Wong <darrick.wong@oracle.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+#include "xfs.h"
+#include "xfs_fs.h"
+#include "xfs_shared.h"
+#include "xfs_format.h"
+#include "xfs_trans_resv.h"
+#include "xfs_mount.h"
+#include "xfs_defer.h"
+#include "xfs_btree.h"
+#include "xfs_bit.h"
+#include "xfs_log_format.h"
+#include "xfs_trans.h"
+#include "xfs_sb.h"
+#include "xfs_alloc.h"
+#include "xfs_rtalloc.h"
+#include "xfs_inode.h"
+#include "scrub/xfs_scrub.h"
+#include "scrub/scrub.h"
+#include "scrub/common.h"
+#include "scrub/trace.h"
+
+/* Set us up with the realtime metadata locked. */
+int
+xfs_scrub_setup_rt(
+	struct xfs_scrub_context	*sc,
+	struct xfs_inode		*ip)
+{
+	struct xfs_mount		*mp = sc->mp;
+	int				lockmode;
+	int				error = 0;
+
+	if (sc->sm->sm_agno || sc->sm->sm_ino || sc->sm->sm_gen)
+		return -EINVAL;
+
+	error = xfs_scrub_setup_fs(sc, ip);
+	if (error)
+		return error;
+
+	lockmode = XFS_ILOCK_EXCL | XFS_ILOCK_RTBITMAP;
+	xfs_ilock(mp->m_rbmip, lockmode);
+	xfs_trans_ijoin(sc->tp, mp->m_rbmip, lockmode);
+
+	return 0;
+}
+
+/* Realtime bitmap. */
+
+/* Scrub a free extent record from the realtime bitmap. */
+STATIC int
+xfs_scrub_rtbitmap_helper(
+	struct xfs_trans		*tp,
+	struct xfs_rtalloc_rec		*rec,
+	void				*priv)
+{
+	return 0;
+}
+
+/* Scrub the realtime bitmap. */
+int
+xfs_scrub_rtbitmap(
+	struct xfs_scrub_context	*sc)
+{
+	int				error;
+
+	error = xfs_rtalloc_query_all(sc->tp, xfs_scrub_rtbitmap_helper, NULL);
+	if (!xfs_scrub_fblock_op_ok(sc, XFS_DATA_FORK, 0, &error))
+		goto out;
+
+out:
+	return error;
+}
+
+/* Scrub the realtime summary. */
+int
+xfs_scrub_rtsummary(
+	struct xfs_scrub_context	*sc)
+{
+	/* XXX: implement this some day */
+	return -ENOENT;
+}
diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index 5d57a5b..348e3c3 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -241,6 +241,21 @@  static const struct xfs_scrub_meta_ops meta_scrub_ops[] = {
 		.setup	= xfs_scrub_setup_parent,
 		.scrub	= xfs_scrub_parent,
 	},
+#ifdef CONFIG_XFS_RT
+	{ /* realtime bitmap */
+		.setup	= xfs_scrub_setup_rt,
+		.scrub	= xfs_scrub_rtbitmap,
+		.has	= xfs_sb_version_hasrealtime,
+	},
+	{ /* realtime summary */
+		.setup	= xfs_scrub_setup_rt,
+		.scrub	= xfs_scrub_rtsummary,
+		.has	= xfs_sb_version_hasrealtime,
+	},
+#else
+	{ NULL },
+	{ NULL },
+#endif
 };
 
 /* This isn't a stable feature, warn once per day. */
diff --git a/fs/xfs/scrub/scrub.h b/fs/xfs/scrub/scrub.h
index a264810..329c169 100644
--- a/fs/xfs/scrub/scrub.h
+++ b/fs/xfs/scrub/scrub.h
@@ -87,5 +87,7 @@  int xfs_scrub_directory(struct xfs_scrub_context *sc);
 int xfs_scrub_xattr(struct xfs_scrub_context *sc);
 int xfs_scrub_symlink(struct xfs_scrub_context *sc);
 int xfs_scrub_parent(struct xfs_scrub_context *sc);
+int xfs_scrub_rtbitmap(struct xfs_scrub_context *sc);
+int xfs_scrub_rtsummary(struct xfs_scrub_context *sc);
 
 #endif	/* __XFS_SCRUB_SCRUB_H__ */