xfs
[Top] [All Lists]

Re: [PATCH 4/6] xfs: fix remote attribute invalidation for a leaf

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH 4/6] xfs: fix remote attribute invalidation for a leaf
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 4 Jun 2013 13:13:40 +1000
Cc: xfs@xxxxxxxxxxx, bpm@xxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <51ACE9FC.9010008@xxxxxxx>
References: <1370237332-24757-1-git-send-email-david@xxxxxxxxxxxxx> <1370237332-24757-5-git-send-email-david@xxxxxxxxxxxxx> <51ACE9FC.9010008@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Jun 03, 2013 at 02:09:48PM -0500, Mark Tinguely wrote:
> On 06/03/13 00:28, Dave Chinner wrote:
> >From: Dave Chinner<dchinner@xxxxxxxxxx>
> >
> >When invalidating an attribute leaf block block, there might be
> >remote attributes that it points to. With the recent rework of the
> >remote attribute format, we have to make sure we calculate the
> >length of the attribute correctly. We aren't doing that in
> >xfs_attr3_leaf_inactive(), so fix it.
> >
> >Signed-off-by: Dave Chinner<dchinner@xxxxxxxxxx>
> 
> I scratched my head reading:
> 
> in xfs_attr_leaf.h:
> /*
>  * Used to keep a list of "remote value" extents when unlinking an inode.
>  */
> typedef struct xfs_attr_inactive_list {
>       xfs_dablk_t     valueblk;       /* block number of value bytes */
>       int             valuelen;       /* number of bytes in value */
>                                                    ^^^^^
>                                                    |||||
> } xfs_attr_inactive_list_t;
> 
> Where "valuelen" is clearly being used as blocks.

Yeah, good point.

This is one of the reasons why I dislike comments explaining what
variables in structures mean. I didn't even look at the definition
of the structure, because it's meaning is obvious from the name of
the varaible of the code that uses it.  ;)

> A more obvious name is
> the former "valueblk". Blame commit d7929ff6 for the confusion.
> Should change
> the comment and/or variable one of these days ...

Actaully, a structure that is used once and local to a single
function shouldn't be declared in a header file - if should be local
to the function. I'll fix this in a separate patch for 3.11.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>