linux-kernel - Re: [PATCH v2] ocfs2: retire ocfs2_drop_inode() and I_WILL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aOAwhPT-rlnxmEtS@google.com>
Date: Fri, 3 Oct 2025 13:22:28 -0700
From: Joel Becker <jlbec@...lplan.org>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: ocfs2-devel@...ts.linux.dev, jack@...e.cz, viro@...iv.linux.org.uk,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	joseph.qi@...ux.alibaba.com, brauner@...nel.org
Subject: Re: [PATCH v2] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage


Since this is in `iput_final()`, it's outside of OCFS2 cluster locking.
The only work `ocfs_drop_inode()` does is to juggle the spinlock and
state while writing the inode.  `evict()` does this just a little later
in `iput_final()`, and there's no real way the flow gets interrupted, so
it is not even moving the writeout that far.

Reviewed-by: Joel Becker <jlbec@...lplan.org>

On Fri, Oct 03, 2025 at 04:36:52AM +0200, 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.
> 
> Reviewed-by: Jan Kara <jack@...e.cz>
> Signed-off-by: Mateusz Guzik <mjguzik@...il.com>
> ---
> 
> v2:
> - rebase -- generic_delete_inode -> inode_just_drop
> 
> The original posting got derailed and then this got lost in the shuffle,
> see: https://lore.kernel.org/linux-fsdevel/20250904154245.644875-1-mjguzik@gmail.com/
> 
> This is the only filesystem using the flag. The only other spot is in
> iput_final().
> 
> I have a wip patch to sort out the writeback vs iput situation a little
> bit and need this out of the way.
> 
> Even if said patch does not go in, this clearly pushes things forward by
> removing flag usage.
> 
>  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 fcc89856ab95..84115bf8b464 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..2c7ba1480f7a 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	= inode_just_drop,
>  	.evict_inode	= ocfs2_evict_inode,
>  	.sync_fs	= ocfs2_sync_fs,
>  	.put_super	= ocfs2_put_super,
> -- 
> 2.43.0
> 
> 

-- 

"Hell is oneself, hell is alone, the other figures in it, merely projections."
        - T. S. Eliot

			http://www.jlbec.org/
			jlbec@...lplan.org