On Sat, Oct 5, 2024 at 3:35 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Oct 04, 2024 at 11:28:11PM +0100, Al Viro wrote:
> > > /*
> > > * 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().
> >
Correct. I had considered renaming the argument to allow_empty_upper_meta
but I don't think that will make the contract a lot better.
The thing is that ovl_fsync() caller is really different in two
different aspects:
1. It wants only upper and therefore fd_empty() is a possible outcome
2. It (may) want the metadata inode (when data is still in lower inode)
Overlayfs can have up to 3 different inodes in the stack for a regular file:
lower_data+lower_metadata+upper_metdata
There is currently no file operation that requires opening the lower_metadata
inode and therefore, staching one backing file in ->private_data and another
optional backing file chained from the first one is enough.
If there is ever a file operation that needs to open a realfile from
lower_metadata (only ioctl comes to mind), we may need to reevaluate.
> > I still don't like the calling conventions, TBH. Let me think a bit...
>
I understand your concern, but honestly, I am not sure that returning
struct fderr is fundamentally different from checking IS_ERR_OR_NULL.
What I can do is refactor the helpers differently so that ovl_fsync() will
call ovl_file_upper() to clarify that it may return NULL, just like
ovl_{dentry,inode,path}_upper() and all the other callers will
call ovl_file_real() which cannot return NULL, because it returns
either lower or upper file, just like ovl_{inode,path}_real{,data}().
> Sorry, I'm afraid I'll have to leave that until tomorrow - over 38C after the sodding
> shingles shot really screws the ability to dig through the code ;-/ My apologies...
Ouch! feel well soon.
Thanks,
Amir.
