Re: [PATCH] ceph: fix copy_file_range error path in short copies

[Ilya Dryomov]

On Wed, Feb 5, 2020 at 11:28 AM Luis Henriques <lhenriques@xxxxxxxx> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Luis Henriques <lhenriques@xxxxxxxx>
> ---
>  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