On Tue, 2022-08-23 at 08:27 -0700, Darrick J. Wong wrote:
> On Tue, Aug 16, 2022 at 10:35:06AM -0700, Allison Henderson wrote:
> > This patch adds two new fields to the atti/d. They are nname and
> > nnamelen. This will be used for parent pointer updates since a
> > rename operation may cause the parent pointer to update both the
> > name and value. So we need to carry both the new name as well as
> > the target name in the attri/d.
> >
> > As discussed in the reviews, I've added a new XFS_ATTRI_OP_FLAGS*
> > type: XFS_ATTRI_OP_FLAGS_NRENAME. However, I do not think it is
>
> How about "NVREPLACE", since we're replacing the name and value?
Sure, sounds reasonable to me.
>
> > needed, since args->new_namelen is always available. I've left the
> > new type in for people to discuss, but unless we find a use for it
> > int the reviews, my recommendation would be to remove it and use
> > the existing XFS_ATTRI_OP_FLAGS_RENAME with a check for
> > args->new_namelen > 0.
>
> I think it's worth the extra bit of redundancy to encode both a new
> op
> flag and a nonzero new_namelen. xfs_attri_validate probably ought to
> check that alfi_value_len==0 if op == XFS_ATTRI_OP_FLAGS_REMOVE.
>
Alright, will add a check there
> Also -- should xlog_recover_attri_commit_pass2 be checking that
> @item->ri_total is 2 for a REMOVE, 3 for a SET or REPLACE, or 4 for
> an
> NRENAME operation to make sure that the number of log iovec items
> matches what the log item says should be there?
I suppose it wouldn't be a bad place to add a check for that
>
> I /think/ the log recovery code already sets item->ri_total to
> alfi_size, and won't recover the item if it doesn't find that number
> of
> iovecs?
I'm pretty sure it does, I seem to recall running into that while
trying to get the recovery routines working the first time
>
> > Feedback appreciated. Thanks all!
> >
> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_attr.c | 12 +++++-
> > fs/xfs/libxfs/xfs_attr.h | 4 +-
> > fs/xfs/libxfs/xfs_da_btree.h | 2 +
> > fs/xfs/libxfs/xfs_log_format.h | 5 ++-
> > fs/xfs/xfs_attr_item.c | 71 ++++++++++++++++++++++++++++
> > ------
> > fs/xfs/xfs_attr_item.h | 1 +
> > fs/xfs/xfs_ondisk.h | 2 +-
> > 7 files changed, 81 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index e28d93d232de..9f2fb4903b71 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -423,6 +423,12 @@ xfs_attr_complete_op(
> > args->op_flags &= ~XFS_DA_OP_REPLACE;
> > if (do_replace) {
> > args->attr_filter &= ~XFS_ATTR_INCOMPLETE;
> > + if (args->new_namelen > 0) {
> > + args->name = args->new_name;
> > + args->namelen = args->new_namelen;
> > + args->hashval = xfs_da_hashname(args->name,
> > + args->namelen);
> > + }
> > return replace_state;
> > }
> > return XFS_DAS_DONE;
> > @@ -922,9 +928,13 @@ xfs_attr_defer_replace(
> > struct xfs_da_args *args)
> > {
> > struct xfs_attr_intent *new;
> > + int op_flag;
> > int error = 0;
> >
> > - error = xfs_attr_intent_init(args, XFS_ATTRI_OP_FLAGS_REPLACE,
> > &new);
> > + op_flag = args->new_namelen == 0 ? XFS_ATTRI_OP_FLAGS_REPLACE :
> > + XFS_ATTRI_OP_FLAGS_NREPLACE;
> > +
> > + error = xfs_attr_intent_init(args, op_flag, &new);
> > if (error)
> > return error;
> >
> > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> > index 81be9b3e4004..3e81f3f48560 100644
> > --- a/fs/xfs/libxfs/xfs_attr.h
> > +++ b/fs/xfs/libxfs/xfs_attr.h
> > @@ -510,8 +510,8 @@ struct xfs_attr_intent {
> > struct xfs_da_args *xattri_da_args;
> >
> > /*
> > - * Shared buffer containing the attr name and value so that the
> > logging
> > - * code can share large memory buffers between log items.
> > + * Shared buffer containing the attr name, new name, and value
> > so that
> > + * the logging code can share large memory buffers between log
> > items.
> > */
> > struct xfs_attri_log_nameval *xattri_nameval;
> >
> > diff --git a/fs/xfs/libxfs/xfs_da_btree.h
> > b/fs/xfs/libxfs/xfs_da_btree.h
> > index ffa3df5b2893..e9fb801844f2 100644
> > --- a/fs/xfs/libxfs/xfs_da_btree.h
> > +++ b/fs/xfs/libxfs/xfs_da_btree.h
> > @@ -56,6 +56,8 @@ typedef struct xfs_da_args {
> > struct xfs_da_geometry *geo; /* da block geometry */
> > const uint8_t *name; /* string (maybe
> > not NULL terminated) */
> > int namelen; /* length of string (maybe no NULL)
> > */
> > + const uint8_t *new_name; /* new attr name */
> > + int new_namelen; /* new attr name len */
>
> I think the pointers and ints should go together to compact this
> structure, and possibly that the da_args should get its own slab for
> faster allocation. Neither of those cleanups should be in this
> patch.
Alrighty, I will re-arrange this and take a peek at args slabs for
another patch.
>
> > uint8_t filetype; /* filetype of inode for
> > directories */
> > void *value; /* set of bytes (maybe
> > contain NULLs) */
> > int valuelen; /* length of value */
> > diff --git a/fs/xfs/libxfs/xfs_log_format.h
> > b/fs/xfs/libxfs/xfs_log_format.h
> > index b351b9dc6561..8a22f315532c 100644
> > --- a/fs/xfs/libxfs/xfs_log_format.h
> > +++ b/fs/xfs/libxfs/xfs_log_format.h
> > @@ -117,7 +117,8 @@ struct xfs_unmount_log_format {
> > #define XLOG_REG_TYPE_ATTRD_FORMAT 28
> > #define XLOG_REG_TYPE_ATTR_NAME 29
> > #define XLOG_REG_TYPE_ATTR_VALUE 30
> > -#define XLOG_REG_TYPE_MAX 30
> > +#define XLOG_REG_TYPE_ATTR_NNAME 31
> > +#define XLOG_REG_TYPE_MAX 31
> >
> >
> > /*
> > @@ -909,6 +910,7 @@ struct xfs_icreate_log {
> > #define XFS_ATTRI_OP_FLAGS_SET 1 /* Set the
> > attribute */
> > #define XFS_ATTRI_OP_FLAGS_REMOVE 2 /* Remove the attribute */
> > #define XFS_ATTRI_OP_FLAGS_REPLACE 3 /* Replace the attribute */
> > +#define XFS_ATTRI_OP_FLAGS_NREPLACE 4 /* Replace attr
> > name and val */
> > #define XFS_ATTRI_OP_FLAGS_TYPE_MASK 0xFF /* Flags
> > type mask */
> >
> > /*
> > @@ -931,6 +933,7 @@ struct xfs_attri_log_format {
> > uint64_t alfi_ino; /* the inode for this attr
> > operation */
> > uint32_t alfi_op_flags; /* marks the op as a set or remove
> > */
> > uint32_t alfi_name_len; /* attr name length */
> > + uint32_t alfi_nname_len; /* attr new name length */
>
> As I said in the other thread, this new field should replace
> alfi_pad,
> for no net gain to the structure size.
>
> > uint32_t alfi_value_len; /* attr value length */
> > uint32_t alfi_attr_filter;/* attr filter flags */
> > };
> > diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> > index 5077a7ad5646..40cbc95bf9b5 100644
> > --- a/fs/xfs/xfs_attr_item.c
> > +++ b/fs/xfs/xfs_attr_item.c
> > @@ -75,6 +75,8 @@ static inline struct xfs_attri_log_nameval *
> > xfs_attri_log_nameval_alloc(
> > const void *name,
> > unsigned int name_len,
> > + const void *nname,
> > + unsigned int nname_len,
> > const void *value,
> > unsigned int value_len)
> > {
> > @@ -85,7 +87,7 @@ xfs_attri_log_nameval_alloc(
> > * this. But kvmalloc() utterly sucks, so we use our own
> > version.
> > */
> > nv = xlog_kvmalloc(sizeof(struct xfs_attri_log_nameval) +
> > - name_len + value_len);
> > + name_len + nname_len +
> > value_len);
> > if (!nv)
> > return nv;
> >
> > @@ -94,8 +96,18 @@ xfs_attri_log_nameval_alloc(
> > nv->name.i_type = XLOG_REG_TYPE_ATTR_NAME;
> > memcpy(nv->name.i_addr, name, name_len);
> >
> > + if (nname_len) {
> > + nv->nname.i_addr = nv->name.i_addr + name_len;
> > + nv->nname.i_len = nname_len;
> > + memcpy(nv->nname.i_addr, nname, nname_len);
> > + } else {
> > + nv->nname.i_addr = NULL;
> > + nv->nname.i_len = 0;
> > + }
> > + nv->nname.i_type = XLOG_REG_TYPE_ATTR_NNAME;
> > +
> > if (value_len) {
> > - nv->value.i_addr = nv->name.i_addr + name_len;
> > + nv->value.i_addr = nv->name.i_addr + nname_len +
> > name_len;
> > nv->value.i_len = value_len;
> > memcpy(nv->value.i_addr, value, value_len);
> > } else {
> > @@ -149,11 +161,15 @@ xfs_attri_item_size(
> > *nbytes += sizeof(struct xfs_attri_log_format) +
> > xlog_calc_iovec_len(nv->name.i_len);
> >
> > - if (!nv->value.i_len)
> > - return;
> > + if (nv->nname.i_len) {
> > + *nvecs += 1;
> > + *nbytes += xlog_calc_iovec_len(nv->nname.i_len);
> > + }
> >
> > - *nvecs += 1;
> > - *nbytes += xlog_calc_iovec_len(nv->value.i_len);
> > + if (nv->value.i_len) {
> > + *nvecs += 1;
> > + *nbytes += xlog_calc_iovec_len(nv->value.i_len);
> > + }
> > }
> >
> > /*
> > @@ -183,6 +199,9 @@ xfs_attri_item_format(
> > ASSERT(nv->name.i_len > 0);
> > attrip->attri_format.alfi_size++;
> >
> > + if (nv->nname.i_len > 0)
> > + attrip->attri_format.alfi_size++;
> > +
> > if (nv->value.i_len > 0)
> > attrip->attri_format.alfi_size++;
> >
> > @@ -190,6 +209,10 @@ xfs_attri_item_format(
> > &attrip->attri_format,
> > sizeof(struct xfs_attri_log_format));
> > xlog_copy_from_iovec(lv, &vecp, &nv->name);
> > +
> > + if (nv->nname.i_len > 0)
> > + xlog_copy_from_iovec(lv, &vecp, &nv->nname);
> > +
> > if (nv->value.i_len > 0)
> > xlog_copy_from_iovec(lv, &vecp, &nv->value);
> > }
> > @@ -398,6 +421,7 @@ xfs_attr_log_item(
> > attrp->alfi_op_flags = attr->xattri_op_flags;
> > attrp->alfi_value_len = attr->xattri_nameval->value.i_len;
> > attrp->alfi_name_len = attr->xattri_nameval->name.i_len;
> > + attrp->alfi_nname_len = attr->xattri_nameval->nname.i_len;
> > ASSERT(!(attr->xattri_da_args->attr_filter &
> > ~XFS_ATTRI_FILTER_MASK));
> > attrp->alfi_attr_filter = attr->xattri_da_args->attr_filter;
> > }
> > @@ -439,7 +463,8 @@ xfs_attr_create_intent(
> > * deferred work state structure.
> > */
> > attr->xattri_nameval =
> > xfs_attri_log_nameval_alloc(args->name,
> > - args->namelen, args->value, args-
> > >valuelen);
> > + args->namelen, args->new_name,
> > + args->new_namelen, args->value, args-
> > >valuelen);
> > }
> > if (!attr->xattri_nameval)
> > return ERR_PTR(-ENOMEM);
> > @@ -543,6 +568,7 @@ xfs_attri_validate(
> > case XFS_ATTRI_OP_FLAGS_SET:
> > case XFS_ATTRI_OP_FLAGS_REPLACE:
> > case XFS_ATTRI_OP_FLAGS_REMOVE:
> > + case XFS_ATTRI_OP_FLAGS_NREPLACE:
> > break;
> > default:
> > return false;
> > @@ -552,6 +578,7 @@ xfs_attri_validate(
> > return false;
> >
> > if ((attrp->alfi_name_len > XATTR_NAME_MAX) ||
> > + (attrp->alfi_nname_len > XATTR_NAME_MAX) ||
> > (attrp->alfi_name_len == 0))
> > return false;
> >
> > @@ -615,6 +642,8 @@ xfs_attri_item_recover(
> > args->whichfork = XFS_ATTR_FORK;
> > args->name = nv->name.i_addr;
> > args->namelen = nv->name.i_len;
> > + args->new_name = nv->nname.i_addr;
> > + args->new_namelen = nv->nname.i_len;
> > args->hashval = xfs_da_hashname(args->name, args->namelen);
> > args->attr_filter = attrp->alfi_attr_filter &
> > XFS_ATTRI_FILTER_MASK;
> > args->op_flags = XFS_DA_OP_RECOVERY | XFS_DA_OP_OKNOENT |
> > @@ -625,6 +654,7 @@ xfs_attri_item_recover(
> > switch (attr->xattri_op_flags) {
> > case XFS_ATTRI_OP_FLAGS_SET:
> > case XFS_ATTRI_OP_FLAGS_REPLACE:
> > + case XFS_ATTRI_OP_FLAGS_NREPLACE:
> > args->value = nv->value.i_addr;
> > args->valuelen = nv->value.i_len;
> > args->total = xfs_attr_calc_size(args, &local);
> > @@ -714,6 +744,7 @@ xfs_attri_item_relog(
> > new_attrp->alfi_op_flags = old_attrp->alfi_op_flags;
> > new_attrp->alfi_value_len = old_attrp->alfi_value_len;
> > new_attrp->alfi_name_len = old_attrp->alfi_name_len;
> > + new_attrp->alfi_nname_len = old_attrp->alfi_nname_len;
> > new_attrp->alfi_attr_filter = old_attrp->alfi_attr_filter;
> >
> > xfs_trans_add_item(tp, &new_attrip->attri_item);
> > @@ -735,10 +766,15 @@ xlog_recover_attri_commit_pass2(
> > struct xfs_attri_log_nameval *nv;
> > const void *attr_value = NULL;
> > const void *attr_name;
> > + const void *attr_nname = NULL;
> > + int i = 0;
> > int error;
> >
> > - attri_formatp = item->ri_buf[0].i_addr;
> > - attr_name = item->ri_buf[1].i_addr;
> > + attri_formatp = item->ri_buf[i].i_addr;
> > + i++;
> > +
> > + attr_name = item->ri_buf[i].i_addr;
> > + i++;
> >
> > /* Validate xfs_attri_log_format before the large memory
> > allocation */
> > if (!xfs_attri_validate(mp, attri_formatp)) {
> > @@ -751,8 +787,20 @@ xlog_recover_attri_commit_pass2(
> > return -EFSCORRUPTED;
> > }
> >
> > + if (attri_formatp->alfi_nname_len) {
> > + attr_nname = item->ri_buf[i].i_addr;
> > + i++;
>
> /me wonders if the recovery function should check item-
> >ri_buf[i].i_type
> to ensure that nobody's switched the order on us, but can't confirm
> that
> the i_type ever gets used anywhere. I don't see it in the ondisk
> format
> documentation, so this might just be a wild goose chase.
Yeah, I was sort of wondering that myself, but I guess since we have it
we could add the check just for sanity.
>
> Sorry it took me a week to get to this. :/
>
No worries, thanks for the reviews!
Allison
> --D
>
> > +
> > + if (!xfs_attr_namecheck(mp, attr_nname,
> > + attri_formatp->alfi_nname_len,
> > + attri_formatp->alfi_attr_filter)) {
> > + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW,
> > mp);
> > + return -EFSCORRUPTED;
> > + }
> > + }
> > +
> > if (attri_formatp->alfi_value_len)
> > - attr_value = item->ri_buf[2].i_addr;
> > + attr_value = item->ri_buf[i].i_addr;
> >
> > /*
> > * Memory alloc failure will cause replay to abort. We attach
> > the
> > @@ -760,7 +808,8 @@ xlog_recover_attri_commit_pass2(
> > * reference.
> > */
> > nv = xfs_attri_log_nameval_alloc(attr_name,
> > - attri_formatp->alfi_name_len, attr_value,
> > + attri_formatp->alfi_name_len, attr_nname,
> > + attri_formatp->alfi_nname_len, attr_value,
> > attri_formatp->alfi_value_len);
> > if (!nv)
> > return -ENOMEM;
> > diff --git a/fs/xfs/xfs_attr_item.h b/fs/xfs/xfs_attr_item.h
> > index 3280a7930287..24d4968dd6cc 100644
> > --- a/fs/xfs/xfs_attr_item.h
> > +++ b/fs/xfs/xfs_attr_item.h
> > @@ -13,6 +13,7 @@ struct kmem_zone;
> >
> > struct xfs_attri_log_nameval {
> > struct xfs_log_iovec name;
> > + struct xfs_log_iovec nname;
> > struct xfs_log_iovec value;
> > refcount_t refcount;
> >
> > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> > index 758702b9495f..97d4ebedcf40 100644
> > --- a/fs/xfs/xfs_ondisk.h
> > +++ b/fs/xfs/xfs_ondisk.h
> > @@ -132,7 +132,7 @@ xfs_check_ondisk_structs(void)
> > XFS_CHECK_STRUCT_SIZE(struct xfs_inode_log_format, 56);
> > XFS_CHECK_STRUCT_SIZE(struct xfs_qoff_logformat, 20);
> > XFS_CHECK_STRUCT_SIZE(struct xfs_trans_header, 16)
> > ;
> > - XFS_CHECK_STRUCT_SIZE(struct xfs_attri_log_format, 40);
> > + XFS_CHECK_STRUCT_SIZE(struct xfs_attri_log_format, 48);
> > XFS_CHECK_STRUCT_SIZE(struct xfs_attrd_log_format, 16);
> >
> > /*
> > --
> > 2.25.1
> >
