Re: [PATCH 2/2] ceph: switch to 4KB block size if quota size is not aligned to 4MB

["Luís Henriques" <lhenriques@suse.de>]

xiubli@redhat.com writes:

> From: Xiubo Li <xiubli@redhat.com>
>
> If the quota size is larger than but not aligned to 4MB, the statfs
> will always set the block size to 4MB and round down the fragment
> size. For exmaple if the quota size is 6MB, the `df` will always
> show 4MB capacity.
>
> Make the block size to 4KB as default if quota size is set unless
> the quota size is larger than or equals to 4MB and at the same time
> it aligns to 4MB.
>
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/quota.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c
> index 64592adfe48f..c50527151913 100644
> --- a/fs/ceph/quota.c
> +++ b/fs/ceph/quota.c
> @@ -483,6 +483,7 @@ bool ceph_quota_update_statfs(struct ceph_fs_client *fsc, struct kstatfs *buf)
>  	struct inode *in;
>  	u64 total = 0, used, free;
>  	bool is_updated = false;
> +	u32 block_shift = CEPH_4K_BLOCK_SHIFT;
>  
>  	down_read(&mdsc->snap_rwsem);
>  	realm = get_quota_realm(mdsc, d_inode(fsc->sb->s_root),
> @@ -498,21 +499,29 @@ bool ceph_quota_update_statfs(struct ceph_fs_client *fsc, struct kstatfs *buf)
>  		ci = ceph_inode(in);
>  		spin_lock(&ci->i_ceph_lock);
>  		if (ci->i_max_bytes) {
> -			total = ci->i_max_bytes >> CEPH_BLOCK_SHIFT;
> -			used = ci->i_rbytes >> CEPH_BLOCK_SHIFT;
> -			/* For quota size less than 4MB, use 4KB block size */
> -			if (!total) {
> -				total = ci->i_max_bytes >> CEPH_4K_BLOCK_SHIFT;
> -				used = ci->i_rbytes >> CEPH_4K_BLOCK_SHIFT;
> -	                        buf->f_frsize = 1 << CEPH_4K_BLOCK_SHIFT;
> -			}
> -			/* It is possible for a quota to be exceeded.
> +			/*
> +			 * Switch to 4MB block size if quota size is
> +			 * larger than or equals to 4MB and at the
> +			 * same time is aligned to 4MB.
> +			 */
> +			if (ci->i_max_bytes >= (1 << CEPH_BLOCK_SHIFT) &&
> +			    !(ci->i_max_bytes % (1 << CEPH_BLOCK_SHIFT)))

Maybe worth replacing this 2nd condition with the IS_ALIGNED() macro.
Other than this, these patches look good.

I do have question though: is it possible that this will behaviour may
break some user-space programs that expect more deterministic values for
these fields (buf->f_frsize and buf->f_bsize)?  Because the same
filesystem will report different values depending on which dir you mount.

Obviously, this isn't a problem with this particular patch, as this
behaviour is already present.

Cheers,
-- 
Luís

> +				block_shift = CEPH_BLOCK_SHIFT;
> +
> +			total = ci->i_max_bytes >> block_shift;
> +			used = ci->i_rbytes >> block_shift;
> +			buf->f_frsize = 1 << block_shift;
> +
> +			/*
> +			 * It is possible for a quota to be exceeded.
>  			 * Report 'zero' in that case
>  			 */
>  			free = total > used ? total - used : 0;
> -			/* For quota size less than 4KB, report the
> +			/*
> +			 * For quota size less than 4KB, report the
>  			 * total=used=4KB,free=0 when quota is full
> -			 * and total=free=4KB, used=0 otherwise */
> +			 * and total=free=4KB, used=0 otherwise
> +			 */
>  			if (!total) {
>  				total = 1;
>  				free = ci->i_max_bytes > ci->i_rbytes ? 1 : 0;
> -- 
>
> 2.36.0.rc1
>