linux-unionfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Miklos Szeredi <miklos@szeredi.hu>
To: Amir Goldstein <amir73il@gmail.com>
Cc: overlayfs <linux-unionfs@vger.kernel.org>,
	fstests <fstests@vger.kernel.org>, Eryu Guan <guaneryu@gmail.com>
Subject: Re: [PATCH 0/2] Test overlayfs readdir cache
Date: Thu, 22 Apr 2021 09:53:08 +0200	[thread overview]
Message-ID: <CAJfpegtoTJRnNQnttVw54pndEqrpzfxttp=NCQ_2za859fWMqA@mail.gmail.com> (raw)
In-Reply-To: <CAOQ4uxhh305WPZ-puLONej2TLQTe54-FUtrsgp2R8ohdDcNP0A@mail.gmail.com>

On Thu, Apr 22, 2021 at 8:18 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Apr 21, 2021 at 12:23 PM Amir Goldstein <amir73il@gmail.com> 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?

POSIX does allow stale readdir data (not just in case of non-zero seek):

"If a file is removed from or added to the directory after the most
recent call to opendir() or rewinddir(), whether a subsequent call to
readdir() returns an entry for that file is unspecified."

If you think about the way readdir(3) is implemented by the libc, this
is inevitable.

Returning ENOENT from readdir(3) is obviously a bug.

The merge case being not super high performance is perfectly okay.
The only thing I've worried about in that case is unbound memory use,
but apparently that hasn't been an issue in practice.

Thanks,
Miklos

  reply	other threads:[~2021-04-22  7:53 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-21  9:23 [PATCH 0/2] Test overlayfs readdir cache Amir Goldstein
2021-04-21  9:23 ` [PATCH 1/2] generic: Test readdir of modified directrory Amir Goldstein
2021-04-21  9:23 ` [PATCH 2/2] overlay: Test invalidate of readdir cache Amir Goldstein
2021-04-21  9:33   ` Amir Goldstein
2021-04-22  6:23     ` Amir Goldstein
2021-04-22  6:18 ` [PATCH 0/2] Test overlayfs " Amir Goldstein
2021-04-22  7:53   ` Miklos Szeredi [this message]
2021-04-22  8:47     ` Amir Goldstein
2021-04-22  9:03       ` Miklos Szeredi
2021-04-23 10:20     ` Amir Goldstein
2021-04-23 19:03     ` Amir Goldstein

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAJfpegtoTJRnNQnttVw54pndEqrpzfxttp=NCQ_2za859fWMqA@mail.gmail.com' \
    --to=miklos@szeredi.hu \
    --cc=amir73il@gmail.com \
    --cc=fstests@vger.kernel.org \
    --cc=guaneryu@gmail.com \
    --cc=linux-unionfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).