On Sun, Nov 19, 2023 at 08:57:25AM +0200, Amir Goldstein wrote:
> On Sat, Nov 18, 2023 at 10:02 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Sun, Nov 12, 2023 at 09:26:28AM +0200, Amir Goldstein wrote:
> >
> > > Tested the patch below.
> > > If you want to apply it as part of dcache cleanup, it's fine by me.
> > > Otherwise, I will queue it for the next overlayfs update.
> >
> > OK... Let's do it that way - overlayfs part goes into never-rebased branch
> > (no matter which tree), pulled into dcache series and into your overlayfs
> > update, with removal of unused stuff done in a separate patch in dcache
> > series.
> >
>
> Sounds good.
>
> > That way we won't step on each other's toes when reordering, etc.
> > Does that work for you? I can put the overlayfs part into #no-rebase-overlayfs
> > in vfs.git, or you could do it in a v6.7-rc1-based branch in your tree -
> > whatever's more convenient for you.
>
> I've reset overlayfs-next to no-rebase-overlayfs, as it had my version
> with removal so far.
>
> For the final update, I doubt I will need to include it at all, because
> the chances of ovl_obtain_alias() colliding with anything for the next
> cycle are pretty slim, but it's good that I have the option and I will
> anyway make sure to always test the next update with this change.
OK... Several overlayfs locking questions:
ovl_indexdir_cleanup()
{
...
inode_lock_nested(dir, I_MUTEX_PARENT);
...
index = ovl_lookup_upper(ofs, p->name, indexdir, p->len);
...
err = ovl_cleanup_and_whiteout(ofs, dir, index);
with ovl_cleanup_and_whiteout() moving stuff between workdir and parent of index.
Where do you do lock_rename()? It's a cross-directory rename, so it *must*
lock both (and take ->s_vfs_rename_mutex as well). How can that possibly
work?
Similar in ovl_cleanup_index() - you lock indexdir, then call
ovl_cleanup_and_whiteout(), with the same locking issues.
Another fun question: ovl_copy_up_one() has
if (parent) {
ovl_path_upper(parent, &parentpath);
ctx.destdir = parentpath.dentry;
ctx.destname = dentry->d_name;
err = vfs_getattr(&parentpath, &ctx.pstat,
STATX_ATIME | STATX_MTIME,
AT_STATX_SYNC_AS_STAT);
if (err)
return err;
}
What stabilizes dentry->d_name here? I might be missing something about the
locking environment here, so it might be OK, but...
