On Mon, Oct 7, 2024 at 4:39 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > > 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? You are right (again) I open code it, it looks much better: /* Don't sync lower file for fear of receiving EROFS error */ - upperfile = ovl_upper_file(file, datasync); - if (IS_ERR_OR_NULL(upperfile)) + type = ovl_path_type(dentry); + if (!OVL_TYPE_UPPER(type) || (datasync && OVL_TYPE_MERGE(type))) + return 0; + + ovl_path_upper(dentry, &upperpath); + upperfile = ovl_real_file_path(file, &upperpath); + if (IS_ERR(upperfile)) return PTR_ERR(upperfile); Thanks, Amir.