On Mon, May 18, 2020 at 7:27 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Mon, May 18, 2020 at 3:53 AM Ian Kent <raven@xxxxxxxxxx> wrote: > > > > On Fri, 2020-05-15 at 15:20 +0800, Chengguang Xu wrote: > > > This series adds a new lookup flag LOOKUP_DONTCACHE_NEGATIVE > > > to indicate to drop negative dentry in slow path of lookup. > > > > > > In overlayfs, negative dentries in upper/lower layers are useless > > > after construction of overlayfs' own dentry, so in order to > > > effectively reclaim those dentries, specify LOOKUP_DONTCACHE_NEGATIVE > > > flag when doing lookup in upper/lower layers. > > > > I've looked at this a couple of times now. > > > > I'm not at all sure of the wisdom of adding a flag to a VFS function > > that allows circumventing what a file system chooses to do. > > But it is not really a conscious choice is it? > How exactly does a filesystem express its desire to cache a negative > dentry? The documentation of lookup() in vfs.rst makes it clear that > it is not up to the filesystem to make that decision. > The VFS needs to cache the negative dentry on lookup(), so > it can turn it positive on create(). > Low level kernel modules that call the VFS lookup() might know > that caching the negative dentry is counter productive. > > > > > I also do really see the need for it because only hashed negative > > dentrys will be retained by the VFS so, if you see a hashed negative > > dentry then you can cause it to be discarded on release of the last > > reference by dropping it. > > > > So what's different here, why is adding an argument to do that drop > > in the VFS itself needed instead of just doing it in overlayfs? > > That was v1 patch. It was dealing with the possible race of > returned negative dentry becoming positive before dropping it > in an intrusive manner. > > In retrospect, I think this race doesn't matter and there is no > harm in dropping a positive dentry in a race obviously caused by > accessing the underlying layer, which as documented results in > "undefined behavior". > > Miklos, am I missing something? Dropping a positive dentry is harmful in case there's a long term reference to the dentry (e.g. an open file) since it will look as if the file was deleted, when in fact it wasn't. It's possible to unhash a negative dentry in a safe way if we make sure it cannot become positive. One way is to grab d_lock and remove it from the hash table only if count is one. So yes, we could have a helper to do that instead of the lookup flag. The disadvantage being that we'd also be dropping negatives that did not enter the cache because of our lookup. I don't really care, both are probably good enough for the overlayfs case. Thanks, Miklos
