Re: [PATCH 13/19] xfs: vectorise directory data operations

Ben Myers
2013-10-24T18:39:26Z

On Tue, Oct 15, 2013 at 09:17:59AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Following from the initial patches to vectorise the shortform
> directory encode/decode operations, convert half the data block
> operations to use the vector. The rest will be done in a second
> patch.
>
> This further reduces the size of the built binary:
>
> text data bss dec hex filename
> 794490 96802 1096 892388 d9de4 fs/xfs/xfs.o.orig
> 792986 96802 1096 890884 d9804 fs/xfs/xfs.o.p1
> 792350 96802 1096 890248 d9588 fs/xfs/xfs.o.p2
> 789293 96802 1096 887191 d8997 fs/xfs/xfs.o.p3
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Generally looks pretty good, I have a question below...

> const struct xfs_dir_ops xfs_dir2_ftype_ops = {
> @@ -223,6 +415,18 @@ const struct xfs_dir_ops xfs_dir2_ftype_ops = {
> .sf_put_ino = xfs_dir3_sfe_put_ino,
> .sf_get_parent_ino = xfs_dir2_sf_get_parent_ino,
> .sf_put_parent_ino = xfs_dir2_sf_put_parent_ino,
> +
> + .data_entsize = xfs_dir3_data_entsize,
> + .data_get_ftype = xfs_dir3_data_get_ftype,
> + .data_put_ftype = xfs_dir3_data_put_ftype,
> + .data_entry_tag_p = xfs_dir3_data_entry_tag_p,
> +
> + .data_dot_offset = xfs_dir2_data_dot_offset,
> + .data_dotdot_offset = xfs_dir2_data_dotdot_offset,
> + .data_first_offset = xfs_dir2_data_first_offset,
> + .data_dot_entry_p = xfs_dir2_data_dot_entry_p,
> + .data_dotdot_entry_p = xfs_dir2_data_dotdot_entry_p,
> + .data_first_entry_p = xfs_dir2_data_first_entry_p,
> };

I think there may be a problem here. Although the dirv2 functions for
., .., and first entry offset account for the v2 header size, they
appear not to be accounting for the modified entry size due to the file
type field. Am I missing something?

> @@ -1158,32 +1159,32 @@ xfs_dir2_sf_to_block(
> /*
> * Create entry for .
> */
> - dep = xfs_dir3_data_dot_entry_p(mp, hdr);
> + dep = dp->d_ops->data_dot_entry_p(hdr);
> dep->inumber = cpu_to_be64(dp->i_ino);
> dep->namelen = 1;
> dep->name[0] = '.';
> - xfs_dir3_dirent_put_ftype(mp, dep, XFS_DIR3_FT_DIR);
> - tagp = xfs_dir3_data_entry_tag_p(mp, dep);
> + dp->d_ops->data_put_ftype(dep, XFS_DIR3_FT_DIR);
> + tagp = dp->d_ops->data_entry_tag_p(dep);
> *tagp = cpu_to_be16((char *)dep - (char *)hdr);
> - xfs_dir2_data_log_entry(tp, bp, dep);
> + xfs_dir2_data_log_entry(tp, dp, bp, dep);
> blp[0].hashval = cpu_to_be32(xfs_dir_hash_dot);
> blp[0].address = cpu_to_be32(xfs_dir2_byte_to_dataptr(mp,
> (char *)dep - (char *)hdr));
> /*
> * Create entry for ..
> */
> - dep = xfs_dir3_data_dotdot_entry_p(mp, hdr);
> + dep = dp->d_ops->data_dotdot_entry_p(hdr);
> dep->inumber = cpu_to_be64(dp->d_ops->sf_get_parent_ino(sfp));
> dep->namelen = 2;
> dep->name[0] = dep->name[1] = '.';
> - xfs_dir3_dirent_put_ftype(mp, dep, XFS_DIR3_FT_DIR);
> - tagp = xfs_dir3_data_entry_tag_p(mp, dep);
> + dp->d_ops->data_put_ftype(dep, XFS_DIR3_FT_DIR);
> + tagp =dp->d_ops-> data_entry_tag_p(dep);

tagp = dp->d_ops->data_entry_tag_p(dep);

> diff --git a/fs/xfs/xfs_dir2_data.c b/fs/xfs/xfs_dir2_data.c
> index ccfeb4d..571e173 100644
> --- a/fs/xfs/xfs_dir2_data.c
> +++ b/fs/xfs/xfs_dir2_data.c
> @@ -62,12 +62,26 @@ __xfs_dir3_data_check(
> char *p; /* current data position */
> int stale; /* count of stale leaves */
> struct xfs_name name;
> + const struct xfs_dir_ops *ops;
>
> mp = bp->b_target->bt_mount;
> hdr = bp->b_addr;
> bf = xfs_dir3_data_bestfree_p(hdr);
> p = (char *)xfs_dir3_data_entry_p(hdr);
>
> + /*
> + * we can be passed a null dp here froma verifier, so manually configure

from a