ceph: fix copy_file_range error path in short copies
diff mbox series

Message ID 20200205102852.12236-1-lhenriques@suse.com
State New
Headers show
Series
  • ceph: fix copy_file_range error path in short copies
Related show

Commit Message

Luis Henriques Feb. 5, 2020, 10:28 a.m. UTC
When there's an error in the copying loop but some bytes have already been
copied into the destination file, it is necessary to dirty the caps and
eventually update the MDS with the file metadata (timestamps, size).  This
patch fixes this error path.

Cc: stable@vger.kernel.org
Signed-off-by: Luis Henriques <lhenriques@suse.com>
---
 fs/ceph/file.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Ilya Dryomov Feb. 5, 2020, 11:16 a.m. UTC | #1
On Wed, Feb 5, 2020 at 11:28 AM Luis Henriques <lhenriques@suse.com> wrote:
>
> When there's an error in the copying loop but some bytes have already been
> copied into the destination file, it is necessary to dirty the caps and
> eventually update the MDS with the file metadata (timestamps, size).  This
> patch fixes this error path.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Luis Henriques <lhenriques@suse.com>
> ---
>  fs/ceph/file.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index 11929d2bb594..7be47d24edb1 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -2104,9 +2104,16 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
>                         CEPH_OSD_OP_FLAG_FADVISE_DONTNEED, 0);
>                 if (err) {
>                         dout("ceph_osdc_copy_from returned %d\n", err);
> -                       if (!ret)
> +                       /*
> +                        * If we haven't done any copy yet, just exit with the
> +                        * error code; otherwise, return the number of bytes
> +                        * already copied, update metadata and dirty caps.
> +                        */
> +                       if (!ret) {
>                                 ret = err;
> -                       goto out_caps;
> +                               goto out_caps;
> +                       }
> +                       goto out_early;
>                 }
>                 len -= object_size;
>                 src_off += object_size;
> @@ -2118,6 +2125,7 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
>                 /* We still need one final local copy */
>                 do_final_copy = true;
>
> +out_early:

out_early is misleading, especially given that there already
is out_caps, which just puts caps.  I suggest something like
update_dst_inode.

>         file_update_time(dst_file);
>         inode_inc_iversion_raw(dst_inode);
>

I think this is still buggy.  What follows is this:

        if (endoff > size) {
                int caps_flags = 0;

                /* Let the MDS know about dst file size change */
                if (ceph_quota_is_max_bytes_approaching(dst_inode, endoff))
                        caps_flags |= CHECK_CAPS_NODELAY;
                if (ceph_inode_set_size(dst_inode, endoff))
                        caps_flags |= CHECK_CAPS_AUTHONLY;
                if (caps_flags)
                        ceph_check_caps(dst_ci, caps_flags, NULL);
        }

with endoff being:

        size = i_size_read(dst_inode);
        endoff = dst_off + len;

So a short copy effectively zero-fills the destination file...

Thanks,

                Ilya
Luis Henriques Feb. 5, 2020, 7:24 p.m. UTC | #2
On Wed, Feb 05, 2020 at 12:16:02PM +0100, Ilya Dryomov wrote:
> On Wed, Feb 5, 2020 at 11:28 AM Luis Henriques <lhenriques@suse.com> wrote:
> >
> > When there's an error in the copying loop but some bytes have already been
> > copied into the destination file, it is necessary to dirty the caps and
> > eventually update the MDS with the file metadata (timestamps, size).  This
> > patch fixes this error path.
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Luis Henriques <lhenriques@suse.com>
> > ---
> >  fs/ceph/file.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > index 11929d2bb594..7be47d24edb1 100644
> > --- a/fs/ceph/file.c
> > +++ b/fs/ceph/file.c
> > @@ -2104,9 +2104,16 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
> >                         CEPH_OSD_OP_FLAG_FADVISE_DONTNEED, 0);
> >                 if (err) {
> >                         dout("ceph_osdc_copy_from returned %d\n", err);
> > -                       if (!ret)
> > +                       /*
> > +                        * If we haven't done any copy yet, just exit with the
> > +                        * error code; otherwise, return the number of bytes
> > +                        * already copied, update metadata and dirty caps.
> > +                        */
> > +                       if (!ret) {
> >                                 ret = err;
> > -                       goto out_caps;
> > +                               goto out_caps;
> > +                       }
> > +                       goto out_early;
> >                 }
> >                 len -= object_size;
> >                 src_off += object_size;
> > @@ -2118,6 +2125,7 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
> >                 /* We still need one final local copy */
> >                 do_final_copy = true;
> >
> > +out_early:
> 
> out_early is misleading, especially given that there already
> is out_caps, which just puts caps.  I suggest something like
> update_dst_inode.
> 
> >         file_update_time(dst_file);
> >         inode_inc_iversion_raw(dst_inode);
> >
> 
> I think this is still buggy.  What follows is this:
> 
>         if (endoff > size) {
>                 int caps_flags = 0;
> 
>                 /* Let the MDS know about dst file size change */
>                 if (ceph_quota_is_max_bytes_approaching(dst_inode, endoff))
>                         caps_flags |= CHECK_CAPS_NODELAY;
>                 if (ceph_inode_set_size(dst_inode, endoff))
>                         caps_flags |= CHECK_CAPS_AUTHONLY;
>                 if (caps_flags)
>                         ceph_check_caps(dst_ci, caps_flags, NULL);
>         }
> 
> with endoff being:
> 
>         size = i_size_read(dst_inode);
>         endoff = dst_off + len;
> 
> So a short copy effectively zero-fills the destination file...

Ah!  What a surprise!  Yet another bug in copy_file_range.  /me hides

I guess that replacing 'endoff' by 'dst_off' in the 'if' statement above
(including the condition itself) should fix it.  But I start to think that
I'm biased and unable to see the most obvious issues with this code :-/

Cheers,
--
Luís

Patch
diff mbox series

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 11929d2bb594..7be47d24edb1 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -2104,9 +2104,16 @@  static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
 			CEPH_OSD_OP_FLAG_FADVISE_DONTNEED, 0);
 		if (err) {
 			dout("ceph_osdc_copy_from returned %d\n", err);
-			if (!ret)
+			/*
+			 * If we haven't done any copy yet, just exit with the
+			 * error code; otherwise, return the number of bytes
+			 * already copied, update metadata and dirty caps.
+			 */
+			if (!ret) {
 				ret = err;
-			goto out_caps;
+				goto out_caps;
+			}
+			goto out_early;
 		}
 		len -= object_size;
 		src_off += object_size;
@@ -2118,6 +2125,7 @@  static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
 		/* We still need one final local copy */
 		do_final_copy = true;
 
+out_early:
 	file_update_time(dst_file);
 	inode_inc_iversion_raw(dst_inode);