linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] xfs: change available ranges in quota check
@ 2012-01-23  3:45 Mitsuo Hayasaka
  2012-01-23  3:45 ` [PATCH 1/3] xfs: consider new reservation for quota check on inode reservation Mitsuo Hayasaka
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Mitsuo Hayasaka @ 2012-01-23  3:45 UTC (permalink / raw)
  To: Ben Myers, Alex Elder, Christoph Hellwig
  Cc: xfs-masters, xfs, linux-kernel, yrl.pp-manager.tt

Hi,

This patch series changes available ranges of softlimit and
hardlimit in quota check, as follows.

(1) Consider new reservation for quota check
    The disk block reservation checks if (current usage + new
    reservation) reach the quota limit although the inode reservation
    does not use the new reservation for quota check. It should
    consider it, as well. This is mandatory for (2).

(2) Change available ranges of softlimit and hardlimit in quota check
    Currently xfs quota allows us to use disk blocks and inodes
    up to lower than quota limits. They should be used up to
    not greater than the limits.

Thanks,

---

Mitsuo Hayasaka (3):
      xfs: cleanup quota check on disk blocks and inodes reservations
      xfs: change available ranges of softlimit and hardlimit in quota check
      xfs: consider new reservation for quota check on inode reservation


 fs/xfs/xfs_dquot.c       |   24 ++++++++++++------------
 fs/xfs/xfs_log_recover.c |    6 +++---
 fs/xfs/xfs_qm_syscalls.c |    4 ++--
 fs/xfs/xfs_trans_dquot.c |   15 +++++++--------
 4 files changed, 24 insertions(+), 25 deletions(-)

-- 
Mitsuo Hayasaka (mitsuo.hayasaka.hu@hitachi.com)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] xfs: consider new reservation for quota check on inode reservation
  2012-01-23  3:45 [PATCH 0/3] xfs: change available ranges in quota check Mitsuo Hayasaka
@ 2012-01-23  3:45 ` Mitsuo Hayasaka
  2012-01-23  3:45 ` [PATCH 2/3] xfs: change available ranges of softlimit and hardlimit in quota check Mitsuo Hayasaka
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Mitsuo Hayasaka @ 2012-01-23  3:45 UTC (permalink / raw)
  To: Ben Myers, Alex Elder, Christoph Hellwig
  Cc: xfs-masters, xfs, linux-kernel, yrl.pp-manager.tt,
	Mitsuo Hayasaka, Ben Myers, Alex Elder, Christoph Hellwig

The xfs checks quota when reserving disk blocks and inodes. In the block
reservation it checks if the total number of blocks including current
usage and new reservation reach quota. However, in the inode reservation
it checks using the total number of inodes including only current usage
without new reservation. It should include the new one, as well.

Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
Cc: Ben Myers <bpm@sgi.com>
Cc: Alex Elder <elder@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
---

 fs/xfs/xfs_trans_dquot.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index 4d00ee6..eb29fe1 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -677,11 +677,13 @@ xfs_trans_dqresv(
 			if (!softlimit)
 				softlimit = q->qi_isoftlimit;
 
-			if (hardlimit > 0ULL && count >= hardlimit) {
+			if (hardlimit > 0ULL &&
+			    hardlimit <= ninos + count) {
 				xfs_quota_warn(mp, dqp, QUOTA_NL_IHARDWARN);
 				goto error_return;
 			}
-			if (softlimit > 0ULL && count >= softlimit) {
+			if (softlimit > 0ULL &&
+			    softlimit <= ninos + count) {
 				if  ((timer != 0 && get_seconds() > timer) ||
 				     (warns != 0 && warns >= warnlimit)) {
 					xfs_quota_warn(mp, dqp,


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] xfs: change available ranges of softlimit and hardlimit in quota check
  2012-01-23  3:45 [PATCH 0/3] xfs: change available ranges in quota check Mitsuo Hayasaka
  2012-01-23  3:45 ` [PATCH 1/3] xfs: consider new reservation for quota check on inode reservation Mitsuo Hayasaka
@ 2012-01-23  3:45 ` Mitsuo Hayasaka
  2012-01-23  3:45 ` [PATCH 3/3] xfs: cleanup quota check on disk blocks and inodes reservations Mitsuo Hayasaka
  2012-01-24 17:46 ` [PATCH 0/3] xfs: change available ranges in quota check Christoph Hellwig
  3 siblings, 0 replies; 11+ messages in thread
From: Mitsuo Hayasaka @ 2012-01-23  3:45 UTC (permalink / raw)
  To: Ben Myers, Alex Elder, Christoph Hellwig
  Cc: xfs-masters, xfs, linux-kernel, yrl.pp-manager.tt,
	Mitsuo Hayasaka, Ben Myers, Alex Elder, Christoph Hellwig

In general, quota allows us to use disk blocks and inodes to each limit,
that is, they are available if they don't exceed their limitations.
Current xfs sets their available ranges to lower than them. So, this
patch changes the ranges to not greater than them.

Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
Cc: Ben Myers <bpm@sgi.com>
Cc: Alex Elder <elder@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
---

 fs/xfs/xfs_dquot.c       |   24 ++++++++++++------------
 fs/xfs/xfs_log_recover.c |    6 +++---
 fs/xfs/xfs_qm_syscalls.c |    4 ++--
 fs/xfs/xfs_trans_dquot.c |    8 ++++----
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index b4ff40b..307e4a3 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -215,10 +215,10 @@ xfs_qm_adjust_dqtimers(
 
 	if (!d->d_btimer) {
 		if ((d->d_blk_softlimit &&
-		     (be64_to_cpu(d->d_bcount) >=
+		     (be64_to_cpu(d->d_bcount) >
 		      be64_to_cpu(d->d_blk_softlimit))) ||
 		    (d->d_blk_hardlimit &&
-		     (be64_to_cpu(d->d_bcount) >=
+		     (be64_to_cpu(d->d_bcount) >
 		      be64_to_cpu(d->d_blk_hardlimit)))) {
 			d->d_btimer = cpu_to_be32(get_seconds() +
 					mp->m_quotainfo->qi_btimelimit);
@@ -227,10 +227,10 @@ xfs_qm_adjust_dqtimers(
 		}
 	} else {
 		if ((!d->d_blk_softlimit ||
-		     (be64_to_cpu(d->d_bcount) <
+		     (be64_to_cpu(d->d_bcount) <=
 		      be64_to_cpu(d->d_blk_softlimit))) &&
 		    (!d->d_blk_hardlimit ||
-		    (be64_to_cpu(d->d_bcount) <
+		    (be64_to_cpu(d->d_bcount) <=
 		     be64_to_cpu(d->d_blk_hardlimit)))) {
 			d->d_btimer = 0;
 		}
@@ -238,10 +238,10 @@ xfs_qm_adjust_dqtimers(
 
 	if (!d->d_itimer) {
 		if ((d->d_ino_softlimit &&
-		     (be64_to_cpu(d->d_icount) >=
+		     (be64_to_cpu(d->d_icount) >
 		      be64_to_cpu(d->d_ino_softlimit))) ||
 		    (d->d_ino_hardlimit &&
-		     (be64_to_cpu(d->d_icount) >=
+		     (be64_to_cpu(d->d_icount) >
 		      be64_to_cpu(d->d_ino_hardlimit)))) {
 			d->d_itimer = cpu_to_be32(get_seconds() +
 					mp->m_quotainfo->qi_itimelimit);
@@ -250,10 +250,10 @@ xfs_qm_adjust_dqtimers(
 		}
 	} else {
 		if ((!d->d_ino_softlimit ||
-		     (be64_to_cpu(d->d_icount) <
+		     (be64_to_cpu(d->d_icount) <=
 		      be64_to_cpu(d->d_ino_softlimit)))  &&
 		    (!d->d_ino_hardlimit ||
-		     (be64_to_cpu(d->d_icount) <
+		     (be64_to_cpu(d->d_icount) <=
 		      be64_to_cpu(d->d_ino_hardlimit)))) {
 			d->d_itimer = 0;
 		}
@@ -261,10 +261,10 @@ xfs_qm_adjust_dqtimers(
 
 	if (!d->d_rtbtimer) {
 		if ((d->d_rtb_softlimit &&
-		     (be64_to_cpu(d->d_rtbcount) >=
+		     (be64_to_cpu(d->d_rtbcount) >
 		      be64_to_cpu(d->d_rtb_softlimit))) ||
 		    (d->d_rtb_hardlimit &&
-		     (be64_to_cpu(d->d_rtbcount) >=
+		     (be64_to_cpu(d->d_rtbcount) >
 		      be64_to_cpu(d->d_rtb_hardlimit)))) {
 			d->d_rtbtimer = cpu_to_be32(get_seconds() +
 					mp->m_quotainfo->qi_rtbtimelimit);
@@ -273,10 +273,10 @@ xfs_qm_adjust_dqtimers(
 		}
 	} else {
 		if ((!d->d_rtb_softlimit ||
-		     (be64_to_cpu(d->d_rtbcount) <
+		     (be64_to_cpu(d->d_rtbcount) <=
 		      be64_to_cpu(d->d_rtb_softlimit))) &&
 		    (!d->d_rtb_hardlimit ||
-		     (be64_to_cpu(d->d_rtbcount) <
+		     (be64_to_cpu(d->d_rtbcount) <=
 		      be64_to_cpu(d->d_rtb_hardlimit)))) {
 			d->d_rtbtimer = 0;
 		}
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 541a508..d3ce9ad 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1981,7 +1981,7 @@ xfs_qm_dqcheck(
 
 	if (!errs && ddq->d_id) {
 		if (ddq->d_blk_softlimit &&
-		    be64_to_cpu(ddq->d_bcount) >=
+		    be64_to_cpu(ddq->d_bcount) >
 				be64_to_cpu(ddq->d_blk_softlimit)) {
 			if (!ddq->d_btimer) {
 				if (flags & XFS_QMOPT_DOWARN)
@@ -1992,7 +1992,7 @@ xfs_qm_dqcheck(
 			}
 		}
 		if (ddq->d_ino_softlimit &&
-		    be64_to_cpu(ddq->d_icount) >=
+		    be64_to_cpu(ddq->d_icount) >
 				be64_to_cpu(ddq->d_ino_softlimit)) {
 			if (!ddq->d_itimer) {
 				if (flags & XFS_QMOPT_DOWARN)
@@ -2003,7 +2003,7 @@ xfs_qm_dqcheck(
 			}
 		}
 		if (ddq->d_rtb_softlimit &&
-		    be64_to_cpu(ddq->d_rtbcount) >=
+		    be64_to_cpu(ddq->d_rtbcount) >
 				be64_to_cpu(ddq->d_rtb_softlimit)) {
 			if (!ddq->d_rtbtimer) {
 				if (flags & XFS_QMOPT_DOWARN)
diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c
index eafbcff..711a86e 100644
--- a/fs/xfs/xfs_qm_syscalls.c
+++ b/fs/xfs/xfs_qm_syscalls.c
@@ -813,11 +813,11 @@ xfs_qm_export_dquot(
 	     (XFS_IS_OQUOTA_ENFORCED(mp) &&
 			(dst->d_flags & (FS_PROJ_QUOTA | FS_GROUP_QUOTA)))) &&
 	    dst->d_id != 0) {
-		if (((int) dst->d_bcount >= (int) dst->d_blk_softlimit) &&
+		if (((int) dst->d_bcount > (int) dst->d_blk_softlimit) &&
 		    (dst->d_blk_softlimit > 0)) {
 			ASSERT(dst->d_btimer != 0);
 		}
-		if (((int) dst->d_icount >= (int) dst->d_ino_softlimit) &&
+		if (((int) dst->d_icount > (int) dst->d_ino_softlimit) &&
 		    (dst->d_ino_softlimit > 0)) {
 			ASSERT(dst->d_itimer != 0);
 		}
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index eb29fe1..c4ba366 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -649,12 +649,12 @@ xfs_trans_dqresv(
 			 * nblks.
 			 */
 			if (hardlimit > 0ULL &&
-			    hardlimit <= nblks + *resbcountp) {
+			    hardlimit < nblks + *resbcountp) {
 				xfs_quota_warn(mp, dqp, QUOTA_NL_BHARDWARN);
 				goto error_return;
 			}
 			if (softlimit > 0ULL &&
-			    softlimit <= nblks + *resbcountp) {
+			    softlimit < nblks + *resbcountp) {
 				if ((timer != 0 && get_seconds() > timer) ||
 				    (warns != 0 && warns >= warnlimit)) {
 					xfs_quota_warn(mp, dqp,
@@ -678,12 +678,12 @@ xfs_trans_dqresv(
 				softlimit = q->qi_isoftlimit;
 
 			if (hardlimit > 0ULL &&
-			    hardlimit <= ninos + count) {
+			    hardlimit < ninos + count) {
 				xfs_quota_warn(mp, dqp, QUOTA_NL_IHARDWARN);
 				goto error_return;
 			}
 			if (softlimit > 0ULL &&
-			    softlimit <= ninos + count) {
+			    softlimit < ninos + count) {
 				if  ((timer != 0 && get_seconds() > timer) ||
 				     (warns != 0 && warns >= warnlimit)) {
 					xfs_quota_warn(mp, dqp,


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] xfs: cleanup quota check on disk blocks and inodes reservations
  2012-01-23  3:45 [PATCH 0/3] xfs: change available ranges in quota check Mitsuo Hayasaka
  2012-01-23  3:45 ` [PATCH 1/3] xfs: consider new reservation for quota check on inode reservation Mitsuo Hayasaka
  2012-01-23  3:45 ` [PATCH 2/3] xfs: change available ranges of softlimit and hardlimit in quota check Mitsuo Hayasaka
@ 2012-01-23  3:45 ` Mitsuo Hayasaka
  2012-02-02 16:07   ` Christoph Hellwig
  2012-01-24 17:46 ` [PATCH 0/3] xfs: change available ranges in quota check Christoph Hellwig
  3 siblings, 1 reply; 11+ messages in thread
From: Mitsuo Hayasaka @ 2012-01-23  3:45 UTC (permalink / raw)
  To: Ben Myers, Alex Elder, Christoph Hellwig
  Cc: xfs-masters, xfs, linux-kernel, yrl.pp-manager.tt,
	Mitsuo Hayasaka, Ben Myers, Alex Elder, Christoph Hellwig

This patch is a cleanup of quota check on disk blocks and inodes
reservations.

Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
Cc: Ben Myers <bpm@sgi.com>
Cc: Alex Elder <elder@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
---

 fs/xfs/xfs_trans_dquot.c |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index c4ba366..5e6cd1c 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -605,7 +605,7 @@ xfs_trans_dqresv(
 	time_t		timer;
 	xfs_qwarncnt_t	warns;
 	xfs_qwarncnt_t	warnlimit;
-	xfs_qcnt_t	count;
+	xfs_qcnt_t	tcount;
 	xfs_qcnt_t	*resbcountp;
 	xfs_quotainfo_t	*q = mp->m_quotainfo;
 
@@ -648,13 +648,12 @@ xfs_trans_dqresv(
 			 * hardlimit or exceed the timelimit if we allocate
 			 * nblks.
 			 */
-			if (hardlimit > 0ULL &&
-			    hardlimit < nblks + *resbcountp) {
+			tcount = *resbcountp + nblks;
+			if (hardlimit > 0ULL && tcount > hardlimit) {
 				xfs_quota_warn(mp, dqp, QUOTA_NL_BHARDWARN);
 				goto error_return;
 			}
-			if (softlimit > 0ULL &&
-			    softlimit < nblks + *resbcountp) {
+			if (softlimit > 0ULL && tcount > softlimit) {
 				if ((timer != 0 && get_seconds() > timer) ||
 				    (warns != 0 && warns >= warnlimit)) {
 					xfs_quota_warn(mp, dqp,
@@ -666,7 +665,7 @@ xfs_trans_dqresv(
 			}
 		}
 		if (ninos > 0) {
-			count = be64_to_cpu(dqp->q_core.d_icount);
+			tcount = be64_to_cpu(dqp->q_core.d_icount) + ninos;
 			timer = be32_to_cpu(dqp->q_core.d_itimer);
 			warns = be16_to_cpu(dqp->q_core.d_iwarns);
 			warnlimit = dqp->q_mount->m_quotainfo->qi_iwarnlimit;
@@ -677,13 +676,11 @@ xfs_trans_dqresv(
 			if (!softlimit)
 				softlimit = q->qi_isoftlimit;
 
-			if (hardlimit > 0ULL &&
-			    hardlimit < ninos + count) {
+			if (hardlimit > 0ULL && tcount > hardlimit) {
 				xfs_quota_warn(mp, dqp, QUOTA_NL_IHARDWARN);
 				goto error_return;
 			}
-			if (softlimit > 0ULL &&
-			    softlimit < ninos + count) {
+			if (softlimit > 0ULL && tcount > softlimit) {
 				if  ((timer != 0 && get_seconds() > timer) ||
 				     (warns != 0 && warns >= warnlimit)) {
 					xfs_quota_warn(mp, dqp,


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] xfs: change available ranges in quota check
  2012-01-23  3:45 [PATCH 0/3] xfs: change available ranges in quota check Mitsuo Hayasaka
                   ` (2 preceding siblings ...)
  2012-01-23  3:45 ` [PATCH 3/3] xfs: cleanup quota check on disk blocks and inodes reservations Mitsuo Hayasaka
@ 2012-01-24 17:46 ` Christoph Hellwig
  2012-01-27  6:21   ` HAYASAKA Mitsuo
  3 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2012-01-24 17:46 UTC (permalink / raw)
  To: Mitsuo Hayasaka
  Cc: Ben Myers, Alex Elder, Christoph Hellwig, xfs-masters,
	yrl.pp-manager.tt, linux-kernel, xfs

On Mon, Jan 23, 2012 at 12:45:14PM +0900, Mitsuo Hayasaka wrote:
> Hi,
> 
> This patch series changes available ranges of softlimit and
> hardlimit in quota check, as follows.
> 
> (1) Consider new reservation for quota check
>     The disk block reservation checks if (current usage + new
>     reservation) reach the quota limit although the inode reservation
>     does not use the new reservation for quota check. It should
>     consider it, as well. This is mandatory for (2).

Can you send a testcase that reproduces issues with the old behaviour?


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] xfs: change available ranges in quota check
  2012-01-24 17:46 ` [PATCH 0/3] xfs: change available ranges in quota check Christoph Hellwig
@ 2012-01-27  6:21   ` HAYASAKA Mitsuo
  2012-01-27 11:02     ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: HAYASAKA Mitsuo @ 2012-01-27  6:21 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Ben Myers, Alex Elder, Christoph Hellwig, xfs-masters, linux-kernel, xfs

Hi Christoph,

(2012/01/25 2:46), Christoph Hellwig wrote:
> On Mon, Jan 23, 2012 at 12:45:14PM +0900, Mitsuo Hayasaka wrote:
>> Hi,
>>
>> This patch series changes available ranges of softlimit and
>> hardlimit in quota check, as follows.
>>
>> (1) Consider new reservation for quota check
>>     The disk block reservation checks if (current usage + new
>>     reservation) reach the quota limit although the inode reservation
>>     does not use the new reservation for quota check. It should
>>     consider it, as well. This is mandatory for (2).
> 
> Can you send a testcase that reproduces issues with the old behaviour?
> 

Regarding (1) related to inode reservation, current xfs works well
because inode is reserved one by one if required.

For example, when an new inode tries to be reserved in xfs_trans_dqresv(),
it checks quota as follows.

if (ninos > 0) {	// ninos: # of new inodes expected to be allocated.
	if (cur_usage >= limit)
		goto error;

	cur_usage += ninos;
}

It checks only current usage, but this code works well since the caller
always set the argument ninos to 1 or 0 in current xfs. So, inode can be
used up to the limits due to the one by one reservation.

To make it more general, this check should be the same way as the new
block quota check introduced in the PATCH 2/3 where the disk block can
be used up to the block quota limits.

So, this part is a kind of cleanup patch. I'm going to split the part
into another patch, and to send a new patch series.

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] xfs: change available ranges in quota check
  2012-01-27  6:21   ` HAYASAKA Mitsuo
@ 2012-01-27 11:02     ` Christoph Hellwig
  2012-01-27 14:02       ` HAYASAKA Mitsuo
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2012-01-27 11:02 UTC (permalink / raw)
  To: HAYASAKA Mitsuo
  Cc: Christoph Hellwig, linux-kernel, xfs, xfs-masters, Ben Myers,
	Alex Elder, Christoph Hellwig

On Fri, Jan 27, 2012 at 03:21:02PM +0900, HAYASAKA Mitsuo wrote:
> > Can you send a testcase that reproduces issues with the old behaviour?
> > 
> 
> Regarding (1) related to inode reservation, current xfs works well
> because inode is reserved one by one if required.
> 
> For example, when an new inode tries to be reserved in xfs_trans_dqresv(),
> it checks quota as follows.

I'm just curious what the intent behdind the patches was.  They look
good to me, but I wonder why we need to change it at all.

> To make it more general, this check should be the same way as the new
> block quota check introduced in the PATCH 2/3 where the disk block can
> be used up to the block quota limits.

So I guess that's the part we'd want a test case for if possible.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] xfs: change available ranges in quota check
  2012-01-27 11:02     ` Christoph Hellwig
@ 2012-01-27 14:02       ` HAYASAKA Mitsuo
  2012-01-27 14:04         ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: HAYASAKA Mitsuo @ 2012-01-27 14:02 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-kernel, xfs, xfs-masters, Ben Myers, Alex Elder, Christoph Hellwig

Hi Christoph,

I'd like to explain the reason why I sent the patch series.

Here is an example where I activated user quota and set each softlimit and
hardlimit as follows.

       | softlimit | hardlimit 
-------------------------------
 block |    1M     |    2M
-------------------------------
 inode |     3     |     5

I succeeded to create files up to the inode hardlimit using touch command.
The quota information is shown as follows.

# xfs_quota -x -c 'report -u -b -i -h' /mnt/xfs2
User quota on /mnt/xfs2 (/dev/vdb)
                        Blocks                            Inodes              
User ID      Used   Soft   Hard Warn/Grace     Used   Soft   Hard Warn/Grace  
---------- --------------------------------- --------------------------------- 
root            0      0      0  00 [------]      3      0      0  00 [------]
xfstest01       0     1M     2M  00 [------]      5      3      5  00 [6 days]
						 ~~~~	        ~~

However, I failed to create and add another file due to the quota limitation.

$ touch /mnt/xfs2/dir00/file05
touch: cannot touch `/mnt/xfs2/dir00/file05': Disk quota exceeded

It seems the inode quota works well.

Regarding the block quota, I got the quota limitation message even if I
created a 2MB file which is equal to the hardlimit of disk quota.

$ dd if=/dev/zero of=/mnt/xfs2/dir00/file01 bs=2M count=1
dd: writing `/mnt/xfs2/dir00/file01': Disk quota exceeded
1+0 records in			     ~~~~~~~~~~~~~~~~~~~~~
0+0 records out
2093056 bytes (2.1 MB) copied, 0.00561516 s, 373 MB/s

I'd like to change the available range of the block quota, and
also change the inode quota check to the same way as the block check
introduced in PATCH 2/3 to make it more general.

Regards.


(2012/01/27 20:02), Christoph Hellwig wrote:
> On Fri, Jan 27, 2012 at 03:21:02PM +0900, HAYASAKA Mitsuo wrote:
>>> Can you send a testcase that reproduces issues with the old behaviour?
>>>
>>
>> Regarding (1) related to inode reservation, current xfs works well
>> because inode is reserved one by one if required.
>>
>> For example, when an new inode tries to be reserved in xfs_trans_dqresv(),
>> it checks quota as follows.
> 
> I'm just curious what the intent behdind the patches was.  They look
> good to me, but I wonder why we need to change it at all.
> 
>> To make it more general, this check should be the same way as the new
>> block quota check introduced in the PATCH 2/3 where the disk block can
>> be used up to the block quota limits.
> 
> So I guess that's the part we'd want a test case for if possible.
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/3] xfs: change available ranges in quota check
  2012-01-27 14:02       ` HAYASAKA Mitsuo
@ 2012-01-27 14:04         ` Christoph Hellwig
  0 siblings, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2012-01-27 14:04 UTC (permalink / raw)
  To: HAYASAKA Mitsuo
  Cc: Christoph Hellwig, linux-kernel, xfs, xfs-masters, Ben Myers,
	Alex Elder, Christoph Hellwig

On Fri, Jan 27, 2012 at 11:02:11PM +0900, HAYASAKA Mitsuo wrote:
> Hi Christoph,
> 
> I'd like to explain the reason why I sent the patch series.
> 
> Here is an example where I activated user quota and set each softlimit and
> hardlimit as follows.
> 
>        | softlimit | hardlimit 
> -------------------------------
>  block |    1M     |    2M
> -------------------------------
>  inode |     3     |     5
> 
> I succeeded to create files up to the inode hardlimit using touch command.
> The quota information is shown as follows.
> 
> # xfs_quota -x -c 'report -u -b -i -h' /mnt/xfs2
> User quota on /mnt/xfs2 (/dev/vdb)
>                         Blocks                            Inodes              
> User ID      Used   Soft   Hard Warn/Grace     Used   Soft   Hard Warn/Grace  
> ---------- --------------------------------- --------------------------------- 
> root            0      0      0  00 [------]      3      0      0  00 [------]
> xfstest01       0     1M     2M  00 [------]      5      3      5  00 [6 days]
> 						 ~~~~	        ~~
> 
> However, I failed to create and add another file due to the quota limitation.
> 
> $ touch /mnt/xfs2/dir00/file05
> touch: cannot touch `/mnt/xfs2/dir00/file05': Disk quota exceeded
> 
> It seems the inode quota works well.
> 
> Regarding the block quota, I got the quota limitation message even if I
> created a 2MB file which is equal to the hardlimit of disk quota.
> 
> $ dd if=/dev/zero of=/mnt/xfs2/dir00/file01 bs=2M count=1
> dd: writing `/mnt/xfs2/dir00/file01': Disk quota exceeded
> 1+0 records in			     ~~~~~~~~~~~~~~~~~~~~~
> 0+0 records out
> 2093056 bytes (2.1 MB) copied, 0.00561516 s, 373 MB/s
> 
> I'd like to change the available range of the block quota, and
> also change the inode quota check to the same way as the block check
> introduced in PATCH 2/3 to make it more general.

Makes sense.  Can you create an xfstests testcase containing the above
test case?


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] xfs: cleanup quota check on disk blocks and inodes reservations
  2012-01-23  3:45 ` [PATCH 3/3] xfs: cleanup quota check on disk blocks and inodes reservations Mitsuo Hayasaka
@ 2012-02-02 16:07   ` Christoph Hellwig
  2012-02-03  4:05     ` HAYASAKA Mitsuo
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2012-02-02 16:07 UTC (permalink / raw)
  To: Mitsuo Hayasaka
  Cc: Ben Myers, Alex Elder, Christoph Hellwig, Alex Elder,
	linux-kernel, xfs, xfs-masters, yrl.pp-manager.tt

On Mon, Jan 23, 2012 at 12:45:43PM +0900, Mitsuo Hayasaka wrote:
> This patch is a cleanup of quota check on disk blocks and inodes
> reservations.
> 
> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
> Cc: Ben Myers <bpm@sgi.com>
> Cc: Alex Elder <elder@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> ---
> 
>  fs/xfs/xfs_trans_dquot.c |   17 +++++++----------
>  1 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
> index c4ba366..5e6cd1c 100644
> --- a/fs/xfs/xfs_trans_dquot.c
> +++ b/fs/xfs/xfs_trans_dquot.c
> @@ -605,7 +605,7 @@ xfs_trans_dqresv(
>  	time_t		timer;
>  	xfs_qwarncnt_t	warns;
>  	xfs_qwarncnt_t	warnlimit;
> -	xfs_qcnt_t	count;
> +	xfs_qcnt_t	tcount;

Can you call this variable total_count to make it a bit more obvious?

> +			tcount = *resbcountp + nblks;
> +			if (hardlimit > 0ULL && tcount > hardlimit) {

Given that xfs_qcnt_t is unsigned what about transforming this into the
more readable:

			if (hardlimit && total_count > hardlimit)

and similar for the others?


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] xfs: cleanup quota check on disk blocks and inodes reservations
  2012-02-02 16:07   ` Christoph Hellwig
@ 2012-02-03  4:05     ` HAYASAKA Mitsuo
  0 siblings, 0 replies; 11+ messages in thread
From: HAYASAKA Mitsuo @ 2012-02-03  4:05 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Ben Myers, Alex Elder, Christoph Hellwig, Alex Elder,
	linux-kernel, xfs, xfs-masters

Hi Christoph

Thank you for your comments.

(2012/02/03 1:07), Christoph Hellwig wrote:
> On Mon, Jan 23, 2012 at 12:45:43PM +0900, Mitsuo Hayasaka wrote:
>> This patch is a cleanup of quota check on disk blocks and inodes
>> reservations.
>>
>> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
>> Cc: Ben Myers <bpm@sgi.com>
>> Cc: Alex Elder <elder@kernel.org>
>> Cc: Christoph Hellwig <hch@lst.de>
>> ---
>>
>>  fs/xfs/xfs_trans_dquot.c |   17 +++++++----------
>>  1 files changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
>> index c4ba366..5e6cd1c 100644
>> --- a/fs/xfs/xfs_trans_dquot.c
>> +++ b/fs/xfs/xfs_trans_dquot.c
>> @@ -605,7 +605,7 @@ xfs_trans_dqresv(
>>  	time_t		timer;
>>  	xfs_qwarncnt_t	warns;
>>  	xfs_qwarncnt_t	warnlimit;
>> -	xfs_qcnt_t	count;
>> +	xfs_qcnt_t	tcount;
> 
> Can you call this variable total_count to make it a bit more obvious?

Sure.

> 
>> +			tcount = *resbcountp + nblks;
>> +			if (hardlimit > 0ULL && tcount > hardlimit) {
> 
> Given that xfs_qcnt_t is unsigned what about transforming this into the
> more readable:
> 
> 			if (hardlimit && total_count > hardlimit)
> 
> and similar for the others?
> 

OK, I'm going to change them and send the revised patch series.

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-02-03  4:06 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-23  3:45 [PATCH 0/3] xfs: change available ranges in quota check Mitsuo Hayasaka
2012-01-23  3:45 ` [PATCH 1/3] xfs: consider new reservation for quota check on inode reservation Mitsuo Hayasaka
2012-01-23  3:45 ` [PATCH 2/3] xfs: change available ranges of softlimit and hardlimit in quota check Mitsuo Hayasaka
2012-01-23  3:45 ` [PATCH 3/3] xfs: cleanup quota check on disk blocks and inodes reservations Mitsuo Hayasaka
2012-02-02 16:07   ` Christoph Hellwig
2012-02-03  4:05     ` HAYASAKA Mitsuo
2012-01-24 17:46 ` [PATCH 0/3] xfs: change available ranges in quota check Christoph Hellwig
2012-01-27  6:21   ` HAYASAKA Mitsuo
2012-01-27 11:02     ` Christoph Hellwig
2012-01-27 14:02       ` HAYASAKA Mitsuo
2012-01-27 14:04         ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).