Re: [PATCH] fs: Kill DCACHE_DONTCACHE dentry even if DCACHE_REFERENCED is set

[Li, Hao]

On 2020/8/23 14:54, Ira Weiny wrote:
> On Fri, Aug 21, 2020 at 10:40:41AM -0700, 'Ira Weiny' wrote:
>> On Fri, Aug 21, 2020 at 09:59:53AM +0800, Hao Li wrote:
>>> Currently, DCACHE_REFERENCED prevents the dentry with DCACHE_DONTCACHE
>>> set from being killed, so the corresponding inode can't be evicted. If
>>> the DAX policy of an inode is changed, we can't make policy changing
>>> take effects unless dropping caches manually.
>>>
>>> This patch fixes this problem and flushes the inode to disk to prepare
>>> for evicting it.
>> This looks intriguing and I really hope this helps but I don't think this will
>> guarantee that the state changes immediately will it?
>>
>> Do you have a test case which fails before and passes after?  Perhaps one of
>> the new xfstests submitted by Xiao?
> Ok I just went back and read your comment before.[1]  Sorry for being a bit
> slow on the correlation between this patch and that email.  (BTW, I think it
> would have been good to put those examples in the commit message and or
> reference that example.)

Thanks for your advice. I will add those examples in v2 after further
discussion of this patch.

> I'm assuming that with this patch example 2 from [1]
> works without a drop_cache _if_ no other task has the file open?

Yes. If no other task is opening the file, the inode and page cache of this
file will be dropped during xfs_io exiting process. There is no need to run
echo 2 > drop_caches.

> Anyway, with that explanation I think you are correct that this improves the
> situation _if_ the only references on the file is controlled by the user and
> they have indeed closed all of them.
>
> The code for DCACHE_DONTCACHE as I attempted to write it was that it should
> have prevented further caching of the inode such that the inode would evict
> sooner.  But it seems you have found a bug/optimization?

Yes. This patch is an optimization and can also be treated as a bugfix.
On the other side, even though this patch can make DCACHE_DONTCACHE more
reasonable, I am not quite sure if my approach is safe and doesn't impact
the fs performance. I hope the community can give me more advice.

> In the end, however, if another user (such as a backup running by the admin)
> has a reference the DAX change may still be delayed.

Yes. In this situation, neither drop_caches approach nor this patch can make
the DAX change take effects soon.
Moreover, things are different if the backup task exits, this patch
will make sure the inode and page cache of the file can be dropped
_automatically_ without manual intervention. By contrast, the original
approach needs a manual cache dropping.

> So I'm thinking the
> documentation should remain largely as is?  But perhaps I am wrong.  Does this
> completely remove the need for a drop_caches or only in the example you gave?

I think the contents related to drop_caches in documentation can be removed
if this patch's approach is acceptable.

> Since I'm not a FS expert I'm still not sure.

Frankly, I'm not an expert either, so I hope this patch can be discussed
further in case it has side effects.

Thanks,
Hao Li

>
> Regardless, thanks for the fixup!  :-D
> Ira
>
> [1] https://lore.kernel.org/linux-xfs/ba98b77e-a806-048a-a0dc-ca585677daf3@xxxxxxxxxxxxxx/
>
>> Ira
>>
>>> Signed-off-by: Hao Li <lihao2018.fnst@xxxxxxxxxxxxxx>
>>> ---
>>>  fs/dcache.c | 3 ++-
>>>  fs/inode.c  | 2 +-
>>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/dcache.c b/fs/dcache.c
>>> index ea0485861d93..486c7409dc82 100644
>>> --- a/fs/dcache.c
>>> +++ b/fs/dcache.c
>>> @@ -796,7 +796,8 @@ static inline bool fast_dput(struct dentry *dentry)
>>>  	 */
>>>  	smp_rmb();
>>>  	d_flags = READ_ONCE(dentry->d_flags);
>>> -	d_flags &= DCACHE_REFERENCED | DCACHE_LRU_LIST | DCACHE_DISCONNECTED;
>>> +	d_flags &= DCACHE_REFERENCED | DCACHE_LRU_LIST | DCACHE_DISCONNECTED
>>> +			| DCACHE_DONTCACHE;
>>>  
>>>  	/* Nothing to do? Dropping the reference was all we needed? */
>>>  	if (d_flags == (DCACHE_REFERENCED | DCACHE_LRU_LIST) && !d_unhashed(dentry))
>>> diff --git a/fs/inode.c b/fs/inode.c
>>> index 72c4c347afb7..5218a8aebd7f 100644
>>> --- a/fs/inode.c
>>> +++ b/fs/inode.c
>>> @@ -1632,7 +1632,7 @@ static void iput_final(struct inode *inode)
>>>  	}
>>>  
>>>  	state = inode->i_state;
>>> -	if (!drop) {
>>> +	if (!drop || (drop && (inode->i_state & I_DONTCACHE))) {
>>>  		WRITE_ONCE(inode->i_state, state | I_WILL_FREE);
>>>  		spin_unlock(&inode->i_lock);
>>>  
>>> -- 
>>> 2.28.0
>>>
>>>
>>>
>