| Message ID | 1508367333-3237-2-git-send-email-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Oct 18, 2017 at 03:55:17PM -0700, Allison Henderson wrote: > These sub-routines set or remove the attributes specified in > @args. We will use this later for setting parent pointers as a > deferred attribute operation. > > Signed-off-by: Allison Henderson <allison.henderson@oracle.com> > --- > fs/xfs/libxfs/xfs_attr.c | 322 +++++++++++++++++++++++++++-------------------- > fs/xfs/xfs_attr.h | 2 + > 2 files changed, 189 insertions(+), 135 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 6249c92..b00ec1f 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -203,6 +203,185 @@ xfs_attr_calc_size( > return nblks; > } > > +/* > + * set the attribute specified in @args. In the case of the parent attribute > + * being set, we do not want to roll the transaction on shortform-to-leaf > + * conversion, as the attribute must be added in the same transaction as the > + * parent directory modifications. Hence @roll_trans needs to be set > + * appropriately to control whether the transaction is committed during this > + * function. Hmm... shouldn't the deferred attribute set code take care of all the conversions/attr fork expansions/whatever is necessary to cram in the parent pointer? So in theory all the parent pointer updates should look like this: xfs_defer_init(&dfops...); xfs_some_name_creating_operation(tp, ...); if (hasparent) { xfs_parent_set(tp, ip); } xfs_defer_finish(&tp, &dfops); xfs_trans_commit(tp); xfs_parent_set() would then be: if (fits in inode) { xfs_attr_set_sf(ip, key, value); xfs_log_attr_area(); return; } xfs_attr_do_conversions_if_needed(dfops); xfs_attr_set(dfops); ...and if there's a really good reason to try to cram things in, we can add those later? As I scanned the series, that was what kept coming up in my head -- just tell the xfs_parent.c code to set a parent pointer; it can figure out if there's sufficient space to put it directly into the inode and log that, or we need something else that actually requires the deferred ops mechanism then do that. (Maybe I'm just fixating on xfs_parent_create...) Higher level questions about robustness: if we try to set a parent ptr and the attr name already exists, do we error out? If we try to remove a parent ptr and there's no attr name, do we error out? I think you've enough here to start thinking what changes need to be made to xfs_repair to validate & fix the parent pointers, and what test cases (and possibly ioctls) will need to be constructed to verify that we actually get the parent pointers we want. --D > + */ > +int > +xfs_attr_set_args( > + struct xfs_da_args *args, > + int flags, > + bool roll_trans) > +{ > + struct xfs_inode *dp = args->dp; > + struct xfs_mount *mp = dp->i_mount; > + struct xfs_trans_res tres; > + int rsvd = 0; > + int error = 0; > + > + tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres + > + M_RES(mp)->tr_attrsetrt.tr_logres * args->total; > + tres.tr_logcount = XFS_ATTRSET_LOG_COUNT; > + tres.tr_logflags = XFS_TRANS_PERM_LOG_RES; > + > + /* > + * Root fork attributes can use reserved data blocks for this > + * operation if necessary > + */ > + error = xfs_trans_alloc(mp, &tres, args->total, 0, > + rsvd ? XFS_TRANS_RESERVE : 0, &args->trans); > + if (error) > + goto out; > + > + error = xfs_trans_reserve_quota_nblks(args->trans, dp, args->total, 0, > + rsvd ? XFS_QMOPT_RES_REGBLKS | > + XFS_QMOPT_FORCE_RES : > + XFS_QMOPT_RES_REGBLKS); > + if (error) > + goto out; > + > + xfs_trans_ijoin(args->trans, dp, 0); > + /* > + * If the attribute list is non-existent or a shortform list, > + * upgrade it to a single-leaf-block attribute list. > + */ > + if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || > + (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && > + 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_shortform_addname(args); > + if (error != -ENOSPC) { > + ASSERT(args->trans); > + if (!error && (flags & ATTR_KERNOTIME) == 0) > + xfs_trans_ichgtime(args->trans, dp, > + XFS_ICHGTIME_CHG); > + goto out; > + } > + > + /* > + * It won't fit in the shortform, transform to a leaf block. > + * GROT: another possible req'mt for a double-split btree op. > + */ > + error = xfs_attr_shortform_to_leaf(args); > + if (error) > + goto out; > + xfs_defer_ijoin(args->dfops, dp); > + if (roll_trans) { > + error = xfs_defer_finish(&args->trans, args->dfops); > + if (error) { > + args->trans = NULL; > + goto out; > + } > + > + /* > + * Commit the leaf transformation. We'll need another > + * (linked) transaction to add the new attribute to the > + * leaf. > + */ > + error = xfs_trans_roll_inode(&args->trans, dp); > + if (error) > + goto out; > + } > + } > + > + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) > + error = xfs_attr_leaf_addname(args); > + else > + error = xfs_attr_node_addname(args); > + if (error) > + goto out; > + > + if ((flags & ATTR_KERNOTIME) == 0) > + xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG); > + > + xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE); > +out: > + return error; > +} > + > +/* > + * Remove the attribute specified in @args. > + */ > +int > +xfs_attr_remove_args( > + struct xfs_da_args *args, > + int flags) > +{ > + struct xfs_inode *dp = args->dp; > + struct xfs_mount *mp = dp->i_mount; > + int error; > + int rsvd = 0; > + > + error = xfs_qm_dqattach_locked(dp, 0); > + if (error) > + return error; > + > + /* > + * Root fork attributes can use reserved data blocks for this > + * operation if necessary > + */ > + if (flags & ATTR_ROOT) > + rsvd = XFS_TRANS_RESERVE; > + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrrm, > + XFS_ATTRRM_SPACE_RES(mp), 0, rsvd, &args->trans); > + > + if (error) > + goto out; > + > + /* > + * No need to make quota reservations here. We expect to release some > + * blocks not allocate in the common case. > + */ > + xfs_trans_ijoin(args->trans, dp, 0); > + > + if (!xfs_inode_hasattr(dp)) { > + error = -ENOATTR; > + } else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) { > + ASSERT(dp->i_afp->if_flags & XFS_IFINLINE); > + error = xfs_attr_shortform_remove(args); > + } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { > + error = xfs_attr_leaf_removename(args); > + } else { > + error = xfs_attr_node_removename(args); > + } > + > + if (error) > + goto out; > + > + /* > + * If this is a synchronous mount, make sure that the > + * transaction goes to disk before returning to the user. > + */ > + if (mp->m_flags & XFS_MOUNT_WSYNC) > + xfs_trans_set_sync(args->trans); > + > + if ((flags & ATTR_KERNOTIME) == 0) > + xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG); > + > + xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE); > + > + return error; > + > +out: > + if (args->trans) > + xfs_trans_cancel(args->trans); > + > + return error; > +} > + > int > xfs_attr_set( > struct xfs_inode *dp, > @@ -214,10 +393,9 @@ xfs_attr_set( > struct xfs_mount *mp = dp->i_mount; > struct xfs_da_args args; > struct xfs_defer_ops dfops; > - struct xfs_trans_res tres; > xfs_fsblock_t firstblock; > int rsvd = (flags & ATTR_ROOT) != 0; > - int error, err2, local; > + int error, local; > > XFS_STATS_INC(mp, xs_attr_set); > > @@ -252,106 +430,11 @@ xfs_attr_set( > return error; > } > > - tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres + > - M_RES(mp)->tr_attrsetrt.tr_logres * args.total; > - tres.tr_logcount = XFS_ATTRSET_LOG_COUNT; > - tres.tr_logflags = XFS_TRANS_PERM_LOG_RES; > - > - /* > - * Root fork attributes can use reserved data blocks for this > - * operation if necessary > - */ > - error = xfs_trans_alloc(mp, &tres, args.total, 0, > - rsvd ? XFS_TRANS_RESERVE : 0, &args.trans); > - if (error) > - return error; > - > xfs_ilock(dp, XFS_ILOCK_EXCL); > - error = xfs_trans_reserve_quota_nblks(args.trans, dp, args.total, 0, > - rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES : > - XFS_QMOPT_RES_REGBLKS); > - if (error) { > - xfs_iunlock(dp, XFS_ILOCK_EXCL); > - xfs_trans_cancel(args.trans); > - return error; > - } > - > - xfs_trans_ijoin(args.trans, dp, 0); > - > - /* > - * If the attribute list is non-existent or a shortform list, > - * upgrade it to a single-leaf-block attribute list. > - */ > - if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || > - (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && > - 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_shortform_addname(&args); > - if (error != -ENOSPC) { > - /* > - * Commit the shortform mods, and we're done. > - * NOTE: this is also the error path (EEXIST, etc). > - */ > - ASSERT(args.trans != NULL); > - > - /* > - * If this is a synchronous mount, make sure that > - * the transaction goes to disk before returning > - * to the user. > - */ > - if (mp->m_flags & XFS_MOUNT_WSYNC) > - xfs_trans_set_sync(args.trans); > - > - if (!error && (flags & ATTR_KERNOTIME) == 0) { > - xfs_trans_ichgtime(args.trans, dp, > - XFS_ICHGTIME_CHG); > - } > - err2 = xfs_trans_commit(args.trans); > - xfs_iunlock(dp, XFS_ILOCK_EXCL); > - > - return error ? error : err2; > - } > - > - /* > - * It won't fit in the shortform, transform to a leaf block. > - * GROT: another possible req'mt for a double-split btree op. > - */ > - xfs_defer_init(args.dfops, args.firstblock); > - error = xfs_attr_shortform_to_leaf(&args); > - if (error) > - goto out_defer_cancel; > - xfs_defer_ijoin(args.dfops, dp); > - error = xfs_defer_finish(&args.trans, args.dfops); > - if (error) > - goto out_defer_cancel; > - > - /* > - * Commit the leaf transformation. We'll need another (linked) > - * transaction to add the new attribute to the leaf. > - */ > - > - error = xfs_trans_roll_inode(&args.trans, dp); > - if (error) > - goto out; > - > - } > - > - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) > - error = xfs_attr_leaf_addname(&args); > - else > - error = xfs_attr_node_addname(&args); > + xfs_defer_init(args.dfops, args.firstblock); > + error = xfs_attr_set_args(&args, flags, true); > if (error) > - goto out; > + goto out_defer_cancel; > > /* > * If this is a synchronous mount, make sure that the > @@ -360,9 +443,6 @@ xfs_attr_set( > if (mp->m_flags & XFS_MOUNT_WSYNC) > xfs_trans_set_sync(args.trans); > > - if ((flags & ATTR_KERNOTIME) == 0) > - xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG); > - > /* > * Commit the last in the sequence of transactions. > */ > @@ -374,10 +454,6 @@ xfs_attr_set( > > out_defer_cancel: > xfs_defer_cancel(&dfops); > - args.trans = NULL; > -out: > - if (args.trans) > - xfs_trans_cancel(args.trans); > xfs_iunlock(dp, XFS_ILOCK_EXCL); > return error; > } > @@ -417,38 +493,15 @@ xfs_attr_remove( > */ > args.op_flags = XFS_DA_OP_OKNOENT; > > - error = xfs_qm_dqattach(dp, 0); > - if (error) > - return error; > - > - /* > - * Root fork attributes can use reserved data blocks for this > - * operation if necessary > - */ > - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrrm, > - XFS_ATTRRM_SPACE_RES(mp), 0, > - (flags & ATTR_ROOT) ? XFS_TRANS_RESERVE : 0, > - &args.trans); > - if (error) > - return error; > - > xfs_ilock(dp, XFS_ILOCK_EXCL); > /* > * No need to make quota reservations here. We expect to release some > * blocks not allocate in the common case. > */ > xfs_trans_ijoin(args.trans, dp, 0); > + xfs_defer_init(args.dfops, args.firstblock); > > - if (!xfs_inode_hasattr(dp)) { > - error = -ENOATTR; > - } else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) { > - ASSERT(dp->i_afp->if_flags & XFS_IFINLINE); > - error = xfs_attr_shortform_remove(&args); > - } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { > - error = xfs_attr_leaf_removename(&args); > - } else { > - error = xfs_attr_node_removename(&args); > - } > + error = xfs_attr_remove_args(&args, flags); > > if (error) > goto out; > @@ -460,9 +513,6 @@ xfs_attr_remove( > if (mp->m_flags & XFS_MOUNT_WSYNC) > xfs_trans_set_sync(args.trans); > > - if ((flags & ATTR_KERNOTIME) == 0) > - xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG); > - > /* > * Commit the last in the sequence of transactions. > */ > @@ -473,6 +523,8 @@ xfs_attr_remove( > return error; > > out: > + xfs_defer_cancel(&dfops); > + > if (args.trans) > xfs_trans_cancel(args.trans); > xfs_iunlock(dp, XFS_ILOCK_EXCL); > diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h > index 5d5a5e2..8542606 100644 > --- a/fs/xfs/xfs_attr.h > +++ b/fs/xfs/xfs_attr.h > @@ -149,7 +149,9 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name, > unsigned char *value, int *valuelenp, int flags); > int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name, > unsigned char *value, int valuelen, int flags); > +int xfs_attr_set_args(struct xfs_da_args *args, int flags, bool roll_trans); > int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags); > +int xfs_attr_remove_args(struct xfs_da_args *args, int flags); > int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize, > int flags, struct attrlist_cursor_kern *cursor); > > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/19/2017 01:03 PM, Darrick J. Wong wrote: > On Wed, Oct 18, 2017 at 03:55:17PM -0700, Allison Henderson wrote: >> These sub-routines set or remove the attributes specified in >> @args. We will use this later for setting parent pointers as a >> deferred attribute operation. >> >> Signed-off-by: Allison Henderson<allison.henderson@oracle.com> >> --- >> fs/xfs/libxfs/xfs_attr.c | 322 +++++++++++++++++++++++++++-------------------- >> fs/xfs/xfs_attr.h | 2 + >> 2 files changed, 189 insertions(+), 135 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 6249c92..b00ec1f 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -203,6 +203,185 @@ xfs_attr_calc_size( >> return nblks; >> } >> >> +/* >> + * set the attribute specified in @args. In the case of the parent attribute >> + * being set, we do not want to roll the transaction on shortform-to-leaf >> + * conversion, as the attribute must be added in the same transaction as the >> + * parent directory modifications. Hence @roll_trans needs to be set >> + * appropriately to control whether the transaction is committed during this >> + * function. > Hmm... shouldn't the deferred attribute set code take care of all the > conversions/attr fork expansions/whatever is necessary to cram in the > parent pointer? So in theory all the parent pointer updates should look > like this: > > xfs_defer_init(&dfops...); > xfs_some_name_creating_operation(tp, ...); > > if (hasparent) { > xfs_parent_set(tp, ip); > } > xfs_defer_finish(&tp, &dfops); > xfs_trans_commit(tp); > > xfs_parent_set() would then be: > > if (fits in inode) { > xfs_attr_set_sf(ip, key, value); > xfs_log_attr_area(); > return; > } > > xfs_attr_do_conversions_if_needed(dfops); > xfs_attr_set(dfops); > > ...and if there's a really good reason to try to cram things in, we can > add those later? As I scanned the series, that was what kept coming up > in my head -- just tell the xfs_parent.c code to set a parent pointer; > it can figure out if there's sufficient space to put it directly into > the inode and log that, or we need something else that actually requires > the deferred ops mechanism then do that. > > (Maybe I'm just fixating on xfs_parent_create...) Ok, I think may have caused some confusion by not revising the comments for these functions. xfs_attr_remove_args and xfs_attr_set_args are sort of subsets of code that came out of xfs_attr_remove and xfs_attr_set. In the first revision of the set, the link/unlink/rename operations called them directly to set up the parent pointers. Now these two functions are called through xfs_trans_attr. But you've still made very good points because I forgot that xfs_create path uses xfs_parent_create not xfs_parent_add. So I think the pseudo code you are proposing makes sense. I will see if I can revise the create path a bit. Thanks for the catch! > Higher level questions about robustness: if we try to set a parent > ptr and the attr name already exists, do we error out? If we try to > remove a parent ptr and there's no attr name, do we error out? Erroring out seems logical sense these these would not be normal conditions? > I think > you've enough here to start thinking what changes need to be made to > xfs_repair to validate & fix the parent pointers, and what test cases > (and possibly ioctls) will need to be constructed to verify that we > actually get the parent pointers we want. > > --D Right, I will put together some basic add/remove/rename tests to add to xfstests. Thank you for the very thorough review!! >> + */ >> +int >> +xfs_attr_set_args( >> + struct xfs_da_args *args, >> + int flags, >> + bool roll_trans) >> +{ >> + struct xfs_inode *dp = args->dp; >> + struct xfs_mount *mp = dp->i_mount; >> + struct xfs_trans_res tres; >> + int rsvd = 0; >> + int error = 0; >> + >> + tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres + >> + M_RES(mp)->tr_attrsetrt.tr_logres * args->total; >> + tres.tr_logcount = XFS_ATTRSET_LOG_COUNT; >> + tres.tr_logflags = XFS_TRANS_PERM_LOG_RES; >> + >> + /* >> + * Root fork attributes can use reserved data blocks for this >> + * operation if necessary >> + */ >> + error = xfs_trans_alloc(mp, &tres, args->total, 0, >> + rsvd ? XFS_TRANS_RESERVE : 0, &args->trans); >> + if (error) >> + goto out; >> + >> + error = xfs_trans_reserve_quota_nblks(args->trans, dp, args->total, 0, >> + rsvd ? XFS_QMOPT_RES_REGBLKS | >> + XFS_QMOPT_FORCE_RES : >> + XFS_QMOPT_RES_REGBLKS); >> + if (error) >> + goto out; >> + >> + xfs_trans_ijoin(args->trans, dp, 0); >> + /* >> + * If the attribute list is non-existent or a shortform list, >> + * upgrade it to a single-leaf-block attribute list. >> + */ >> + if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || >> + (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && >> + 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_shortform_addname(args); >> + if (error != -ENOSPC) { >> + ASSERT(args->trans); >> + if (!error && (flags & ATTR_KERNOTIME) == 0) >> + xfs_trans_ichgtime(args->trans, dp, >> + XFS_ICHGTIME_CHG); >> + goto out; >> + } >> + >> + /* >> + * It won't fit in the shortform, transform to a leaf block. >> + * GROT: another possible req'mt for a double-split btree op. >> + */ >> + error = xfs_attr_shortform_to_leaf(args); >> + if (error) >> + goto out; >> + xfs_defer_ijoin(args->dfops, dp); >> + if (roll_trans) { >> + error = xfs_defer_finish(&args->trans, args->dfops); >> + if (error) { >> + args->trans = NULL; >> + goto out; >> + } >> + >> + /* >> + * Commit the leaf transformation. We'll need another >> + * (linked) transaction to add the new attribute to the >> + * leaf. >> + */ >> + error = xfs_trans_roll_inode(&args->trans, dp); >> + if (error) >> + goto out; >> + } >> + } >> + >> + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) >> + error = xfs_attr_leaf_addname(args); >> + else >> + error = xfs_attr_node_addname(args); >> + if (error) >> + goto out; >> + >> + if ((flags & ATTR_KERNOTIME) == 0) >> + xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG); >> + >> + xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE); >> +out: >> + return error; >> +} >> + >> +/* >> + * Remove the attribute specified in @args. >> + */ >> +int >> +xfs_attr_remove_args( >> + struct xfs_da_args *args, >> + int flags) >> +{ >> + struct xfs_inode *dp = args->dp; >> + struct xfs_mount *mp = dp->i_mount; >> + int error; >> + int rsvd = 0; >> + >> + error = xfs_qm_dqattach_locked(dp, 0); >> + if (error) >> + return error; >> + >> + /* >> + * Root fork attributes can use reserved data blocks for this >> + * operation if necessary >> + */ >> + if (flags & ATTR_ROOT) >> + rsvd = XFS_TRANS_RESERVE; >> + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrrm, >> + XFS_ATTRRM_SPACE_RES(mp), 0, rsvd, &args->trans); >> + >> + if (error) >> + goto out; >> + >> + /* >> + * No need to make quota reservations here. We expect to release some >> + * blocks not allocate in the common case. >> + */ >> + xfs_trans_ijoin(args->trans, dp, 0); >> + >> + if (!xfs_inode_hasattr(dp)) { >> + error = -ENOATTR; >> + } else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) { >> + ASSERT(dp->i_afp->if_flags & XFS_IFINLINE); >> + error = xfs_attr_shortform_remove(args); >> + } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { >> + error = xfs_attr_leaf_removename(args); >> + } else { >> + error = xfs_attr_node_removename(args); >> + } >> + >> + if (error) >> + goto out; >> + >> + /* >> + * If this is a synchronous mount, make sure that the >> + * transaction goes to disk before returning to the user. >> + */ >> + if (mp->m_flags & XFS_MOUNT_WSYNC) >> + xfs_trans_set_sync(args->trans); >> + >> + if ((flags & ATTR_KERNOTIME) == 0) >> + xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG); >> + >> + xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE); >> + >> + return error; >> + >> +out: >> + if (args->trans) >> + xfs_trans_cancel(args->trans); >> + >> + return error; >> +} >> + >> int >> xfs_attr_set( >> struct xfs_inode *dp, >> @@ -214,10 +393,9 @@ xfs_attr_set( >> struct xfs_mount *mp = dp->i_mount; >> struct xfs_da_args args; >> struct xfs_defer_ops dfops; >> - struct xfs_trans_res tres; >> xfs_fsblock_t firstblock; >> int rsvd = (flags & ATTR_ROOT) != 0; >> - int error, err2, local; >> + int error, local; >> >> XFS_STATS_INC(mp, xs_attr_set); >> >> @@ -252,106 +430,11 @@ xfs_attr_set( >> return error; >> } >> >> - tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres + >> - M_RES(mp)->tr_attrsetrt.tr_logres * args.total; >> - tres.tr_logcount = XFS_ATTRSET_LOG_COUNT; >> - tres.tr_logflags = XFS_TRANS_PERM_LOG_RES; >> - >> - /* >> - * Root fork attributes can use reserved data blocks for this >> - * operation if necessary >> - */ >> - error = xfs_trans_alloc(mp, &tres, args.total, 0, >> - rsvd ? XFS_TRANS_RESERVE : 0, &args.trans); >> - if (error) >> - return error; >> - >> xfs_ilock(dp, XFS_ILOCK_EXCL); >> - error = xfs_trans_reserve_quota_nblks(args.trans, dp, args.total, 0, >> - rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES : >> - XFS_QMOPT_RES_REGBLKS); >> - if (error) { >> - xfs_iunlock(dp, XFS_ILOCK_EXCL); >> - xfs_trans_cancel(args.trans); >> - return error; >> - } >> - >> - xfs_trans_ijoin(args.trans, dp, 0); >> - >> - /* >> - * If the attribute list is non-existent or a shortform list, >> - * upgrade it to a single-leaf-block attribute list. >> - */ >> - if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || >> - (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && >> - 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_shortform_addname(&args); >> - if (error != -ENOSPC) { >> - /* >> - * Commit the shortform mods, and we're done. >> - * NOTE: this is also the error path (EEXIST, etc). >> - */ >> - ASSERT(args.trans != NULL); >> - >> - /* >> - * If this is a synchronous mount, make sure that >> - * the transaction goes to disk before returning >> - * to the user. >> - */ >> - if (mp->m_flags & XFS_MOUNT_WSYNC) >> - xfs_trans_set_sync(args.trans); >> - >> - if (!error && (flags & ATTR_KERNOTIME) == 0) { >> - xfs_trans_ichgtime(args.trans, dp, >> - XFS_ICHGTIME_CHG); >> - } >> - err2 = xfs_trans_commit(args.trans); >> - xfs_iunlock(dp, XFS_ILOCK_EXCL); >> - >> - return error ? error : err2; >> - } >> - >> - /* >> - * It won't fit in the shortform, transform to a leaf block. >> - * GROT: another possible req'mt for a double-split btree op. >> - */ >> - xfs_defer_init(args.dfops, args.firstblock); >> - error = xfs_attr_shortform_to_leaf(&args); >> - if (error) >> - goto out_defer_cancel; >> - xfs_defer_ijoin(args.dfops, dp); >> - error = xfs_defer_finish(&args.trans, args.dfops); >> - if (error) >> - goto out_defer_cancel; >> - >> - /* >> - * Commit the leaf transformation. We'll need another (linked) >> - * transaction to add the new attribute to the leaf. >> - */ >> - >> - error = xfs_trans_roll_inode(&args.trans, dp); >> - if (error) >> - goto out; >> - >> - } >> - >> - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) >> - error = xfs_attr_leaf_addname(&args); >> - else >> - error = xfs_attr_node_addname(&args); >> + xfs_defer_init(args.dfops, args.firstblock); >> + error = xfs_attr_set_args(&args, flags, true); >> if (error) >> - goto out; >> + goto out_defer_cancel; >> >> /* >> * If this is a synchronous mount, make sure that the >> @@ -360,9 +443,6 @@ xfs_attr_set( >> if (mp->m_flags & XFS_MOUNT_WSYNC) >> xfs_trans_set_sync(args.trans); >> >> - if ((flags & ATTR_KERNOTIME) == 0) >> - xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG); >> - >> /* >> * Commit the last in the sequence of transactions. >> */ >> @@ -374,10 +454,6 @@ xfs_attr_set( >> >> out_defer_cancel: >> xfs_defer_cancel(&dfops); >> - args.trans = NULL; >> -out: >> - if (args.trans) >> - xfs_trans_cancel(args.trans); >> xfs_iunlock(dp, XFS_ILOCK_EXCL); >> return error; >> } >> @@ -417,38 +493,15 @@ xfs_attr_remove( >> */ >> args.op_flags = XFS_DA_OP_OKNOENT; >> >> - error = xfs_qm_dqattach(dp, 0); >> - if (error) >> - return error; >> - >> - /* >> - * Root fork attributes can use reserved data blocks for this >> - * operation if necessary >> - */ >> - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrrm, >> - XFS_ATTRRM_SPACE_RES(mp), 0, >> - (flags & ATTR_ROOT) ? XFS_TRANS_RESERVE : 0, >> - &args.trans); >> - if (error) >> - return error; >> - >> xfs_ilock(dp, XFS_ILOCK_EXCL); >> /* >> * No need to make quota reservations here. We expect to release some >> * blocks not allocate in the common case. >> */ >> xfs_trans_ijoin(args.trans, dp, 0); >> + xfs_defer_init(args.dfops, args.firstblock); >> >> - if (!xfs_inode_hasattr(dp)) { >> - error = -ENOATTR; >> - } else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) { >> - ASSERT(dp->i_afp->if_flags & XFS_IFINLINE); >> - error = xfs_attr_shortform_remove(&args); >> - } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { >> - error = xfs_attr_leaf_removename(&args); >> - } else { >> - error = xfs_attr_node_removename(&args); >> - } >> + error = xfs_attr_remove_args(&args, flags); >> >> if (error) >> goto out; >> @@ -460,9 +513,6 @@ xfs_attr_remove( >> if (mp->m_flags & XFS_MOUNT_WSYNC) >> xfs_trans_set_sync(args.trans); >> >> - if ((flags & ATTR_KERNOTIME) == 0) >> - xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG); >> - >> /* >> * Commit the last in the sequence of transactions. >> */ >> @@ -473,6 +523,8 @@ xfs_attr_remove( >> return error; >> >> out: >> + xfs_defer_cancel(&dfops); >> + >> if (args.trans) >> xfs_trans_cancel(args.trans); >> xfs_iunlock(dp, XFS_ILOCK_EXCL); >> diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h >> index 5d5a5e2..8542606 100644 >> --- a/fs/xfs/xfs_attr.h >> +++ b/fs/xfs/xfs_attr.h >> @@ -149,7 +149,9 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name, >> unsigned char *value, int *valuelenp, int flags); >> int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name, >> unsigned char *value, int valuelen, int flags); >> +int xfs_attr_set_args(struct xfs_da_args *args, int flags, bool roll_trans); >> int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags); >> +int xfs_attr_remove_args(struct xfs_da_args *args, int flags); >> int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize, >> int flags, struct attrlist_cursor_kern *cursor); >> >> -- >> 2.7.4 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in >> the body of a message tomajordomo@vger.kernel.org >> More majordomo info athttps://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=XFp4B05bcXkJ0dhYaFjd3F8telP01COkBp9cI7mKLb4&m=aicitOc3doix8VyB_l-FSSD7nPvIu7TIiw0VVfKnBhc&s=JKZ0ozAg9jwyNpJK-9-53aT_kpKIedFceJDGJpDQh3U&e= > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message tomajordomo@vger.kernel.org > More majordomo info athttps://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=XFp4B05bcXkJ0dhYaFjd3F8telP01COkBp9cI7mKLb4&m=aicitOc3doix8VyB_l-FSSD7nPvIu7TIiw0VVfKnBhc&s=JKZ0ozAg9jwyNpJK-9-53aT_kpKIedFceJDGJpDQh3U&e= -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 6249c92..b00ec1f 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -203,6 +203,185 @@ xfs_attr_calc_size( return nblks; } +/* + * set the attribute specified in @args. In the case of the parent attribute + * being set, we do not want to roll the transaction on shortform-to-leaf + * conversion, as the attribute must be added in the same transaction as the + * parent directory modifications. Hence @roll_trans needs to be set + * appropriately to control whether the transaction is committed during this + * function. + */ +int +xfs_attr_set_args( + struct xfs_da_args *args, + int flags, + bool roll_trans) +{ + struct xfs_inode *dp = args->dp; + struct xfs_mount *mp = dp->i_mount; + struct xfs_trans_res tres; + int rsvd = 0; + int error = 0; + + tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres + + M_RES(mp)->tr_attrsetrt.tr_logres * args->total; + tres.tr_logcount = XFS_ATTRSET_LOG_COUNT; + tres.tr_logflags = XFS_TRANS_PERM_LOG_RES; + + /* + * Root fork attributes can use reserved data blocks for this + * operation if necessary + */ + error = xfs_trans_alloc(mp, &tres, args->total, 0, + rsvd ? XFS_TRANS_RESERVE : 0, &args->trans); + if (error) + goto out; + + error = xfs_trans_reserve_quota_nblks(args->trans, dp, args->total, 0, + rsvd ? XFS_QMOPT_RES_REGBLKS | + XFS_QMOPT_FORCE_RES : + XFS_QMOPT_RES_REGBLKS); + if (error) + goto out; + + xfs_trans_ijoin(args->trans, dp, 0); + /* + * If the attribute list is non-existent or a shortform list, + * upgrade it to a single-leaf-block attribute list. + */ + if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || + (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && + 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_shortform_addname(args); + if (error != -ENOSPC) { + ASSERT(args->trans); + if (!error && (flags & ATTR_KERNOTIME) == 0) + xfs_trans_ichgtime(args->trans, dp, + XFS_ICHGTIME_CHG); + goto out; + } + + /* + * It won't fit in the shortform, transform to a leaf block. + * GROT: another possible req'mt for a double-split btree op. + */ + error = xfs_attr_shortform_to_leaf(args); + if (error) + goto out; + xfs_defer_ijoin(args->dfops, dp); + if (roll_trans) { + error = xfs_defer_finish(&args->trans, args->dfops); + if (error) { + args->trans = NULL; + goto out; + } + + /* + * Commit the leaf transformation. We'll need another + * (linked) transaction to add the new attribute to the + * leaf. + */ + error = xfs_trans_roll_inode(&args->trans, dp); + if (error) + goto out; + } + } + + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) + error = xfs_attr_leaf_addname(args); + else + error = xfs_attr_node_addname(args); + if (error) + goto out; + + if ((flags & ATTR_KERNOTIME) == 0) + xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG); + + xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE); +out: + return error; +} + +/* + * Remove the attribute specified in @args. + */ +int +xfs_attr_remove_args( + struct xfs_da_args *args, + int flags) +{ + struct xfs_inode *dp = args->dp; + struct xfs_mount *mp = dp->i_mount; + int error; + int rsvd = 0; + + error = xfs_qm_dqattach_locked(dp, 0); + if (error) + return error; + + /* + * Root fork attributes can use reserved data blocks for this + * operation if necessary + */ + if (flags & ATTR_ROOT) + rsvd = XFS_TRANS_RESERVE; + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrrm, + XFS_ATTRRM_SPACE_RES(mp), 0, rsvd, &args->trans); + + if (error) + goto out; + + /* + * No need to make quota reservations here. We expect to release some + * blocks not allocate in the common case. + */ + xfs_trans_ijoin(args->trans, dp, 0); + + if (!xfs_inode_hasattr(dp)) { + error = -ENOATTR; + } else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) { + ASSERT(dp->i_afp->if_flags & XFS_IFINLINE); + error = xfs_attr_shortform_remove(args); + } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { + error = xfs_attr_leaf_removename(args); + } else { + error = xfs_attr_node_removename(args); + } + + if (error) + goto out; + + /* + * If this is a synchronous mount, make sure that the + * transaction goes to disk before returning to the user. + */ + if (mp->m_flags & XFS_MOUNT_WSYNC) + xfs_trans_set_sync(args->trans); + + if ((flags & ATTR_KERNOTIME) == 0) + xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG); + + xfs_trans_log_inode(args->trans, dp, XFS_ILOG_CORE); + + return error; + +out: + if (args->trans) + xfs_trans_cancel(args->trans); + + return error; +} + int xfs_attr_set( struct xfs_inode *dp, @@ -214,10 +393,9 @@ xfs_attr_set( struct xfs_mount *mp = dp->i_mount; struct xfs_da_args args; struct xfs_defer_ops dfops; - struct xfs_trans_res tres; xfs_fsblock_t firstblock; int rsvd = (flags & ATTR_ROOT) != 0; - int error, err2, local; + int error, local; XFS_STATS_INC(mp, xs_attr_set); @@ -252,106 +430,11 @@ xfs_attr_set( return error; } - tres.tr_logres = M_RES(mp)->tr_attrsetm.tr_logres + - M_RES(mp)->tr_attrsetrt.tr_logres * args.total; - tres.tr_logcount = XFS_ATTRSET_LOG_COUNT; - tres.tr_logflags = XFS_TRANS_PERM_LOG_RES; - - /* - * Root fork attributes can use reserved data blocks for this - * operation if necessary - */ - error = xfs_trans_alloc(mp, &tres, args.total, 0, - rsvd ? XFS_TRANS_RESERVE : 0, &args.trans); - if (error) - return error; - xfs_ilock(dp, XFS_ILOCK_EXCL); - error = xfs_trans_reserve_quota_nblks(args.trans, dp, args.total, 0, - rsvd ? XFS_QMOPT_RES_REGBLKS | XFS_QMOPT_FORCE_RES : - XFS_QMOPT_RES_REGBLKS); - if (error) { - xfs_iunlock(dp, XFS_ILOCK_EXCL); - xfs_trans_cancel(args.trans); - return error; - } - - xfs_trans_ijoin(args.trans, dp, 0); - - /* - * If the attribute list is non-existent or a shortform list, - * upgrade it to a single-leaf-block attribute list. - */ - if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL || - (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS && - 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_shortform_addname(&args); - if (error != -ENOSPC) { - /* - * Commit the shortform mods, and we're done. - * NOTE: this is also the error path (EEXIST, etc). - */ - ASSERT(args.trans != NULL); - - /* - * If this is a synchronous mount, make sure that - * the transaction goes to disk before returning - * to the user. - */ - if (mp->m_flags & XFS_MOUNT_WSYNC) - xfs_trans_set_sync(args.trans); - - if (!error && (flags & ATTR_KERNOTIME) == 0) { - xfs_trans_ichgtime(args.trans, dp, - XFS_ICHGTIME_CHG); - } - err2 = xfs_trans_commit(args.trans); - xfs_iunlock(dp, XFS_ILOCK_EXCL); - - return error ? error : err2; - } - - /* - * It won't fit in the shortform, transform to a leaf block. - * GROT: another possible req'mt for a double-split btree op. - */ - xfs_defer_init(args.dfops, args.firstblock); - error = xfs_attr_shortform_to_leaf(&args); - if (error) - goto out_defer_cancel; - xfs_defer_ijoin(args.dfops, dp); - error = xfs_defer_finish(&args.trans, args.dfops); - if (error) - goto out_defer_cancel; - - /* - * Commit the leaf transformation. We'll need another (linked) - * transaction to add the new attribute to the leaf. - */ - - error = xfs_trans_roll_inode(&args.trans, dp); - if (error) - goto out; - - } - - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) - error = xfs_attr_leaf_addname(&args); - else - error = xfs_attr_node_addname(&args); + xfs_defer_init(args.dfops, args.firstblock); + error = xfs_attr_set_args(&args, flags, true); if (error) - goto out; + goto out_defer_cancel; /* * If this is a synchronous mount, make sure that the @@ -360,9 +443,6 @@ xfs_attr_set( if (mp->m_flags & XFS_MOUNT_WSYNC) xfs_trans_set_sync(args.trans); - if ((flags & ATTR_KERNOTIME) == 0) - xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG); - /* * Commit the last in the sequence of transactions. */ @@ -374,10 +454,6 @@ xfs_attr_set( out_defer_cancel: xfs_defer_cancel(&dfops); - args.trans = NULL; -out: - if (args.trans) - xfs_trans_cancel(args.trans); xfs_iunlock(dp, XFS_ILOCK_EXCL); return error; } @@ -417,38 +493,15 @@ xfs_attr_remove( */ args.op_flags = XFS_DA_OP_OKNOENT; - error = xfs_qm_dqattach(dp, 0); - if (error) - return error; - - /* - * Root fork attributes can use reserved data blocks for this - * operation if necessary - */ - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_attrrm, - XFS_ATTRRM_SPACE_RES(mp), 0, - (flags & ATTR_ROOT) ? XFS_TRANS_RESERVE : 0, - &args.trans); - if (error) - return error; - xfs_ilock(dp, XFS_ILOCK_EXCL); /* * No need to make quota reservations here. We expect to release some * blocks not allocate in the common case. */ xfs_trans_ijoin(args.trans, dp, 0); + xfs_defer_init(args.dfops, args.firstblock); - if (!xfs_inode_hasattr(dp)) { - error = -ENOATTR; - } else if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) { - ASSERT(dp->i_afp->if_flags & XFS_IFINLINE); - error = xfs_attr_shortform_remove(&args); - } else if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) { - error = xfs_attr_leaf_removename(&args); - } else { - error = xfs_attr_node_removename(&args); - } + error = xfs_attr_remove_args(&args, flags); if (error) goto out; @@ -460,9 +513,6 @@ xfs_attr_remove( if (mp->m_flags & XFS_MOUNT_WSYNC) xfs_trans_set_sync(args.trans); - if ((flags & ATTR_KERNOTIME) == 0) - xfs_trans_ichgtime(args.trans, dp, XFS_ICHGTIME_CHG); - /* * Commit the last in the sequence of transactions. */ @@ -473,6 +523,8 @@ xfs_attr_remove( return error; out: + xfs_defer_cancel(&dfops); + if (args.trans) xfs_trans_cancel(args.trans); xfs_iunlock(dp, XFS_ILOCK_EXCL); diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h index 5d5a5e2..8542606 100644 --- a/fs/xfs/xfs_attr.h +++ b/fs/xfs/xfs_attr.h @@ -149,7 +149,9 @@ int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name, unsigned char *value, int *valuelenp, int flags); int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name, unsigned char *value, int valuelen, int flags); +int xfs_attr_set_args(struct xfs_da_args *args, int flags, bool roll_trans); int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags); +int xfs_attr_remove_args(struct xfs_da_args *args, int flags); int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize, int flags, struct attrlist_cursor_kern *cursor);
These sub-routines set or remove the attributes specified in @args. We will use this later for setting parent pointers as a deferred attribute operation. Signed-off-by: Allison Henderson <allison.henderson@oracle.com> --- fs/xfs/libxfs/xfs_attr.c | 322 +++++++++++++++++++++++++++-------------------- fs/xfs/xfs_attr.h | 2 + 2 files changed, 189 insertions(+), 135 deletions(-)