[PATCH] logprint: Fix printing of AGF buffers

[PATCH] logprint: Fix printing of AGF buffers

[Jan Kara]

Currently xfs_logprint doesn't show detailed data about AGF buffers and
instead always shows "Out of space". This is because xfs_agf_t has
additional fields and padding which we never read from disk and thus
buffer length is always smaller than the size of xfs_agf_t.

Fix the problem by only making sure we have enough data in the buffer
to contain all the information we want to print.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 logprint/log_misc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/logprint/log_misc.c b/logprint/log_misc.c
index d482cf3fba57..e7306c08789a 100644
--- a/logprint/log_misc.c
+++ b/logprint/log_misc.c
@@ -367,7 +367,8 @@ xlog_print_trans_buffer(xfs_caddr_t *ptr, int len, int *i, int num_ops)
 	} else if (be32_to_cpu(*(__be32 *)(*ptr)) == XFS_AGF_MAGIC) {
 		agf = (xfs_agf_t *)(*ptr);
 		printf(_("AGF Buffer: XAGF  "));
-		if (be32_to_cpu(head->oh_len) < sizeof(xfs_agf_t)) {
+		/* Make sure buffer contains all the data we want to print */
+		if (be32_to_cpu(head->oh_len) < offsetof(xfs_agf_t, agf_uuid)) {
 			printf(_("Out of space\n"));
 		} else {
 			printf("\n");
-- 
1.8.1.4

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

Re: [PATCH] logprint: Fix printing of AGF buffers

[Christoph Hellwig]

On Mon, Jul 14, 2014 at 04:45:00PM +0200, Jan Kara wrote:
> Currently xfs_logprint doesn't show detailed data about AGF buffers and
> instead always shows "Out of space". This is because xfs_agf_t has
> additional fields and padding which we never read from disk and thus
> buffer length is always smaller than the size of xfs_agf_t.
> 
> Fix the problem by only making sure we have enough data in the buffer
> to contain all the information we want to print.

The fix looks correct to me, but te explanation should be more verbose:
the reason why we don't read agf_uuid and above is because you're
probably dumping a v4 filesystems which doesn't even have those fields.

Its seems like various other fields have the same issue in logprint,
and I also suspect we want some defines for the v4 size instead of using
the offsetoff tricks.

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

Re: [PATCH] logprint: Fix printing of AGF buffers

[Jan Kara]

On Tue 15-07-14 03:19:31, Christoph Hellwig wrote:
> On Mon, Jul 14, 2014 at 04:45:00PM +0200, Jan Kara wrote:
> > Currently xfs_logprint doesn't show detailed data about AGF buffers and
> > instead always shows "Out of space". This is because xfs_agf_t has
> > additional fields and padding which we never read from disk and thus
> > buffer length is always smaller than the size of xfs_agf_t.
> > 
> > Fix the problem by only making sure we have enough data in the buffer
> > to contain all the information we want to print.
> 
> The fix looks correct to me, but te explanation should be more verbose:
> the reason why we don't read agf_uuid and above is because you're
> probably dumping a v4 filesystems which doesn't even have those fields.
  I'm actually dumping v5 filesystem. The issue is that with v5 filesystem
not all fields are logged (e.g. CRC isn't) and thus the length of logged
buffer is shorter than the length of in-memory structure.

> Its seems like various other fields have the same issue in logprint,
> and I also suspect we want some defines for the v4 size instead of using
> the offsetoff tricks.
  I had a look before I submitted this patch and I didn't find anything.
Now that I'm looking again, AGI buffers probably need a similar treatment.
Superblock buffers are already checked against fixed number so those don't
have a problem. Dquot buffers should be fine as well because those don't
have a checksum and other unlogged stuff. And I didn't find any other
structures in the log that would have the problem (please point me if I
missed something).

Regarding how to fix this cleanly - offsetof() seems like a reasonably
clean way to me. If you prefer to define number of bytes each buffer type
has to have in the log, I can do that as well. Or I could define
alternative structures only containing fields we need in the log so that we
can print info but this all seems like an additional complexity with
disputable gain...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

Re: [PATCH] logprint: Fix printing of AGF buffers

[Christoph Hellwig]

On Tue, Jul 15, 2014 at 04:09:38PM +0200, Jan Kara wrote:
>   I had a look before I submitted this patch and I didn't find anything.
> Now that I'm looking again, AGI buffers probably need a similar treatment.
> Superblock buffers are already checked against fixed number so those don't
> have a problem. Dquot buffers should be fine as well because those don't
> have a checksum and other unlogged stuff. And I didn't find any other
> structures in the log that would have the problem (please point me if I
> missed something).
> 
> Regarding how to fix this cleanly - offsetof() seems like a reasonably
> clean way to me. If you prefer to define number of bytes each buffer type
> has to have in the log, I can do that as well. Or I could define
> alternative structures only containing fields we need in the log so that we
> can print info but this all seems like an additional complexity with
> disputable gain...

I've taken a bit of a closer log (OMG, what a mess logprint is..), and
it seems at least in this are the AGF and AGI are affected of the struct
growth in v4.

It seems like in this specific case using your offsetoff trick is
fine, it just needs a good comment explaining it.

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

Re: [PATCH] logprint: Fix printing of AGF buffers

[Dave Chinner]

On Tue, Jul 15, 2014 at 08:39:22AM -0700, Christoph Hellwig wrote:
> On Tue, Jul 15, 2014 at 04:09:38PM +0200, Jan Kara wrote:
> >   I had a look before I submitted this patch and I didn't find anything.
> > Now that I'm looking again, AGI buffers probably need a similar treatment.
> > Superblock buffers are already checked against fixed number so those don't
> > have a problem. Dquot buffers should be fine as well because those don't
> > have a checksum and other unlogged stuff. And I didn't find any other
> > structures in the log that would have the problem (please point me if I
> > missed something).
> > 
> > Regarding how to fix this cleanly - offsetof() seems like a reasonably
> > clean way to me. If you prefer to define number of bytes each buffer type
> > has to have in the log, I can do that as well. Or I could define
> > alternative structures only containing fields we need in the log so that we
> > can print info but this all seems like an additional complexity with
> > disputable gain...
> 
> I've taken a bit of a closer log (OMG, what a mess logprint is..), and
> it seems at least in this are the AGF and AGI are affected of the struct
> growth in v4.

Yes, logprint is a steaming pile. At some point I'll properly
abstract the kernel side log recovery code and share that with
userspace and then convert logprint to use that....

> It seems like in this specific case using your offsetoff trick is
> fine, it just needs a good comment explaining it.

I added this:

	/*
	 * The addition of spare space and the non-logged CRC format
	 * fields to the AGF mean that the size that is logged is almost
	 * always going to be smaller than the structure itself. Hence
	 * we need to make sure that the buffer contains all the data we
	 * want to print rather than just check against the structure
	 * size.
	 */

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] logprint: Fix printing of AGF buffers

[Christoph Hellwig]

On Wed, Jul 16, 2014 at 10:38:51AM +1000, Dave Chinner wrote:
> I added this:
> 
> 	/*
> 	 * The addition of spare space and the non-logged CRC format
> 	 * fields to the AGF mean that the size that is logged is almost
> 	 * always going to be smaller than the structure itself. Hence
> 	 * we need to make sure that the buffer contains all the data we
> 	 * want to print rather than just check against the structure
> 	 * size.
> 	 */
> 
> Cheers,

I'd prefer to mention v4 filesystems as well:

	/*
	 * v4 filesystems only contain the fields before the uuid, and
	 * even v5 filesystems don't usually log any field beneath it.
	 */

note that the AGI case also needs the same treatment.

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

Re: [PATCH] logprint: Fix printing of AGF buffers

[Jan Kara]

On Wed 16-07-14 01:11:05, Christoph Hellwig wrote:
> On Wed, Jul 16, 2014 at 10:38:51AM +1000, Dave Chinner wrote:
> > I added this:
> > 
> > 	/*
> > 	 * The addition of spare space and the non-logged CRC format
> > 	 * fields to the AGF mean that the size that is logged is almost
> > 	 * always going to be smaller than the structure itself. Hence
> > 	 * we need to make sure that the buffer contains all the data we
> > 	 * want to print rather than just check against the structure
> > 	 * size.
> > 	 */
> > 
> > Cheers,
> 
> I'd prefer to mention v4 filesystems as well:
> 
> 	/*
> 	 * v4 filesystems only contain the fields before the uuid, and
> 	 * even v5 filesystems don't usually log any field beneath it.
> 	 */
> 
> note that the AGI case also needs the same treatment.
  Yep, I'll send an updated patch in a moment.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

Re: [PATCH] logprint: Fix printing of AGF buffers

[Dave Chinner]

On Wed, Jul 16, 2014 at 01:11:05AM -0700, Christoph Hellwig wrote:
> On Wed, Jul 16, 2014 at 10:38:51AM +1000, Dave Chinner wrote:
> > I added this:
> > 
> > 	/*
> > 	 * The addition of spare space and the non-logged CRC format
> > 	 * fields to the AGF mean that the size that is logged is almost
> > 	 * always going to be smaller than the structure itself. Hence
> > 	 * we need to make sure that the buffer contains all the data we
> > 	 * want to print rather than just check against the structure
> > 	 * size.
> > 	 */
> > 
> > Cheers,
> 
> I'd prefer to mention v4 filesystems as well:
> 
> 	/*
> 	 * v4 filesystems only contain the fields before the uuid, and
> 	 * even v5 filesystems don't usually log any field beneath it.
> 	 */

It actually has nothing to do with v4 or v5 filesystems - it's to do
with the fact that we do partial buffer logging but logprint is
assuming the structure is always logging as a whole.  There's
mistakes like this all through logprint - we whack them like this
with a big stick (i.e. refuse to parse the structure) when they are
found.

Did you notice the way that log_print_all.c handled these issues?
For the AGI, it simply looks at the length of the region and sizes
the output accordingly. And for the AGF, it just ignores the size of
the region and assumes that it captured everything that is to be
printed.

IOWs, we've played whack-a-mole on this again while ignoring the
fundamental issues ithat still remain:

	- that logprint has a lot of assumptions that simply aren't
	  true; and
	- that logprint simply does not handle split region
	  continuations like the kernel recovery code does.

Both of these things lead to having to handle these strange "out of
space" cases in multiple places, and simply not handling them in
places that actually need to.

These are just more reasons why logprint - as it says itself in a
couple of comments - needs a complete rewrite.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] logprint: Fix printing of AGF buffers

[Jan Kara]

On Thu 17-07-14 09:17:25, Dave Chinner wrote:
> On Wed, Jul 16, 2014 at 01:11:05AM -0700, Christoph Hellwig wrote:
> > On Wed, Jul 16, 2014 at 10:38:51AM +1000, Dave Chinner wrote:
> > > I added this:
> > > 
> > > 	/*
> > > 	 * The addition of spare space and the non-logged CRC format
> > > 	 * fields to the AGF mean that the size that is logged is almost
> > > 	 * always going to be smaller than the structure itself. Hence
> > > 	 * we need to make sure that the buffer contains all the data we
> > > 	 * want to print rather than just check against the structure
> > > 	 * size.
> > > 	 */
> > > 
> > > Cheers,
> > 
> > I'd prefer to mention v4 filesystems as well:
> > 
> > 	/*
> > 	 * v4 filesystems only contain the fields before the uuid, and
> > 	 * even v5 filesystems don't usually log any field beneath it.
> > 	 */
> 
> It actually has nothing to do with v4 or v5 filesystems - it's to do
> with the fact that we do partial buffer logging but logprint is
> assuming the structure is always logging as a whole.  There's
> mistakes like this all through logprint - we whack them like this
> with a big stick (i.e. refuse to parse the structure) when they are
> found.
> 
> Did you notice the way that log_print_all.c handled these issues?
> For the AGI, it simply looks at the length of the region and sizes
> the output accordingly. And for the AGF, it just ignores the size of
> the region and assumes that it captured everything that is to be
> printed.
> 
> IOWs, we've played whack-a-mole on this again while ignoring the
> fundamental issues ithat still remain:
> 
> 	- that logprint has a lot of assumptions that simply aren't
> 	  true; and
> 	- that logprint simply does not handle split region
> 	  continuations like the kernel recovery code does.
> 
> Both of these things lead to having to handle these strange "out of
> space" cases in multiple places, and simply not handling them in
> places that actually need to.
> 
> These are just more reasons why logprint - as it says itself in a
> couple of comments - needs a complete rewrite.
  Yeah, I've noticed that logprint definitely doesn't handle all possible
cases and the code is ... well, organic :). OTOH it worked for me (except
for this bug) when I needed so for a debug tool like that it seemed good
enough. I don't feel like rewriting it from scratch (at least in near
future) but if someone can find time for that, it would be surely welcome.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

Re: [PATCH] logprint: Fix printing of AGF buffers

[Dave Chinner]

On Thu, Jul 17, 2014 at 10:39:05AM +0200, Jan Kara wrote:
> On Thu 17-07-14 09:17:25, Dave Chinner wrote:
> > IOWs, we've played whack-a-mole on this again while ignoring the
> > fundamental issues ithat still remain:
> > 
> > 	- that logprint has a lot of assumptions that simply aren't
> > 	  true; and
> > 	- that logprint simply does not handle split region
> > 	  continuations like the kernel recovery code does.
> > 
> > Both of these things lead to having to handle these strange "out of
> > space" cases in multiple places, and simply not handling them in
> > places that actually need to.
> > 
> > These are just more reasons why logprint - as it says itself in a
> > couple of comments - needs a complete rewrite.
>   Yeah, I've noticed that logprint definitely doesn't handle all possible
> cases and the code is ... well, organic :).

That's a very polite way of putting it ;)

> OTOH it worked for me (except
> for this bug) when I needed so for a debug tool like that it seemed good
> enough. I don't feel like rewriting it from scratch (at least in near
> future) but if someone can find time for that, it would be surely welcome.

Right, that's effectively been the state of play for some time. It's
a useful tool that could do with some work to make it better, but
nobody has had the time to do that work because most of the time it
works fine...

If only there were some minions around looking to learn about XFS
log recovery....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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