On Thu, Mar 22, 2018 at 1:55 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> On Thu, Mar 22, 2018 at 12:41 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>> On Sun, Mar 11, 2018 at 12:22 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>>> When decoding a lower file handle, we need to check if lower file was
>>> copied up and indexed and if it has a whiteout index, we need to check
>>> if this is an unlinked but open non-dir before returning -ESTALE.
>>>
>>> To find out if this is an unlinked but open non-dir we need to lookup
>>> an overlay inode in inode cache by lower inode and that requires decoding
>>> the lower file handle before looking in inode cache.
>>>
>>> Before this change, if the lower inode turned out to be a directory, we
>>> may have paid an expensive cost to reconnect that lower directory for
>>> nothing.
>>>
>>> After this change, we start by decoding a disconnected lower dentry and
>>> using the lower inode for looking up an overlay inode in inode cache.
>>> If we find overlay inode and dentry in cache, we avoid the index lookup
>>> overhead. If we don't find an overlay inode and dentry in cache, then we
>>> only need to decode a connected lower dentry in case the lower dentry is
>>> a non-indexed directory.
>>>
>>> The xfstests group overlay/exportfs tests decoding overlayfs file
>>> handles after drop_caches with different states of the file at encode
>>> and decode time. Overall the tests in the group call ovl_lower_fh_to_d()
>>> 89 times to decode a lower file handle.
>>>
>>> Before this change, the tests called ovl_get_index_fh() 75 times and
>>> reconnect_one() 61 times.
>>> After this change, the tests call ovl_get_index_fh() 70 times and
>>> reconnect_one() 59 times. The 2 cases where reconnect_one() was avoided
>>> are cases where a non-upper directory file handle was encoded, then the
>>> directory removed and then file handle was decoded.
>>>
>>> To demonstrate the affect on decoding file handles with hot inode/dentry
>>> cache, the drop_caches call in the tests was disabled. Without
>>> drop_caches, there are no reconnect_one() calls at all before or after
>>> the change. Before the change, there are 75 calls to ovl_get_index_fh(),
>>> exactly as the case with drop_caches. After the change, there are only
>>> 10 calls to ovl_get_index_fh().
>>>
>>> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>>> ---
>>> fs/overlayfs/export.c | 58 +++++++++++++++++++++++++++++----------------------
>>> 1 file changed, 33 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
>>> index 15411350a7a1..a908da8b63c1 100644
>>> --- a/fs/overlayfs/export.c
>>> +++ b/fs/overlayfs/export.c
>>> @@ -703,25 +703,39 @@ static struct dentry *ovl_lower_fh_to_d(struct super_block *sb,
>>> struct ovl_path *stack = &origin;
>>> struct dentry *dentry = NULL;
>>> struct dentry *index = NULL;
>>> - struct inode *inode = NULL;
>>> - bool is_deleted = false;
>>> + struct inode *inode;
>>> int err;
>>>
>>> - /* First lookup indexed upper by fh */
>>> + /* First lookup overlay inode in inode cache by origin fh */
>>> + err = ovl_check_origin_fh(ofs, fh, false, NULL, &stack);
>>> + if (err)
>>> + return ERR_PTR(err);
>>> +
>>> + if (d_is_dir(origin.dentry) ||
>>> + !(origin.dentry->d_flags & DCACHE_DISCONNECTED)) {
>>
>> I don't understand the above test.
>>
>> If origin is a connected directory, then we have a chance of being
>> able to lookup the overlay inode in the icache.
>>
>> If origin is a disconnected directory, then we don't have a chance,
>> because if overlay dentry was cached, it would hold a ref to the
>> connected origin.
>>
>> If origin is a non-dir, then we just don't care about being disconnected or not.
>>
>> So test should be
>>
>> if (!d_is_dir(origin.dentry) || !(origin.dentry->d_flags &
>> DCACHE_DISCONNECTED))
>>
>> What am I missing?
>
> Probably my mistake.
> I will look into it.
>
Yes, it is a mistake as you observed.
I was trying to understand why it didn't affect the tests.
Apparently, the case where underlying dentry is disconnected
and overlay inode is in cache is harder to reproduce and is not covered by
the current tests. I think it requires:
- encoding non-upper non-dir overlay file handle
- drop caches
- open by non-upper file handle (disconnected underlying and not in
overlay icache)
- open by non-upper file handle again (disconnected underlying and in
overlay icache)
The current tests only cover:
- encoding non-upper non-dir overlay file handle
- drop caches
- open by non-upper file handle (disconnected underlying and not in
overlay icache)
AND:
- encoding non-upper non-dir overlay file handle AND keep the file open
- drop caches
- open by non-upper file handle (connected underlying and in overlay icache)
I will add the missing tests to my TODO.
I suppose you will be fixing this bug on commit?
Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
