Message ID | 20210727062053.11129-12-allison.henderson@oracle.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | Delayed Attributes | expand |
On Mon, Jul 26, 2021 at 11:20:48PM -0700, Allison Henderson wrote: > From: Allison Collins <allison.henderson@oracle.com> > > These routines set up and queue a new deferred attribute operations. > These functions are meant to be called by any routine needing to > initiate a deferred attribute operation as opposed to the existing > inline operations. New helper function xfs_attr_item_init also added. > > Finally enable delayed attributes in xfs_attr_set and xfs_attr_remove. > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/xfs/libxfs/xfs_attr.c | 70 +++++++++++++++++++++++++++++++++++++++++++++--- > fs/xfs/libxfs/xfs_attr.h | 2 ++ > fs/xfs/xfs_log.c | 41 ++++++++++++++++++++++++++++ > fs/xfs/xfs_log.h | 1 + > 4 files changed, 111 insertions(+), 3 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index eee219c6..c447c21 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -25,6 +25,8 @@ > #include "xfs_trans_space.h" > #include "xfs_trace.h" > #include "xfs_attr_item.h" > +#include "xfs_attr.h" > +#include "xfs_log.h" > > /* > * xfs_attr.c > @@ -779,13 +781,19 @@ xfs_attr_set( > rmt_blks = xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX); > } > > + if (xfs_hasdelattr(mp)) { > + error = xfs_attr_use_log_assist(mp); > + if (error) > + return error; > + } > + > /* > * Root fork attributes can use reserved data blocks for this > * operation if necessary > */ > error = xfs_trans_alloc_inode(dp, &tres, total, 0, rsvd, &args->trans); > if (error) > - return error; > + goto drop_incompat; > > if (args->value || xfs_inode_hasattr(dp)) { > error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK, > @@ -803,9 +811,10 @@ xfs_attr_set( > if (error != -ENOATTR && error != -EEXIST) > goto out_trans_cancel; > > - error = xfs_attr_set_args(args); > + error = xfs_attr_set_deferred(args); > if (error) > goto out_trans_cancel; > + > /* shortform attribute has already been committed */ > if (!args->trans) > goto out_unlock; > @@ -814,7 +823,7 @@ xfs_attr_set( > if (error != -EEXIST) > goto out_trans_cancel; > > - error = xfs_attr_remove_args(args); > + error = xfs_attr_remove_deferred(args); > if (error) > goto out_trans_cancel; > } > @@ -836,6 +845,9 @@ xfs_attr_set( > error = xfs_trans_commit(args->trans); > out_unlock: > xfs_iunlock(dp, XFS_ILOCK_EXCL); > +drop_incompat: > + if (xfs_hasdelattr(mp)) Subtle race here: if you turn this into a debug option and someone changes the value from 1 to 0 while this function is running, we'll fail to call _drop_incompat here. You should sample the value at the start of the function into a local variable and use that throughout. > + xlog_drop_incompat_feat(mp->m_log); > return error; > > out_trans_cancel: > @@ -844,6 +856,58 @@ xfs_attr_set( > goto out_unlock; > } > > +STATIC int > +xfs_attr_item_init( > + struct xfs_da_args *args, > + unsigned int op_flags, /* op flag (set or remove) */ > + struct xfs_attr_item **attr) /* new xfs_attr_item */ > +{ > + > + struct xfs_attr_item *new; > + > + new = kmem_zalloc(sizeof(struct xfs_attr_item), KM_NOFS); > + new->xattri_op_flags = op_flags; > + new->xattri_dac.da_args = args; > + > + *attr = new; > + return 0; > +} > + > +/* Sets an attribute for an inode as a deferred operation */ > +int > +xfs_attr_set_deferred( > + struct xfs_da_args *args) > +{ > + struct xfs_attr_item *new; > + int error = 0; > + > + error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new); > + if (error) > + return error; > + > + xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list); > + > + return 0; > +} > + > +/* Removes an attribute for an inode as a deferred operation */ > +int > +xfs_attr_remove_deferred( > + struct xfs_da_args *args) > +{ > + > + struct xfs_attr_item *new; > + int error; > + > + error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE, &new); > + if (error) > + return error; > + > + xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list); > + > + return 0; > +} > + > /*======================================================================== > * External routines when attribute list is inside the inode > *========================================================================*/ > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > index 463b2be..72b0ea5 100644 > --- a/fs/xfs/libxfs/xfs_attr.h > +++ b/fs/xfs/libxfs/xfs_attr.h > @@ -527,5 +527,7 @@ bool xfs_attr_namecheck(const void *name, size_t length); > void xfs_delattr_context_init(struct xfs_delattr_context *dac, > struct xfs_da_args *args); > int xfs_attr_calc_size(struct xfs_da_args *args, int *local); > +int xfs_attr_set_deferred(struct xfs_da_args *args); > +int xfs_attr_remove_deferred(struct xfs_da_args *args); > > #endif /* __XFS_ATTR_H__ */ > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index 7c593d9..216de6c 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -3948,3 +3948,44 @@ xlog_drop_incompat_feat( > { > up_read(&log->l_incompat_users); > } > + > +/* > + * Get permission to use log-assisted atomic exchange of file extents. > + * > + * Callers must not be running any transactions or hold any inode locks, and > + * they must release the permission by calling xlog_drop_incompat_feat > + * when they're done. > + */ > +int > +xfs_attr_use_log_assist( > + struct xfs_mount *mp) > +{ > + int error = 0; > + > + /* > + * Protect ourselves from an idle log clearing the logged xattrs log > + * incompat feature bit. > + */ > + xlog_use_incompat_feat(mp->m_log); > + > + /* > + * If log-assisted xattrs are already enabled, the caller can use the > + * log assisted swap functions with the log-incompat reference we got. > + */ > + if (xfs_sb_version_hasdelattr(&mp->m_sb)) > + return 0; > + > + /* Enable log-assisted xattrs. */ > + xfs_warn_once(mp, > +"EXPERIMENTAL logged extended attributes feature added. Use at your own risk!"); > + > + error = xfs_add_incompat_log_feature(mp, > + XFS_SB_FEAT_INCOMPAT_LOG_DELATTR); > + if (error) > + goto drop_incompat; Minor bug here (and yes, it's also in the swapext code that I pointed you to; apologies for that): We shouldn't print the warning unless the feature addition actually succeeds. IOWs, move the xfs_warn_once() here. (That said, if the feature add fails then the fs is probably going to crash soon anyway so it probably doesn't matter...) --D > + > + return 0; > +drop_incompat: > + xlog_drop_incompat_feat(mp->m_log); > + return error; > +} > diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h > index b274fb9..1e461671 100644 > --- a/fs/xfs/xfs_log.h > +++ b/fs/xfs/xfs_log.h > @@ -144,5 +144,6 @@ xfs_lsn_t xlog_grant_push_threshold(struct xlog *log, int need_bytes); > > void xlog_use_incompat_feat(struct xlog *log); > void xlog_drop_incompat_feat(struct xlog *log); > +int xfs_attr_use_log_assist(struct xfs_mount *mp); > > #endif /* __XFS_LOG_H__ */ > -- > 2.7.4 >
On 27 Jul 2021 at 11:50, Allison Henderson wrote: > From: Allison Collins <allison.henderson@oracle.com> > > These routines set up and queue a new deferred attribute operations. > These functions are meant to be called by any routine needing to > initiate a deferred attribute operation as opposed to the existing > inline operations. New helper function xfs_attr_item_init also added. > > Finally enable delayed attributes in xfs_attr_set and xfs_attr_remove. Apart from the issues pointed out by Darrick, the remaining changes seem to be fine. > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/xfs/libxfs/xfs_attr.c | 70 +++++++++++++++++++++++++++++++++++++++++++++--- > fs/xfs/libxfs/xfs_attr.h | 2 ++ > fs/xfs/xfs_log.c | 41 ++++++++++++++++++++++++++++ > fs/xfs/xfs_log.h | 1 + > 4 files changed, 111 insertions(+), 3 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index eee219c6..c447c21 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -25,6 +25,8 @@ > #include "xfs_trans_space.h" > #include "xfs_trace.h" > #include "xfs_attr_item.h" > +#include "xfs_attr.h" > +#include "xfs_log.h" > > /* > * xfs_attr.c > @@ -779,13 +781,19 @@ xfs_attr_set( > rmt_blks = xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX); > } > > + if (xfs_hasdelattr(mp)) { > + error = xfs_attr_use_log_assist(mp); > + if (error) > + return error; > + } > + > /* > * Root fork attributes can use reserved data blocks for this > * operation if necessary > */ > error = xfs_trans_alloc_inode(dp, &tres, total, 0, rsvd, &args->trans); > if (error) > - return error; > + goto drop_incompat; > > if (args->value || xfs_inode_hasattr(dp)) { > error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK, > @@ -803,9 +811,10 @@ xfs_attr_set( > if (error != -ENOATTR && error != -EEXIST) > goto out_trans_cancel; > > - error = xfs_attr_set_args(args); > + error = xfs_attr_set_deferred(args); > if (error) > goto out_trans_cancel; > + > /* shortform attribute has already been committed */ > if (!args->trans) > goto out_unlock; > @@ -814,7 +823,7 @@ xfs_attr_set( > if (error != -EEXIST) > goto out_trans_cancel; > > - error = xfs_attr_remove_args(args); > + error = xfs_attr_remove_deferred(args); > if (error) > goto out_trans_cancel; > } > @@ -836,6 +845,9 @@ xfs_attr_set( > error = xfs_trans_commit(args->trans); > out_unlock: > xfs_iunlock(dp, XFS_ILOCK_EXCL); > +drop_incompat: > + if (xfs_hasdelattr(mp)) > + xlog_drop_incompat_feat(mp->m_log); > return error; > > out_trans_cancel: > @@ -844,6 +856,58 @@ xfs_attr_set( > goto out_unlock; > } > > +STATIC int > +xfs_attr_item_init( > + struct xfs_da_args *args, > + unsigned int op_flags, /* op flag (set or remove) */ > + struct xfs_attr_item **attr) /* new xfs_attr_item */ > +{ > + > + struct xfs_attr_item *new; > + > + new = kmem_zalloc(sizeof(struct xfs_attr_item), KM_NOFS); > + new->xattri_op_flags = op_flags; > + new->xattri_dac.da_args = args; > + > + *attr = new; > + return 0; > +} > + > +/* Sets an attribute for an inode as a deferred operation */ > +int > +xfs_attr_set_deferred( > + struct xfs_da_args *args) > +{ > + struct xfs_attr_item *new; > + int error = 0; > + > + error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new); > + if (error) > + return error; > + > + xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list); > + > + return 0; > +} > + > +/* Removes an attribute for an inode as a deferred operation */ > +int > +xfs_attr_remove_deferred( > + struct xfs_da_args *args) > +{ > + > + struct xfs_attr_item *new; > + int error; > + > + error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE, &new); > + if (error) > + return error; > + > + xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list); > + > + return 0; > +} > + > /*======================================================================== > * External routines when attribute list is inside the inode > *========================================================================*/ > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > index 463b2be..72b0ea5 100644 > --- a/fs/xfs/libxfs/xfs_attr.h > +++ b/fs/xfs/libxfs/xfs_attr.h > @@ -527,5 +527,7 @@ bool xfs_attr_namecheck(const void *name, size_t length); > void xfs_delattr_context_init(struct xfs_delattr_context *dac, > struct xfs_da_args *args); > int xfs_attr_calc_size(struct xfs_da_args *args, int *local); > +int xfs_attr_set_deferred(struct xfs_da_args *args); > +int xfs_attr_remove_deferred(struct xfs_da_args *args); > > #endif /* __XFS_ATTR_H__ */ > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index 7c593d9..216de6c 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -3948,3 +3948,44 @@ xlog_drop_incompat_feat( > { > up_read(&log->l_incompat_users); > } > + > +/* > + * Get permission to use log-assisted atomic exchange of file extents. > + * > + * Callers must not be running any transactions or hold any inode locks, and > + * they must release the permission by calling xlog_drop_incompat_feat > + * when they're done. > + */ > +int > +xfs_attr_use_log_assist( > + struct xfs_mount *mp) > +{ > + int error = 0; > + > + /* > + * Protect ourselves from an idle log clearing the logged xattrs log > + * incompat feature bit. > + */ > + xlog_use_incompat_feat(mp->m_log); > + > + /* > + * If log-assisted xattrs are already enabled, the caller can use the > + * log assisted swap functions with the log-incompat reference we got. > + */ > + if (xfs_sb_version_hasdelattr(&mp->m_sb)) > + return 0; > + > + /* Enable log-assisted xattrs. */ > + xfs_warn_once(mp, > +"EXPERIMENTAL logged extended attributes feature added. Use at your own risk!"); > + > + error = xfs_add_incompat_log_feature(mp, > + XFS_SB_FEAT_INCOMPAT_LOG_DELATTR); > + if (error) > + goto drop_incompat; > + > + return 0; > +drop_incompat: > + xlog_drop_incompat_feat(mp->m_log); > + return error; > +} > diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h > index b274fb9..1e461671 100644 > --- a/fs/xfs/xfs_log.h > +++ b/fs/xfs/xfs_log.h > @@ -144,5 +144,6 @@ xfs_lsn_t xlog_grant_push_threshold(struct xlog *log, int need_bytes); > > void xlog_use_incompat_feat(struct xlog *log); > void xlog_drop_incompat_feat(struct xlog *log); > +int xfs_attr_use_log_assist(struct xfs_mount *mp); > > #endif /* __XFS_LOG_H__ */ -- chandan
On 7/28/21 12:24 PM, Darrick J. Wong wrote: > On Mon, Jul 26, 2021 at 11:20:48PM -0700, Allison Henderson wrote: >> From: Allison Collins <allison.henderson@oracle.com> >> >> These routines set up and queue a new deferred attribute operations. >> These functions are meant to be called by any routine needing to >> initiate a deferred attribute operation as opposed to the existing >> inline operations. New helper function xfs_attr_item_init also added. >> >> Finally enable delayed attributes in xfs_attr_set and xfs_attr_remove. >> >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> Reviewed-by: Darrick J. Wong <djwong@kernel.org> >> --- >> fs/xfs/libxfs/xfs_attr.c | 70 +++++++++++++++++++++++++++++++++++++++++++++--- >> fs/xfs/libxfs/xfs_attr.h | 2 ++ >> fs/xfs/xfs_log.c | 41 ++++++++++++++++++++++++++++ >> fs/xfs/xfs_log.h | 1 + >> 4 files changed, 111 insertions(+), 3 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index eee219c6..c447c21 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -25,6 +25,8 @@ >> #include "xfs_trans_space.h" >> #include "xfs_trace.h" >> #include "xfs_attr_item.h" >> +#include "xfs_attr.h" >> +#include "xfs_log.h" >> >> /* >> * xfs_attr.c >> @@ -779,13 +781,19 @@ xfs_attr_set( >> rmt_blks = xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX); >> } >> >> + if (xfs_hasdelattr(mp)) { >> + error = xfs_attr_use_log_assist(mp); >> + if (error) >> + return error; >> + } >> + >> /* >> * Root fork attributes can use reserved data blocks for this >> * operation if necessary >> */ >> error = xfs_trans_alloc_inode(dp, &tres, total, 0, rsvd, &args->trans); >> if (error) >> - return error; >> + goto drop_incompat; >> >> if (args->value || xfs_inode_hasattr(dp)) { >> error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK, >> @@ -803,9 +811,10 @@ xfs_attr_set( >> if (error != -ENOATTR && error != -EEXIST) >> goto out_trans_cancel; >> >> - error = xfs_attr_set_args(args); >> + error = xfs_attr_set_deferred(args); >> if (error) >> goto out_trans_cancel; >> + >> /* shortform attribute has already been committed */ >> if (!args->trans) >> goto out_unlock; >> @@ -814,7 +823,7 @@ xfs_attr_set( >> if (error != -EEXIST) >> goto out_trans_cancel; >> >> - error = xfs_attr_remove_args(args); >> + error = xfs_attr_remove_deferred(args); >> if (error) >> goto out_trans_cancel; >> } >> @@ -836,6 +845,9 @@ xfs_attr_set( >> error = xfs_trans_commit(args->trans); >> out_unlock: >> xfs_iunlock(dp, XFS_ILOCK_EXCL); >> +drop_incompat: >> + if (xfs_hasdelattr(mp)) > > Subtle race here: if you turn this into a debug option and someone > changes the value from 1 to 0 while this function is running, we'll fail > to call _drop_incompat here. You should sample the value at the start > of the function into a local variable and use that throughout. I see, will be sure to do that in the next revision then. > >> + xlog_drop_incompat_feat(mp->m_log); >> return error; >> >> out_trans_cancel: >> @@ -844,6 +856,58 @@ xfs_attr_set( >> goto out_unlock; >> } >> >> +STATIC int >> +xfs_attr_item_init( >> + struct xfs_da_args *args, >> + unsigned int op_flags, /* op flag (set or remove) */ >> + struct xfs_attr_item **attr) /* new xfs_attr_item */ >> +{ >> + >> + struct xfs_attr_item *new; >> + >> + new = kmem_zalloc(sizeof(struct xfs_attr_item), KM_NOFS); >> + new->xattri_op_flags = op_flags; >> + new->xattri_dac.da_args = args; >> + >> + *attr = new; >> + return 0; >> +} >> + >> +/* Sets an attribute for an inode as a deferred operation */ >> +int >> +xfs_attr_set_deferred( >> + struct xfs_da_args *args) >> +{ >> + struct xfs_attr_item *new; >> + int error = 0; >> + >> + error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new); >> + if (error) >> + return error; >> + >> + xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list); >> + >> + return 0; >> +} >> + >> +/* Removes an attribute for an inode as a deferred operation */ >> +int >> +xfs_attr_remove_deferred( >> + struct xfs_da_args *args) >> +{ >> + >> + struct xfs_attr_item *new; >> + int error; >> + >> + error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE, &new); >> + if (error) >> + return error; >> + >> + xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list); >> + >> + return 0; >> +} >> + >> /*======================================================================== >> * External routines when attribute list is inside the inode >> *========================================================================*/ >> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h >> index 463b2be..72b0ea5 100644 >> --- a/fs/xfs/libxfs/xfs_attr.h >> +++ b/fs/xfs/libxfs/xfs_attr.h >> @@ -527,5 +527,7 @@ bool xfs_attr_namecheck(const void *name, size_t length); >> void xfs_delattr_context_init(struct xfs_delattr_context *dac, >> struct xfs_da_args *args); >> int xfs_attr_calc_size(struct xfs_da_args *args, int *local); >> +int xfs_attr_set_deferred(struct xfs_da_args *args); >> +int xfs_attr_remove_deferred(struct xfs_da_args *args); >> >> #endif /* __XFS_ATTR_H__ */ >> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c >> index 7c593d9..216de6c 100644 >> --- a/fs/xfs/xfs_log.c >> +++ b/fs/xfs/xfs_log.c >> @@ -3948,3 +3948,44 @@ xlog_drop_incompat_feat( >> { >> up_read(&log->l_incompat_users); >> } >> + >> +/* >> + * Get permission to use log-assisted atomic exchange of file extents. >> + * >> + * Callers must not be running any transactions or hold any inode locks, and >> + * they must release the permission by calling xlog_drop_incompat_feat >> + * when they're done. >> + */ >> +int >> +xfs_attr_use_log_assist( >> + struct xfs_mount *mp) >> +{ >> + int error = 0; >> + >> + /* >> + * Protect ourselves from an idle log clearing the logged xattrs log >> + * incompat feature bit. >> + */ >> + xlog_use_incompat_feat(mp->m_log); >> + >> + /* >> + * If log-assisted xattrs are already enabled, the caller can use the >> + * log assisted swap functions with the log-incompat reference we got. >> + */ >> + if (xfs_sb_version_hasdelattr(&mp->m_sb)) >> + return 0; >> + >> + /* Enable log-assisted xattrs. */ >> + xfs_warn_once(mp, >> +"EXPERIMENTAL logged extended attributes feature added. Use at your own risk!"); >> + >> + error = xfs_add_incompat_log_feature(mp, >> + XFS_SB_FEAT_INCOMPAT_LOG_DELATTR); >> + if (error) >> + goto drop_incompat; > > Minor bug here (and yes, it's also in the swapext code that I pointed > you to; apologies for that): We shouldn't print the warning unless the > feature addition actually succeeds. IOWs, move the xfs_warn_once() > here. No worries, will move the warning down here then. Thanks for the reviews! Allison > > (That said, if the feature add fails then the fs is probably going to > crash soon anyway so it probably doesn't matter...) > > --D > >> + >> + return 0; >> +drop_incompat: >> + xlog_drop_incompat_feat(mp->m_log); >> + return error; >> +} >> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h >> index b274fb9..1e461671 100644 >> --- a/fs/xfs/xfs_log.h >> +++ b/fs/xfs/xfs_log.h >> @@ -144,5 +144,6 @@ xfs_lsn_t xlog_grant_push_threshold(struct xlog *log, int need_bytes); >> >> void xlog_use_incompat_feat(struct xlog *log); >> void xlog_drop_incompat_feat(struct xlog *log); >> +int xfs_attr_use_log_assist(struct xfs_mount *mp); >> >> #endif /* __XFS_LOG_H__ */ >> -- >> 2.7.4 >>
On 7/30/21 7:58 AM, Chandan Babu R wrote: > On 27 Jul 2021 at 11:50, Allison Henderson wrote: >> From: Allison Collins <allison.henderson@oracle.com> >> >> These routines set up and queue a new deferred attribute operations. >> These functions are meant to be called by any routine needing to >> initiate a deferred attribute operation as opposed to the existing >> inline operations. New helper function xfs_attr_item_init also added. >> >> Finally enable delayed attributes in xfs_attr_set and xfs_attr_remove. > > Apart from the issues pointed out by Darrick, the remaining changes seem to be > fine. Alrighty! Thanks! Allison > >> >> Signed-off-by: Allison Henderson <allison.henderson@oracle.com> >> Reviewed-by: Darrick J. Wong <djwong@kernel.org> >> --- >> fs/xfs/libxfs/xfs_attr.c | 70 +++++++++++++++++++++++++++++++++++++++++++++--- >> fs/xfs/libxfs/xfs_attr.h | 2 ++ >> fs/xfs/xfs_log.c | 41 ++++++++++++++++++++++++++++ >> fs/xfs/xfs_log.h | 1 + >> 4 files changed, 111 insertions(+), 3 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index eee219c6..c447c21 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -25,6 +25,8 @@ >> #include "xfs_trans_space.h" >> #include "xfs_trace.h" >> #include "xfs_attr_item.h" >> +#include "xfs_attr.h" >> +#include "xfs_log.h" >> >> /* >> * xfs_attr.c >> @@ -779,13 +781,19 @@ xfs_attr_set( >> rmt_blks = xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX); >> } >> >> + if (xfs_hasdelattr(mp)) { >> + error = xfs_attr_use_log_assist(mp); >> + if (error) >> + return error; >> + } >> + >> /* >> * Root fork attributes can use reserved data blocks for this >> * operation if necessary >> */ >> error = xfs_trans_alloc_inode(dp, &tres, total, 0, rsvd, &args->trans); >> if (error) >> - return error; >> + goto drop_incompat; >> >> if (args->value || xfs_inode_hasattr(dp)) { >> error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK, >> @@ -803,9 +811,10 @@ xfs_attr_set( >> if (error != -ENOATTR && error != -EEXIST) >> goto out_trans_cancel; >> >> - error = xfs_attr_set_args(args); >> + error = xfs_attr_set_deferred(args); >> if (error) >> goto out_trans_cancel; >> + >> /* shortform attribute has already been committed */ >> if (!args->trans) >> goto out_unlock; >> @@ -814,7 +823,7 @@ xfs_attr_set( >> if (error != -EEXIST) >> goto out_trans_cancel; >> >> - error = xfs_attr_remove_args(args); >> + error = xfs_attr_remove_deferred(args); >> if (error) >> goto out_trans_cancel; >> } >> @@ -836,6 +845,9 @@ xfs_attr_set( >> error = xfs_trans_commit(args->trans); >> out_unlock: >> xfs_iunlock(dp, XFS_ILOCK_EXCL); >> +drop_incompat: >> + if (xfs_hasdelattr(mp)) >> + xlog_drop_incompat_feat(mp->m_log); >> return error; >> >> out_trans_cancel: >> @@ -844,6 +856,58 @@ xfs_attr_set( >> goto out_unlock; >> } >> >> +STATIC int >> +xfs_attr_item_init( >> + struct xfs_da_args *args, >> + unsigned int op_flags, /* op flag (set or remove) */ >> + struct xfs_attr_item **attr) /* new xfs_attr_item */ >> +{ >> + >> + struct xfs_attr_item *new; >> + >> + new = kmem_zalloc(sizeof(struct xfs_attr_item), KM_NOFS); >> + new->xattri_op_flags = op_flags; >> + new->xattri_dac.da_args = args; >> + >> + *attr = new; >> + return 0; >> +} >> + >> +/* Sets an attribute for an inode as a deferred operation */ >> +int >> +xfs_attr_set_deferred( >> + struct xfs_da_args *args) >> +{ >> + struct xfs_attr_item *new; >> + int error = 0; >> + >> + error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new); >> + if (error) >> + return error; >> + >> + xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list); >> + >> + return 0; >> +} >> + >> +/* Removes an attribute for an inode as a deferred operation */ >> +int >> +xfs_attr_remove_deferred( >> + struct xfs_da_args *args) >> +{ >> + >> + struct xfs_attr_item *new; >> + int error; >> + >> + error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE, &new); >> + if (error) >> + return error; >> + >> + xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list); >> + >> + return 0; >> +} >> + >> /*======================================================================== >> * External routines when attribute list is inside the inode >> *========================================================================*/ >> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h >> index 463b2be..72b0ea5 100644 >> --- a/fs/xfs/libxfs/xfs_attr.h >> +++ b/fs/xfs/libxfs/xfs_attr.h >> @@ -527,5 +527,7 @@ bool xfs_attr_namecheck(const void *name, size_t length); >> void xfs_delattr_context_init(struct xfs_delattr_context *dac, >> struct xfs_da_args *args); >> int xfs_attr_calc_size(struct xfs_da_args *args, int *local); >> +int xfs_attr_set_deferred(struct xfs_da_args *args); >> +int xfs_attr_remove_deferred(struct xfs_da_args *args); >> >> #endif /* __XFS_ATTR_H__ */ >> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c >> index 7c593d9..216de6c 100644 >> --- a/fs/xfs/xfs_log.c >> +++ b/fs/xfs/xfs_log.c >> @@ -3948,3 +3948,44 @@ xlog_drop_incompat_feat( >> { >> up_read(&log->l_incompat_users); >> } >> + >> +/* >> + * Get permission to use log-assisted atomic exchange of file extents. >> + * >> + * Callers must not be running any transactions or hold any inode locks, and >> + * they must release the permission by calling xlog_drop_incompat_feat >> + * when they're done. >> + */ >> +int >> +xfs_attr_use_log_assist( >> + struct xfs_mount *mp) >> +{ >> + int error = 0; >> + >> + /* >> + * Protect ourselves from an idle log clearing the logged xattrs log >> + * incompat feature bit. >> + */ >> + xlog_use_incompat_feat(mp->m_log); >> + >> + /* >> + * If log-assisted xattrs are already enabled, the caller can use the >> + * log assisted swap functions with the log-incompat reference we got. >> + */ >> + if (xfs_sb_version_hasdelattr(&mp->m_sb)) >> + return 0; >> + >> + /* Enable log-assisted xattrs. */ >> + xfs_warn_once(mp, >> +"EXPERIMENTAL logged extended attributes feature added. Use at your own risk!"); >> + >> + error = xfs_add_incompat_log_feature(mp, >> + XFS_SB_FEAT_INCOMPAT_LOG_DELATTR); >> + if (error) >> + goto drop_incompat; >> + >> + return 0; >> +drop_incompat: >> + xlog_drop_incompat_feat(mp->m_log); >> + return error; >> +} >> diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h >> index b274fb9..1e461671 100644 >> --- a/fs/xfs/xfs_log.h >> +++ b/fs/xfs/xfs_log.h >> @@ -144,5 +144,6 @@ xfs_lsn_t xlog_grant_push_threshold(struct xlog *log, int need_bytes); >> >> void xlog_use_incompat_feat(struct xlog *log); >> void xlog_drop_incompat_feat(struct xlog *log); >> +int xfs_attr_use_log_assist(struct xfs_mount *mp); >> >> #endif /* __XFS_LOG_H__ */ > > > -- > chandan >
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index eee219c6..c447c21 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -25,6 +25,8 @@ #include "xfs_trans_space.h" #include "xfs_trace.h" #include "xfs_attr_item.h" +#include "xfs_attr.h" +#include "xfs_log.h" /* * xfs_attr.c @@ -779,13 +781,19 @@ xfs_attr_set( rmt_blks = xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX); } + if (xfs_hasdelattr(mp)) { + error = xfs_attr_use_log_assist(mp); + if (error) + return error; + } + /* * Root fork attributes can use reserved data blocks for this * operation if necessary */ error = xfs_trans_alloc_inode(dp, &tres, total, 0, rsvd, &args->trans); if (error) - return error; + goto drop_incompat; if (args->value || xfs_inode_hasattr(dp)) { error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK, @@ -803,9 +811,10 @@ xfs_attr_set( if (error != -ENOATTR && error != -EEXIST) goto out_trans_cancel; - error = xfs_attr_set_args(args); + error = xfs_attr_set_deferred(args); if (error) goto out_trans_cancel; + /* shortform attribute has already been committed */ if (!args->trans) goto out_unlock; @@ -814,7 +823,7 @@ xfs_attr_set( if (error != -EEXIST) goto out_trans_cancel; - error = xfs_attr_remove_args(args); + error = xfs_attr_remove_deferred(args); if (error) goto out_trans_cancel; } @@ -836,6 +845,9 @@ xfs_attr_set( error = xfs_trans_commit(args->trans); out_unlock: xfs_iunlock(dp, XFS_ILOCK_EXCL); +drop_incompat: + if (xfs_hasdelattr(mp)) + xlog_drop_incompat_feat(mp->m_log); return error; out_trans_cancel: @@ -844,6 +856,58 @@ xfs_attr_set( goto out_unlock; } +STATIC int +xfs_attr_item_init( + struct xfs_da_args *args, + unsigned int op_flags, /* op flag (set or remove) */ + struct xfs_attr_item **attr) /* new xfs_attr_item */ +{ + + struct xfs_attr_item *new; + + new = kmem_zalloc(sizeof(struct xfs_attr_item), KM_NOFS); + new->xattri_op_flags = op_flags; + new->xattri_dac.da_args = args; + + *attr = new; + return 0; +} + +/* Sets an attribute for an inode as a deferred operation */ +int +xfs_attr_set_deferred( + struct xfs_da_args *args) +{ + struct xfs_attr_item *new; + int error = 0; + + error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_SET, &new); + if (error) + return error; + + xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list); + + return 0; +} + +/* Removes an attribute for an inode as a deferred operation */ +int +xfs_attr_remove_deferred( + struct xfs_da_args *args) +{ + + struct xfs_attr_item *new; + int error; + + error = xfs_attr_item_init(args, XFS_ATTR_OP_FLAGS_REMOVE, &new); + if (error) + return error; + + xfs_defer_add(args->trans, XFS_DEFER_OPS_TYPE_ATTR, &new->xattri_list); + + return 0; +} + /*======================================================================== * External routines when attribute list is inside the inode *========================================================================*/ diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h index 463b2be..72b0ea5 100644 --- a/fs/xfs/libxfs/xfs_attr.h +++ b/fs/xfs/libxfs/xfs_attr.h @@ -527,5 +527,7 @@ bool xfs_attr_namecheck(const void *name, size_t length); void xfs_delattr_context_init(struct xfs_delattr_context *dac, struct xfs_da_args *args); int xfs_attr_calc_size(struct xfs_da_args *args, int *local); +int xfs_attr_set_deferred(struct xfs_da_args *args); +int xfs_attr_remove_deferred(struct xfs_da_args *args); #endif /* __XFS_ATTR_H__ */ diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index 7c593d9..216de6c 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -3948,3 +3948,44 @@ xlog_drop_incompat_feat( { up_read(&log->l_incompat_users); } + +/* + * Get permission to use log-assisted atomic exchange of file extents. + * + * Callers must not be running any transactions or hold any inode locks, and + * they must release the permission by calling xlog_drop_incompat_feat + * when they're done. + */ +int +xfs_attr_use_log_assist( + struct xfs_mount *mp) +{ + int error = 0; + + /* + * Protect ourselves from an idle log clearing the logged xattrs log + * incompat feature bit. + */ + xlog_use_incompat_feat(mp->m_log); + + /* + * If log-assisted xattrs are already enabled, the caller can use the + * log assisted swap functions with the log-incompat reference we got. + */ + if (xfs_sb_version_hasdelattr(&mp->m_sb)) + return 0; + + /* Enable log-assisted xattrs. */ + xfs_warn_once(mp, +"EXPERIMENTAL logged extended attributes feature added. Use at your own risk!"); + + error = xfs_add_incompat_log_feature(mp, + XFS_SB_FEAT_INCOMPAT_LOG_DELATTR); + if (error) + goto drop_incompat; + + return 0; +drop_incompat: + xlog_drop_incompat_feat(mp->m_log); + return error; +} diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h index b274fb9..1e461671 100644 --- a/fs/xfs/xfs_log.h +++ b/fs/xfs/xfs_log.h @@ -144,5 +144,6 @@ xfs_lsn_t xlog_grant_push_threshold(struct xlog *log, int need_bytes); void xlog_use_incompat_feat(struct xlog *log); void xlog_drop_incompat_feat(struct xlog *log); +int xfs_attr_use_log_assist(struct xfs_mount *mp); #endif /* __XFS_LOG_H__ */