xfs
[Top] [All Lists]

Re: [PATCH 3/3] xfs: allocate xfs_da_args to reduce stack footprint

To: Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Subject: Re: [PATCH 3/3] xfs: allocate xfs_da_args to reduce stack footprint
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Wed, 19 Feb 2014 13:25:31 -0500
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1392783402-4726-4-git-send-email-david@xxxxxxxxxxxxx>
References: <1392783402-4726-1-git-send-email-david@xxxxxxxxxxxxx> <1392783402-4726-4-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0
On 02/18/2014 11:16 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> The struct xfs_da_args used to pass directory/attribute operation
> information to the lower layers is 128 bytes in size and is
> allocated on the stack. Dynamically allocate them to reduce the
> stack footprint of directory operations.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
>  fs/xfs/xfs_dir2.c | 344 
> +++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 214 insertions(+), 130 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dir2.c b/fs/xfs/xfs_dir2.c
> index ce16ef0..fc9a41f 100644
> --- a/fs/xfs/xfs_dir2.c
> +++ b/fs/xfs/xfs_dir2.c
> @@ -180,16 +180,23 @@ xfs_dir_init(
>       xfs_inode_t     *dp,
>       xfs_inode_t     *pdp)
>  {
> -     xfs_da_args_t   args;
> +     struct xfs_da_args *args;
>       int             error;
>  
> -     memset((char *)&args, 0, sizeof(args));
> -     args.dp = dp;
> -     args.trans = tp;
>       ASSERT(S_ISDIR(dp->i_d.di_mode));
> -     if ((error = xfs_dir_ino_validate(tp->t_mountp, pdp->i_ino)))
> +     error = xfs_dir_ino_validate(tp->t_mountp, pdp->i_ino);
> +     if (error)
>               return error;
> -     return xfs_dir2_sf_create(&args, pdp->i_ino);
> +
> +     args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
> +     if (!args)
> +             return ENOMEM;
> +
> +     args->dp = dp;
> +     args->trans = tp;
> +     error = xfs_dir2_sf_create(args, pdp->i_ino);
> +     kmem_free(args);
> +     return error;
>  }
>  
>  /*
> @@ -205,41 +212,56 @@ xfs_dir_createname(
>       xfs_bmap_free_t         *flist,         /* bmap's freeblock list */
>       xfs_extlen_t            total)          /* bmap's total block count */
>  {
> -     xfs_da_args_t           args;
> +     struct xfs_da_args      *args;
>       int                     rval;
>       int                     v;              /* type-checking value */
>  
>       ASSERT(S_ISDIR(dp->i_d.di_mode));
> -     if ((rval = xfs_dir_ino_validate(tp->t_mountp, inum)))
> +     rval = xfs_dir_ino_validate(tp->t_mountp, inum);
> +     if (rval)
>               return rval;
>       XFS_STATS_INC(xs_dir_create);
>  
> -     memset(&args, 0, sizeof(xfs_da_args_t));
> -     args.name = name->name;
> -     args.namelen = name->len;
> -     args.filetype = name->type;
> -     args.hashval = dp->i_mount->m_dirnameops->hashname(name);
> -     args.inumber = inum;
> -     args.dp = dp;
> -     args.firstblock = first;
> -     args.flist = flist;
> -     args.total = total;
> -     args.whichfork = XFS_DATA_FORK;
> -     args.trans = tp;
> -     args.op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
> -
> -     if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL)
> -             rval = xfs_dir2_sf_addname(&args);
> -     else if ((rval = xfs_dir2_isblock(tp, dp, &v)))
> -             return rval;
> -     else if (v)
> -             rval = xfs_dir2_block_addname(&args);
> -     else if ((rval = xfs_dir2_isleaf(tp, dp, &v)))
> -             return rval;
> -     else if (v)
> -             rval = xfs_dir2_leaf_addname(&args);
> +     args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
> +     if (!args)
> +             return ENOMEM;
> +
> +     args->name = name->name;
> +     args->namelen = name->len;
> +     args->filetype = name->type;
> +     args->hashval = dp->i_mount->m_dirnameops->hashname(name);
> +     args->inumber = inum;
> +     args->dp = dp;
> +     args->firstblock = first;
> +     args->flist = flist;
> +     args->total = total;
> +     args->whichfork = XFS_DATA_FORK;
> +     args->trans = tp;
> +     args->op_flags = XFS_DA_OP_ADDNAME | XFS_DA_OP_OKNOENT;
> +
> +     if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
> +             rval = xfs_dir2_sf_addname(args);
> +             goto out_free;
> +     }
> +
> +     rval = xfs_dir2_isblock(tp, dp, &v);
> +     if (rval)
> +             goto out_free;
> +     if (v) {
> +             rval = xfs_dir2_block_addname(args);
> +             goto out_free;
> +     }
> +
> +     rval = xfs_dir2_isleaf(tp, dp, &v);
> +     if (rval)
> +             goto out_free;
> +     if (v)
> +             rval = xfs_dir2_leaf_addname(args);
>       else
> -             rval = xfs_dir2_node_addname(&args);
> +             rval = xfs_dir2_node_addname(args);
> +
> +out_free:
> +     kmem_free(args);
>       return rval;
>  }
>  
> @@ -282,46 +304,68 @@ xfs_dir_lookup(
>       xfs_ino_t       *inum,          /* out: inode number */
>       struct xfs_name *ci_name)       /* out: actual name if CI match */
>  {
> -     xfs_da_args_t   args;
> +     struct xfs_da_args *args;
>       int             rval;
>       int             v;              /* type-checking value */
>  
>       ASSERT(S_ISDIR(dp->i_d.di_mode));
>       XFS_STATS_INC(xs_dir_lookup);
>  
> -     memset(&args, 0, sizeof(xfs_da_args_t));
> -     args.name = name->name;
> -     args.namelen = name->len;
> -     args.filetype = name->type;
> -     args.hashval = dp->i_mount->m_dirnameops->hashname(name);
> -     args.dp = dp;
> -     args.whichfork = XFS_DATA_FORK;
> -     args.trans = tp;
> -     args.op_flags = XFS_DA_OP_OKNOENT;
> +     /*
> +      * If we don't use KM_NOFS here, lockdep will through false positive

                                                        throw?

Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>


> +      * deadlock warnings when we come through here of the non-transactional
> +      * lookup path because the allocation can recurse into inode reclaim.
> +      * Doing this avoids having to add a bunch of lockdep class
> +      * annotations into the reclaim patch for the ilock.
> +      */
> +     args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
> +     if (!args)
> +             return ENOMEM;
> +
> +     args->name = name->name;
> +     args->namelen = name->len;
> +     args->filetype = name->type;
> +     args->hashval = dp->i_mount->m_dirnameops->hashname(name);
> +     args->dp = dp;
> +     args->whichfork = XFS_DATA_FORK;
> +     args->trans = tp;
> +     args->op_flags = XFS_DA_OP_OKNOENT;
>       if (ci_name)
> -             args.op_flags |= XFS_DA_OP_CILOOKUP;
> +             args->op_flags |= XFS_DA_OP_CILOOKUP;
>  
> -     if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL)
> -             rval = xfs_dir2_sf_lookup(&args);
> -     else if ((rval = xfs_dir2_isblock(tp, dp, &v)))
> -             return rval;
> -     else if (v)
> -             rval = xfs_dir2_block_lookup(&args);
> -     else if ((rval = xfs_dir2_isleaf(tp, dp, &v)))
> -             return rval;
> -     else if (v)
> -             rval = xfs_dir2_leaf_lookup(&args);
> +     if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
> +             rval = xfs_dir2_sf_lookup(args);
> +             goto out_check_rval;
> +     }
> +
> +     rval = xfs_dir2_isblock(tp, dp, &v);
> +     if (rval)
> +             goto out_free;
> +     if (v) {
> +             rval = xfs_dir2_block_lookup(args);
> +             goto out_check_rval;
> +     }
> +
> +     rval = xfs_dir2_isleaf(tp, dp, &v);
> +     if (rval)
> +             goto out_free;
> +     if (v)
> +             rval = xfs_dir2_leaf_lookup(args);
>       else
> -             rval = xfs_dir2_node_lookup(&args);
> +             rval = xfs_dir2_node_lookup(args);
> +
> +out_check_rval:
>       if (rval == EEXIST)
>               rval = 0;
>       if (!rval) {
> -             *inum = args.inumber;
> +             *inum = args->inumber;
>               if (ci_name) {
> -                     ci_name->name = args.value;
> -                     ci_name->len = args.valuelen;
> +                     ci_name->name = args->value;
> +                     ci_name->len = args->valuelen;
>               }
>       }
> +out_free:
> +     kmem_free(args);
>       return rval;
>  }
>  
> @@ -338,38 +382,51 @@ xfs_dir_removename(
>       xfs_bmap_free_t *flist,         /* bmap's freeblock list */
>       xfs_extlen_t    total)          /* bmap's total block count */
>  {
> -     xfs_da_args_t   args;
> +     struct xfs_da_args *args;
>       int             rval;
>       int             v;              /* type-checking value */
>  
>       ASSERT(S_ISDIR(dp->i_d.di_mode));
>       XFS_STATS_INC(xs_dir_remove);
>  
> -     memset(&args, 0, sizeof(xfs_da_args_t));
> -     args.name = name->name;
> -     args.namelen = name->len;
> -     args.filetype = name->type;
> -     args.hashval = dp->i_mount->m_dirnameops->hashname(name);
> -     args.inumber = ino;
> -     args.dp = dp;
> -     args.firstblock = first;
> -     args.flist = flist;
> -     args.total = total;
> -     args.whichfork = XFS_DATA_FORK;
> -     args.trans = tp;
> -
> -     if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL)
> -             rval = xfs_dir2_sf_removename(&args);
> -     else if ((rval = xfs_dir2_isblock(tp, dp, &v)))
> -             return rval;
> -     else if (v)
> -             rval = xfs_dir2_block_removename(&args);
> -     else if ((rval = xfs_dir2_isleaf(tp, dp, &v)))
> -             return rval;
> -     else if (v)
> -             rval = xfs_dir2_leaf_removename(&args);
> +     args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
> +     if (!args)
> +             return ENOMEM;
> +
> +     args->name = name->name;
> +     args->namelen = name->len;
> +     args->filetype = name->type;
> +     args->hashval = dp->i_mount->m_dirnameops->hashname(name);
> +     args->inumber = ino;
> +     args->dp = dp;
> +     args->firstblock = first;
> +     args->flist = flist;
> +     args->total = total;
> +     args->whichfork = XFS_DATA_FORK;
> +     args->trans = tp;
> +
> +     if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
> +             rval = xfs_dir2_sf_removename(args);
> +             goto out_free;
> +     }
> +
> +     rval = xfs_dir2_isblock(tp, dp, &v);
> +     if (rval)
> +             goto out_free;
> +     if (v) {
> +             rval = xfs_dir2_block_removename(args);
> +             goto out_free;
> +     }
> +
> +     rval = xfs_dir2_isleaf(tp, dp, &v);
> +     if (rval)
> +             goto out_free;
> +     if (v)
> +             rval = xfs_dir2_leaf_removename(args);
>       else
> -             rval = xfs_dir2_node_removename(&args);
> +             rval = xfs_dir2_node_removename(args);
> +out_free:
> +     kmem_free(args);
>       return rval;
>  }
>  
> @@ -386,40 +443,54 @@ xfs_dir_replace(
>       xfs_bmap_free_t *flist,         /* bmap's freeblock list */
>       xfs_extlen_t    total)          /* bmap's total block count */
>  {
> -     xfs_da_args_t   args;
> +     struct xfs_da_args *args;
>       int             rval;
>       int             v;              /* type-checking value */
>  
>       ASSERT(S_ISDIR(dp->i_d.di_mode));
>  
> -     if ((rval = xfs_dir_ino_validate(tp->t_mountp, inum)))
> +     rval = xfs_dir_ino_validate(tp->t_mountp, inum);
> +     if (rval)
>               return rval;
>  
> -     memset(&args, 0, sizeof(xfs_da_args_t));
> -     args.name = name->name;
> -     args.namelen = name->len;
> -     args.filetype = name->type;
> -     args.hashval = dp->i_mount->m_dirnameops->hashname(name);
> -     args.inumber = inum;
> -     args.dp = dp;
> -     args.firstblock = first;
> -     args.flist = flist;
> -     args.total = total;
> -     args.whichfork = XFS_DATA_FORK;
> -     args.trans = tp;
> -
> -     if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL)
> -             rval = xfs_dir2_sf_replace(&args);
> -     else if ((rval = xfs_dir2_isblock(tp, dp, &v)))
> -             return rval;
> -     else if (v)
> -             rval = xfs_dir2_block_replace(&args);
> -     else if ((rval = xfs_dir2_isleaf(tp, dp, &v)))
> -             return rval;
> -     else if (v)
> -             rval = xfs_dir2_leaf_replace(&args);
> +     args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
> +     if (!args)
> +             return ENOMEM;
> +
> +     args->name = name->name;
> +     args->namelen = name->len;
> +     args->filetype = name->type;
> +     args->hashval = dp->i_mount->m_dirnameops->hashname(name);
> +     args->inumber = inum;
> +     args->dp = dp;
> +     args->firstblock = first;
> +     args->flist = flist;
> +     args->total = total;
> +     args->whichfork = XFS_DATA_FORK;
> +     args->trans = tp;
> +
> +     if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
> +             rval = xfs_dir2_sf_replace(args);
> +             goto out_free;
> +     }
> +
> +     rval = xfs_dir2_isblock(tp, dp, &v);
> +     if (rval)
> +             goto out_free;
> +     if (v) {
> +             rval = xfs_dir2_block_replace(args);
> +             goto out_free;
> +     }
> +
> +     rval = xfs_dir2_isleaf(tp, dp, &v);
> +     if (rval)
> +             goto out_free;
> +     if (v)
> +             rval = xfs_dir2_leaf_replace(args);
>       else
> -             rval = xfs_dir2_node_replace(&args);
> +             rval = xfs_dir2_node_replace(args);
> +out_free:
> +     kmem_free(args);
>       return rval;
>  }
>  
> @@ -434,7 +505,7 @@ xfs_dir_canenter(
>       struct xfs_name *name,          /* name of entry to add */
>       uint            resblks)
>  {
> -     xfs_da_args_t   args;
> +     struct xfs_da_args *args;
>       int             rval;
>       int             v;              /* type-checking value */
>  
> @@ -443,29 +514,42 @@ xfs_dir_canenter(
>  
>       ASSERT(S_ISDIR(dp->i_d.di_mode));
>  
> -     memset(&args, 0, sizeof(xfs_da_args_t));
> -     args.name = name->name;
> -     args.namelen = name->len;
> -     args.filetype = name->type;
> -     args.hashval = dp->i_mount->m_dirnameops->hashname(name);
> -     args.dp = dp;
> -     args.whichfork = XFS_DATA_FORK;
> -     args.trans = tp;
> -     args.op_flags = XFS_DA_OP_JUSTCHECK | XFS_DA_OP_ADDNAME |
> +     args = kmem_zalloc(sizeof(*args), KM_SLEEP | KM_NOFS);
> +     if (!args)
> +             return ENOMEM;
> +
> +     args->name = name->name;
> +     args->namelen = name->len;
> +     args->filetype = name->type;
> +     args->hashval = dp->i_mount->m_dirnameops->hashname(name);
> +     args->dp = dp;
> +     args->whichfork = XFS_DATA_FORK;
> +     args->trans = tp;
> +     args->op_flags = XFS_DA_OP_JUSTCHECK | XFS_DA_OP_ADDNAME |
>                                                       XFS_DA_OP_OKNOENT;
>  
> -     if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL)
> -             rval = xfs_dir2_sf_addname(&args);
> -     else if ((rval = xfs_dir2_isblock(tp, dp, &v)))
> -             return rval;
> -     else if (v)
> -             rval = xfs_dir2_block_addname(&args);
> -     else if ((rval = xfs_dir2_isleaf(tp, dp, &v)))
> -             return rval;
> -     else if (v)
> -             rval = xfs_dir2_leaf_addname(&args);
> +     if (dp->i_d.di_format == XFS_DINODE_FMT_LOCAL) {
> +             rval = xfs_dir2_sf_addname(args);
> +             goto out_free;
> +     }
> +
> +     rval = xfs_dir2_isblock(tp, dp, &v);
> +     if (rval)
> +             goto out_free;
> +     if (v) {
> +             rval = xfs_dir2_block_addname(args);
> +             goto out_free;
> +     }
> +
> +     rval = xfs_dir2_isleaf(tp, dp, &v);
> +     if (rval)
> +             goto out_free;
> +     if (v)
> +             rval = xfs_dir2_leaf_addname(args);
>       else
> -             rval = xfs_dir2_node_addname(&args);
> +             rval = xfs_dir2_node_addname(args);
> +out_free:
> +     kmem_free(args);
>       return rval;
>  }
>  
> 

<Prev in Thread] Current Thread [Next in Thread>