From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 122AC3081B7; Mon, 15 Sep 2025 12:21:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757938868; cv=none; b=Lr+fmhaOzA7QGMaPMBmv2mixJRlfssVnCmDSUlb6MTy1VWx2pqxBkWksWmY4epuOdeO2EiFQPm+h0rH8NOmRGJRY4ryJfRY7pL7ZZo/19Bgy2SO1hgm19ovgwjfl2x/jkaPEJXBB+K7dl1JNQ1XxNcUPWxPcQAamdcY+OK6D8Co= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757938868; c=relaxed/simple; bh=8lfc6XMmCEqrdB6ag90R+cZmEFmO/+qdAqdG2uHElgY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=TvjSCpMK/2aNL3ueWsGf1vQ2ScGPrhly1BLeJH92SMUdPWAXHuJp/q3KMl4v6MedgGF3EKsxOn/nRRiH/skiD2QgJ/DkMHS3+z8AsOMFg8ng1xAHAAoPHTc9o/Grhoqqqq8QTuCPqwPodKv1tw6at+sOkTAhS1SU9Bri1tdUKPg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=E2vy+jzO; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="E2vy+jzO" Received: by smtp.kernel.org (Postfix) with ESMTPSA id BDA75C4CEF5; Mon, 15 Sep 2025 12:21:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1757938867; bh=8lfc6XMmCEqrdB6ag90R+cZmEFmO/+qdAqdG2uHElgY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=E2vy+jzOOvWjLtfVkBg+1i/BTUsPhURr6bEieNtkgZI/ilXfym63hUkcu1bcMi6qN +IWn1gJHUssY5s+irbl1BegmcWrGw4bLTx7s5vsb637yMnRvlKl+svwzA4+DpuKjhw VhSi+FtGemVi5uL//6v6U51Q9/HuIIyS7dn8W9YHGrKIslSGshMbU4p6Qq9Qw5B8IM vCNy9DTOQm64Aqhf/zhvSc5JFI01oibrEAC8Qc3yOWPEMgzLtw6K+We3ohdUAG6Bbu YiidQM7wbH3dL0z0C+ptVCHCUTqoDw4yE6yQOVcYpk3eKoI2UrNgULkBTMPyNZs6ep cgHlVudFh83CQ== Date: Mon, 15 Sep 2025 14:21:02 +0200 From: Christian Brauner To: Joseph Qi Cc: Jan Kara , Mateusz Guzik , Mark Tinguely , ocfs2-devel@lists.linux.dev, viro@zeniv.linux.org.uk, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, josef@toxicpanda.com, jlbec@evilplan.org, mark@fasheh.com, willy@infradead.org, david@fromorbit.com Subject: Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage Message-ID: <20250915-trieb-undeutlich-e68568ff9fe7@brauner> References: <20250904154245.644875-1-mjguzik@gmail.com> <8ddcaa59-0cf0-4b7c-a121-924105f7f5a6@linux.alibaba.com> <02814cfb-9a51-4e67-942c-4da0c57a75c4@linux.alibaba.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <02814cfb-9a51-4e67-942c-4da0c57a75c4@linux.alibaba.com> On Tue, Sep 09, 2025 at 05:58:21PM +0800, Joseph Qi wrote: > > > On 2025/9/9 17:49, Jan Kara wrote: > > On Tue 09-09-25 09:23:56, Joseph Qi wrote: > >> On 2025/9/8 21:54, Jan Kara wrote: > >>> On Mon 08-09-25 20:41:21, Joseph Qi wrote: > >>>> > >>>> > >>>> On 2025/9/8 18:23, Jan Kara wrote: > >>>>> On Mon 08-09-25 09:51:36, Joseph Qi wrote: > >>>>>> On 2025/9/5 00:22, Mateusz Guzik wrote: > >>>>>>> On Thu, Sep 4, 2025 at 6:15 PM Mark Tinguely wrote: > >>>>>>>> > >>>>>>>> On 9/4/25 10:42 AM, Mateusz Guzik wrote: > >>>>>>>>> This postpones the writeout to ocfs2_evict_inode(), which I'm told is > >>>>>>>>> fine (tm). > >>>>>>>>> > >>>>>>>>> The intent is to retire the I_WILL_FREE flag. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Mateusz Guzik > >>>>>>>>> --- > >>>>>>>>> > >>>>>>>>> ACHTUNG: only compile-time tested. Need an ocfs2 person to ack it. > >>>>>>>>> > >>>>>>>>> btw grep shows comments referencing ocfs2_drop_inode() which are already > >>>>>>>>> stale on the stock kernel, I opted to not touch them. > >>>>>>>>> > >>>>>>>>> This ties into an effort to remove the I_WILL_FREE flag, unblocking > >>>>>>>>> other work. If accepted would be probably best taken through vfs > >>>>>>>>> branches with said work, see https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs-6.18.inode.refcount.preliminaries__;!!ACWV5N9M2RV99hQ!OLwk8DVo7uvC-Pd6XVTiUCgP6MUDMKBMEyuV27h_yPGXOjaq078-kMdC9ILFoYQh-4WX93yb0nMfBDFFY_0$ > >>>>>>>>> > >>>>>>>>> fs/ocfs2/inode.c | 23 ++--------------------- > >>>>>>>>> fs/ocfs2/inode.h | 1 - > >>>>>>>>> fs/ocfs2/ocfs2_trace.h | 2 -- > >>>>>>>>> fs/ocfs2/super.c | 2 +- > >>>>>>>>> 4 files changed, 3 insertions(+), 25 deletions(-) > >>>>>>>>> > >>>>>>>>> diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c > >>>>>>>>> index 6c4f78f473fb..5f4a2cbc505d 100644 > >>>>>>>>> --- a/fs/ocfs2/inode.c > >>>>>>>>> +++ b/fs/ocfs2/inode.c > >>>>>>>>> @@ -1290,6 +1290,8 @@ static void ocfs2_clear_inode(struct inode *inode) > >>>>>>>>> > >>>>>>>>> void ocfs2_evict_inode(struct inode *inode) > >>>>>>>>> { > >>>>>>>>> + write_inode_now(inode, 1); > >>>>>>>>> + > >>>>>>>>> if (!inode->i_nlink || > >>>>>>>>> (OCFS2_I(inode)->ip_flags & OCFS2_INODE_MAYBE_ORPHANED)) { > >>>>>>>>> ocfs2_delete_inode(inode); > >>>>>>>>> @@ -1299,27 +1301,6 @@ void ocfs2_evict_inode(struct inode *inode) > >>>>>>>>> ocfs2_clear_inode(inode); > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> -/* Called under inode_lock, with no more references on the > >>>>>>>>> - * struct inode, so it's safe here to check the flags field > >>>>>>>>> - * and to manipulate i_nlink without any other locks. */ > >>>>>>>>> -int ocfs2_drop_inode(struct inode *inode) > >>>>>>>>> -{ > >>>>>>>>> - struct ocfs2_inode_info *oi = OCFS2_I(inode); > >>>>>>>>> - > >>>>>>>>> - trace_ocfs2_drop_inode((unsigned long long)oi->ip_blkno, > >>>>>>>>> - inode->i_nlink, oi->ip_flags); > >>>>>>>>> - > >>>>>>>>> - assert_spin_locked(&inode->i_lock); > >>>>>>>>> - inode->i_state |= I_WILL_FREE; > >>>>>>>>> - spin_unlock(&inode->i_lock); > >>>>>>>>> - write_inode_now(inode, 1); > >>>>>>>>> - spin_lock(&inode->i_lock); > >>>>>>>>> - WARN_ON(inode->i_state & I_NEW); > >>>>>>>>> - inode->i_state &= ~I_WILL_FREE; > >>>>>>>>> - > >>>>>>>>> - return 1; > >>>>>>>>> -} > >>>>>>>>> - > >>>>>>>>> /* > >>>>>>>>> * This is called from our getattr. > >>>>>>>>> */ > >>>>>>>>> diff --git a/fs/ocfs2/inode.h b/fs/ocfs2/inode.h > >>>>>>>>> index accf03d4765e..07bd838e7843 100644 > >>>>>>>>> --- a/fs/ocfs2/inode.h > >>>>>>>>> +++ b/fs/ocfs2/inode.h > >>>>>>>>> @@ -116,7 +116,6 @@ static inline struct ocfs2_caching_info *INODE_CACHE(struct inode *inode) > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> void ocfs2_evict_inode(struct inode *inode); > >>>>>>>>> -int ocfs2_drop_inode(struct inode *inode); > >>>>>>>>> > >>>>>>>>> /* Flags for ocfs2_iget() */ > >>>>>>>>> #define OCFS2_FI_FLAG_SYSFILE 0x1 > >>>>>>>>> diff --git a/fs/ocfs2/ocfs2_trace.h b/fs/ocfs2/ocfs2_trace.h > >>>>>>>>> index 54ed1495de9a..4b32fb5658ad 100644 > >>>>>>>>> --- a/fs/ocfs2/ocfs2_trace.h > >>>>>>>>> +++ b/fs/ocfs2/ocfs2_trace.h > >>>>>>>>> @@ -1569,8 +1569,6 @@ DEFINE_OCFS2_ULL_ULL_UINT_EVENT(ocfs2_delete_inode); > >>>>>>>>> > >>>>>>>>> DEFINE_OCFS2_ULL_UINT_EVENT(ocfs2_clear_inode); > >>>>>>>>> > >>>>>>>>> -DEFINE_OCFS2_ULL_UINT_UINT_EVENT(ocfs2_drop_inode); > >>>>>>>>> - > >>>>>>>>> TRACE_EVENT(ocfs2_inode_revalidate, > >>>>>>>>> TP_PROTO(void *inode, unsigned long long ino, > >>>>>>>>> unsigned int flags), > >>>>>>>>> diff --git a/fs/ocfs2/super.c b/fs/ocfs2/super.c > >>>>>>>>> index 53daa4482406..e4b0d25f4869 100644 > >>>>>>>>> --- a/fs/ocfs2/super.c > >>>>>>>>> +++ b/fs/ocfs2/super.c > >>>>>>>>> @@ -129,7 +129,7 @@ static const struct super_operations ocfs2_sops = { > >>>>>>>>> .statfs = ocfs2_statfs, > >>>>>>>>> .alloc_inode = ocfs2_alloc_inode, > >>>>>>>>> .free_inode = ocfs2_free_inode, > >>>>>>>>> - .drop_inode = ocfs2_drop_inode, > >>>>>>>>> + .drop_inode = generic_delete_inode, > >>>>>>>>> .evict_inode = ocfs2_evict_inode, > >>>>>>>>> .sync_fs = ocfs2_sync_fs, > >>>>>>>>> .put_super = ocfs2_put_super, > >>>>>>>> > >>>>>>>> > >>>>>>>> I agree, fileystems should not use I_FREEING/I_WILL_FREE. > >>>>>>>> Doing the sync write_inode_now() should be fine in ocfs_evict_inode(). > >>>>>>>> > >>>>>>>> Question is ocfs_drop_inode. In commit 513e2dae9422: > >>>>>>>> ocfs2: flush inode data to disk and free inode when i_count becomes zero > >>>>>>>> the return of 1 drops immediate to fix a memory caching issue. > >>>>>>>> Shouldn't .drop_inode() still return 1? > >>>>>>> > >>>>>>> generic_delete_inode is a stub doing just that. > >>>>>>> > >>>>>> In case of "drop = 0", it may return directly without calling evict(). > >>>>>> This seems break the expectation of commit 513e2dae9422. > >>>>> > >>>>> generic_delete_inode() always returns 1 so evict() will be called. > >>>>> ocfs2_drop_inode() always returns 1 as well after 513e2dae9422. So I'm not > >>>>> sure which case of "drop = 0" do you see... > >>>>> > >>>> I don't see a real case, just in theory. > >>>> As I described before, if we make sure write_inode_now() will be called > >>>> in iput_final(), it would be fine. > >>> > >>> I'm sorry but I still don't quite understand what you are proposing. If > >>> ->drop() returns 1, the filesystem wants to remove the inode from cache > >>> (perhaps because it was deleted). Hence iput_final() doesn't bother with > >>> writing out such inodes. This doesn't work well with ocfs2 wanting to > >>> always drop inodes hence ocfs2 needs to write the inode itself in > >>> ocfs2_evice_inode(). Perhaps you have some modification to iput_final() in > >>> mind but I'm not sure how that would work so can you perhaps suggest a > >>> patch if you think iput_final() should work differently? Thanks! > >>> > >> I'm just discussing if generic_delete_inode() will always returns 1. And > >> if it is, I'm fine with this change. Sorry for the confusion. > > > > generic_delete_inode() is defined as: > > > > int generic_delete_inode(struct inode *inode) > > { > > return 1; > > } > > > > So the return is pretty much guaranteed :). But I agree with Mateusz the > > function name could be less confusing. > > > Oops, I've mixed it with generic_drop_inode()... Not that uncommon of a mixup...