| Message ID | 20191107012801.22863-10-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: Delay Ready Attributes | expand |
On Wed, Nov 06, 2019 at 06:27:53PM -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 | 23 ++++++++++++----------- > 1 file changed, 12 insertions(+), 11 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index dda2eba..e0a38a2 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -227,8 +227,7 @@ xfs_attr_try_sf_addname( > struct xfs_da_args *args) > { > > - struct xfs_mount *mp = dp->i_mount; > - int error, error2; > + int error; > > error = xfs_attr_shortform_addname(args); > if (error == -ENOSPC) > @@ -241,12 +240,7 @@ xfs_attr_try_sf_addname( > if (!error && (args->name.type & ATTR_KERNOTIME) == 0) > xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG); What if you moved this part (the conditional ichgtime) into xfs_attr_shortform_addname? Then this function can just go away. > > - 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; > } > > /* > @@ -258,7 +252,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, > @@ -278,8 +272,15 @@ 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 != -ENOSPC) { > + 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; Can error be something other than 0 or EEXIST? If so, does it make sense to commit even in those cases? (Have I asked this before...?) It looks odd to me that we'd commit the transaction even if something handed back EFSCORRUPTED. Hm, it's a local attr fork so I guess the only possible error is ENOSPC? If that's true then please add a comment/ASSERT to that effect. --D > + } > + > > /* > * It won't fit in the shortform, transform to a leaf block. > -- > 2.7.4 >
On 11/8/19 2:04 PM, Darrick J. Wong wrote: > On Wed, Nov 06, 2019 at 06:27:53PM -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 | 23 ++++++++++++----------- >> 1 file changed, 12 insertions(+), 11 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index dda2eba..e0a38a2 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -227,8 +227,7 @@ xfs_attr_try_sf_addname( >> struct xfs_da_args *args) >> { >> >> - struct xfs_mount *mp = dp->i_mount; >> - int error, error2; >> + int error; >> >> error = xfs_attr_shortform_addname(args); >> if (error == -ENOSPC) >> @@ -241,12 +240,7 @@ xfs_attr_try_sf_addname( >> if (!error && (args->name.type & ATTR_KERNOTIME) == 0) >> xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG); > > What if you moved this part (the conditional ichgtime) into > xfs_attr_shortform_addname? Then this function can just go away. Sure, I may do that in a separate patch though just to make it easier to review. > >> >> - 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; >> } >> >> /* >> @@ -258,7 +252,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, >> @@ -278,8 +272,15 @@ 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 != -ENOSPC) { >> + 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; > > Can error be something other than 0 or EEXIST? If so, does it make > sense to commit even in those cases? (Have I asked this before...?) Yeah, this came up once before: https://patchwork.kernel.org/patch/11087647/ So I simplified it, but then I think people were more comfortable with a straight refactor with no function change: https://patchwork.kernel.org/patch/11134023/ So I put it back. :-) It > looks odd to me that we'd commit the transaction even if something > handed back EFSCORRUPTED. > > Hm, it's a local attr fork so I guess the only possible error is ENOSPC? > If that's true then please add a comment/ASSERT to that effect. Ok, how about a comment. Something like /* Should only be 0, EEXIST or ENOSPC */ Sound good? Allison > > --D > >> + } >> + >> >> /* >> * 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 dda2eba..e0a38a2 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -227,8 +227,7 @@ xfs_attr_try_sf_addname( struct xfs_da_args *args) { - struct xfs_mount *mp = dp->i_mount; - int error, error2; + int error; error = xfs_attr_shortform_addname(args); if (error == -ENOSPC) @@ -241,12 +240,7 @@ 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) - xfs_trans_set_sync(args->trans); - - error2 = xfs_trans_commit(args->trans); - args->trans = NULL; - return error ? error : error2; + return error; } /* @@ -258,7 +252,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, @@ -278,8 +272,15 @@ 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 != -ENOSPC) { + 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, so factor this up to the calling function. Signed-off-by: Allison Collins <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)