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 DC06CC433FE for ; Wed, 23 Nov 2022 12:35:26 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7EA6D6B0078; Wed, 23 Nov 2022 07:35:26 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 79A246B007B; Wed, 23 Nov 2022 07:35:26 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6146F6B007D; Wed, 23 Nov 2022 07:35:26 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 4E5D46B0078 for ; Wed, 23 Nov 2022 07:35:26 -0500 (EST) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id EB59BABB2E for ; Wed, 23 Nov 2022 12:35:25 +0000 (UTC) X-FDA: 80164652610.06.FC7E8D3 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf02.hostedemail.com (Postfix) with ESMTP id 770C480010 for ; Wed, 23 Nov 2022 12:35:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1669206924; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=FczM2U2c8jPhBN7mu3+VprdRsFNycIyXmZ6j137DtTU=; b=O34oOtjvfOHZFIcCojsFFfuxQVi1JbyEcTK6uHs6cToFd5QxSoZ/hTF3v9ohR3BOw7YVF9 QmGLsteWmnaH9Ld+EC0+c62AbKwI0V4l2oB7M2qekQUhMIHXs6hzfYPIs5Qp1jXlggxN/N NfcprAUQI3iy0qPrzhZ5xPfw+oiK+rc= Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-39-HY_ufyIJNgmIP1qGfvdmsQ-1; Wed, 23 Nov 2022 07:35:21 -0500 X-MC-Unique: HY_ufyIJNgmIP1qGfvdmsQ-1 Received: by mail-qk1-f198.google.com with SMTP id bl21-20020a05620a1a9500b006fa35db066aso22350084qkb.19 for ; Wed, 23 Nov 2022 04:35:21 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=FczM2U2c8jPhBN7mu3+VprdRsFNycIyXmZ6j137DtTU=; b=2muPip/8hnbSyJe/wHb7UYiGIgGDJNyzk2gL/19U7zBkK0uMbex7QhJ38e4NOtN5ST mhtAqlu9htAmvEenfrsvd46SDvUufhHFNfOht8OjB2lv+EDwY91BBYvdlAGpzBoFh70o a6Zm5ubfUJHedFuMn32dpmwDZAEnBscWgDr9djJ0bn9fwGzzM8J85V5xagt5B4MjG5Ws sDIJN8W9mAZryqRm4FWVyaaJ/yr8Z4sZ6SolGDj7zVoX1mjROn9vZx4dAsVNgXunSjig dzc5R4gzpbV4dWzgA1uWy/JMNn5nKeHTqY8MIEFE/ag4MW33NLwIyW5hk4mRVsY4Y+KE Ri8g== X-Gm-Message-State: ANoB5pnMLlg5pXWdlOcMfWGPjqSqLtCGN579uMBx8D8LwcwutLpGuEeP uE8ZIrWKav1zuQ/X2OF0Qz2kf+H0O9oxb0ZegbrZnKTtBKZ46T0m6/Sv9gGBkavQTF8AQzKvmKK 0c7xwwV2G/gs= X-Received: by 2002:ac8:7250:0:b0:3a5:946e:8ba6 with SMTP id l16-20020ac87250000000b003a5946e8ba6mr8504406qtp.688.1669206921061; Wed, 23 Nov 2022 04:35:21 -0800 (PST) X-Google-Smtp-Source: AA0mqf7QPXWO6DRgHCjspzY8wSUV72x9k24SE/GR4yO7Hd8ZE5YClIS7ZVu+wQRPG/flWUfByDNcSw== X-Received: by 2002:ac8:7250:0:b0:3a5:946e:8ba6 with SMTP id l16-20020ac87250000000b003a5946e8ba6mr8504378qtp.688.1669206920740; Wed, 23 Nov 2022 04:35:20 -0800 (PST) Received: from bfoster (c-24-61-119-116.hsd1.ma.comcast.net. [24.61.119.116]) by smtp.gmail.com with ESMTPSA id x8-20020ac87a88000000b003a494b61e67sm9669572qtr.46.2022.11.23.04.35.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 23 Nov 2022 04:35:20 -0800 (PST) Date: Wed, 23 Nov 2022 07:35:26 -0500 From: Brian Foster To: Lukas Czerner Cc: Hugh Dickins , Jan Kara , Eric Sandeen , linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, djwong@kernel.org Subject: Re: [PATCH v2 2/3] shmem: implement user/group quota support for tmpfs Message-ID: References: <20221121142854.91109-1-lczerner@redhat.com> <20221121142854.91109-3-lczerner@redhat.com> <20221123090137.wnnbpg2laauiado6@fedora> MIME-Version: 1.0 In-Reply-To: <20221123090137.wnnbpg2laauiado6@fedora> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1669206925; a=rsa-sha256; cv=none; b=Ir9c/fQuYyq1XkZmx86lhqj6BxmDwDfXJYSCWnvrjtGEzExod6yIYluhJk626o/j472mB2 pfZPE6l3okEJZl350F17zBmR39tvRIcmOwV26M6aCMrUJUQJeJmiXKqvmJLQQd71FfMfq8 MnbIbr1LbPJWLsZBg07UeXqvkolL6Ls= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=O34oOtjv; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf02.hostedemail.com: domain of bfoster@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=bfoster@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1669206925; 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=FczM2U2c8jPhBN7mu3+VprdRsFNycIyXmZ6j137DtTU=; b=e/o8DFDsU6R09z5HiV/ewepARxTbT4x/FmNZp3sR8vlGTOKEyNkAxpy3Tf/VziT8IqdNxu K+Wz32lvZy7IrmhL47SK2my3426XGmBFTxNeCom95V7UQv5C3MFdyBKNJaiyZQZcfZ+Oun zNy8KDgDTsKDtSIHGynmKMkNTRuPjOo= X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 770C480010 X-Rspam-User: Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=O34oOtjv; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf02.hostedemail.com: domain of bfoster@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=bfoster@redhat.com X-Stat-Signature: u3tyu6r4mffe8mt8p88ue9nn7e517s74 X-HE-Tag: 1669206925-376763 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, Nov 23, 2022 at 10:01:37AM +0100, Lukas Czerner wrote: > On Tue, Nov 22, 2022 at 03:57:57PM -0500, Brian Foster wrote: > > On Mon, Nov 21, 2022 at 03:28:53PM +0100, Lukas Czerner wrote: > > > Implement user and group quota support for tmpfs using system quota file > > > in vfsv0 quota format. Because everything in tmpfs is temporary and as a > > > result is lost on umount, the quota files are initialized on every > > > mount. This also goes for quota limits, that needs to be set up after > > > every mount. > > > > > > The quota support in tmpfs is well separated from the rest of the > > > filesystem and is only enabled using mount option -o quota (and > > > usrquota and grpquota for compatibility reasons). Only quota accounting > > > is enabled this way, enforcement needs to be enable by regular quota > > > tools (using Q_QUOTAON ioctl). > > > > > Hi Brian, > > thanks for the review. > > > > > FWIW, just from a first look through, it seems like this could be made a > > little easier to review by splitting it up into a few smaller patches. > > For example, could the accounting and enforcement support split into > > separate patches? > > Maybe a little, it seems a bit pointless to me. > It seems like this is often the case for the patch author. ;) For the reviewer (or for me at least), it's usually quite helpful to see things broken down into the smallest possible changes. Not only does it help a single review pass, but if you have multiple reviewers and expect multiple review cycles, then it saves overall reviewer time not having to revisit logic that has been acked and might not have changed since last posted. That being said, note that I don't know this code terribly well and so this might be less relevant to others. In general, feel free to dismiss any feedback from me that just doesn't make sense. :) > > > > A few more random notes/questions... > > > > > Signed-off-by: Lukas Czerner > > > --- > > > v2: Use the newly introduced in-memory only quota foramt QFMT_MEM_ONLY > > > > > > Documentation/filesystems/tmpfs.rst | 12 ++ > > > fs/quota/dquot.c | 10 +- > > > include/linux/shmem_fs.h | 3 + > > > mm/shmem.c | 200 ++++++++++++++++++++++++---- > > > 4 files changed, 197 insertions(+), 28 deletions(-) > > > > > ... > > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > > > index f1a7a03632a2..007604e7eb09 100644 > > > --- a/fs/quota/dquot.c > > > +++ b/fs/quota/dquot.c > > > @@ -716,11 +716,11 @@ int dquot_quota_sync(struct super_block *sb, int type) > > > for (cnt = 0; cnt < MAXQUOTAS; cnt++) { > > > if (type != -1 && cnt != type) > > > continue; > > > - if (!sb_has_quota_active(sb, cnt)) > > > - continue; > > > - inode_lock(dqopt->files[cnt]); > > > - truncate_inode_pages(&dqopt->files[cnt]->i_data, 0); > > > - inode_unlock(dqopt->files[cnt]); > > > + if (sb_has_quota_active(sb, cnt) && dqopt->files[cnt]) { > > > + inode_lock(dqopt->files[cnt]); > > > + truncate_inode_pages(&dqopt->files[cnt]->i_data, 0); > > > + inode_unlock(dqopt->files[cnt]); > > > + } > > > > Perhaps a separate patch with some context for the change in the commit > > log? (Maybe it's obvious to others, I'm just not familiar with the core > > quota code.) > > Oops, this hunk needs to be in the first patch. It is making sure that > we won't touch dquot->files[] if we don't have any quota files which is > the case for QFMT_MEM_ONLY format. I'll add some comment here. > Ok. > > > > > } > > > > > > return 0; > > ... > > > diff --git a/mm/shmem.c b/mm/shmem.c > > > index c1d8b8a1aa3b..26f2effd8f7c 100644 > > > --- a/mm/shmem.c > > > +++ b/mm/shmem.c ... > > > @@ -353,7 +429,6 @@ static void shmem_recalc_inode(struct inode *inode) > > > freed = info->alloced - info->swapped - inode->i_mapping->nrpages; > > > if (freed > 0) { > > > info->alloced -= freed; > > > - inode->i_blocks -= freed * BLOCKS_PER_PAGE; > > > > Did these various ->i_blocks updates get moved somewhere? > > Yes, it is being taken care of by dquot_alloc_block_nodirty() and > dquot_free_block_nodirty() in dquot_alloc_block_nodirty() and > dquot_free_block_nodirty() respectively. > Ah, Ok... I assume you mean __inode_[add|sub]_bytes(), called via __dquot_alloc_space() and friends. > You're not the first to ask about this, I'll put that into commit > description. > Ack, thanks. > > > > > shmem_inode_unacct_blocks(inode, freed); > > > } > > > } > > ... > > > @@ -2384,6 +2467,35 @@ static struct inode *shmem_get_inode(struct super_block *sb, struct inode *dir, > > > return inode; > > > } > > > > > > +static struct inode *shmem_get_inode(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(sb, dir, mode, dev, flags); > > > + if (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; > > > > I assume this is here so the free path won't unaccount an inode from the > > quota that wasn't able to allocate, but is it needed with the > > dquot_drop() above? If so, a comment might be helpful. :) > > Yes it is needed. Quota code generally expects dquot to be initialized > for operations such as dquot_free_inode(). It won't be in this case and > hece we have to avoid quota accounting. > Ok. FWIW, it looks to me that the dquot_free_inode() path checks for and handles the case of NULL dquots. That said, I see this pattern is used elsewhere in such error scenarios and on a second look, it seems like explicitly defensive logic. Particularly to prevent something else from perhaps trying to initialize the inode again (assuming failures would be persistent). Makes more sense now, thanks. Brian > > > > > Brian > > Thanks Brian! > > -Lukas > > > > > + iput(inode); > > > + shmem_free_inode(sb); > > > + if (err) > > > + return ERR_PTR(err); > > > + return NULL; > > > +} > > > + > > > #ifdef CONFIG_USERFAULTFD > > > int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, > > > pmd_t *dst_pmd, > > > @@ -2403,7 +2515,7 @@ int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, > > > int ret; > > > pgoff_t max_off; > > > > > > - if (!shmem_inode_acct_block(inode, 1)) { > > > + if (shmem_inode_acct_block(inode, 1)) { > > > /* > > > * We may have got a page, returned -ENOENT triggering a retry, > > > * and now we find ourselves with -ENOMEM. Release the page, to > > > @@ -2487,7 +2599,6 @@ int shmem_mfill_atomic_pte(struct mm_struct *dst_mm, > > > > > > spin_lock_irq(&info->lock); > > > info->alloced++; > > > - inode->i_blocks += BLOCKS_PER_PAGE; > > > shmem_recalc_inode(inode); > > > spin_unlock_irq(&info->lock); > > > > > > @@ -2908,7 +3019,7 @@ shmem_mknod(struct user_namespace *mnt_userns, struct inode *dir, > > > int error = -ENOSPC; > > > > > > inode = shmem_get_inode(dir->i_sb, dir, mode, dev, VM_NORESERVE); > > > - if (inode) { > > > + if (!IS_ERR_OR_NULL(inode)) { > > > error = simple_acl_create(dir, inode); > > > if (error) > > > goto out_iput; > > > @@ -2924,7 +3035,8 @@ shmem_mknod(struct user_namespace *mnt_userns, struct inode *dir, > > > inode_inc_iversion(dir); > > > d_instantiate(dentry, inode); > > > dget(dentry); /* Extra count - pin the dentry in core */ > > > - } > > > + } else if (IS_ERR(inode)) > > > + error = PTR_ERR(inode); > > > return error; > > > out_iput: > > > iput(inode); > > > @@ -2939,7 +3051,7 @@ shmem_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, > > > int error = -ENOSPC; > > > > > > inode = shmem_get_inode(dir->i_sb, dir, mode, 0, VM_NORESERVE); > > > - if (inode) { > > > + if (!IS_ERR_OR_NULL(inode)) { > > > error = security_inode_init_security(inode, dir, > > > NULL, > > > shmem_initxattrs, NULL); > > > @@ -2949,7 +3061,8 @@ shmem_tmpfile(struct user_namespace *mnt_userns, struct inode *dir, > > > if (error) > > > goto out_iput; > > > d_tmpfile(file, inode); > > > - } > > > + } else if (IS_ERR(inode)) > > > + error = PTR_ERR(inode); > > > return finish_open_simple(file, error); > > > out_iput: > > > iput(inode); > > > @@ -3126,6 +3239,8 @@ static int shmem_symlink(struct user_namespace *mnt_userns, struct inode *dir, > > > VM_NORESERVE); > > > if (!inode) > > > return -ENOSPC; > > > + else if (IS_ERR(inode)) > > > + return PTR_ERR(inode); > > > > > > error = security_inode_init_security(inode, dir, &dentry->d_name, > > > shmem_initxattrs, NULL); > > > @@ -3443,6 +3558,7 @@ enum shmem_param { > > > Opt_uid, > > > Opt_inode32, > > > Opt_inode64, > > > + Opt_quota, > > > }; > > > > > > static const struct constant_table shmem_param_enums_huge[] = { > > > @@ -3464,6 +3580,9 @@ const struct fs_parameter_spec shmem_fs_parameters[] = { > > > fsparam_u32 ("uid", Opt_uid), > > > fsparam_flag ("inode32", Opt_inode32), > > > fsparam_flag ("inode64", Opt_inode64), > > > + fsparam_flag ("quota", Opt_quota), > > > + fsparam_flag ("usrquota", Opt_quota), > > > + fsparam_flag ("grpquota", Opt_quota), > > > {} > > > }; > > > > > > @@ -3547,6 +3666,13 @@ 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: > > > +#ifdef CONFIG_QUOTA > > > + ctx->seen |= SHMEM_SEEN_QUOTA; > > > +#else > > > + goto unsupported_parameter; > > > +#endif > > > + break; > > > } > > > return 0; > > > > > > @@ -3646,6 +3772,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) > > > @@ -3728,6 +3860,9 @@ static void shmem_put_super(struct super_block *sb) > > > { > > > struct shmem_sb_info *sbinfo = SHMEM_SB(sb); > > > > > > +#ifdef SHMEM_QUOTA_TMPFS > > > + shmem_disable_quotas(sb); > > > +#endif > > > free_percpu(sbinfo->ino_batch); > > > percpu_counter_destroy(&sbinfo->used_blocks); > > > mpol_put(sbinfo->mpol); > > > @@ -3805,14 +3940,26 @@ static int shmem_fill_super(struct super_block *sb, struct fs_context *fc) > > > #endif > > > uuid_gen(&sb->s_uuid); > > > > > > +#ifdef SHMEM_QUOTA_TMPFS > > > + if (ctx->seen & SHMEM_SEEN_QUOTA) { > > > + sb->dq_op = &dquot_operations; > > > + sb->s_qcop = &dquot_quotactl_sysfile_ops; > > > + sb->s_quota_types = QTYPE_MASK_USR | QTYPE_MASK_GRP; > > > + > > > + if (shmem_enable_quotas(sb)) > > > + goto failed; > > > + } > > > +#endif /* SHMEM_QUOTA_TMPFS */ > > > + > > > inode = shmem_get_inode(sb, NULL, S_IFDIR | sbinfo->mode, 0, VM_NORESERVE); > > > - if (!inode) > > > + if (IS_ERR_OR_NULL(inode)) > > > goto failed; > > > inode->i_uid = sbinfo->uid; > > > inode->i_gid = sbinfo->gid; > > > sb->s_root = d_make_root(inode); > > > if (!sb->s_root) > > > goto failed; > > > + > > > return 0; > > > > > > failed: > > > @@ -3976,7 +4123,12 @@ static const struct super_operations shmem_ops = { > > > #ifdef CONFIG_TMPFS > > > .statfs = shmem_statfs, > > > .show_options = shmem_show_options, > > > -#endif > > > +#ifdef CONFIG_QUOTA > > > + .quota_read = shmem_quota_read, > > > + .quota_write = shmem_quota_write, > > > + .get_dquots = shmem_get_dquots, > > > +#endif /* CONFIG_QUOTA */ > > > +#endif /* CONFIG_TMPFS */ > > > .evict_inode = shmem_evict_inode, > > > .drop_inode = generic_delete_inode, > > > .put_super = shmem_put_super, > > > @@ -4196,8 +4348,10 @@ static struct file *__shmem_file_setup(struct vfsmount *mnt, const char *name, l > > > > > > inode = shmem_get_inode(mnt->mnt_sb, NULL, S_IFREG | S_IRWXUGO, 0, > > > flags); > > > - if (unlikely(!inode)) { > > > + if (IS_ERR_OR_NULL(inode)) { > > > shmem_unacct_size(flags, size); > > > + if (IS_ERR(inode)) > > > + return (struct file *)inode; > > > return ERR_PTR(-ENOSPC); > > > } > > > inode->i_flags |= i_flags; > > > -- > > > 2.38.1 > > > > > > > > >