Re: [PATCH v2 05/11] ovl: lookup redirect by file handle

From: Miklos Szeredi <miklos@szeredi.hu>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Vivek Goyal <vgoyal@redhat.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	"linux-unionfs@vger.kernel.org" <linux-unionfs@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH v2 05/11] ovl: lookup redirect by file handle
Date: Wed, 26 Apr 2017 14:15:11 +0200	[thread overview]
Message-ID: <CAJfpegtv4kN4d7ti5f5p=51aQk8U2UT9xdsvZkYoXWOfQ5BNcQ@mail.gmail.com> (raw)
In-Reply-To: <CAOQ4uxiozume+xj4KHiDTwqgWmmXKL+6Ksw1EDWXuZYDFa2KVQ@mail.gmail.com>

On Wed, Apr 26, 2017 at 12:17 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Apr 26, 2017 at 12:55 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Wed, Apr 26, 2017 at 11:40 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>
>>> Just to see that I understand you correctly.
>>>
>>> I am now working on storing the following:
>>>
>>> /*
>>>  * The tuple origin.{fh,layer,uuid} is a universal unique identifier
>>>  * for a copy up origin, where:
>>>  * origin.fh    - exported file handle of the lower file
>>>  * origin.root - exported file handle of the lower layer root
>>>  * origin.uuid  - uuid of the lower filesystem
>>
>> I wouldn't even store origin.root.
>>
>>>  *
>>>  * origin.{fh,root} are stored in format of a variable length binary blob
>>>  * with struct ovl_fh header (total blob size up to 20 bytes).
>>>  * uuid is stored in raw format (16 bytes) as published by sb->s_uuid.
>>>  */
>>>
>>> I intend to implement lookup as follows:
>>> - compare(origin.uuid, same_lower_sb->s_uuid)
>>> # layer root dentries cannot be DCACHE_DISCONNECTED, so
>>> # exportfs_decode_fh ignores mnt arg and returns the cached dentry
>>> - root = exportfs_decode_fh(lowerstack[0].mnt, origin.root)
>>> - find layer where lowerstack[layer].dentry == root
>>> - this = exportfs_decode_fh(lowerstack[layer].mnt, origin.fh)
>>>
>>> is_subdir() is NOT needed for decoding the layer root
>>> is_subdir() is optional for decoding the lower file, because
>>> it is not needed to identify the layer
>>
>> Hmm, we can just force exportfs_decode_fh() to return a connected
>> dentry (return false from *acceptable() if the dentry is disconnected)
>> before going on to iterate the layers to see which one contains it.
>>
>
> Hmm, this might work, but to quote from exportfs_decode_fh():
> "It's not a directory.  Life is a little more complicated."
>
> IIUC, 'connected' means 'connected to sb root', and not
> 'connected to mnt root', so in the optimal case where
> all lower dentries are cached,  exportfs_decode_fh() will return
> a connected dentry for every fh we give it regardless of the
> mnt argument, so we will have to use is_subdir() to find the
> right layer, which brings us back to O(numlower*depth)

It just means that we might have to make up an artificial mount which
has its root at the sb root to be able to decode the handle into a
connected one.

>
> With the extra cost of storing the deducible information origin.root,
> we will have less complex and more efficient lookup code.
>
> Let me try and implement it and see if I am right.
> We can always discard origin.root from v4 if it turns
> out to be unhelpful.

I don't have good feelings about storing the root fh just because we
don't special case the layer root anywhere yet, and I wouldn't want to
do that unless there's a good reason.

Thanks,
Miklos