Re: [PATCH 1/4] ovl: do not open non-data lower file for fsync

[Al Viro <viro@zeniv.linux.org.uk>]

On Fri, Oct 04, 2024 at 11:16:25PM +0100, Al Viro wrote:
> On Fri, Oct 04, 2024 at 12:23:39PM +0200, Amir Goldstein wrote:
> > ovl_fsync() with !datasync opens a backing file from the top most dentry
> > in the stack, checks if this dentry is non-upper and skips the fsync.
> > 
> > In case of an overlay dentry stack with lower data and lower metadata
> > above it, but without an upper metadata above it, the backing file is
> > opened from the top most lower metadata dentry and never used.
> > 
> > Fix the helper ovl_real_fdget_meta() to return an empty struct fd in
> > that case to avoid the unneeded backing file open.
> 
> Umm...  Won't that screw the callers of ovl_real_fd()?
> 
> I mean, here
> 
> > @@ -395,7 +397,7 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> >  		return ret;
> >  
> >  	ret = ovl_real_fdget_meta(file, &real, !datasync);
> > -	if (ret)
> > +	if (ret || fd_empty(real))
> >  		return ret;
> >  
> 
> you are taking account of that, but what of e.g.
>         ret = ovl_real_fdget(file, &real);
>         if (ret)
>                 return ret;
> 
>         /*
>          * Overlay file f_pos is the master copy that is preserved
>          * through copy up and modified on read/write, but only real
>          * fs knows how to SEEK_HOLE/SEEK_DATA and real fs may impose
>          * limitations that are more strict than ->s_maxbytes for specific
>          * files, so we use the real file to perform seeks.
>          */
>         ovl_inode_lock(inode);
>         fd_file(real)->f_pos = file->f_pos;
> in ovl_llseek()?  Get ovl_real_fdget_meta() called by ovl_real_fdget() and
> have it return 0 with NULL in fd_file(real), and you've got an oops right
> there, don't you?

I see... so you rely upon that thing never happening when the last argument of
ovl_real_fdget_meta() is false, including the call from ovl_real_fdget().

I still don't like the calling conventions, TBH.  Let me think a bit...