[PATCH] quota: additional range checks and mem_dqblk updates to handle 64-bit limits

[PATCH] quota: additional range checks and mem_dqblk updates to handle 64-bit limits

[Andrew Perepechko]

mem_dqblk is extended to hold 64-bit quota limits, checks are added to deny
setting quota limits which aren't supported by a quota format module

Signed-off-by: Andrew Perepechko <andrew.perepechko@sun.com>

---

 fs/dquot.c            |   22 +++++++++++++++++-----
 fs/quota_v1.c         |    5 +++++
 fs/quota_v2.c         |   15 ++++++++++-----
 include/linux/quota.h |   14 +++++++++-----
 4 files changed, 41 insertions(+), 15 deletions(-)

diff -rNpu quota.orig/fs/dquot.c quota/fs/dquot.c
--- quota.orig/fs/dquot.c	2008-01-25 01:58:37.000000000 +0300
+++ quota/fs/dquot.c	2008-03-07 01:37:46.000000000 +0300
@@ -1703,10 +1703,19 @@ int vfs_get_dqblk(struct super_block *sb
 }
 
 /* Generic routine for setting common part of quota structure */
-static void do_set_dqblk(struct dquot *dquot, struct if_dqblk *di)
+static int do_set_dqblk(struct dquot *dquot, struct if_dqblk *di)
 {
 	struct mem_dqblk *dm = &dquot->dq_dqb;
 	int check_blim = 0, check_ilim = 0;
+	struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_type];
+
+	if (((di->dqb_valid & QIF_BLIMITS) &&
+	    ((di->dqb_bhardlimit > dqi->dqi_maxbhardlimit) ||
+	     (di->dqb_bsoftlimit > dqi->dqi_maxbsoftlimit))) ||
+	    ((di->dqb_valid & QIF_ILIMITS) &&
+	    ((di->dqb_ihardlimit > dqi->dqi_maxihardlimit) ||
+	     (di->dqb_isoftlimit > dqi->dqi_maxisoftlimit))))
+		return -ERANGE;
 
 	spin_lock(&dq_data_lock);
 	if (di->dqb_valid & QIF_SPACE) {
@@ -1738,7 +1747,7 @@ static void do_set_dqblk(struct dquot *d
 			clear_bit(DQ_BLKS_B, &dquot->dq_flags);
 		}
 		else if (!(di->dqb_valid & QIF_BTIME))	/* Set grace only if user hasn't provided his own... */
-			dm->dqb_btime = get_seconds() + sb_dqopt(dquot->dq_sb)->info[dquot->dq_type].dqi_bgrace;
+			dm->dqb_btime = get_seconds() + dqi->dqi_bgrace;
 	}
 	if (check_ilim) {
 		if (!dm->dqb_isoftlimit || dm->dqb_curinodes < dm->dqb_isoftlimit) {
@@ -1746,7 +1755,7 @@ static void do_set_dqblk(struct dquot *d
 			clear_bit(DQ_INODES_B, &dquot->dq_flags);
 		}
 		else if (!(di->dqb_valid & QIF_ITIME))	/* Set grace only if user hasn't provided his own... */
-			dm->dqb_itime = get_seconds() + sb_dqopt(dquot->dq_sb)->info[dquot->dq_type].dqi_igrace;
+			dm->dqb_itime = get_seconds() + dqi->dqi_igrace;
 	}
 	if (dm->dqb_bhardlimit || dm->dqb_bsoftlimit || dm->dqb_ihardlimit || dm->dqb_isoftlimit)
 		clear_bit(DQ_FAKE_B, &dquot->dq_flags);
@@ -1754,21 +1763,24 @@ static void do_set_dqblk(struct dquot *d
 		set_bit(DQ_FAKE_B, &dquot->dq_flags);
 	spin_unlock(&dq_data_lock);
 	mark_dquot_dirty(dquot);
+
+	return 0;
 }
 
 int vfs_set_dqblk(struct super_block *sb, int type, qid_t id, struct if_dqblk *di)
 {
 	struct dquot *dquot;
+	int rc;
 
 	mutex_lock(&sb_dqopt(sb)->dqonoff_mutex);
 	if (!(dquot = dqget(sb, id, type))) {
 		mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
 		return -ESRCH;
 	}
-	do_set_dqblk(dquot, di);
+	rc = do_set_dqblk(dquot, di);
 	dqput(dquot);
 	mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
-	return 0;
+	return rc;
 }
 
 /* Generic routine for getting common part of quota file information */
diff -rNpu quota.orig/fs/quota_v1.c quota/fs/quota_v1.c
--- quota.orig/fs/quota_v1.c	2008-01-25 01:58:37.000000000 +0300
+++ quota/fs/quota_v1.c	2008-03-07 01:39:00.000000000 +0300
@@ -139,6 +139,11 @@ static int v1_read_file_info(struct supe
 		goto out;
 	}
 	ret = 0;
+	/* limits are stored as unsigned 32-bit data */
+	dqopt->info[type].dqi_maxbhardlimit = 0xffffffff;
+	dqopt->info[type].dqi_maxbsoftlimit = 0xffffffff;
+	dqopt->info[type].dqi_maxihardlimit = 0xffffffff;
+	dqopt->info[type].dqi_maxisoftlimit = 0xffffffff;
 	dqopt->info[type].dqi_igrace = dqblk.dqb_itime ? dqblk.dqb_itime : MAX_IQ_TIME;
 	dqopt->info[type].dqi_bgrace = dqblk.dqb_btime ? dqblk.dqb_btime : MAX_DQ_TIME;
 out:
diff -rNpu quota.orig/fs/quota_v2.c quota/fs/quota_v2.c
--- quota.orig/fs/quota_v2.c	2008-01-25 01:58:37.000000000 +0300
+++ quota/fs/quota_v2.c	2008-03-07 02:40:23.000000000 +0300
@@ -59,6 +59,11 @@ static int v2_read_file_info(struct supe
 			sb->s_id);
 		return -1;
 	}
+	/* limits are stored as unsigned 32-bit data */
+	info->dqi_maxbhardlimit = 0xffffffff;
+	info->dqi_maxbsoftlimit = 0xffffffff;
+	info->dqi_maxihardlimit = 0xffffffff;
+	info->dqi_maxisoftlimit = 0xffffffff;
 	info->dqi_bgrace = le32_to_cpu(dinfo.dqi_bgrace);
 	info->dqi_igrace = le32_to_cpu(dinfo.dqi_igrace);
 	info->dqi_flags = le32_to_cpu(dinfo.dqi_flags);
@@ -108,12 +113,12 @@ static void disk2memdqb(struct mem_dqblk
 
 static void mem2diskdqb(struct v2_disk_dqblk *d, struct mem_dqblk *m, qid_t id)
 {
-	d->dqb_ihardlimit = cpu_to_le32(m->dqb_ihardlimit);
-	d->dqb_isoftlimit = cpu_to_le32(m->dqb_isoftlimit);
-	d->dqb_curinodes = cpu_to_le32(m->dqb_curinodes);
+	d->dqb_ihardlimit = cpu_to_le32((u32)m->dqb_ihardlimit);
+	d->dqb_isoftlimit = cpu_to_le32((u32)m->dqb_isoftlimit);
+	d->dqb_curinodes = cpu_to_le32((u32)m->dqb_curinodes);
 	d->dqb_itime = cpu_to_le64(m->dqb_itime);
-	d->dqb_bhardlimit = cpu_to_le32(m->dqb_bhardlimit);
-	d->dqb_bsoftlimit = cpu_to_le32(m->dqb_bsoftlimit);
+	d->dqb_bhardlimit = cpu_to_le32((u32)m->dqb_bhardlimit);
+	d->dqb_bsoftlimit = cpu_to_le32((u32)m->dqb_bsoftlimit);
 	d->dqb_curspace = cpu_to_le64(m->dqb_curspace);
 	d->dqb_btime = cpu_to_le64(m->dqb_btime);
 	d->dqb_id = cpu_to_le32(id);
diff -rNpu quota.orig/include/linux/quota.h quota/include/linux/quota.h
--- quota.orig/include/linux/quota.h	2008-01-25 01:58:37.000000000 +0300
+++ quota/include/linux/quota.h	2008-03-06 23:18:10.000000000 +0300
@@ -181,12 +181,12 @@ extern spinlock_t dq_data_lock;
  * Data for one user/group kept in memory
  */
 struct mem_dqblk {
-	__u32 dqb_bhardlimit;	/* absolute limit on disk blks alloc */
-	__u32 dqb_bsoftlimit;	/* preferred limit on disk blks */
+	qsize_t dqb_bhardlimit;	/* absolute limit on disk blks alloc */
+	qsize_t dqb_bsoftlimit;	/* preferred limit on disk blks */
 	qsize_t dqb_curspace;	/* current used space */
-	__u32 dqb_ihardlimit;	/* absolute limit on allocated inodes */
-	__u32 dqb_isoftlimit;	/* preferred inode limit */
-	__u32 dqb_curinodes;	/* current # allocated inodes */
+	qsize_t dqb_ihardlimit;	/* absolute limit on allocated inodes */
+	qsize_t dqb_isoftlimit;	/* preferred inode limit */
+	qsize_t dqb_curinodes;	/* current # allocated inodes */
 	time_t dqb_btime;	/* time limit for excessive disk use */
 	time_t dqb_itime;	/* time limit for excessive inode use */
 };
@@ -202,6 +202,10 @@ struct mem_dqinfo {
 	unsigned long dqi_flags;
 	unsigned int dqi_bgrace;
 	unsigned int dqi_igrace;
+	qsize_t dqi_maxbhardlimit;
+	qsize_t dqi_maxbsoftlimit;
+	qsize_t dqi_maxihardlimit;
+	qsize_t dqi_maxisoftlimit;
 	union {
 		struct v1_mem_dqinfo v1_i;
 		struct v2_mem_dqinfo v2_i;

Re: [PATCH] quota: additional range checks and mem_dqblk updates to handle 64-bit limits

[Jan Kara]

On Fri 07-03-08 03:29:29, Andrew Perepechko wrote:
> mem_dqblk is extended to hold 64-bit quota limits, checks are added to deny
> setting quota limits which aren't supported by a quota format module
> 
> Signed-off-by: Andrew Perepechko <andrew.perepechko@sun.com>
  Great, thanks. The patch is fine. Yesterday evening I got an idea, how to
solve your problem with too low limits even easier. What we could do is to
introduce a "block-limit-scale" and "inode-limit-scale" parameter to the
quota info and we keep the rest of the file format the same. Now, the meaning
of this parameter would simply be a unit in which space and inode limits
are specified. When you have a filesystem where you'd like to set quotas
over 4 TB, you probably don't want to specify limits with 1KB precision
anyway... So you can just set scale to 1MB or even 16MB (giving you maximal
limit of 64 PB) and 10000 files or so. This has two advantages - only a few
trivial modifications to current kernel code, no change in quota file space
usage. We could then provide a way to set this scale via setquota / edquota
(which would have to convert the whole file but that should be no big deal).
  What do you think about such solution? Would it fit your needs? Sorry,
that I haven't through of this solution earlier...
								Honza
> 
> ---
> 
>  fs/dquot.c            |   22 +++++++++++++++++-----
>  fs/quota_v1.c         |    5 +++++
>  fs/quota_v2.c         |   15 ++++++++++-----
>  include/linux/quota.h |   14 +++++++++-----
>  4 files changed, 41 insertions(+), 15 deletions(-)
> 
> diff -rNpu quota.orig/fs/dquot.c quota/fs/dquot.c
> --- quota.orig/fs/dquot.c	2008-01-25 01:58:37.000000000 +0300
> +++ quota/fs/dquot.c	2008-03-07 01:37:46.000000000 +0300
> @@ -1703,10 +1703,19 @@ int vfs_get_dqblk(struct super_block *sb
>  }
>  
>  /* Generic routine for setting common part of quota structure */
> -static void do_set_dqblk(struct dquot *dquot, struct if_dqblk *di)
> +static int do_set_dqblk(struct dquot *dquot, struct if_dqblk *di)
>  {
>  	struct mem_dqblk *dm = &dquot->dq_dqb;
>  	int check_blim = 0, check_ilim = 0;
> +	struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_type];
> +
> +	if (((di->dqb_valid & QIF_BLIMITS) &&
> +	    ((di->dqb_bhardlimit > dqi->dqi_maxbhardlimit) ||
> +	     (di->dqb_bsoftlimit > dqi->dqi_maxbsoftlimit))) ||
> +	    ((di->dqb_valid & QIF_ILIMITS) &&
> +	    ((di->dqb_ihardlimit > dqi->dqi_maxihardlimit) ||
> +	     (di->dqb_isoftlimit > dqi->dqi_maxisoftlimit))))
> +		return -ERANGE;
>  
>  	spin_lock(&dq_data_lock);
>  	if (di->dqb_valid & QIF_SPACE) {
> @@ -1738,7 +1747,7 @@ static void do_set_dqblk(struct dquot *d
>  			clear_bit(DQ_BLKS_B, &dquot->dq_flags);
>  		}
>  		else if (!(di->dqb_valid & QIF_BTIME))	/* Set grace only if user hasn't provided his own... */
> -			dm->dqb_btime = get_seconds() + sb_dqopt(dquot->dq_sb)->info[dquot->dq_type].dqi_bgrace;
> +			dm->dqb_btime = get_seconds() + dqi->dqi_bgrace;
>  	}
>  	if (check_ilim) {
>  		if (!dm->dqb_isoftlimit || dm->dqb_curinodes < dm->dqb_isoftlimit) {
> @@ -1746,7 +1755,7 @@ static void do_set_dqblk(struct dquot *d
>  			clear_bit(DQ_INODES_B, &dquot->dq_flags);
>  		}
>  		else if (!(di->dqb_valid & QIF_ITIME))	/* Set grace only if user hasn't provided his own... */
> -			dm->dqb_itime = get_seconds() + sb_dqopt(dquot->dq_sb)->info[dquot->dq_type].dqi_igrace;
> +			dm->dqb_itime = get_seconds() + dqi->dqi_igrace;
>  	}
>  	if (dm->dqb_bhardlimit || dm->dqb_bsoftlimit || dm->dqb_ihardlimit || dm->dqb_isoftlimit)
>  		clear_bit(DQ_FAKE_B, &dquot->dq_flags);
> @@ -1754,21 +1763,24 @@ static void do_set_dqblk(struct dquot *d
>  		set_bit(DQ_FAKE_B, &dquot->dq_flags);
>  	spin_unlock(&dq_data_lock);
>  	mark_dquot_dirty(dquot);
> +
> +	return 0;
>  }
>  
>  int vfs_set_dqblk(struct super_block *sb, int type, qid_t id, struct if_dqblk *di)
>  {
>  	struct dquot *dquot;
> +	int rc;
>  
>  	mutex_lock(&sb_dqopt(sb)->dqonoff_mutex);
>  	if (!(dquot = dqget(sb, id, type))) {
>  		mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
>  		return -ESRCH;
>  	}
> -	do_set_dqblk(dquot, di);
> +	rc = do_set_dqblk(dquot, di);
>  	dqput(dquot);
>  	mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
> -	return 0;
> +	return rc;
>  }
>  
>  /* Generic routine for getting common part of quota file information */
> diff -rNpu quota.orig/fs/quota_v1.c quota/fs/quota_v1.c
> --- quota.orig/fs/quota_v1.c	2008-01-25 01:58:37.000000000 +0300
> +++ quota/fs/quota_v1.c	2008-03-07 01:39:00.000000000 +0300
> @@ -139,6 +139,11 @@ static int v1_read_file_info(struct supe
>  		goto out;
>  	}
>  	ret = 0;
> +	/* limits are stored as unsigned 32-bit data */
> +	dqopt->info[type].dqi_maxbhardlimit = 0xffffffff;
> +	dqopt->info[type].dqi_maxbsoftlimit = 0xffffffff;
> +	dqopt->info[type].dqi_maxihardlimit = 0xffffffff;
> +	dqopt->info[type].dqi_maxisoftlimit = 0xffffffff;
>  	dqopt->info[type].dqi_igrace = dqblk.dqb_itime ? dqblk.dqb_itime : MAX_IQ_TIME;
>  	dqopt->info[type].dqi_bgrace = dqblk.dqb_btime ? dqblk.dqb_btime : MAX_DQ_TIME;
>  out:
> diff -rNpu quota.orig/fs/quota_v2.c quota/fs/quota_v2.c
> --- quota.orig/fs/quota_v2.c	2008-01-25 01:58:37.000000000 +0300
> +++ quota/fs/quota_v2.c	2008-03-07 02:40:23.000000000 +0300
> @@ -59,6 +59,11 @@ static int v2_read_file_info(struct supe
>  			sb->s_id);
>  		return -1;
>  	}
> +	/* limits are stored as unsigned 32-bit data */
> +	info->dqi_maxbhardlimit = 0xffffffff;
> +	info->dqi_maxbsoftlimit = 0xffffffff;
> +	info->dqi_maxihardlimit = 0xffffffff;
> +	info->dqi_maxisoftlimit = 0xffffffff;
>  	info->dqi_bgrace = le32_to_cpu(dinfo.dqi_bgrace);
>  	info->dqi_igrace = le32_to_cpu(dinfo.dqi_igrace);
>  	info->dqi_flags = le32_to_cpu(dinfo.dqi_flags);
> @@ -108,12 +113,12 @@ static void disk2memdqb(struct mem_dqblk
>  
>  static void mem2diskdqb(struct v2_disk_dqblk *d, struct mem_dqblk *m, qid_t id)
>  {
> -	d->dqb_ihardlimit = cpu_to_le32(m->dqb_ihardlimit);
> -	d->dqb_isoftlimit = cpu_to_le32(m->dqb_isoftlimit);
> -	d->dqb_curinodes = cpu_to_le32(m->dqb_curinodes);
> +	d->dqb_ihardlimit = cpu_to_le32((u32)m->dqb_ihardlimit);
> +	d->dqb_isoftlimit = cpu_to_le32((u32)m->dqb_isoftlimit);
> +	d->dqb_curinodes = cpu_to_le32((u32)m->dqb_curinodes);
>  	d->dqb_itime = cpu_to_le64(m->dqb_itime);
> -	d->dqb_bhardlimit = cpu_to_le32(m->dqb_bhardlimit);
> -	d->dqb_bsoftlimit = cpu_to_le32(m->dqb_bsoftlimit);
> +	d->dqb_bhardlimit = cpu_to_le32((u32)m->dqb_bhardlimit);
> +	d->dqb_bsoftlimit = cpu_to_le32((u32)m->dqb_bsoftlimit);
>  	d->dqb_curspace = cpu_to_le64(m->dqb_curspace);
>  	d->dqb_btime = cpu_to_le64(m->dqb_btime);
>  	d->dqb_id = cpu_to_le32(id);
> diff -rNpu quota.orig/include/linux/quota.h quota/include/linux/quota.h
> --- quota.orig/include/linux/quota.h	2008-01-25 01:58:37.000000000 +0300
> +++ quota/include/linux/quota.h	2008-03-06 23:18:10.000000000 +0300
> @@ -181,12 +181,12 @@ extern spinlock_t dq_data_lock;
>   * Data for one user/group kept in memory
>   */
>  struct mem_dqblk {
> -	__u32 dqb_bhardlimit;	/* absolute limit on disk blks alloc */
> -	__u32 dqb_bsoftlimit;	/* preferred limit on disk blks */
> +	qsize_t dqb_bhardlimit;	/* absolute limit on disk blks alloc */
> +	qsize_t dqb_bsoftlimit;	/* preferred limit on disk blks */
>  	qsize_t dqb_curspace;	/* current used space */
> -	__u32 dqb_ihardlimit;	/* absolute limit on allocated inodes */
> -	__u32 dqb_isoftlimit;	/* preferred inode limit */
> -	__u32 dqb_curinodes;	/* current # allocated inodes */
> +	qsize_t dqb_ihardlimit;	/* absolute limit on allocated inodes */
> +	qsize_t dqb_isoftlimit;	/* preferred inode limit */
> +	qsize_t dqb_curinodes;	/* current # allocated inodes */
>  	time_t dqb_btime;	/* time limit for excessive disk use */
>  	time_t dqb_itime;	/* time limit for excessive inode use */
>  };
> @@ -202,6 +202,10 @@ struct mem_dqinfo {
>  	unsigned long dqi_flags;
>  	unsigned int dqi_bgrace;
>  	unsigned int dqi_igrace;
> +	qsize_t dqi_maxbhardlimit;
> +	qsize_t dqi_maxbsoftlimit;
> +	qsize_t dqi_maxihardlimit;
> +	qsize_t dqi_maxisoftlimit;
>  	union {
>  		struct v1_mem_dqinfo v1_i;
>  		struct v2_mem_dqinfo v2_i;
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] quota: additional range checks and mem_dqblk updates to handle 64-bit limits

[Andreas Dilger]

On Mar 07, 2008  17:00 +0100, Jan Kara wrote:
> On Fri 07-03-08 03:29:29, Andrew Perepechko wrote:
>   Great, thanks. The patch is fine. Yesterday evening I got an idea, how to
> solve your problem with too low limits even easier. What we could do is to
> introduce a "block-limit-scale" and "inode-limit-scale" parameter to the
> quota info and we keep the rest of the file format the same. Now, the meaning
> of this parameter would simply be a unit in which space and inode limits
> are specified. When you have a filesystem where you'd like to set quotas
> over 4 TB, you probably don't want to specify limits with 1KB precision
> anyway... So you can just set scale to 1MB or even 16MB (giving you maximal
> limit of 64 PB) and 10000 files or so. This has two advantages - only a few
> trivial modifications to current kernel code, no change in quota file space
> usage. We could then provide a way to set this scale via setquota / edquota
> (which would have to convert the whole file but that should be no big deal).
>   What do you think about such solution? Would it fit your needs? Sorry,
> that I haven't through of this solution earlier...

I can't speak fully for Andrew, as he is one of our quota gurus, but my
thought is that there is a risk of introducing corruption into the quota
file while it is entirely being rewritten and the system crashes or is
rebooted because the admin is impatient if this takes a long time.

Moving to a second quota file is pretty safe, can be done incrementally
(i.e. check new file and then old file, if it exists) and allows a fallback
if the update fails in the middle.


Also, while the "scale" parameter has merit in allowing the upper limit
of quota to be changed, the problem still exists on how to measure the
actual quota usage in that case.  If we assume a scale of 1MB (which is
fine for Lustre, that is the minimum we grant quota to different servers
anyways :-) but some user is only consuming 100k of quota at a time, then
this will continually be rounded down to 0 quota usage...

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: [PATCH] quota: additional range checks and mem_dqblk updates to handle 64-bit limits

[Andrew Perepechko]

Jan, this idea would work for us for now. As Andreas pointed out, we deal 
with MB-aligned quota limits, so this would give us the largest possible block 
quota limit value of 4 PB.

However, I feel that it is worth to implement a clean 64-bit format, so that
we avoid additional semantics (like quota unit size) and do make quota scale 
better (4 PB clusters already exist).

Do you feel it might take considerably larger efforts to update quotatools
to 64-bit version compared to 32-bit version with variable quota unit size?

I'd like to start working on a kernel patch implementing 64-bit limits provided 
you approve the approach.

Of course, it seems to be a good idea to have different file names for each version,
not only for each format (where format and version have the same meaning
as in quota kernel modules). The drawback is that then journalling quota users 
need to know the exact quota file version, not only format...

Thank you for you kind response.
Andrew.


On Friday 07 March 2008 19:00:43 Jan Kara wrote:
>   Great, thanks. The patch is fine. Yesterday evening I got an idea, how to
> solve your problem with too low limits even easier. What we could do is to
> introduce a "block-limit-scale" and "inode-limit-scale" parameter to the
> quota info and we keep the rest of the file format the same. Now, the meaning
> of this parameter would simply be a unit in which space and inode limits
> are specified. When you have a filesystem where you'd like to set quotas
> over 4 TB, you probably don't want to specify limits with 1KB precision
> anyway... So you can just set scale to 1MB or even 16MB (giving you maximal
> limit of 64 PB) and 10000 files or so. This has two advantages - only a few
> trivial modifications to current kernel code, no change in quota file space
> usage. We could then provide a way to set this scale via setquota / edquota
> (which would have to convert the whole file but that should be no big deal).
>   What do you think about such solution? Would it fit your needs? Sorry,
> that I haven't through of this solution earlier...
> 								Honza

Re: [PATCH] quota: additional range checks and mem_dqblk updates to handle 64-bit limits

[Jan Kara]

On Fri 07-03-08 14:10:39, Andreas Dilger wrote:
> On Mar 07, 2008  17:00 +0100, Jan Kara wrote:
> > On Fri 07-03-08 03:29:29, Andrew Perepechko wrote:
> >   Great, thanks. The patch is fine. Yesterday evening I got an idea, how to
> > solve your problem with too low limits even easier. What we could do is to
> > introduce a "block-limit-scale" and "inode-limit-scale" parameter to the
> > quota info and we keep the rest of the file format the same. Now, the meaning
> > of this parameter would simply be a unit in which space and inode limits
> > are specified. When you have a filesystem where you'd like to set quotas
> > over 4 TB, you probably don't want to specify limits with 1KB precision
> > anyway... So you can just set scale to 1MB or even 16MB (giving you maximal
> > limit of 64 PB) and 10000 files or so. This has two advantages - only a few
> > trivial modifications to current kernel code, no change in quota file space
> > usage. We could then provide a way to set this scale via setquota / edquota
> > (which would have to convert the whole file but that should be no big deal).
> >   What do you think about such solution? Would it fit your needs? Sorry,
> > that I haven't through of this solution earlier...
> 
> I can't speak fully for Andrew, as he is one of our quota gurus, but my
> thought is that there is a risk of introducing corruption into the quota
> file while it is entirely being rewritten and the system crashes or is
> rebooted because the admin is impatient if this takes a long time.
> 
> Moving to a second quota file is pretty safe, can be done incrementally
> (i.e. check new file and then old file, if it exists) and allows a fallback
> if the update fails in the middle.
  This rewriting is going to happen from tools in userspace - i.e., you
turn quotas off, run a tool which does the conversion - it will create new
converted file and just it move over the old file when it's done. So I
think this should be no issue.

> Also, while the "scale" parameter has merit in allowing the upper limit
> of quota to be changed, the problem still exists on how to measure the
> actual quota usage in that case.  If we assume a scale of 1MB (which is
> fine for Lustre, that is the minimum we grant quota to different servers
> anyways :-) but some user is only consuming 100k of quota at a time, then
> this will continually be rounded down to 0 quota usage...
  Quota usage is already measured in bytes and the format has 64-bit field
for it already. So that's no problem. But I've just realized we might have
a problem in case we want to allow user to have more that 2^32 files as
the number of files user has is stored in a 32-bit field.

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] quota: additional range checks and mem_dqblk updates?to handle 64-bit limits

[Jan Kara]

On Sat 08-03-08 03:56:17, Andrew Perepechko wrote:
> Jan, this idea would work for us for now. As Andreas pointed out, we deal 
> with MB-aligned quota limits, so this would give us the largest possible block 
> quota limit value of 4 PB.
> 
> However, I feel that it is worth to implement a clean 64-bit format, so that
> we avoid additional semantics (like quota unit size) and do make quota scale 
> better (4 PB clusters already exist).
  Wow. I wonder when 64-bits won't be enough :). Anyway, my point was that
when you get to 4 PB limits, you can just run: "setquota --set-scale 1GB"
(hopefully at that time you won't care about rounding limits to 1GB) and
you are at 4 HB limits (or what is the right suffix). No quota format
change needed.
  But what I fear more is that we may run out of 2^32 limit on the number
of files one user can have (I don't have experience with that large systems
but I guess you are comming near to 2^32 files on the filesystem, aren't
you?). And for that we would have to do basically the changes you've
suggested.

> Do you feel it might take considerably larger efforts to update quotatools
> to 64-bit version compared to 32-bit version with variable quota unit size?
  Well, I don't fear that much about quota tools but more about the kernel
code and duplication of code in there.

> I'd like to start working on a kernel patch implementing 64-bit limits provided 
> you approve the approach.
  I'm glad you're eager to work on that :) I'd just like to think twice
before going for the new format and changing quota code.

> Of course, it seems to be a good idea to have different file names for each version,
> not only for each format (where format and version have the same meaning
> as in quota kernel modules). The drawback is that then journalling quota users 
> need to know the exact quota file version, not only format...
  Hmm, I don't get this. I don't think there's a real need for changing the
quota file name. That is exactly what I'd like to avoid. Why do you think
new quota file name is better? Those conversion things Andreas pointed out,
are solved by doing the conversion into the temporary file...

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] quota: additional range checks and mem_dqblk updates to handle 64-bit limits

[Jan Kara]

On Fri 07-03-08 03:29:29, Andrew Perepechko wrote:
> mem_dqblk is extended to hold 64-bit quota limits, checks are added to deny
> setting quota limits which aren't supported by a quota format module
  Could you just use dqi_maxilimit and dqi_maxblimit as I've suggested in
my other email and base your second patch implementing new version of the
format on this one? That would make my life easier. Thanks.

									Honza
> 
> Signed-off-by: Andrew Perepechko <andrew.perepechko@sun.com>
> 
> ---
> 
>  fs/dquot.c            |   22 +++++++++++++++++-----
>  fs/quota_v1.c         |    5 +++++
>  fs/quota_v2.c         |   15 ++++++++++-----
>  include/linux/quota.h |   14 +++++++++-----
>  4 files changed, 41 insertions(+), 15 deletions(-)
> 
> diff -rNpu quota.orig/fs/dquot.c quota/fs/dquot.c
> --- quota.orig/fs/dquot.c	2008-01-25 01:58:37.000000000 +0300
> +++ quota/fs/dquot.c	2008-03-07 01:37:46.000000000 +0300
> @@ -1703,10 +1703,19 @@ int vfs_get_dqblk(struct super_block *sb
>  }
>  
>  /* Generic routine for setting common part of quota structure */
> -static void do_set_dqblk(struct dquot *dquot, struct if_dqblk *di)
> +static int do_set_dqblk(struct dquot *dquot, struct if_dqblk *di)
>  {
>  	struct mem_dqblk *dm = &dquot->dq_dqb;
>  	int check_blim = 0, check_ilim = 0;
> +	struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_type];
> +
> +	if (((di->dqb_valid & QIF_BLIMITS) &&
> +	    ((di->dqb_bhardlimit > dqi->dqi_maxbhardlimit) ||
> +	     (di->dqb_bsoftlimit > dqi->dqi_maxbsoftlimit))) ||
> +	    ((di->dqb_valid & QIF_ILIMITS) &&
> +	    ((di->dqb_ihardlimit > dqi->dqi_maxihardlimit) ||
> +	     (di->dqb_isoftlimit > dqi->dqi_maxisoftlimit))))
> +		return -ERANGE;
>  
>  	spin_lock(&dq_data_lock);
>  	if (di->dqb_valid & QIF_SPACE) {
> @@ -1738,7 +1747,7 @@ static void do_set_dqblk(struct dquot *d
>  			clear_bit(DQ_BLKS_B, &dquot->dq_flags);
>  		}
>  		else if (!(di->dqb_valid & QIF_BTIME))	/* Set grace only if user hasn't provided his own... */
> -			dm->dqb_btime = get_seconds() + sb_dqopt(dquot->dq_sb)->info[dquot->dq_type].dqi_bgrace;
> +			dm->dqb_btime = get_seconds() + dqi->dqi_bgrace;
>  	}
>  	if (check_ilim) {
>  		if (!dm->dqb_isoftlimit || dm->dqb_curinodes < dm->dqb_isoftlimit) {
> @@ -1746,7 +1755,7 @@ static void do_set_dqblk(struct dquot *d
>  			clear_bit(DQ_INODES_B, &dquot->dq_flags);
>  		}
>  		else if (!(di->dqb_valid & QIF_ITIME))	/* Set grace only if user hasn't provided his own... */
> -			dm->dqb_itime = get_seconds() + sb_dqopt(dquot->dq_sb)->info[dquot->dq_type].dqi_igrace;
> +			dm->dqb_itime = get_seconds() + dqi->dqi_igrace;
>  	}
>  	if (dm->dqb_bhardlimit || dm->dqb_bsoftlimit || dm->dqb_ihardlimit || dm->dqb_isoftlimit)
>  		clear_bit(DQ_FAKE_B, &dquot->dq_flags);
> @@ -1754,21 +1763,24 @@ static void do_set_dqblk(struct dquot *d
>  		set_bit(DQ_FAKE_B, &dquot->dq_flags);
>  	spin_unlock(&dq_data_lock);
>  	mark_dquot_dirty(dquot);
> +
> +	return 0;
>  }
>  
>  int vfs_set_dqblk(struct super_block *sb, int type, qid_t id, struct if_dqblk *di)
>  {
>  	struct dquot *dquot;
> +	int rc;
>  
>  	mutex_lock(&sb_dqopt(sb)->dqonoff_mutex);
>  	if (!(dquot = dqget(sb, id, type))) {
>  		mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
>  		return -ESRCH;
>  	}
> -	do_set_dqblk(dquot, di);
> +	rc = do_set_dqblk(dquot, di);
>  	dqput(dquot);
>  	mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
> -	return 0;
> +	return rc;
>  }
>  
>  /* Generic routine for getting common part of quota file information */
> diff -rNpu quota.orig/fs/quota_v1.c quota/fs/quota_v1.c
> --- quota.orig/fs/quota_v1.c	2008-01-25 01:58:37.000000000 +0300
> +++ quota/fs/quota_v1.c	2008-03-07 01:39:00.000000000 +0300
> @@ -139,6 +139,11 @@ static int v1_read_file_info(struct supe
>  		goto out;
>  	}
>  	ret = 0;
> +	/* limits are stored as unsigned 32-bit data */
> +	dqopt->info[type].dqi_maxbhardlimit = 0xffffffff;
> +	dqopt->info[type].dqi_maxbsoftlimit = 0xffffffff;
> +	dqopt->info[type].dqi_maxihardlimit = 0xffffffff;
> +	dqopt->info[type].dqi_maxisoftlimit = 0xffffffff;
>  	dqopt->info[type].dqi_igrace = dqblk.dqb_itime ? dqblk.dqb_itime : MAX_IQ_TIME;
>  	dqopt->info[type].dqi_bgrace = dqblk.dqb_btime ? dqblk.dqb_btime : MAX_DQ_TIME;
>  out:
> diff -rNpu quota.orig/fs/quota_v2.c quota/fs/quota_v2.c
> --- quota.orig/fs/quota_v2.c	2008-01-25 01:58:37.000000000 +0300
> +++ quota/fs/quota_v2.c	2008-03-07 02:40:23.000000000 +0300
> @@ -59,6 +59,11 @@ static int v2_read_file_info(struct supe
>  			sb->s_id);
>  		return -1;
>  	}
> +	/* limits are stored as unsigned 32-bit data */
> +	info->dqi_maxbhardlimit = 0xffffffff;
> +	info->dqi_maxbsoftlimit = 0xffffffff;
> +	info->dqi_maxihardlimit = 0xffffffff;
> +	info->dqi_maxisoftlimit = 0xffffffff;
>  	info->dqi_bgrace = le32_to_cpu(dinfo.dqi_bgrace);
>  	info->dqi_igrace = le32_to_cpu(dinfo.dqi_igrace);
>  	info->dqi_flags = le32_to_cpu(dinfo.dqi_flags);
> @@ -108,12 +113,12 @@ static void disk2memdqb(struct mem_dqblk
>  
>  static void mem2diskdqb(struct v2_disk_dqblk *d, struct mem_dqblk *m, qid_t id)
>  {
> -	d->dqb_ihardlimit = cpu_to_le32(m->dqb_ihardlimit);
> -	d->dqb_isoftlimit = cpu_to_le32(m->dqb_isoftlimit);
> -	d->dqb_curinodes = cpu_to_le32(m->dqb_curinodes);
> +	d->dqb_ihardlimit = cpu_to_le32((u32)m->dqb_ihardlimit);
> +	d->dqb_isoftlimit = cpu_to_le32((u32)m->dqb_isoftlimit);
> +	d->dqb_curinodes = cpu_to_le32((u32)m->dqb_curinodes);
>  	d->dqb_itime = cpu_to_le64(m->dqb_itime);
> -	d->dqb_bhardlimit = cpu_to_le32(m->dqb_bhardlimit);
> -	d->dqb_bsoftlimit = cpu_to_le32(m->dqb_bsoftlimit);
> +	d->dqb_bhardlimit = cpu_to_le32((u32)m->dqb_bhardlimit);
> +	d->dqb_bsoftlimit = cpu_to_le32((u32)m->dqb_bsoftlimit);
>  	d->dqb_curspace = cpu_to_le64(m->dqb_curspace);
>  	d->dqb_btime = cpu_to_le64(m->dqb_btime);
>  	d->dqb_id = cpu_to_le32(id);
> diff -rNpu quota.orig/include/linux/quota.h quota/include/linux/quota.h
> --- quota.orig/include/linux/quota.h	2008-01-25 01:58:37.000000000 +0300
> +++ quota/include/linux/quota.h	2008-03-06 23:18:10.000000000 +0300
> @@ -181,12 +181,12 @@ extern spinlock_t dq_data_lock;
>   * Data for one user/group kept in memory
>   */
>  struct mem_dqblk {
> -	__u32 dqb_bhardlimit;	/* absolute limit on disk blks alloc */
> -	__u32 dqb_bsoftlimit;	/* preferred limit on disk blks */
> +	qsize_t dqb_bhardlimit;	/* absolute limit on disk blks alloc */
> +	qsize_t dqb_bsoftlimit;	/* preferred limit on disk blks */
>  	qsize_t dqb_curspace;	/* current used space */
> -	__u32 dqb_ihardlimit;	/* absolute limit on allocated inodes */
> -	__u32 dqb_isoftlimit;	/* preferred inode limit */
> -	__u32 dqb_curinodes;	/* current # allocated inodes */
> +	qsize_t dqb_ihardlimit;	/* absolute limit on allocated inodes */
> +	qsize_t dqb_isoftlimit;	/* preferred inode limit */
> +	qsize_t dqb_curinodes;	/* current # allocated inodes */
>  	time_t dqb_btime;	/* time limit for excessive disk use */
>  	time_t dqb_itime;	/* time limit for excessive inode use */
>  };
> @@ -202,6 +202,10 @@ struct mem_dqinfo {
>  	unsigned long dqi_flags;
>  	unsigned int dqi_bgrace;
>  	unsigned int dqi_igrace;
> +	qsize_t dqi_maxbhardlimit;
> +	qsize_t dqi_maxbsoftlimit;
> +	qsize_t dqi_maxihardlimit;
> +	qsize_t dqi_maxisoftlimit;
>  	union {
>  		struct v1_mem_dqinfo v1_i;
>  		struct v2_mem_dqinfo v2_i;
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] quota: additional range checks and mem_dqblk updates to handle 64-bit limits

[Andrew Perepechko]

mem_dqblk is extended to hold 64-bit quota limits, checks are added to deny
setting quota limits which aren't supported by a quota format module

Signed-off-by: Andrew Perepechko <andrew.perepechko@sun.com>

---

 fs/dquot.c            |   22 +++++++++++++++++-----
 fs/quota_v1.c         |    3 +++
 fs/quota_v2.c         |   13 ++++++++-----
 include/linux/quota.h |   12 +++++++-----
 4 files changed, 35 insertions(+), 15 deletions(-)

diff -rNpu quota.orig/fs/dquot.c quota/fs/dquot.c
--- quota.orig/fs/dquot.c	2008-01-25 01:58:37.000000000 +0300
+++ quota/fs/dquot.c	2008-03-07 01:37:46.000000000 +0300
@@ -1703,10 +1703,19 @@ int vfs_get_dqblk(struct super_block *sb
 }
 
 /* Generic routine for setting common part of quota structure */
-static void do_set_dqblk(struct dquot *dquot, struct if_dqblk *di)
+static int do_set_dqblk(struct dquot *dquot, struct if_dqblk *di)
 {
 	struct mem_dqblk *dm = &dquot->dq_dqb;
 	int check_blim = 0, check_ilim = 0;
+	struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_type];
+
+	if ((di->dqb_valid & QIF_BLIMITS &&
+	     (di->dqb_bhardlimit > dqi->dqi_maxblimit ||
+	      di->dqb_bsoftlimit > dqi->dqi_maxblimit)) ||
+	    (di->dqb_valid & QIF_ILIMITS &&
+	     (di->dqb_ihardlimit > dqi->dqi_maxilimit ||
+	      di->dqb_isoftlimit > dqi->dqi_maxilimit)))
+		return -ERANGE;
 
 	spin_lock(&dq_data_lock);
 	if (di->dqb_valid & QIF_SPACE) {
@@ -1738,7 +1747,7 @@ static void do_set_dqblk(struct dquot *d
 			clear_bit(DQ_BLKS_B, &dquot->dq_flags);
 		}
 		else if (!(di->dqb_valid & QIF_BTIME))	/* Set grace only if user hasn't provided his own... */
-			dm->dqb_btime = get_seconds() + sb_dqopt(dquot->dq_sb)->info[dquot->dq_type].dqi_bgrace;
+			dm->dqb_btime = get_seconds() + dqi->dqi_bgrace;
 	}
 	if (check_ilim) {
 		if (!dm->dqb_isoftlimit || dm->dqb_curinodes < dm->dqb_isoftlimit) {
@@ -1746,7 +1755,7 @@ static void do_set_dqblk(struct dquot *d
 			clear_bit(DQ_INODES_B, &dquot->dq_flags);
 		}
 		else if (!(di->dqb_valid & QIF_ITIME))	/* Set grace only if user hasn't provided his own... */
-			dm->dqb_itime = get_seconds() + sb_dqopt(dquot->dq_sb)->info[dquot->dq_type].dqi_igrace;
+			dm->dqb_itime = get_seconds() + dqi->dqi_igrace;
 	}
 	if (dm->dqb_bhardlimit || dm->dqb_bsoftlimit || dm->dqb_ihardlimit || dm->dqb_isoftlimit)
 		clear_bit(DQ_FAKE_B, &dquot->dq_flags);
@@ -1754,21 +1763,24 @@ static void do_set_dqblk(struct dquot *d
 		set_bit(DQ_FAKE_B, &dquot->dq_flags);
 	spin_unlock(&dq_data_lock);
 	mark_dquot_dirty(dquot);
+
+	return 0;
 }
 
 int vfs_set_dqblk(struct super_block *sb, int type, qid_t id, struct if_dqblk *di)
 {
 	struct dquot *dquot;
+	int rc;
 
 	mutex_lock(&sb_dqopt(sb)->dqonoff_mutex);
 	if (!(dquot = dqget(sb, id, type))) {
 		mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
 		return -ESRCH;
 	}
-	do_set_dqblk(dquot, di);
+	rc = do_set_dqblk(dquot, di);
 	dqput(dquot);
 	mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
-	return 0;
+	return rc;
 }
 
 /* Generic routine for getting common part of quota file information */
diff -rNpu quota.orig/fs/quota_v1.c quota/fs/quota_v1.c
--- quota.orig/fs/quota_v1.c	2008-01-25 01:58:37.000000000 +0300
+++ quota/fs/quota_v1.c	2008-03-07 01:39:00.000000000 +0300
@@ -139,6 +139,9 @@ static int v1_read_file_info(struct supe
 		goto out;
 	}
 	ret = 0;
+	/* limits are stored as unsigned 32-bit data */
+	dqopt->info[type].dqi_maxblimit = 0xffffffff;
+	dqopt->info[type].dqi_maxilimit = 0xffffffff;
 	dqopt->info[type].dqi_igrace = dqblk.dqb_itime ? dqblk.dqb_itime : MAX_IQ_TIME;
 	dqopt->info[type].dqi_bgrace = dqblk.dqb_btime ? dqblk.dqb_btime : MAX_DQ_TIME;
 out:
diff -rNpu quota.orig/fs/quota_v2.c quota/fs/quota_v2.c
--- quota.orig/fs/quota_v2.c	2008-01-25 01:58:37.000000000 +0300
+++ quota/fs/quota_v2.c	2008-03-07 02:40:23.000000000 +0300
@@ -59,6 +59,9 @@ static int v2_read_file_info(struct supe
 			sb->s_id);
 		return -1;
 	}
+	/* limits are stored as unsigned 32-bit data */
+	info->dqi_maxblimit = 0xffffffff;
+	info->dqi_maxilimit = 0xffffffff;
 	info->dqi_bgrace = le32_to_cpu(dinfo.dqi_bgrace);
 	info->dqi_igrace = le32_to_cpu(dinfo.dqi_igrace);
 	info->dqi_flags = le32_to_cpu(dinfo.dqi_flags);
@@ -108,12 +113,12 @@ static void disk2memdqb(struct mem_dqblk
 
 static void mem2diskdqb(struct v2_disk_dqblk *d, struct mem_dqblk *m, qid_t id)
 {
-	d->dqb_ihardlimit = cpu_to_le32(m->dqb_ihardlimit);
-	d->dqb_isoftlimit = cpu_to_le32(m->dqb_isoftlimit);
-	d->dqb_curinodes = cpu_to_le32(m->dqb_curinodes);
+	d->dqb_ihardlimit = cpu_to_le32((u32)m->dqb_ihardlimit);
+	d->dqb_isoftlimit = cpu_to_le32((u32)m->dqb_isoftlimit);
+	d->dqb_curinodes = cpu_to_le32((u32)m->dqb_curinodes);
 	d->dqb_itime = cpu_to_le64(m->dqb_itime);
-	d->dqb_bhardlimit = cpu_to_le32(m->dqb_bhardlimit);
-	d->dqb_bsoftlimit = cpu_to_le32(m->dqb_bsoftlimit);
+	d->dqb_bhardlimit = cpu_to_le32((u32)m->dqb_bhardlimit);
+	d->dqb_bsoftlimit = cpu_to_le32((u32)m->dqb_bsoftlimit);
 	d->dqb_curspace = cpu_to_le64(m->dqb_curspace);
 	d->dqb_btime = cpu_to_le64(m->dqb_btime);
 	d->dqb_id = cpu_to_le32(id);
diff -rNpu quota.orig/include/linux/quota.h quota/include/linux/quota.h
--- quota.orig/include/linux/quota.h	2008-01-25 01:58:37.000000000 +0300
+++ quota/include/linux/quota.h	2008-03-06 23:18:10.000000000 +0300
@@ -181,12 +181,12 @@ extern spinlock_t dq_data_lock;
  * Data for one user/group kept in memory
  */
 struct mem_dqblk {
-	__u32 dqb_bhardlimit;	/* absolute limit on disk blks alloc */
-	__u32 dqb_bsoftlimit;	/* preferred limit on disk blks */
+	qsize_t dqb_bhardlimit;	/* absolute limit on disk blks alloc */
+	qsize_t dqb_bsoftlimit;	/* preferred limit on disk blks */
 	qsize_t dqb_curspace;	/* current used space */
-	__u32 dqb_ihardlimit;	/* absolute limit on allocated inodes */
-	__u32 dqb_isoftlimit;	/* preferred inode limit */
-	__u32 dqb_curinodes;	/* current # allocated inodes */
+	qsize_t dqb_ihardlimit;	/* absolute limit on allocated inodes */
+	qsize_t dqb_isoftlimit;	/* preferred inode limit */
+	qsize_t dqb_curinodes;	/* current # allocated inodes */
 	time_t dqb_btime;	/* time limit for excessive disk use */
 	time_t dqb_itime;	/* time limit for excessive inode use */
 };
@@ -202,6 +202,8 @@ struct mem_dqinfo {
 	unsigned long dqi_flags;
 	unsigned int dqi_bgrace;
 	unsigned int dqi_igrace;
+	qsize_t dqi_maxblimit;
+	qsize_t dqi_maxilimit;
 	union {
 		struct v1_mem_dqinfo v1_i;
 		struct v2_mem_dqinfo v2_i;

Re: [PATCH] quota: additional range checks and mem_dqblk updates?to handle 64-bit limits

[Andrew Perepechko]

On Monday 10 March 2008 18:20:59 Jan Kara wrote:

>   Wow. I wonder when 64-bits won't be enough :). 

Let's hope for the better :)

>   Anyway, my point was that 
> when you get to 4 PB limits, you can just run: "setquota --set-scale 1GB"
> (hopefully at that time you won't care about rounding limits to 1GB) and
> you are at 4 HB limits (or what is the right suffix). No quota format
> change needed.

Indeed, this would work. And it is a fast.. and an elegant solution.

My point is that having 64-bit quota limit there's no need to implement 
new (and unusual) scaling interface for a user. Independent of filesystem 
size (whether it is 100 Gb or 100 Tb) user would be able to set quota limits 
as he used to, in kilobyte blocks. There would be no need for quota tools to
obtainthe  scaling factor before applying quota limits, no need to do the scaling
"behind the curtains", in kernel. :)

>   But what I fear more is that we may run out of 2^32 limit on the number
> of files one user can have (I don't have experience with that large systems
> but I guess you are comming near to 2^32 files on the filesystem, aren't
> you?). And for that we would have to do basically the changes you've
> suggested.
> 

As far as I know, we haven't yet hit this limit (though I can be wrong with this).
But, yes, you are right, it is very likely to happen soon anyway. :)

> > Do you feel it might take considerably larger efforts to update quotatools
> > to 64-bit version compared to 32-bit version with variable quota unit size?
>   Well, I don't fear that much about quota tools but more about the kernel
> code and duplication of code in there.
> 
> > I'd like to start working on a kernel patch implementing 64-bit limits provided 
> > you approve the approach.
>   I'm glad you're eager to work on that :) I'd just like to think twice
> before going for the new format and changing quota code.
> 

Too late. :)

>   Hmm, I don't get this. I don't think there's a real need for changing the
> quota file name. That is exactly what I'd like to avoid. Why do you think
> new quota file name is better? Those conversion things Andreas pointed out,
> are solved by doing the conversion into the temporary file...

I was thinking that having separate file names could help, along with the magic
field, to recognize the file format. As for conversion, I was thinking about such
a sequence: create new file, write bad header, sync, convert, sync, write good
header, sync, switch to the new file, finally erase the old one.
With journalling, of course, adding rename seems to be atomic. Without
journalling (ext2) could both files get lost in crash during conversion rename?

Probably, it's not an issue here since those who take care of crashes use journalled
filesystems. Then keeping the same name is ok...


Thank you.
Andrew.
> 
> 									Honza

Re: [PATCH] quota: additional range checks and mem_dqblk updates?to handle 64-bit limits

[Zhiyong Landen tian]

Jan Kara wrote:
> On Sat 08-03-08 03:56:17, Andrew Perepechko wrote:
>   
>> Jan, this idea would work for us for now. As Andreas pointed out, we deal 
>> with MB-aligned quota limits, so this would give us the largest possible block 
>> quota limit value of 4 PB.
>>
>> However, I feel that it is worth to implement a clean 64-bit format, so that
>> we avoid additional semantics (like quota unit size) and do make quota scale 
>> better (4 PB clusters already exist).
>>     
>   Wow. I wonder when 64-bits won't be enough :). Anyway, my point was that
> when you get to 4 PB limits, you can just run: "setquota --set-scale 1GB"
> (hopefully at that time you won't care about rounding limits to 1GB) and
> you are at 4 HB limits (or what is the right suffix). No quota format
> change needed.
>   But what I fear more is that we may run out of 2^32 limit on the number
> of files one user can have (I don't have experience with that large systems
> but I guess you are comming near to 2^32 files on the filesystem, aren't
> you?). And for that we would have to do basically the changes you've
> suggested
The core of the "scale" way is: it does harm to the accuracy of quota so 
that we can set a larger
quota limitation. If the scale is 1G, 500M quota the user sets is equal 
to 0. Some customers may
like it; others may confuse. Anyway, we can't regulate the way of users 
using quota.

For lustre, I have a plan to set a minimum unit to 1K instead of current 
1M. So 64bit quota is
a must. I just wonder why not we implement these two points in different 
patches:
1. 64bit quota limitation
2. give a "scale" to the users who are glad to adjust it.

Re: [PATCH] quota: additional range checks and mem_dqblk updates?to handle 64-bit limits

[Jan Kara]

  Hello Andrew,

  below is the patch I've ended with. I've removed the modification of
mem_dqblk, so that the patch is a bugfix only (structure if_dqblk is able
to keep 64-bit values already). I'll push this patch to Andrew, for
inclusion.
  Also after some thinking I like the idea of 64-bit quota format so please
send me the final patch based on the patch below when you have it ready and
I'll include it as well.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

----

From: Andrew Perepechko <andrew.perepechko@sun.com>

We should check whether quota limits set via Q_SETQUOTA are not exceeding
limits which quota format is able to handle.

Signed-off-by: Andrew Perepechko <andrew.perepechko@sun.com>
Signed-off-by: Jan Kara <jack@suse.cz>

---
 fs/dquot.c            |   22 +++++++++++++++++-----
 fs/quota_v1.c         |    3 +++
 fs/quota_v2.c         |    3 +++
 include/linux/quota.h |    2 ++
 4 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/fs/dquot.c b/fs/dquot.c
index 9c7feb6..f816d06 100644
--- a/fs/dquot.c
+++ b/fs/dquot.c
@@ -1709,10 +1709,19 @@ int vfs_get_dqblk(struct super_block *sb, int type, qid_t id, struct if_dqblk *d
 }
 
 /* Generic routine for setting common part of quota structure */
-static void do_set_dqblk(struct dquot *dquot, struct if_dqblk *di)
+static int do_set_dqblk(struct dquot *dquot, struct if_dqblk *di)
 {
 	struct mem_dqblk *dm = &dquot->dq_dqb;
 	int check_blim = 0, check_ilim = 0;
+	struct mem_dqinfo *dqi = &sb_dqopt(dquot->dq_sb)->info[dquot->dq_type];
+
+	if ((di->dqb_valid & QIF_BLIMITS &&
+	     (di->dqb_bhardlimit > dqi->dqi_maxblimit ||
+	      di->dqb_bsoftlimit > dqi->dqi_maxblimit)) ||
+	    (di->dqb_valid & QIF_ILIMITS &&
+	     (di->dqb_ihardlimit > dqi->dqi_maxilimit ||
+	      di->dqb_isoftlimit > dqi->dqi_maxilimit)))
+		return -ERANGE;
 
 	spin_lock(&dq_data_lock);
 	if (di->dqb_valid & QIF_SPACE) {
@@ -1744,7 +1753,7 @@ static void do_set_dqblk(struct dquot *dquot, struct if_dqblk *di)
 			clear_bit(DQ_BLKS_B, &dquot->dq_flags);
 		}
 		else if (!(di->dqb_valid & QIF_BTIME))	/* Set grace only if user hasn't provided his own... */
-			dm->dqb_btime = get_seconds() + sb_dqopt(dquot->dq_sb)->info[dquot->dq_type].dqi_bgrace;
+			dm->dqb_btime = get_seconds() + dqi->dqi_bgrace;
 	}
 	if (check_ilim) {
 		if (!dm->dqb_isoftlimit || dm->dqb_curinodes < dm->dqb_isoftlimit) {
@@ -1752,7 +1761,7 @@ static void do_set_dqblk(struct dquot *dquot, struct if_dqblk *di)
 			clear_bit(DQ_INODES_B, &dquot->dq_flags);
 		}
 		else if (!(di->dqb_valid & QIF_ITIME))	/* Set grace only if user hasn't provided his own... */
-			dm->dqb_itime = get_seconds() + sb_dqopt(dquot->dq_sb)->info[dquot->dq_type].dqi_igrace;
+			dm->dqb_itime = get_seconds() + dqi->dqi_igrace;
 	}
 	if (dm->dqb_bhardlimit || dm->dqb_bsoftlimit || dm->dqb_ihardlimit || dm->dqb_isoftlimit)
 		clear_bit(DQ_FAKE_B, &dquot->dq_flags);
@@ -1760,21 +1769,24 @@ static void do_set_dqblk(struct dquot *dquot, struct if_dqblk *di)
 		set_bit(DQ_FAKE_B, &dquot->dq_flags);
 	spin_unlock(&dq_data_lock);
 	mark_dquot_dirty(dquot);
+
+	return 0;
 }
 
 int vfs_set_dqblk(struct super_block *sb, int type, qid_t id, struct if_dqblk *di)
 {
 	struct dquot *dquot;
+	int rc;
 
 	mutex_lock(&sb_dqopt(sb)->dqonoff_mutex);
 	if (!(dquot = dqget(sb, id, type))) {
 		mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
 		return -ESRCH;
 	}
-	do_set_dqblk(dquot, di);
+	rc = do_set_dqblk(dquot, di);
 	dqput(dquot);
 	mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
-	return 0;
+	return rc;
 }
 
 /* Generic routine for getting common part of quota file information */
diff --git a/fs/quota_v1.c b/fs/quota_v1.c
index f3841f2..a6cf926 100644
--- a/fs/quota_v1.c
+++ b/fs/quota_v1.c
@@ -139,6 +139,9 @@ static int v1_read_file_info(struct super_block *sb, int type)
 		goto out;
 	}
 	ret = 0;
+	/* limits are stored as unsigned 32-bit data */
+	dqopt->info[type].dqi_maxblimit = 0xffffffff;
+	dqopt->info[type].dqi_maxilimit = 0xffffffff;
 	dqopt->info[type].dqi_igrace = dqblk.dqb_itime ? dqblk.dqb_itime : MAX_IQ_TIME;
 	dqopt->info[type].dqi_bgrace = dqblk.dqb_btime ? dqblk.dqb_btime : MAX_DQ_TIME;
 out:
diff --git a/fs/quota_v2.c b/fs/quota_v2.c
index c519a58..23b647f 100644
--- a/fs/quota_v2.c
+++ b/fs/quota_v2.c
@@ -59,6 +59,9 @@ static int v2_read_file_info(struct super_block *sb, int type)
 			sb->s_id);
 		return -1;
 	}
+	/* limits are stored as unsigned 32-bit data */
+	info->dqi_maxblimit = 0xffffffff;
+	info->dqi_maxilimit = 0xffffffff;
 	info->dqi_bgrace = le32_to_cpu(dinfo.dqi_bgrace);
 	info->dqi_igrace = le32_to_cpu(dinfo.dqi_igrace);
 	info->dqi_flags = le32_to_cpu(dinfo.dqi_flags);
diff --git a/include/linux/quota.h b/include/linux/quota.h
index 6e0393a..c1cdbb7 100644
--- a/include/linux/quota.h
+++ b/include/linux/quota.h
@@ -202,6 +202,8 @@ struct mem_dqinfo {
 	unsigned long dqi_flags;
 	unsigned int dqi_bgrace;
 	unsigned int dqi_igrace;
+	qsize_t dqi_maxblimit;
+	qsize_t dqi_maxilimit;
 	union {
 		struct v1_mem_dqinfo v1_i;
 		struct v2_mem_dqinfo v2_i;

Re: [PATCH] quota: additional range checks and mem_dqblk updates?to handle 64-bit limits

[Andrew Perepechko]

Hello, Jan!

Thank you very much, first of all.
I'm going to update the patch with respect to your considerations ASAP.

Andrew.


On Wednesday 12 March 2008 20:21:19 Jan Kara wrote:
>   Hello Andrew,
> 
>   below is the patch I've ended with. I've removed the modification of
> mem_dqblk, so that the patch is a bugfix only (structure if_dqblk is able
> to keep 64-bit values already). I'll push this patch to Andrew, for
> inclusion.
>   Also after some thinking I like the idea of 64-bit quota format so please
> send me the final patch based on the patch below when you have it ready and
> I'll include it as well.
> 
> 								Honza