[Ocfs2-devel] [PATCH 3/3] du-enhancement: show the shared extents per file and the footprint v2

[Tao Ma]

Hi jeff,

jeff.liu wrote:
> Hi Tao,
> 
> Thanks for your comments.
> 
> Tao Ma wrote:
>> Jie Liu wrote:
>>> this patch add fiemap feature support in du, du show the shared
>>> extents size in parens per file
>>> as well as the footprint for each request with either '--shared-size'
>>> or '-E' option.
>>>
>>> the footprint which is total minus the sum total of all extents in the
>>> rbtree
>>> that have ei_shared_count > 0.
>>>
>>> Signed-off-by: Jie Liu <jeff.liu at oracle.com>
>>> ---
>>>  coreutils-6.9/src/du.c |  444
>>> +++++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 files changed, 437 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/coreutils-6.9/src/du.c b/coreutils-6.9/src/du.c
>>> index 206d318..3adc3a8 100644
>>> --- a/coreutils-6.9/src/du.c
>>> +++ b/coreutils-6.9/src/du.c
>>> +/* Insert new extent based on the new parent.  */
>>> +
>>> +static void
>>> +insert_new_extent_info (struct rb_node *parent,
>>> +                        uint64_t fe_physical,
>>> +                        uint64_t fe_length,
>>> +                        size_t extent_shared_count)
>>> +{
>>> +  struct rb_node **p = parent ? &parent : &((&fe_root)->rb_node);
>>> +  struct rb_node *pp = NULL;
>>> +  struct extent_info *this = NULL;
>>> +  struct extent_info *ei;
>>> +
>>> +  while (*p)
>>> +    {
>>> +      pp = *p;
>>> +      this = rb_entry (*p, struct extent_info, ei_node);
>>> +
>>> +      if (this->ei_physical > fe_physical)
>>> +        p = &(*p)->rb_left;
>>> +      else if (this->ei_physical < fe_physical)
>>> +    p = &(*p)->rb_right;
>>> +      else
>>> +    return;
>>>   
>> I am just curious in your code the last "else" should never happen? If
>> yes, maybe if we meet with this, it means a bug in the code.
>> So better assert here?
> IMHO, the last 'else' should not happen if the split algorithm works correctly.
> The current algorithm is designed to make sure there is no extent overlap occurrs in the rbtree.
> So I am prefer to abort the DU process by asserting there.
as you said, it is *designed* to work well, but you can't say it works 
well. With a "assert", you know there is a problem. Without assert, you 
may cheated by the codes. ;) btw, you can have a look in ocfs2-tools. 
there are some codes like it with "assert".
>>> +    /* If 0 extents are returned, then more ioctls
>>> +       are not needed.  */
>>> +    if (fiemap->fm_mapped_extents == 0)
>>> +      goto close_file;
>>> +
>>> +    uint64_t ext_phy_offset;
>>> +    uint64_t ext_len;
>>> +    size_t i;
>>>   
>> better move these declaration above.
> Do you means this is a bad coding style?
> 
> Due to these variables resides function exceeds 1 screen page, so I declares them here to
> facilitate the variables reference like type and name. or else, someone have to page up to refer to
> them.
> I'd like to keep the ext_phy_offset and ext_len in the bottom FOR logical, but move 'i' to the
> head of this function since it is well-known as an iterate counter.
> 
> How do you think about that?
> I just want to make the code looks more professional.
> 
>>> +    for (i = 0; i < fiemap->fm_mapped_extents; i++)
>>> +      {
>             uint64_t ext_phy_offset;
>             uint64_t ext_len;
yeah, this looks better and you can combine the 4 lines into 2 actually. ;)
		uint64_t ext_phy_offset = fm_ext[i].fe_physical;
		uint64_t ext_len = fm_ext[i].fe_length;
>>> +        ext_phy_offset = fm_ext[i].fe_physical;
>>> +    ext_len = fm_ext[i].fe_length;

Regards,
Tao