| Message ID | 20190905221837.17388-9-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Delayed Attributes | expand |
On Thu, Sep 05, 2019 at 03:18:26PM -0700, Allison Collins wrote: > New delayed attribute routines cannot handle transactions, > so factor this up to the calling function. > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index f27e2c6..318c543 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -227,7 +227,7 @@ xfs_attr_try_sf_addname( > { > > struct xfs_mount *mp = dp->i_mount; > - int error, error2; > + int error; > > error = xfs_attr_shortform_addname(args); > if (error == -ENOSPC) > @@ -243,9 +243,7 @@ xfs_attr_try_sf_addname( > if (mp->m_flags & XFS_MOUNT_WSYNC) > xfs_trans_set_sync(args->trans); > Perhaps the above check should stay along with the tx commit code..? > - error2 = xfs_trans_commit(args->trans); > - args->trans = NULL; > - return error ? error : error2; > + return error; > } > > /* > @@ -257,7 +255,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,8 +275,11 @@ 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; > + if (!error) { > + error2 = xfs_trans_commit(args->trans); > + args->trans = NULL; > + return error ? error : error2; We've already checked that error == 0 here, so this can be simplified. Hmm.. that said, the original code looks like it commits the transaction on error != -ENOSPC, which means this slightly changes behavior when (error && error != -ENOSPC) is true. So perhaps it is the error check that should be fixed up and not the error2 logic.. Brian > + } > > /* > * It won't fit in the shortform, transform to a leaf block. > -- > 2.7.4 >
On 9/20/19 6:50 AM, Brian Foster wrote: > On Thu, Sep 05, 2019 at 03:18:26PM -0700, Allison Collins wrote: >> New delayed attribute routines cannot handle transactions, >> so factor this up to the calling function. >> >> Signed-off-by: Allison Collins <allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 15 ++++++++------- >> 1 file changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index f27e2c6..318c543 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -227,7 +227,7 @@ xfs_attr_try_sf_addname( >> { >> >> struct xfs_mount *mp = dp->i_mount; >> - int error, error2; >> + int error; >> >> error = xfs_attr_shortform_addname(args); >> if (error == -ENOSPC) >> @@ -243,9 +243,7 @@ xfs_attr_try_sf_addname( >> if (mp->m_flags & XFS_MOUNT_WSYNC) >> xfs_trans_set_sync(args->trans); >> > > Perhaps the above check should stay along with the tx commit code..? That makes sense, I will move it upwards > >> - error2 = xfs_trans_commit(args->trans); >> - args->trans = NULL; >> - return error ? error : error2; >> + return error; >> } >> >> /* >> @@ -257,7 +255,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,8 +275,11 @@ 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; >> + if (!error) { >> + error2 = xfs_trans_commit(args->trans); >> + args->trans = NULL; >> + return error ? error : error2; > > We've already checked that error == 0 here, so this can be simplified. > Hmm.. that said, the original code looks like it commits the transaction > on error != -ENOSPC, which means this slightly changes behavior when > (error && error != -ENOSPC) is true. So perhaps it is the error check > that should be fixed up and not the error2 logic.. Yes, I believe this got some attention in the last review. While it is different logic now, I think we reasoned that committing on say -EIO or some other such unexpected error didn't make much sense either, so we cleaned it up a bit. Though you're probably right about the simplification now with the change. Is there a reason we would want to commit in the case of unexpected errors? Allison > > Brian > >> + } >> >> /* >> * It won't fit in the shortform, transform to a leaf block. >> -- >> 2.7.4 >>
On Fri, Sep 20, 2019 at 06:25:04PM -0700, Allison Collins wrote: > > > On 9/20/19 6:50 AM, Brian Foster wrote: > > On Thu, Sep 05, 2019 at 03:18:26PM -0700, Allison Collins wrote: > > > New delayed attribute routines cannot handle transactions, > > > so factor this up to the calling function. > > > > > > Signed-off-by: Allison Collins <allison.henderson@oracle.com> > > > --- > > > fs/xfs/libxfs/xfs_attr.c | 15 ++++++++------- > > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > > > index f27e2c6..318c543 100644 > > > --- a/fs/xfs/libxfs/xfs_attr.c > > > +++ b/fs/xfs/libxfs/xfs_attr.c > > > @@ -227,7 +227,7 @@ xfs_attr_try_sf_addname( > > > { > > > struct xfs_mount *mp = dp->i_mount; > > > - int error, error2; > > > + int error; > > > error = xfs_attr_shortform_addname(args); > > > if (error == -ENOSPC) > > > @@ -243,9 +243,7 @@ xfs_attr_try_sf_addname( > > > if (mp->m_flags & XFS_MOUNT_WSYNC) > > > xfs_trans_set_sync(args->trans); > > > > Perhaps the above check should stay along with the tx commit code..? > That makes sense, I will move it upwards > > > > > - error2 = xfs_trans_commit(args->trans); > > > - args->trans = NULL; > > > - return error ? error : error2; > > > + return error; > > > } > > > /* > > > @@ -257,7 +255,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,8 +275,11 @@ 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; > > > + if (!error) { > > > + error2 = xfs_trans_commit(args->trans); > > > + args->trans = NULL; > > > + return error ? error : error2; > > > > We've already checked that error == 0 here, so this can be simplified. > > Hmm.. that said, the original code looks like it commits the transaction > > on error != -ENOSPC, which means this slightly changes behavior when > > (error && error != -ENOSPC) is true. So perhaps it is the error check > > that should be fixed up and not the error2 logic.. > > Yes, I believe this got some attention in the last review. While it is > different logic now, I think we reasoned that committing on say -EIO or some > other such unexpected error didn't make much sense either, so we cleaned it > up a bit. Though you're probably right about the simplification now with > the change. Is there a reason we would want to commit in the case of > unexpected errors? > I'm not 100% sure tbh. There is a comment that acknowledges unrelated errors so it might be intentional, at least for some errors. I'd at least suggest to preserve the existing logic in the refactoring patch and if you think there's a bug, perhaps tack on a separate patch later for independent review. That way if we find we broke something down the line, it's easier to identify and test the logic change separately from the broader rework. Brian > Allison > > > > > Brian > > > > > + } > > > /* > > > * 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 f27e2c6..318c543 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -227,7 +227,7 @@ xfs_attr_try_sf_addname( { struct xfs_mount *mp = dp->i_mount; - int error, error2; + int error; error = xfs_attr_shortform_addname(args); if (error == -ENOSPC) @@ -243,9 +243,7 @@ xfs_attr_try_sf_addname( 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; + return error; } /* @@ -257,7 +255,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,8 +275,11 @@ 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; + if (!error) { + 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, so factor this up to the calling function. Signed-off-by: Allison Collins <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)