Re: [PATCH v2 10/17] ovl: decode lower file handles of unlinked but open files

[Miklos Szeredi <miklos@szeredi.hu>]

On Tue, Jan 16, 2018 at 10:37 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Jan 16, 2018 at 11:16 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Thu, Jan 4, 2018 at 6:20 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> Lookup overlay inode in cache by origin inode, so we can decode a file
>>> handle of an open file even if the index has a whiteout index entry to
>>> mark this overlay inode was unlinked.
>>>
>>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>>> ---
>>>  fs/overlayfs/export.c    | 22 ++++++++++++++++++++--
>>>  fs/overlayfs/inode.c     | 16 ++++++++++++++++
>>>  fs/overlayfs/overlayfs.h |  1 +
>>>  3 files changed, 37 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
>>> index 602bada474ba..6ecb54d4b52c 100644
>>> --- a/fs/overlayfs/export.c
>>> +++ b/fs/overlayfs/export.c
>>> @@ -385,13 +385,21 @@ 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;
>>>         int err;
>>>
>>>         /* First lookup indexed upper by fh */
>>
>> Why not first look up origin, then look up ovl inode by origin?  It
>> seems a faster path than going through the index first.  Obviously if
>> icache lookup fails then we need to look up index, but the common case
>> will the cached one, so that should be the fast one, no?
>>
>
> Not really, because we do not know if the file handle is dir or non-dir.
> If file handle is dir than decode of file handle is expensive and can
> reduce worst case from two file handle decodes to just one:
>
> For lower dir:
> - one index lookup fails
> - one lower dir decode
> - one icache lookup
> - maybe one ovl_lookup_real(is_upper=false)
>
> For copied up indexed dir:
> - one index lookup success
> - one upper dir decode
> - one ovl_lookup_real(is_upper=true)
>
> That method avoids the origin dir decode for upper indexed
> dir at the cost of not looking for the decoded dir in icache.
>
> How about this as in idea: hash overlay inodes for NFS export
> by origin fh instead of by origin inode pointer.

Good idea.  That way we can leave out the middleman (underlying fh
decode) in the cached case.

> We can also avoid the "lookup index first" for non-dir
> if we set a flag OVL_FH_FLAG_CONNECTABLE on exported
> dir file handle, but my thinking was trying to keep the first version
> simple with as fewer special cases as possible.

Not sure I understand.  If cached lookup fails, then we do always need
to try and lookup index first before falling back to decoding origin,
right?

Thanks,
Miklos




>
> Thanks,
> Amir.