Re: [PATCH v2 03/17] ovl: decode pure upper file handles

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Jan 24, 2018 at 12:34 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Fri, Jan 19, 2018 at 9:10 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Fri, Jan 19, 2018 at 1:07 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On Fri, Jan 19, 2018 at 12:39 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>> On Fri, Jan 19, 2018 at 02:23:35AM +0200, Amir Goldstein wrote:
>>>>> > How is this for an option?
>>>> [...]
>>>>> > +struct dentry *d_obtain_alias_fsdata(struct inode *inode, void **fsdata)
>>>>> > +{
>>>>> > +       return __d_obtain_alias(inode, 1, fsdata);
>>>>> >  }
>>>>> >  EXPORT_SYMBOL(d_obtain_alias);
>>>>
>>>> It would work, but I like this interface better:
>>>>
>>>> +extern struct dentry * d_alloc_anon(struct super_block *);
>>>> +extern struct dentry * d_instantiate_anon(struct dentry *, struct inode *);
>>>>
>>>
>>> OK. Thanks for the patch!
>>>
>>
>> Added your dcache patch to the series and reworked my patches
>> to use the new helpers.
>>
>> Tested result is pushed to:
>> https://github.com/amir73il/linux/commits/ovl-nfs-export-v3
>>
>> Prep patches changes since v2:
>> - Rebased over fix patch "hash all directory inodes for fsnotify"
>> - Rename mount/config option from "verify" to "nfs_export"
>> - Force r/o mount when index dir creation fails
>> - Allow enabling "nfs_export" for non-upper mount
>> - Require "redirect_dir=nofollow" for non-upper mount
>> - Rename dir index entries xattr from ".origin" to ".upper"
>> - Re-factor ovl_{get|set|verify}_origin() helpers
>> - Simplify test for temp index name (starts with #)
>> - Abandon ovl_dentry_is_renamed() test for lower st_ino
>> - Document overhead on mount with full index
>> - Document change of behavior when verifying lower origin
>> - Added patch to make room in ovl_entry struct
>>
>> NFS export changes since v2:
>> - Fix exportfs ops for r/o overlay with no upperdir
>> - Document reason for copy up directory on encode
>> - Take care of racing with rename while connecting dir
>> - Explain the reasons for choosing the 'connected' dir approach
>> - Do not add dentry without ovl_entry to dcache
>>
>> Optimizations TODO:
>> - Copy up on encode only when lower ancestor is below middle layer redirect
>> - Hash inode by fh to avoid origin decode of whiteout fh
>>
>> As far as I know, the series is now functionally correct and all comments
>> so far addressed. The remaining optimizations will be done on top of this
>> series.
>
> Pushed to overlayfs-next with one fix (do not warn about falling back
> to nfs_export=off if nfs_export is already off).

Good fix.
That warning was a late addition to V3 after allowing nfs_export without
index for on-upper r/o mount.

>
> That spurious warning makes me wonder: how much of the option matrix is tested?
>

Good question... (adding Eryu in CC)

I know Eryu tested with default kernel configuration, because one
of my tests found a bug with NFS export and redirect_dir=off.
(I fixed the specific test to require redirect_dir)

I have posted xfstests for overlay NFS export support with samefs
and non-samefs configuration and for non-samefs, there are two
lower layers not on the same fs. I also have posted a test for non-upper
overlay with two lower layers on samefs and non-samefs.

One of the overlay xfstests explicitly turns off index with index=off,
(overlay/036), so we have coverage for falling back to nfs_export=off
when NFS export is enabled by default (as it usually is in my setup).

There is an overlay xfstest with multiple lower layers and no-upper
and that test shows the warning about falling back to nfs_export=off
due to redirect_dir=nofollow requirement (with default kernel configuration).

I did not specifically test underlying fs with no xattr support and no
file handle support, though the latter was already tested for index
with squashfs (no uuid) and the nfs_export=off code is piggy backed
in the same place as index=off.

Thanks,
Amir.