On Tue, Feb 26, 2019 at 6:20 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Tue, Feb 26, 2019 at 6:07 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> >
> > On Tue, Feb 26, 2019 at 4:27 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > >
> > > Overlay file f_pos is the master copy that is preserved
> > > through copy up, but only real fs knows how to SEEK_HOLE/
> > > SEEK_DATA. So we copy f_pos from overlay file, perform the seek
> > > on real file and copy f_pos back to overlay file.
> > >
> > > Fixes: d1d04ef8572b ("ovl: stack file ops")
> > > Reported-by: Eddie Horng <eddiehorng.tw@xxxxxxxxx>
> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > ---
> > >
> > > Miklos,
> > >
> > > I verified no regressions with xfstests quick tests.
> > > The improved generic/seek tests that I posted to fstests
> > > are failing on master and passing with this fix.
> > >
> > > Thanks,
> > > Amir.
> > >
> > > fs/overlayfs/file.c | 27 +++++++++++++++++++++++----
> > > 1 file changed, 23 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > > index 84dd957efa24..0d472940ce9e 100644
> > > --- a/fs/overlayfs/file.c
> > > +++ b/fs/overlayfs/file.c
> > > @@ -145,11 +145,30 @@ static int ovl_release(struct inode *inode, struct file *file)
> > >
> > > static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
> > > {
> > > - struct inode *realinode = ovl_inode_real(file_inode(file));
> > > + struct fd real;
> > > + const struct cred *old_cred;
> > > + ssize_t ret;
> > >
> > > - return generic_file_llseek_size(file, offset, whence,
> > > - realinode->i_sb->s_maxbytes,
> > > - i_size_read(realinode));
> > > + ret = ovl_real_fdget(file, &real);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /*
> > > + * Overlay file f_pos is the master copy that is preserved
> > > + * through copy up, but only real fs knows how to SEEK_HOLE/
> > > + * SEEK_DATA.
> > > + */
> > > + real.file->f_pos = file->f_pos;
> >
> > Parallel invocations of ovl_llseek would mean trouble. I suggest
> > doing generic_file_llseek_size for SET/CUR/END and doing the recursive
> > one under ovl inode lock for HOLE/DATA.
> >
>
> OK. What about Eddie's question:
> If realinode is nested overlay inode, then realinode->i_sb->s_maxbytes
> will not reflect the "real" inode's maxbytes.
> Not to mention other crazy things that filesystems may do for SET/CUR/END.
> Take ext4_llseek() for example, for the less common case (ext3 format inode)
> the value of inode->i_sb->s_maxbytes is not used as maxbytes, but a smaller
> value.
>
> Would it be worse or better than exclusive lock if we allocate a new
> realfile instance for every call to ovl_llseek? (as we do anyway for the
> transient state of ro fd of file that was copied up).
Hell knows. Simplest is definitely the exclusive inode lock. Then we
can look at optimizing that... I don't think lseek would be
particularly performance sensitive: those would be using the
pread/pwrite interfaces anyway.
Thanks,
Miklos
