Re: Possible small bug in xfsprogs-dev/db/metadump.c

Re: Possible small bug in xfsprogs-dev/db/metadump.c

[Christoph Hellwig]

[Cc'ed to the list, where people including the most active person on the
 userspace side hang out]

On Sun, Sep 27, 2009 at 12:20:33PM -0700, Richard Sharpe wrote:
> Hi folks,
> 
> There seems to be a small bug in
> xfsprogs-dev/db/metadump.c:scanfunc_freesp (although I think the same
> problem exists in other functions).
> 
> It has a check to see if the number of records is invalid:
> 
>         numrecs = be16_to_cpu(block->bb_numrecs);
>         if (numrecs > mp->m_alloc_mxr[1]) {
>                 if (show_warnings)
>                         print_warning("invalid numrecs (%u) in %s block %u/%u",
>                                 numrecs, typtab[btype].name, agno, agbno);
>                 return 1;
>         }
> 
> However, it seems to me that you should pay attention to bb_level in
> the node when using that test, because leaf nodes can appear at
> multiple levels in the tree.

Before that code there is a

	if (level == 0)
		return 1;

which should take care of the leaf nodes by exiting early.

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

Re: Possible small bug in xfsprogs-dev/db/metadump.c

[Richard Sharpe]

On Mon, Sep 28, 2009 at 10:21 AM, Christoph Hellwig <hch@infradead.org> wrote:
> [Cc'ed to the list, where people including the most active person on the
>  userspace side hang out]
>
> On Sun, Sep 27, 2009 at 12:20:33PM -0700, Richard Sharpe wrote:
>> Hi folks,
>>
>> There seems to be a small bug in
>> xfsprogs-dev/db/metadump.c:scanfunc_freesp (although I think the same
>> problem exists in other functions).
>>
>> It has a check to see if the number of records is invalid:
>>
>>         numrecs = be16_to_cpu(block->bb_numrecs);
>>         if (numrecs > mp->m_alloc_mxr[1]) {
>>                 if (show_warnings)
>>                         print_warning("invalid numrecs (%u) in %s block %u/%u",
>>                                 numrecs, typtab[btype].name, agno, agbno);
>>                 return 1;
>>         }
>>
>> However, it seems to me that you should pay attention to bb_level in
>> the node when using that test, because leaf nodes can appear at
>> multiple levels in the tree.
>
> Before that code there is a
>
>        if (level == 0)
>                return 1;
>
> which should take care of the leaf nodes by exiting early.

Well, yes there is, but that is the problem I encountered. It is level
as passed in when starting at the top of the tree, which is obtained
from the levels value in the AGF, and is decremented by one on each
recursion:

        if (!(*func)(iocur_top->data, agno, agbno, level - 1, btype, arg))

However, what should really be looked at is the value bb_level in the
header in each free-space Btree node.

After I made that change to my changes, I started being able to
properly count all leaf nodes and free extents, and the numbers came
out where I expected them to be (instead of not seeing many leaf nodes
and vastly undercounting free extents).





-- 
Regards,
Richard Sharpe

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

Re: Possible small bug in xfsprogs-dev/db/metadump.c

[Christoph Hellwig]

On Mon, Sep 28, 2009 at 10:36:13AM -0700, Richard Sharpe wrote:
> >
> > ? ? ? ?if (level == 0)
> > ? ? ? ? ? ? ? ?return 1;
> >
> > which should take care of the leaf nodes by exiting early.
> 
> Well, yes there is, but that is the problem I encountered. It is level
> as passed in when starting at the top of the tree, which is obtained
> from the levels value in the AGF, and is decremented by one on each
> recursion:
> 
>         if (!(*func)(iocur_top->data, agno, agbno, level - 1, btype, arg))
> 
> However, what should really be looked at is the value bb_level in the
> header in each free-space Btree node.

I think bother are equally valid.  The one passed in fro mthe top should
always be right while the ondisk one might be corrupted.

> After I made that change to my changes, I started being able to
> properly count all leaf nodes and free extents, and the numbers came
> out where I expected them to be (instead of not seeing many leaf nodes
> and vastly undercounting free extents).

Still not quite understanding your problem here.  Do you have a tool
of your own that doesn't start from the btree root but only walks
subtrees?  In that case you need to look at the on-disk level unless
you can find out easily what level you start at.  But I don't think we
need this for the existing tools that always walk from the root.

> 
> 
> 
> 
> 
> -- 
> Regards,
> Richard Sharpe
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---

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

Re: Possible small bug in xfsprogs-dev/db/metadump.c

[Richard Sharpe]

On Tue, Sep 29, 2009 at 6:00 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Sep 28, 2009 at 10:36:13AM -0700, Richard Sharpe wrote:
>> Well, yes there is, but that is the problem I encountered. It is level
>> as passed in when starting at the top of the tree, which is obtained
>> from the levels value in the AGF, and is decremented by one on each
>> recursion:
>>
>>         if (!(*func)(iocur_top->data, agno, agbno, level - 1, btype, arg))
>>
>> However, what should really be looked at is the value bb_level in the
>> header in each free-space Btree node.
>
> I think bother are equally valid.  The one passed in fro mthe top should
> always be right while the ondisk one might be corrupted.

Well, yes, but then anything could be corrupted on disk, including the
superblock and the pointers in interior nodes in the free space Btree.

>> After I made that change to my changes, I started being able to
>> properly count all leaf nodes and free extents, and the numbers came
>> out where I expected them to be (instead of not seeing many leaf nodes
>> and vastly undercounting free extents).
>
> Still not quite understanding your problem here.  Do you have a tool
> of your own that doesn't start from the btree root but only walks
> subtrees?  In that case you need to look at the on-disk level unless
> you can find out easily what level you start at.  But I don't think we
> need this for the existing tools that always walk from the root.

No, I was walking down from the root of the tree.

However, now I understand what is going on.

Assume a free space tree with levels = 3 (from the AGF). However, not
all leaf nodes will be at depth 3 in the tree, some will be at depth 2
in the tree.

However, in scanfunc_freesp we see:

        if (level == 0)
                return 1;

        numrecs = be16_to_cpu(block->bb_numrecs);
        if (numrecs > mp->m_alloc_mxr[1]) {
                if (show_warnings)
                        print_warning("invalid numrecs (%u) in %s block %u/%u",
                                numrecs, typtab[btype].name, agno, agbno);
                return 1;
        }

If we hit level == 0 then we exit, however, for a leaf node that is at
depth 2 (level 1, but bb_level == 0) we will see numrecs >
mp->m_alloc_mxr[1] and will also skip such records (ie, will not
recurse into them).

However, if the user does "metadump -w" they will see warnings that
are bogus and suggests that the author was not really aware of the
real structure of the tree.

I do think it is a minor issue since it will not lead to dropping any
records as I first feared.

Since I wanted to count the number of free extents in the free-space
trees I needed to actually see all leaf nodes, and thus it became a
problem for me.

-- 
Regards,
Richard Sharpe

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

Re: Possible small bug in xfsprogs-dev/db/metadump.c

[Dave Chinner]

On Tue, Sep 29, 2009 at 08:45:56AM -0700, Richard Sharpe wrote:
> However, now I understand what is going on.
> 
> Assume a free space tree with levels = 3 (from the AGF). However, not
> all leaf nodes will be at depth 3 in the tree, some will be at depth 2
> in the tree.

No, that is not possible. By definition, a consistent filesystem
image has all leaf nodes at level zero.

> However, in scanfunc_freesp we see:
> 
>         if (level == 0)
>                 return 1;
> 
>         numrecs = be16_to_cpu(block->bb_numrecs);
>         if (numrecs > mp->m_alloc_mxr[1]) {
>                 if (show_warnings)
>                         print_warning("invalid numrecs (%u) in %s block %u/%u",
>                                 numrecs, typtab[btype].name, agno, agbno);
>                 return 1;
>         }
> 
> If we hit level == 0 then we exit, however, for a leaf node that is at
> depth 2 (level 1, but bb_level == 0) we will see numrecs >
> mp->m_alloc_mxr[1] and will also skip such records (ie, will not
> recurse into them).

This sounds to me like the log has not been replayed on this
filesystem. AFAICT, when looking at a raw disk image of an XFS
filesystem, the only way to get leaf nodes at non-zero levels is to
have a dirty log. i.e. the log contains allocation/free transactions
that have resulted in a multi-level rebalance of the tree
that have not been replayed and written to disk and hence on-disk
image of the tree is unbalanced. When the log is replayed, the
on disk image will get updated and the tree will appear balanced
with all leaves at level 0.

> However, if the user does "metadump -w" they will see warnings that
> are bogus and suggests that the author was not really aware of the
> real structure of the tree.

I think he was aware of the structure. ;)

It seems to me that you are trying to use the wrong tool to walk
free space trees and interpret the number of extents.  xfs_metadump
is intended to capture the exact layout of the filesystem metadata
so that it can be reproduced exactly in a different environment. It
was not intended as a method of interpreting the potentially
inconsistent metadata that it records.

xfs_db does what you are trying to do. It already has commands that
walk the per AG free space trees and tells you the number of
extents, gives an extent size histogram, etc....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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