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

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Jan 24, 2018 at 1:18 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Jan 24, 2018 at 1:04 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> 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.
>>

Correction. no file handle support is covered by nested overlay test
(overlay/029), so I do see those "falling back" warnings as well with v3.
(I did not see the warnings in all my tests because I sometimes apply an
 extra patch to support NFS export on nested overlay).

>
> Clarification: it may be implied when I wrote that "we have test coverage..."
> that there are automatic tests to verify falling back to nfs_export=off.
> This is not the case. I was only listing "falling back" cases whose warning
> I regularly see in my routine overlay/quick xfstest as expected.
>
> Thanks,
> Amir.