| Message ID | 20200403221229.4995-14-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: Delay Ready Attributes | expand |
On Fri, Apr 03, 2020 at 03:12:22PM -0700, Allison Collins wrote: > In this patch, we hoist code from xfs_attr_set_args into two new helpers > xfs_attr_is_shortform and xfs_attr_set_shortform. These two will help > to simplify xfs_attr_set_args when we get into delayed attrs later. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 107 +++++++++++++++++++++++++++++++---------------- > 1 file changed, 72 insertions(+), 35 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 4225a94..ba26ffe 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -204,6 +204,66 @@ 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_is_shortform( > + struct xfs_inode *ip) > +{ > + return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || > + (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && > + ip->i_d.di_anextents == 0); Logic should be indented similar to the original: return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && ip->i_d.di_anextents == 0); Otherwise looks good: Reviewed-by: Brian Foster <bfoster@redhat.com> > +} > + > +/* > + * Attempts to set an attr in shortform, or converts the tree to leaf form if > + * there is not enough room. If the attr is set, the transaction is committed > + * and set to NULL. > + */ > +STATIC int > +xfs_attr_set_shortform( > + struct xfs_da_args *args, > + struct xfs_buf **leaf_bp) > +{ > + struct xfs_inode *dp = args->dp; > + int error, error2 = 0; > + > + /* > + * Try to add the attr to the attribute list in the inode. > + */ > + error = xfs_attr_try_sf_addname(dp, args); > + if (error != -ENOSPC) { > + error2 = xfs_trans_commit(args->trans); > + args->trans = NULL; > + return error ? error : error2; > + } > + /* > + * 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. Once we're done rolling the transaction we > + * can release the hold and add the attr to the leaf. > + */ > + xfs_trans_bhold(args->trans, *leaf_bp); > + error = xfs_defer_finish(&args->trans); > + xfs_trans_bhold_release(args->trans, *leaf_bp); > + if (error) { > + xfs_trans_brelse(args->trans, *leaf_bp); > + return error; > + } > + > + return 0; > +} > + > +/* > * Set the attribute specified in @args. > */ > int > @@ -212,48 +272,25 @@ xfs_attr_set_args( > { > struct xfs_inode *dp = args->dp; > struct xfs_buf *leaf_bp = NULL; > - int error, error2 = 0; > + int error = 0; > > /* > - * 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)) { > - > - /* > - * Try to add the attr to the attribute list in the inode. > - */ > - error = xfs_attr_try_sf_addname(dp, args); > - if (error != -ENOSPC) { > - error2 = xfs_trans_commit(args->trans); > - args->trans = NULL; > - return error ? error : error2; > - } > - > - /* > - * 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; > + if (xfs_attr_is_shortform(dp)) { > > /* > - * 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. > - * Once we're done rolling the transaction we can release > - * the hold and add the attr to the leaf. > + * If the attr was successfully set in shortform, the > + * transaction is committed and set to NULL. Otherwise, is it > + * converted from shortform to leaf, and the transaction is > + * retained. > */ > - xfs_trans_bhold(args->trans, leaf_bp); > - error = xfs_defer_finish(&args->trans); > - xfs_trans_bhold_release(args->trans, leaf_bp); > - if (error) { > - xfs_trans_brelse(args->trans, leaf_bp); > + error = xfs_attr_set_shortform(args, &leaf_bp); > + if (error || !args->trans) > return error; > - } > } > > if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { > -- > 2.7.4 >
On 4/7/20 8:23 AM, Brian Foster wrote: > On Fri, Apr 03, 2020 at 03:12:22PM -0700, Allison Collins wrote: >> In this patch, we hoist code from xfs_attr_set_args into two new helpers >> xfs_attr_is_shortform and xfs_attr_set_shortform. These two will help >> to simplify xfs_attr_set_args when we get into delayed attrs later. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 107 +++++++++++++++++++++++++++++++---------------- >> 1 file changed, 72 insertions(+), 35 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 4225a94..ba26ffe 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -204,6 +204,66 @@ 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_is_shortform( >> + struct xfs_inode *ip) >> +{ >> + return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || >> + (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && >> + ip->i_d.di_anextents == 0); > > Logic should be indented similar to the original: > > return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || > (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && > ip->i_d.di_anextents == 0); > > Otherwise looks good: > > Reviewed-by: Brian Foster <bfoster@redhat.com> Alrighty, will fix. Thanks! Allison > >> +} >> + >> +/* >> + * Attempts to set an attr in shortform, or converts the tree to leaf form if >> + * there is not enough room. If the attr is set, the transaction is committed >> + * and set to NULL. >> + */ >> +STATIC int >> +xfs_attr_set_shortform( >> + struct xfs_da_args *args, >> + struct xfs_buf **leaf_bp) >> +{ >> + struct xfs_inode *dp = args->dp; >> + int error, error2 = 0; >> + >> + /* >> + * Try to add the attr to the attribute list in the inode. >> + */ >> + error = xfs_attr_try_sf_addname(dp, args); >> + if (error != -ENOSPC) { >> + error2 = xfs_trans_commit(args->trans); >> + args->trans = NULL; >> + return error ? error : error2; >> + } >> + /* >> + * 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. Once we're done rolling the transaction we >> + * can release the hold and add the attr to the leaf. >> + */ >> + xfs_trans_bhold(args->trans, *leaf_bp); >> + error = xfs_defer_finish(&args->trans); >> + xfs_trans_bhold_release(args->trans, *leaf_bp); >> + if (error) { >> + xfs_trans_brelse(args->trans, *leaf_bp); >> + return error; >> + } >> + >> + return 0; >> +} >> + >> +/* >> * Set the attribute specified in @args. >> */ >> int >> @@ -212,48 +272,25 @@ xfs_attr_set_args( >> { >> struct xfs_inode *dp = args->dp; >> struct xfs_buf *leaf_bp = NULL; >> - int error, error2 = 0; >> + int error = 0; >> >> /* >> - * 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)) { >> - >> - /* >> - * Try to add the attr to the attribute list in the inode. >> - */ >> - error = xfs_attr_try_sf_addname(dp, args); >> - if (error != -ENOSPC) { >> - error2 = xfs_trans_commit(args->trans); >> - args->trans = NULL; >> - return error ? error : error2; >> - } >> - >> - /* >> - * 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; >> + if (xfs_attr_is_shortform(dp)) { >> >> /* >> - * 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. >> - * Once we're done rolling the transaction we can release >> - * the hold and add the attr to the leaf. >> + * If the attr was successfully set in shortform, the >> + * transaction is committed and set to NULL. Otherwise, is it >> + * converted from shortform to leaf, and the transaction is >> + * retained. >> */ >> - xfs_trans_bhold(args->trans, leaf_bp); >> - error = xfs_defer_finish(&args->trans); >> - xfs_trans_bhold_release(args->trans, leaf_bp); >> - if (error) { >> - xfs_trans_brelse(args->trans, leaf_bp); >> + error = xfs_attr_set_shortform(args, &leaf_bp); >> + if (error || !args->trans) >> return error; >> - } >> } >> >> if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { >> -- >> 2.7.4 >> >
On 4/7/20 2:53 PM, Allison Collins wrote: > > > On 4/7/20 8:23 AM, Brian Foster wrote: >> On Fri, Apr 03, 2020 at 03:12:22PM -0700, Allison Collins wrote: >>> In this patch, we hoist code from xfs_attr_set_args into two new helpers >>> xfs_attr_is_shortform and xfs_attr_set_shortform. These two will help >>> to simplify xfs_attr_set_args when we get into delayed attrs later. >>> >>> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >>> --- >>> fs/xfs/libxfs/xfs_attr.c | 107 >>> +++++++++++++++++++++++++++++++---------------- >>> 1 file changed, 72 insertions(+), 35 deletions(-) >>> >>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >>> index 4225a94..ba26ffe 100644 >>> --- a/fs/xfs/libxfs/xfs_attr.c >>> +++ b/fs/xfs/libxfs/xfs_attr.c >>> @@ -204,6 +204,66 @@ 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_is_shortform( >>> + struct xfs_inode *ip) >>> +{ >>> + return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || >>> + (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && >>> + ip->i_d.di_anextents == 0); >> >> Logic should be indented similar to the original: >> >> return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || >> (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && >> ip->i_d.di_anextents == 0); Did you really mean to have the last line offset like this? On second pass, it doesnt look similar to the original, and looks more like it may have been a typo in the review. Just trying to avoid more cycles on spacing goofs. Thx! Allison >> >> Otherwise looks good: >> >> Reviewed-by: Brian Foster <bfoster@redhat.com> > Alrighty, will fix. Thanks! > > Allison > >> >>> +} >>> + >>> +/* >>> + * Attempts to set an attr in shortform, or converts the tree to >>> leaf form if >>> + * there is not enough room. If the attr is set, the transaction is >>> committed >>> + * and set to NULL. >>> + */ >>> +STATIC int >>> +xfs_attr_set_shortform( >>> + struct xfs_da_args *args, >>> + struct xfs_buf **leaf_bp) >>> +{ >>> + struct xfs_inode *dp = args->dp; >>> + int error, error2 = 0; >>> + >>> + /* >>> + * Try to add the attr to the attribute list in the inode. >>> + */ >>> + error = xfs_attr_try_sf_addname(dp, args); >>> + if (error != -ENOSPC) { >>> + error2 = xfs_trans_commit(args->trans); >>> + args->trans = NULL; >>> + return error ? error : error2; >>> + } >>> + /* >>> + * 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. Once we're done rolling the >>> transaction we >>> + * can release the hold and add the attr to the leaf. >>> + */ >>> + xfs_trans_bhold(args->trans, *leaf_bp); >>> + error = xfs_defer_finish(&args->trans); >>> + xfs_trans_bhold_release(args->trans, *leaf_bp); >>> + if (error) { >>> + xfs_trans_brelse(args->trans, *leaf_bp); >>> + return error; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +/* >>> * Set the attribute specified in @args. >>> */ >>> int >>> @@ -212,48 +272,25 @@ xfs_attr_set_args( >>> { >>> struct xfs_inode *dp = args->dp; >>> struct xfs_buf *leaf_bp = NULL; >>> - int error, error2 = 0; >>> + int error = 0; >>> /* >>> - * 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)) { >>> - >>> - /* >>> - * Try to add the attr to the attribute list in the inode. >>> - */ >>> - error = xfs_attr_try_sf_addname(dp, args); >>> - if (error != -ENOSPC) { >>> - error2 = xfs_trans_commit(args->trans); >>> - args->trans = NULL; >>> - return error ? error : error2; >>> - } >>> - >>> - /* >>> - * 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; >>> + if (xfs_attr_is_shortform(dp)) { >>> /* >>> - * 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. >>> - * Once we're done rolling the transaction we can release >>> - * the hold and add the attr to the leaf. >>> + * If the attr was successfully set in shortform, the >>> + * transaction is committed and set to NULL. Otherwise, is it >>> + * converted from shortform to leaf, and the transaction is >>> + * retained. >>> */ >>> - xfs_trans_bhold(args->trans, leaf_bp); >>> - error = xfs_defer_finish(&args->trans); >>> - xfs_trans_bhold_release(args->trans, leaf_bp); >>> - if (error) { >>> - xfs_trans_brelse(args->trans, leaf_bp); >>> + error = xfs_attr_set_shortform(args, &leaf_bp); >>> + if (error || !args->trans) >>> return error; >>> - } >>> } >>> if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { >>> -- >>> 2.7.4 >>> >>
On Fri, Apr 10, 2020 at 09:55:55AM -0700, Allison Collins wrote: > > > On 4/7/20 2:53 PM, Allison Collins wrote: > > > > > > On 4/7/20 8:23 AM, Brian Foster wrote: > > > On Fri, Apr 03, 2020 at 03:12:22PM -0700, Allison Collins wrote: > > > > In this patch, we hoist code from xfs_attr_set_args into two new helpers > > > > xfs_attr_is_shortform and xfs_attr_set_shortform. These two will help > > > > to simplify xfs_attr_set_args when we get into delayed attrs later. > > > > > > > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > > > > --- > > > > fs/xfs/libxfs/xfs_attr.c | 107 > > > > +++++++++++++++++++++++++++++++---------------- > > > > 1 file changed, 72 insertions(+), 35 deletions(-) > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > > > > index 4225a94..ba26ffe 100644 > > > > --- a/fs/xfs/libxfs/xfs_attr.c > > > > +++ b/fs/xfs/libxfs/xfs_attr.c > > > > @@ -204,6 +204,66 @@ 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_is_shortform( > > > > + struct xfs_inode *ip) > > > > +{ > > > > + return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || > > > > + (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && > > > > + ip->i_d.di_anextents == 0); > > > > > > Logic should be indented similar to the original: > > > > > > return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || > > > (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && > > > ip->i_d.di_anextents == 0); > Did you really mean to have the last line offset like this? On second pass, > it doesnt look similar to the original, and looks more like it may have been > a typo in the review. Just trying to avoid more cycles on spacing goofs. > Thx! > Nope. What is quoted here is not how my reply appears in my mailer. :/ Anyways, the last line is supposed to be aligned to the first character inside the opening brace of the previous line. Brian > Allison > > > > > > > Otherwise looks good: > > > > > > Reviewed-by: Brian Foster <bfoster@redhat.com> > > Alrighty, will fix. Thanks! > > > > Allison > > > > > > > > > +} > > > > + > > > > +/* > > > > + * Attempts to set an attr in shortform, or converts the tree > > > > to leaf form if > > > > + * there is not enough room. If the attr is set, the > > > > transaction is committed > > > > + * and set to NULL. > > > > + */ > > > > +STATIC int > > > > +xfs_attr_set_shortform( > > > > + struct xfs_da_args *args, > > > > + struct xfs_buf **leaf_bp) > > > > +{ > > > > + struct xfs_inode *dp = args->dp; > > > > + int error, error2 = 0; > > > > + > > > > + /* > > > > + * Try to add the attr to the attribute list in the inode. > > > > + */ > > > > + error = xfs_attr_try_sf_addname(dp, args); > > > > + if (error != -ENOSPC) { > > > > + error2 = xfs_trans_commit(args->trans); > > > > + args->trans = NULL; > > > > + return error ? error : error2; > > > > + } > > > > + /* > > > > + * 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. Once we're done rolling the > > > > transaction we > > > > + * can release the hold and add the attr to the leaf. > > > > + */ > > > > + xfs_trans_bhold(args->trans, *leaf_bp); > > > > + error = xfs_defer_finish(&args->trans); > > > > + xfs_trans_bhold_release(args->trans, *leaf_bp); > > > > + if (error) { > > > > + xfs_trans_brelse(args->trans, *leaf_bp); > > > > + return error; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +/* > > > > * Set the attribute specified in @args. > > > > */ > > > > int > > > > @@ -212,48 +272,25 @@ xfs_attr_set_args( > > > > { > > > > struct xfs_inode *dp = args->dp; > > > > struct xfs_buf *leaf_bp = NULL; > > > > - int error, error2 = 0; > > > > + int error = 0; > > > > /* > > > > - * 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)) { > > > > - > > > > - /* > > > > - * Try to add the attr to the attribute list in the inode. > > > > - */ > > > > - error = xfs_attr_try_sf_addname(dp, args); > > > > - if (error != -ENOSPC) { > > > > - error2 = xfs_trans_commit(args->trans); > > > > - args->trans = NULL; > > > > - return error ? error : error2; > > > > - } > > > > - > > > > - /* > > > > - * 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; > > > > + if (xfs_attr_is_shortform(dp)) { > > > > /* > > > > - * 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. > > > > - * Once we're done rolling the transaction we can release > > > > - * the hold and add the attr to the leaf. > > > > + * If the attr was successfully set in shortform, the > > > > + * transaction is committed and set to NULL. Otherwise, is it > > > > + * converted from shortform to leaf, and the transaction is > > > > + * retained. > > > > */ > > > > - xfs_trans_bhold(args->trans, leaf_bp); > > > > - error = xfs_defer_finish(&args->trans); > > > > - xfs_trans_bhold_release(args->trans, leaf_bp); > > > > - if (error) { > > > > - xfs_trans_brelse(args->trans, leaf_bp); > > > > + error = xfs_attr_set_shortform(args, &leaf_bp); > > > > + if (error || !args->trans) > > > > return error; > > > > - } > > > > } > > > > if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { > > > > -- > > > > 2.7.4 > > > > > > > >
On Saturday, April 4, 2020 3:42 AM Allison Collins wrote: > In this patch, we hoist code from xfs_attr_set_args into two new helpers > xfs_attr_is_shortform and xfs_attr_set_shortform. These two will help > to simplify xfs_attr_set_args when we get into delayed attrs later. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 107 +++++++++++++++++++++++++++++++---------------- > 1 file changed, 72 insertions(+), 35 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 4225a94..ba26ffe 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -204,6 +204,66 @@ 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_is_shortform( > + struct xfs_inode *ip) > +{ > + return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || > + (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && > + ip->i_d.di_anextents == 0); > +} > + > +/* > + * Attempts to set an attr in shortform, or converts the tree to leaf form if I think you meant to say "converts short form to leaf form". The functional changes look logically correct, Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> > + * there is not enough room. If the attr is set, the transaction is committed > + * and set to NULL. > + */ > +STATIC int > +xfs_attr_set_shortform( > + struct xfs_da_args *args, > + struct xfs_buf **leaf_bp) > +{ > + struct xfs_inode *dp = args->dp; > + int error, error2 = 0; > + > + /* > + * Try to add the attr to the attribute list in the inode. > + */ > + error = xfs_attr_try_sf_addname(dp, args); > + if (error != -ENOSPC) { > + error2 = xfs_trans_commit(args->trans); > + args->trans = NULL; > + return error ? error : error2; > + } > + /* > + * 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. Once we're done rolling the transaction we > + * can release the hold and add the attr to the leaf. > + */ > + xfs_trans_bhold(args->trans, *leaf_bp); > + error = xfs_defer_finish(&args->trans); > + xfs_trans_bhold_release(args->trans, *leaf_bp); > + if (error) { > + xfs_trans_brelse(args->trans, *leaf_bp); > + return error; > + } > + > + return 0; > +} > + > +/* > * Set the attribute specified in @args. > */ > int > @@ -212,48 +272,25 @@ xfs_attr_set_args( > { > struct xfs_inode *dp = args->dp; > struct xfs_buf *leaf_bp = NULL; > - int error, error2 = 0; > + int error = 0; > > /* > - * 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)) { > - > - /* > - * Try to add the attr to the attribute list in the inode. > - */ > - error = xfs_attr_try_sf_addname(dp, args); > - if (error != -ENOSPC) { > - error2 = xfs_trans_commit(args->trans); > - args->trans = NULL; > - return error ? error : error2; > - } > - > - /* > - * 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; > + if (xfs_attr_is_shortform(dp)) { > > /* > - * 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. > - * Once we're done rolling the transaction we can release > - * the hold and add the attr to the leaf. > + * If the attr was successfully set in shortform, the > + * transaction is committed and set to NULL. Otherwise, is it > + * converted from shortform to leaf, and the transaction is > + * retained. > */ > - xfs_trans_bhold(args->trans, leaf_bp); > - error = xfs_defer_finish(&args->trans); > - xfs_trans_bhold_release(args->trans, leaf_bp); > - if (error) { > - xfs_trans_brelse(args->trans, leaf_bp); > + error = xfs_attr_set_shortform(args, &leaf_bp); > + if (error || !args->trans) > return error; > - } > } > > if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { >
On 4/19/20 10:30 PM, Chandan Rajendra wrote: > On Saturday, April 4, 2020 3:42 AM Allison Collins wrote: >> In this patch, we hoist code from xfs_attr_set_args into two new helpers >> xfs_attr_is_shortform and xfs_attr_set_shortform. These two will help >> to simplify xfs_attr_set_args when we get into delayed attrs later. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 107 +++++++++++++++++++++++++++++++---------------- >> 1 file changed, 72 insertions(+), 35 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 4225a94..ba26ffe 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -204,6 +204,66 @@ 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_is_shortform( >> + struct xfs_inode *ip) >> +{ >> + return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || >> + (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && >> + ip->i_d.di_anextents == 0); >> +} >> + >> +/* >> + * Attempts to set an attr in shortform, or converts the tree to leaf form if > > I think you meant to say "converts short form to leaf form". > > The functional changes look logically correct, Ok, will fix the comment. Thanks for the review! Allison > > Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> > >> + * there is not enough room. If the attr is set, the transaction is committed >> + * and set to NULL. >> + */ >> +STATIC int >> +xfs_attr_set_shortform( >> + struct xfs_da_args *args, >> + struct xfs_buf **leaf_bp) >> +{ >> + struct xfs_inode *dp = args->dp; >> + int error, error2 = 0; >> + >> + /* >> + * Try to add the attr to the attribute list in the inode. >> + */ >> + error = xfs_attr_try_sf_addname(dp, args); >> + if (error != -ENOSPC) { >> + error2 = xfs_trans_commit(args->trans); >> + args->trans = NULL; >> + return error ? error : error2; >> + } >> + /* >> + * 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. Once we're done rolling the transaction we >> + * can release the hold and add the attr to the leaf. >> + */ >> + xfs_trans_bhold(args->trans, *leaf_bp); >> + error = xfs_defer_finish(&args->trans); >> + xfs_trans_bhold_release(args->trans, *leaf_bp); >> + if (error) { >> + xfs_trans_brelse(args->trans, *leaf_bp); >> + return error; >> + } >> + >> + return 0; >> +} >> + >> +/* >> * Set the attribute specified in @args. >> */ >> int >> @@ -212,48 +272,25 @@ xfs_attr_set_args( >> { >> struct xfs_inode *dp = args->dp; >> struct xfs_buf *leaf_bp = NULL; >> - int error, error2 = 0; >> + int error = 0; >> >> /* >> - * 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)) { >> - >> - /* >> - * Try to add the attr to the attribute list in the inode. >> - */ >> - error = xfs_attr_try_sf_addname(dp, args); >> - if (error != -ENOSPC) { >> - error2 = xfs_trans_commit(args->trans); >> - args->trans = NULL; >> - return error ? error : error2; >> - } >> - >> - /* >> - * 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; >> + if (xfs_attr_is_shortform(dp)) { >> >> /* >> - * 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. >> - * Once we're done rolling the transaction we can release >> - * the hold and add the attr to the leaf. >> + * If the attr was successfully set in shortform, the >> + * transaction is committed and set to NULL. Otherwise, is it >> + * converted from shortform to leaf, and the transaction is >> + * retained. >> */ >> - xfs_trans_bhold(args->trans, leaf_bp); >> - error = xfs_defer_finish(&args->trans); >> - xfs_trans_bhold_release(args->trans, leaf_bp); >> - if (error) { >> - xfs_trans_brelse(args->trans, leaf_bp); >> + error = xfs_attr_set_shortform(args, &leaf_bp); >> + if (error || !args->trans) >> return error; >> - } >> } >> >> if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { >> > >
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 4225a94..ba26ffe 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -204,6 +204,66 @@ 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_is_shortform( + struct xfs_inode *ip) +{ + return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || + (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && + ip->i_d.di_anextents == 0); +} + +/* + * Attempts to set an attr in shortform, or converts the tree to leaf form if + * there is not enough room. If the attr is set, the transaction is committed + * and set to NULL. + */ +STATIC int +xfs_attr_set_shortform( + struct xfs_da_args *args, + struct xfs_buf **leaf_bp) +{ + struct xfs_inode *dp = args->dp; + int error, error2 = 0; + + /* + * Try to add the attr to the attribute list in the inode. + */ + error = xfs_attr_try_sf_addname(dp, args); + if (error != -ENOSPC) { + error2 = xfs_trans_commit(args->trans); + args->trans = NULL; + return error ? error : error2; + } + /* + * 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. Once we're done rolling the transaction we + * can release the hold and add the attr to the leaf. + */ + xfs_trans_bhold(args->trans, *leaf_bp); + error = xfs_defer_finish(&args->trans); + xfs_trans_bhold_release(args->trans, *leaf_bp); + if (error) { + xfs_trans_brelse(args->trans, *leaf_bp); + return error; + } + + return 0; +} + +/* * Set the attribute specified in @args. */ int @@ -212,48 +272,25 @@ xfs_attr_set_args( { struct xfs_inode *dp = args->dp; struct xfs_buf *leaf_bp = NULL; - int error, error2 = 0; + int error = 0; /* - * 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)) { - - /* - * Try to add the attr to the attribute list in the inode. - */ - error = xfs_attr_try_sf_addname(dp, args); - if (error != -ENOSPC) { - error2 = xfs_trans_commit(args->trans); - args->trans = NULL; - return error ? error : error2; - } - - /* - * 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; + if (xfs_attr_is_shortform(dp)) { /* - * 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. - * Once we're done rolling the transaction we can release - * the hold and add the attr to the leaf. + * If the attr was successfully set in shortform, the + * transaction is committed and set to NULL. Otherwise, is it + * converted from shortform to leaf, and the transaction is + * retained. */ - xfs_trans_bhold(args->trans, leaf_bp); - error = xfs_defer_finish(&args->trans); - xfs_trans_bhold_release(args->trans, leaf_bp); - if (error) { - xfs_trans_brelse(args->trans, leaf_bp); + error = xfs_attr_set_shortform(args, &leaf_bp); + if (error || !args->trans) return error; - } } if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
In this patch, we hoist code from xfs_attr_set_args into two new helpers xfs_attr_is_shortform and xfs_attr_set_shortform. These two will help to simplify xfs_attr_set_args when we get into delayed attrs later. Signed-off-by: Allison Collins <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 107 +++++++++++++++++++++++++++++++---------------- 1 file changed, 72 insertions(+), 35 deletions(-)