Message ID | 20200118225035.19503-8-allison.henderson@oracle.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfs: Delay Ready Attributes | expand |
On Sat, Jan 18, 2020 at 03:50:26PM -0700, Allison Collins wrote: > To help pre-simplify xfs_attr_set_args, we need to hoist transacation > handling up, while modularizing the adjacent code down into helpers. In > this patch, hoist the commit in xfs_attr_try_sf_addname up into the > calling function, and also pull the attr list creation down. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 9ed7e94..c15361a 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -227,8 +227,13 @@ xfs_attr_try_sf_addname( > struct xfs_da_args *args) > { > > - struct xfs_mount *mp = dp->i_mount; > - int error, error2; > + int error; > + > + /* > + * Build initial attribute list (if required). > + */ > + if (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS) > + xfs_attr_shortform_create(args); > > error = xfs_attr_shortform_addname(args); > if (error == -ENOSPC) > @@ -241,12 +246,10 @@ xfs_attr_try_sf_addname( > if (!error && (args->name.type & ATTR_KERNOTIME) == 0) > xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG); > > - if (mp->m_flags & XFS_MOUNT_WSYNC) > + if (dp->i_mount->m_flags & XFS_MOUNT_WSYNC) > xfs_trans_set_sync(args->trans); > > - error2 = xfs_trans_commit(args->trans); > - args->trans = NULL; > - return error ? error : error2; > + return error; > } > > /* > @@ -258,7 +261,7 @@ xfs_attr_set_args( > { > struct xfs_inode *dp = args->dp; > struct xfs_buf *leaf_bp = NULL; > - int error; > + int error, error2 = 0; > > /* > * If the attribute list is non-existent or a shortform list, > @@ -269,17 +272,14 @@ xfs_attr_set_args( > dp->i_d.di_anextents == 0)) { > > /* > - * Build initial attribute list (if required). > - */ > - if (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS) > - xfs_attr_shortform_create(args); > - > - /* > * Try to add the attr to the attribute list in the inode. > */ > error = xfs_attr_try_sf_addname(dp, args); > - if (error != -ENOSPC) > - return error; > + if (error != -ENOSPC) { > + error2 = xfs_trans_commit(args->trans); /me wonders if there's really a point in committing the transaction for things like EEXIST and ENOMEM, but I guess this is a straight conversion and the current code really does this... Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > + args->trans = NULL; > + return error ? error : error2; > + } > > /* > * It won't fit in the shortform, transform to a leaf block. > -- > 2.7.4 >
On 1/21/20 4:07 PM, Darrick J. Wong wrote: > On Sat, Jan 18, 2020 at 03:50:26PM -0700, Allison Collins wrote: >> To help pre-simplify xfs_attr_set_args, we need to hoist transacation >> handling up, while modularizing the adjacent code down into helpers. In >> this patch, hoist the commit in xfs_attr_try_sf_addname up into the >> calling function, and also pull the attr list creation down. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 30 +++++++++++++++--------------- >> 1 file changed, 15 insertions(+), 15 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 9ed7e94..c15361a 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -227,8 +227,13 @@ xfs_attr_try_sf_addname( >> struct xfs_da_args *args) >> { >> >> - struct xfs_mount *mp = dp->i_mount; >> - int error, error2; >> + int error; >> + >> + /* >> + * Build initial attribute list (if required). >> + */ >> + if (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS) >> + xfs_attr_shortform_create(args); >> >> error = xfs_attr_shortform_addname(args); >> if (error == -ENOSPC) >> @@ -241,12 +246,10 @@ xfs_attr_try_sf_addname( >> if (!error && (args->name.type & ATTR_KERNOTIME) == 0) >> xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG); >> >> - if (mp->m_flags & XFS_MOUNT_WSYNC) >> + if (dp->i_mount->m_flags & XFS_MOUNT_WSYNC) >> xfs_trans_set_sync(args->trans); >> >> - error2 = xfs_trans_commit(args->trans); >> - args->trans = NULL; >> - return error ? error : error2; >> + return error; >> } >> >> /* >> @@ -258,7 +261,7 @@ xfs_attr_set_args( >> { >> struct xfs_inode *dp = args->dp; >> struct xfs_buf *leaf_bp = NULL; >> - int error; >> + int error, error2 = 0; >> >> /* >> * If the attribute list is non-existent or a shortform list, >> @@ -269,17 +272,14 @@ xfs_attr_set_args( >> dp->i_d.di_anextents == 0)) { >> >> /* >> - * Build initial attribute list (if required). >> - */ >> - if (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS) >> - xfs_attr_shortform_create(args); >> - >> - /* >> * Try to add the attr to the attribute list in the inode. >> */ >> error = xfs_attr_try_sf_addname(dp, args); >> - if (error != -ENOSPC) >> - return error; >> + if (error != -ENOSPC) { >> + error2 = xfs_trans_commit(args->trans); > > /me wonders if there's really a point in committing the transaction for > things like EEXIST and ENOMEM, but I guess this is a straight > conversion and the current code really does this... > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > --D Alrighty, thanks for the review! Allison > >> + args->trans = NULL; >> + return error ? error : error2; >> + } >> >> /* >> * It won't fit in the shortform, transform to a leaf block. >> -- >> 2.7.4 >>
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 9ed7e94..c15361a 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -227,8 +227,13 @@ xfs_attr_try_sf_addname( struct xfs_da_args *args) { - struct xfs_mount *mp = dp->i_mount; - int error, error2; + int error; + + /* + * Build initial attribute list (if required). + */ + if (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS) + xfs_attr_shortform_create(args); error = xfs_attr_shortform_addname(args); if (error == -ENOSPC) @@ -241,12 +246,10 @@ xfs_attr_try_sf_addname( if (!error && (args->name.type & ATTR_KERNOTIME) == 0) xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG); - if (mp->m_flags & XFS_MOUNT_WSYNC) + if (dp->i_mount->m_flags & XFS_MOUNT_WSYNC) xfs_trans_set_sync(args->trans); - error2 = xfs_trans_commit(args->trans); - args->trans = NULL; - return error ? error : error2; + return error; } /* @@ -258,7 +261,7 @@ xfs_attr_set_args( { struct xfs_inode *dp = args->dp; struct xfs_buf *leaf_bp = NULL; - int error; + int error, error2 = 0; /* * If the attribute list is non-existent or a shortform list, @@ -269,17 +272,14 @@ xfs_attr_set_args( dp->i_d.di_anextents == 0)) { /* - * Build initial attribute list (if required). - */ - if (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS) - xfs_attr_shortform_create(args); - - /* * Try to add the attr to the attribute list in the inode. */ error = xfs_attr_try_sf_addname(dp, args); - if (error != -ENOSPC) - return error; + 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.
To help pre-simplify xfs_attr_set_args, we need to hoist transacation handling up, while modularizing the adjacent code down into helpers. In this patch, hoist the commit in xfs_attr_try_sf_addname up into the calling function, and also pull the attr list creation down. Signed-off-by: Allison Collins <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)