Re: [PATCH v7 16/19] xfs: Simplify xfs_attr_set_iter

Chandan Rajendra
2020-03-05T03:36:41Z

On Wednesday, March 4, 2020 10:34 PM Allison Collins wrote:
>
> On 3/3/20 9:30 PM, Chandan Rajendra wrote:
> > On Sunday, February 23, 2020 7:36 AM Allison Collins wrote:
> >> Delayed attribute mechanics make frequent use of goto statements. We can use this
> >> to further simplify xfs_attr_set_iter. Because states tend to fall between if
> >> conditions, we can invert the if logic and jump to the goto. This helps to reduce
> >> indentation and simplify things.
> >>
> >
> > I don't see any logical errors.
> >
> > Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
> Alrighty, thanks for the reviews! I got some feed back in other reviews
> to move the patches 13 and 14 to the end of the set. Which means the
> patches ahead of them may change a bit in order to seat correctly. For
> example, this patch will likely go back to being more like it's v6 version:
>
> https://www.spinics.net/lists/linux-xfs/msg36072.html
>
> Would you prefer I keep or drop your RVB's in this case? Functionally
> they wont change much, but I understand that function is a lot of what
> your are analyzing too. Let me know what you are comfortable with. Thanks!

If functionalilty changes trivally then you can retain my RVBs.

>
> Allison
>
> >
> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> >> ---
> >> fs/xfs/libxfs/xfs_attr.c | 71 ++++++++++++++++++++++++++++--------------------
> >> 1 file changed, 42 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> >> index 30a16fe..dd935ff 100644
> >> --- a/fs/xfs/libxfs/xfs_attr.c
> >> +++ b/fs/xfs/libxfs/xfs_attr.c
> >> @@ -254,6 +254,19 @@ xfs_attr_try_sf_addname(
> >> }
> >>
> >> /*
> >> + * Check to see if the attr should be upgraded from non-existent or shortform to
> >> + * single-leaf-block attribute list.
> >> + */
> >> +static inline bool
> >> +xfs_attr_fmt_needs_update(
> >> + struct xfs_inode *dp)
> >> +{
> >> + return dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> >> + (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> >> + dp->i_d.di_anextents == 0);
> >> +}
> >> +
> >> +/*
> >> * Set the attribute specified in @args.
> >> */
> >> int
> >> @@ -342,40 +355,40 @@ xfs_attr_set_iter(
> >> }
> >>
> >> /*
> >> - * If the attribute list is non-existent or a shortform list,
> >> - * upgrade it to a single-leaf-block attribute list.
> >> + * If the attribute list is already in leaf format, jump straight to
> >> + * leaf handling. Otherwise, try to add the attribute to the shortform
> >> + * list; if there's no room then convert the list to leaf format and try
> >> + * again.
> >> */
> >> - if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> >> - (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> >> - dp->i_d.di_anextents == 0)) {
> >> + if (!xfs_attr_fmt_needs_update(dp))
> >> + goto add_leaf;
> >>
> >> - /*
> >> - * Try to add the attr to the attribute list in the inode.
> >> - */
> >> - error = xfs_attr_try_sf_addname(dp, args);
> >> + /*
> >> + * Try to add the attr to the attribute list in the inode.
> >> + */
> >> + error = xfs_attr_try_sf_addname(dp, args);
> >>
> >> - /* Should only be 0, -EEXIST or ENOSPC */
> >> - if (error != -ENOSPC)
> >> - return error;
> >> + /* Should only be 0, -EEXIST or ENOSPC */
> >> + if (error != -ENOSPC)
> >> + return error;
> >>
> >> - /*
> >> - * It won't fit in the shortform, transform to a leaf block.
> >> - * GROT: another possible req'mt for a double-split btree op.
> >> - */
> >> - error = xfs_attr_shortform_to_leaf(args, leaf_bp);
> >> - if (error)
> >> - return error;
> >> + /*
> >> + * It won't fit in the shortform, transform to a leaf block.
> >> + * GROT: another possible req'mt for a double-split btree op.
> >> + */
> >> + error = xfs_attr_shortform_to_leaf(args, leaf_bp);
> >> + if (error)
> >> + return error;
> >>
> >> - /*
> >> - * Prevent the leaf buffer from being unlocked so that a
> >> - * concurrent AIL push cannot grab the half-baked leaf
> >> - * buffer and run into problems with the write verifier.
> >> - */
> >> - xfs_trans_bhold(args->trans, *leaf_bp);
> >> - args->dac.flags |= XFS_DAC_FINISH_TRANS;
> >> - args->dac.dela_state = XFS_DAS_ADD_LEAF;
> >> - return -EAGAIN;
> >> - }
> >> + /*
> >> + * Prevent the leaf buffer from being unlocked so that a
> >> + * concurrent AIL push cannot grab the half-baked leaf
> >> + * buffer and run into problems with the write verifier.
> >> + */
> >> + xfs_trans_bhold(args->trans, *leaf_bp);
> >> + args->dac.flags |= XFS_DAC_FINISH_TRANS;
> >> + args->dac.dela_state = XFS_DAS_ADD_LEAF;
> >> + return -EAGAIN;
> >>
> >> add_leaf:
> >>
> >>
> >
> >
>

--
chandan