| Message ID | 1525627494-12873-4-git-send-email-allison.henderson@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, May 06, 2018 at 10:24:36AM -0700, Allison Henderson wrote: > This patch adds xfs_attr_set_args and xfs_bmap_set_attrforkoff. > These sub-routines set 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 | 217 ++++++++++++++++++++++++++++------------------- > fs/xfs/libxfs/xfs_attr.h | 2 + > fs/xfs/libxfs/xfs_bmap.c | 49 ++++++----- > fs/xfs/libxfs/xfs_bmap.h | 1 + > 4 files changed, 165 insertions(+), 104 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c > index 0ade22b..99c4a31 100644 > --- a/fs/xfs/libxfs/xfs_attr.c > +++ b/fs/xfs/libxfs/xfs_attr.c > @@ -168,6 +168,134 @@ xfs_attr_get( > } > > /* > + * 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, > + struct xfs_buf *leaf_bp, > + bool roll_trans) > +{ > + struct xfs_inode *dp = args->dp; > + struct xfs_mount *mp = dp->i_mount; > + int error = 0; > + int err2 = 0; > + int sf_size; > + > + /* > + * New inodes setting the parent pointer attr will > + * not have an attribute fork yet. So set the attribute > + * fork appropriately > + */ > + if (XFS_IFORK_Q((args->dp)) == 0) { > + sf_size = sizeof(struct xfs_attr_sf_hdr) + > + XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen); > + xfs_bmap_set_attrforkoff(args->dp, sf_size, NULL); > + args->dp->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP); > + args->dp->i_afp->if_flags = XFS_IFEXTENTS; > + } > + > + 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, roll_trans); > + if (error != -ENOSPC) { > + if (roll_trans) { I dislike this roll_trans parameter. Most other places in xfs when a function is passed in a defer_ops or a transaction it's assumed that we don't own the transaction or the defer_ops and so while it's ok to attach dirty things to the dfops or the tp, we let the caller decide when it's appropriate to start committing things. This function is getting rather long and indenty, can it be broken up into smaller pieces? That should make it easier to reuse the core logic of "try to stuff it in the sfattr, if it doesn't fit then convert to attr block and retry the add" without having to add extra parameters to control whether or not we commit transactions. This is more complex than in other parts of xfs because we're (for the moment anyway) leaving both the deferred and non-deferred paths, but at least the attr logic and the transaction management logic should be split into separate functions to handle the unique situations of both the deferred and non-deferred xattr setting code. Also, please don't hoist code into a helper function /and/ change its behavior & parameters in the same patch. --D > + /* > + * 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); > + error = error ? error : err2; > + } > + 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, &leaf_bp); > + if (error) > + goto out; > + > + xfs_defer_bjoin(args->dfops, leaf_bp); > + xfs_defer_ijoin(args->dfops, dp); > + if (roll_trans) { > + /* > + * Prevent the leaf buffer from being unlocked so that a > + * concurrent AIL push cannot grab the half-baked leaf > + * buffer and run into problems with the write verifier. > + */ > + xfs_trans_bhold(args->trans, leaf_bp); > + > + 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; > + xfs_defer_ijoin(args->dfops, dp); > + xfs_trans_bjoin(args->trans, leaf_bp); > + leaf_bp = NULL; > + } > + } > + > + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) > + error = xfs_attr_leaf_addname(args, roll_trans); > + else > + error = xfs_attr_node_addname(args, roll_trans); > + if (error) > + goto out; > + > +out: > + return error; > +} > + > +/* > * Calculate how many blocks we need for the new attribute, > */ > STATIC int > @@ -218,7 +346,7 @@ xfs_attr_set( > 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); > > @@ -279,88 +407,11 @@ xfs_attr_set( > > 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, true); > - 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, &leaf_bp); > - if (error) > - goto out_defer_cancel; > - /* > - * Prevent the leaf buffer from being unlocked so that a > - * concurrent AIL push cannot grab the half-baked leaf > - * buffer and run into problems with the write verifier. > - */ > - xfs_trans_bhold(args.trans, leaf_bp); > - xfs_defer_bjoin(args.dfops, leaf_bp); > - 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, which > - * means that we have to hold & join the leaf buffer here too. > - */ > - error = xfs_trans_roll_inode(&args.trans, dp); > - if (error) > - goto out; > - xfs_trans_bjoin(args.trans, leaf_bp); > - leaf_bp = NULL; > - } > + xfs_defer_init(args.dfops, args.firstblock); > + error = xfs_attr_set_args(&args, flags, leaf_bp, true); > > - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) > - error = xfs_attr_leaf_addname(&args, true); > - else > - error = xfs_attr_node_addname(&args, true); > if (error) > - goto out; > + goto out_defer_cancel; > > /* > * If this is a synchronous mount, make sure that the > @@ -369,9 +420,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. > */ > @@ -383,7 +431,6 @@ xfs_attr_set( > > out_defer_cancel: > xfs_defer_cancel(&dfops); > -out: > if (leaf_bp) > xfs_trans_brelse(args.trans, leaf_bp); > if (args.trans) > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > index d07bf27..b5dc02c 100644 > --- a/fs/xfs/libxfs/xfs_attr.h > +++ b/fs/xfs/libxfs/xfs_attr.h > @@ -152,6 +152,8 @@ 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, > + struct xfs_buf *leaf_bp, bool roll_trans); > int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags); > int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize, > int flags, struct attrlist_cursor_kern *cursor); > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 6a7c2f0..4e16a5d 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -1031,6 +1031,34 @@ xfs_bmap_add_attrfork_local( > return -EFSCORRUPTED; > } > > +/* Set an inode attr fork off based on the format */ > +int > +xfs_bmap_set_attrforkoff( > + struct xfs_inode *ip, > + int size, > + int *version) > +{ > + switch (ip->i_d.di_format) { > + case XFS_DINODE_FMT_DEV: > + ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3; > + break; > + case XFS_DINODE_FMT_LOCAL: > + case XFS_DINODE_FMT_EXTENTS: > + case XFS_DINODE_FMT_BTREE: > + ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size); > + if (!ip->i_d.di_forkoff) > + ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3; > + else if ((ip->i_mount->m_flags & XFS_MOUNT_ATTR2) && version) > + *version = 2; > + break; > + default: > + ASSERT(0); > + return -EINVAL; > + } > + > + return 0; > +} > + > /* > * Convert inode from non-attributed to attributed. > * Must not be in a transaction, ip must not be locked. > @@ -1084,26 +1112,9 @@ xfs_bmap_add_attrfork( > > xfs_trans_ijoin(tp, ip, 0); > xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); > - > - switch (ip->i_d.di_format) { > - case XFS_DINODE_FMT_DEV: > - ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3; > - break; > - case XFS_DINODE_FMT_LOCAL: > - case XFS_DINODE_FMT_EXTENTS: > - case XFS_DINODE_FMT_BTREE: > - ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size); > - if (!ip->i_d.di_forkoff) > - ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3; > - else if (mp->m_flags & XFS_MOUNT_ATTR2) > - version = 2; > - break; > - default: > - ASSERT(0); > - error = -EINVAL; > + error = xfs_bmap_set_attrforkoff(ip, size, &version); > + if (error) > goto trans_cancel; > - } > - > ASSERT(ip->i_afp == NULL); > ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP); > ip->i_afp->if_flags = XFS_IFEXTENTS; > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h > index 2b766b3..50e9115 100644 > --- a/fs/xfs/libxfs/xfs_bmap.h > +++ b/fs/xfs/libxfs/xfs_bmap.h > @@ -191,6 +191,7 @@ void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno, > xfs_filblks_t len); > void xfs_trim_extent_eof(struct xfs_bmbt_irec *, struct xfs_inode *); > int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd); > +int xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version); > void xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork); > void xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops, > xfs_fsblock_t bno, xfs_filblks_t len, > -- > 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 Tue, May 8, 2018 at 2:36 AM, Darrick J. Wong <darrick.wong@oracle.com> wrote: > On Sun, May 06, 2018 at 10:24:36AM -0700, Allison Henderson wrote: >> This patch adds xfs_attr_set_args and xfs_bmap_set_attrforkoff. >> These sub-routines set 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 | 217 ++++++++++++++++++++++++++++------------------- >> fs/xfs/libxfs/xfs_attr.h | 2 + >> fs/xfs/libxfs/xfs_bmap.c | 49 ++++++----- >> fs/xfs/libxfs/xfs_bmap.h | 1 + >> 4 files changed, 165 insertions(+), 104 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 0ade22b..99c4a31 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -168,6 +168,134 @@ xfs_attr_get( >> } >> >> /* >> + * 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, >> + struct xfs_buf *leaf_bp, >> + bool roll_trans) >> +{ >> + struct xfs_inode *dp = args->dp; >> + struct xfs_mount *mp = dp->i_mount; >> + int error = 0; >> + int err2 = 0; >> + int sf_size; >> + >> + /* >> + * New inodes setting the parent pointer attr will >> + * not have an attribute fork yet. So set the attribute >> + * fork appropriately >> + */ >> + if (XFS_IFORK_Q((args->dp)) == 0) { >> + sf_size = sizeof(struct xfs_attr_sf_hdr) + >> + XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen); >> + xfs_bmap_set_attrforkoff(args->dp, sf_size, NULL); >> + args->dp->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP); >> + args->dp->i_afp->if_flags = XFS_IFEXTENTS; >> + } >> + >> + 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, roll_trans); >> + if (error != -ENOSPC) { >> + if (roll_trans) { > > I dislike this roll_trans parameter. Most other places in xfs when a > function is passed in a defer_ops or a transaction it's assumed that we > don't own the transaction or the defer_ops and so while it's ok to > attach dirty things to the dfops or the tp, we let the caller decide > when it's appropriate to start committing things. > > This function is getting rather long and indenty, can it be broken up > into smaller pieces? That should make it easier to reuse the core > logic of "try to stuff it in the sfattr, if it doesn't fit then convert > to attr block and retry the add" without having to add extra parameters > to control whether or not we commit transactions. > > This is more complex than in other parts of xfs because we're (for the > moment anyway) leaving both the deferred and non-deferred paths, but at > least the attr logic and the transaction management logic should be > split into separate functions to handle the unique situations of both > the deferred and non-deferred xattr setting code. > > Also, please don't hoist code into a helper function /and/ change its > behavior & parameters in the same patch. > Indeed. I was going to comment that the description should say "factor out helper" and "doesn't change logic" so reviewers can review it properly, although now I am not sure if that is really the case, so please make it the case. Thanks, Amir. -- 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 05/07/2018 04:36 PM, Darrick J. Wong wrote: > On Sun, May 06, 2018 at 10:24:36AM -0700, Allison Henderson wrote: >> This patch adds xfs_attr_set_args and xfs_bmap_set_attrforkoff. >> These sub-routines set 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 | 217 ++++++++++++++++++++++++++++------------------- >> fs/xfs/libxfs/xfs_attr.h | 2 + >> fs/xfs/libxfs/xfs_bmap.c | 49 ++++++----- >> fs/xfs/libxfs/xfs_bmap.h | 1 + >> 4 files changed, 165 insertions(+), 104 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >> index 0ade22b..99c4a31 100644 >> --- a/fs/xfs/libxfs/xfs_attr.c >> +++ b/fs/xfs/libxfs/xfs_attr.c >> @@ -168,6 +168,134 @@ xfs_attr_get( >> } >> >> /* >> + * 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, >> + struct xfs_buf *leaf_bp, >> + bool roll_trans) >> +{ >> + struct xfs_inode *dp = args->dp; >> + struct xfs_mount *mp = dp->i_mount; >> + int error = 0; >> + int err2 = 0; >> + int sf_size; >> + >> + /* >> + * New inodes setting the parent pointer attr will >> + * not have an attribute fork yet. So set the attribute >> + * fork appropriately >> + */ >> + if (XFS_IFORK_Q((args->dp)) == 0) { >> + sf_size = sizeof(struct xfs_attr_sf_hdr) + >> + XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen); >> + xfs_bmap_set_attrforkoff(args->dp, sf_size, NULL); >> + args->dp->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP); >> + args->dp->i_afp->if_flags = XFS_IFEXTENTS; >> + } >> + >> + 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, roll_trans); >> + if (error != -ENOSPC) { >> + if (roll_trans) { > > I dislike this roll_trans parameter. Most other places in xfs when a > function is passed in a defer_ops or a transaction it's assumed that we > don't own the transaction or the defer_ops and so while it's ok to > attach dirty things to the dfops or the tp, we let the caller decide > when it's appropriate to start committing things. > > This function is getting rather long and indenty, can it be broken up > into smaller pieces? That should make it easier to reuse the core > logic of "try to stuff it in the sfattr, if it doesn't fit then convert > to attr block and retry the add" without having to add extra parameters > to control whether or not we commit transactions. > > This is more complex than in other parts of xfs because we're (for the > moment anyway) leaving both the deferred and non-deferred paths, but at > least the attr logic and the transaction management logic should be > split into separate functions to handle the unique situations of both > the deferred and non-deferred xattr setting code. > > Also, please don't hoist code into a helper function /and/ change its > behavior & parameters in the same patch. Sure, I'll see if I can split it up a little more to make it easier to follow. Sorry, about the hoist and hijack... a lot of this comes from collecting fixes on top of the set, and then moving them down to an appropriate patch. The roll_trans and related probably should have gone to the set below. I think there's more discussion concerning roll_trans in the next patch, so I'll jump that way.... > > --D > >> + /* >> + * 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); >> + error = error ? error : err2; >> + } >> + 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, &leaf_bp); >> + if (error) >> + goto out; >> + >> + xfs_defer_bjoin(args->dfops, leaf_bp); >> + xfs_defer_ijoin(args->dfops, dp); >> + if (roll_trans) { >> + /* >> + * Prevent the leaf buffer from being unlocked so that a >> + * concurrent AIL push cannot grab the half-baked leaf >> + * buffer and run into problems with the write verifier. >> + */ >> + xfs_trans_bhold(args->trans, leaf_bp); >> + >> + 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; >> + xfs_defer_ijoin(args->dfops, dp); >> + xfs_trans_bjoin(args->trans, leaf_bp); >> + leaf_bp = NULL; >> + } >> + } >> + >> + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) >> + error = xfs_attr_leaf_addname(args, roll_trans); >> + else >> + error = xfs_attr_node_addname(args, roll_trans); >> + if (error) >> + goto out; >> + >> +out: >> + return error; >> +} >> + >> +/* >> * Calculate how many blocks we need for the new attribute, >> */ >> STATIC int >> @@ -218,7 +346,7 @@ xfs_attr_set( >> 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); >> >> @@ -279,88 +407,11 @@ xfs_attr_set( >> >> 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, true); >> - 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, &leaf_bp); >> - if (error) >> - goto out_defer_cancel; >> - /* >> - * Prevent the leaf buffer from being unlocked so that a >> - * concurrent AIL push cannot grab the half-baked leaf >> - * buffer and run into problems with the write verifier. >> - */ >> - xfs_trans_bhold(args.trans, leaf_bp); >> - xfs_defer_bjoin(args.dfops, leaf_bp); >> - 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, which >> - * means that we have to hold & join the leaf buffer here too. >> - */ >> - error = xfs_trans_roll_inode(&args.trans, dp); >> - if (error) >> - goto out; >> - xfs_trans_bjoin(args.trans, leaf_bp); >> - leaf_bp = NULL; >> - } >> + xfs_defer_init(args.dfops, args.firstblock); >> + error = xfs_attr_set_args(&args, flags, leaf_bp, true); >> >> - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) >> - error = xfs_attr_leaf_addname(&args, true); >> - else >> - error = xfs_attr_node_addname(&args, true); >> if (error) >> - goto out; >> + goto out_defer_cancel; >> >> /* >> * If this is a synchronous mount, make sure that the >> @@ -369,9 +420,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. >> */ >> @@ -383,7 +431,6 @@ xfs_attr_set( >> >> out_defer_cancel: >> xfs_defer_cancel(&dfops); >> -out: >> if (leaf_bp) >> xfs_trans_brelse(args.trans, leaf_bp); >> if (args.trans) >> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h >> index d07bf27..b5dc02c 100644 >> --- a/fs/xfs/libxfs/xfs_attr.h >> +++ b/fs/xfs/libxfs/xfs_attr.h >> @@ -152,6 +152,8 @@ 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, >> + struct xfs_buf *leaf_bp, bool roll_trans); >> int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags); >> int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize, >> int flags, struct attrlist_cursor_kern *cursor); >> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c >> index 6a7c2f0..4e16a5d 100644 >> --- a/fs/xfs/libxfs/xfs_bmap.c >> +++ b/fs/xfs/libxfs/xfs_bmap.c >> @@ -1031,6 +1031,34 @@ xfs_bmap_add_attrfork_local( >> return -EFSCORRUPTED; >> } >> >> +/* Set an inode attr fork off based on the format */ >> +int >> +xfs_bmap_set_attrforkoff( >> + struct xfs_inode *ip, >> + int size, >> + int *version) >> +{ >> + switch (ip->i_d.di_format) { >> + case XFS_DINODE_FMT_DEV: >> + ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3; >> + break; >> + case XFS_DINODE_FMT_LOCAL: >> + case XFS_DINODE_FMT_EXTENTS: >> + case XFS_DINODE_FMT_BTREE: >> + ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size); >> + if (!ip->i_d.di_forkoff) >> + ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3; >> + else if ((ip->i_mount->m_flags & XFS_MOUNT_ATTR2) && version) >> + *version = 2; >> + break; >> + default: >> + ASSERT(0); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> /* >> * Convert inode from non-attributed to attributed. >> * Must not be in a transaction, ip must not be locked. >> @@ -1084,26 +1112,9 @@ xfs_bmap_add_attrfork( >> >> xfs_trans_ijoin(tp, ip, 0); >> xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); >> - >> - switch (ip->i_d.di_format) { >> - case XFS_DINODE_FMT_DEV: >> - ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3; >> - break; >> - case XFS_DINODE_FMT_LOCAL: >> - case XFS_DINODE_FMT_EXTENTS: >> - case XFS_DINODE_FMT_BTREE: >> - ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size); >> - if (!ip->i_d.di_forkoff) >> - ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3; >> - else if (mp->m_flags & XFS_MOUNT_ATTR2) >> - version = 2; >> - break; >> - default: >> - ASSERT(0); >> - error = -EINVAL; >> + error = xfs_bmap_set_attrforkoff(ip, size, &version); >> + if (error) >> goto trans_cancel; >> - } >> - >> ASSERT(ip->i_afp == NULL); >> ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP); >> ip->i_afp->if_flags = XFS_IFEXTENTS; >> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h >> index 2b766b3..50e9115 100644 >> --- a/fs/xfs/libxfs/xfs_bmap.h >> +++ b/fs/xfs/libxfs/xfs_bmap.h >> @@ -191,6 +191,7 @@ void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno, >> xfs_filblks_t len); >> void xfs_trim_extent_eof(struct xfs_bmbt_irec *, struct xfs_inode *); >> int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd); >> +int xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version); >> void xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork); >> void xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops, >> xfs_fsblock_t bno, xfs_filblks_t len, >> -- >> 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 05/08/2018 12:25 AM, Amir Goldstein wrote: > On Tue, May 8, 2018 at 2:36 AM, Darrick J. Wong <darrick.wong@oracle.com> wrote: >> On Sun, May 06, 2018 at 10:24:36AM -0700, Allison Henderson wrote: >>> This patch adds xfs_attr_set_args and xfs_bmap_set_attrforkoff. >>> These sub-routines set 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 | 217 ++++++++++++++++++++++++++++------------------- >>> fs/xfs/libxfs/xfs_attr.h | 2 + >>> fs/xfs/libxfs/xfs_bmap.c | 49 ++++++----- >>> fs/xfs/libxfs/xfs_bmap.h | 1 + >>> 4 files changed, 165 insertions(+), 104 deletions(-) >>> >>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c >>> index 0ade22b..99c4a31 100644 >>> --- a/fs/xfs/libxfs/xfs_attr.c >>> +++ b/fs/xfs/libxfs/xfs_attr.c >>> @@ -168,6 +168,134 @@ xfs_attr_get( >>> } >>> >>> /* >>> + * 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, >>> + struct xfs_buf *leaf_bp, >>> + bool roll_trans) >>> +{ >>> + struct xfs_inode *dp = args->dp; >>> + struct xfs_mount *mp = dp->i_mount; >>> + int error = 0; >>> + int err2 = 0; >>> + int sf_size; >>> + >>> + /* >>> + * New inodes setting the parent pointer attr will >>> + * not have an attribute fork yet. So set the attribute >>> + * fork appropriately >>> + */ >>> + if (XFS_IFORK_Q((args->dp)) == 0) { >>> + sf_size = sizeof(struct xfs_attr_sf_hdr) + >>> + XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen); >>> + xfs_bmap_set_attrforkoff(args->dp, sf_size, NULL); >>> + args->dp->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP); >>> + args->dp->i_afp->if_flags = XFS_IFEXTENTS; >>> + } >>> + >>> + 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, roll_trans); >>> + if (error != -ENOSPC) { >>> + if (roll_trans) { >> >> I dislike this roll_trans parameter. Most other places in xfs when a >> function is passed in a defer_ops or a transaction it's assumed that we >> don't own the transaction or the defer_ops and so while it's ok to >> attach dirty things to the dfops or the tp, we let the caller decide >> when it's appropriate to start committing things. >> >> This function is getting rather long and indenty, can it be broken up >> into smaller pieces? That should make it easier to reuse the core >> logic of "try to stuff it in the sfattr, if it doesn't fit then convert >> to attr block and retry the add" without having to add extra parameters >> to control whether or not we commit transactions. >> >> This is more complex than in other parts of xfs because we're (for the >> moment anyway) leaving both the deferred and non-deferred paths, but at >> least the attr logic and the transaction management logic should be >> split into separate functions to handle the unique situations of both >> the deferred and non-deferred xattr setting code. >> >> Also, please don't hoist code into a helper function /and/ change its >> behavior & parameters in the same patch. >> > > Indeed. I was going to comment that the description should say > "factor out helper" and "doesn't change logic" so reviewers can > review it properly, although now I am not sure if that is really the > case, so please make it the case. > > Thanks, > Amir. > Sorry about that. Yes, the roll_trans logic should have gone to the patch below, leaving this one just a refactor. Will fix. Thx! :-) Allison -- 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 0ade22b..99c4a31 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -168,6 +168,134 @@ xfs_attr_get( } /* + * 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, + struct xfs_buf *leaf_bp, + bool roll_trans) +{ + struct xfs_inode *dp = args->dp; + struct xfs_mount *mp = dp->i_mount; + int error = 0; + int err2 = 0; + int sf_size; + + /* + * New inodes setting the parent pointer attr will + * not have an attribute fork yet. So set the attribute + * fork appropriately + */ + if (XFS_IFORK_Q((args->dp)) == 0) { + sf_size = sizeof(struct xfs_attr_sf_hdr) + + XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen); + xfs_bmap_set_attrforkoff(args->dp, sf_size, NULL); + args->dp->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP); + args->dp->i_afp->if_flags = XFS_IFEXTENTS; + } + + 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, roll_trans); + if (error != -ENOSPC) { + if (roll_trans) { + /* + * 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); + error = error ? error : err2; + } + 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, &leaf_bp); + if (error) + goto out; + + xfs_defer_bjoin(args->dfops, leaf_bp); + xfs_defer_ijoin(args->dfops, dp); + if (roll_trans) { + /* + * Prevent the leaf buffer from being unlocked so that a + * concurrent AIL push cannot grab the half-baked leaf + * buffer and run into problems with the write verifier. + */ + xfs_trans_bhold(args->trans, leaf_bp); + + 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; + xfs_defer_ijoin(args->dfops, dp); + xfs_trans_bjoin(args->trans, leaf_bp); + leaf_bp = NULL; + } + } + + if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) + error = xfs_attr_leaf_addname(args, roll_trans); + else + error = xfs_attr_node_addname(args, roll_trans); + if (error) + goto out; + +out: + return error; +} + +/* * Calculate how many blocks we need for the new attribute, */ STATIC int @@ -218,7 +346,7 @@ xfs_attr_set( 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); @@ -279,88 +407,11 @@ xfs_attr_set( 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, true); - 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, &leaf_bp); - if (error) - goto out_defer_cancel; - /* - * Prevent the leaf buffer from being unlocked so that a - * concurrent AIL push cannot grab the half-baked leaf - * buffer and run into problems with the write verifier. - */ - xfs_trans_bhold(args.trans, leaf_bp); - xfs_defer_bjoin(args.dfops, leaf_bp); - 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, which - * means that we have to hold & join the leaf buffer here too. - */ - error = xfs_trans_roll_inode(&args.trans, dp); - if (error) - goto out; - xfs_trans_bjoin(args.trans, leaf_bp); - leaf_bp = NULL; - } + xfs_defer_init(args.dfops, args.firstblock); + error = xfs_attr_set_args(&args, flags, leaf_bp, true); - if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) - error = xfs_attr_leaf_addname(&args, true); - else - error = xfs_attr_node_addname(&args, true); if (error) - goto out; + goto out_defer_cancel; /* * If this is a synchronous mount, make sure that the @@ -369,9 +420,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. */ @@ -383,7 +431,6 @@ xfs_attr_set( out_defer_cancel: xfs_defer_cancel(&dfops); -out: if (leaf_bp) xfs_trans_brelse(args.trans, leaf_bp); if (args.trans) diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h index d07bf27..b5dc02c 100644 --- a/fs/xfs/libxfs/xfs_attr.h +++ b/fs/xfs/libxfs/xfs_attr.h @@ -152,6 +152,8 @@ 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, + struct xfs_buf *leaf_bp, bool roll_trans); int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags); int xfs_attr_list(struct xfs_inode *dp, char *buffer, int bufsize, int flags, struct attrlist_cursor_kern *cursor); diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 6a7c2f0..4e16a5d 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -1031,6 +1031,34 @@ xfs_bmap_add_attrfork_local( return -EFSCORRUPTED; } +/* Set an inode attr fork off based on the format */ +int +xfs_bmap_set_attrforkoff( + struct xfs_inode *ip, + int size, + int *version) +{ + switch (ip->i_d.di_format) { + case XFS_DINODE_FMT_DEV: + ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3; + break; + case XFS_DINODE_FMT_LOCAL: + case XFS_DINODE_FMT_EXTENTS: + case XFS_DINODE_FMT_BTREE: + ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size); + if (!ip->i_d.di_forkoff) + ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3; + else if ((ip->i_mount->m_flags & XFS_MOUNT_ATTR2) && version) + *version = 2; + break; + default: + ASSERT(0); + return -EINVAL; + } + + return 0; +} + /* * Convert inode from non-attributed to attributed. * Must not be in a transaction, ip must not be locked. @@ -1084,26 +1112,9 @@ xfs_bmap_add_attrfork( xfs_trans_ijoin(tp, ip, 0); xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); - - switch (ip->i_d.di_format) { - case XFS_DINODE_FMT_DEV: - ip->i_d.di_forkoff = roundup(sizeof(xfs_dev_t), 8) >> 3; - break; - case XFS_DINODE_FMT_LOCAL: - case XFS_DINODE_FMT_EXTENTS: - case XFS_DINODE_FMT_BTREE: - ip->i_d.di_forkoff = xfs_attr_shortform_bytesfit(ip, size); - if (!ip->i_d.di_forkoff) - ip->i_d.di_forkoff = xfs_default_attroffset(ip) >> 3; - else if (mp->m_flags & XFS_MOUNT_ATTR2) - version = 2; - break; - default: - ASSERT(0); - error = -EINVAL; + error = xfs_bmap_set_attrforkoff(ip, size, &version); + if (error) goto trans_cancel; - } - ASSERT(ip->i_afp == NULL); ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP); ip->i_afp->if_flags = XFS_IFEXTENTS; diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h index 2b766b3..50e9115 100644 --- a/fs/xfs/libxfs/xfs_bmap.h +++ b/fs/xfs/libxfs/xfs_bmap.h @@ -191,6 +191,7 @@ void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno, xfs_filblks_t len); void xfs_trim_extent_eof(struct xfs_bmbt_irec *, struct xfs_inode *); int xfs_bmap_add_attrfork(struct xfs_inode *ip, int size, int rsvd); +int xfs_bmap_set_attrforkoff(struct xfs_inode *ip, int size, int *version); void xfs_bmap_local_to_extents_empty(struct xfs_inode *ip, int whichfork); void xfs_bmap_add_free(struct xfs_mount *mp, struct xfs_defer_ops *dfops, xfs_fsblock_t bno, xfs_filblks_t len,
This patch adds xfs_attr_set_args and xfs_bmap_set_attrforkoff. These sub-routines set 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 | 217 ++++++++++++++++++++++++++++------------------- fs/xfs/libxfs/xfs_attr.h | 2 + fs/xfs/libxfs/xfs_bmap.c | 49 ++++++----- fs/xfs/libxfs/xfs_bmap.h | 1 + 4 files changed, 165 insertions(+), 104 deletions(-)