xfsprogs: roll-up of previously sent patches
cover xfsprogs: roll-up of previously sent patches
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/13 xfs_db: Allow writes of corrupted data by optionally skipping write verifiers
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.
--- 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(-)
db/io.c +7 -0
@@ -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 ) {
db/io.h +1 -0
@@ -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
db/write.c +31 -4
@@ -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 ; }
man/man8/xfs_db.8 +7 -1
@@ -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,
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 01/13] xfs_db: Allow writes of corrupted data by optionally skipping write verifiers
Quoted 69 lines · 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 <[email protected]>--- 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(-)db/io.c +7
-0@@ -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 ) {db/io.h +1
-0@@ -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 db/write.c +2
-1@@ -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" )); }
db/write.c write_f( · line 97
Quoted 7 lines
db/write.c +4
-0@@ -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:
Quoted 74 lines
db/write.c +25
-3if (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 ; }man/man8/xfs_db.8 +7
-1@@ -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,Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 1/13 xfs_db: Allow writes of corrupted data by optionally skipping write verifiers
In reply to Eric SandeenBeing 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.
---
V2: Fix whitespace, remove "if (argc)", and test for iocur_top->bp->b_ops before attempting to deref it.
db/io.c +7 -0
@@ -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 ) {
db/io.h +1 -0
@@ -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
db/write.c +31 -4
@@ -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 ; }
man/man8/xfs_db.8 +7 -1
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 01/13 V2] xfs_db: Allow writes of corrupted data by optionally skipping write verifiers
Quoted 6 lines · 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.
Quoted 6 lines
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 <[email protected]>---
Quoted 131 lines
V2: Fix whitespace, remove "if (argc)", and test for
iocur_top->bp->b_opsbefore attempting to deref it.db/io.c +7
-0@@ -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 ) {db/io.h +1
-0@@ -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 db/write.c +31
-4@@ -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 ; }man/man8/xfs_db.8 +7
-1@@ -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,
Quoted 4 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/13 xfs_db: fix inode CRC validity state, and warn on read if invalid
In reply to Eric SandeenCurrently, 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
--- db/inode.c | 7 ++++++- db/io.c | 4 +++- include/libxfs.h | 2 ++ 3 files changed, 11 insertions(+), 2 deletions(-)
db/inode.c +6 -1
@@ -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 (); }
db/io.c +3 -1
@@ -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 );
include/libxfs.h +2 -0
@@ -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) {
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 02/13] xfs_db: fix inode CRC validity state, and warn on read if invalid
Quoted 13 lines · 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:
Quoted 8 lines
xfs_db> inode 129
Metadata corruption detected at block 0x80/0x2000 Metadata corruption detected at block 0x80/0x2000
...
Metadata CRC error detected for ino 129Signed-off-by Eric Sandeen <[email protected]>---
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?
Quoted 10 lines
db/inode.c | 7 ++++++- db/io.c | 4 +++- include/libxfs.h | 2 ++ 3 files changed, 11 insertions(+), 2 deletions(-)db/inode.c +0
-0
db/inode.c set_cur_inode( · line 689
Quoted 7 lines
db/inode.c +3
-1@@ -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.
db/inode.c set_cur_inode( · line 698
Quoted 9 lines
db/inode.c +3
-0iocur_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
Quoted 39 lines
db/inode.c +0
-0/* track updated info in ring */ ring_add (); }db/io.c +3
-1@@ -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 );include/libxfs.h +2
-0@@ -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) {Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 02/13] xfs_db: fix inode CRC validity state, and warn on read if invalid
Quoted 20 lines · Brian Foster
Quoted 19 lines · 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
Quoted 6 lines
Quoted 2 lines
Signed-off-by Eric Sandeen <[email protected]>---
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. :)
Quoted 14 lines
Quoted 14 lines
db/inode.c | 7 ++++++- db/io.c | 4 +++- include/libxfs.h | 2 ++ 3 files changed, 11 insertions(+), 2 deletions(-)db/inode.c +0
-1@@ -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);
Quoted 8 lines
db/inode.c set_cur_inode( · line 689
Quoted 3 lines
db/inode.c +3
-0+ 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 toxfs_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?
Quoted 6 lines
Quoted 6 lines
db/inode.c +0
-0iocur_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;
Quoted 7 lines
db/inode.c set_cur_inode( · line 698
Quoted 3 lines
db/inode.c +3
-0+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 theverify_cksum()check as well, rather than verifying a cksum and ignoring it.
ok, sure.
Quoted 5 lines
It's also a little confusing how this field is handled without crc enabled.
write_cur()looks like it sets it and callslibxfs_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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 02/13] xfs_db: fix inode CRC validity state, and warn on read if invalid
Quoted 67 lines · Eric Sandeen
Quoted 20 lines · Brian Foster
Quoted 19 lines · 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 129Quoted 6 lines
Quoted 2 lines
Signed-off-by Eric Sandeen <[email protected]>---
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_okkinda indicates that the inode... crc ... is ok? :)Prior to this, it was only used in
iocur_crc_valid(), which is called fromfp_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. :)
Quoted 14 lines
Quoted 14 lines
db/inode.c | 7 ++++++- db/io.c | 4 +++- include/libxfs.h | 2 ++ 3 files changed, 11 insertions(+), 2 deletions(-)db/inode.c +0
-1@@ -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);Quoted 8 lines
db/inode.c set_cur_inode( · line 689
Quoted 3 lines
db/inode.c +3
-0+ 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 toxfs_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.
Quoted 2 lines
xfs_ireadstill callsxfs_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... ;)
Quoted 16 lines
Quoted 6 lines
Quoted 6 lines
db/inode.c +0
-0iocur_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;Quoted 7 lines
db/inode.c set_cur_inode( · line 698
Quoted 3 lines
db/inode.c +3
-0+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 theverify_cksum()check as well, rather than verifying a cksum and ignoring it.ok, sure.
Quoted 9 lines
Quoted 5 lines
It's also a little confusing how this field is handled without crc enabled.
write_cur()looks like it sets it and callslibxfs_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
Quoted 6 lines
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 2/13 xfs_db: fix inode CRC validity state, and warn on read if invalid
In reply to Eric SandeenCurrently, 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
---
V2: move ino_crc_ok calculations under "if (xfs_sb_version_hascrc())"
db/inode.c +10 -1
@@ -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 (); }
db/io.c +3 -1
@@ -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 );
include/libxfs.h +2 -0
@@ -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
[email protected]
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
Quoted 13 lines · 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:
Quoted 8 lines
xfs_db> inode 129
Metadata corruption detected at block 0x80/0x2000 Metadata corruption detected at block 0x80/0x2000
...
Metadata CRC error detected for ino 129Signed-off-by Eric Sandeen <[email protected]>---
Quoted 62 lines
V2: move
ino_crc_okcalculations under "if (xfs_sb_version_hascrc())"db/inode.c +10
-1@@ -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 (); }db/io.c +3
-1@@ -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 );include/libxfs.h +2
-0@@ -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) {
Quoted 4 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 3/13 xfs_db: add crc manipulation commands
In reply to Eric SandeenThis 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 0xfs_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.
--- 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
db/Makefile +1 -1
@@ -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 \
db/command.c +2 -0
@@ -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 ();
db/crc.c new file +188 -0
188 lines not on this page · raw message
db/crc.h new file +22 -0
@@ -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);
db/flist.c +34 -0
@@ -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)
db/flist.h +1 -0
@@ -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);
db/io.c +22 -2
@@ -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
db/write.h +1 -1
@@ -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);
man/man8/xfs_db.8 +22 -5
@@ -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 .
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 03/13] xfs_db: add crc manipulation commands
Quoted 155 lines · Eric Sandeen
This adds a new "crc" command to
xfs_dbfor CRC-enabled filesystems.If a structure has a CRC field, we can validate it, invalidate/corrupt it, or revalidate/rewrite it:
xfs_db> sb 0xfs_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 <[email protected]>--- 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.hdb/Makefile +1
-1@@ -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 \db/command.c +2
-0@@ -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 ();db/crc.c new file +78
-0@@ -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" ));
db/crc.c line 88
Quoted 10 lines
db/crc.c new file +10
-0+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?
Quoted 62 lines
db/crc.c new file +62
-0+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;
db/crc.c line 160
Quoted 10 lines
db/crc.c new file +10
-0+ 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.
Quoted 11 lines
db/crc.c new file +11
-0+ +/* 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" ));
db/crc.c line 181
Quoted 10 lines
db/crc.c new file +10
-0+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)..?
Quoted 63 lines
db/crc.c new file +7
-0+ +/* And show us what we've got! */ +flist_print (fl); +print_flist (fl); +flist_free (fl); +return 0 ; +}db/crc.h new file +22
-0@@ -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);db/flist.c +20
-0@@ -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;
db/flist.c flist_split( · line 443
Quoted 10 lines
db/flist.c +10
-0+ 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.
Quoted 55 lines
db/flist.c +4
-0+ } +return NULL ; +} +static void ftok_free (ftok_t *ft)db/flist.h +1
-0@@ -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);db/io.c +10
-2@@ -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 ();
db/io.c xfs_dummy_verify( · line 500
Quoted 10 lines
db/io.c +8
-0else 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.
db/io.c xfs_dummy_verify( · line 504
Quoted 4 lines
db/io.c +4
-0+ 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
Quoted 71 lines
db/io.c +0
-0}void db/write.h +1
-1@@ -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);man/man8/xfs_db.8 +22
-5@@ -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 .Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 03/13] xfs_db: add crc manipulation commands
Quoted 2 lines · Brian Foster
Quoted 1 line · Eric Sandeen
This adds a new "crc" command to
xfs_dbfor CRC-enabled filesystems.
...
if (argc) while ((c = getopt(argc, argv, "irv")) != EOF) {
Quoted 3 lines
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.
...
Quoted 6 lines
db/crc.c line 160
Quoted 3 lines
db/crc.c new file +3
-0+/* 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.
Quoted 6 lines
db/crc.c line 181
Quoted 1 line
db/crc.c new file +1
-0+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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 03/13] xfs_db: add crc manipulation commands
Quoted 31 lines · Eric Sandeen
Quoted 2 lines · Brian Foster
Quoted 1 line · Eric Sandeen
This adds a new "crc" command to
xfs_dbfor CRC-enabled filesystems....
if (argc) while ((c =
getopt(argc, argv, "irv")) != EOF) {Quoted 3 lines
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.
...
Quoted 6 lines
db/crc.c line 160
Quoted 3 lines
db/crc.c new file +3
-0+/* 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.
Quoted 6 lines
db/crc.c line 181
Quoted 1 line
db/crc.c new file +1
-0+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)..?
Quoted 4 lines
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.
Quoted 3 lines
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
Quoted 6 lines
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 3/13 xfs_db: add crc manipulation commands
In reply to Eric SandeenJust 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 4/13 xfs_db: nlink fields are valid for di_version == 3, too
In reply to Eric SandeenPrinting 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.
--- db/inode.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
db/inode.c +3 -3
@@ -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
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 5/13 xfs_repair: dirty inode in process_sf_dir2 if we change namelen
In reply to Eric SandeenThere 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.
--- repair/dir2.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
repair/dir2.c +1 -0
@@ -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"),
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 6/13 xfs_repair: remove impossible tests in process_sf_dir2
In reply to Eric SandeenWe'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.
--- repair/dir2.c | 26 ++++++-------------------- 1 files changed, 6 insertions(+), 20 deletions(-)
repair/dir2.c +6 -20
@@ -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 ; } }
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 7/13 xfs_repair: collapse 2 cases in process_sf_dir2
In reply to Eric Sandeenprocess_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.
--- repair/dir2.c | 71 ++++++++++++++++++-------------------------------------- 1 files changed, 23 insertions(+), 48 deletions(-)
repair/dir2.c +23 -48
@@ -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 ; - } }/*
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 8/13 xfs_repair: remove last-entry hack in process_sf_dir2
In reply to Eric Sandeenprocess_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:
Quoted 3 lines
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.
--- repair/dir2.c | 47 ++++++++++++----------------------------------- 1 files changed, 12 insertions(+), 35 deletions(-)
repair/dir2.c +12 -35
@@ -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 ; }/*
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 9/13 libxfs: remove ASSERT on ftype read from disk
In reply to Eric SandeenThis 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.
--- include/xfs_da_format.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
include/xfs_da_format.h +1 -1
@@ -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;
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 09/13] libxfs: remove ASSERT on ftype read from disk
Quoted 18 lines · 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_repairis a very unfriendly thing to do.Signed-off-by Eric Sandeen <[email protected]>--- include/xfs_da_format.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)include/xfs_da_format.h +0
-0
include/xfs_da_format.h xfs_dir3_dirent_get_ftype( · line 564
Quoted 5 lines
include/xfs_da_format.h +1
-1@@ -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
Quoted 10 lines
include/xfs_da_format.h +0
-0if (type <XFS_DIR3_FT_MAX )return type;Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 09/13] libxfs: remove ASSERT on ftype read from disk
Quoted 8 lines · Brian Foster
Quoted 8 lines
include/xfs_da_format.h +0
-0@@ -561,7 +561,7 @@ xfs_dir3_dirent_get_ftype(if (xfs_sb_version_hasftype (&mp->m_sb)) {__uint8_t type = dep->name[dep->namelen];
Quoted 4 lines
include/xfs_da_format.h xfs_dir3_dirent_get_ftype( · line 564
Quoted 2 lines
include/xfs_da_format.h +1
-1-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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 09/13 V2] libxfs: remove ASSERT on ftype read from disk
In reply to Eric SandeenThis 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.
---
Remove it, don't cplusplus-comment it out! o_O
include/xfs_da_format.h +0 -1
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Brian Foster
Reviewed-byRe: PATCH 09/13 V2] libxfs: remove ASSERT on ftype read from disk
Quoted 10 lines · 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_repairis a very unfriendly thing to do.Signed-off-by Eric Sandeen <[email protected]>---
Quoted 16 lines
Remove it, don't cplusplus-comment it out!
o_Oinclude/xfs_da_format.h +0
-1@@ -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;
Quoted 4 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 10/13 xfs_repair: clear need_root_dotdot if we rebuild the root dir
In reply to Eric SandeenIt'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.
--- repair/phase6.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
repair/phase6.c +3 -0
@@ -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) {
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 11/13 xfs_repair: set *parent if process_dir2_data() fixes root inode
In reply to Eric Sandeenprocess_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.
--- repair/dir2.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
repair/dir2.c +1 -0
@@ -1407,6 +1407,7 @@ _("bad .. entry in root directory inode %" PRIu64 ", was %" PRIu64 ": "), }else {do_warn (_ ("would correct\n" )); } + *parent = ino; } }/*
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 12/13 xfs_repair: don't clear . or .. in process_dir2_data
In reply to Eric Sandeenprocess_dir2_data() has special . and .. processing; it is able to correct these inodes, so there is no reason to clear them.
--- repair/dir2.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
repair/dir2.c +12 -0
@@ -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. */
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 12/13] xfs_repair: don't clear . or .. in process_dir2_data
Quoted 18 lines · 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 <[email protected]>--- repair/dir2.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)repair/dir2.c +2
-0@@ -1331,6 +1331,18 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64 dep->namelen =1 ; clearino =1 ; } + +/*
repair/dir2.c _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64 · line 1345
Quoted 10 lines
repair/dir2.c +10
-0+* 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
Quoted 10 lines
repair/dir2.c +0
-0/* * If needed to clear the inode number, do it now. */ Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 12/13] xfs_repair: don't clear . or .. in process_dir2_data
Quoted 32 lines · Brian Foster
Quoted 18 lines · 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 <[email protected]>--- repair/dir2.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)repair/dir2.c +2
-0@@ -1331,6 +1331,18 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64 dep->namelen =1 ; clearino =1 ; } + +/* repair/dir2.c _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64 · line 1345
Quoted 10 lines
repair/dir2.c +10
-0+* 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.
Quoted 2 lines
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 12/13] xfs_repair: don't clear . or .. in process_dir2_data
Quoted 33 lines · Eric Sandeen
Quoted 32 lines · Brian Foster
Quoted 18 lines · 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 <[email protected]>--- repair/dir2.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)repair/dir2.c +2
-0@@ -1331,6 +1331,18 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64 dep->namelen =1 ; clearino =1 ; } + +/* repair/dir2.c _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64 · line 1345
Quoted 10 lines
repair/dir2.c +10
-0+* 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.
Quoted 9 lines
Quoted 2 lines
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
Quoted 6 lines
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 12/13] xfs_repair: don't clear . or .. in process_dir2_data
Quoted 44 lines · Brian Foster
Quoted 33 lines · Eric Sandeen
Quoted 32 lines · Brian Foster
Quoted 18 lines · 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 <[email protected]>--- repair/dir2.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)repair/dir2.c +2
-0@@ -1331,6 +1331,18 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64 dep->namelen =1 ; clearino =1 ; } + +/* repair/dir2.c _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64 · line 1345
Quoted 10 lines
repair/dir2.c +10
-0+* 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.
Quoted 9 lines
Quoted 2 lines
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?
Quoted 5 lines
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 12/13] xfs_repair: don't clear . or .. in process_dir2_data
Quoted 45 lines · Eric Sandeen
Quoted 44 lines · Brian Foster
Quoted 42 lines · Eric Sandeen
Quoted 32 lines · Brian Foster
Quoted 18 lines · 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 <[email protected]>--- repair/dir2.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)repair/dir2.c +2
-0@@ -1331,6 +1331,18 @@ _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64 dep->namelen =1 ; clearino =1 ; } + +/* repair/dir2.c _("entry at block %u offset %" PRIdPTR " in directory inode %" PRIu64 · line 1345
Quoted 10 lines
repair/dir2.c +10
-0+* 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.
Quoted 2 lines
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?
Quoted 8 lines
Quoted 5 lines
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
Quoted 6 lines
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 12/13 xfs_repair: don't clear . or .. in process_dir2_data
In reply to Eric Sandeenprocess_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.
---
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.
repair/dir2.c +12 -0
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 12/13 V2] xfs_repair: don't clear . or .. in process_dir2_data
Quoted 9 lines · 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 <[email protected]>---
Quoted 29 lines
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.
repair/dir2.c +12
-0@@ -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.
Quoted 4 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 13/13 xfs_repair: validate & fix inode CRCs
In reply to Eric Sandeenxfs_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.
--- repair/dinode.c | 24 ++++++++++++++++++++++++ 1 files changed, 24 insertions(+), 0 deletions(-)
repair/dinode.c +24 -0
@@ -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)
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 13/13] xfs_repair: validate & fix inode CRCs
Quoted 22 lines · Eric Sandeen
xfs_repairdoesn'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 usingxfs_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 <[email protected]>--- repair/dinode.c | 24 ++++++++++++++++++++++++ 1 files changed, 24 insertions(+), 0 deletions(-)repair/dinode.c +1
-0@@ -2291,6 +2291,30 @@ process_dinode_int(xfs_mount_t *mp,*/ ASSERT (uncertain ==0 || verify_mode !=0 ); +/*
repair/dinode.c process_dinode_int(xfs_mount_t *mp, · line 2304
Quoted 10 lines
repair/dinode.c +10
-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:
Quoted 23 lines
repair/dinode.c +13
-0+ 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)Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 13/13 xfs_repair: validate & fix inode CRCs
In reply to Eric Sandeenxfs_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.
---
V2: fix whitespace, urgh!
repair/dinode.c +24 -0
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs