From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.4 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 597D7C352A4 for ; Thu, 13 Feb 2020 02:05:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1443C20659 for ; Thu, 13 Feb 2020 02:05:52 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=oracle.com header.i=@oracle.com header.b="WNoA/X07" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729348AbgBMCFv (ORCPT ); Wed, 12 Feb 2020 21:05:51 -0500 Received: from userp2120.oracle.com ([156.151.31.85]:54282 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729333AbgBMCFv (ORCPT ); Wed, 12 Feb 2020 21:05:51 -0500 Received: from pps.filterd (userp2120.oracle.com [127.0.0.1]) by userp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 01D21F2Y194257; Thu, 13 Feb 2020 02:05:49 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : in-reply-to; s=corp-2020-01-29; bh=a9GZQ8Z76ufVAeMTjS1K6FG/CUwIwQZkqL0wtnQzmGE=; b=WNoA/X07ZKqe8uhcx9RRiP4ia2AyZDruZ4ePAScEi0rL0GDqnXnKxI82+AJA7AYTY0zn EZgtQZiwxICe91ts584NgSET+uVB2+uQu69FQOnQYm+yuBoSKFKTfh3iHCJW5uYzB+55 poeX4iQVBiRMBOZRttgFVVKTKPmoHUFu8TxhCISy+G9YXkI5oIkjEsgU8D7ygDA6ezEu Z0EtKuQOw1FyKbBQmRw9no46dLPAbMy+6R61EreYfPk35WlNZs4Mse/DJ5jq4kMoE1x5 jZTeeJ2uS22/iQNZeVT1+1mQgHwsjG3xWJ9MFAxGfqj/Qd4IJ1Z9nP1xPsQugv32a9NA qQ== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by userp2120.oracle.com with ESMTP id 2y2p3spgb3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 13 Feb 2020 02:05:48 +0000 Received: from pps.filterd (aserp3020.oracle.com [127.0.0.1]) by aserp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 01D1vijq014439; Thu, 13 Feb 2020 02:03:48 GMT Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserp3020.oracle.com with ESMTP id 2y4k33fcvq-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 13 Feb 2020 02:03:48 +0000 Received: from abhmp0003.oracle.com (abhmp0003.oracle.com [141.146.116.9]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 01D23i1Y005693; Thu, 13 Feb 2020 02:03:47 GMT Received: from localhost (/10.159.151.237) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 12 Feb 2020 18:03:44 -0800 Date: Wed, 12 Feb 2020 18:03:42 -0800 From: "Darrick J. Wong" To: Eric Sandeen Cc: linux-xfs@vger.kernel.org Subject: Re: [PATCH 06/14] xfs: refactor default quota grace period setting code Message-ID: <20200213020342.GC6870@magnolia> References: <157784106066.1364230.569420432829402226.stgit@magnolia> <157784110016.1364230.5024129406313355261.stgit@magnolia> <2fde1e65-7ede-3c47-81bd-d39906a8dc77@sandeen.net> <20200213015302.GZ6870@magnolia> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200213015302.GZ6870@magnolia> User-Agent: Mutt/1.9.4 (2018-02-28) X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9529 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 mlxlogscore=999 adultscore=0 bulkscore=0 malwarescore=0 phishscore=0 suspectscore=0 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2001150001 definitions=main-2002130015 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9529 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxscore=0 malwarescore=0 suspectscore=0 mlxlogscore=999 priorityscore=1501 clxscore=1015 impostorscore=0 lowpriorityscore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2001150001 definitions=main-2002130015 Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-xfs@vger.kernel.org On Wed, Feb 12, 2020 at 05:53:02PM -0800, Darrick J. Wong wrote: > On Wed, Feb 12, 2020 at 06:15:18PM -0600, Eric Sandeen wrote: > > On 12/31/19 7:11 PM, Darrick J. Wong wrote: > > > From: Darrick J. Wong > > > > > > Refactor the code that sets the default quota grace period into a helper > > > function so that we can override the ondisk behavior later. > > > > > > Signed-off-by: Darrick J. Wong > > > --- > > > fs/xfs/libxfs/xfs_format.h | 8 ++++++++ > > > fs/xfs/xfs_ondisk.h | 2 ++ > > > fs/xfs/xfs_qm_syscalls.c | 35 +++++++++++++++++++++++------------ > > > fs/xfs/xfs_trans_dquot.c | 16 ++++++++++++---- > > > 4 files changed, 45 insertions(+), 16 deletions(-) > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > > > index 95761b38fe86..557db5e51eec 100644 > > > --- a/fs/xfs/libxfs/xfs_format.h > > > +++ b/fs/xfs/libxfs/xfs_format.h > > > @@ -1188,6 +1188,10 @@ static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev) > > > * time zero is the Unix epoch, Jan 1 00:00:01 UTC 1970. An expiration value > > > * of zero means that the quota limit has not been reached, and therefore no > > > * expiration has been set. > > > + * > > > + * The length of quota grace periods are unsigned 32-bit quantities in units of > > > + * seconds (which are stored in the root dquot). A value of zero means to use > > > + * the default period. > > > > Doesn't a value of zero mean that the soft limit has not been exceeded, and no > > timer is in force? And when soft limit is exceeded, the timer starts ticking > > based on the value in the root dquot? > > Yes and yes. > > > i.e. you can't set a custom per-user grace period, can you? > > And yes. > > > Perhaps: > > > > * The length of quota grace periods are unsigned 32-bit quantities in units of > > * seconds. The grace period for each quota type is stored in the root dquot > > * and is applied/transferred to a user quota when it exceeds a soft limit. > > Much better. I'll crib your version. :) > > > > */ > > > > > > /* > > > @@ -1202,6 +1206,10 @@ static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev) > > > */ > > > #define XFS_DQ_TIMEOUT_MAX ((int64_t)U32_MAX) > > > > > > +/* Quota grace periods, ranging from zero (use the defaults) to ~136 years. */ > > > > same thing. The default can be set between 0 and ~136 years, that gets transferred > > to any user who exceeds soft quota, and it counts down from there. > > Yeah, I'll crib this too. > > --D > > > > +#define XFS_DQ_GRACE_MIN ((int64_t)0) > > > +#define XFS_DQ_GRACE_MAX ((int64_t)U32_MAX) > > > + > > > /* > > > * This is the main portion of the on-disk representation of quota > > > * information for a user. This is the q_core of the struct xfs_dquot that > > > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h > > > index 52dc5326b7bf..b8811f927a3c 100644 > > > --- a/fs/xfs/xfs_ondisk.h > > > +++ b/fs/xfs/xfs_ondisk.h > > > @@ -27,6 +27,8 @@ xfs_check_ondisk_structs(void) > > > XFS_CHECK_VALUE(XFS_INO_TIME_MAX, 2147483647LL); > > > XFS_CHECK_VALUE(XFS_DQ_TIMEOUT_MIN, 1LL); > > > XFS_CHECK_VALUE(XFS_DQ_TIMEOUT_MAX, 4294967295LL); > > > + XFS_CHECK_VALUE(XFS_DQ_GRACE_MIN, 0LL); > > > + XFS_CHECK_VALUE(XFS_DQ_GRACE_MAX, 4294967295LL); > > > > *cough* notondisk *cough* > > > > > > > > /* ag/file structures */ > > > XFS_CHECK_STRUCT_SIZE(struct xfs_acl, 4); > > > diff --git a/fs/xfs/xfs_qm_syscalls.c b/fs/xfs/xfs_qm_syscalls.c > > > index 74220948a360..20a6d304d1be 100644 > > > --- a/fs/xfs/xfs_qm_syscalls.c > > > +++ b/fs/xfs/xfs_qm_syscalls.c > > > @@ -438,6 +438,20 @@ xfs_qm_scall_quotaon( > > > return 0; > > > } > > > > > > +/* Set a new quota grace period. */ > > > +static inline void > > > +xfs_qm_set_grace( > > > + time_t *qi_limit, > > ^ doesn't get used? > > > + __be32 *dtimer, > > > + const s64 grace) > > > +{ > > > + time64_t new_grace; > > > + > > > + new_grace = clamp_t(time64_t, grace, XFS_DQ_GRACE_MIN, > > > + XFS_DQ_GRACE_MAX); > > > + *dtimer = cpu_to_be32(new_grace); > > > > You've lost setting the qi_limit here (q->qi_btimelimit etc) > > > > > +} > > > + > > > #define XFS_QC_MASK \ > > > (QC_LIMIT_MASK | QC_TIMER_MASK | QC_WARNS_MASK) > > > > > > @@ -567,18 +581,15 @@ xfs_qm_scall_setqlim( > > > * soft and hard limit values (already done, above), and > > > * for warnings. > > > */ > > > - if (newlim->d_fieldmask & QC_SPC_TIMER) { > > > - q->qi_btimelimit = newlim->d_spc_timer; > > > > i.e. qi_btimelimit never gets set now, which is what actually controls > > the timers when a uid/gid/pid goes over softlimit. Oops, good catch. Thank you for starting a review of this! --D > > > - ddq->d_btimer = cpu_to_be32(newlim->d_spc_timer); > > > - } > > > - if (newlim->d_fieldmask & QC_INO_TIMER) { > > > - q->qi_itimelimit = newlim->d_ino_timer; > > > - ddq->d_itimer = cpu_to_be32(newlim->d_ino_timer); > > > - } > > > - if (newlim->d_fieldmask & QC_RT_SPC_TIMER) { > > > - q->qi_rtbtimelimit = newlim->d_rt_spc_timer; > > > - ddq->d_rtbtimer = cpu_to_be32(newlim->d_rt_spc_timer); > > > - } > > > + if (newlim->d_fieldmask & QC_SPC_TIMER) > > > + xfs_qm_set_grace(&q->qi_btimelimit, &ddq->d_btimer, > > > + newlim->d_spc_timer); > > > + if (newlim->d_fieldmask & QC_INO_TIMER) > > > + xfs_qm_set_grace(&q->qi_itimelimit, &ddq->d_itimer, > > > + newlim->d_ino_timer); > > > + if (newlim->d_fieldmask & QC_RT_SPC_TIMER) > > > + xfs_qm_set_grace(&q->qi_rtbtimelimit, &ddq->d_rtbtimer, > > > + newlim->d_rt_spc_timer); > > > if (newlim->d_fieldmask & QC_SPC_WARNS) > > > q->qi_bwarnlimit = newlim->d_spc_warns; > > > if (newlim->d_fieldmask & QC_INO_WARNS) > > > diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c > > > index 248cfc369efc..7a2a3bd11db9 100644 > > > --- a/fs/xfs/xfs_trans_dquot.c > > > +++ b/fs/xfs/xfs_trans_dquot.c > > > @@ -563,6 +563,14 @@ xfs_quota_warn( > > > mp->m_super->s_dev, type); > > > } > > > > > > +/* Has a quota grace period expired? */ > > > > seems like this is not part of "quota grace period setting code" > > - needs to be in a separate patch? > > Yeah, it can be a separate refactor patch. > > > > > +static inline bool > > > +xfs_quota_timer_exceeded( > > > + time64_t timer) > > > +{ > > > + return timer != 0 && get_seconds() > timer; > > > +} > > > + > > > /* > > > * This reserves disk blocks and inodes against a dquot. > > > * Flags indicate if the dquot is to be locked here and also > > > @@ -580,7 +588,7 @@ xfs_trans_dqresv( > > > { > > > xfs_qcnt_t hardlimit; > > > xfs_qcnt_t softlimit; > > > - time_t timer; > > > + time64_t timer; > > > > > > Probably. :) > > > > xfs_qwarncnt_t warns; > > > xfs_qwarncnt_t warnlimit; > > > xfs_qcnt_t total_count; > > > @@ -635,7 +643,7 @@ xfs_trans_dqresv( > > > goto error_return; > > > } > > > if (softlimit && total_count > softlimit) { > > > - if ((timer != 0 && get_seconds() > timer) || > > > + if (xfs_quota_timer_exceeded(timer) || > > > (warns != 0 && warns >= warnlimit)) { > > > xfs_quota_warn(mp, dqp, > > > QUOTA_NL_BSOFTLONGWARN); > > > @@ -662,8 +670,8 @@ xfs_trans_dqresv( > > > goto error_return; > > > } > > > if (softlimit && total_count > softlimit) { > > > - if ((timer != 0 && get_seconds() > timer) || > > > - (warns != 0 && warns >= warnlimit)) { > > > + if (xfs_quota_timer_exceeded(timer) || > > > + (warns != 0 && warns >= warnlimit)) { > > > > TBH don't really see the point of this refactoring/helper, especially if not > > done for warns. I think open coding is fine. > > Yeah, in the end the helper doesn't add a lot anymore. IIRC it did in > previous versions of this patch. > > --D > > > > xfs_quota_warn(mp, dqp, > > > QUOTA_NL_ISOFTLONGWARN); > > > goto error_return; > > >