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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A7E4C76196 for ; Tue, 11 Apr 2023 09:37:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1792C900004; Tue, 11 Apr 2023 05:37:35 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1295F900002; Tue, 11 Apr 2023 05:37:35 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F341C900004; Tue, 11 Apr 2023 05:37:34 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id E4A3E900002 for ; Tue, 11 Apr 2023 05:37:34 -0400 (EDT) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id ACC33C0461 for ; Tue, 11 Apr 2023 09:37:34 +0000 (UTC) X-FDA: 80668607628.13.9FEC139 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf19.hostedemail.com (Postfix) with ESMTP id EDC071A0007 for ; Tue, 11 Apr 2023 09:37:32 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=pBL2eaTc; spf=pass (imf19.hostedemail.com: domain of cem@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cem@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1681205853; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=nRdtqmVuxt0nMVA7mz5AI71jlUAuVMFzGpZlcOu5nJ0=; b=pCrlCqpsRBwn5Qdc+mf9WJN0hHiAcOeSrNt381FiY9AcEGrmwTZqhouQpGkZAcowKbrIoS OFddw8vsBOYqnWbY3lrjqq7eNAtdAPdevKpbcBuXDqj9Q7ysHvgExDDCCwHH1jzzHVPl5r n5s/8YZvCkVurQ8uXq1sTbKSfOaXPlU= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=pBL2eaTc; spf=pass (imf19.hostedemail.com: domain of cem@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cem@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1681205853; a=rsa-sha256; cv=none; b=Rw1OoF+tOVP7THa2xUIwsNOyDQ8z8gBMoZNU+sx3d/XT32ZPzG5FNCgJg+Kw1yXW29Ztky wWhKpnDPGPmbIzm9QWpn3HfF6gz3/m6a2jHB6FcIyXrgidBiKrXiXHMP3SZ5mmWNjrAyLL Cm4vMaJ+ZXQ735aoI/omJjjBrLAJIEU= Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id ECCE262340; Tue, 11 Apr 2023 09:37:31 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id ED750C433EF; Tue, 11 Apr 2023 09:37:29 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1681205851; bh=5PlEMqN/UBrhLJVA69a54wVxgF/au+xwUROz5ZRZTLs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=pBL2eaTcpn4mx+2/MhZEz5SbV4/na1I2Ha8usy222aGjSKdxSmEnzRC2qxVZEcRLh BJ/3f5C98ZN2l5mEIhtVqkykHk60nYVG8ftItWudRfQeM0MesP6JQvU1KBO4iJ34qE /GU8z4KL9pCVceD8X1HXQ9PscjIQSlcw+Vb2kjMiFWIFkM4JYIBCdxalFO0YeAltwN 0CTvAxLvLtOzi9WzvsoarCXP5jYTDKkpRLEG16lEWgWx8T3m/3TE2hTVOH8wL7lk5m RGeC4UndqJ2HS0MYef432DIYw9eit1makyMZefbCsfx3mOc7bQfDhTLr4LVobUWGFT N61OF4pnc4KgQ== Date: Tue, 11 Apr 2023 11:37:26 +0200 From: Carlos Maiolino To: Jan Kara Cc: hughd@google.com, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, djwong@kernel.org Subject: Re: [PATCH 5/6] shmem: quota support Message-ID: <20230411093726.ry3e6espmocvwq6f@andromeda> References: <20230403084759.884681-1-cem@kernel.org> <20230403084759.884681-6-cem@kernel.org> <20230405114245.nnzorjm5nlr4l4g6@quack3> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230405114245.nnzorjm5nlr4l4g6@quack3> X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: EDC071A0007 X-Stat-Signature: othx3tx6rzuooc9n8aeb6tgzcmz9rnc7 X-Rspam-User: X-HE-Tag: 1681205852-204843 X-HE-Meta: U2FsdGVkX1/S3hEFv8Cn4yT4G5SLOW7+hbtvrXKrc7xY5clniDzyaB8pLeS3s7KjU6XVxh70LqfI72uApb0D8WGTWyk889yOWb2MTTi2BaLiGctcLV5TKmbgczyRkrPlzms6n3HmR1QR5vqsbwb4j+S02BFBN77PtPvKZQ9iyLn4cBRZ2xlinXTzFng7ixdhSQJ0QnbHs6ndo5sZbf9F9nIOBEu/dnePv1vgCRCcRlrJJ8cVtjNNduWgRpVklK4hhi9HtvMP8O2tmo9Pb4RnkJ1RE2zjsg7m+ZYq7m1YWT/SLwqMJ2Pw+umPOOeHQI2LpPmVjYu4e0J+R9c66wo7ZcmAJ6vvpr1q+RQEUMjtDUOYkPRtJc9B1Y/UehySKx4Jzm5colsE5Z0grgaNlE01MFwN4nLdC4+GQzXvbOeaspVyefi/gwj8VO2xQnQ0Xg9VF1ZnkIxKfxV9QUTan+Dt0gsuC1QvtQcviVi0CvHhTLRkV39KMeKOYaw0nLuOSOqRpHhU97bb27f+Oxk553kZhhPPv6FJ89SI1l5axwoIkrFb7nOd6WoeXBYK0mtepwYCavszFungJgXaRd1lUQrC8RbAe+NbMIEGi9s95XLcqls7CGJRa4VuJlKIEcOiXocRWp93sVxfbZhCCybicep0S+UWYxIVlFa7o2RpuC11iInWnAuNj7/0h//VZqJ7DWLh3I1mem6Ib9Pz7AW6Mu5b0weBGncSoCiLhy0OYxbkhxoDjewSa8N4W8RnSilciHTVcMKXJP0iUMwelMpyRLdEO/0p+4Zv+TPqijRwqplag936/Bzo7ZMwmt3ETULsnPB9UBq3mN1baM42sij1WT0alfSgHNVx9vpFDyauF7a1RXJ3BzD47yzoJGEczccYRHJccERfhzzZ0ctMjfWAlDj2HDyhyxgkDCXgzINGYRdjHp3q8EUoMT6npiLAYuryVLxaWwqoITTPPyCHdrrRY2S GZsbnDV6 x+PRIP9AA/wLiTmGLjEVBLYv5P5ApoZgTdxZlve66eGGLIjQomXglqcuY/RaN2v6AK3g8zpSQfWHSawlm15SCGs5dgmw25nux6ISNLH6oRGGivMjSgIJplSzbji0djF3Sp3XJPVQo/yD0PJWS0KIDV1HLykhN9wY66a4gvIdjjaaEnXPZuFIi30QMcgjWpwM/4uhPvHK4UHIRs+E4aSxZwrM6BV9yfpGPRA1SDDq7bxu2JCWA1FtM7J49oTVEFMEZQAkNUsEDcgPCXs9nxPdeS7k2+e5d7cKgwHWg9bARwce+ngHQnhciO3MB60DddfSVU5cxDpFosTlO8kmsFCajeMVYdcb2E+uSqtXkte3rQKjZjbw4Ufdi0KohWiZbztkZIYmk X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Apr 05, 2023 at 01:42:45PM +0200, Jan Kara wrote: > On Mon 03-04-23 10:47:58, cem@kernel.org wrote: > > From: Lukas Czerner > > > > Now the basic infra-structure is in place, enable quota support for tmpfs. > > > > Signed-off-by: Lukas Czerner > > Signed-off-by: Carlos Maiolino > > Some comments below... > > > diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h > > index cf38381bdb4c1..3e7e18726feb5 100644 > > --- a/include/linux/shmem_fs.h > > +++ b/include/linux/shmem_fs.h > > @@ -26,6 +26,9 @@ struct shmem_inode_info { > > atomic_t stop_eviction; /* hold when working on inode */ > > struct timespec64 i_crtime; /* file creation time */ > > unsigned int fsflags; /* flags for FS_IOC_[SG]ETFLAGS */ > > +#ifdef CONFIG_TMPFS_QUOTA > > + struct dquot *i_dquot[MAXQUOTAS]; > > +#endif > > struct inode vfs_inode; > > }; > > > > @@ -171,4 +174,10 @@ extern int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, pmd_t *dst_pmd, > > #define SHMEM_QUOTA_MAX_SPC_LIMIT 0x7fffffffffffffffLL /* 2^63-1 */ > > #define SHMEM_QUOTA_MAX_INO_LIMIT 0x7fffffffffffffffLL > > > > +#ifdef CONFIG_TMPFS_QUOTA > > +#define SHMEM_MAXQUOTAS 2 > > You have this definition already in mm/shmem_quota.c. > True, that define is not visible from here though, I'll simply remove the one from mm/shmem_quota.c and keep it here in shmem_fs.h. > > +extern const struct dquot_operations shmem_quota_operations; > > +extern struct quota_format_type shmem_quota_format; > > +#endif /* CONFIG_TMPFS_QUOTA */ > > + > > #endif > > diff --git a/mm/shmem.c b/mm/shmem.c > > index 88e13930fc013..d7529c883eaf5 100644 > > --- a/mm/shmem.c > > +++ b/mm/shmem.c > > @@ -79,6 +79,7 @@ static struct vfsmount *shm_mnt; > > #include > > #include > > #include > > +#include > > > > #include > > > > @@ -116,10 +117,12 @@ struct shmem_options { > > bool full_inums; > > int huge; > > int seen; > > + unsigned short quota_types; > > #define SHMEM_SEEN_BLOCKS 1 > > #define SHMEM_SEEN_INODES 2 > > #define SHMEM_SEEN_HUGE 4 > > #define SHMEM_SEEN_INUMS 8 > > +#define SHMEM_SEEN_QUOTA 16 > > }; > > > > #ifdef CONFIG_TMPFS > > @@ -211,8 +214,11 @@ static inline int shmem_inode_acct_block(struct inode *inode, long pages) > > if (percpu_counter_compare(&sbinfo->used_blocks, > > sbinfo->max_blocks - pages) > 0) > > goto unacct; > > + if ((err = dquot_alloc_block_nodirty(inode, pages)) != 0) > > + goto unacct; > > We generally try to avoid assignments in conditions so I'd do: > > err = dquot_alloc_block_nodirty(inode, pages); > if (err) > goto unacct; Fair enough. Will update it for the new version > > > percpu_counter_add(&sbinfo->used_blocks, pages); > > - } > > + } else if ((err = dquot_alloc_block_nodirty(inode, pages)) != 0) > > + goto unacct; > > > > The same here... > > > @@ -1133,6 +1179,15 @@ static int shmem_setattr(struct mnt_idmap *idmap, > > } > > } > > > > + /* Transfer quota accounting */ > > + if (i_uid_needs_update(idmap, attr, inode) || > > + i_gid_needs_update(idmap, attr,inode)) { > > + error = dquot_transfer(idmap, inode, attr); > > + > > + if (error) > > + return error; > > + } > > + > > I think you also need to add: > > if (is_quota_modification(idmap, inode, attr)) { > error = dquot_initialize(inode); > if (error) > return error; > } > > to shmem_setattr(). Ok. > > > setattr_copy(idmap, inode, attr); > > if (attr->ia_valid & ATTR_MODE) > > error = posix_acl_chmod(idmap, dentry, inode->i_mode); > > @@ -1178,7 +1233,9 @@ static void shmem_evict_inode(struct inode *inode) > > simple_xattrs_free(&info->xattrs); > > WARN_ON(inode->i_blocks); > > shmem_free_inode(inode->i_sb); > > + dquot_free_inode(inode); > > clear_inode(inode); > > + dquot_drop(inode); > > } > > > > static int shmem_find_swap_entries(struct address_space *mapping, > > @@ -1975,7 +2032,6 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index, > > > > spin_lock_irq(&info->lock); > > info->alloced += folio_nr_pages(folio); > > - inode->i_blocks += (blkcnt_t)BLOCKS_PER_PAGE << folio_order(folio); > > shmem_recalc_inode(inode); > > spin_unlock_irq(&info->lock); > > alloced = true; > > @@ -2346,9 +2402,10 @@ static void shmem_set_inode_flags(struct inode *inode, unsigned int fsflags) > > #define shmem_initxattrs NULL > > #endif > > > > -static struct inode *shmem_get_inode(struct mnt_idmap *idmap, struct super_block *sb, > > - struct inode *dir, umode_t mode, dev_t dev, > > - unsigned long flags) > > +static struct inode *shmem_get_inode_noquota(struct mnt_idmap *idmap, > > + struct super_block *sb, > > + struct inode *dir, umode_t mode, > > + dev_t dev, unsigned long flags) > > { > > struct inode *inode; > > struct shmem_inode_info *info; > > @@ -2422,6 +2479,37 @@ static struct inode *shmem_get_inode(struct mnt_idmap *idmap, struct super_block > > return inode; > > } > > > > +static struct inode *shmem_get_inode(struct mnt_idmap *idmap, > > + struct super_block *sb, struct inode *dir, > > + umode_t mode, dev_t dev, unsigned long flags) > > +{ > > + int err; > > + struct inode *inode; > > + > > + inode = shmem_get_inode_noquota(idmap, sb, dir, mode, dev, flags); > > + if (IS_ERR(inode)) > > + return inode; > > + > > + err = dquot_initialize(inode); > > + if (err) > > + goto errout; > > + > > + err = dquot_alloc_inode(inode); > > + if (err) { > > + dquot_drop(inode); > > + goto errout; > > + } > > + return inode; > > + > > +errout: > > + inode->i_flags |= S_NOQUOTA; > > + iput(inode); > > + shmem_free_inode(sb); > > I think shmem_free_inode() is superfluous here. iput() above should already > unaccount the inode... Right, I see it can be called from .evict_inode during iput_final(). Thanks for spotting it. > > > + if (err) > > How could err be possibly unset here? I don't think it can, I'll update it. > > > + return ERR_PTR(err); > > + return NULL; > > +} > > + > > > @@ -3582,6 +3679,18 @@ static int shmem_parse_one(struct fs_context *fc, struct fs_parameter *param) > > ctx->full_inums = true; > > ctx->seen |= SHMEM_SEEN_INUMS; > > break; > > + case Opt_quota: > > + ctx->seen |= SHMEM_SEEN_QUOTA; > > + ctx->quota_types |= (QTYPE_MASK_USR | QTYPE_MASK_GRP); > > + break; > > + case Opt_usrquota: > > + ctx->seen |= SHMEM_SEEN_QUOTA; > > + ctx->quota_types |= QTYPE_MASK_USR; > > + break; > > + case Opt_grpquota: > > + ctx->seen |= SHMEM_SEEN_QUOTA; > > + ctx->quota_types |= QTYPE_MASK_GRP; > > + break; > > } > > return 0; > > > > @@ -3681,6 +3790,12 @@ static int shmem_reconfigure(struct fs_context *fc) > > goto out; > > } > > > > + if (ctx->seen & SHMEM_SEEN_QUOTA && > > + !sb_any_quota_loaded(fc->root->d_sb)) { > > + err = "Cannot enable quota on remount"; > > + goto out; > > + } > > + > > if (ctx->seen & SHMEM_SEEN_HUGE) > > sbinfo->huge = ctx->huge; > > if (ctx->seen & SHMEM_SEEN_INUMS) > > @@ -3763,6 +3878,9 @@ static void shmem_put_super(struct super_block *sb) > > { > > struct shmem_sb_info *sbinfo = SHMEM_SB(sb); > > > > +#ifdef CONFIG_TMPFS_QUOTA > > + shmem_disable_quotas(sb); > > +#endif > > free_percpu(sbinfo->ino_batch); > > percpu_counter_destroy(&sbinfo->used_blocks); > > mpol_put(sbinfo->mpol); > > @@ -3841,6 +3959,17 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc) > > #endif > > uuid_gen(&sb->s_uuid); > > > > +#ifdef CONFIG_TMPFS_QUOTA > > + if (ctx->seen & SHMEM_SEEN_QUOTA) { > > + sb->dq_op = &shmem_quota_operations; > > + sb->s_qcop = &dquot_quotactl_sysfile_ops; > > + sb->s_quota_types = QTYPE_MASK_USR | QTYPE_MASK_GRP; > > s_quota_types should rather be copied from ctx, shouldn't it? Or why is > s_quota_types inconsistent with ctx->quota_types? I believe s_qupta_types here is a bitmask of supported quota types, while ctx->quota_types refers to the mount options being passed from the user. So we should enable in sb->s_quota_types which quota types the filesystem supports, not which were enabled by the user. Cheers. -- Carlos Maiolino