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(-)
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.0On 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
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
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
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>
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
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
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
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
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
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
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
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...
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>
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
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>
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>
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.
© 2016 - 2025 Red Hat, Inc.