* [PATCH] xfs: clean up xfs_dir2_leafn_add @ 2019-03-08 16:13 Darrick J. Wong 2019-03-08 18:12 ` Nick Desaulniers 0 siblings, 1 reply; 6+ messages in thread From: Darrick J. Wong @ 2019-03-08 16:13 UTC (permalink / raw) To: linux-xfs Cc: Nathan Chancellor, linux-kernel, Nick Desaulniers, clang-built-linux From: Darrick J. Wong <darrick.wong@oracle.com> Remove typedefs and consolidate local variable initialization. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> --- 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); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: clean up xfs_dir2_leafn_add 2019-03-08 16:13 [PATCH] xfs: clean up xfs_dir2_leafn_add Darrick J. Wong @ 2019-03-08 18:12 ` Nick Desaulniers 2019-03-08 18:38 ` Darrick J. Wong 0 siblings, 1 reply; 6+ messages in thread From: Nick Desaulniers @ 2019-03-08 18:12 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, Nathan Chancellor, LKML, clang-built-linux On Fri, Mar 8, 2019 at 8:13 AM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > Remove typedefs and consolidate local variable initialization. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > 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?) In general, I like this change (I considered asking Nathan to do the consolidation of declaration and initialization as you've done). The additional part of converting from _t to struct * is less interesting (but still ok). It's better to be consistent within the function scope, but this translation unit is still a mismatch of styles. If the goal is to avoid typedefs, consider removing them from existence and the required work to consistently not use them throughout this code. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: clean up xfs_dir2_leafn_add 2019-03-08 18:12 ` Nick Desaulniers @ 2019-03-08 18:38 ` Darrick J. Wong 2019-03-08 18:42 ` Nick Desaulniers 0 siblings, 1 reply; 6+ messages in thread From: Darrick J. Wong @ 2019-03-08 18:38 UTC (permalink / raw) To: Nick Desaulniers; +Cc: linux-xfs, Nathan Chancellor, LKML, clang-built-linux 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@oracle.com> wrote: > > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Remove typedefs and consolidate local variable initialization. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > 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. > In general, I like this change (I considered asking Nathan to do the > consolidation of declaration and initialization as you've done). The > additional part of converting from _t to struct * is less interesting > (but still ok). It's better to be consistent within the function > scope, but this translation unit is still a mismatch of styles. If > the goal is to avoid typedefs, consider removing them from existence > and the required work to consistently not use them throughout this > code. Yeah, we're slowly converting away from the _t typedefs (and other IRIXisms) as parts of code get reworked, though of course there's nothing obvious to suggest that this (slow transition) is in progress nor that we're ok with the inconsistent formatting in the meantime, which is why I accepted the original patch and added this one to do the style conversions. --D > -- > Thanks, > ~Nick Desaulniers ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: clean up xfs_dir2_leafn_add 2019-03-08 18:38 ` Darrick J. Wong @ 2019-03-08 18:42 ` Nick Desaulniers 2019-03-08 23:11 ` Darrick J. Wong 0 siblings, 1 reply; 6+ messages in thread From: Nick Desaulniers @ 2019-03-08 18:42 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, Nathan Chancellor, LKML, clang-built-linux On Fri, Mar 8, 2019 at 10:38 AM Darrick J. Wong <darrick.wong@oracle.com> 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@oracle.com> wrote: > > > > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > Remove typedefs and consolidate local variable initialization. > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > --- > > > 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@google.com> -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: clean up xfs_dir2_leafn_add 2019-03-08 18:42 ` Nick Desaulniers @ 2019-03-08 23:11 ` Darrick J. Wong 2019-03-08 23:31 ` Nathan Chancellor 0 siblings, 1 reply; 6+ messages in thread From: Darrick J. Wong @ 2019-03-08 23:11 UTC (permalink / raw) To: Nick Desaulniers; +Cc: linux-xfs, Nathan Chancellor, LKML, clang-built-linux On Fri, Mar 08, 2019 at 10:42:59AM -0800, Nick Desaulniers wrote: > On Fri, Mar 8, 2019 at 10:38 AM Darrick J. Wong <darrick.wong@oracle.com> 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@oracle.com> wrote: > > > > > > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > > > Remove typedefs and consolidate local variable initialization. > > > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > > --- > > > > 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@google.com> Thanks for the review! By the way, does clang complain about highstale/lowstale in xfs_dir2_leaf_addname being uninitialized too? Just wondering since smatch/sparse do... --D > -- > Thanks, > ~Nick Desaulniers ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: clean up xfs_dir2_leafn_add 2019-03-08 23:11 ` Darrick J. Wong @ 2019-03-08 23:31 ` Nathan Chancellor 0 siblings, 0 replies; 6+ messages in thread From: Nathan Chancellor @ 2019-03-08 23:31 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Nick Desaulniers, linux-xfs, LKML, clang-built-linux On Fri, Mar 08, 2019 at 03:11:14PM -0800, Darrick J. Wong wrote: > On Fri, Mar 08, 2019 at 10:42:59AM -0800, Nick Desaulniers wrote: > > On Fri, Mar 8, 2019 at 10:38 AM Darrick J. Wong <darrick.wong@oracle.com> 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@oracle.com> wrote: > > > > > > > > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > > > > > Remove typedefs and consolidate local variable initialization. > > > > > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > > > --- > > > > > 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@google.com> > > Thanks for the review! > > By the way, does clang complain about highstale/lowstale in > xfs_dir2_leaf_addname being uninitialized too? Just wondering since > smatch/sparse do... > It does not, which is bizarre since it's the exact same pattern. Definitely something to look into... Cheers, Nathan > --D > > > -- > > Thanks, > > ~Nick Desaulniers ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-03-08 23:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-03-08 16:13 [PATCH] xfs: clean up xfs_dir2_leafn_add Darrick J. Wong 2019-03-08 18:12 ` Nick Desaulniers 2019-03-08 18:38 ` Darrick J. Wong 2019-03-08 18:42 ` Nick Desaulniers 2019-03-08 23:11 ` Darrick J. Wong 2019-03-08 23:31 ` Nathan Chancellor
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox