| Message ID | 20200223020611.1802-17-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: Delayed Ready Attrs | expand |
On Sun, Feb 23, 2020 at 4:07 AM Allison Collins <allison.henderson@oracle.com> 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. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> Looks better IMO and doesn't change logic. Reviewed-by: Amir Goldstein <amir73il@gmail.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: > > -- > 2.7.4 >
On 2/23/20 6:26 AM, Amir Goldstein wrote: > On Sun, Feb 23, 2020 at 4:07 AM Allison Collins > <allison.henderson@oracle.com> 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. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> > > Looks better IMO and doesn't change logic. > > Reviewed-by: Amir Goldstein <amir73il@gmail.com> Alrighty, thanks! Allison > >> --- >> 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: >> >> -- >> 2.7.4 >>
On Sat, Feb 22, 2020 at 07:06:08PM -0700, 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. > > 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) Can we use *ip for the inode in newly factored code helpers like this? > +{ > + 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; The logic seems inverted to me here, but that really indicates a sub-optimal function name. It's really checking if the attribute fork is empty or in shortform format. Hence: if (!xfs_attr_is_shortform(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; Heh. This is an example of exactly why I think this should be factored into functions first. Move all the code you just re-indented into xfs_attr_set_shortform(), and the goto disappears because this code becomes: if (xfs_attr_is_shortform(dp)) return xfs_attr_set_shortform(dp, args); add_leaf: That massively improves the readability of the code - it separates the operation implementation from the decision logic nice and cleanly, and lends itself to being implemented in the delayed attr state machine without needing gotos at all. Cheers, Dave.
On 2/25/20 2:21 AM, Dave Chinner wrote: > On Sat, Feb 22, 2020 at 07:06:08PM -0700, 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. >> >> 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) > > Can we use *ip for the inode in newly factored code helpers like > this? Sure, will fix > >> +{ >> + 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; > > The logic seems inverted to me here, but that really indicates a > sub-optimal function name. It's really checking if the attribute > fork is empty or in shortform format. Hence: > > if (!xfs_attr_is_shortform(dp)) > goto add_leaf; > Ok, this had come up in the last review too, we had tried to simplify it to an "is_leaf" helper, though it turned out the logic didnt quite work the same functionally, so I put it back and renamed it "needs_update". I think "is_shortform" is a closer description though. Will update. >> - /* >> - * 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; > > Heh. This is an example of exactly why I think this should be > factored into functions first. Move all the code you just > re-indented into xfs_attr_set_shortform(), and the goto disappears > because this code becomes: > > if (xfs_attr_is_shortform(dp)) > return xfs_attr_set_shortform(dp, args); > > add_leaf: > > That massively improves the readability of the code - it separates > the operation implementation from the decision logic nice and > cleanly, and lends itself to being implemented in the delayed attr > state machine without needing gotos at all. Sure, I actually had it more like that in the last version. I flipped it around because I thought it would help people understand what the refactoring was for if they could see it in context with the states. But if the other way is more helpful, its easy to put back. Will move :-) Allison > > Cheers, > > Dave. >
On Tue, Feb 25, 2020 at 07:13:42PM -0700, Allison Collins wrote: > On 2/25/20 2:21 AM, Dave Chinner wrote: > > Heh. This is an example of exactly why I think this should be > > factored into functions first. Move all the code you just > > re-indented into xfs_attr_set_shortform(), and the goto disappears > > because this code becomes: > > > > if (xfs_attr_is_shortform(dp)) > > return xfs_attr_set_shortform(dp, args); > > > > add_leaf: > > > > That massively improves the readability of the code - it separates > > the operation implementation from the decision logic nice and > > cleanly, and lends itself to being implemented in the delayed attr > > state machine without needing gotos at all. > Sure, I actually had it more like that in the last version. I flipped it > around because I thought it would help people understand what the > refactoring was for if they could see it in context with the states. But if > the other way is more helpful, its easy to put back. Will move :-) In general, factoring first is best. Factoring should not change behaviour, nor change the actual code much. Then when the logic surrounding the new function gets changed later on, it's much easier to see and understand the logic changes as they aren't hidden amongst mass code movements (like re-indenting). Cheers, Dave.
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> > 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: > >
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! 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: >> >> > >
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: > >> > >> > > > > >
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:
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. Signed-off-by: Allison Collins <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 71 ++++++++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 29 deletions(-)