| Message ID | 20200909013251.GG7955@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] quota: widen timestamps for the fs_disk_quota structure | expand |
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?)
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
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
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.
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
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 >
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
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). */