On Wed, Apr 21, 2021 at 12:23 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > Eryu, > > This extends the generic t_dir_offset2 test to verify > some more test cases and adds a new generic test which > passes on overlayfs (and other fs) on upstream kernel. > > The overlayfs specific test fails on upstream kernel > and the fix commit is currently in linux-next. > As usual, you may want to wait with merging until the fix > commit hits upstream. > > Miklos, > > I had noticed in the test full logs that readdir of > a merged dir behaves strangely - when seeking backwards > to offsets > 0, readdir returns unlinked entries in results. > The test does not fail on that behavior because the test > only asserts that this is not allowed after seek to offset 0. > > Knowing the implementation of overlayfs readdir cache this is > not surprising to me, but I wonder if this behavior is POSIX > compliant, and if not, whether we should document it and/or > add a failing test for it. > I think the outcome could be worse. If a copied up entry is unlinked after populating the dir cache but before ovl_cache_update_ino() then ovl_cache_update_ino() and subsequently the getdents call will fail with ENOENT. This test is not smart enough to cover this case (if it really exists). I think we need to relax the case of negative lookup result in ovl_cache_update_ino() and just set p->whiteout without and continue with no error. This can solve several test cases. In principle, we can "semi-reset" the merge dir cache if the dir was modified after every seek, not just seek to 0. By "semi-reset" I mean use the list, but force ovl_cache_update_ino() to all upper entries, similar to ovl_dir_read_impure(). OR.. we can just do that unconditionally in ovl_iterate(). The ovl dentry cache for the children will be populated after the first ovl_iterate() anyway, so maybe the penalty is not so bad? Thanks, Amir.
