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

[Xiubo Li <xiubli@redhat.com>]

On 6/24/22 10:47 PM, Luís Henriques wrote:
> 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.

Sure, will fix it.

> 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.

Yeah, I was also thinking about this, but haven't found which use case 
could be broke yet.

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

Till now this works well and no test case complains it.

-- Xiubo

> Cheers,