[PATCH 00/13] xfsprogs: roll-up of previously sent patches

[PATCH 00/13] xfsprogs: roll-up of previously sent patches

[Eric Sandeen]

Sending these in one series to make it easier on Dave. Some already have
reviews from the prior posting, so I took the liberty of including the
reviewed-bys if they existed.

I'm still working on some refcounting problems in repair, but figured I'd
get this much out there.

Thanks,
-Eric

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

[PATCH 01/13] xfs_db: Allow writes of corrupted data by optionally skipping write verifiers

[Eric Sandeen]

Being able to write corrupt data is handy if we wish to test
repair against specific types of corruptions.

Add a "-c" option to the write command which allows this.

Note that this also skips CRC updates; it's not currently possible
to write invalid data with a valid CRC; CRC recalculation is
intertwined with validation.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 db/io.c           |    7 +++++++
 db/io.h           |    1 +
 db/write.c        |   35 +++++++++++++++++++++++++++++++----
 man/man8/xfs_db.8 |    8 +++++++-
 4 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/db/io.c b/db/io.c
index 7f1b76a..c5898f1 100644
--- a/db/io.c
+++ b/db/io.c
@@ -457,6 +457,13 @@ write_cur_bbs(void)
 }
 
 void
+xfs_dummy_verify(
+	struct xfs_buf *bp)
+{
+	return;
+}
+
+void
 write_cur(void)
 {
 	if (iocur_sp < 0) {
diff --git a/db/io.h b/db/io.h
index 71082e6..31d96b4 100644
--- a/db/io.h
+++ b/db/io.h
@@ -63,6 +63,7 @@ extern void	set_cur(const struct typ *t, __int64_t d, int c, int ring_add,
 			bbmap_t *bbmap);
 extern void     ring_add(void);
 extern void	set_iocur_type(const struct typ *t);
+extern void	xfs_dummy_verify(struct xfs_buf *bp);
 
 /*
  * returns -1 for unchecked, 0 for bad and 1 for good
diff --git a/db/write.c b/db/write.c
index a0f14f4..4511859 100644
--- a/db/write.c
+++ b/db/write.c
@@ -38,7 +38,7 @@ static int	write_f(int argc, char **argv);
 static void     write_help(void);
 
 static const cmdinfo_t	write_cmd =
-	{ "write", NULL, write_f, 0, -1, 0, N_("[field or value]..."),
+	{ "write", NULL, write_f, 0, -1, 0, N_("[-c] [field or value]..."),
 	  N_("write value to disk"), write_help };
 
 void
@@ -79,6 +79,7 @@ write_help(void)
 "  String mode: 'write \"This_is_a_filename\" - write null terminated string.\n"
 "\n"
 " In data mode type 'write' by itself for a list of specific commands.\n\n"
+" Specifying the -c option will allow writes of invalid (corrupt) data.\n\n"
 ));
 
 }
@@ -90,6 +91,10 @@ write_f(
 {
 	pfunc_t	pf;
 	extern char *progname;
+	int c;
+	int corrupt = 0;		/* Allow write of corrupt data; skip verification */
+	struct xfs_buf_ops nowrite_ops;
+	const struct xfs_buf_ops *stashed_ops = NULL; 
 
 	if (x.isreadonly & LIBXFS_ISREADONLY) {
 		dbprintf(_("%s started in read only mode, writing disabled\n"),
@@ -109,12 +114,34 @@ write_f(
 		return 0;
 	}
 
-	/* move past the "write" command */
-	argc--;
-	argv++;
+	if (argc) while ((c = getopt(argc, argv, "c")) != EOF) {
+		switch (c) {
+		case 'c':
+			corrupt = 1;
+			break;
+		default:
+			dbprintf(_("bad option for write command\n"));
+			return 0;
+		}
+	}
+
+	argc -= optind;
+	argv += optind;
+
+	if (corrupt) {
+		/* Temporarily remove write verifier to write bad data */
+		stashed_ops = iocur_top->bp->b_ops;
+		nowrite_ops.verify_read = stashed_ops->verify_read;
+		nowrite_ops.verify_write = xfs_dummy_verify;
+		iocur_top->bp->b_ops = &nowrite_ops;
+		dbprintf(_("Allowing write of corrupted data\n"));
+	}
 
 	(*pf)(DB_WRITE, cur_typ->fields, argc, argv);
 
+	if (stashed_ops)
+		iocur_top->bp->b_ops = stashed_ops;
+
 	return 0;
 }
 
diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
index 4d8d4ff..d527230 100644
--- a/man/man8/xfs_db.8
+++ b/man/man8/xfs_db.8
@@ -711,7 +711,7 @@ and
 bits respectively, and their string equivalent reported
 (but no modifications are made).
 .TP
-.BI "write [" "field value" "] ..."
+.BI "write [\-c] [" "field value" "] ..."
 Write a value to disk.
 Specific fields can be set in structures (struct mode),
 or a block can be set to data values (data mode),
@@ -729,6 +729,12 @@ contents of the block can be shifted or rotated left or right, or filled
 with a sequence, a constant value, or a random value. In this mode
 .B write
 with no arguments gives more information on the allowed commands.
+.RS 1.0i
+.TP 0.4i
+.B \-c
+Skip write verifiers and CRC recalculation; allows invalid data to be written
+to disk.
+.RE
 .SH TYPES
 This section gives the fields in each structure type and their meanings.
 Note that some types of block cover multiple actual structures,
-- 
1.7.1

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

Re: [PATCH 01/13] xfs_db: Allow writes of corrupted data by optionally skipping write verifiers

[Brian Foster]

On Tue, Mar 17, 2015 at 03:33:03PM -0500, Eric Sandeen wrote:
> Being able to write corrupt data is handy if we wish to test
> repair against specific types of corruptions.
> 
> Add a "-c" option to the write command which allows this.
> 
> Note that this also skips CRC updates; it's not currently possible
> to write invalid data with a valid CRC; CRC recalculation is
> intertwined with validation.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  db/io.c           |    7 +++++++
>  db/io.h           |    1 +
>  db/write.c        |   35 +++++++++++++++++++++++++++++++----
>  man/man8/xfs_db.8 |    8 +++++++-
>  4 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/db/io.c b/db/io.c
> index 7f1b76a..c5898f1 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -457,6 +457,13 @@ write_cur_bbs(void)
>  }
>  
>  void
> +xfs_dummy_verify(
> +	struct xfs_buf *bp)
> +{
> +	return;
> +}
> +
> +void
>  write_cur(void)
>  {
>  	if (iocur_sp < 0) {
> diff --git a/db/io.h b/db/io.h
> index 71082e6..31d96b4 100644
> --- a/db/io.h
> +++ b/db/io.h
> @@ -63,6 +63,7 @@ extern void	set_cur(const struct typ *t, __int64_t d, int c, int ring_add,
>  			bbmap_t *bbmap);
>  extern void     ring_add(void);
>  extern void	set_iocur_type(const struct typ *t);
> +extern void	xfs_dummy_verify(struct xfs_buf *bp);
>  
>  /*
>   * returns -1 for unchecked, 0 for bad and 1 for good
> diff --git a/db/write.c b/db/write.c
> index a0f14f4..4511859 100644
> --- a/db/write.c
> +++ b/db/write.c
> @@ -38,7 +38,7 @@ static int	write_f(int argc, char **argv);
>  static void     write_help(void);
>  
>  static const cmdinfo_t	write_cmd =
> -	{ "write", NULL, write_f, 0, -1, 0, N_("[field or value]..."),
> +	{ "write", NULL, write_f, 0, -1, 0, N_("[-c] [field or value]..."),
>  	  N_("write value to disk"), write_help };
>  
>  void
> @@ -79,6 +79,7 @@ write_help(void)
>  "  String mode: 'write \"This_is_a_filename\" - write null terminated string.\n"
>  "\n"
>  " In data mode type 'write' by itself for a list of specific commands.\n\n"
> +" Specifying the -c option will allow writes of invalid (corrupt) data.\n\n"
>  ));
>  
>  }
> @@ -90,6 +91,10 @@ write_f(
>  {
>  	pfunc_t	pf;
>  	extern char *progname;
> +	int c;
> +	int corrupt = 0;		/* Allow write of corrupt data; skip verification */
> +	struct xfs_buf_ops nowrite_ops;
> +	const struct xfs_buf_ops *stashed_ops = NULL; 

Still has whitespace damage on the line above and the if (argc) thing in
the hunk below. Can we kill those? This looks fine to me otherwise:

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

>  
>  	if (x.isreadonly & LIBXFS_ISREADONLY) {
>  		dbprintf(_("%s started in read only mode, writing disabled\n"),
> @@ -109,12 +114,34 @@ write_f(
>  		return 0;
>  	}
>  
> -	/* move past the "write" command */
> -	argc--;
> -	argv++;
> +	if (argc) while ((c = getopt(argc, argv, "c")) != EOF) {
> +		switch (c) {
> +		case 'c':
> +			corrupt = 1;
> +			break;
> +		default:
> +			dbprintf(_("bad option for write command\n"));
> +			return 0;
> +		}
> +	}
> +
> +	argc -= optind;
> +	argv += optind;
> +
> +	if (corrupt) {
> +		/* Temporarily remove write verifier to write bad data */
> +		stashed_ops = iocur_top->bp->b_ops;
> +		nowrite_ops.verify_read = stashed_ops->verify_read;
> +		nowrite_ops.verify_write = xfs_dummy_verify;
> +		iocur_top->bp->b_ops = &nowrite_ops;
> +		dbprintf(_("Allowing write of corrupted data\n"));
> +	}
>  
>  	(*pf)(DB_WRITE, cur_typ->fields, argc, argv);
>  
> +	if (stashed_ops)
> +		iocur_top->bp->b_ops = stashed_ops;
> +
>  	return 0;
>  }
>  
> diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
> index 4d8d4ff..d527230 100644
> --- a/man/man8/xfs_db.8
> +++ b/man/man8/xfs_db.8
> @@ -711,7 +711,7 @@ and
>  bits respectively, and their string equivalent reported
>  (but no modifications are made).
>  .TP
> -.BI "write [" "field value" "] ..."
> +.BI "write [\-c] [" "field value" "] ..."
>  Write a value to disk.
>  Specific fields can be set in structures (struct mode),
>  or a block can be set to data values (data mode),
> @@ -729,6 +729,12 @@ contents of the block can be shifted or rotated left or right, or filled
>  with a sequence, a constant value, or a random value. In this mode
>  .B write
>  with no arguments gives more information on the allowed commands.
> +.RS 1.0i
> +.TP 0.4i
> +.B \-c
> +Skip write verifiers and CRC recalculation; allows invalid data to be written
> +to disk.
> +.RE
>  .SH TYPES
>  This section gives the fields in each structure type and their meanings.
>  Note that some types of block cover multiple actual structures,
> -- 
> 1.7.1
> 
> _______________________________________________
> 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

[PATCH 01/13 V2] xfs_db: Allow writes of corrupted data by optionally skipping write verifiers

[Eric Sandeen]

Being able to write corrupt data is handy if we wish to test
repair against specific types of corruptions.

Add a "-c" option to the write command which allows this by removing
the write verifier.

Note that this also skips CRC updates; it's not currently possible
to write invalid data with a valid CRC; CRC recalculation is
intertwined with validation.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: Fix whitespace, remove "if (argc)", and test for
iocur_top->bp->b_ops before attempting to deref it.

diff --git a/db/io.c b/db/io.c
index 7f1b76a..c5898f1 100644
--- a/db/io.c
+++ b/db/io.c
@@ -457,6 +457,13 @@ write_cur_bbs(void)
 }
 
 void
+xfs_dummy_verify(
+	struct xfs_buf *bp)
+{
+	return;
+}
+
+void
 write_cur(void)
 {
 	if (iocur_sp < 0) {
diff --git a/db/io.h b/db/io.h
index 71082e6..31d96b4 100644
--- a/db/io.h
+++ b/db/io.h
@@ -63,6 +63,7 @@ extern void	set_cur(const struct typ *t, __int64_t d, int c, int ring_add,
 			bbmap_t *bbmap);
 extern void     ring_add(void);
 extern void	set_iocur_type(const struct typ *t);
+extern void	xfs_dummy_verify(struct xfs_buf *bp);
 
 /*
  * returns -1 for unchecked, 0 for bad and 1 for good
diff --git a/db/write.c b/db/write.c
index a0f14f4..655b618 100644
--- a/db/write.c
+++ b/db/write.c
@@ -38,7 +38,7 @@ static int	write_f(int argc, char **argv);
 static void     write_help(void);
 
 static const cmdinfo_t	write_cmd =
-	{ "write", NULL, write_f, 0, -1, 0, N_("[field or value]..."),
+	{ "write", NULL, write_f, 0, -1, 0, N_("[-c] [field or value]..."),
 	  N_("write value to disk"), write_help };
 
 void
@@ -79,6 +79,7 @@ write_help(void)
 "  String mode: 'write \"This_is_a_filename\" - write null terminated string.\n"
 "\n"
 " In data mode type 'write' by itself for a list of specific commands.\n\n"
+" Specifying the -c option will allow writes of invalid (corrupt) data.\n\n"
 ));
 
 }
@@ -90,6 +91,10 @@ write_f(
 {
 	pfunc_t	pf;
 	extern char *progname;
+	int c;
+	int corrupt = 0;		/* Allow write of corrupt data; skip verification */
+	struct xfs_buf_ops nowrite_ops;
+	const struct xfs_buf_ops *stashed_ops = NULL;
 
 	if (x.isreadonly & LIBXFS_ISREADONLY) {
 		dbprintf(_("%s started in read only mode, writing disabled\n"),
@@ -109,12 +114,34 @@ write_f(
 		return 0;
 	}
 
-	/* move past the "write" command */
-	argc--;
-	argv++;
+	while ((c = getopt(argc, argv, "c")) != EOF) {
+		switch (c) {
+		case 'c':
+			corrupt = 1;
+			break;
+		default:
+			dbprintf(_("bad option for write command\n"));
+			return 0;
+		}
+	}
+
+	argc -= optind;
+	argv += optind;
+
+	if (iocur_top->bp->b_ops && corrupt) {
+		/* Temporarily remove write verifier to write bad data */
+		stashed_ops = iocur_top->bp->b_ops;
+		nowrite_ops.verify_read = stashed_ops->verify_read;
+		nowrite_ops.verify_write = xfs_dummy_verify;
+		iocur_top->bp->b_ops = &nowrite_ops;
+		dbprintf(_("Allowing write of corrupted data\n"));
+	}
 
 	(*pf)(DB_WRITE, cur_typ->fields, argc, argv);
 
+	if (stashed_ops)
+		iocur_top->bp->b_ops = stashed_ops;
+
 	return 0;
 }
 
diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
index 4d8d4ff..d527230 100644
--- a/man/man8/xfs_db.8
+++ b/man/man8/xfs_db.8
@@ -711,7 +711,7 @@ and
 bits respectively, and their string equivalent reported
 (but no modifications are made).
 .TP
-.BI "write [" "field value" "] ..."
+.BI "write [\-c] [" "field value" "] ..."
 Write a value to disk.
 Specific fields can be set in structures (struct mode),
 or a block can be set to data values (data mode),
@@ -729,6 +729,12 @@ contents of the block can be shifted or rotated left or right, or filled
 with a sequence, a constant value, or a random value. In this mode
 .B write
 with no arguments gives more information on the allowed commands.
+.RS 1.0i
+.TP 0.4i
+.B \-c
+Skip write verifiers and CRC recalculation; allows invalid data to be written
+to disk.
+.RE
 .SH TYPES
 This section gives the fields in each structure type and their meanings.
 Note that some types of block cover multiple actual structures,

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

Re: [PATCH 01/13 V2] xfs_db: Allow writes of corrupted data by optionally skipping write verifiers

[Brian Foster]

On Mon, Mar 23, 2015 at 03:00:12PM -0500, Eric Sandeen wrote:
> Being able to write corrupt data is handy if we wish to test
> repair against specific types of corruptions.
> 
> Add a "-c" option to the write command which allows this by removing
> the write verifier.
> 
> Note that this also skips CRC updates; it's not currently possible
> to write invalid data with a valid CRC; CRC recalculation is
> intertwined with validation.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

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

> 
> V2: Fix whitespace, remove "if (argc)", and test for
> iocur_top->bp->b_ops before attempting to deref it.
> 
> diff --git a/db/io.c b/db/io.c
> index 7f1b76a..c5898f1 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -457,6 +457,13 @@ write_cur_bbs(void)
>  }
>  
>  void
> +xfs_dummy_verify(
> +	struct xfs_buf *bp)
> +{
> +	return;
> +}
> +
> +void
>  write_cur(void)
>  {
>  	if (iocur_sp < 0) {
> diff --git a/db/io.h b/db/io.h
> index 71082e6..31d96b4 100644
> --- a/db/io.h
> +++ b/db/io.h
> @@ -63,6 +63,7 @@ extern void	set_cur(const struct typ *t, __int64_t d, int c, int ring_add,
>  			bbmap_t *bbmap);
>  extern void     ring_add(void);
>  extern void	set_iocur_type(const struct typ *t);
> +extern void	xfs_dummy_verify(struct xfs_buf *bp);
>  
>  /*
>   * returns -1 for unchecked, 0 for bad and 1 for good
> diff --git a/db/write.c b/db/write.c
> index a0f14f4..655b618 100644
> --- a/db/write.c
> +++ b/db/write.c
> @@ -38,7 +38,7 @@ static int	write_f(int argc, char **argv);
>  static void     write_help(void);
>  
>  static const cmdinfo_t	write_cmd =
> -	{ "write", NULL, write_f, 0, -1, 0, N_("[field or value]..."),
> +	{ "write", NULL, write_f, 0, -1, 0, N_("[-c] [field or value]..."),
>  	  N_("write value to disk"), write_help };
>  
>  void
> @@ -79,6 +79,7 @@ write_help(void)
>  "  String mode: 'write \"This_is_a_filename\" - write null terminated string.\n"
>  "\n"
>  " In data mode type 'write' by itself for a list of specific commands.\n\n"
> +" Specifying the -c option will allow writes of invalid (corrupt) data.\n\n"
>  ));
>  
>  }
> @@ -90,6 +91,10 @@ write_f(
>  {
>  	pfunc_t	pf;
>  	extern char *progname;
> +	int c;
> +	int corrupt = 0;		/* Allow write of corrupt data; skip verification */
> +	struct xfs_buf_ops nowrite_ops;
> +	const struct xfs_buf_ops *stashed_ops = NULL;
>  
>  	if (x.isreadonly & LIBXFS_ISREADONLY) {
>  		dbprintf(_("%s started in read only mode, writing disabled\n"),
> @@ -109,12 +114,34 @@ write_f(
>  		return 0;
>  	}
>  
> -	/* move past the "write" command */
> -	argc--;
> -	argv++;
> +	while ((c = getopt(argc, argv, "c")) != EOF) {
> +		switch (c) {
> +		case 'c':
> +			corrupt = 1;
> +			break;
> +		default:
> +			dbprintf(_("bad option for write command\n"));
> +			return 0;
> +		}
> +	}
> +
> +	argc -= optind;
> +	argv += optind;
> +
> +	if (iocur_top->bp->b_ops && corrupt) {
> +		/* Temporarily remove write verifier to write bad data */
> +		stashed_ops = iocur_top->bp->b_ops;
> +		nowrite_ops.verify_read = stashed_ops->verify_read;
> +		nowrite_ops.verify_write = xfs_dummy_verify;
> +		iocur_top->bp->b_ops = &nowrite_ops;
> +		dbprintf(_("Allowing write of corrupted data\n"));
> +	}
>  
>  	(*pf)(DB_WRITE, cur_typ->fields, argc, argv);
>  
> +	if (stashed_ops)
> +		iocur_top->bp->b_ops = stashed_ops;
> +
>  	return 0;
>  }
>  
> diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
> index 4d8d4ff..d527230 100644
> --- a/man/man8/xfs_db.8
> +++ b/man/man8/xfs_db.8
> @@ -711,7 +711,7 @@ and
>  bits respectively, and their string equivalent reported
>  (but no modifications are made).
>  .TP
> -.BI "write [" "field value" "] ..."
> +.BI "write [\-c] [" "field value" "] ..."
>  Write a value to disk.
>  Specific fields can be set in structures (struct mode),
>  or a block can be set to data values (data mode),
> @@ -729,6 +729,12 @@ contents of the block can be shifted or rotated left or right, or filled
>  with a sequence, a constant value, or a random value. In this mode
>  .B write
>  with no arguments gives more information on the allowed commands.
> +.RS 1.0i
> +.TP 0.4i
> +.B \-c
> +Skip write verifiers and CRC recalculation; allows invalid data to be written
> +to disk.
> +.RE
>  .SH TYPES
>  This section gives the fields in each structure type and their meanings.
>  Note that some types of block cover multiple actual structures,
> 
> _______________________________________________
> 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

[PATCH 02/13] xfs_db: fix inode CRC validity state, and warn on read if invalid

[Eric Sandeen]

Currently, the "ino_crc_ok" field on the io cursor reflects
overall inode validity, not CRC correctness.  Because it is
only used when printing CRC validity, change it to reflect
only that state - and update it whenever we re-write the
inode (thus updating the CRC).

In addition, when reading an inode, warn if the CRC is bad.

Note, when specifying an inode which doesn't actually exist,
this will claim corruption; I'm not sure if that's good or
bad. Today, it already issues corruption errors on the way;
this adds a new message as well:

xfs_db> inode 129
Metadata corruption detected at block 0x80/0x2000
Metadata corruption detected at block 0x80/0x2000
...
Metadata CRC error detected for ino 129

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 db/inode.c       |    7 ++++++-
 db/io.c          |    4 +++-
 include/libxfs.h |    2 ++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/db/inode.c b/db/inode.c
index 24170ba..982acb7 100644
--- a/db/inode.c
+++ b/db/inode.c
@@ -684,13 +684,18 @@ set_cur_inode(
 		numblks, DB_RING_IGN, NULL);
 	off_cur(offset << mp->m_sb.sb_inodelog, mp->m_sb.sb_inodesize);
 	dip = iocur_top->data;
-	iocur_top->ino_crc_ok = libxfs_dinode_verify(mp, ino, dip);
+	iocur_top->ino_crc_ok = libxfs_verify_cksum((char *)dip,
+						    mp->m_sb.sb_inodesize,
+						    XFS_DINODE_CRC_OFF);
 	iocur_top->ino_buf = 1;
 	iocur_top->ino = ino;
 	iocur_top->mode = be16_to_cpu(dip->di_mode);
 	if ((iocur_top->mode & S_IFMT) == S_IFDIR)
 		iocur_top->dirino = ino;
 
+	if (xfs_sb_version_hascrc(&mp->m_sb) && !iocur_top->ino_crc_ok)
+		dbprintf(_("Metadata CRC error detected for ino %lld\n"), ino);
+
 	/* track updated info in ring */
 	ring_add();
 }
diff --git a/db/io.c b/db/io.c
index c5898f1..91858e2 100644
--- a/db/io.c
+++ b/db/io.c
@@ -471,8 +471,10 @@ write_cur(void)
 		return;
 	}
 
-	if (iocur_top->ino_buf)
+	if (iocur_top->ino_buf) {
 		libxfs_dinode_calc_crc(mp, iocur_top->data);
+		iocur_top->ino_crc_ok = 1;
+	}
 	if (iocur_top->dquot_buf)
 		xfs_update_cksum(iocur_top->data, sizeof(struct xfs_dqblk),
 				 XFS_DQUOT_CRC_OFF);
diff --git a/include/libxfs.h b/include/libxfs.h
index 45a924f..962e319 100644
--- a/include/libxfs.h
+++ b/include/libxfs.h
@@ -782,6 +782,8 @@ extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
 
 #include <xfs/xfs_cksum.h>
 
+#define libxfs_verify_cksum	xfs_verify_cksum
+
 static inline int
 xfs_buf_verify_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
 {
-- 
1.7.1

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

Re: [PATCH 02/13] xfs_db: fix inode CRC validity state, and warn on read if invalid

[Brian Foster]

On Tue, Mar 17, 2015 at 03:33:04PM -0500, Eric Sandeen wrote:
> Currently, the "ino_crc_ok" field on the io cursor reflects
> overall inode validity, not CRC correctness.  Because it is
> only used when printing CRC validity, change it to reflect
> only that state - and update it whenever we re-write the
> inode (thus updating the CRC).
> 
> In addition, when reading an inode, warn if the CRC is bad.
> 
> Note, when specifying an inode which doesn't actually exist,
> this will claim corruption; I'm not sure if that's good or
> bad. Today, it already issues corruption errors on the way;
> this adds a new message as well:
> 
> xfs_db> inode 129
> Metadata corruption detected at block 0x80/0x2000
> Metadata corruption detected at block 0x80/0x2000
> ...
> Metadata CRC error detected for ino 129
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

So is the objective here to simply give the field an explicit meaning?
E.g., indicate whether the crc is valid, irrespective of whether
something else might be wrong with the inode?

>  db/inode.c       |    7 ++++++-
>  db/io.c          |    4 +++-
>  include/libxfs.h |    2 ++
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/db/inode.c b/db/inode.c
> index 24170ba..982acb7 100644
> --- a/db/inode.c
> +++ b/db/inode.c
> @@ -684,13 +684,18 @@ set_cur_inode(
>  		numblks, DB_RING_IGN, NULL);
>  	off_cur(offset << mp->m_sb.sb_inodelog, mp->m_sb.sb_inodesize);
>  	dip = iocur_top->data;
> -	iocur_top->ino_crc_ok = libxfs_dinode_verify(mp, ino, dip);
> +	iocur_top->ino_crc_ok = libxfs_verify_cksum((char *)dip,
> +						    mp->m_sb.sb_inodesize,
> +						    XFS_DINODE_CRC_OFF);

With this replaced, it doesn't look like anybody else will call
libxfs_dinode_verify (analogous to xfs_iread() in kernel). Is that
intentional? I guess the magic and version should be checked in the read
verifier, but there are a couple other checks in that helper as well.

>  	iocur_top->ino_buf = 1;
>  	iocur_top->ino = ino;
>  	iocur_top->mode = be16_to_cpu(dip->di_mode);
>  	if ((iocur_top->mode & S_IFMT) == S_IFDIR)
>  		iocur_top->dirino = ino;
>  
> +	if (xfs_sb_version_hascrc(&mp->m_sb) && !iocur_top->ino_crc_ok)
> +		dbprintf(_("Metadata CRC error detected for ino %lld\n"), ino);
> +

Hmm.. if we keep this, could we combine with the hunk above? I ask
simply because I'd rather see the _hascrc() check around the verify_cksum()
check as well, rather than verifying a cksum and ignoring it.

It's also a little confusing how this field is handled without crc
enabled. write_cur() looks like it sets it and calls
libxfs_dinode_calc_crc() blindly, which asserts that crc is enabled. I
guess we just don't print anything if crc=0, but it would be nice if the
flag was consistent.

Brian

>  	/* track updated info in ring */
>  	ring_add();
>  }
> diff --git a/db/io.c b/db/io.c
> index c5898f1..91858e2 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -471,8 +471,10 @@ write_cur(void)
>  		return;
>  	}
>  
> -	if (iocur_top->ino_buf)
> +	if (iocur_top->ino_buf) {
>  		libxfs_dinode_calc_crc(mp, iocur_top->data);
> +		iocur_top->ino_crc_ok = 1;
> +	}
>  	if (iocur_top->dquot_buf)
>  		xfs_update_cksum(iocur_top->data, sizeof(struct xfs_dqblk),
>  				 XFS_DQUOT_CRC_OFF);
> diff --git a/include/libxfs.h b/include/libxfs.h
> index 45a924f..962e319 100644
> --- a/include/libxfs.h
> +++ b/include/libxfs.h
> @@ -782,6 +782,8 @@ extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
>  
>  #include <xfs/xfs_cksum.h>
>  
> +#define libxfs_verify_cksum	xfs_verify_cksum
> +
>  static inline int
>  xfs_buf_verify_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
>  {
> -- 
> 1.7.1
> 
> _______________________________________________
> 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 02/13] xfs_db: fix inode CRC validity state, and warn on read if invalid

[Eric Sandeen]

On 3/19/15 10:07 AM, Brian Foster wrote:
> On Tue, Mar 17, 2015 at 03:33:04PM -0500, Eric Sandeen wrote:
>> Currently, the "ino_crc_ok" field on the io cursor reflects
>> overall inode validity, not CRC correctness.  Because it is
>> only used when printing CRC validity, change it to reflect
>> only that state - and update it whenever we re-write the
>> inode (thus updating the CRC).
>>
>> In addition, when reading an inode, warn if the CRC is bad.
>>
>> Note, when specifying an inode which doesn't actually exist,
>> this will claim corruption; I'm not sure if that's good or
>> bad. Today, it already issues corruption errors on the way;
>> this adds a new message as well:
>>
>> xfs_db> inode 129
>> Metadata corruption detected at block 0x80/0x2000
>> Metadata corruption detected at block 0x80/0x2000
>> ...
>> Metadata CRC error detected for ino 129
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
> 
> So is the objective here to simply give the field an explicit meaning?
> E.g., indicate whether the crc is valid, irrespective of whether
> something else might be wrong with the inode?

Yeah, ino_crc_ok kinda indicates that the inode... crc ... is ok? :)

Prior to this, it was only used in iocur_crc_valid(), which is called
from fp_crc() to print unchecked/bad/correct/unknown for the crc.

That's only used to print crc fields in the table-driven db stuff:

        { FLDT_CRC, "crc", fp_crc, "%#x (%s)", SI(bitsz(__uint32_t)),
          0, NULL, NULL },

so it seems to have a very specific meaning, and wrapping it up
w/ the verifier didn't make sense.

I had something more specific when I first sent the patch but that
was yonks ago.  :)

>>  db/inode.c       |    7 ++++++-
>>  db/io.c          |    4 +++-
>>  include/libxfs.h |    2 ++
>>  3 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/db/inode.c b/db/inode.c
>> index 24170ba..982acb7 100644
>> --- a/db/inode.c
>> +++ b/db/inode.c
>> @@ -684,13 +684,18 @@ set_cur_inode(
>>  		numblks, DB_RING_IGN, NULL);
>>  	off_cur(offset << mp->m_sb.sb_inodelog, mp->m_sb.sb_inodesize);
>>  	dip = iocur_top->data;
>> -	iocur_top->ino_crc_ok = libxfs_dinode_verify(mp, ino, dip);
>> +	iocur_top->ino_crc_ok = libxfs_verify_cksum((char *)dip,
>> +						    mp->m_sb.sb_inodesize,
>> +						    XFS_DINODE_CRC_OFF);
> 
> With this replaced, it doesn't look like anybody else will call
> libxfs_dinode_verify (analogous to xfs_iread() in kernel). Is that
> intentional? I guess the magic and version should be checked in the read
> verifier, but there are a couple other checks in that helper as well.

xfs_iread still calls xfs_dinode_verify, right?

>>  	iocur_top->ino_buf = 1;
>>  	iocur_top->ino = ino;
>>  	iocur_top->mode = be16_to_cpu(dip->di_mode);
>>  	if ((iocur_top->mode & S_IFMT) == S_IFDIR)
>>  		iocur_top->dirino = ino;
>>  
>> +	if (xfs_sb_version_hascrc(&mp->m_sb) && !iocur_top->ino_crc_ok)
>> +		dbprintf(_("Metadata CRC error detected for ino %lld\n"), ino);
>> +
> 
> Hmm.. if we keep this, could we combine with the hunk above? I ask
> simply because I'd rather see the _hascrc() check around the verify_cksum()
> check as well, rather than verifying a cksum and ignoring it.

ok, sure.

> It's also a little confusing how this field is handled without crc
> enabled. write_cur() looks like it sets it and calls
> libxfs_dinode_calc_crc() blindly, which asserts that crc is enabled. I
> guess we just don't print anything if crc=0, but it would be nice if the
> flag was consistent.

Hm, I can see that, but I don't know how we can test for the presence of
crcs in write_cur.  We don't have that info, AFAICT.

-Eric

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

Re: [PATCH 02/13] xfs_db: fix inode CRC validity state, and warn on read if invalid

[Brian Foster]

On Fri, Mar 20, 2015 at 09:26:12PM -0500, Eric Sandeen wrote:
> On 3/19/15 10:07 AM, Brian Foster wrote:
> > On Tue, Mar 17, 2015 at 03:33:04PM -0500, Eric Sandeen wrote:
> >> Currently, the "ino_crc_ok" field on the io cursor reflects
> >> overall inode validity, not CRC correctness.  Because it is
> >> only used when printing CRC validity, change it to reflect
> >> only that state - and update it whenever we re-write the
> >> inode (thus updating the CRC).
> >>
> >> In addition, when reading an inode, warn if the CRC is bad.
> >>
> >> Note, when specifying an inode which doesn't actually exist,
> >> this will claim corruption; I'm not sure if that's good or
> >> bad. Today, it already issues corruption errors on the way;
> >> this adds a new message as well:
> >>
> >> xfs_db> inode 129
> >> Metadata corruption detected at block 0x80/0x2000
> >> Metadata corruption detected at block 0x80/0x2000
> >> ...
> >> Metadata CRC error detected for ino 129
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> > 
> > So is the objective here to simply give the field an explicit meaning?
> > E.g., indicate whether the crc is valid, irrespective of whether
> > something else might be wrong with the inode?
> 
> Yeah, ino_crc_ok kinda indicates that the inode... crc ... is ok? :)
> 
> Prior to this, it was only used in iocur_crc_valid(), which is called
> from fp_crc() to print unchecked/bad/correct/unknown for the crc.
> 
> That's only used to print crc fields in the table-driven db stuff:
> 
>         { FLDT_CRC, "crc", fp_crc, "%#x (%s)", SI(bitsz(__uint32_t)),
>           0, NULL, NULL },
> 
> so it seems to have a very specific meaning, and wrapping it up
> w/ the verifier didn't make sense.
> 
> I had something more specific when I first sent the patch but that
> was yonks ago.  :)
> 
> >>  db/inode.c       |    7 ++++++-
> >>  db/io.c          |    4 +++-
> >>  include/libxfs.h |    2 ++
> >>  3 files changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/db/inode.c b/db/inode.c
> >> index 24170ba..982acb7 100644
> >> --- a/db/inode.c
> >> +++ b/db/inode.c
> >> @@ -684,13 +684,18 @@ set_cur_inode(
> >>  		numblks, DB_RING_IGN, NULL);
> >>  	off_cur(offset << mp->m_sb.sb_inodelog, mp->m_sb.sb_inodesize);
> >>  	dip = iocur_top->data;
> >> -	iocur_top->ino_crc_ok = libxfs_dinode_verify(mp, ino, dip);
> >> +	iocur_top->ino_crc_ok = libxfs_verify_cksum((char *)dip,
> >> +						    mp->m_sb.sb_inodesize,
> >> +						    XFS_DINODE_CRC_OFF);
> > 
> > With this replaced, it doesn't look like anybody else will call
> > libxfs_dinode_verify (analogous to xfs_iread() in kernel). Is that
> > intentional? I guess the magic and version should be checked in the read
> > verifier, but there are a couple other checks in that helper as well.
> 
> xfs_iread still calls xfs_dinode_verify, right?
> 

Yeah, but I didn't see that being called in this path. My question was
basically if we would notice whether the uuid or i_ino value got
scribbled. The verifier looks like it only checks the magic and version
number. Thinking more about it, it probably doesn't matter. If some
field is wrong, chances are the crc is busted. I think the uuid is
actually a v5 only field as well. Finally, the actual ino check in
xfs_verify_dinode() only occurs for v5, so there's no change there
either. Nothing to see here, move along... ;)

> >>  	iocur_top->ino_buf = 1;
> >>  	iocur_top->ino = ino;
> >>  	iocur_top->mode = be16_to_cpu(dip->di_mode);
> >>  	if ((iocur_top->mode & S_IFMT) == S_IFDIR)
> >>  		iocur_top->dirino = ino;
> >>  
> >> +	if (xfs_sb_version_hascrc(&mp->m_sb) && !iocur_top->ino_crc_ok)
> >> +		dbprintf(_("Metadata CRC error detected for ino %lld\n"), ino);
> >> +
> > 
> > Hmm.. if we keep this, could we combine with the hunk above? I ask
> > simply because I'd rather see the _hascrc() check around the verify_cksum()
> > check as well, rather than verifying a cksum and ignoring it.
> 
> ok, sure.
> 
> > It's also a little confusing how this field is handled without crc
> > enabled. write_cur() looks like it sets it and calls
> > libxfs_dinode_calc_crc() blindly, which asserts that crc is enabled. I
> > guess we just don't print anything if crc=0, but it would be nice if the
> > flag was consistent.
> 
> Hm, I can see that, but I don't know how we can test for the presence of
> crcs in write_cur.  We don't have that info, AFAICT.
> 

mp is a global. That's how it's resolved in set_cur_inode() as well.
That could also be a separate patch, btw. ;) This seems Ok to me with
the minor cleanup above, and we should probably get these fixes in at
some point. :)

Brian

> -Eric
> 
> _______________________________________________
> 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

[PATCH 02/13 V2] xfs_db: fix inode CRC validity state, and warn on read if invalid

[Eric Sandeen]

Currently, the "ino_crc_ok" field on the io cursor reflects
overall inode validity, not CRC correctness.  Because it is
only used when printing CRC validity, change it to reflect
only that state - and update it whenever we re-write the
inode (thus updating the CRC).

In addition, when reading an inode, warn if the CRC is bad.

Note, when specifying an inode which doesn't actually exist,
this will claim corruption; I'm not sure if that's good or
bad. Today, it already issues corruption errors on the way;
this adds a new message as well:

xfs_db> inode 129
Metadata corruption detected at block 0x80/0x2000
Metadata corruption detected at block 0x80/0x2000
...
Metadata CRC error detected for ino 129

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: move ino_crc_ok calculations under "if (xfs_sb_version_hascrc())"

diff --git a/db/inode.c b/db/inode.c
index 24170ba..4f9c65d 100644
--- a/db/inode.c
+++ b/db/inode.c
@@ -684,13 +684,22 @@ set_cur_inode(
 		numblks, DB_RING_IGN, NULL);
 	off_cur(offset << mp->m_sb.sb_inodelog, mp->m_sb.sb_inodesize);
 	dip = iocur_top->data;
-	iocur_top->ino_crc_ok = libxfs_dinode_verify(mp, ino, dip);
 	iocur_top->ino_buf = 1;
 	iocur_top->ino = ino;
 	iocur_top->mode = be16_to_cpu(dip->di_mode);
 	if ((iocur_top->mode & S_IFMT) == S_IFDIR)
 		iocur_top->dirino = ino;
 
+	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+		iocur_top->ino_crc_ok = libxfs_verify_cksum((char *)dip,
+						    mp->m_sb.sb_inodesize,
+						    XFS_DINODE_CRC_OFF);
+		if (!iocur_top->ino_crc_ok)
+			dbprintf(
+_("Metadata CRC error detected for ino %lld\n"),
+				ino);
+	}
+
 	/* track updated info in ring */
 	ring_add();
 }
diff --git a/db/io.c b/db/io.c
index c5898f1..d906123 100644
--- a/db/io.c
+++ b/db/io.c
@@ -471,8 +471,10 @@ write_cur(void)
 		return;
 	}
 
-	if (iocur_top->ino_buf)
+	if (xfs_sb_version_hascrc(&mp->m_sb) && iocur_top->ino_buf) {
 		libxfs_dinode_calc_crc(mp, iocur_top->data);
+		iocur_top->ino_crc_ok = 1;
+	}
 	if (iocur_top->dquot_buf)
 		xfs_update_cksum(iocur_top->data, sizeof(struct xfs_dqblk),
 				 XFS_DQUOT_CRC_OFF);
diff --git a/include/libxfs.h b/include/libxfs.h
index 45a924f..962e319 100644
--- a/include/libxfs.h
+++ b/include/libxfs.h
@@ -782,6 +782,8 @@ extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
 
 #include <xfs/xfs_cksum.h>
 
+#define libxfs_verify_cksum	xfs_verify_cksum
+
 static inline int
 xfs_buf_verify_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
 {


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

Re: [PATCH 02/13 V2] xfs_db: fix inode CRC validity state, and warn on read if invalid

[Brian Foster]

On Mon, Mar 23, 2015 at 03:11:25PM -0500, Eric Sandeen wrote:
> Currently, the "ino_crc_ok" field on the io cursor reflects
> overall inode validity, not CRC correctness.  Because it is
> only used when printing CRC validity, change it to reflect
> only that state - and update it whenever we re-write the
> inode (thus updating the CRC).
> 
> In addition, when reading an inode, warn if the CRC is bad.
> 
> Note, when specifying an inode which doesn't actually exist,
> this will claim corruption; I'm not sure if that's good or
> bad. Today, it already issues corruption errors on the way;
> this adds a new message as well:
> 
> xfs_db> inode 129
> Metadata corruption detected at block 0x80/0x2000
> Metadata corruption detected at block 0x80/0x2000
> ...
> Metadata CRC error detected for ino 129
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

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

> 
> V2: move ino_crc_ok calculations under "if (xfs_sb_version_hascrc())"
> 
> diff --git a/db/inode.c b/db/inode.c
> index 24170ba..4f9c65d 100644
> --- a/db/inode.c
> +++ b/db/inode.c
> @@ -684,13 +684,22 @@ set_cur_inode(
>  		numblks, DB_RING_IGN, NULL);
>  	off_cur(offset << mp->m_sb.sb_inodelog, mp->m_sb.sb_inodesize);
>  	dip = iocur_top->data;
> -	iocur_top->ino_crc_ok = libxfs_dinode_verify(mp, ino, dip);
>  	iocur_top->ino_buf = 1;
>  	iocur_top->ino = ino;
>  	iocur_top->mode = be16_to_cpu(dip->di_mode);
>  	if ((iocur_top->mode & S_IFMT) == S_IFDIR)
>  		iocur_top->dirino = ino;
>  
> +	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +		iocur_top->ino_crc_ok = libxfs_verify_cksum((char *)dip,
> +						    mp->m_sb.sb_inodesize,
> +						    XFS_DINODE_CRC_OFF);
> +		if (!iocur_top->ino_crc_ok)
> +			dbprintf(
> +_("Metadata CRC error detected for ino %lld\n"),
> +				ino);
> +	}
> +
>  	/* track updated info in ring */
>  	ring_add();
>  }
> diff --git a/db/io.c b/db/io.c
> index c5898f1..d906123 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -471,8 +471,10 @@ write_cur(void)
>  		return;
>  	}
>  
> -	if (iocur_top->ino_buf)
> +	if (xfs_sb_version_hascrc(&mp->m_sb) && iocur_top->ino_buf) {
>  		libxfs_dinode_calc_crc(mp, iocur_top->data);
> +		iocur_top->ino_crc_ok = 1;
> +	}
>  	if (iocur_top->dquot_buf)
>  		xfs_update_cksum(iocur_top->data, sizeof(struct xfs_dqblk),
>  				 XFS_DQUOT_CRC_OFF);
> diff --git a/include/libxfs.h b/include/libxfs.h
> index 45a924f..962e319 100644
> --- a/include/libxfs.h
> +++ b/include/libxfs.h
> @@ -782,6 +782,8 @@ extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
>  
>  #include <xfs/xfs_cksum.h>
>  
> +#define libxfs_verify_cksum	xfs_verify_cksum
> +
>  static inline int
>  xfs_buf_verify_cksum(struct xfs_buf *bp, unsigned long cksum_offset)
>  {
> 
> 
> _______________________________________________
> 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

[PATCH 03/13] xfs_db: add crc manipulation commands

[Eric Sandeen]

This adds a new "crc" command to xfs_db for CRC-enabled filesystems.

If a structure has a CRC field, we can validate it, invalidate/corrupt
it, or revalidate/rewrite it:

xfs_db> sb 0
xfs_db> crc -v
crc = 0x796c814f (correct)
xfs_db> crc -i
Metadata CRC error detected at block 0x0/0x200
crc = 0x796c8150 (bad)
xfs_db> crc -r
crc = 0x796c814f (correct)

(-i and -r require "expert" write-capable mode)

This requires temporarily replacing the write verifier with
a dummy which won't recalculate the CRC on the way to disk.

It also required me to write a new flist function, which is
totally foreign to me, so hopefully done right - but it seems
to work here.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 db/Makefile       |    2 +-
 db/command.c      |    2 +
 db/crc.c          |  188 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 db/crc.h          |   22 ++++++
 db/flist.c        |   34 ++++++++++
 db/flist.h        |    1 +
 db/io.c           |   24 ++++++-
 db/write.h        |    2 +-
 man/man8/xfs_db.8 |   27 ++++++--
 9 files changed, 293 insertions(+), 9 deletions(-)
 create mode 100644 db/crc.c
 create mode 100644 db/crc.h

diff --git a/db/Makefile b/db/Makefile
index fb01bdd..500f94b 100644
--- a/db/Makefile
+++ b/db/Makefile
@@ -8,7 +8,7 @@ include $(TOPDIR)/include/builddefs
 LTCOMMAND = xfs_db
 
 HFILES = addr.h agf.h agfl.h agi.h attr.h attrshort.h bit.h block.h bmap.h \
-	btblock.h bmroot.h check.h command.h convert.h debug.h \
+	btblock.h bmroot.h check.h command.h convert.h crc.h debug.h \
 	dir2.h dir2sf.h dquot.h echo.h faddr.h field.h \
 	flist.h fprint.h frag.h freesp.h hash.h help.h init.h inode.h input.h \
 	io.h malloc.h metadump.h output.h print.h quit.h sb.h sig.h strvec.h \
diff --git a/db/command.c b/db/command.c
index b7e3165..d44e0a5 100644
--- a/db/command.c
+++ b/db/command.c
@@ -48,6 +48,7 @@
 #include "write.h"
 #include "malloc.h"
 #include "dquot.h"
+#include "crc.h"
 
 cmdinfo_t	*cmdtab;
 int		ncmds;
@@ -123,6 +124,7 @@ init_commands(void)
 	bmap_init();
 	check_init();
 	convert_init();
+	crc_init();
 	debug_init();
 	echo_init();
 	frag_init();
diff --git a/db/crc.c b/db/crc.c
new file mode 100644
index 0000000..b58a594
--- /dev/null
+++ b/db/crc.c
@@ -0,0 +1,188 @@
+/*
+ * Copyright (c) 2014 Red Hat, Inc.
+ * All Rights Reserved.
+ *
+ * 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.
+ *
+ * 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/libxfs.h>
+#include "addr.h"
+#include "command.h"
+#include "type.h"
+#include "faddr.h"
+#include "fprint.h"
+#include "field.h"
+#include "flist.h"
+#include "io.h"
+#include "init.h"
+#include "output.h"
+#include "bit.h"
+#include "print.h"
+
+static int crc_f(int argc, char **argv);
+static void crc_help(void);
+
+static const cmdinfo_t crc_cmd =
+	{ "crc", NULL, crc_f, 0, 1, 0, "[-i|-r|-v]",
+	  N_("manipulate crc values for V5 filesystem structures"), crc_help };
+
+void
+crc_init(void)
+{
+	if (xfs_sb_version_hascrc(&mp->m_sb))
+		add_command(&crc_cmd);
+}
+
+static void
+crc_help(void)
+{
+	dbprintf(_(
+"\n"
+" 'crc' validates, invalidates, or recalculates the crc value for\n"
+" the current on-disk metadata structures in Version 5 filesystems.\n"
+"\n"
+" Usage:  \"crc [-i|-r|-v]\"\n"
+"\n"
+));
+
+}
+
+static int
+crc_f(
+	int		argc,
+	char		**argv)
+{
+	struct xfs_buf_ops nowrite_ops;
+	const struct xfs_buf_ops *stashed_ops = NULL;
+	extern char	*progname;
+	const field_t	*fields;
+	const ftattr_t	*fa;
+	flist_t		*fl;
+	int		invalidate = 0;
+	int		recalculate = 0;
+	int		validate = 0;
+	int		c;
+
+	if (cur_typ == NULL) {
+		dbprintf(_("no current type\n"));
+		return 0;
+	}
+
+	if (cur_typ->fields == NULL) {
+		dbprintf(_("current type (%s) is not a structure\n"),
+			 cur_typ->name);
+		return 0;
+	}
+
+	if (argc) while ((c = getopt(argc, argv, "irv")) != EOF) {
+		switch (c) {
+		case 'i':
+			invalidate = 1;
+			break;
+		case 'r':
+			recalculate = 1;
+			break;
+		case 'v':
+			validate = 1;
+			break;
+		default:
+			dbprintf(_("bad option for crc command\n"));
+			return 0;
+		}
+	} else
+		validate = 1;
+
+	if (invalidate + recalculate + validate > 1) {
+		dbprintf(_("crc command accepts only one option\n"));
+		return 0;
+	}
+
+	if ((invalidate || recalculate) &&
+	    ((x.isreadonly & LIBXFS_ISREADONLY) || !expert_mode)) {
+		dbprintf(_("%s not in expert mode, writing disabled\n"),
+			progname);
+		return 0;
+	}
+
+	fields = cur_typ->fields;
+
+	/* if we're a root field type, go down 1 layer to get field list */
+	if (fields->name[0] == '\0') {
+		fa = &ftattrtab[fields->ftyp];
+		ASSERT(fa->ftyp == fields->ftyp);
+		fields = fa->subfld;
+	}
+
+	/* Search for a CRC field */
+	fl = flist_find_ftyp(fields, FLDT_CRC);
+	if (!fl) {
+		dbprintf(_("No CRC field found for type %s\n"), cur_typ->name);
+		return 0;
+	}
+
+	/* run down the field list and set offsets into the data */
+	if (!flist_parse(fields, fl, iocur_top->data, 0)) {
+		flist_free(fl);
+		dbprintf(_("parsing error\n"));
+		return 0;
+	}
+
+	if (invalidate) {
+		flist_t		*sfl;
+		int		bit_length;
+		int		parentoffset;
+		int		crc;
+
+		sfl = fl;
+		parentoffset = 0;
+		while (sfl->child) {
+			parentoffset = sfl->offset;
+			sfl = sfl->child;
+		}
+		ASSERT(sfl->fld->ftyp == FLDT_CRC);
+
+		bit_length = fsize(sfl->fld, iocur_top->data, parentoffset, 0);
+		bit_length *= fcount(sfl->fld, iocur_top->data, parentoffset);
+		crc = getbitval(iocur_top->data, sfl->offset, bit_length, BVUNSIGNED);
+		/* Off by one.. */
+		crc = cpu_to_be32(crc + 1);
+		setbitval(iocur_top->data, sfl->offset, bit_length, &crc);
+
+		/* Temporarily remove write verifier to write a bad CRC */
+		stashed_ops = iocur_top->bp->b_ops;
+		nowrite_ops.verify_read = stashed_ops->verify_read;
+		nowrite_ops.verify_write = xfs_dummy_verify;
+		iocur_top->bp->b_ops = &nowrite_ops;
+	}
+
+	if (invalidate || recalculate) {
+		if (invalidate)
+			dbprintf(_("Invalidating CRC:\n"));
+		else
+			dbprintf(_("Recalculating CRC:\n"));
+
+		write_cur();
+		if (stashed_ops)
+			iocur_top->bp->b_ops = stashed_ops;
+		/* re-verify to get proper b_error state */
+		iocur_top->bp->b_ops->verify_read(iocur_top->bp);
+	} else
+		dbprintf(_("Verifying CRC:\n"));
+
+	/* And show us what we've got! */
+	flist_print(fl);
+	print_flist(fl);
+	flist_free(fl);
+	return 0;
+}
diff --git a/db/crc.h b/db/crc.h
new file mode 100644
index 0000000..9f44615
--- /dev/null
+++ b/db/crc.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) 2014 Red Hat, Inc.
+ * All Rights Reserved.
+ *
+ * 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.
+ *
+ * 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
+ */
+
+struct field;
+
+extern void	crc_init(void);
+extern void	crc_struct(const field_t *fields, int argc, char **argv);
diff --git a/db/flist.c b/db/flist.c
index 33f7da7..fa19f70 100644
--- a/db/flist.c
+++ b/db/flist.c
@@ -411,6 +411,40 @@ flist_split(
 	return v;
 }
 
+/*
+ * Given a set of fields, scan for a field of the given type.
+ * Return an flist leading to the first found field
+ * of that type.
+ * Return NULL if no field of the given type is found.
+ */
+flist_t *
+flist_find_ftyp(
+	const field_t *fields,
+	fldt_t	type)
+{
+	flist_t	*fl;
+	const field_t	*f;
+	const ftattr_t  *fa;
+
+	for (f = fields; f->name; f++) {
+		fl = flist_make(f->name);
+		fl->fld = f;
+		if (f->ftyp == type)
+			return fl;
+		fa = &ftattrtab[f->ftyp];
+		if (fa->subfld) {
+			flist_t *nfl;
+			nfl = flist_find_ftyp(fa->subfld, type);
+			if (nfl) {
+				fl->child = nfl;
+				return fl;
+			}
+		}
+		flist_free(fl);
+	}
+	return NULL;
+}
+
 static void
 ftok_free(
 	ftok_t	*ft)
diff --git a/db/flist.h b/db/flist.h
index 5c9fba0..3f4b312 100644
--- a/db/flist.h
+++ b/db/flist.h
@@ -37,3 +37,4 @@ extern int	flist_parse(const struct field *fields, flist_t *fl, void *obj,
 			    int startoff);
 extern void	flist_print(flist_t *fl);
 extern flist_t	*flist_scan(char *name);
+extern flist_t	*flist_find_ftyp(const field_t *fields, fldt_t  type);
diff --git a/db/io.c b/db/io.c
index 91858e2..732b320 100644
--- a/db/io.c
+++ b/db/io.c
@@ -27,6 +27,7 @@
 #include "output.h"
 #include "init.h"
 #include "malloc.h"
+#include "crc.h"
 
 static int	pop_f(int argc, char **argv);
 static void     pop_help(void);
@@ -466,22 +467,41 @@ xfs_dummy_verify(
 void
 write_cur(void)
 {
+	int skip_crc = 0;
+
+	if (iocur_top->bp->b_ops &&
+	    iocur_top->bp->b_ops->verify_write == xfs_dummy_verify)
+		skip_crc = 1;
+
 	if (iocur_sp < 0) {
 		dbprintf(_("nothing to write\n"));
 		return;
 	}
 
-	if (iocur_top->ino_buf) {
+	if (iocur_top->ino_buf && !skip_crc) {
 		libxfs_dinode_calc_crc(mp, iocur_top->data);
 		iocur_top->ino_crc_ok = 1;
 	}
-	if (iocur_top->dquot_buf)
+
+	if (iocur_top->dquot_buf && !skip_crc)
 		xfs_update_cksum(iocur_top->data, sizeof(struct xfs_dqblk),
 				 XFS_DQUOT_CRC_OFF);
 	if (iocur_top->bbmap)
 		write_cur_bbs();
 	else
 		write_cur_buf();
+
+	/* If we didn't write the crc automatically, re-check validity */
+	if (iocur_top->ino_buf && skip_crc) {
+		xfs_dinode_t	*dip;
+		xfs_ino_t	ino;
+
+		dip = iocur_top->data;
+		ino = iocur_top->ino;
+		iocur_top->ino_crc_ok = xfs_verify_cksum((char *)dip,
+						mp->m_sb.sb_inodesize,
+						XFS_DINODE_CRC_OFF);
+	} 
 }
 
 void
diff --git a/db/write.h b/db/write.h
index 31e2665..664ddcc 100644
--- a/db/write.h
+++ b/db/write.h
@@ -20,5 +20,5 @@ struct field;
 
 extern void	write_init(void);
 extern void	write_block(const field_t *fields, int argc, char **argv);
-extern void	write_string(const field_t *fields, int argc, char **argv);
 extern void	write_struct(const field_t *fields, int argc, char **argv);
+extern void	write_string(const field_t *fields, int argc, char **argv);
diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
index d527230..46a1f39 100644
--- a/man/man8/xfs_db.8
+++ b/man/man8/xfs_db.8
@@ -87,16 +87,14 @@ or
 .I filename
 read-only. This option is required if the filesystem is mounted.
 It is only necessary to omit this flag if a command that changes data
-.RB ( write ", " blocktrash )
+.RB ( write ", " blocktrash ", " crc )
 is to be used.
 .TP
 .B \-x
 Specifies expert mode.
 This enables the
-.B write
-and
-.B blocktrash
-commands.
+.RB ( write ", " blocktrash ", " crc
+invalidate/revalidate) commands.
 .TP
 .B \-V
 Prints the version number and exits.
@@ -409,6 +407,25 @@ conversions such as
 .I agb
 .BR fsblock .
 .TP
+.B crc [\-i|\-r|\-v]
+Invalidates, revalidates, or validates the CRC (checksum)
+field of the current structure, if it has one.
+This command is available only on CRC-enabled filesystems.
+With no argument, validation is performed.
+Each command will display the resulting CRC value and state.
+.RS 1.0i
+.TP 0.4i
+.B \-i
+Invalidate the structure's CRC value (incrementing it by one),
+and write it to disk.
+.TP
+.B \-r
+Recalculate the current structure's correct CRC value, and write it to disk.
+.TP
+.B \-v
+Validate and display the current value and state of the structure's CRC.
+.RE
+.TP
 .BI "daddr [" d ]
 Set current address to the daddr (512 byte block) given by
 .IR d .
-- 
1.7.1

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

Re: [PATCH 03/13] xfs_db: add crc manipulation commands

[Brian Foster]

On Tue, Mar 17, 2015 at 03:33:05PM -0500, Eric Sandeen wrote:
> This adds a new "crc" command to xfs_db for CRC-enabled filesystems.
> 
> If a structure has a CRC field, we can validate it, invalidate/corrupt
> it, or revalidate/rewrite it:
> 
> xfs_db> sb 0
> xfs_db> crc -v
> crc = 0x796c814f (correct)
> xfs_db> crc -i
> Metadata CRC error detected at block 0x0/0x200
> crc = 0x796c8150 (bad)
> xfs_db> crc -r
> crc = 0x796c814f (correct)
> 
> (-i and -r require "expert" write-capable mode)
> 
> This requires temporarily replacing the write verifier with
> a dummy which won't recalculate the CRC on the way to disk.
> 
> It also required me to write a new flist function, which is
> totally foreign to me, so hopefully done right - but it seems
> to work here.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  db/Makefile       |    2 +-
>  db/command.c      |    2 +
>  db/crc.c          |  188 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  db/crc.h          |   22 ++++++
>  db/flist.c        |   34 ++++++++++
>  db/flist.h        |    1 +
>  db/io.c           |   24 ++++++-
>  db/write.h        |    2 +-
>  man/man8/xfs_db.8 |   27 ++++++--
>  9 files changed, 293 insertions(+), 9 deletions(-)
>  create mode 100644 db/crc.c
>  create mode 100644 db/crc.h
> 
> diff --git a/db/Makefile b/db/Makefile
> index fb01bdd..500f94b 100644
> --- a/db/Makefile
> +++ b/db/Makefile
> @@ -8,7 +8,7 @@ include $(TOPDIR)/include/builddefs
>  LTCOMMAND = xfs_db
>  
>  HFILES = addr.h agf.h agfl.h agi.h attr.h attrshort.h bit.h block.h bmap.h \
> -	btblock.h bmroot.h check.h command.h convert.h debug.h \
> +	btblock.h bmroot.h check.h command.h convert.h crc.h debug.h \
>  	dir2.h dir2sf.h dquot.h echo.h faddr.h field.h \
>  	flist.h fprint.h frag.h freesp.h hash.h help.h init.h inode.h input.h \
>  	io.h malloc.h metadump.h output.h print.h quit.h sb.h sig.h strvec.h \
> diff --git a/db/command.c b/db/command.c
> index b7e3165..d44e0a5 100644
> --- a/db/command.c
> +++ b/db/command.c
> @@ -48,6 +48,7 @@
>  #include "write.h"
>  #include "malloc.h"
>  #include "dquot.h"
> +#include "crc.h"
>  
>  cmdinfo_t	*cmdtab;
>  int		ncmds;
> @@ -123,6 +124,7 @@ init_commands(void)
>  	bmap_init();
>  	check_init();
>  	convert_init();
> +	crc_init();
>  	debug_init();
>  	echo_init();
>  	frag_init();
> diff --git a/db/crc.c b/db/crc.c
> new file mode 100644
> index 0000000..b58a594
> --- /dev/null
> +++ b/db/crc.c
> @@ -0,0 +1,188 @@
> +/*
> + * Copyright (c) 2014 Red Hat, Inc.
> + * All Rights Reserved.
> + *
> + * 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.
> + *
> + * 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/libxfs.h>
> +#include "addr.h"
> +#include "command.h"
> +#include "type.h"
> +#include "faddr.h"
> +#include "fprint.h"
> +#include "field.h"
> +#include "flist.h"
> +#include "io.h"
> +#include "init.h"
> +#include "output.h"
> +#include "bit.h"
> +#include "print.h"
> +
> +static int crc_f(int argc, char **argv);
> +static void crc_help(void);
> +
> +static const cmdinfo_t crc_cmd =
> +	{ "crc", NULL, crc_f, 0, 1, 0, "[-i|-r|-v]",
> +	  N_("manipulate crc values for V5 filesystem structures"), crc_help };
> +
> +void
> +crc_init(void)
> +{
> +	if (xfs_sb_version_hascrc(&mp->m_sb))
> +		add_command(&crc_cmd);
> +}
> +
> +static void
> +crc_help(void)
> +{
> +	dbprintf(_(
> +"\n"
> +" 'crc' validates, invalidates, or recalculates the crc value for\n"
> +" the current on-disk metadata structures in Version 5 filesystems.\n"
> +"\n"
> +" Usage:  \"crc [-i|-r|-v]\"\n"
> +"\n"
> +));
> +
> +}
> +
> +static int
> +crc_f(
> +	int		argc,
> +	char		**argv)
> +{
> +	struct xfs_buf_ops nowrite_ops;
> +	const struct xfs_buf_ops *stashed_ops = NULL;
> +	extern char	*progname;
> +	const field_t	*fields;
> +	const ftattr_t	*fa;
> +	flist_t		*fl;
> +	int		invalidate = 0;
> +	int		recalculate = 0;
> +	int		validate = 0;
> +	int		c;
> +
> +	if (cur_typ == NULL) {
> +		dbprintf(_("no current type\n"));
> +		return 0;
> +	}
> +
> +	if (cur_typ->fields == NULL) {
> +		dbprintf(_("current type (%s) is not a structure\n"),
> +			 cur_typ->name);
> +		return 0;
> +	}
> +
> +	if (argc) while ((c = getopt(argc, argv, "irv")) != EOF) {

The 'if (argc)' thing strikes again. Does this address something I'm not
aware of?

> +		switch (c) {
> +		case 'i':
> +			invalidate = 1;
> +			break;
> +		case 'r':
> +			recalculate = 1;
> +			break;
> +		case 'v':
> +			validate = 1;
> +			break;
> +		default:
> +			dbprintf(_("bad option for crc command\n"));
> +			return 0;
> +		}
> +	} else
> +		validate = 1;
> +
> +	if (invalidate + recalculate + validate > 1) {
> +		dbprintf(_("crc command accepts only one option\n"));
> +		return 0;
> +	}
> +
> +	if ((invalidate || recalculate) &&
> +	    ((x.isreadonly & LIBXFS_ISREADONLY) || !expert_mode)) {
> +		dbprintf(_("%s not in expert mode, writing disabled\n"),
> +			progname);
> +		return 0;
> +	}
> +
> +	fields = cur_typ->fields;
> +
> +	/* if we're a root field type, go down 1 layer to get field list */
> +	if (fields->name[0] == '\0') {
> +		fa = &ftattrtab[fields->ftyp];
> +		ASSERT(fa->ftyp == fields->ftyp);
> +		fields = fa->subfld;
> +	}
> +
> +	/* Search for a CRC field */
> +	fl = flist_find_ftyp(fields, FLDT_CRC);
> +	if (!fl) {
> +		dbprintf(_("No CRC field found for type %s\n"), cur_typ->name);
> +		return 0;
> +	}
> +
> +	/* run down the field list and set offsets into the data */
> +	if (!flist_parse(fields, fl, iocur_top->data, 0)) {
> +		flist_free(fl);
> +		dbprintf(_("parsing error\n"));
> +		return 0;
> +	}
> +
> +	if (invalidate) {
> +		flist_t		*sfl;
> +		int		bit_length;
> +		int		parentoffset;
> +		int		crc;
> +
> +		sfl = fl;
> +		parentoffset = 0;
> +		while (sfl->child) {
> +			parentoffset = sfl->offset;
> +			sfl = sfl->child;
> +		}
> +		ASSERT(sfl->fld->ftyp == FLDT_CRC);
> +
> +		bit_length = fsize(sfl->fld, iocur_top->data, parentoffset, 0);
> +		bit_length *= fcount(sfl->fld, iocur_top->data, parentoffset);
> +		crc = getbitval(iocur_top->data, sfl->offset, bit_length, BVUNSIGNED);
> +		/* Off by one.. */
> +		crc = cpu_to_be32(crc + 1);
> +		setbitval(iocur_top->data, sfl->offset, bit_length, &crc);

Some comments on the above code would be nice to explain what it's
calculating.

> +
> +		/* Temporarily remove write verifier to write a bad CRC */
> +		stashed_ops = iocur_top->bp->b_ops;
> +		nowrite_ops.verify_read = stashed_ops->verify_read;
> +		nowrite_ops.verify_write = xfs_dummy_verify;
> +		iocur_top->bp->b_ops = &nowrite_ops;
> +	}
> +
> +	if (invalidate || recalculate) {
> +		if (invalidate)
> +			dbprintf(_("Invalidating CRC:\n"));
> +		else
> +			dbprintf(_("Recalculating CRC:\n"));
> +
> +		write_cur();
> +		if (stashed_ops)
> +			iocur_top->bp->b_ops = stashed_ops;
> +		/* re-verify to get proper b_error state */
> +		iocur_top->bp->b_ops->verify_read(iocur_top->bp);
> +	} else
> +		dbprintf(_("Verifying CRC:\n"));

Does the "verify" aspect of this command do anything, or are we just
printing the state that was determined from a previous cursor load? If
the latter, I wonder if it's worth retaining that option (e.g., just
print an inode to see the crc state)..?

> +
> +	/* And show us what we've got! */
> +	flist_print(fl);
> +	print_flist(fl);
> +	flist_free(fl);
> +	return 0;
> +}
> diff --git a/db/crc.h b/db/crc.h
> new file mode 100644
> index 0000000..9f44615
> --- /dev/null
> +++ b/db/crc.h
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright (c) 2014 Red Hat, Inc.
> + * All Rights Reserved.
> + *
> + * 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.
> + *
> + * 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
> + */
> +
> +struct field;
> +
> +extern void	crc_init(void);
> +extern void	crc_struct(const field_t *fields, int argc, char **argv);
> diff --git a/db/flist.c b/db/flist.c
> index 33f7da7..fa19f70 100644
> --- a/db/flist.c
> +++ b/db/flist.c
> @@ -411,6 +411,40 @@ flist_split(
>  	return v;
>  }
>  
> +/*
> + * Given a set of fields, scan for a field of the given type.
> + * Return an flist leading to the first found field
> + * of that type.
> + * Return NULL if no field of the given type is found.
> + */
> +flist_t *
> +flist_find_ftyp(
> +	const field_t *fields,
> +	fldt_t	type)
> +{
> +	flist_t	*fl;
> +	const field_t	*f;
> +	const ftattr_t  *fa;
> +
> +	for (f = fields; f->name; f++) {
> +		fl = flist_make(f->name);
> +		fl->fld = f;
> +		if (f->ftyp == type)
> +			return fl;
> +		fa = &ftattrtab[f->ftyp];
> +		if (fa->subfld) {
> +			flist_t *nfl;
> +			nfl = flist_find_ftyp(fa->subfld, type);
> +			if (nfl) {
> +				fl->child = nfl;
> +				return fl;
> +			}
> +		}
> +		flist_free(fl);

Ok, I haven't really dug into the type code to grok this, but it looks
like we're doing an flist allocation/free for each iteration of the
loop. Could we turn that inside out to only do that in the path where we
find the type to return? It looks to me like we could, but I could
easily be missing something.

> +	}
> +	return NULL;
> +}
> +
>  static void
>  ftok_free(
>  	ftok_t	*ft)
> diff --git a/db/flist.h b/db/flist.h
> index 5c9fba0..3f4b312 100644
> --- a/db/flist.h
> +++ b/db/flist.h
> @@ -37,3 +37,4 @@ extern int	flist_parse(const struct field *fields, flist_t *fl, void *obj,
>  			    int startoff);
>  extern void	flist_print(flist_t *fl);
>  extern flist_t	*flist_scan(char *name);
> +extern flist_t	*flist_find_ftyp(const field_t *fields, fldt_t  type);
> diff --git a/db/io.c b/db/io.c
> index 91858e2..732b320 100644
> --- a/db/io.c
> +++ b/db/io.c
> @@ -27,6 +27,7 @@
>  #include "output.h"
>  #include "init.h"
>  #include "malloc.h"
> +#include "crc.h"
>  
>  static int	pop_f(int argc, char **argv);
>  static void     pop_help(void);
> @@ -466,22 +467,41 @@ xfs_dummy_verify(
>  void
>  write_cur(void)
>  {
> +	int skip_crc = 0;
> +
> +	if (iocur_top->bp->b_ops &&
> +	    iocur_top->bp->b_ops->verify_write == xfs_dummy_verify)
> +		skip_crc = 1;
> +
>  	if (iocur_sp < 0) {
>  		dbprintf(_("nothing to write\n"));
>  		return;
>  	}
>  
> -	if (iocur_top->ino_buf) {
> +	if (iocur_top->ino_buf && !skip_crc) {
>  		libxfs_dinode_calc_crc(mp, iocur_top->data);
>  		iocur_top->ino_crc_ok = 1;
>  	}
> -	if (iocur_top->dquot_buf)
> +
> +	if (iocur_top->dquot_buf && !skip_crc)
>  		xfs_update_cksum(iocur_top->data, sizeof(struct xfs_dqblk),
>  				 XFS_DQUOT_CRC_OFF);
>  	if (iocur_top->bbmap)
>  		write_cur_bbs();
>  	else
>  		write_cur_buf();
> +
> +	/* If we didn't write the crc automatically, re-check validity */
> +	if (iocur_top->ino_buf && skip_crc) {
> +		xfs_dinode_t	*dip;
> +		xfs_ino_t	ino;
> +
> +		dip = iocur_top->data;
> +		ino = iocur_top->ino;

ino is unused.

> +		iocur_top->ino_crc_ok = xfs_verify_cksum((char *)dip,
> +						mp->m_sb.sb_inodesize,
> +						XFS_DINODE_CRC_OFF);
> +	} 

Trailing space on the above line.

Brian

>  }
>  
>  void
> diff --git a/db/write.h b/db/write.h
> index 31e2665..664ddcc 100644
> --- a/db/write.h
> +++ b/db/write.h
> @@ -20,5 +20,5 @@ struct field;
>  
>  extern void	write_init(void);
>  extern void	write_block(const field_t *fields, int argc, char **argv);
> -extern void	write_string(const field_t *fields, int argc, char **argv);
>  extern void	write_struct(const field_t *fields, int argc, char **argv);
> +extern void	write_string(const field_t *fields, int argc, char **argv);
> diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
> index d527230..46a1f39 100644
> --- a/man/man8/xfs_db.8
> +++ b/man/man8/xfs_db.8
> @@ -87,16 +87,14 @@ or
>  .I filename
>  read-only. This option is required if the filesystem is mounted.
>  It is only necessary to omit this flag if a command that changes data
> -.RB ( write ", " blocktrash )
> +.RB ( write ", " blocktrash ", " crc )
>  is to be used.
>  .TP
>  .B \-x
>  Specifies expert mode.
>  This enables the
> -.B write
> -and
> -.B blocktrash
> -commands.
> +.RB ( write ", " blocktrash ", " crc
> +invalidate/revalidate) commands.
>  .TP
>  .B \-V
>  Prints the version number and exits.
> @@ -409,6 +407,25 @@ conversions such as
>  .I agb
>  .BR fsblock .
>  .TP
> +.B crc [\-i|\-r|\-v]
> +Invalidates, revalidates, or validates the CRC (checksum)
> +field of the current structure, if it has one.
> +This command is available only on CRC-enabled filesystems.
> +With no argument, validation is performed.
> +Each command will display the resulting CRC value and state.
> +.RS 1.0i
> +.TP 0.4i
> +.B \-i
> +Invalidate the structure's CRC value (incrementing it by one),
> +and write it to disk.
> +.TP
> +.B \-r
> +Recalculate the current structure's correct CRC value, and write it to disk.
> +.TP
> +.B \-v
> +Validate and display the current value and state of the structure's CRC.
> +.RE
> +.TP
>  .BI "daddr [" d ]
>  Set current address to the daddr (512 byte block) given by
>  .IR d .
> -- 
> 1.7.1
> 
> _______________________________________________
> 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 03/13] xfs_db: add crc manipulation commands

[Eric Sandeen]

On 3/19/15 10:07 AM, Brian Foster wrote:
> On Tue, Mar 17, 2015 at 03:33:05PM -0500, Eric Sandeen wrote:
>> This adds a new "crc" command to xfs_db for CRC-enabled filesystems.
...

if (argc) while ((c = getopt(argc, argv, "irv")) != EOF) {
> 
> The 'if (argc)' thing strikes again. Does this address something I'm not
> aware of?

just a dumb/unfortunate cut & paste from elsewhere, I'll fix.

...

>> +		/* Off by one.. */
>> +		crc = cpu_to_be32(crc + 1);
>> +		setbitval(iocur_top->data, sfl->offset, bit_length, &crc);
> 
> Some comments on the above code would be nice to explain what it's
> calculating.

Yeah, wish I'd written them "yonks ago" ;)  I'll reverse engineer my
patch & add comments.
 
>> +		dbprintf(_("Verifying CRC:\n"));
> 
> Does the "verify" aspect of this command do anything, or are we just
> printing the state that was determined from a previous cursor load? If
> the latter, I wonder if it's worth retaining that option (e.g., just
> print an inode to see the crc state)..?

If having a specific CRC handler makes sense (and, maybe it doesn't,
with the other patch that allows corrupted write), the symmetry seemed
nice.

Given the amount of code this adds, perhaps we should just drop it,
TBH, and use the "write -c" functionality I added, instead.

-Eric

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

Re: [PATCH 03/13] xfs_db: add crc manipulation commands

[Brian Foster]

On Fri, Mar 20, 2015 at 09:30:13PM -0500, Eric Sandeen wrote:
> On 3/19/15 10:07 AM, Brian Foster wrote:
> > On Tue, Mar 17, 2015 at 03:33:05PM -0500, Eric Sandeen wrote:
> >> This adds a new "crc" command to xfs_db for CRC-enabled filesystems.
> ...
> 
> if (argc) while ((c = getopt(argc, argv, "irv")) != EOF) {
> > 
> > The 'if (argc)' thing strikes again. Does this address something I'm not
> > aware of?
> 
> just a dumb/unfortunate cut & paste from elsewhere, I'll fix.
> 
> ...
> 
> >> +		/* Off by one.. */
> >> +		crc = cpu_to_be32(crc + 1);
> >> +		setbitval(iocur_top->data, sfl->offset, bit_length, &crc);
> > 
> > Some comments on the above code would be nice to explain what it's
> > calculating.
> 
> Yeah, wish I'd written them "yonks ago" ;)  I'll reverse engineer my
> patch & add comments.
>  
> >> +		dbprintf(_("Verifying CRC:\n"));
> > 
> > Does the "verify" aspect of this command do anything, or are we just
> > printing the state that was determined from a previous cursor load? If
> > the latter, I wonder if it's worth retaining that option (e.g., just
> > print an inode to see the crc state)..?
> 
> If having a specific CRC handler makes sense (and, maybe it doesn't,
> with the other patch that allows corrupted write), the symmetry seemed
> nice.
> 

It's not really a big deal, it just wasn't quite clear. A comment would
help.

> Given the amount of code this adds, perhaps we should just drop it,
> TBH, and use the "write -c" functionality I added, instead.
> 

Hmm, well personally I'm not against having both. My initial reasoning
was to say that there's a difference between corrupting a structure
field and the crc. Thinking further, I suppose we can use the write -c
mechanism to write to the crc field itself. Given that and the type tree
gymnastics that this implementation has to do, I suppose I could see
leaning in favor of just write -c.

It's your call I guess. Maybe it's not worth it if you have to go back
through the type stuff and remember what it does to document it. :)

Brian

> -Eric
> 
> _______________________________________________
> 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

[PATCH 03/13 DROP] xfs_db: add crc manipulation commands

[Eric Sandeen]

Just drop this patch, it adds xfs_db complexity, and isn't needed as
long as we can write any arbitrary "corrupt" field, including the crc
field of any structure, which my previous patch implements.

Thanks,
-Eric

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

[PATCH 04/13] xfs_db: nlink fields are valid for di_version == 3, too

[Eric Sandeen]

Printing inodes with di_version == 3 skips the nlink
fields, because they are only printed if di_version == 2.
This was intended to separate them from di_version == 1,
but it mistakenly excluded di_version == 3, which also contains
these fields.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 db/inode.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/db/inode.c b/db/inode.c
index 982acb7..c26e1a0 100644
--- a/db/inode.c
+++ b/db/inode.c
@@ -369,7 +369,7 @@ inode_core_nlinkv2_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int
@@ -382,7 +382,7 @@ inode_core_onlink_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int
@@ -395,7 +395,7 @@ inode_core_projid_count(
 	ASSERT(startoff == 0);
 	ASSERT(obj == iocur_top->data);
 	dic = obj;
-	return dic->di_version == 2;
+	return dic->di_version >= 2;
 }
 
 static int
-- 
1.7.1

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

[PATCH 05/13] xfs_repair: dirty inode in process_sf_dir2 if we change namelen

[Eric Sandeen]

There are two "fix sfep->namelen" cases, but we only mark
*dino_dirty = 1 in one of them.  Add the other to ensure that
the change gets written out.

Reviewed-by: Brian Foster <bfoster@redhat.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 repair/dir2.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/repair/dir2.c b/repair/dir2.c
index 6b8964d..25793e9 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -881,6 +881,7 @@ _("entry \"%*.*s\" in shortform directory %" PRIu64 " references %s inode %" PRI
 _("zero length entry in shortform dir %" PRIu64 ", resetting to %d\n"),
 						ino, namelen);
 					sfep->namelen = namelen;
+					*dino_dirty = 1;
 				} else  {
 					do_warn(
 _("zero length entry in shortform dir %" PRIu64 ", would set to %d\n"),
-- 
1.7.1

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

[PATCH 06/13] xfs_repair: remove impossible tests in process_sf_dir2

[Eric Sandeen]

We're testing for an impossible case in here:

                if (i == num_entries - 1)  {
...
                } else  {
...
                                if (i == num_entries - 1)
                                    /* can't happen! */
...
                }

So, remove the impossible case.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 repair/dir2.c |   26 ++++++--------------------
 1 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/repair/dir2.c b/repair/dir2.c
index 25793e9..7aede6a 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -928,26 +928,12 @@ _("size of last entry overflows space left in in shortform dir %" PRIu64 ", "),
 				do_warn(
 _("size of entry #%d overflows space left in in shortform dir %" PRIu64 "\n"),
 					i, ino);
-				if (!no_modify)  {
-					if (i == num_entries - 1)
-						do_warn(
-						_("junking entry #%d\n"),
-							i);
-					else
-						do_warn(
-						_("junking %d entries\n"),
-							num_entries - i);
-				} else  {
-					if (i == num_entries - 1)
-						do_warn(
-						_("would junk entry #%d\n"),
-							i);
-					else
-						do_warn(
-						_("would junk %d entries\n"),
-							num_entries - i);
-				}
-
+				if (!no_modify)
+					do_warn(_("junking %d entries\n"),
+						num_entries - i);
+				else
+					do_warn(_("would junk %d entries\n"),
+						num_entries - i);
 				break;
 			}
 		}
-- 
1.7.1

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

[PATCH 07/13] xfs_repair: collapse 2 cases in process_sf_dir2

[Eric Sandeen]

process_sf_dir2() has 2 cases for a bad namelen: on-disk namelen
is 0, or on-disk namelen extends beyond the directory i_size.

After the prior 2 patches, the code for each case is now essentially
the same, so collapse the two cases.

Note, this does slightly change some of the error messages.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 repair/dir2.c |   71 ++++++++++++++++++--------------------------------------
 1 files changed, 23 insertions(+), 48 deletions(-)

diff --git a/repair/dir2.c b/repair/dir2.c
index 7aede6a..5512e41 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -861,36 +861,44 @@ process_sf_dir2(
 _("entry \"%*.*s\" in shortform directory %" PRIu64 " references %s inode %" PRIu64 "\n"),
 				namelen, namelen, sfep->name, ino, junkreason,
 				lino);
-		if (namelen == 0)  {
-			/*
-			 * if we're really lucky, this is
-			 * the last entry in which case we
-			 * can use the dir size to set the
-			 * namelen value.  otherwise, forget
-			 * it because we're not going to be
-			 * able to find the next entry.
-			 */
+
+		/* is dir namelen 0 or does this entry extend past dir size? */
+		if (namelen == 0) {
+			junkreason = _("is zero length");
 			bad_sfnamelen = 1;
+		} else if ((__psint_t) sfep - (__psint_t) sfp +
+				xfs_dir3_sf_entsize(mp, sfp, sfep->namelen)
+							> ino_dir_size)  {
+			junkreason = _("extends past end of dir");
+			bad_sfnamelen = 1;
+		}
 
+		if (bad_sfnamelen) {
+			/*
+			 * if we're really lucky, this is the last entry in
+			 * case we can use the dir size to set the namelen
+			 * value.  otherwise, forget it because we're not going
+			 * to be able to find the next entry.
+			 */
 			if (i == num_entries - 1)  {
 				namelen = ino_dir_size -
 					((__psint_t) &sfep->name[0] -
 					 (__psint_t) sfp);
 				if (!no_modify)  {
 					do_warn(
-_("zero length entry in shortform dir %" PRIu64 ", resetting to %d\n"),
-						ino, namelen);
+_("entry #%d %s in shortform dir %" PRIu64 ", resetting to %d\n"),
+						i, junkreason, ino, namelen);
 					sfep->namelen = namelen;
 					*dino_dirty = 1;
 				} else  {
 					do_warn(
-_("zero length entry in shortform dir %" PRIu64 ", would set to %d\n"),
-						ino, namelen);
+_("entry #%d %s in shortform dir %" PRIu64 ", would set to %d\n"),
+						i, junkreason, ino, namelen);
 				}
 			} else  {
 				do_warn(
-_("zero length entry in shortform dir %" PRIu64 ""),
-					ino);
+_("entry #%d %s in shortform dir %" PRIu64),
+					i, junkreason, ino);
 				if (!no_modify)
 					do_warn(_(", junking %d entries\n"),
 						num_entries - i);
@@ -903,39 +911,6 @@ _("zero length entry in shortform dir %" PRIu64 ""),
 				 */
 				break;
 			}
-		} else if ((__psint_t) sfep - (__psint_t) sfp +
-				xfs_dir3_sf_entsize(mp, sfp, sfep->namelen)
-							> ino_dir_size)  {
-			bad_sfnamelen = 1;
-
-			if (i == num_entries - 1)  {
-				namelen = ino_dir_size -
-					((__psint_t) &sfep->name[0] -
-					 (__psint_t) sfp);
-				do_warn(
-_("size of last entry overflows space left in in shortform dir %" PRIu64 ", "),
-					ino);
-				if (!no_modify)  {
-					do_warn(_("resetting to %d\n"),
-						namelen);
-					sfep->namelen = namelen;
-					*dino_dirty = 1;
-				} else  {
-					do_warn(_("would reset to %d\n"),
-						namelen);
-				}
-			} else  {
-				do_warn(
-_("size of entry #%d overflows space left in in shortform dir %" PRIu64 "\n"),
-					i, ino);
-				if (!no_modify)
-					do_warn(_("junking %d entries\n"),
-						num_entries - i);
-				else
-					do_warn(_("would junk %d entries\n"),
-						num_entries - i);
-				break;
-			}
 		}
 
 		/*
-- 
1.7.1

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

[PATCH 08/13] xfs_repair: remove last-entry hack in process_sf_dir2

[Eric Sandeen]

process_sf_dir2() tries to special-case the last entry in
a short form dir, and salvage it if the name length is wrong
by using the dir size as a clue to what the length should be.

However, this doesn't actually work; there is either a 32-bit
or 64-bit inode after the name, and with dirv3 there's a file
type in there as well.  Extending the name length to the dir
size means it overlaps these fields, which often have a 0 in
the top bits, and then namecheck() fails the result and junks
it anyway:

> entry #1 is zero length in shortform dir 67, resetting to 29
> entry contains illegal character in shortform dir 67
> junking entry "bbbbbbbbbbbbbbbbbbbbbbbb" in directory inode 67

And depending on the corruption, the current code will set
a new negative namelen if it turns out that the name itself
starts beyond dir size; it can't be shortened enough.

So, we could fix this up, and choose a new namelen such that
the xfs_dir2_ino8_t and/or xfs_dir2_ino8_t and/or file type
still fits at the end, but we really seem to be reaching the
point of diminishing returns.  The chances that nothing but
the name length is wrong, and that the remaining few
bytes at the end of the dir size are actually correct, seems
awfully small.

Therefore just remove this special case, which I think is
of questionable value.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 repair/dir2.c |   47 ++++++++++++-----------------------------------
 1 files changed, 12 insertions(+), 35 deletions(-)

diff --git a/repair/dir2.c b/repair/dir2.c
index 5512e41..b1816f4 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -874,43 +874,20 @@ _("entry \"%*.*s\" in shortform directory %" PRIu64 " references %s inode %" PRI
 		}
 
 		if (bad_sfnamelen) {
+			do_warn(
+_("entry #%d %s in shortform dir %" PRIu64),
+				i, junkreason, ino);
+			if (!no_modify)
+				do_warn(_(", junking %d entries\n"),
+					num_entries - i);
+			else
+				do_warn(_(", would junk %d entries\n"),
+					num_entries - i);
 			/*
-			 * if we're really lucky, this is the last entry in
-			 * case we can use the dir size to set the namelen
-			 * value.  otherwise, forget it because we're not going
-			 * to be able to find the next entry.
+			 * don't process the rest of the directory,
+			 * break out of processing loop
 			 */
-			if (i == num_entries - 1)  {
-				namelen = ino_dir_size -
-					((__psint_t) &sfep->name[0] -
-					 (__psint_t) sfp);
-				if (!no_modify)  {
-					do_warn(
-_("entry #%d %s in shortform dir %" PRIu64 ", resetting to %d\n"),
-						i, junkreason, ino, namelen);
-					sfep->namelen = namelen;
-					*dino_dirty = 1;
-				} else  {
-					do_warn(
-_("entry #%d %s in shortform dir %" PRIu64 ", would set to %d\n"),
-						i, junkreason, ino, namelen);
-				}
-			} else  {
-				do_warn(
-_("entry #%d %s in shortform dir %" PRIu64),
-					i, junkreason, ino);
-				if (!no_modify)
-					do_warn(_(", junking %d entries\n"),
-						num_entries - i);
-				else
-					do_warn(_(", would junk %d entries\n"),
-						num_entries - i);
-				/*
-				 * don't process the rest of the directory,
-				 * break out of processing looop
-				 */
-				break;
-			}
+			break;
 		}
 
 		/*
-- 
1.7.1

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

[PATCH 09/13] libxfs: remove ASSERT on ftype read from disk

[Eric Sandeen]

This one is already fixed in the kernel, with
	fb04013 xfs: don't ASSERT on corrupt ftype
but that kernel<->userspace merge is still pending.

In the meantime, just fix it as a one-off here, because ASSERTing
on bad on-disk values when running xfs_repair is a very unfriendly
thing to do.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 include/xfs_da_format.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
index 89a1a21..695e698 100644
--- a/include/xfs_da_format.h
+++ b/include/xfs_da_format.h
@@ -561,7 +561,7 @@ xfs_dir3_dirent_get_ftype(
 	if (xfs_sb_version_hasftype(&mp->m_sb)) {
 		__uint8_t	type = dep->name[dep->namelen];
 
-		ASSERT(type < XFS_DIR3_FT_MAX);
+		//ASSERT(type < XFS_DIR3_FT_MAX);
 		if (type < XFS_DIR3_FT_MAX)
 			return type;
 
-- 
1.7.1

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

Re: [PATCH 09/13] libxfs: remove ASSERT on ftype read from disk

[Brian Foster]

On Tue, Mar 17, 2015 at 03:33:11PM -0500, Eric Sandeen wrote:
> This one is already fixed in the kernel, with
> 	fb04013 xfs: don't ASSERT on corrupt ftype
> but that kernel<->userspace merge is still pending.
> 
> In the meantime, just fix it as a one-off here, because ASSERTing
> on bad on-disk values when running xfs_repair is a very unfriendly
> thing to do.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  include/xfs_da_format.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
> index 89a1a21..695e698 100644
> --- a/include/xfs_da_format.h
> +++ b/include/xfs_da_format.h
> @@ -561,7 +561,7 @@ xfs_dir3_dirent_get_ftype(
>  	if (xfs_sb_version_hasftype(&mp->m_sb)) {
>  		__uint8_t	type = dep->name[dep->namelen];
>  
> -		ASSERT(type < XFS_DIR3_FT_MAX);
> +		//ASSERT(type < XFS_DIR3_FT_MAX);

Just delete it..?

Brian

>  		if (type < XFS_DIR3_FT_MAX)
>  			return type;
>  
> -- 
> 1.7.1
> 
> _______________________________________________
> 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 09/13] libxfs: remove ASSERT on ftype read from disk

[Eric Sandeen]

On 3/19/15 11:46 AM, Brian Foster wrote:

>> diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
>> index 89a1a21..695e698 100644
>> --- a/include/xfs_da_format.h
>> +++ b/include/xfs_da_format.h
>> @@ -561,7 +561,7 @@ xfs_dir3_dirent_get_ftype(
>>  	if (xfs_sb_version_hasftype(&mp->m_sb)) {
>>  		__uint8_t	type = dep->name[dep->namelen];
>>  
>> -		ASSERT(type < XFS_DIR3_FT_MAX);
>> +		//ASSERT(type < XFS_DIR3_FT_MAX);
> 
> Just delete it..?

Urgh.  yes.  I thought I had done so.

Thanks,
-Eric

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

PATCH 09/13 V2] libxfs: remove ASSERT on ftype read from disk

[Eric Sandeen]

This one is already fixed in the kernel, with
	fb04013 xfs: don't ASSERT on corrupt ftype
but that kernel<->userspace merge is still pending.

In the meantime, just fix it as a one-off here, because ASSERTing
on bad on-disk values when running xfs_repair is a very unfriendly
thing to do.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Remove it, don't cplusplus-comment it out!  o_O

diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
index 89a1a21..11f1420 100644
--- a/include/xfs_da_format.h
+++ b/include/xfs_da_format.h
@@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
 	if (xfs_sb_version_hasftype(&mp->m_sb)) {
 		__uint8_t	type = dep->name[dep->namelen];
 
-		ASSERT(type < XFS_DIR3_FT_MAX);
 		if (type < XFS_DIR3_FT_MAX)
 			return type;
 

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

Re: PATCH 09/13 V2] libxfs: remove ASSERT on ftype read from disk

[Brian Foster]

On Mon, Mar 23, 2015 at 03:13:45PM -0500, Eric Sandeen wrote:
> This one is already fixed in the kernel, with
> 	fb04013 xfs: don't ASSERT on corrupt ftype
> but that kernel<->userspace merge is still pending.
> 
> In the meantime, just fix it as a one-off here, because ASSERTing
> on bad on-disk values when running xfs_repair is a very unfriendly
> thing to do.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

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

> 
> Remove it, don't cplusplus-comment it out!  o_O
> 
> diff --git a/include/xfs_da_format.h b/include/xfs_da_format.h
> index 89a1a21..11f1420 100644
> --- a/include/xfs_da_format.h
> +++ b/include/xfs_da_format.h
> @@ -561,7 +561,6 @@ xfs_dir3_dirent_get_ftype(
>  	if (xfs_sb_version_hasftype(&mp->m_sb)) {
>  		__uint8_t	type = dep->name[dep->namelen];
>  
> -		ASSERT(type < XFS_DIR3_FT_MAX);
>  		if (type < XFS_DIR3_FT_MAX)
>  			return type;
>  
> 
> _______________________________________________
> 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

[PATCH 10/13] xfs_repair: clear need_root_dotdot if we rebuild the root dir

[Eric Sandeen]

It's possible to enter longform_dir2_rebuild with need_root_dotdot
set; rebuilding it will add "..", and if need_root_dotdot stays set,
we'll add another ".." entry, causing a second repair to find and
fix that problem.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 repair/phase6.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/repair/phase6.c b/repair/phase6.c
index 0ec4f07..1728609 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1354,6 +1354,9 @@ longform_dir2_rebuild(
 
 	libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
 
+	if (ino == mp->m_sb.sb_rootino)
+		need_root_dotdot = 0;
+
 	/* go through the hash list and re-add the inodes */
 
 	for (p = hashtab->first; p; p = p->nextbyorder) {
-- 
1.7.1

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

[PATCH 11/13] xfs_repair: set *parent if process_dir2_data() fixes root inode

[Eric Sandeen]

process_dir2_data() may fix the root dir's parent inode:

"bad .. entry in root directory inode 6912, was 7159: correcting"

But we don't update the *parent passed in in that case; this then leads to
an assert later in process_dir2:

xfs_repair: dir2.c:2039: process_dir2:
  Assertion `(ino != mp->m_sb.sb_rootino && ino != *parent) ||
  (ino == mp->m_sb.sb_rootino && (ino == *parent || need_root_dotdot == 1))'
  failed.

Updating the value of *parent when we fix the parent value resolves this
problem.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 repair/dir2.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/repair/dir2.c b/repair/dir2.c
index b1816f4..9e6c67d 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -1407,6 +1407,7 @@ _("bad .. entry in root directory inode %" PRIu64 ", was %" PRIu64 ": "),
 					} else {
 						do_warn(_("would correct\n"));
 					}
+					*parent = ino;
 				}
 			}
 			/*
-- 
1.7.1

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

[PATCH 12/13] xfs_repair: don't clear . or .. in process_dir2_data

[Eric Sandeen]

process_dir2_data() has special . and .. processing; it is able
to correct these inodes, so there is no reason to clear them.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 repair/dir2.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/repair/dir2.c b/repair/dir2.c
index 9e6c67d..3acf71c 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -1331,6 +1331,18 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64
 				dep->namelen = 1;
 			clearino = 1;
 		}
+
+		/*
+		 * We have a special dot & dotdot fixer-upper below which can
+		 * sort out the proper inode number, so don't clear it.
+		 */
+		if ((dep->namelen == 1 && dep->name[0] == '.') ||
+		    (dep->namelen == 2 &&
+		     dep->name[0] == '.' && dep->name[1] == '.')) {
+			clearino = 0;
+			clearreason = NULL;
+		}
+		    
 		/*
 		 * If needed to clear the inode number, do it now.
 		 */
-- 
1.7.1

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

Re: [PATCH 12/13] xfs_repair: don't clear . or .. in process_dir2_data

[Brian Foster]

On Tue, Mar 17, 2015 at 03:33:14PM -0500, Eric Sandeen wrote:
> process_dir2_data() has special . and .. processing; it is able
> to correct these inodes, so there is no reason to clear them.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  repair/dir2.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/repair/dir2.c b/repair/dir2.c
> index 9e6c67d..3acf71c 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -1331,6 +1331,18 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64
>  				dep->namelen = 1;
>  			clearino = 1;
>  		}
> +
> +		/*
> +		 * We have a special dot & dotdot fixer-upper below which can
> +		 * sort out the proper inode number, so don't clear it.
> +		 */
> +		if ((dep->namelen == 1 && dep->name[0] == '.') ||
> +		    (dep->namelen == 2 &&
> +		     dep->name[0] == '.' && dep->name[1] == '.')) {
> +			clearino = 0;
> +			clearreason = NULL;
> +		}
> +		    

Whitespace damage on the blank line above.

Seems Ok, but the question I have is what happens if the dot or dotdot
namelen was bogus? The change in behavior here is that we don't trash
the entry, right? If so, are we reliant on something else detecting
absence of either entry later and fixing it up?

Brian

>  		/*
>  		 * If needed to clear the inode number, do it now.
>  		 */
> -- 
> 1.7.1
> 
> _______________________________________________
> 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 12/13] xfs_repair: don't clear . or .. in process_dir2_data

[Eric Sandeen]

On 3/19/15 11:47 AM, Brian Foster wrote:
> On Tue, Mar 17, 2015 at 03:33:14PM -0500, Eric Sandeen wrote:
>> process_dir2_data() has special . and .. processing; it is able
>> to correct these inodes, so there is no reason to clear them.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  repair/dir2.c |   12 ++++++++++++
>>  1 files changed, 12 insertions(+), 0 deletions(-)
>>
>> diff --git a/repair/dir2.c b/repair/dir2.c
>> index 9e6c67d..3acf71c 100644
>> --- a/repair/dir2.c
>> +++ b/repair/dir2.c
>> @@ -1331,6 +1331,18 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64
>>  				dep->namelen = 1;
>>  			clearino = 1;
>>  		}
>> +
>> +		/*
>> +		 * We have a special dot & dotdot fixer-upper below which can
>> +		 * sort out the proper inode number, so don't clear it.
>> +		 */
>> +		if ((dep->namelen == 1 && dep->name[0] == '.') ||
>> +		    (dep->namelen == 2 &&
>> +		     dep->name[0] == '.' && dep->name[1] == '.')) {
>> +			clearino = 0;
>> +			clearreason = NULL;
>> +		}
>> +		    
> 
> Whitespace damage on the blank line above.
> 
> Seems Ok, but the question I have is what happens if the dot or dotdot
> namelen was bogus? 

If namelen is 1 and name[0] is '.', or
if namelen is 2 and name[0] is '.' and name[1] is '..'

then how can that the len be bogus?  The test is for the name being
either precisely '.' or '..' and nothing else, right?

-Eric

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

Re: [PATCH 12/13] xfs_repair: don't clear . or .. in process_dir2_data

[Brian Foster]

On Thu, Mar 19, 2015 at 12:29:27PM -0500, Eric Sandeen wrote:
> On 3/19/15 11:47 AM, Brian Foster wrote:
> > On Tue, Mar 17, 2015 at 03:33:14PM -0500, Eric Sandeen wrote:
> >> process_dir2_data() has special . and .. processing; it is able
> >> to correct these inodes, so there is no reason to clear them.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>  repair/dir2.c |   12 ++++++++++++
> >>  1 files changed, 12 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/repair/dir2.c b/repair/dir2.c
> >> index 9e6c67d..3acf71c 100644
> >> --- a/repair/dir2.c
> >> +++ b/repair/dir2.c
> >> @@ -1331,6 +1331,18 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64
> >>  				dep->namelen = 1;
> >>  			clearino = 1;
> >>  		}
> >> +
> >> +		/*
> >> +		 * We have a special dot & dotdot fixer-upper below which can
> >> +		 * sort out the proper inode number, so don't clear it.
> >> +		 */
> >> +		if ((dep->namelen == 1 && dep->name[0] == '.') ||
> >> +		    (dep->namelen == 2 &&
> >> +		     dep->name[0] == '.' && dep->name[1] == '.')) {
> >> +			clearino = 0;
> >> +			clearreason = NULL;
> >> +		}
> >> +		    
> > 
> > Whitespace damage on the blank line above.
> > 
> > Seems Ok, but the question I have is what happens if the dot or dotdot
> > namelen was bogus? 
> 
> If namelen is 1 and name[0] is '.', or
> if namelen is 2 and name[0] is '.' and name[1] is '..'
> 
> then how can that the len be bogus?  The test is for the name being
> either precisely '.' or '..' and nothing else, right?
> 

Ah, yeah I see. So it would be cleared in that case.

Note that just above if namelen == 0 we set it to 1. Would we have the
opposite problem for hidden files with bogus namelen (i.e., not clear
entries that we should)?

Brian

> -Eric
> 
> _______________________________________________
> 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 12/13] xfs_repair: don't clear . or .. in process_dir2_data

[Eric Sandeen]

On 3/19/15 12:54 PM, Brian Foster wrote:
> On Thu, Mar 19, 2015 at 12:29:27PM -0500, Eric Sandeen wrote:
>> On 3/19/15 11:47 AM, Brian Foster wrote:
>>> On Tue, Mar 17, 2015 at 03:33:14PM -0500, Eric Sandeen wrote:
>>>> process_dir2_data() has special . and .. processing; it is able
>>>> to correct these inodes, so there is no reason to clear them.
>>>>
>>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>>> ---
>>>>  repair/dir2.c |   12 ++++++++++++
>>>>  1 files changed, 12 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/repair/dir2.c b/repair/dir2.c
>>>> index 9e6c67d..3acf71c 100644
>>>> --- a/repair/dir2.c
>>>> +++ b/repair/dir2.c
>>>> @@ -1331,6 +1331,18 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64
>>>>  				dep->namelen = 1;
>>>>  			clearino = 1;
>>>>  		}
>>>> +
>>>> +		/*
>>>> +		 * We have a special dot & dotdot fixer-upper below which can
>>>> +		 * sort out the proper inode number, so don't clear it.
>>>> +		 */
>>>> +		if ((dep->namelen == 1 && dep->name[0] == '.') ||
>>>> +		    (dep->namelen == 2 &&
>>>> +		     dep->name[0] == '.' && dep->name[1] == '.')) {
>>>> +			clearino = 0;
>>>> +			clearreason = NULL;
>>>> +		}
>>>> +		    
>>>
>>> Whitespace damage on the blank line above.
>>>
>>> Seems Ok, but the question I have is what happens if the dot or dotdot
>>> namelen was bogus? 
>>
>> If namelen is 1 and name[0] is '.', or
>> if namelen is 2 and name[0] is '.' and name[1] is '..'
>>
>> then how can that the len be bogus?  The test is for the name being
>> either precisely '.' or '..' and nothing else, right?
>>
> 
> Ah, yeah I see. So it would be cleared in that case.
> 
> Note that just above if namelen == 0 we set it to 1. Would we have the
> opposite problem for hidden files with bogus namelen (i.e., not clear
> entries that we should)?

Hm, yeah.  Maybe moving my new hunk above that check makes sense.

-Eric

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

Re: [PATCH 12/13] xfs_repair: don't clear . or .. in process_dir2_data

[Brian Foster]

On Thu, Mar 19, 2015 at 12:59:15PM -0500, Eric Sandeen wrote:
> On 3/19/15 12:54 PM, Brian Foster wrote:
> > On Thu, Mar 19, 2015 at 12:29:27PM -0500, Eric Sandeen wrote:
> >> On 3/19/15 11:47 AM, Brian Foster wrote:
> >>> On Tue, Mar 17, 2015 at 03:33:14PM -0500, Eric Sandeen wrote:
> >>>> process_dir2_data() has special . and .. processing; it is able
> >>>> to correct these inodes, so there is no reason to clear them.
> >>>>
> >>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >>>> ---
> >>>>  repair/dir2.c |   12 ++++++++++++
> >>>>  1 files changed, 12 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/repair/dir2.c b/repair/dir2.c
> >>>> index 9e6c67d..3acf71c 100644
> >>>> --- a/repair/dir2.c
> >>>> +++ b/repair/dir2.c
> >>>> @@ -1331,6 +1331,18 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64
> >>>>  				dep->namelen = 1;
> >>>>  			clearino = 1;
> >>>>  		}
> >>>> +
> >>>> +		/*
> >>>> +		 * We have a special dot & dotdot fixer-upper below which can
> >>>> +		 * sort out the proper inode number, so don't clear it.
> >>>> +		 */
> >>>> +		if ((dep->namelen == 1 && dep->name[0] == '.') ||
> >>>> +		    (dep->namelen == 2 &&
> >>>> +		     dep->name[0] == '.' && dep->name[1] == '.')) {
> >>>> +			clearino = 0;
> >>>> +			clearreason = NULL;
> >>>> +		}
> >>>> +		    
> >>>
> >>> Whitespace damage on the blank line above.
> >>>
> >>> Seems Ok, but the question I have is what happens if the dot or dotdot
> >>> namelen was bogus? 
> >>
> >> If namelen is 1 and name[0] is '.', or
> >> if namelen is 2 and name[0] is '.' and name[1] is '..'
> >>
> >> then how can that the len be bogus?  The test is for the name being
> >> either precisely '.' or '..' and nothing else, right?
> >>
> > 
> > Ah, yeah I see. So it would be cleared in that case.
> > 
> > Note that just above if namelen == 0 we set it to 1. Would we have the
> > opposite problem for hidden files with bogus namelen (i.e., not clear
> > entries that we should)?
> 
> Hm, yeah.  Maybe moving my new hunk above that check makes sense.
> 

I think that makes sense. I guess we ultimately can't get around a file
that starts with dot looking like the dot entry if the entry is
corrupted just right, but at least we don't risk fabricating that
scenario ourselves.

Brian

> -Eric
> 
> _______________________________________________
> 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

[PATCH 12/13 V2] xfs_repair: don't clear . or .. in process_dir2_data

[Eric Sandeen]

process_dir2_data() has special . and .. processing; it is able
to correct these inodes, so there is no reason to clear them.

Do this before we adjust a length 0 filename to length 1, so
that we don't take this action on an accidentally created "."
name from a hidden dotfile.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: Move the new hunk up, before we possibly reset the namelen to 1; 
a 0-length ".hidden" file could turn in a length-1 "." and not get
cleared.

diff --git a/repair/dir2.c b/repair/dir2.c
index 9e6c67d..8ecc29b 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -1318,6 +1318,18 @@ _("entry \"%*.*s\" at block %d offset %" PRIdPTR " in directory inode %" PRIu64
 				dep->namelen, dep->namelen, dep->name,
 				da_bno, (intptr_t)ptr - (intptr_t)d, ino,
 				clearreason, ent_ino);
+
+		/*
+		 * We have a special dot & dotdot fixer-upper below which can
+		 * sort out the proper inode number, so don't clear it.
+		 */
+		if ((dep->namelen == 1 && dep->name[0] == '.') ||
+		    (dep->namelen == 2 &&
+		     dep->name[0] == '.' && dep->name[1] == '.')) {
+			clearino = 0;
+			clearreason = NULL;
+		}
+
 		/*
 		 * If the name length is 0 (illegal) make it 1 and blast
 		 * the entry.

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

Re: [PATCH 12/13 V2] xfs_repair: don't clear . or .. in process_dir2_data

[Brian Foster]

On Mon, Mar 23, 2015 at 03:17:58PM -0500, Eric Sandeen wrote:
> process_dir2_data() has special . and .. processing; it is able
> to correct these inodes, so there is no reason to clear them.
> 
> Do this before we adjust a length 0 filename to length 1, so
> that we don't take this action on an accidentally created "."
> name from a hidden dotfile.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

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

> 
> V2: Move the new hunk up, before we possibly reset the namelen to 1; 
> a 0-length ".hidden" file could turn in a length-1 "." and not get
> cleared.
> 
> diff --git a/repair/dir2.c b/repair/dir2.c
> index 9e6c67d..8ecc29b 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -1318,6 +1318,18 @@ _("entry \"%*.*s\" at block %d offset %" PRIdPTR " in directory inode %" PRIu64
>  				dep->namelen, dep->namelen, dep->name,
>  				da_bno, (intptr_t)ptr - (intptr_t)d, ino,
>  				clearreason, ent_ino);
> +
> +		/*
> +		 * We have a special dot & dotdot fixer-upper below which can
> +		 * sort out the proper inode number, so don't clear it.
> +		 */
> +		if ((dep->namelen == 1 && dep->name[0] == '.') ||
> +		    (dep->namelen == 2 &&
> +		     dep->name[0] == '.' && dep->name[1] == '.')) {
> +			clearino = 0;
> +			clearreason = NULL;
> +		}
> +
>  		/*
>  		 * If the name length is 0 (illegal) make it 1 and blast
>  		 * the entry.
> 
> _______________________________________________
> 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

[PATCH 13/13] xfs_repair: validate & fix inode CRCs

[Eric Sandeen]

xfs_repair doesn't ever check an inode's CRC, so it never repairs
them.  If the root inode or realtime inodes have bad crcs, the
fs won't even mount and can't be fixed (without using xfs_db).

It's fairly straightforward to just test the inode CRC before
we do any other checking or modification of the inode, and
just mark it dirty if it's wrong and needs to be re-written.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 repair/dinode.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/repair/dinode.c b/repair/dinode.c
index 5d9094b..05db838 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2291,6 +2291,30 @@ process_dinode_int(xfs_mount_t *mp,
 	 */
 	ASSERT(uncertain == 0 || verify_mode != 0);
 
+	/*
+	 * This is the only valid point to check the CRC; after this we may have
+	 * made changes which invalidate it, and the CRC is only updated again
+	 * when it gets written out.
+	 *
+	 * Of course if we make any modifications after this, the inode gets
+	 * rewritten, and the CRC is updated automagically.
+	 */
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	    !xfs_verify_cksum((char *)dino, mp->m_sb.sb_inodesize,
+                		XFS_DINODE_CRC_OFF)) {
+		retval = 1;
+		if (!uncertain)
+			do_warn(_("bad CRC for inode %" PRIu64 "%c"),
+				lino, verify_mode ? '\n' : ',');
+		if (!verify_mode) {
+			if (!no_modify) {
+				do_warn(_(" will rewrite\n"));
+				*dirty = 1;
+			} else
+				do_warn(_(" would rewrite\n"));
+		}
+	}
+
 	if (be16_to_cpu(dino->di_magic) != XFS_DINODE_MAGIC)  {
 		retval = 1;
 		if (!uncertain)
-- 
1.7.1

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

Re: [PATCH 13/13] xfs_repair: validate & fix inode CRCs

[Brian Foster]

On Tue, Mar 17, 2015 at 03:33:15PM -0500, Eric Sandeen wrote:
> xfs_repair doesn't ever check an inode's CRC, so it never repairs
> them.  If the root inode or realtime inodes have bad crcs, the
> fs won't even mount and can't be fixed (without using xfs_db).
> 
> It's fairly straightforward to just test the inode CRC before
> we do any other checking or modification of the inode, and
> just mark it dirty if it's wrong and needs to be re-written.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  repair/dinode.c |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 5d9094b..05db838 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2291,6 +2291,30 @@ process_dinode_int(xfs_mount_t *mp,
>  	 */
>  	ASSERT(uncertain == 0 || verify_mode != 0);
>  
> +	/*
> +	 * This is the only valid point to check the CRC; after this we may have
> +	 * made changes which invalidate it, and the CRC is only updated again
> +	 * when it gets written out.
> +	 *
> +	 * Of course if we make any modifications after this, the inode gets
> +	 * rewritten, and the CRC is updated automagically.
> +	 */
> +	if (xfs_sb_version_hascrc(&mp->m_sb) &&
> +	    !xfs_verify_cksum((char *)dino, mp->m_sb.sb_inodesize,
> +                		XFS_DINODE_CRC_OFF)) {

   ^^^^^^ whitespace ;)

Otherwise looks good to me:

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

> +		retval = 1;
> +		if (!uncertain)
> +			do_warn(_("bad CRC for inode %" PRIu64 "%c"),
> +				lino, verify_mode ? '\n' : ',');
> +		if (!verify_mode) {
> +			if (!no_modify) {
> +				do_warn(_(" will rewrite\n"));
> +				*dirty = 1;
> +			} else
> +				do_warn(_(" would rewrite\n"));
> +		}
> +	}
> +
>  	if (be16_to_cpu(dino->di_magic) != XFS_DINODE_MAGIC)  {
>  		retval = 1;
>  		if (!uncertain)
> -- 
> 1.7.1
> 
> _______________________________________________
> 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

[PATCH 13/13 V2] xfs_repair: validate & fix inode CRCs

[Eric Sandeen]

xfs_repair doesn't ever check an inode's CRC, so it never repairs
them.  If the root inode or realtime inodes have bad crcs, the
fs won't even mount and can't be fixed (without using xfs_db).

It's fairly straightforward to just test the inode CRC before
we do any other checking or modification of the inode, and
just mark it dirty if it's wrong and needs to be re-written.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---

V2: fix whitespace, urgh!

diff --git a/repair/dinode.c b/repair/dinode.c
index 5d9094b..035212c 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2291,6 +2291,30 @@ process_dinode_int(xfs_mount_t *mp,
 	 */
 	ASSERT(uncertain == 0 || verify_mode != 0);
 
+	/*
+	 * This is the only valid point to check the CRC; after this we may have
+	 * made changes which invalidate it, and the CRC is only updated again
+	 * when it gets written out.
+	 *
+	 * Of course if we make any modifications after this, the inode gets
+	 * rewritten, and the CRC is updated automagically.
+	 */
+	if (xfs_sb_version_hascrc(&mp->m_sb) &&
+	    !xfs_verify_cksum((char *)dino, mp->m_sb.sb_inodesize,
+				XFS_DINODE_CRC_OFF)) {
+		retval = 1;
+		if (!uncertain)
+			do_warn(_("bad CRC for inode %" PRIu64 "%c"),
+				lino, verify_mode ? '\n' : ',');
+		if (!verify_mode) {
+			if (!no_modify) {
+				do_warn(_(" will rewrite\n"));
+				*dirty = 1;
+			} else
+				do_warn(_(" would rewrite\n"));
+		}
+	}
+
 	if (be16_to_cpu(dino->di_magic) != XFS_DINODE_MAGIC)  {
 		retval = 1;
 		if (!uncertain)


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