Re: [RFC PATCH 08/13] xfs: parent pointer attribute creation

Darrick J. Wong
2017-08-15T00:06:08Z

On Wed, Aug 09, 2017 at 06:41:28PM -0700, Allison Henderson wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> [bfoster: rebase, use VFS inode generation]
> [achender: rebased, changed __unint32_t to xfs_dir2_dataptr_t,
> fixed some null pointer bugs]
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> ---
> :100644 100644 a2a5d04... 90b51ae... M fs/xfs/Makefile
> :100644 100644 646d41b... ed13c67... M fs/xfs/libxfs/xfs_attr.c
> :100644 100644 a2d6466... 91c403e... M fs/xfs/libxfs/xfs_bmap.c
> :100644 100644 851982a... 533f40f... M fs/xfs/libxfs/xfs_bmap.h
> :000000 100644 0000000... 88f7edc... A fs/xfs/libxfs/xfs_parent.c
> :100644 100644 3031a09... fad2fbd... M fs/xfs/xfs_attr.h
> :100644 100644 52c331e... 3557791... M fs/xfs/xfs_inode.c
> fs/xfs/Makefile | 1 +
> fs/xfs/libxfs/xfs_attr.c | 80 ++++++++++++++++++++++++++++++++++---
> fs/xfs/libxfs/xfs_bmap.c | 51 ++++++++++++++----------
> fs/xfs/libxfs/xfs_bmap.h | 1 +
> fs/xfs/libxfs/xfs_parent.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++
> fs/xfs/xfs_attr.h | 13 +++++-
> fs/xfs/xfs_inode.c | 16 +++++++-
> 7 files changed, 232 insertions(+), 28 deletions(-)
>
> diff --git a/fs/xfs/Makefile b/fs/xfs/Makefile
> index a2a5d04..90b51ae 100644
> --- a/fs/xfs/Makefile
> +++ b/fs/xfs/Makefile
> @@ -52,6 +52,7 @@ xfs-y += $(addprefix libxfs/, \
> xfs_inode_fork.o \
> xfs_inode_buf.o \
> xfs_log_rlimit.o \
> + xfs_parent.o \
> xfs_ag_resv.o \
> xfs_rmap.o \
> xfs_rmap_btree.o \
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 646d41b..ed13c67 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -90,12 +90,14 @@ xfs_attr_args_init(
> args->whichfork = XFS_ATTR_FORK;
> args->dp = dp;
> args->flags = flags;
> - args->name = name;
> - args->namelen = strlen((const char *)name);
> - if (args->namelen >= MAXNAMELEN)
> - return -EFAULT; /* match IRIX behaviour */
> + if (name) {
> + args->name = name;
> + args->namelen = strlen((const char *)name);
> + if (args->namelen >= MAXNAMELEN)
> + return -EFAULT; /* match IRIX behaviour */
>
> - args->hashval = xfs_da_hashname(args->name, args->namelen);
> + args->hashval = xfs_da_hashname(args->name, args->namelen);
> + }
> return 0;
> }
>
> @@ -203,6 +205,74 @@ xfs_attr_calc_size(
> return nblks;
> }
>
> +/*
> + * Add the initial parent pointer attribute.
> + *
> + * Inode must be locked and completely empty as we are adding the attribute
> + * fork to the inode. This open codes bits of xfs_bmap_add_attrfork() and
> + * xfs_attr_set() because we know the inode is completely empty at this point
> + * and so don't need to handle all the different combinations of fork
> + * configurations here.
> + */
> +int
> +xfs_attr_set_first_parent(
> + struct xfs_trans *tp,
> + struct xfs_inode *ip,
> + struct xfs_parent_name_rec *rec,
> + int reclen,
> + const char *value,
> + int valuelen,
> + struct xfs_defer_ops *dfops,
> + xfs_fsblock_t *firstblock)
> +{
> + struct xfs_da_args args;
> + int flags = ATTR_PARENT;
> + int local;
> + int sf_size;
> + int error;
> +
> + tp->t_flags |= XFS_TRANS_RESERVE;
> +
> + error = xfs_attr_args_init(&args, ip, (char *)rec, flags);
> + if (error)
> + return error;
> +
> + args.name = (char *)rec;
> + args.namelen = reclen;
> + args.hashval = xfs_da_hashname(args.name, args.namelen);
> + args.value = (char *)value;
> + args.valuelen = valuelen;
> + args.firstblock = firstblock;
> + args.dfops = dfops;
> + args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
> + args.total = xfs_attr_calc_size(&args, &local);
> + args.trans = tp;
> + ASSERT(local);
> +
> + /* set the attribute fork appropriately */
> + sf_size = sizeof(struct xfs_attr_sf_hdr) +
> + XFS_ATTR_SF_ENTSIZE_BYNAME(reclen, valuelen);
> + xfs_bmap_set_attrforkoff(ip, sf_size, NULL);
> + ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP);
> + ip->i_afp->if_flags = XFS_IFEXTENTS;
> +
> +
> + /* Try to add the attr to the attribute list in the inode. */
> + xfs_attr_shortform_create(&args);
> + error = xfs_attr_shortform_addname(&args);
> + if (error != -ENOSPC)
> + return error;

I wonder, what happens if we exceed the maximum number of attr fork
extents by linking a file too many times? Would be nice if we could
bail out early (instead of shutting down the fs) if we were reasonably
sure it was going to blow up anyway.

Though, I'm wondering how we handle that in general since I don't see
any obvious checking...

Also seems a little fishy that we keep going even if ENOSPC, even though
the comment below says we're allowed to dip into the reserved space to
avoid this condition?

--D

> +
> + /* It won't fit in the shortform, transform to a leaf block. */
> + xfs_defer_init(args.dfops, args.firstblock);
> + error = xfs_attr_shortform_to_leaf(&args);
> + if (error)
> + return error;
> +
> + ASSERT(xfs_bmap_one_block(ip, XFS_ATTR_FORK));
> + return xfs_attr_leaf_addname(&args);
> +}
> +
> int
> xfs_attr_set(
> struct xfs_inode *dp,
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index a2d6466..91c403e 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -1076,6 +1076,35 @@ xfs_bmap_add_attrfork_local(
> return -EFSCORRUPTED;
> }
>
> +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_UUID:
> + ip->i_d.di_forkoff = roundup(sizeof(uuid_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.
> @@ -1130,27 +1159,7 @@ 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_UUID:
> - ip->i_d.di_forkoff = roundup(sizeof(uuid_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;
> - goto trans_cancel;
> - }
> + xfs_bmap_set_attrforkoff(ip, size, &version);
>
> ASSERT(ip->i_afp == NULL);
> ip->i_afp = kmem_zone_zalloc(xfs_ifork_zone, KM_SLEEP);
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index 851982a..533f40f 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -209,6 +209,7 @@ void xfs_bmap_trace_exlist(struct xfs_inode *ip, xfs_extnum_t cnt,
> void xfs_trim_extent(struct xfs_bmbt_irec *irec, xfs_fileoff_t bno,
> xfs_filblks_t len);
> 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,
> diff --git a/fs/xfs/libxfs/xfs_parent.c b/fs/xfs/libxfs/xfs_parent.c
> new file mode 100644
> index 0000000..88f7edc
> --- /dev/null
> +++ b/fs/xfs/libxfs/xfs_parent.c
> @@ -0,0 +1,98 @@
> +/*
> + * Copyright (c) 2015 Red Hat, Inc.
> + * All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write the Free Software Foundation
> + */
> +#include "xfs.h"
> +#include "xfs_fs.h"
> +#include "xfs_format.h"
> +#include "xfs_log_format.h"
> +#include "xfs_shared.h"
> +#include "xfs_trans_resv.h"
> +#include "xfs_mount.h"
> +#include "xfs_bmap_btree.h"
> +#include "xfs_inode.h"
> +#include "xfs_error.h"
> +#include "xfs_trace.h"
> +#include "xfs_trans.h"
> +#include "xfs_attr.h"
> +
> +/*
> + * Parent pointer attribute handling.
> + *
> + * Because the attribute value is a filename component, it will never be longer
> + * than 255 bytes. This means the attribute will always be a local format
> + * attribute as it is xfs_attr_leaf_entsize_local_max() for v5 filesystems will
> + * always be larger than this (max is 75% of block size).
> + *
> + * Creating a new parent attribute will always create a new attribute - there
> + * should never, ever be an existing attribute in the tree for a new inode.
> + * ENOSPC behaviour is problematic - creating the inode without the parent
> + * pointer is effectively a corruption, so we allow parent attribute creation
> + * to dip into the reserve block pool to avoid unexpected ENOSPC errors from
> + * occurring.
> + */
> +
> +/*
> + * Create the initial parent attribute.
> + *
> + * The initial attribute creation also needs to be atomic w.r.t the parent
> + * directory modification. Hence it needs to run in the same transaction and the
> + * transaction committed by the caller. Because the attribute created is
> + * guaranteed to be a local attribute and is always going to be the first
> + * attribute in the attribute fork, we can do this safely in the single
> + * transaction context as it is impossible for an overwrite to occur and hence
> + * we'll never have a rolling overwrite transaction occurring here. Hence we
> + * can short-cut a lot of the normal xfs_attr_set() code paths that are needed
> + * to handle the generic cases.
> + */
> +static int
> +xfs_parent_create_nrec(
> + struct xfs_trans *tp,
> + struct xfs_inode *child,
> + struct xfs_parent_name_irec *nrec,
> + struct xfs_defer_ops *dfops,
> + xfs_fsblock_t *firstblock)
> +{
> + struct xfs_parent_name_rec rec;
> +
> + rec.p_ino = cpu_to_be64(nrec->p_ino);
> + rec.p_gen = cpu_to_be32(nrec->p_gen);
> + rec.p_diroffset = cpu_to_be32(nrec->p_diroffset);
> +
> + return xfs_attr_set_first_parent(tp, child, &rec, sizeof(rec),
> + nrec->p_name, nrec->p_namelen,
> + dfops, firstblock);
> +}
> +
> +int
> +xfs_parent_create(
> + struct xfs_trans *tp,
> + struct xfs_inode *parent,
> + struct xfs_inode *child,
> + struct xfs_name *child_name,
> + xfs_dir2_dataptr_t diroffset,
> + struct xfs_defer_ops *dfops,
> + xfs_fsblock_t *firstblock)
> +{
> + struct xfs_parent_name_irec nrec;
> +
> + nrec.p_ino = parent->i_ino;
> + nrec.p_gen = VFS_I(parent)->i_generation;
> + nrec.p_diroffset = diroffset;
> + nrec.p_name = child_name->name;
> + nrec.p_namelen = child_name->len;
> +
> + return xfs_parent_create_nrec(tp, child, &nrec, dfops, firstblock);
> +}
> diff --git a/fs/xfs/xfs_attr.h b/fs/xfs/xfs_attr.h
> index 3031a09..fad2fbd 100644
> --- a/fs/xfs/xfs_attr.h
> +++ b/fs/xfs/xfs_attr.h
> @@ -155,5 +155,16 @@ 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);
>
> -
> +/*
> + * Parent pointer attribute prototypes
> + */
> +int xfs_parent_create(struct xfs_trans *tp, struct xfs_inode *parent,
> + struct xfs_inode *child, struct xfs_name *child_name,
> + xfs_dir2_dataptr_t diroffset, struct xfs_defer_ops *dfops,
> + xfs_fsblock_t *firstblock);
> +int xfs_attr_set_first_parent(struct xfs_trans *tp, struct xfs_inode *ip,
> + struct xfs_parent_name_rec *rec, int reclen,
> + const char *value, int valuelen,
> + struct xfs_defer_ops *dfops,
> + xfs_fsblock_t *firstblock);
> #endif /* __XFS_ATTR_H__ */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 52c331e..3557791 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1162,6 +1162,7 @@ xfs_create(
> struct xfs_dquot *pdqp = NULL;
> struct xfs_trans_res *tres;
> uint resblks;
> + xfs_dir2_dataptr_t diroffset;
>
> trace_xfs_create(dp, name);
>
> @@ -1251,7 +1252,7 @@ xfs_create(
> error = xfs_dir_createname(tp, dp, name, ip->i_ino,
> &first_block, &dfops, resblks ?
> resblks - XFS_IALLOC_SPACE_RES(mp) : 0,
> - NULL);
> + &diroffset);
> if (error) {
> ASSERT(error != -ENOSPC);
> goto out_trans_cancel;
> @@ -1270,6 +1271,19 @@ xfs_create(
> }
>
> /*
> + * If we have parent pointers, we need to add the attribute containing
> + * the parent information now. This must be done within the same
> + * transaction the directory entry is created, while the new inode
> + * contains nothing in the inode literal area.
> + */
> + if (xfs_sb_version_hasparent(&mp->m_sb)) {
> + error = xfs_parent_create(tp, dp, ip, name, diroffset,
> + &dfops, &first_block);
> + if (error)
> + goto out_bmap_cancel;
> + }
> +
> + /*
> * If this is a synchronous mount, make sure that the
> * create transaction goes to disk before returning to
> * the user.
> --
> 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