| Message ID | 20200223020611.1802-9-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded, 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: > > To help pre-simplify xfs_attr_set_args, we need to hoist transacation handling up, typo: transacation > 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> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > --- And I don't think the subject (Refactoring) is a reliable description of this change, but if nobody else cares, I don't mind what you call it. As far as not changing logic, you may add: Reviewed-by: Amir Goldstein <amir73il@gmail.com> Thanks, Amir.
On 2/23/20 6:04 AM, Amir Goldstein wrote: > On Sun, Feb 23, 2020 at 4:07 AM Allison Collins > <allison.henderson@oracle.com> wrote: >> >> To help pre-simplify xfs_attr_set_args, we need to hoist transacation handling up, > > typo: transacation Will fix :-) > > >> 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> >> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> >> --- > > And I don't think the subject (Refactoring) is a reliable description > of this change, > but if nobody else cares, I don't mind what you call it. Yes, I struggled with that a bit since things are just generally being re-arranged, but if there's a more descriptive term folks prefer, please call it out. > > As far as not changing logic, you may add: > > Reviewed-by: Amir Goldstein <amir73il@gmail.com> Thanks! Allison > > > Thanks, > Amir. >
On Sat, Feb 22, 2020 at 07:06:00PM -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> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > --- Reviewed-by: Brian Foster <bfoster@redhat.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 b2f0780..71298b9 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. > -- > 2.7.4 >
On 2/24/20 6:08 AM, Brian Foster wrote: > On Sat, Feb 22, 2020 at 07:06:00PM -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> >> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> >> --- > > Reviewed-by: Brian Foster <bfoster@redhat.com> Alrighty, thank you! Allison > >> 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 b2f0780..71298b9 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. >> -- >> 2.7.4 >> >
On Sunday, February 23, 2020 7:36 AM 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. > I don't see any logical errors. Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > Reviewed-by: Darrick J. Wong <darrick.wong@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 b2f0780..71298b9 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. >
On 2/28/20 12:42 AM, Chandan Rajendra wrote: > On Sunday, February 23, 2020 7:36 AM 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. >> > > I don't see any logical errors. > > Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com> Alrighty, thank you! Allison > >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> Reviewed-by: Darrick J. Wong <darrick.wong@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 b2f0780..71298b9 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. >> > >
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index b2f0780..71298b9 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.