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

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

On Mon, Oct 07, 2024 at 04:19:21PM +0200, Amir Goldstein wrote:
> +static int ovl_upper_fdget(const struct file *file, struct fd *real, bool data)
> +{
> +	struct dentry *dentry = file_dentry(file);
> +	struct path realpath;
> +	enum ovl_path_type type;
> +
> +	if (data)
> +		type = ovl_path_realdata(dentry, &realpath);
> +	else
> +		type = ovl_path_real(dentry, &realpath);
> +
> +	real->word = 0;
> +	/* Not interested in lower nor in upper meta if data was requested */
> +	if (!OVL_TYPE_UPPER(type) || (data && OVL_TYPE_MERGE(type)))
> +		return 0;
> +
> +	return ovl_real_fdget_path(file, real, &realpath);
>  }
>  
>  static int ovl_open(struct inode *inode, struct file *file)
> @@ -394,16 +411,14 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>  	if (ret <= 0)
>  		return ret;
>  
> -	ret = ovl_real_fdget_meta(file, &real, !datasync);
> -	if (ret)
> +	/* Don't sync lower file for fear of receiving EROFS error */
> +	ret = ovl_upper_fdget(file, &real, datasync);
> +	if (ret || fd_empty(real))
>  		return ret;

Is there any real point in keeping ovl_upper_fdget() separate from the
only caller?  Note that the checks for type make a lot more sense
in ovl_fsync() than buried in a separate helper and this fd_empty()
is a "do we have the wrong type?" check in disguise.

Why not just expand it at the call site?