[PATCH 0/2] xfsdump: 2 fixes

[PATCH 0/2] xfsdump: 2 fixes

[Eric Sandeen]

I've run into a couple bugs running logprint, 2 patches follow to try to fix them up.

-Eric

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

[PATCH 1/2] xfsdump: Handle multiply-logged inode fields

[Eric Sandeen]

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).

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

diff --git a/logprint/log_misc.c b/logprint/log_misc.c
index e42e108..be2426e 100644
--- a/logprint/log_misc.c
+++ b/logprint/log_misc.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] xfs_logprint: Handle continued inode transactions

[Eric Sandeen]

xlog_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...

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


diff --git a/logprint/log_misc.c b/logprint/log_misc.c
index be2426e..75e221d 100644
--- a/logprint/log_misc.c
+++ b/logprint/log_misc.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/2] xfs_logprint: 2 fixes

[Eric Sandeen]

On 11/1/12 11:22 AM, Eric Sandeen wrote:
> 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/2 V2] xfs_logprint: Handle multiply-logged inode fields

[Eric Sandeen]

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).

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

V2: Fix subject, sigh.

diff --git a/logprint/log_misc.c b/logprint/log_misc.c
index e42e108..be2426e 100644
--- a/logprint/log_misc.c
+++ b/logprint/log_misc.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs


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

Re: [PATCH 1/2 V2] xfs_logprint: Handle multiply-logged inode fields

[Christoph Hellwig]

> 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?

> +
> +    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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_logprint: Handle continued inode transactions

[Christoph Hellwig]

On Thu, Nov 01, 2012 at 11:32:48AM -0500, Eric Sandeen wrote:
> 
> 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...

Add this testcase to xfstests, please.

>  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)

Shouldn't this be a bool?

>  	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));

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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs_logprint: Handle continued inode transactions

[Eric Sandeen]

On 11/2/12 8:04 AM, Christoph Hellwig wrote:
> On Thu, Nov 01, 2012 at 11:32:48AM -0500, Eric Sandeen wrote:
>>
>> 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...
> 
> 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?

>>  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)
> 
> Shouldn't this be a bool?

sure

>>  	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));
> 
> 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.

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

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

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

Re: [PATCH 1/2 V2] xfs_logprint: Handle multiply-logged inode fields

[Eric Sandeen]

On 11/2/12 8:01 AM, Christoph Hellwig wrote:
>> 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.

>> +
>> +    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.

> 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

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

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

Re: [PATCH 1/2 V2] xfs_logprint: Handle multiply-logged inode fields

[Eric Sandeen]

On 1/22/13 3:05 PM, Dave Chinner wrote:
> On Tue, Jan 22, 2013 at 11:55:30AM -0600, Ben Myers wrote:
>> Hey Eric,
>>
>> On Thu, Nov 01, 2012 at 11:33:46AM -0500, Eric Sandeen wrote:
>>> 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'm trying to wrap my head around this one.  I have a few stupid questions.
>>
>>> 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 <sandeen@redhat.com>
>>> ---
>>>
>>> V2: Fix subject, sigh.
>>>
>>> diff --git a/logprint/log_misc.c b/logprint/log_misc.c
>>> index e42e108..be2426e 100644
>>> --- a/logprint/log_misc.c
>>> +++ b/logprint/log_misc.c
>>> @@ -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)) {
>>
>> Here you kept only XFS_ILOG_DEV and 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:
>>
>> 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_DEV or XFS_ILOG_UUID is
> set, the data fork format flags should not be set as the other two
> fields indicate the data fork contents....
> 
>>> +	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);
>>
>> Can you explain this ASSERT?  I saw only ilf_size == 3 in the old code.  Under
>> what circumstances can it be 4?  Maybe when multiple ilf_fields are set?
> 
> XFS_ILOG_CORE | XFS_ILOG_TIMESTAMP | XFS_ILOG_DFORK | XFS_ILOG_AFORK
> 
>>> +	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.
> 
>>
>>> +	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:
>>
>> ILOG_DEV and ILOG_UUID aren't in ILOG_DFORK.  You needn't test for them, correct?
> 
> 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2 V2] xfs_logprint: Handle multiply-logged inode fields

[Ben Myers]

On Mon, Jan 28, 2013 at 02:47:42PM -0600, Eric Sandeen wrote:
> On 1/22/13 3:05 PM, Dave Chinner wrote:
> > On Tue, Jan 22, 2013 at 11:55:30AM -0600, Ben Myers wrote:
> >> Hey Eric,
> >>
> >> On Thu, Nov 01, 2012 at 11:33:46AM -0500, Eric Sandeen wrote:
> >>> 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'm trying to wrap my head around this one.  I have a few stupid questions.
> >>
> >>> 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 <sandeen@redhat.com>
> >>> ---
> 
> 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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs