xfs
[Top] [All Lists]

Re: [PATCH 13/25] xfs: factor dir2 block read operations

To: Phil White <pwhite@xxxxxxx>
Subject: Re: [PATCH 13/25] xfs: factor dir2 block read operations
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 31 Oct 2012 09:16:11 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20121030032309.GP30227@xxxxxxxxxxxxxxxxxxxx>
References: <1351146854-19343-1-git-send-email-david@xxxxxxxxxxxxx> <1351146854-19343-14-git-send-email-david@xxxxxxxxxxxxx> <20121030032309.GP30227@xxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Oct 29, 2012 at 08:23:09PM -0700, Phil White wrote:
> On Thu, Oct 25, 2012 at 05:34:02PM +1100, Dave Chinner wrote:
> > +static void
> > +xfs_dir2_block_need_space(
> > ...
> > +   /*
> > +    * If there are stale entries we'll use one for the leaf.
> > +    */
> > +   if (btp->stale) {
> > +           if (be16_to_cpu(bf[0].length) >= len) {
> > +                   /*
> > +                    * The biggest entry enough to avoid compaction.
> > +                    */
> > +                   dup = (xfs_dir2_data_unused_t *)
> > +                         ((char *)hdr + be16_to_cpu(bf[0].offset));
> > +                   goto out;
> > +           }
> > +
> > +           /*
> > +            * Will need to compact to make this work.
> > +            * Tag just before the first leaf entry.
> > +            */
> > +           *compact = 1;
> > +           tagp = (__be16 *)blp - 1;
> > +
> > +           /* Data object just before the first leaf entry.  */
> > +           dup = (xfs_dir2_data_unused_t *)((char *)hdr + 
> > be16_to_cpu(*tagp));
> > +
> > +           /*
> > +            * If it's not free then the data will go where the
> > +            * leaf data starts now, if it works at all.
> > +            */
> > +           if (be16_to_cpu(dup->freetag) == XFS_DIR2_DATA_FREE_TAG) {
> > +                   if (be16_to_cpu(dup->length) + (be32_to_cpu(btp->stale) 
> > - 1) *
> > +                       (uint)sizeof(*blp) < len)
> > +                           dup = NULL;
> > +           } else if ((be32_to_cpu(btp->stale) - 1) * (uint)sizeof(*blp) < 
> > len)
> > +                   dup = NULL;
> > +           else
> > +                   dup = (xfs_dir2_data_unused_t *)blp;
> > +           goto out;
> > +   }
> > +
> > +   /*
> > +    * no stale entries, so just use free space.
> > +    * Tag just before the first leaf entry.
> > +    */
> > +   tagp = (__be16 *)blp - 1;
> 
> Shouldn't tagp just be set before this if statement rather than inside of it
> and outside of it?

No, because there is a case where it isn't set.

In general, when factoring code it's not a good idea to change logic
because that's where bugs most commonly creep in. At some point in
the future this could probably do with a more robust cleanup (rather
than just factoring), but right now that's out-of-scope for what I'm
doing...

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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