Re: [PATCH] xfs: clean up xfs_dir2_leafn_add

[Nick Desaulniers]

On Fri, Mar 8, 2019 at 10:38 AM Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote:
>
> On Fri, Mar 08, 2019 at 10:12:33AM -0800, Nick Desaulniers wrote:
> > On Fri, Mar 8, 2019 at 8:13 AM Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote:
> > >
> > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > >
> > > Remove typedefs and consolidate local variable initialization.
> > >
> > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > > ---
> > >  fs/xfs/libxfs/xfs_dir2_node.c |   20 ++++++++------------
> > >  1 file changed, 8 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> > > index de46f26c5292..16731d2d684b 100644
> > > --- a/fs/xfs/libxfs/xfs_dir2_node.c
> > > +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> > > @@ -426,26 +426,22 @@ xfs_dir2_leaf_to_node(
> > >  static int                                     /* error */
> > >  xfs_dir2_leafn_add(
> > >         struct xfs_buf          *bp,            /* leaf buffer */
> > > -       xfs_da_args_t           *args,          /* operation arguments */
> > > +       struct xfs_da_args      *args,          /* operation arguments */
> > >         int                     index)          /* insertion pt for new entry */
> > >  {
> > > +       struct xfs_dir3_icleaf_hdr leafhdr;
> > > +       struct xfs_inode        *dp = args->dp;
> > > +       struct xfs_dir2_leaf    *leaf = bp->b_addr;
> > > +       struct xfs_dir2_leaf_entry *lep;
> > > +       struct xfs_dir2_leaf_entry *ents;
> > >         int                     compact;        /* compacting stale leaves */
> > > -       xfs_inode_t             *dp;            /* incore directory inode */
> > > -       int                     highstale;      /* next stale entry */
> > > -       xfs_dir2_leaf_t         *leaf;          /* leaf structure */
> > > -       xfs_dir2_leaf_entry_t   *lep;           /* leaf entry */
> > > +       int                     highstale = 0;  /* next stale entry */
> > >         int                     lfloghigh;      /* high leaf entry logging */
> > >         int                     lfloglow;       /* low leaf entry logging */
> > > -       int                     lowstale;       /* previous stale entry */
> > > -       struct xfs_dir3_icleaf_hdr leafhdr;
> > > -       struct xfs_dir2_leaf_entry *ents;
> > > +       int                     lowstale = 0;   /* previous stale entry */
> > >
> > >         trace_xfs_dir2_leafn_add(args, index);
> > >
> > > -       dp = args->dp;
> > > -       leaf = bp->b_addr;
> > > -       highstale = 0;
> > > -       lowstale = 0;
> > >         dp->d_ops->leaf_hdr_from_disk(&leafhdr, leaf);
> > >         ents = dp->d_ops->leaf_ents_p(leaf);
> >
> > What about moving the initialization of `ents` above? (Or would that
> > be over the line limit?)
>
> It might be over the line limit, but more importantly I prefer to have
> the tracepoint fire before we start interpreting the on-disk metadata.
> That way, ftrace data will show exactly where we were in the kernel
> if any corruption warnings are emitted during that interpretation.
>
> I don't think either of those two functions do that today, but I don't
> want to leave a logic bomb in case they ever start doing that.

Makes sense, ents is initialized to the results of function call.
Thanks for the additional info, for accepting the earlier patch, and
this additional cleanup.
Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>

-- 
Thanks,
~Nick Desaulniers