On Mon, Sep 8, 2025 at 4:07 AM NeilBrown <neilb@xxxxxxxxxxx> wrote:
>
> On Sat, 06 Sep 2025, Amir Goldstein wrote:
> > On Sat, Sep 6, 2025 at 7:00 AM NeilBrown <neilb@xxxxxxxxxxx> wrote:
> > >
> > > From: NeilBrown <neil@xxxxxxxxxx>
> > >
> > > ovl wants a lookup which won't block on a fatal signal.
> > > It currently uses down_write_killable() and then repeated
> > > calls to lookup_one()
> > >
> > > The lock may not be needed if the name is already in the dcache and it
> > > aid proposed future changes if the locking is kept internal to namei.c
> > >
> > > So this patch adds lookup_one_positive_killable() which is like
> > > lookup_one_positive() but will abort in the face of a fatal signal.
> > > overlayfs is changed to use this.
> > >
> > > Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> >
> > I think the commit should mention that this changes from
> > inode_lock_killable() to inode_lock_shared_killable() on the
> > underlying dir inode which is a good thing for this scope.
> >
> > BTW I was reading the git history that led to down_write_killable()
> > in this code and I had noticed that commit 3e32715496707
> > ("vfs: get rid of old '->iterate' directory operation") has made
> > the ovl directory iteration non-killable when promoting the read
> > lock on the ovl directory to write lock.
>
> hmmmm....
>
> So the reason that this uses a killable lock is simply because it used
> to happen under readdir and readdir uses a killable lock. Is that
> right?
I think the semantics were copied from readdir of that moment yes.
>
> So there is no particularly reason that "killable" is important here?
I can think of some reasons -
Maybe overlayfs (ever user mounted overlayfs) has just one process
accessing it but underlying lower layer is remote fs with many processes
accessing it so chances of lower layer dir lock being held by another thread
are much higher than chances of overlayfs dir lock being held.
> So I could simply change it to use lookup_one_positive() and you
> wouldn't mind?
>
I do mind and prefer that you keep this killable as you patch does.
The more important reason to keep this killable IMO is that we can and
should make overlayfs readdir shared lock one day.
> I'd actually like to make all directory/dentry locking killable - I
> don't think there is any downside. But I don't want to try pushing that
> until my current exercise is finished.
>
The path to making overlayfs readdir shared and killable is
to move the synchronization of ovl readdir cache and
OVL_I(inode)->version from the implicit vfs inode_lock() to
explicit ovl_inode_lock().
The mechanical change is easy.
My concern is from hidden assumptions in the code that
I am not aware of, ones which are not annotated with
inode_is_locked() like ovl_inode_version_get() and
ovl_dir_version_inc() are.
And the fact that noone has yet to complain about overlayfs readdir
scalability makes this conversion non urgent.
If you have other reasons to want to make ovl readdir killable
or shared, we can look into that.
Thanks,
Amir.
