[PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Mateusz Guzik]

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 <mjguzik@gmail.com>
---

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://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs-6.18.inode.refcount.preliminaries

 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,
-- 
2.43.0

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Mark Tinguely]

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 <mjguzik@gmail.com>
> ---
> 
> 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?

Mark Tinguely

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Mateusz Guzik]

On Thu, Sep 4, 2025 at 6:15 PM Mark Tinguely <mark.tinguely@oracle.com> 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 <mjguzik@gmail.com>
> > ---
> >
> > 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.

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Joseph Qi]

On 2025/9/5 00:22, Mateusz Guzik wrote:
> On Thu, Sep 4, 2025 at 6:15 PM Mark Tinguely <mark.tinguely@oracle.com> 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 <mjguzik@gmail.com>
>>> ---
>>>
>>> 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.

Thanks,
Joseph

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Jan Kara]

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 <mark.tinguely@oracle.com> 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 <mjguzik@gmail.com>
> >>> ---
> >>>
> >>> 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...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Joseph Qi]

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 <mark.tinguely@oracle.com> 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 <mjguzik@gmail.com>
>>>>> ---
>>>>>
>>>>> 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.

Thanks,
Joseph

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Jan Kara]

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 <mark.tinguely@oracle.com> 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 <mjguzik@gmail.com>
> >>>>> ---
> >>>>>
> >>>>> 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!

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Mateusz Guzik]

On Mon, Sep 8, 2025 at 3:54 PM Jan Kara <jack@suse.cz> 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 <mark.tinguely@oracle.com> 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 <mjguzik@gmail.com>
> > >>>>> ---
> > >>>>>
> > >>>>> 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 think generic_delete_inode is a really bad name for what the routine
is doing and it perhaps contributes to the confusion in the thread.

Perhaps it could be renamed to inode_op_stub_always_drop or similar? I
don't for specifics, apart from explicitly stating that the return
value is to drop and bonus points for a prefix showing this is an
inode thing.

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Jan Kara]

On Mon 08-09-25 17:39:22, Mateusz Guzik wrote:
> On Mon, Sep 8, 2025 at 3:54 PM Jan Kara <jack@suse.cz> 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 <mark.tinguely@oracle.com> 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 <mjguzik@gmail.com>
> > > >>>>> ---
> > > >>>>>
> > > >>>>> 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 think generic_delete_inode is a really bad name for what the routine
> is doing and it perhaps contributes to the confusion in the thread.
> 
> Perhaps it could be renamed to inode_op_stub_always_drop or similar? I
> don't for specifics, apart from explicitly stating that the return
> value is to drop and bonus points for a prefix showing this is an
> inode thing.

I think inode_always_drop() would be fine...

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Mateusz Guzik]

On Tue, Sep 9, 2025 at 11:51 AM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 08-09-25 17:39:22, Mateusz Guzik wrote:
> > I think generic_delete_inode is a really bad name for what the routine
> > is doing and it perhaps contributes to the confusion in the thread.
> >
> > Perhaps it could be renamed to inode_op_stub_always_drop or similar? I
> > don't for specifics, apart from explicitly stating that the return
> > value is to drop and bonus points for a prefix showing this is an
> > inode thing.
>
> I think inode_always_drop() would be fine...

sgtm. unfortunately there are quite a few consumers, so I don't know
if this is worth the churn and consequently I'm not going for it.

But should you feel inclined... ;-)
-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Mateusz Guzik]

On Tue, Sep 9, 2025 at 11:52 AM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On Tue, Sep 9, 2025 at 11:51 AM Jan Kara <jack@suse.cz> wrote:
> >
> > On Mon 08-09-25 17:39:22, Mateusz Guzik wrote:
> > > I think generic_delete_inode is a really bad name for what the routine
> > > is doing and it perhaps contributes to the confusion in the thread.
> > >
> > > Perhaps it could be renamed to inode_op_stub_always_drop or similar? I
> > > don't for specifics, apart from explicitly stating that the return
> > > value is to drop and bonus points for a prefix showing this is an
> > > inode thing.
> >
> > I think inode_always_drop() would be fine...
>
> sgtm. unfortunately there are quite a few consumers, so I don't know
> if this is worth the churn and consequently I'm not going for it.
>
> But should you feel inclined... ;-)

Actually got one better: inode_just_drop(), so that it is clear this
is not doing anything else.

Perhaps something to do after the dust settles.
-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Christian Brauner]

On Tue, Sep 09, 2025 at 11:57:11AM +0200, Mateusz Guzik wrote:
> On Tue, Sep 9, 2025 at 11:52 AM Mateusz Guzik <mjguzik@gmail.com> wrote:
> >
> > On Tue, Sep 9, 2025 at 11:51 AM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Mon 08-09-25 17:39:22, Mateusz Guzik wrote:
> > > > I think generic_delete_inode is a really bad name for what the routine
> > > > is doing and it perhaps contributes to the confusion in the thread.
> > > >
> > > > Perhaps it could be renamed to inode_op_stub_always_drop or similar? I
> > > > don't for specifics, apart from explicitly stating that the return
> > > > value is to drop and bonus points for a prefix showing this is an
> > > > inode thing.
> > >
> > > I think inode_always_drop() would be fine...
> >
> > sgtm. unfortunately there are quite a few consumers, so I don't know
> > if this is worth the churn and consequently I'm not going for it.
> >
> > But should you feel inclined... ;-)
> 
> Actually got one better: inode_just_drop(), so that it is clear this
> is not doing anything else.

That's a simple git sed tbh. Just send it to me this week. All big
changes should be done by now.

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Joseph Qi]

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 <mark.tinguely@oracle.com> 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 <mjguzik@gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>> 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.

Before commit 513e2dae9422, ocfs2_drop_inode() may return 1 and postpone
the work to orphan scan. So commit 513e2dae9422 make write_inode_now()
is determinately called by move it to drop_inode().

Now this patch move write_inode_now() down to evict(), and in iput_final()
it has:

if (!drop &&
    !(inode->i_state & I_DONTCACHE) &&
    (sb->s_flags & SB_ACTIVE)) {
	......
	return;
}

So we have to make sure the above condition is not true, otherwise it
breaks the case commit 513e2dae9422 describes.

Thanks,
Joseph

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Jan Kara]

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 <mark.tinguely@oracle.com> 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 <mjguzik@gmail.com>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> 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.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Joseph Qi]

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 <mark.tinguely@oracle.com> 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 <mjguzik@gmail.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> 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()...

Thanks,
Joseph

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Christian Brauner]

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 <mark.tinguely@oracle.com> 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 <mjguzik@gmail.com>
> >>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>> 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...

Re: [External] : [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Joseph Qi]

On 2025/9/5 00:15, 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 <mjguzik@gmail.com>
>> ---
>>
>> 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?
> 
I think commit 513e2dae9422 only expected the write_inode_now() is
determinately called in iput_final(), no matter drop_inode() return 0 or
1.

Thanks,
Joseph

Re: [PATCH] ocfs2: retire ocfs2_drop_inode() and I_WILL_FREE usage

[Jan Kara]

On Thu 04-09-25 17:42:45, 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 <mjguzik@gmail.com>

Looks good to me! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
> 
> 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://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs-6.18.inode.refcount.preliminaries
> 
>  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,
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR