diff mbox series

[v1,1/1] ceph: Fix incorrect statfs report for small quota

Message ID 20211109091041.121750-2-khiremat@redhat.com (mailing list archive)
State New, archived
Headers show
Series ceph: Fix incorrect statfs report | expand

Commit Message

Kotresh Hiremath Ravishankar Nov. 9, 2021, 9:10 a.m. UTC
From: Kotresh HR <khiremat@redhat.com>

Problem:
The statfs reports incorrect free/available space
for quota less then CEPH_BLOCK size (4M).

Solution:
For quotas less than CEPH_BLOCK size, it is
decided to go with binary use/free of full block.
For quota size less than CEPH_BLOCK size, report
the total=used=CEPH_BLOCK,free=0 when quota is
full and total=free=CEPH_BLOCK, used=0 otherwise.

Signed-off-by: Kotresh HR <khiremat@redhat.com>
---
 fs/ceph/quota.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Luís Henriques Nov. 9, 2021, 2:43 p.m. UTC | #1
On Tue, Nov 09, 2021 at 02:40:41PM +0530, khiremat@redhat.com wrote:
> From: Kotresh HR <khiremat@redhat.com>
> 
> Problem:
> The statfs reports incorrect free/available space
> for quota less then CEPH_BLOCK size (4M).
> 
> Solution:
> For quotas less than CEPH_BLOCK size, it is
> decided to go with binary use/free of full block.
> For quota size less than CEPH_BLOCK size, report
> the total=used=CEPH_BLOCK,free=0 when quota is
> full and total=free=CEPH_BLOCK, used=0 otherwise.

This sounds good to me, although it's still not really accurate, as it
will always use the block size granularity.  However, using these small
values for quotas isn't probably quite common anyway, so meh.

> Signed-off-by: Kotresh HR <khiremat@redhat.com>
> ---
>  fs/ceph/quota.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c
> index 620c691af40e..d49ba82d08bf 100644
> --- a/fs/ceph/quota.c
> +++ b/fs/ceph/quota.c
> @@ -505,6 +505,22 @@ bool ceph_quota_update_statfs(struct ceph_fs_client *fsc, struct kstatfs *buf)
>  			buf->f_bfree = free;
>  			buf->f_bavail = free;
>  			is_updated = true;
> +		} else if (ci->i_max_bytes) {
> +			/* For quota size less than CEPH_BLOCK size, report
> +			 * the total=used=CEPH_BLOCK,free=0 when quota is full and
> +			 * total=free=CEPH_BLOCK, used=0 otherwise  */
> +			total = ci->i_max_bytes;
> +			used = ci->i_rbytes;
> +
> +			buf->f_blocks = 1;
> +			if (total > used) {
> +				buf->f_bfree = 1;
> +				buf->f_bavail = 1;
> +			} else {
> +				buf->f_bfree = 0;
> +				buf->f_bavail = 0;
> +			}
> +			is_updated = true;
>  		}
>  		iput(in);
>  	}
> -- 
> 2.31.1
> 

I think it would be more correct to move this logic into the spinlock
protected code.  Which would even make the code simpler, something like:

	spin_lock(&ci->i_ceph_lock);
	if (ci->i_max_bytes) {
		total = ci->i_max_bytes >> CEPH_BLOCK_SHIFT;
		if (total) {
			/* ... */
		} else {
			total = 1;
			free = ci->i_max_bytes > ci->i_rbytes ? 1 : 0;
		}
	}
	spin_unlock(&ci->i_ceph_lock);

I guess the 'if (total)' condition (and the 'is_updated' variable) can
also be removed.

Cheers,
--
Luís
Jeff Layton Nov. 9, 2021, 5:02 p.m. UTC | #2
On Tue, 2021-11-09 at 14:40 +0530, khiremat@redhat.com wrote:
> From: Kotresh HR <khiremat@redhat.com>
> 
> Problem:
> The statfs reports incorrect free/available space
> for quota less then CEPH_BLOCK size (4M).
> 
> Solution:
> For quotas less than CEPH_BLOCK size, it is
> decided to go with binary use/free of full block.
> For quota size less than CEPH_BLOCK size, report
> the total=used=CEPH_BLOCK,free=0 when quota is
> full and total=free=CEPH_BLOCK, used=0 otherwise.
> 
> Signed-off-by: Kotresh HR <khiremat@redhat.com>
> ---
>  fs/ceph/quota.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c
> index 620c691af40e..d49ba82d08bf 100644
> --- a/fs/ceph/quota.c
> +++ b/fs/ceph/quota.c
> @@ -505,6 +505,22 @@ bool ceph_quota_update_statfs(struct ceph_fs_client *fsc, struct kstatfs *buf)
>  			buf->f_bfree = free;
>  			buf->f_bavail = free;
>  			is_updated = true;
> +		} else if (ci->i_max_bytes) {
> +			/* For quota size less than CEPH_BLOCK size, report
> +			 * the total=used=CEPH_BLOCK,free=0 when quota is full and
> +			 * total=free=CEPH_BLOCK, used=0 otherwise  */
> +			total = ci->i_max_bytes;
> +			used = ci->i_rbytes;
> +
> +			buf->f_blocks = 1;
> +			if (total > used) {
> +				buf->f_bfree = 1;
> +				buf->f_bavail = 1;
> +			} else {
> +				buf->f_bfree = 0;
> +				buf->f_bavail = 0;
> +			}
> +			is_updated = true;
>  		}
>  		iput(in);
>  	}

Technically this is (sort of) correct, but the lack of granularity makes
this reporting somewhat useless.

Honestly, I'd rather see this switch to reporting a smaller blocksize
when you get down to <4M max_bytes. If people are setting quotas that
are that small, then they probably would like to be able to look at "df"
and see if they're approaching their quota. This doesn't give them that
capability.

Instead, could we just switch to reporting a 4k blocksize when
i_max_bytes is that small? Then you can multiply the other values by 1k
and it should all be correct.
diff mbox series

Patch

diff --git a/fs/ceph/quota.c b/fs/ceph/quota.c
index 620c691af40e..d49ba82d08bf 100644
--- a/fs/ceph/quota.c
+++ b/fs/ceph/quota.c
@@ -505,6 +505,22 @@  bool ceph_quota_update_statfs(struct ceph_fs_client *fsc, struct kstatfs *buf)
 			buf->f_bfree = free;
 			buf->f_bavail = free;
 			is_updated = true;
+		} else if (ci->i_max_bytes) {
+			/* For quota size less than CEPH_BLOCK size, report
+			 * the total=used=CEPH_BLOCK,free=0 when quota is full and
+			 * total=free=CEPH_BLOCK, used=0 otherwise  */
+			total = ci->i_max_bytes;
+			used = ci->i_rbytes;
+
+			buf->f_blocks = 1;
+			if (total > used) {
+				buf->f_bfree = 1;
+				buf->f_bavail = 1;
+			} else {
+				buf->f_bfree = 0;
+				buf->f_bavail = 0;
+			}
+			is_updated = true;
 		}
 		iput(in);
 	}