cifs: add support for fallocate mode 0 for non-sparse files
diff mbox series

Message ID 20200115012321.6780-2-lsahlber@redhat.com
State New
Headers show
Series
  • add support for fallocate mode 0
Related show

Commit Message

Ronnie Sahlberg Jan. 15, 2020, 1:23 a.m. UTC
RHBZ 1336264

Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/smb2ops.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Steve French Jan. 15, 2020, 1:25 a.m. UTC | #1
Does it affect (or enable) any xfstests?

On Tue, Jan 14, 2020 at 7:23 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>
> RHBZ 1336264
>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/smb2ops.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 6250370c1170..91818f7c1b9c 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -3106,9 +3106,13 @@ static long smb3_simple_falloc(struct file *file, struct cifs_tcon *tcon,
>                 else if (i_size_read(inode) >= off + len)
>                         /* not extending file and already not sparse */
>                         rc = 0;
> -               /* BB: in future add else clause to extend file */
> -               else
> -                       rc = -EOPNOTSUPP;
> +               /* extend file */
> +               else {
> +                       eof = cpu_to_le64(off + len);
> +                       rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
> +                                         cfile->fid.volatile_fid, cfile->pid,
> +                                         &eof);
> +               }
>                 if (rc)
>                         trace_smb3_falloc_err(xid, cfile->fid.persistent_fid,
>                                 tcon->tid, tcon->ses->Suid, off, len, rc);
> --
> 2.13.6
>
ronnie sahlberg Jan. 15, 2020, 2:25 a.m. UTC | #2
On Wed, Jan 15, 2020 at 11:25 AM Steve French <smfrench@gmail.com> wrote:
>
> Does it affect (or enable) any xfstests?

It shouldn't affect any current tests.
It adds support for
   xfs_io -c "falloc 0 512M" <file>

generic/071 now passes with this patch.   Possibly other tests as well
that use "xfs_io -c falloc" as well


>
> On Tue, Jan 14, 2020 at 7:23 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> >
> > RHBZ 1336264
> >
> > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > ---
> >  fs/cifs/smb2ops.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > index 6250370c1170..91818f7c1b9c 100644
> > --- a/fs/cifs/smb2ops.c
> > +++ b/fs/cifs/smb2ops.c
> > @@ -3106,9 +3106,13 @@ static long smb3_simple_falloc(struct file *file, struct cifs_tcon *tcon,
> >                 else if (i_size_read(inode) >= off + len)
> >                         /* not extending file and already not sparse */
> >                         rc = 0;
> > -               /* BB: in future add else clause to extend file */
> > -               else
> > -                       rc = -EOPNOTSUPP;
> > +               /* extend file */
> > +               else {
> > +                       eof = cpu_to_le64(off + len);
> > +                       rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
> > +                                         cfile->fid.volatile_fid, cfile->pid,
> > +                                         &eof);
> > +               }
> >                 if (rc)
> >                         trace_smb3_falloc_err(xid, cfile->fid.persistent_fid,
> >                                 tcon->tid, tcon->ses->Suid, off, len, rc);
> > --
> > 2.13.6
> >
>
>
> --
> Thanks,
>
> Steve
Steve French Jan. 15, 2020, 8:14 p.m. UTC | #3
Tentatively merged into cifs-2.6.git for-next pending more testing

On Tue, Jan 14, 2020 at 8:25 PM ronnie sahlberg
<ronniesahlberg@gmail.com> wrote:
>
> On Wed, Jan 15, 2020 at 11:25 AM Steve French <smfrench@gmail.com> wrote:
> >
> > Does it affect (or enable) any xfstests?
>
> It shouldn't affect any current tests.
> It adds support for
>    xfs_io -c "falloc 0 512M" <file>
>
> generic/071 now passes with this patch.   Possibly other tests as well
> that use "xfs_io -c falloc" as well
>
>
> >
> > On Tue, Jan 14, 2020 at 7:23 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> > >
> > > RHBZ 1336264
> > >
> > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > ---
> > >  fs/cifs/smb2ops.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > index 6250370c1170..91818f7c1b9c 100644
> > > --- a/fs/cifs/smb2ops.c
> > > +++ b/fs/cifs/smb2ops.c
> > > @@ -3106,9 +3106,13 @@ static long smb3_simple_falloc(struct file *file, struct cifs_tcon *tcon,
> > >                 else if (i_size_read(inode) >= off + len)
> > >                         /* not extending file and already not sparse */
> > >                         rc = 0;
> > > -               /* BB: in future add else clause to extend file */
> > > -               else
> > > -                       rc = -EOPNOTSUPP;
> > > +               /* extend file */
> > > +               else {
> > > +                       eof = cpu_to_le64(off + len);
> > > +                       rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
> > > +                                         cfile->fid.volatile_fid, cfile->pid,
> > > +                                         &eof);
> > > +               }
> > >                 if (rc)
> > >                         trace_smb3_falloc_err(xid, cfile->fid.persistent_fid,
> > >                                 tcon->tid, tcon->ses->Suid, off, len, rc);
> > > --
> > > 2.13.6
> > >
> >
> >
> > --
> > Thanks,
> >
> > Steve
Steve French Jan. 16, 2020, 2:03 a.m. UTC | #4
temporarily removed to allow Ronnie to debug a test failure

On Wed, Jan 15, 2020 at 2:14 PM Steve French <smfrench@gmail.com> wrote:
>
> Tentatively merged into cifs-2.6.git for-next pending more testing
>
> On Tue, Jan 14, 2020 at 8:25 PM ronnie sahlberg
> <ronniesahlberg@gmail.com> wrote:
> >
> > On Wed, Jan 15, 2020 at 11:25 AM Steve French <smfrench@gmail.com> wrote:
> > >
> > > Does it affect (or enable) any xfstests?
> >
> > It shouldn't affect any current tests.
> > It adds support for
> >    xfs_io -c "falloc 0 512M" <file>
> >
> > generic/071 now passes with this patch.   Possibly other tests as well
> > that use "xfs_io -c falloc" as well
> >
> >
> > >
> > > On Tue, Jan 14, 2020 at 7:23 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> > > >
> > > > RHBZ 1336264
> > > >
> > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > > ---
> > > >  fs/cifs/smb2ops.c | 10 +++++++---
> > > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > > index 6250370c1170..91818f7c1b9c 100644
> > > > --- a/fs/cifs/smb2ops.c
> > > > +++ b/fs/cifs/smb2ops.c
> > > > @@ -3106,9 +3106,13 @@ static long smb3_simple_falloc(struct file *file, struct cifs_tcon *tcon,
> > > >                 else if (i_size_read(inode) >= off + len)
> > > >                         /* not extending file and already not sparse */
> > > >                         rc = 0;
> > > > -               /* BB: in future add else clause to extend file */
> > > > -               else
> > > > -                       rc = -EOPNOTSUPP;
> > > > +               /* extend file */
> > > > +               else {
> > > > +                       eof = cpu_to_le64(off + len);
> > > > +                       rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
> > > > +                                         cfile->fid.volatile_fid, cfile->pid,
> > > > +                                         &eof);
> > > > +               }
> > > >                 if (rc)
> > > >                         trace_smb3_falloc_err(xid, cfile->fid.persistent_fid,
> > > >                                 tcon->tid, tcon->ses->Suid, off, len, rc);
> > > > --
> > > > 2.13.6
> > > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
>
>
>
> --
> Thanks,
>
> Steve
ronnie sahlberg Jan. 16, 2020, 8:42 a.m. UTC | #5
The bug is basically that if we extend a file by fallocate mode==0
and immediately afterwards mmap() the file we will only mmap() as much
as end-of-file was
prior to the truncate  and then if we try to touch any
address in this extended region userspace dies with bus error.

The patch added "extend a file with fallocate mode==0 for NON-Sparse
files" and caused xfstest to fail.
The fix is to force us to revalidate the file attributes (the size is
the important one) when we extend the file so
mmap() will work properly.
I have fixed this in the patch and will resend tomorrow after some more testing.

Looking for other SMB2_set_eof() callsites I see we already had the
same bug for the case of extending a SPARSE
file using fallocate mode==0. I fixed that too and will audit all
other plases where we use SMB2_set_eof()
to see if they are safe or not before resending.


On Thu, Jan 16, 2020 at 12:33 PM Steve French <smfrench@gmail.com> wrote:
>
> temporarily removed to allow Ronnie to debug a test failure
>
> On Wed, Jan 15, 2020 at 2:14 PM Steve French <smfrench@gmail.com> wrote:
> >
> > Tentatively merged into cifs-2.6.git for-next pending more testing
> >
> > On Tue, Jan 14, 2020 at 8:25 PM ronnie sahlberg
> > <ronniesahlberg@gmail.com> wrote:
> > >
> > > On Wed, Jan 15, 2020 at 11:25 AM Steve French <smfrench@gmail.com> wrote:
> > > >
> > > > Does it affect (or enable) any xfstests?
> > >
> > > It shouldn't affect any current tests.
> > > It adds support for
> > >    xfs_io -c "falloc 0 512M" <file>
> > >
> > > generic/071 now passes with this patch.   Possibly other tests as well
> > > that use "xfs_io -c falloc" as well
> > >
> > >
> > > >
> > > > On Tue, Jan 14, 2020 at 7:23 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> > > > >
> > > > > RHBZ 1336264
> > > > >
> > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> > > > > ---
> > > > >  fs/cifs/smb2ops.c | 10 +++++++---
> > > > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> > > > > index 6250370c1170..91818f7c1b9c 100644
> > > > > --- a/fs/cifs/smb2ops.c
> > > > > +++ b/fs/cifs/smb2ops.c
> > > > > @@ -3106,9 +3106,13 @@ static long smb3_simple_falloc(struct file *file, struct cifs_tcon *tcon,
> > > > >                 else if (i_size_read(inode) >= off + len)
> > > > >                         /* not extending file and already not sparse */
> > > > >                         rc = 0;
> > > > > -               /* BB: in future add else clause to extend file */
> > > > > -               else
> > > > > -                       rc = -EOPNOTSUPP;
> > > > > +               /* extend file */
> > > > > +               else {
> > > > > +                       eof = cpu_to_le64(off + len);
> > > > > +                       rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
> > > > > +                                         cfile->fid.volatile_fid, cfile->pid,
> > > > > +                                         &eof);
> > > > > +               }
> > > > >                 if (rc)
> > > > >                         trace_smb3_falloc_err(xid, cfile->fid.persistent_fid,
> > > > >                                 tcon->tid, tcon->ses->Suid, off, len, rc);
> > > > > --
> > > > > 2.13.6
> > > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > >
> > > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve
Pavel Shilovsky Jan. 17, 2020, 12:27 a.m. UTC | #6
чт, 16 янв. 2020 г. в 01:05, ronnie sahlberg <ronniesahlberg@gmail.com>:
>
> The bug is basically that if we extend a file by fallocate mode==0
> and immediately afterwards mmap() the file we will only mmap() as much
> as end-of-file was
> prior to the truncate  and then if we try to touch any
> address in this extended region userspace dies with bus error.
>
> The patch added "extend a file with fallocate mode==0 for NON-Sparse
> files" and caused xfstest to fail.
> The fix is to force us to revalidate the file attributes (the size is
> the important one) when we extend the file so
> mmap() will work properly.
> I have fixed this in the patch and will resend tomorrow after some more testing.
>
> Looking for other SMB2_set_eof() callsites I see we already had the
> same bug for the case of extending a SPARSE

I agree that regardless of file being sparse or not, we should somehow
update a size in the VFS after calling SMB2_set_eof().

> file using fallocate mode==0. I fixed that too and will audit all
> other plases where we use SMB2_set_eof()
> to see if they are safe or not before resending.

One of those places where SMB2_set_eof() is called is
cifs_set_file_size() which does call the following after getting a
successful response from the server:

2250 >-------if (rc == 0) {
2251 >------->-------cifsInode->server_eof = attrs->ia_size;
2252 >------->-------cifs_setsize(inode, attrs->ia_size);
2253 >------->-------cifs_truncate_page(inode->i_mapping, inode->i_size);
2254 >-------}

This is called by cifs_setattr_[no]unix() which does the following afterwards:

2569 >-------if ((attrs->ia_valid & ATTR_SIZE) &&
2570 >-------    attrs->ia_size != i_size_read(inode))
2571 >------->-------truncate_setsize(inode, attrs->ia_size);

truncate_setsize() does  similar things as cifs_setsize() besides
setting cinode->time to 0. This code path probably needs to be
refactored. But putting this aside, for the current fallocate fix I
think we should use the same existing mechanism to update a file size
in the VFS without revalidating the inode.

--
Best regards,
Pavel Shilovsky
ronnie sahlberg Jan. 17, 2020, 12:56 a.m. UTC | #7
On Fri, Jan 17, 2020 at 10:27 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>

Thanks.

> чт, 16 янв. 2020 г. в 01:05, ronnie sahlberg <ronniesahlberg@gmail.com>:
> >
> > The bug is basically that if we extend a file by fallocate mode==0
> > and immediately afterwards mmap() the file we will only mmap() as much
> > as end-of-file was
> > prior to the truncate  and then if we try to touch any
> > address in this extended region userspace dies with bus error.
> >
> > The patch added "extend a file with fallocate mode==0 for NON-Sparse
> > files" and caused xfstest to fail.
> > The fix is to force us to revalidate the file attributes (the size is
> > the important one) when we extend the file so
> > mmap() will work properly.
> > I have fixed this in the patch and will resend tomorrow after some more testing.
> >
> > Looking for other SMB2_set_eof() callsites I see we already had the
> > same bug for the case of extending a SPARSE
>
> I agree that regardless of file being sparse or not, we should somehow
> update a size in the VFS after calling SMB2_set_eof().
>
> > file using fallocate mode==0. I fixed that too and will audit all
> > other plases where we use SMB2_set_eof()
> > to see if they are safe or not before resending.
>
> One of those places where SMB2_set_eof() is called is
> cifs_set_file_size() which does call the following after getting a
> successful response from the server:
>
> 2250 >-------if (rc == 0) {
> 2251 >------->-------cifsInode->server_eof = attrs->ia_size;
> 2252 >------->-------cifs_setsize(inode, attrs->ia_size);
> 2253 >------->-------cifs_truncate_page(inode->i_mapping, inode->i_size);
> 2254 >-------}
>
> This is called by cifs_setattr_[no]unix() which does the following afterwards:
>
> 2569 >-------if ((attrs->ia_valid & ATTR_SIZE) &&
> 2570 >-------    attrs->ia_size != i_size_read(inode))
> 2571 >------->-------truncate_setsize(inode, attrs->ia_size);
>
> truncate_setsize() does  similar things as cifs_setsize() besides
> setting cinode->time to 0. This code path probably needs to be
> refactored. But putting this aside, for the current fallocate fix I
> think we should use the same existing mechanism to update a file size
> in the VFS without revalidating the inode.

I can do that change. Note however that since we are still setting
cinode->time to 0
we are still going to trigger a revalidation at some stage.

>
> --
> Best regards,
> Pavel Shilovsky

Patch
diff mbox series

diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 6250370c1170..91818f7c1b9c 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -3106,9 +3106,13 @@  static long smb3_simple_falloc(struct file *file, struct cifs_tcon *tcon,
 		else if (i_size_read(inode) >= off + len)
 			/* not extending file and already not sparse */
 			rc = 0;
-		/* BB: in future add else clause to extend file */
-		else
-			rc = -EOPNOTSUPP;
+		/* extend file */
+		else {
+			eof = cpu_to_le64(off + len);
+			rc = SMB2_set_eof(xid, tcon, cfile->fid.persistent_fid,
+					  cfile->fid.volatile_fid, cfile->pid,
+					  &eof);
+		}
 		if (rc)
 			trace_smb3_falloc_err(xid, cfile->fid.persistent_fid,
 				tcon->tid, tcon->ses->Suid, off, len, rc);