Re: [PATCH 30/46] xfs: cleanup xfs_dir2_leaf_getdents

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Nov 07, 2019 at 07:23:54PM +0100, Christoph Hellwig wrote:
> Use an offset as the main means for iteration, and only do pointer
> arithmetics to find the data/unused entries.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_dir2_readdir.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
> index 0d234b649d65..c4314e9e3dd8 100644
> --- a/fs/xfs/xfs_dir2_readdir.c
> +++ b/fs/xfs/xfs_dir2_readdir.c
> @@ -351,13 +351,13 @@ xfs_dir2_leaf_getdents(
>  	xfs_dir2_data_hdr_t	*hdr;		/* data block header */

gcc complained about this variable being set but not used.

>  	xfs_dir2_data_entry_t	*dep;		/* data entry */
>  	xfs_dir2_data_unused_t	*dup;		/* unused entry */
> -	char			*ptr = NULL;	/* pointer to current data */
>  	struct xfs_da_geometry	*geo = args->geo;
>  	xfs_dablk_t		rablk = 0;	/* current readahead block */
>  	xfs_dir2_off_t		curoff;		/* current overall offset */
>  	int			length;		/* temporary length value */
>  	int			byteoff;	/* offset in current block */
>  	int			lock_mode;
> +	unsigned int		offset = 0;

This is the offset within the block, right?

>  	int			error = 0;	/* error return value */
>  
>  	/*
> @@ -384,7 +384,7 @@ xfs_dir2_leaf_getdents(
>  		 * If we have no buffer, or we're off the end of the
>  		 * current buffer, need to get another one.
>  		 */
> -		if (!bp || ptr >= (char *)bp->b_addr + geo->blksize) {
> +		if (!bp || offset + geo->blksize) {

                           ^^^^^^^^^^^^^^^^^^^^^

In which case, isn't this always true?  Was this supposed to be
offset >= geo->blksize?

--D

>  			if (bp) {
>  				xfs_trans_brelse(args->trans, bp);
>  				bp = NULL;
> @@ -402,7 +402,7 @@ xfs_dir2_leaf_getdents(
>  			/*
>  			 * Find our position in the block.
>  			 */
> -			ptr = (char *)dp->d_ops->data_entry_p(hdr);
> +			offset = dp->d_ops->data_entry_offset;
>  			byteoff = xfs_dir2_byte_to_off(geo, curoff);
>  			/*
>  			 * Skip past the header.
> @@ -413,20 +413,20 @@ xfs_dir2_leaf_getdents(
>  			 * Skip past entries until we reach our offset.
>  			 */
>  			else {
> -				while ((char *)ptr - (char *)hdr < byteoff) {
> -					dup = (xfs_dir2_data_unused_t *)ptr;
> +				while (offset < byteoff) {
> +					dup = bp->b_addr + offset;
>  
>  					if (be16_to_cpu(dup->freetag)
>  						  == XFS_DIR2_DATA_FREE_TAG) {
>  
>  						length = be16_to_cpu(dup->length);
> -						ptr += length;
> +						offset += length;
>  						continue;
>  					}
> -					dep = (xfs_dir2_data_entry_t *)ptr;
> +					dep = bp->b_addr + offset;
>  					length =
>  					   dp->d_ops->data_entsize(dep->namelen);
> -					ptr += length;
> +					offset += length;
>  				}
>  				/*
>  				 * Now set our real offset.
> @@ -434,28 +434,28 @@ xfs_dir2_leaf_getdents(
>  				curoff =
>  					xfs_dir2_db_off_to_byte(geo,
>  					    xfs_dir2_byte_to_db(geo, curoff),
> -					    (char *)ptr - (char *)hdr);
> -				if (ptr >= (char *)hdr + geo->blksize) {
> +					    offset);
> +				if (offset >= geo->blksize)
>  					continue;
> -				}
>  			}
>  		}
> +
>  		/*
> -		 * We have a pointer to an entry.
> -		 * Is it a live one?
> +		 * We have a pointer to an entry.  Is it a live one?
>  		 */
> -		dup = (xfs_dir2_data_unused_t *)ptr;
> +		dup = bp->b_addr + offset;
> +
>  		/*
>  		 * No, it's unused, skip over it.
>  		 */
>  		if (be16_to_cpu(dup->freetag) == XFS_DIR2_DATA_FREE_TAG) {
>  			length = be16_to_cpu(dup->length);
> -			ptr += length;
> +			offset += length;
>  			curoff += length;
>  			continue;
>  		}
>  
> -		dep = (xfs_dir2_data_entry_t *)ptr;
> +		dep = bp->b_addr + offset;
>  		length = dp->d_ops->data_entsize(dep->namelen);
>  		filetype = dp->d_ops->data_get_ftype(dep);
>  
> @@ -474,7 +474,7 @@ xfs_dir2_leaf_getdents(
>  		/*
>  		 * Advance to next entry in the block.
>  		 */
> -		ptr += length;
> +		offset += length;
>  		curoff += length;
>  		/* bufsize may have just been a guess; don't go negative */
>  		bufsize = bufsize > length ? bufsize - length : 0;
> -- 
> 2.20.1
>