xfsdump: 2 fixes
cover xfsdump: 2 fixes
I've run into a couple bugs running logprint, 2 patches follow to try to fix them up.
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/2 xfsdump: Handle multiply-logged inode fields
As xlog_print_trans_inode() stands today, it will error out if more than one flag is set on f->ilf_fields:
xlog_print_trans_inode: illegal inode type
but this is a perfectly valid case, to have i.e. a data and an attr flag set.
Following is a pretty big reworking of the function to handle more than one field type set.
I've tested this by a simple test such as creating one file on an selinux box, so that data+attr is set, and logprinting; I've also tested by running logprint after subsequent xfstest runs (although we hit other bugs that way).
---
logprint/log_misc.c +63 -76
@@ -657,97 +657,84 @@ xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops)/* does anything come next */ op_head = (xlog_op_header_t *)*ptr; -switch (f->ilf_fields &XFS_ILOG_NONCORE ) { -case XFS_ILOG_DEXT : { -ASSERT (f->ilf_size ==3 ); - (*i)++; -xlog_print_op_header (op_head, *i, ptr); -printf (_ ("EXTENTS inode data\n" )); - *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { -return 1 ; - } -break ; - } -case XFS_ILOG_DBROOT : { -ASSERT (f->ilf_size ==3 ); - (*i)++; -xlog_print_op_header (op_head, *i, ptr); -printf (_ ("BTREE inode data\n" )); - *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { -return 1 ; - } + +if (f->ilf_fields & (XFS_ILOG_DEV |XFS_ILOG_UUID )) { +switch (f->ilf_fields & (XFS_ILOG_DEV |XFS_ILOG_UUID )) { +case XFS_ILOG_DEV : +printf (_ ("DEV inode: no extra region\n" ));break ; - } -case XFS_ILOG_DDATA : { -ASSERT (f->ilf_size ==3 ); - (*i)++; -xlog_print_op_header (op_head, *i, ptr); -printf (_ ("LOCAL inode data\n" )); -if (mode ==S_IFDIR ) { -xlog_print_dir_sf ((xfs_dir_shortform_t *)*ptr, size); - } - *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { -return 1 ; - } +case XFS_ILOG_UUID : +printf (_ ("UUID inode: no extra region\n" ));break ; +case XFS_ILOG_DEXT : +case XFS_ILOG_DBROOT : +case XFS_ILOG_DDATA : +default : +xlog_panic (_ ("xlog_print_trans_inode: illegal inode type 0x%x" ), + f->ilf_fields); } -case XFS_ILOG_AEXT : { -ASSERT (f->ilf_size ==3 ); + } + +if (f->ilf_fields & (XFS_ILOG_DFORK |XFS_ILOG_AFORK )) { +ASSERT (f->ilf_size <=4 ); +ASSERT ((f->ilf_size ==3 ) || (f->ilf_fields &XFS_ILOG_AFORK )); +if (f->ilf_fields &XFS_ILOG_DFORK ) { (*i)++;xlog_print_op_header (op_head, *i, ptr); -printf (_ ("EXTENTS inode attr\n" )); - *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { -return 1 ; + +switch (f->ilf_fields &XFS_ILOG_DFORK ) { +case XFS_ILOG_DEXT : +printf (_ ("EXTENTS inode data\n" )); +break ; +case XFS_ILOG_DBROOT : +printf (_ ("BTREE inode data\n" )); +break ; +case XFS_ILOG_DDATA : +printf (_ ("LOCAL inode data\n" )); +if (mode ==S_IFDIR ) +xlog_print_dir_sf ((xfs_dir_shortform_t *)*ptr, size); +break ; +case XFS_ILOG_DEV : +case XFS_ILOG_UUID : +default : +xlog_panic (_ ("xlog_print_trans_inode: illegal inode type 0x%x" ), + f->ilf_fields); } -break ; - } -case XFS_ILOG_ABROOT : { -ASSERT (f->ilf_size ==3 ); - (*i)++; -xlog_print_op_header (op_head, *i, ptr); -printf (_ ("BTREE inode attr\n" )); + *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { +if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS ))return 1 ; - } -break ; + op_head = (xlog_op_header_t *)*ptr; } -case XFS_ILOG_ADATA : { -ASSERT (f->ilf_size ==3 ); + +if (f->ilf_fields &XFS_ILOG_AFORK ) { (*i)++;xlog_print_op_header (op_head, *i, ptr); -printf (_ ("LOCAL inode attr\n" )); -if (mode ==S_IFDIR ) { -xlog_print_dir_sf ((xfs_dir_shortform_t *)*ptr, size); + +switch (f->ilf_fields &XFS_ILOG_AFORK ) { +case XFS_ILOG_AEXT : +printf (_ ("EXTENTS attr data\n" )); +break ; +case XFS_ILOG_ABROOT : +printf (_ ("BTREE attr data\n" )); +break ; +case XFS_ILOG_ADATA : +printf (_ ("LOCAL attr data\n" )); +if (mode ==S_IFDIR ) +xlog_print_dir_sf ((xfs_dir_shortform_t *)*ptr, size); +break ; +default : +xlog_panic (_ ("xlog_print_trans_inode: illegal inode type 0x%x" ), + f->ilf_fields); } *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { +if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS ))return 1 ; - } -break ; - } -case XFS_ILOG_DEV : { -ASSERT (f->ilf_size ==2 ); -printf (_ ("DEV inode: no extra region\n" )); -break ; - } -case XFS_ILOG_UUID : { -ASSERT (f->ilf_size ==2 ); -printf (_ ("UUID inode: no extra region\n" )); -break ; - } -case 0 : { -ASSERT (f->ilf_size ==2 ); -break ; + op_head = (xlog_op_header_t *)*ptr; } -default : { -xlog_panic (_ ("xlog_print_trans_inode: illegal inode type" )); - } - } + }else /* neither XFS_ILOG_DFORK nor XFS_ILOG_AFORK */ +ASSERT (f->ilf_size ==2 ); +return 0 ; }/* xlog_print_trans_inode */
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/2 xfs_logprint: Handle continued inode transactions
In reply to Eric Sandeenxlog_print_trans_inode() has a special case for 2 specific op_head->oh_len lengths. If it matches sizeof(xfs_inode_log_format_32_t) or sizeof(xfs_inode_log_format_64_t), it assumes that it's got an inode, and attempts to convert it and print it accordingly.
However, if we arrive here via an op header which is continued, then the length is simply a continuation of the previous op, and it might *randomly* match the size of one of the inode log formats, and thus get parsed incorrectly.
Change the caller to pass in whether or not it's a continued op, so that it can be handled correctly.
Tested by running xfs_logprint of TEST_DEV in xfsprogs after sequential tests; without this change it gets off in the weeds eventually; with this fix, it lasts longer, until it hits some other yet-unfixed logprint bug...
---
logprint/log_misc.c +10 -7
@@ -595,7 +595,7 @@ xlog_print_dir_sf(xfs_dir_shortform_t *sfp, int size) }int -xlog_print_trans_inode (xfs_caddr_t *ptr,int len,int *i,int num_ops) +xlog_print_trans_inode (xfs_caddr_t *ptr,int len,int *i,int num_ops,int continued) {xfs_icdinode_t dino;xlog_op_header_t *op_head;
@@ -617,8 +617,9 @@ xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops)memmove (&src_lbuf, *ptr,MIN (sizeof (xfs_inode_log_format_64_t), len)); (*i)++;/* bump index */ *ptr += len; -if (len ==sizeof (xfs_inode_log_format_32_t) || - len ==sizeof (xfs_inode_log_format_64_t)) { +if (!continued && + (len ==sizeof (xfs_inode_log_format_32_t) || + len ==sizeof (xfs_inode_log_format_64_t))) { f =xfs_inode_item_format_convert ((char *)&src_lbuf, len, &dst_lbuf);printf (_ ("INODE: " )); printf(_("#regs: %d ino: 0x%llx flags: 0x%x dsize: %d\n"),
@@ -932,16 +933,18 @@ xlog_print_record(int fd, ptr = buf;for (i=0 ; i<num_ops; i++) { +int continued; +xlog_op_header_t *op_head = (xlog_op_header_t *)ptr;print_xlog_op_line ();xlog_print_op_header (op_head, i, &ptr); + continued = (XLOG_SET (op_head->oh_flags,XLOG_WAS_CONT_TRANS ) || +XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS ));/* print transaction data */ if (print_no_data || - ((XLOG_SET (op_head->oh_flags,XLOG_WAS_CONT_TRANS ) || -XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) && -be32_to_cpu (op_head->oh_len) ==0 )) { + (continued &&be32_to_cpu (op_head->oh_len) ==0 )) {for (n =0 ; n <be32_to_cpu (op_head->oh_len); n++) {printf ("%c" , *ptr); ptr++;
@@ -970,7 +973,7 @@ xlog_print_record(int fd,case XFS_LI_INODE : { skip =xlog_print_trans_inode (&ptr,be32_to_cpu (op_head->oh_len), - &i, num_ops); + &i, num_ops, continued);break ; }case XFS_LI_DQUOT : {
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 0/2] xfs_logprint: 2 fixes
In reply to Eric SandeenQuoted 3 lines · Eric Sandeen
I've run into a couple bugs running logprint, 2 patches follow to try to fix them up.
-Eric
Sigh, fix subject. xfsdump on the brain, still.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 1/2 xfs_logprint: Handle multiply-logged inode fields
In reply to Eric SandeenAs xlog_print_trans_inode() stands today, it will error out if more than one flag is set on f->ilf_fields:
xlog_print_trans_inode: illegal inode type
but this is a perfectly valid case, to have i.e. a data and an attr flag set.
Following is a pretty big reworking of the function to handle more than one field type set.
I've tested this by a simple test such as creating one file on an selinux box, so that data+attr is set, and logprinting; I've also tested by running logprint after subsequent xfstest runs (although we hit other bugs that way).
---
V2: Fix subject, sigh.
logprint/log_misc.c +63 -76
@@ -657,97 +657,84 @@ xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops)/* does anything come next */ op_head = (xlog_op_header_t *)*ptr; -switch (f->ilf_fields &XFS_ILOG_NONCORE ) { -case XFS_ILOG_DEXT : { -ASSERT (f->ilf_size ==3 ); - (*i)++; -xlog_print_op_header (op_head, *i, ptr); -printf (_ ("EXTENTS inode data\n" )); - *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { -return 1 ; - } -break ; - } -case XFS_ILOG_DBROOT : { -ASSERT (f->ilf_size ==3 ); - (*i)++; -xlog_print_op_header (op_head, *i, ptr); -printf (_ ("BTREE inode data\n" )); - *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { -return 1 ; - } + +if (f->ilf_fields & (XFS_ILOG_DEV |XFS_ILOG_UUID )) { +switch (f->ilf_fields & (XFS_ILOG_DEV |XFS_ILOG_UUID )) { +case XFS_ILOG_DEV : +printf (_ ("DEV inode: no extra region\n" ));break ; - } -case XFS_ILOG_DDATA : { -ASSERT (f->ilf_size ==3 ); - (*i)++; -xlog_print_op_header (op_head, *i, ptr); -printf (_ ("LOCAL inode data\n" )); -if (mode ==S_IFDIR ) { -xlog_print_dir_sf ((xfs_dir_shortform_t *)*ptr, size); - } - *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { -return 1 ; - } +case XFS_ILOG_UUID : +printf (_ ("UUID inode: no extra region\n" ));break ; +case XFS_ILOG_DEXT : +case XFS_ILOG_DBROOT : +case XFS_ILOG_DDATA : +default : +xlog_panic (_ ("xlog_print_trans_inode: illegal inode type 0x%x" ), + f->ilf_fields); } -case XFS_ILOG_AEXT : { -ASSERT (f->ilf_size ==3 ); + } + +if (f->ilf_fields & (XFS_ILOG_DFORK |XFS_ILOG_AFORK )) { +ASSERT (f->ilf_size <=4 ); +ASSERT ((f->ilf_size ==3 ) || (f->ilf_fields &XFS_ILOG_AFORK )); +if (f->ilf_fields &XFS_ILOG_DFORK ) { (*i)++;xlog_print_op_header (op_head, *i, ptr); -printf (_ ("EXTENTS inode attr\n" )); - *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { -return 1 ; + +switch (f->ilf_fields &XFS_ILOG_DFORK ) { +case XFS_ILOG_DEXT : +printf (_ ("EXTENTS inode data\n" )); +break ; +case XFS_ILOG_DBROOT : +printf (_ ("BTREE inode data\n" )); +break ; +case XFS_ILOG_DDATA : +printf (_ ("LOCAL inode data\n" )); +if (mode ==S_IFDIR ) +xlog_print_dir_sf ((xfs_dir_shortform_t *)*ptr, size); +break ; +case XFS_ILOG_DEV : +case XFS_ILOG_UUID : +default : +xlog_panic (_ ("xlog_print_trans_inode: illegal inode type 0x%x" ), + f->ilf_fields); } -break ; - } -case XFS_ILOG_ABROOT : { -ASSERT (f->ilf_size ==3 ); - (*i)++; -xlog_print_op_header (op_head, *i, ptr); -printf (_ ("BTREE inode attr\n" )); + *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { +if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS ))return 1 ; - } -break ; + op_head = (xlog_op_header_t *)*ptr; } -case XFS_ILOG_ADATA : { -ASSERT (f->ilf_size ==3 ); + +if (f->ilf_fields &XFS_ILOG_AFORK ) { (*i)++;xlog_print_op_header (op_head, *i, ptr); -printf (_ ("LOCAL inode attr\n" )); -if (mode ==S_IFDIR ) { -xlog_print_dir_sf ((xfs_dir_shortform_t *)*ptr, size); + +switch (f->ilf_fields &XFS_ILOG_AFORK ) { +case XFS_ILOG_AEXT : +printf (_ ("EXTENTS attr data\n" )); +break ; +case XFS_ILOG_ABROOT : +printf (_ ("BTREE attr data\n" )); +break ; +case XFS_ILOG_ADATA : +printf (_ ("LOCAL attr data\n" )); +if (mode ==S_IFDIR ) +xlog_print_dir_sf ((xfs_dir_shortform_t *)*ptr, size); +break ; +default : +xlog_panic (_ ("xlog_print_trans_inode: illegal inode type 0x%x" ), + f->ilf_fields); } *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { +if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS ))return 1 ; - } -break ; - } -case XFS_ILOG_DEV : { -ASSERT (f->ilf_size ==2 ); -printf (_ ("DEV inode: no extra region\n" )); -break ; - } -case XFS_ILOG_UUID : { -ASSERT (f->ilf_size ==2 ); -printf (_ ("UUID inode: no extra region\n" )); -break ; - } -case 0 : { -ASSERT (f->ilf_size ==2 ); -break ; + op_head = (xlog_op_header_t *)*ptr; } -default : { -xlog_panic (_ ("xlog_print_trans_inode: illegal inode type" )); - } - } + }else /* neither XFS_ILOG_DFORK nor XFS_ILOG_AFORK */ +ASSERT (f->ilf_size ==2 ); +return 0 ; }/* xlog_print_trans_inode */
_______________________________________________
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 1/2 V2] xfs_logprint: Handle multiply-logged inode fields
Quoted 5 lines
I've tested this by a simple test such as creating one file on an selinux box, so that data+attr is set, and logprinting; I've also tested by running logprint after subsequent xfstest runs (although we hit other bugs that way).
Can you add this test to xfstests, please?
logprint/log_misc.c xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops) · line 664
Quoted 5 lines
logprint/log_misc.c +5
-0+ +if (f->ilf_fields & (XFS_ILOG_DEV |XFS_ILOG_UUID )) { +switch (f->ilf_fields & (XFS_ILOG_DEV |XFS_ILOG_UUID )) { +case XFS_ILOG_DEV : +printf (_ ("DEV inode: no extra region\n" ));
The if here looks odd, I think you should follow the style with a switch on a masked value as it's done in xlog_recover_inode_pass2() in the kernel.
I also reall hate the indentation in this function, can you thrown in a preparatory patch to change it to the normal one?
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/2] xfs_logprint: Handle continued inode transactions
In reply to Eric SandeenQuoted 5 lines · Eric Sandeen
Tested by running
xfs_logprintofTEST_DEVin xfsprogs after sequential tests; without this change it gets off in the weeds eventually; with this fix, it lasts longer, until it hits some other yet-unfixed logprint bug...
Add this testcase to xfstests, please.
logprint/log_misc.c xlog_print_dir_sf(xfs_dir_shortform_t *sfp, int size) · line 598
Quoted 3 lines
int
logprint/log_misc.c +1
-1-xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops) +xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops, int continued)
Shouldn't this be a bool?
logprint/log_misc.c xlog_print_record(int fd, · line 943
Quoted 3 lines
logprint/log_misc.c +2
-0xlog_print_op_header (op_head, i, &ptr); + continued = (XLOG_SET (op_head->oh_flags,XLOG_WAS_CONT_TRANS ) || +XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS ));
I'd also really love to see another cleanup patch to kill the XLOG_SET macro. The XLOG_ flags are true flags so they can be replaced by a simple op_head->oh_flags & XLOG_FLAG.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/2] xfs_logprint: Handle continued inode transactions
Quoted 8 lines · Christoph Hellwig
Quoted 5 lines · Eric Sandeen
Tested by running
xfs_logprintofTEST_DEVin xfsprogs after sequential tests; without this change it gets off in the weeds eventually; with this fix, it lasts longer, until it hits some other yet-unfixed logprint bug...Add this testcase to xfstests, please.
well, "this" isn't really a testcase; it's just running logprint after every test. And in some cases, esp. if the log has wrapped, that just fails outright due to unrelated problems.
TBH I'm not sure how to generate a testcase for just this issue. Any ideas?
Quoted 5 lines
logprint/log_misc.c xlog_print_dir_sf(xfs_dir_shortform_t *sfp, int size) · line 598
Quoted 3 lines
int
logprint/log_misc.c +1
-1-xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops) +xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops, int continued)Shouldn't this be a bool?
sure
Quoted 7 lines
logprint/log_misc.c xlog_print_record(int fd, · line 943
Quoted 3 lines
logprint/log_misc.c +2
-0xlog_print_op_header (op_head, i, &ptr); + continued = (XLOG_SET (op_head->oh_flags,XLOG_WAS_CONT_TRANS ) || +XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS ));I'd also really love to see another cleanup patch to kill the
XLOG_SETmacro. TheXLOG_flags are true flags so they can be replaced by a simpleop_head->oh_flags&XLOG_FLAG.
hm, yeah, that is odd.
if (*last_was_partial_copy)
ophdr->oh_flags |= (XLOG_END_TRANS|XLOG_WAS_CONT_TRANS);whereas XLOG_SET tests for one and only one.
-Eric
Quoted 5 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
Re: [PATCH 1/2 V2] xfs_logprint: Handle multiply-logged inode fields
In reply to Christoph HellwigQuoted 7 lines · Christoph Hellwig
Quoted 5 lines
I've tested this by a simple test such as creating one file on an selinux box, so that data+attr is set, and logprinting; I've also tested by running logprint after subsequent xfstest runs (although we hit other bugs that way).
Can you add this test to xfstests, please?
Yeah that should be easy enough.
Quoted 9 lines
logprint/log_misc.c xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops) · line 664
Quoted 5 lines
logprint/log_misc.c +5
-0+ +if (f->ilf_fields & (XFS_ILOG_DEV |XFS_ILOG_UUID )) { +switch (f->ilf_fields & (XFS_ILOG_DEV |XFS_ILOG_UUID )) { +case XFS_ILOG_DEV : +printf (_ ("DEV inode: no extra region\n" ));The if here looks odd, I think you should follow the style with a switch on a masked value as it's done in
xlog_recover_inode_pass2()in the kernel.
Hm TBH I'm not sure why I left that if in there.
For the DFORK/AFORK case I think the if made sense, but not for the DEV/UUID case I think. I'll take another look.
Quoted 2 lines
I also reall hate the indentation in this function, can you thrown in a preparatory patch to change it to the normal one?
to 8-char tabs? Ok
-Eric
Quoted 5 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
Re: [PATCH 1/2 V2] xfs_logprint: Handle multiply-logged inode fields
In reply to a message not in the archiveQuoted 143 lines · Dave Chinner
Quoted 78 lines · Ben Myers
Hey Eric,
Quoted 10 lines · Eric Sandeen
As
xlog_print_trans_inode()stands today, it will error out if more than one flag is set onf->ilf_fields:
xlog_print_trans_inode: illegal inode typebut this is a perfectly valid case, to have i.e. a data and an attr flag set.
Following is a pretty big reworking of the function to handle more than one field type set.
I'm trying to wrap my head around this one. I have a few stupid questions.
Quoted 33 lines
I've tested this by a simple test such as creating one file on an selinux box, so that data+attr is set, and logprinting; I've also tested by running logprint after subsequent xfstest runs (although we hit other bugs that way).
Signed-off-by Eric Sandeen <[email protected]>---
V2: Fix subject, sigh.
logprint/log_misc.c +0
-14@@ -657,97 +657,84 @@ xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops)/* does anything come next */ op_head = (xlog_op_header_t *)*ptr; -switch (f->ilf_fields &XFS_ILOG_NONCORE ) { -case XFS_ILOG_DEXT : { -ASSERT (f->ilf_size ==3 ); - (*i)++; -xlog_print_op_header (op_head, *i, ptr); -printf (_ ("EXTENTS inode data\n" )); - *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { -return 1 ; - } -break ; - } -case XFS_ILOG_DBROOT : { -ASSERT (f->ilf_size ==3 );logprint/log_misc.c xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops) · line 662
Quoted 10 lines
logprint/log_misc.c +3
-7- (*i)++; -xlog_print_op_header (op_head, *i, ptr); -printf (_ ("BTREE inode data\n" )); - *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { -return 1 ; - } + +if (f->ilf_fields & (XFS_ILOG_DEV |XFS_ILOG_UUID )) { +switch (f->ilf_fields & (XFS_ILOG_DEV |XFS_ILOG_UUID )) {Here you kept only
XFS_ILOG_DEVandXFS_ILOG_UUID...Quoted 16 lines
logprint/log_misc.c +2
-13+case XFS_ILOG_DEV : +printf (_ ("DEV inode: no extra region\n" ));break ; - } -case XFS_ILOG_DDATA : { -ASSERT (f->ilf_size ==3 ); - (*i)++; -xlog_print_op_header (op_head, *i, ptr); -printf (_ ("LOCAL inode data\n" )); -if (mode ==S_IFDIR ) { -xlog_print_dir_sf ((xfs_dir_shortform_t *)*ptr, size); - } - *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { -return 1 ; - }Quoted 8 lines
logprint/log_misc.c xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops) · line 671
Quoted 6 lines
logprint/log_misc.c +5
-0+case XFS_ILOG_UUID : +printf (_ ("UUID inode: no extra region\n" ));break ; +case XFS_ILOG_DEXT : +case XFS_ILOG_DBROOT : +case XFS_ILOG_DDATA :Do you need to test for these other flags here?
It's defensive code - ensuring that they never overlap as the fields are mutually exclusive. i.e if
XFS_ILOG_DEVorXFS_ILOG_UUIDis set, the data fork format flags should not be set as the other two fields indicate the data fork contents....Quoted 5 lines
Quoted 5 lines
logprint/log_misc.c +3
-1+default : +xlog_panic (_ ("xlog_print_trans_inode: illegal inode type 0x%x" ), + f->ilf_fields); } -case XFS_ILOG_AEXT : {Quoted 8 lines
logprint/log_misc.c xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops) · line 679
Quoted 5 lines
logprint/log_misc.c +4
-1-ASSERT (f->ilf_size ==3 ); + } + +if (f->ilf_fields & (XFS_ILOG_DFORK |XFS_ILOG_AFORK )) { +ASSERT (f->ilf_size <=4 );Can you explain this ASSERT? I saw only
ilf_size== 3 in the old code. Under what circumstances can it be 4? Maybe when multipleilf_fieldsare set?
XFS_ILOG_CORE|XFS_ILOG_TIMESTAMP|XFS_ILOG_DFORK|XFS_ILOG_AFORKQuoted 4 lines
logprint/log_misc.c xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops) · line 680
Quoted 1 line
logprint/log_misc.c +1
-0+ASSERT ((f->ilf_size ==3 ) || (f->ilf_fields &XFS_ILOG_AFORK ));I also don't understand this ASSERT. It seems like in the old code all of the AFORK related cases had an ASSERT for
ilf_size== 3.It's valid - if the size is not 3, the the attribute for must be logged - it's the only way to get 4 format items.
Quoted 17 lines
Quoted 17 lines
logprint/log_misc.c +10
-4+if (f->ilf_fields &XFS_ILOG_DFORK ) { (*i)++;xlog_print_op_header (op_head, *i, ptr); -printf (_ ("EXTENTS inode attr\n" )); - *ptr +=be32_to_cpu (op_head->oh_len); -if (XLOG_SET (op_head->oh_flags,XLOG_CONTINUE_TRANS )) { -return 1 ; + +switch (f->ilf_fields &XFS_ILOG_DFORK ) { +case XFS_ILOG_DEXT : +printf (_ ("EXTENTS inode data\n" )); +break ; +case XFS_ILOG_DBROOT : +printf (_ ("BTREE inode data\n" )); +break ; +case XFS_ILOG_DDATA :Quoted 8 lines
logprint/log_misc.c xlog_print_trans_inode(xfs_caddr_t *ptr, int len, int *i, int num_ops) · line 698
Quoted 6 lines
logprint/log_misc.c +6
-0+printf (_ ("LOCAL inode data\n" )); +if (mode ==S_IFDIR ) +xlog_print_dir_sf ((xfs_dir_shortform_t *)*ptr, size); +break ; +case XFS_ILOG_DEV : +case XFS_ILOG_UUID :
ILOG_DEVandILOG_UUIDaren't inILOG_DFORK. You needn't test for them, correct?
Quoted 6 lines
See above.
Cheers,
Dave.
Ben, Mark -
Where are we at with this one? We have a partner who is interested in the fix. Do you want anything more from me before it can be merged?
Thanks,
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 1/2 V2] xfs_logprint: Handle multiply-logged inode fields
Quoted 24 lines · Eric Sandeen
Quoted 23 lines · Dave Chinner
Quoted 22 lines · Ben Myers
Hey Eric,
Quoted 10 lines · Eric Sandeen
As
xlog_print_trans_inode()stands today, it will error out if more than one flag is set onf->ilf_fields:
xlog_print_trans_inode: illegal inode typebut this is a perfectly valid case, to have i.e. a data and an attr flag set.
Following is a pretty big reworking of the function to handle more than one field type set.
I'm trying to wrap my head around this one. I have a few stupid questions.
Quoted 6 lines
I've tested this by a simple test such as creating one file on an selinux box, so that data+attr is set, and logprinting; I've also tested by running logprint after subsequent xfstest runs (although we hit other bugs that way).
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Signed-off-by Eric Sandeen <[email protected]>---
Ben, Mark -
Where are we at with this one? We have a partner who is interested in the fix. Do you want anything more from me before it can be merged?
Hey Eric,
I think it's ready to be merged... It looks like you reposted Jan 02 and I replied to the wrong one. If that code isn't significantly different we're good to go.
Thanks,
Ben
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs