[PATCH] ovl: support stacked SEEK_HOLE/SEEK_DATA

[PATCH] ovl: support stacked SEEK_HOLE/SEEK_DATA

[Amir Goldstein]

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@gmail.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

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;
+
+	old_cred = ovl_override_creds(file_inode(file)->i_sb);
+	ret = vfs_llseek(real.file, offset, whence);
+	revert_creds(old_cred);
+
+	file->f_pos = real.file->f_pos;
+
+	fdput(real);
+
+	return ret;
 }
 
 static void ovl_file_accessed(struct file *file)
-- 
2.17.1

Re: [PATCH] ovl: support stacked SEEK_HOLE/SEEK_DATA

[Miklos Szeredi]

On Tue, Feb 26, 2019 at 4:27 PM Amir Goldstein <amir73il@gmail.com> 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@gmail.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>
> 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.

Thanks,
Miklos


> +
> +       old_cred = ovl_override_creds(file_inode(file)->i_sb);
> +       ret = vfs_llseek(real.file, offset, whence);
> +       revert_creds(old_cred);
> +
> +       file->f_pos = real.file->f_pos;
> +
> +       fdput(real);
> +
> +       return ret;
>  }
>
>  static void ovl_file_accessed(struct file *file)
> --
> 2.17.1
>

Re: [PATCH] ovl: support stacked SEEK_HOLE/SEEK_DATA

[Amir Goldstein]

On Tue, Feb 26, 2019 at 6:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, Feb 26, 2019 at 4:27 PM Amir Goldstein <amir73il@gmail.com> 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@gmail.com>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >
> > 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).

Also, I see that we need to use vfs_setpos() to copy f_pos back from
real file in order to reset file->f_version.

Thanks,
Amir.
> Thanks,
> Miklos
>
>
> > +
> > +       old_cred = ovl_override_creds(file_inode(file)->i_sb);
> > +       ret = vfs_llseek(real.file, offset, whence);
> > +       revert_creds(old_cred);
> > +
> > +       file->f_pos = real.file->f_pos;
> > +
> > +       fdput(real);
> > +
> > +       return ret;
> >  }
> >
> >  static void ovl_file_accessed(struct file *file)
> > --
> > 2.17.1
> >

Re: [PATCH] ovl: support stacked SEEK_HOLE/SEEK_DATA

[Miklos Szeredi]

On Tue, Feb 26, 2019 at 6:20 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Feb 26, 2019 at 6:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, Feb 26, 2019 at 4:27 PM Amir Goldstein <amir73il@gmail.com> 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@gmail.com>
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >
> > > 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

Re: [PATCH] ovl: support stacked SEEK_HOLE/SEEK_DATA

[Amir Goldstein]

On Tue, Feb 26, 2019 at 10:32 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, Feb 26, 2019 at 6:20 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Feb 26, 2019 at 6:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Tue, Feb 26, 2019 at 4:27 PM Amir Goldstein <amir73il@gmail.com> 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@gmail.com>
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
> > > >
> > > > 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.
>

Just to be clear, so exclusive inode lock and recursive for any type of seek?
To be on the safe side?

Thanks,
Amir.

Re: [PATCH] ovl: support stacked SEEK_HOLE/SEEK_DATA

[Miklos Szeredi]

On Tue, Feb 26, 2019 at 10:16 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Feb 26, 2019 at 10:32 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, Feb 26, 2019 at 6:20 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, Feb 26, 2019 at 6:07 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Tue, Feb 26, 2019 at 4:27 PM Amir Goldstein <amir73il@gmail.com> 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@gmail.com>
> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > ---
> > > > >
> > > > > 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.
> >
>
> Just to be clear, so exclusive inode lock and recursive for any type of seek?
> To be on the safe side?

Yes.  As a trivial optimization SEEK_CUR/0 (return current pos) could
be done without messing with underlying fs.  Probably same for
SEEK_SET/0.

Looking ahead, inode lock is definitely excessive, since we are not
doing anything to the inode here, so ideally this would be a per-file
mutex, but we've nowhere to put such a thing at the moment.

Thanks,
Miklos

Re: [PATCH] ovl: support stacked SEEK_HOLE/SEEK_DATA

[Eddie Horng]

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

Hi Amir,
Thanks your detail information and the patch.
I ran into this issue by using a proprietary stacked file system as lower
and the file system does not initial it's s_maxbytes, therefore
caused file larger than 2GB fails lseek. A quick workaround is to set the fs
s_maxbytes to MAX_LFS_FILESIZE, but like you said, for certain cases this might
be a problem.
I have verified this patch with same configuration and the lower inode
f_op->llseek
does get invoked, looks like enough to resolve my particular case.

thanks,
Eddie