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 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?

At the very least it's a bisect hazard...