Re: [PATCH] xfs: Make the incore inode di_size to xfs_ufsize_t

[Ben Myers <bpm@sgi.com>]

Hey Jeff,

On Wed, Sep 04, 2013 at 10:41:56AM +0800, Jeff Liu wrote:
> From: Jie Liu <jeff.liu@oracle.com>
> 
> Make the incore inode di_size to unsigned, this would be helpful
> to catch the negative sizes of it in many cases, so that we don't
> need to perform additional check for it being less than ZERO or not.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> ---
>  fs/xfs/xfs_inode_fork.c |    3 +--
>  fs/xfs/xfs_log_format.h |    2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode_fork.c b/fs/xfs/xfs_inode_fork.c
> index 02f1083..2b60a5a 100644
> --- a/fs/xfs/xfs_inode_fork.c
> +++ b/fs/xfs/xfs_inode_fork.c
> @@ -167,8 +167,7 @@ xfs_iformat_fork(
>  			}
>  
>  			di_size = be64_to_cpu(dip->di_size);
> -			if (unlikely(di_size < 0 ||
> -				     di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) {
> +			if (unlikely(di_size > XFS_DFORK_DSIZE(dip, ip->i_mount))) {
>  				xfs_warn(ip->i_mount,
>  			"corrupt inode %Lu (bad size %Ld for local inode).",
>  					(unsigned long long) ip->i_ino,
> diff --git a/fs/xfs/xfs_log_format.h b/fs/xfs/xfs_log_format.h
> index a49ab2c..2795fc5 100644
> --- a/fs/xfs/xfs_log_format.h
> +++ b/fs/xfs/xfs_log_format.h
> @@ -547,7 +547,7 @@ typedef struct xfs_icdinode {
>  	xfs_ictimestamp_t di_atime;	/* time last accessed */
>  	xfs_ictimestamp_t di_mtime;	/* time last modified */
>  	xfs_ictimestamp_t di_ctime;	/* time created/inode modified */
> -	xfs_fsize_t	di_size;	/* number of bytes in file */
> +	xfs_ufsize_t	di_size;	/* number of bytes in file */

These two changes by themselves look fairly innocuous, but upon closer
inspection I'm not so sure...

e.g. xfs_fsize_t is still signed, and i_size is loff_t is still signed.
I'm wondering if this doesn't represent a subtle change in the on-disk
format for inodes up in that size range.  This was on my 3.12 queue.  I
think it bears more discussion, so I'll hold off on this one for now.

FWIW I believe we're still ok with just Dan's fix because the maximum
size for local format is half a block or less.

Thanks,
	Ben

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs