On Fri, 11 Jul 2025, Amir Goldstein wrote: > On Fri, Jul 11, 2025 at 1:21 AM NeilBrown <neil@xxxxxxxxxx> wrote: > > > > Normally it is ok to include a lookup with the subsequent operation on > > the result. However in this case ovl_cleanup_and_whiteout() already > > (potentially) creates a whiteout inode so we need separate locking. > > The change itself looks fine and simple, but I didn't understand the text above. > > Can you please explain? Maybe that was really a note to myself - at first glance the change looked a little misguided. While it is possible to perform the lookups outside the directory lock, the take the lock, check the parents, perform the operation, it is generally better to combine the lookup with the lock (hence my proposed lookup_and_lock operations). In the current locking scheme, performing the lookup and the operation under the one lock avoids some races. In my new code we don't avoid the race but the lookup-and-lock can detect the race and repeat the lookup. So in generally we can avoid returning the -EINVAL if the parent check fails. So changing code that did a lookup and rename in the same lock to code which takes the lock twice seems wrong. I wanted to justify it, and the justification is the need to create the whiteout between the lookup and the rename. A different way to do this might be the create the whiteout before doing the lookup_upper. That would require a larger refactoring that probably isn't justified. I've changed it to: =========== This code: performs a lookup_upper created a whiteout object renames the whiteout over the result of the lookup The create and the rename must be locked separated for proposed directory locking changes. This patch takes a first step of moving the lookup out of the locked region. =========== Thanks, NeilBrown > > Thanks, > Amir. > > > > > Signed-off-by: NeilBrown <neil@xxxxxxxxxx> > > --- > > fs/overlayfs/dir.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > > index d01e83f9d800..8580cd5c61e4 100644 > > --- a/fs/overlayfs/dir.c > > +++ b/fs/overlayfs/dir.c > > @@ -769,15 +769,11 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, > > goto out; > > } > > > > - err = ovl_lock_rename_workdir(workdir, NULL, upperdir, NULL); > > - if (err) > > - goto out_dput; > > - > > - upper = ovl_lookup_upper(ofs, dentry->d_name.name, upperdir, > > - dentry->d_name.len); > > + upper = ovl_lookup_upper_unlocked(ofs, dentry->d_name.name, upperdir, > > + dentry->d_name.len); > > err = PTR_ERR(upper); > > if (IS_ERR(upper)) > > - goto out_unlock; > > + goto out_dput; > > > > err = -ESTALE; > > if ((opaquedir && upper != opaquedir) || > > @@ -786,6 +782,10 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, > > goto out_dput_upper; > > } > > > > + err = ovl_lock_rename_workdir(workdir, NULL, upperdir, upper); > > + if (err) > > + goto out_dput_upper; > > + > > err = ovl_cleanup_and_whiteout(ofs, upperdir, upper); > > if (err) > > goto out_d_drop; > > @@ -793,10 +793,9 @@ static int ovl_remove_and_whiteout(struct dentry *dentry, > > ovl_dir_modified(dentry->d_parent, true); > > out_d_drop: > > d_drop(dentry); > > + unlock_rename(workdir, upperdir); > > out_dput_upper: > > dput(upper); > > -out_unlock: > > - unlock_rename(workdir, upperdir); > > out_dput: > > dput(opaquedir); > > out: > > -- > > 2.49.0 > > >
