[PATCH v3] quota: widen timestamps for the fs_disk_quota structure

[PATCH v3] quota: widen timestamps for the fs_disk_quota structure

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Soon, XFS will support quota grace period expiration timestamps beyond
the year 2038, widen the timestamp fields to handle the extra time bits.
Internally, XFS now stores unsigned 34-bit quantities, so the extra 8
bits here should work fine.  (Note that XFS is the only user of this
structure.)

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v3: remove the old rt_spc_timer assignment statement
v2: fix calling conventions, widen timestamps
---
 fs/quota/quota.c               |   44 +++++++++++++++++++++++++++++++++++-----
 include/uapi/linux/dqblk_xfs.h |   11 +++++++++-
 2 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/fs/quota/quota.c b/fs/quota/quota.c
index 47f9e151988b..77abd1d9f02f 100644
--- a/fs/quota/quota.c
+++ b/fs/quota/quota.c
@@ -481,6 +481,14 @@ static inline u64 quota_btobb(u64 bytes)
 	return (bytes + (1 << XFS_BB_SHIFT) - 1) >> XFS_BB_SHIFT;
 }
 
+static inline s64 copy_from_xfs_dqblk_ts(const struct fs_disk_quota *d,
+		__s32 timer, __s8 timer_hi)
+{
+	if (d->d_fieldmask & FS_DQ_BIGTIME)
+		return (u32)timer | (s64)timer_hi << 32;
+	return timer;
+}
+
 static void copy_from_xfs_dqblk(struct qc_dqblk *dst, struct fs_disk_quota *src)
 {
 	dst->d_spc_hardlimit = quota_bbtob(src->d_blk_hardlimit);
@@ -489,14 +497,17 @@ static void copy_from_xfs_dqblk(struct qc_dqblk *dst, struct fs_disk_quota *src)
 	dst->d_ino_softlimit = src->d_ino_softlimit;
 	dst->d_space = quota_bbtob(src->d_bcount);
 	dst->d_ino_count = src->d_icount;
-	dst->d_ino_timer = src->d_itimer;
-	dst->d_spc_timer = src->d_btimer;
+	dst->d_ino_timer = copy_from_xfs_dqblk_ts(src, src->d_itimer,
+						  src->d_itimer_hi);
+	dst->d_spc_timer = copy_from_xfs_dqblk_ts(src, src->d_btimer,
+						  src->d_btimer_hi);
 	dst->d_ino_warns = src->d_iwarns;
 	dst->d_spc_warns = src->d_bwarns;
 	dst->d_rt_spc_hardlimit = quota_bbtob(src->d_rtb_hardlimit);
 	dst->d_rt_spc_softlimit = quota_bbtob(src->d_rtb_softlimit);
 	dst->d_rt_space = quota_bbtob(src->d_rtbcount);
-	dst->d_rt_spc_timer = src->d_rtbtimer;
+	dst->d_rt_spc_timer = copy_from_xfs_dqblk_ts(src, src->d_rtbtimer,
+						     src->d_rtbtimer_hi);
 	dst->d_rt_spc_warns = src->d_rtbwarns;
 	dst->d_fieldmask = 0;
 	if (src->d_fieldmask & FS_DQ_ISOFT)
@@ -588,10 +599,28 @@ static int quota_setxquota(struct super_block *sb, int type, qid_t id,
 	return sb->s_qcop->set_dqblk(sb, qid, &qdq);
 }
 
+static inline void copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
+		__s32 *timer_lo, __s8 *timer_hi, s64 timer)
+{
+	*timer_lo = timer;
+	if (d->d_fieldmask & FS_DQ_BIGTIME)
+		*timer_hi = timer >> 32;
+	else
+		*timer_hi = 0;
+}
+
+static inline bool want_bigtime(s64 timer)
+{
+	return timer > S32_MAX || timer < S32_MIN;
+}
+
 static void copy_to_xfs_dqblk(struct fs_disk_quota *dst, struct qc_dqblk *src,
 			      int type, qid_t id)
 {
 	memset(dst, 0, sizeof(*dst));
+	if (want_bigtime(src->d_ino_timer) || want_bigtime(src->d_spc_timer) ||
+	    want_bigtime(src->d_rt_spc_timer))
+		dst->d_fieldmask |= FS_DQ_BIGTIME;
 	dst->d_version = FS_DQUOT_VERSION;
 	dst->d_id = id;
 	if (type == USRQUOTA)
@@ -606,14 +635,17 @@ static void copy_to_xfs_dqblk(struct fs_disk_quota *dst, struct qc_dqblk *src,
 	dst->d_ino_softlimit = src->d_ino_softlimit;
 	dst->d_bcount = quota_btobb(src->d_space);
 	dst->d_icount = src->d_ino_count;
-	dst->d_itimer = src->d_ino_timer;
-	dst->d_btimer = src->d_spc_timer;
+	copy_to_xfs_dqblk_ts(dst, &dst->d_itimer, &dst->d_itimer_hi,
+			     src->d_ino_timer);
+	copy_to_xfs_dqblk_ts(dst, &dst->d_btimer, &dst->d_btimer_hi,
+			     src->d_spc_timer);
 	dst->d_iwarns = src->d_ino_warns;
 	dst->d_bwarns = src->d_spc_warns;
 	dst->d_rtb_hardlimit = quota_btobb(src->d_rt_spc_hardlimit);
 	dst->d_rtb_softlimit = quota_btobb(src->d_rt_spc_softlimit);
 	dst->d_rtbcount = quota_btobb(src->d_rt_space);
-	dst->d_rtbtimer = src->d_rt_spc_timer;
+	copy_to_xfs_dqblk_ts(dst, &dst->d_rtbtimer, &dst->d_rtbtimer_hi,
+			     src->d_rt_spc_timer);
 	dst->d_rtbwarns = src->d_rt_spc_warns;
 }
 
diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h
index 03d890b80ebc..16d73f54376d 100644
--- a/include/uapi/linux/dqblk_xfs.h
+++ b/include/uapi/linux/dqblk_xfs.h
@@ -66,7 +66,10 @@ typedef struct fs_disk_quota {
 	__s32		d_btimer;	/* similar to above; for disk blocks */
 	__u16	  	d_iwarns;       /* # warnings issued wrt num inodes */
 	__u16	  	d_bwarns;       /* # warnings issued wrt disk blocks */
-	__s32		d_padding2;	/* padding2 - for future use */
+	__s8		d_itimer_hi;	/* upper 8 bits of timer values */
+	__s8		d_btimer_hi;
+	__s8		d_rtbtimer_hi;
+	__s8		d_padding2;	/* padding2 - for future use */
 	__u64		d_rtb_hardlimit;/* absolute limit on realtime blks */
 	__u64		d_rtb_softlimit;/* preferred limit on RT disk blks */
 	__u64		d_rtbcount;	/* # realtime blocks owned */
@@ -121,6 +124,12 @@ typedef struct fs_disk_quota {
 #define FS_DQ_RTBCOUNT		(1<<14)
 #define FS_DQ_ACCT_MASK		(FS_DQ_BCOUNT | FS_DQ_ICOUNT | FS_DQ_RTBCOUNT)
 
+/*
+ * Quota expiration timestamps are 40-bit signed integers, with the upper 8
+ * bits encoded in the _hi fields.
+ */
+#define FS_DQ_BIGTIME		(1<<15)
+
 /*
  * Various flags related to quotactl(2).
  */

Re: [PATCH v3] quota: widen timestamps for the fs_disk_quota structure

[Matthew Wilcox]

On Tue, Sep 08, 2020 at 06:32:51PM -0700, Darrick J. Wong wrote:
> +static inline void copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
> +		__s32 *timer_lo, __s8 *timer_hi, s64 timer)
> +{
> +	*timer_lo = timer;
> +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> +		*timer_hi = timer >> 32;
> +	else
> +		*timer_hi = 0;
> +}

I'm still confused by this.  What breaks if you just do:

	*timer_lo = timer;
	*timer_hi = timer >> 32;

>  	memset(dst, 0, sizeof(*dst));
> +	if (want_bigtime(src->d_ino_timer) || want_bigtime(src->d_spc_timer) ||
> +	    want_bigtime(src->d_rt_spc_timer))
> +		dst->d_fieldmask |= FS_DQ_BIGTIME;

You still need this (I guess?  In case somebody's written to the
d_padding2 field?)

Re: [PATCH v3] quota: widen timestamps for the fs_disk_quota structure

[Darrick J. Wong]

On Wed, Sep 09, 2020 at 02:49:33AM +0100, Matthew Wilcox wrote:
> On Tue, Sep 08, 2020 at 06:32:51PM -0700, Darrick J. Wong wrote:
> > +static inline void copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
> > +		__s32 *timer_lo, __s8 *timer_hi, s64 timer)
> > +{
> > +	*timer_lo = timer;
> > +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> > +		*timer_hi = timer >> 32;
> > +	else
> > +		*timer_hi = 0;
> > +}
> 
> I'm still confused by this.  What breaks if you just do:
> 
> 	*timer_lo = timer;
> 	*timer_hi = timer >> 32;

"I don't know."

The manpage for quotactl doesn't actually specify the behavior of the
padding fields.  The /implementation/ is careful enough to zero
everything, but the interface specification doesn't explicitly require
software to do so.

Because the contents of the padding fields aren't defined by the
documentation, the kernel cannot simply start using the d_padding2 field
because there could be an old kernel that doesn't zero the padding,
which would lead to confusion if the new userspace were mated to such a
kernel.

Therefore, we have to add a flag that states explicitly that we are
using the timer_hi fields.  This is also the only way that an old
program can detect that it's being fed a structure that it might not
recognise.

Keep in mind that if @timer is negative (i.e. before the unix epoch)
then technically timer_hi has to be all ones if FS_DQ_BIGTIME is set.
The only user doesn't support that, but that's no excuse for sloppiness.

> >  	memset(dst, 0, sizeof(*dst));
> > +	if (want_bigtime(src->d_ino_timer) || want_bigtime(src->d_spc_timer) ||
> > +	    want_bigtime(src->d_rt_spc_timer))
> > +		dst->d_fieldmask |= FS_DQ_BIGTIME;
> 
> You still need this (I guess?  In case somebody's written to the
> d_padding2 field?)

Yes.

--D

Re: [PATCH v3] quota: widen timestamps for the fs_disk_quota structure

[Jan Kara]

On Tue 08-09-20 19:29:09, Darrick J. Wong wrote:
> On Wed, Sep 09, 2020 at 02:49:33AM +0100, Matthew Wilcox wrote:
> > On Tue, Sep 08, 2020 at 06:32:51PM -0700, Darrick J. Wong wrote:
> > > +static inline void copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
> > > +		__s32 *timer_lo, __s8 *timer_hi, s64 timer)
> > > +{
> > > +	*timer_lo = timer;
> > > +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> > > +		*timer_hi = timer >> 32;
> > > +	else
> > > +		*timer_hi = 0;
> > > +}
> > 
> > I'm still confused by this.  What breaks if you just do:
> > 
> > 	*timer_lo = timer;
> > 	*timer_hi = timer >> 32;
> 
> "I don't know."
> 
> The manpage for quotactl doesn't actually specify the behavior of the
> padding fields.  The /implementation/ is careful enough to zero
> everything, but the interface specification doesn't explicitly require
> software to do so.
> 
> Because the contents of the padding fields aren't defined by the
> documentation, the kernel cannot simply start using the d_padding2 field
> because there could be an old kernel that doesn't zero the padding,
> which would lead to confusion if the new userspace were mated to such a
> kernel.
> 
> Therefore, we have to add a flag that states explicitly that we are
> using the timer_hi fields.  This is also the only way that an old
> program can detect that it's being fed a structure that it might not
> recognise.

Well, this is in the direction from kernel to userspace and what Matthew
suggests would just make kernel posssibly store non-zero value in *timer_hi
without setting FS_DQ_BIGTIME flag (for negative values of timer). I don't
think it would break anything but I agree the complication isn't big so
let's be careful and only set *timer_hi to non-zero if FS_DQ_BIGTIME is
set.

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

Re: [PATCH v3] quota: widen timestamps for the fs_disk_quota structure

[Matthew Wilcox]

On Wed, Sep 09, 2020 at 12:51:33PM +0200, Jan Kara wrote:
> On Tue 08-09-20 19:29:09, Darrick J. Wong wrote:
> > On Wed, Sep 09, 2020 at 02:49:33AM +0100, Matthew Wilcox wrote:
> > > On Tue, Sep 08, 2020 at 06:32:51PM -0700, Darrick J. Wong wrote:
> > > > +static inline void copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
> > > > +		__s32 *timer_lo, __s8 *timer_hi, s64 timer)
> > > > +{
> > > > +	*timer_lo = timer;
> > > > +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> > > > +		*timer_hi = timer >> 32;
> > > > +	else
> > > > +		*timer_hi = 0;
> > > > +}
> > > 
> > > I'm still confused by this.  What breaks if you just do:
> > > 
> > > 	*timer_lo = timer;
> > > 	*timer_hi = timer >> 32;
> > 
> > "I don't know."
> > 
> > The manpage for quotactl doesn't actually specify the behavior of the
> > padding fields.  The /implementation/ is careful enough to zero
> > everything, but the interface specification doesn't explicitly require
> > software to do so.
> > 
> > Because the contents of the padding fields aren't defined by the
> > documentation, the kernel cannot simply start using the d_padding2 field
> > because there could be an old kernel that doesn't zero the padding,
> > which would lead to confusion if the new userspace were mated to such a
> > kernel.
> > 
> > Therefore, we have to add a flag that states explicitly that we are
> > using the timer_hi fields.  This is also the only way that an old
> > program can detect that it's being fed a structure that it might not
> > recognise.
> 
> Well, this is in the direction from kernel to userspace and what Matthew
> suggests would just make kernel posssibly store non-zero value in *timer_hi
> without setting FS_DQ_BIGTIME flag (for negative values of timer). I don't
> think it would break anything but I agree the complication isn't big so
> let's be careful and only set *timer_hi to non-zero if FS_DQ_BIGTIME is
> set.

OK, thanks.  I must admit, I'd completely forgotten about the negative
values ... and the manpage (quotactl(2)) could be clearer:

                      int32_t  d_itimer;    /* Zero if within inode limits */
                                            /* If not, we refuse service */
                      int32_t  d_btimer;    /* Similar to above; for
                                               disk blocks */

I can't tell if this is a relative time or seconds since 1970 since we
exceeded the quota.

Re: [PATCH v3] quota: widen timestamps for the fs_disk_quota structure

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 2853 bytes --]

On Wed 09-09-20 13:42:52, Matthew Wilcox wrote:
> On Wed, Sep 09, 2020 at 12:51:33PM +0200, Jan Kara wrote:
> > On Tue 08-09-20 19:29:09, Darrick J. Wong wrote:
> > > On Wed, Sep 09, 2020 at 02:49:33AM +0100, Matthew Wilcox wrote:
> > > > On Tue, Sep 08, 2020 at 06:32:51PM -0700, Darrick J. Wong wrote:
> > > > > +static inline void copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
> > > > > +		__s32 *timer_lo, __s8 *timer_hi, s64 timer)
> > > > > +{
> > > > > +	*timer_lo = timer;
> > > > > +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> > > > > +		*timer_hi = timer >> 32;
> > > > > +	else
> > > > > +		*timer_hi = 0;
> > > > > +}
> > > > 
> > > > I'm still confused by this.  What breaks if you just do:
> > > > 
> > > > 	*timer_lo = timer;
> > > > 	*timer_hi = timer >> 32;
> > > 
> > > "I don't know."
> > > 
> > > The manpage for quotactl doesn't actually specify the behavior of the
> > > padding fields.  The /implementation/ is careful enough to zero
> > > everything, but the interface specification doesn't explicitly require
> > > software to do so.
> > > 
> > > Because the contents of the padding fields aren't defined by the
> > > documentation, the kernel cannot simply start using the d_padding2 field
> > > because there could be an old kernel that doesn't zero the padding,
> > > which would lead to confusion if the new userspace were mated to such a
> > > kernel.
> > > 
> > > Therefore, we have to add a flag that states explicitly that we are
> > > using the timer_hi fields.  This is also the only way that an old
> > > program can detect that it's being fed a structure that it might not
> > > recognise.
> > 
> > Well, this is in the direction from kernel to userspace and what Matthew
> > suggests would just make kernel posssibly store non-zero value in *timer_hi
> > without setting FS_DQ_BIGTIME flag (for negative values of timer). I don't
> > think it would break anything but I agree the complication isn't big so
> > let's be careful and only set *timer_hi to non-zero if FS_DQ_BIGTIME is
> > set.
> 
> OK, thanks.  I must admit, I'd completely forgotten about the negative
> values ... and the manpage (quotactl(2)) could be clearer:
> 
>                       int32_t  d_itimer;    /* Zero if within inode limits */
>                                             /* If not, we refuse service */
>                       int32_t  d_btimer;    /* Similar to above; for
>                                                disk blocks */
> 
> I can't tell if this is a relative time or seconds since 1970 since we
> exceeded the quota.

In fact, it is time (in seconds since epoch) when softlimit becomes
enforced (i.e. when you cannot write any more blocks/inodes if you are
still over softlimit). I agree the comment incomplete at best. Something
like attached patch?

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

[-- Attachment #2: 0001-quota-Expand-comment-describing-d_itimer.patch --]
[-- Type: text/x-patch, Size: 1170 bytes --]

From 3e3260a337ff444e3a1396834b20da63d7b87ccb Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Wed, 9 Sep 2020 15:54:46 +0200
Subject: [PATCH] quota: Expand comment describing d_itimer

Expand comment describing d_itimer in struct fs_disk_quota.

Reported-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 include/uapi/linux/dqblk_xfs.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h
index 16d73f54376d..e4b3fd7f0a50 100644
--- a/include/uapi/linux/dqblk_xfs.h
+++ b/include/uapi/linux/dqblk_xfs.h
@@ -62,7 +62,8 @@ typedef struct fs_disk_quota {
 	__u64		d_bcount;	/* # disk blocks owned by the user */
 	__u64		d_icount;	/* # inodes owned by the user */
 	__s32		d_itimer;	/* zero if within inode limits */
-					/* if not, we refuse service */
+					/* if not, we refuse service at this
+					 * time (in seconds since epoch) */
 	__s32		d_btimer;	/* similar to above; for disk blocks */
 	__u16	  	d_iwarns;       /* # warnings issued wrt num inodes */
 	__u16	  	d_bwarns;       /* # warnings issued wrt disk blocks */
-- 
2.16.4

Re: [PATCH v3] quota: widen timestamps for the fs_disk_quota structure

[Darrick J. Wong]

On Wed, Sep 09, 2020 at 03:56:45PM +0200, Jan Kara wrote:
> On Wed 09-09-20 13:42:52, Matthew Wilcox wrote:
> > On Wed, Sep 09, 2020 at 12:51:33PM +0200, Jan Kara wrote:
> > > On Tue 08-09-20 19:29:09, Darrick J. Wong wrote:
> > > > On Wed, Sep 09, 2020 at 02:49:33AM +0100, Matthew Wilcox wrote:
> > > > > On Tue, Sep 08, 2020 at 06:32:51PM -0700, Darrick J. Wong wrote:
> > > > > > +static inline void copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
> > > > > > +		__s32 *timer_lo, __s8 *timer_hi, s64 timer)
> > > > > > +{
> > > > > > +	*timer_lo = timer;
> > > > > > +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> > > > > > +		*timer_hi = timer >> 32;
> > > > > > +	else
> > > > > > +		*timer_hi = 0;
> > > > > > +}
> > > > > 
> > > > > I'm still confused by this.  What breaks if you just do:
> > > > > 
> > > > > 	*timer_lo = timer;
> > > > > 	*timer_hi = timer >> 32;
> > > > 
> > > > "I don't know."
> > > > 
> > > > The manpage for quotactl doesn't actually specify the behavior of the
> > > > padding fields.  The /implementation/ is careful enough to zero
> > > > everything, but the interface specification doesn't explicitly require
> > > > software to do so.
> > > > 
> > > > Because the contents of the padding fields aren't defined by the
> > > > documentation, the kernel cannot simply start using the d_padding2 field
> > > > because there could be an old kernel that doesn't zero the padding,
> > > > which would lead to confusion if the new userspace were mated to such a
> > > > kernel.
> > > > 
> > > > Therefore, we have to add a flag that states explicitly that we are
> > > > using the timer_hi fields.  This is also the only way that an old
> > > > program can detect that it's being fed a structure that it might not
> > > > recognise.
> > > 
> > > Well, this is in the direction from kernel to userspace and what Matthew
> > > suggests would just make kernel posssibly store non-zero value in *timer_hi
> > > without setting FS_DQ_BIGTIME flag (for negative values of timer). I don't
> > > think it would break anything but I agree the complication isn't big so
> > > let's be careful and only set *timer_hi to non-zero if FS_DQ_BIGTIME is
> > > set.
> > 
> > OK, thanks.  I must admit, I'd completely forgotten about the negative
> > values ... and the manpage (quotactl(2)) could be clearer:
> > 
> >                       int32_t  d_itimer;    /* Zero if within inode limits */
> >                                             /* If not, we refuse service */
> >                       int32_t  d_btimer;    /* Similar to above; for
> >                                                disk blocks */
> > 
> > I can't tell if this is a relative time or seconds since 1970 since we
> > exceeded the quota.
> 
> In fact, it is time (in seconds since epoch) when softlimit becomes
> enforced (i.e. when you cannot write any more blocks/inodes if you are
> still over softlimit). I agree the comment incomplete at best. Something
> like attached patch?
> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

> From 3e3260a337ff444e3a1396834b20da63d7b87ccb Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@suse.cz>
> Date: Wed, 9 Sep 2020 15:54:46 +0200
> Subject: [PATCH] quota: Expand comment describing d_itimer
> 
> Expand comment describing d_itimer in struct fs_disk_quota.
> 
> Reported-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  include/uapi/linux/dqblk_xfs.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h
> index 16d73f54376d..e4b3fd7f0a50 100644
> --- a/include/uapi/linux/dqblk_xfs.h
> +++ b/include/uapi/linux/dqblk_xfs.h
> @@ -62,7 +62,8 @@ typedef struct fs_disk_quota {
>  	__u64		d_bcount;	/* # disk blocks owned by the user */
>  	__u64		d_icount;	/* # inodes owned by the user */
>  	__s32		d_itimer;	/* zero if within inode limits */
> -					/* if not, we refuse service */
> +					/* if not, we refuse service at this
> +					 * time (in seconds since epoch) */

"since Unix epoch"?

Otherwise looks fine to me...

--D

>  	__s32		d_btimer;	/* similar to above; for disk blocks */
>  	__u16	  	d_iwarns;       /* # warnings issued wrt num inodes */
>  	__u16	  	d_bwarns;       /* # warnings issued wrt disk blocks */
> -- 
> 2.16.4
> 

Re: [PATCH v3] quota: widen timestamps for the fs_disk_quota structure

[Jan Kara]

On Wed 09-09-20 10:27:01, Darrick J. Wong wrote:
> On Wed, Sep 09, 2020 at 03:56:45PM +0200, Jan Kara wrote:
> > On Wed 09-09-20 13:42:52, Matthew Wilcox wrote:
> > > On Wed, Sep 09, 2020 at 12:51:33PM +0200, Jan Kara wrote:
> > > > On Tue 08-09-20 19:29:09, Darrick J. Wong wrote:
> > > > > On Wed, Sep 09, 2020 at 02:49:33AM +0100, Matthew Wilcox wrote:
> > > > > > On Tue, Sep 08, 2020 at 06:32:51PM -0700, Darrick J. Wong wrote:
> > > > > > > +static inline void copy_to_xfs_dqblk_ts(const struct fs_disk_quota *d,
> > > > > > > +		__s32 *timer_lo, __s8 *timer_hi, s64 timer)
> > > > > > > +{
> > > > > > > +	*timer_lo = timer;
> > > > > > > +	if (d->d_fieldmask & FS_DQ_BIGTIME)
> > > > > > > +		*timer_hi = timer >> 32;
> > > > > > > +	else
> > > > > > > +		*timer_hi = 0;
> > > > > > > +}
> > > > > > 
> > > > > > I'm still confused by this.  What breaks if you just do:
> > > > > > 
> > > > > > 	*timer_lo = timer;
> > > > > > 	*timer_hi = timer >> 32;
> > > > > 
> > > > > "I don't know."
> > > > > 
> > > > > The manpage for quotactl doesn't actually specify the behavior of the
> > > > > padding fields.  The /implementation/ is careful enough to zero
> > > > > everything, but the interface specification doesn't explicitly require
> > > > > software to do so.
> > > > > 
> > > > > Because the contents of the padding fields aren't defined by the
> > > > > documentation, the kernel cannot simply start using the d_padding2 field
> > > > > because there could be an old kernel that doesn't zero the padding,
> > > > > which would lead to confusion if the new userspace were mated to such a
> > > > > kernel.
> > > > > 
> > > > > Therefore, we have to add a flag that states explicitly that we are
> > > > > using the timer_hi fields.  This is also the only way that an old
> > > > > program can detect that it's being fed a structure that it might not
> > > > > recognise.
> > > > 
> > > > Well, this is in the direction from kernel to userspace and what Matthew
> > > > suggests would just make kernel posssibly store non-zero value in *timer_hi
> > > > without setting FS_DQ_BIGTIME flag (for negative values of timer). I don't
> > > > think it would break anything but I agree the complication isn't big so
> > > > let's be careful and only set *timer_hi to non-zero if FS_DQ_BIGTIME is
> > > > set.
> > > 
> > > OK, thanks.  I must admit, I'd completely forgotten about the negative
> > > values ... and the manpage (quotactl(2)) could be clearer:
> > > 
> > >                       int32_t  d_itimer;    /* Zero if within inode limits */
> > >                                             /* If not, we refuse service */
> > >                       int32_t  d_btimer;    /* Similar to above; for
> > >                                                disk blocks */
> > > 
> > > I can't tell if this is a relative time or seconds since 1970 since we
> > > exceeded the quota.
> > 
> > In fact, it is time (in seconds since epoch) when softlimit becomes
> > enforced (i.e. when you cannot write any more blocks/inodes if you are
> > still over softlimit). I agree the comment incomplete at best. Something
> > like attached patch?
> > 
> > 								Honza
> > -- 
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
> 
> > From 3e3260a337ff444e3a1396834b20da63d7b87ccb Mon Sep 17 00:00:00 2001
> > From: Jan Kara <jack@suse.cz>
> > Date: Wed, 9 Sep 2020 15:54:46 +0200
> > Subject: [PATCH] quota: Expand comment describing d_itimer
> > 
> > Expand comment describing d_itimer in struct fs_disk_quota.
> > 
> > Reported-by: Matthew Wilcox <willy@infradead.org>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  include/uapi/linux/dqblk_xfs.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/uapi/linux/dqblk_xfs.h b/include/uapi/linux/dqblk_xfs.h
> > index 16d73f54376d..e4b3fd7f0a50 100644
> > --- a/include/uapi/linux/dqblk_xfs.h
> > +++ b/include/uapi/linux/dqblk_xfs.h
> > @@ -62,7 +62,8 @@ typedef struct fs_disk_quota {
> >  	__u64		d_bcount;	/* # disk blocks owned by the user */
> >  	__u64		d_icount;	/* # inodes owned by the user */
> >  	__s32		d_itimer;	/* zero if within inode limits */
> > -					/* if not, we refuse service */
> > +					/* if not, we refuse service at this
> > +					 * time (in seconds since epoch) */
> 
> "since Unix epoch"?
> 
> Otherwise looks fine to me...

Thanks for having a look. Patch updated and added to my tree.

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