| Message ID | 20191212041513.13855-9-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | xfs: Delay Ready Attributes | expand |
On Wed, Dec 11, 2019 at 09:15:07PM -0700, Allison Collins wrote: > New delayed attribute routines cannot handle transactions. We > can factor up the commit, but there is little left in this > function other than some error handling and an ichgtime. So > hoist all of xfs_attr_try_sf_addname up at this time. We will > remove all the commits in this set. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 54 ++++++++++++++++++++---------------------------- > 1 file changed, 22 insertions(+), 32 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 36f6a43..9c78e0d 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c ... > @@ -258,7 +230,7 @@ xfs_attr_set_args( > { > struct xfs_inode *dp = args->dp; > struct xfs_buf *leaf_bp = NULL; > - int error; > + int error, error2 = 0;; Extra semicolon on the above line. > > /* > * If the attribute list is non-existent or a shortform list, > @@ -277,9 +249,27 @@ xfs_attr_set_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; > + > + error = xfs_attr_shortform_addname(args); > + > + /* Should only be 0, -EEXIST or ENOSPC */ > + if (error != -ENOSPC) { > + /* > + * Commit the shortform mods, and we're done. > + * NOTE: this is also the error path (EEXIST, etc). > + */ > + if (!error && (args->name.type & ATTR_KERNOTIME) == 0) > + xfs_trans_ichgtime(args->trans, dp, > + XFS_ICHGTIME_CHG); > + > + 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; > + } > + And extra whitespace here. With those nits fixed: Reviewed-by: Brian Foster <bfoster@redhat.com> > > /* > * It won't fit in the shortform, transform to a leaf block. > -- > 2.7.4 >
On 12/13/19 7:15 AM, Brian Foster wrote: > On Wed, Dec 11, 2019 at 09:15:07PM -0700, Allison Collins wrote: >> New delayed attribute routines cannot handle transactions. We >> can factor up the commit, but there is little left in this >> function other than some error handling and an ichgtime. So >> hoist all of xfs_attr_try_sf_addname up at this time. We will >> remove all the commits in this set. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 54 ++++++++++++++++++++---------------------------- >> 1 file changed, 22 insertions(+), 32 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 36f6a43..9c78e0d 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c > ... >> @@ -258,7 +230,7 @@ xfs_attr_set_args( >> { >> struct xfs_inode *dp = args->dp; >> struct xfs_buf *leaf_bp = NULL; >> - int error; >> + int error, error2 = 0;; > > Extra semicolon on the above line. > >> >> /* >> * If the attribute list is non-existent or a shortform list, >> @@ -277,9 +249,27 @@ xfs_attr_set_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; >> + >> + error = xfs_attr_shortform_addname(args); >> + >> + /* Should only be 0, -EEXIST or ENOSPC */ >> + if (error != -ENOSPC) { >> + /* >> + * Commit the shortform mods, and we're done. >> + * NOTE: this is also the error path (EEXIST, etc). >> + */ >> + if (!error && (args->name.type & ATTR_KERNOTIME) == 0) >> + xfs_trans_ichgtime(args->trans, dp, >> + XFS_ICHGTIME_CHG); >> + >> + 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; >> + } >> + > > And extra whitespace here. With those nits fixed: > > Reviewed-by: Brian Foster <bfoster@redhat.com> > Ok, will do. Thank you! Allison >> >> /* >> * It won't fit in the shortform, transform to a leaf block. >> -- >> 2.7.4 >> >
On Wed, Dec 11, 2019 at 09:15:07PM -0700, Allison Collins wrote: > New delayed attribute routines cannot handle transactions. We > can factor up the commit, but there is little left in this > function other than some error handling and an ichgtime. So > hoist all of xfs_attr_try_sf_addname up at this time. We will > remove all the commits in this set. I really don't like this one. xfs_attr_try_sf_addname is a nice abstration, so merging it into the caller makes the code much harder to understand. If that is different after changes to the transaction change it can be removed at that point, but merging all the different attr formats into one big monster function is a bad idea. Also Factor up still sounds very, very strange to me. I would have worded it as "merge xfs_attr_try_sf_addname into its only caller"
On 12/24/19 5:25 AM, Christoph Hellwig wrote: > On Wed, Dec 11, 2019 at 09:15:07PM -0700, Allison Collins wrote: >> New delayed attribute routines cannot handle transactions. We >> can factor up the commit, but there is little left in this >> function other than some error handling and an ichgtime. So >> hoist all of xfs_attr_try_sf_addname up at this time. We will >> remove all the commits in this set. > > I really don't like this one. xfs_attr_try_sf_addname is a nice > abstration, so merging it into the caller makes the code much harder > to understand. If that is different after changes to the transaction > change it can be removed at that point, but merging all the different > attr formats into one big monster function is a bad idea. Well, in the last version of the set, I did keep it around, but it was so small, Darrick suggested cleaning it out all together. Here is the same patch in the last set: https://patchwork.kernel.org/patch/11231511/ Maybe when more folks get back from break they can chime in about keeping it or not. I think I commented pretty well on "monster function" in the last patch. It is in all in the pursuit of pulling things up and then breaking them back down again. > > Also Factor up still sounds very, very strange to me. I would > have worded it as "merge xfs_attr_try_sf_addname into its only caller" > I always assumed it to be a figure of speech in reference to factoring out common code, sort of like how one might factor out a common denominator or common multiple from an algebraic expression. If it really bothers folks though, I don't mind changing it. Allison
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 36f6a43..9c78e0d 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -221,34 +221,6 @@ xfs_attr_calc_size( return nblks; } -STATIC int -xfs_attr_try_sf_addname( - struct xfs_inode *dp, - struct xfs_da_args *args) -{ - - struct xfs_mount *mp = dp->i_mount; - int error, error2; - - error = xfs_attr_shortform_addname(args); - if (error == -ENOSPC) - return error; - - /* - * Commit the shortform mods, and we're done. - * NOTE: this is also the error path (EEXIST, etc). - */ - if (!error && (args->name.type & ATTR_KERNOTIME) == 0) - xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG); - - if (mp->m_flags & XFS_MOUNT_WSYNC) - xfs_trans_set_sync(args->trans); - - error2 = xfs_trans_commit(args->trans); - args->trans = NULL; - return error ? error : error2; -} - /* * Set the attribute specified in @args. */ @@ -258,7 +230,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, @@ -277,9 +249,27 @@ xfs_attr_set_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; + + error = xfs_attr_shortform_addname(args); + + /* Should only be 0, -EEXIST or ENOSPC */ + if (error != -ENOSPC) { + /* + * Commit the shortform mods, and we're done. + * NOTE: this is also the error path (EEXIST, etc). + */ + if (!error && (args->name.type & ATTR_KERNOTIME) == 0) + xfs_trans_ichgtime(args->trans, dp, + XFS_ICHGTIME_CHG); + + 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; + } + /* * It won't fit in the shortform, transform to a leaf block.
New delayed attribute routines cannot handle transactions. We can factor up the commit, but there is little left in this function other than some error handling and an ichgtime. So hoist all of xfs_attr_try_sf_addname up at this time. We will remove all the commits in this set. Signed-off-by: Allison Collins <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 54 ++++++++++++++++++++---------------------------- 1 file changed, 22 insertions(+), 32 deletions(-)