lkml.org 
[lkml]   [2020]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v2] ceph: fix copy_file_range error path in short copies
    On Thu, Feb 6, 2020 at 11:38 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.
    >
    > Another issue this patch fixes is the destination file size being reported
    > to the MDS. If we're on the error path but the amount of bytes written
    > has already changed the destination file size, the offset to use is
    > dst_off and not endoff.
    >
    > Cc: stable@vger.kernel.org
    > Signed-off-by: Luis Henriques <lhenriques@suse.com>
    > ---
    > fs/ceph/file.c | 18 +++++++++++++-----
    > 1 file changed, 13 insertions(+), 5 deletions(-)
    >
    > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
    > index 11929d2bb594..f7f8cb6c243f 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 update_dst_inode;
    > }
    > len -= object_size;
    > src_off += object_size;
    > @@ -2118,16 +2125,17 @@ 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;
    >
    > +update_dst_inode:
    > file_update_time(dst_file);
    > inode_inc_iversion_raw(dst_inode);
    >
    > - if (endoff > size) {
    > + if (dst_off > size) {
    > int caps_flags = 0;
    >
    > /* Let the MDS know about dst file size change */
    > - if (ceph_quota_is_max_bytes_approaching(dst_inode, endoff))
    > + if (ceph_quota_is_max_bytes_approaching(dst_inode, dst_off))
    > caps_flags |= CHECK_CAPS_NODELAY;
    > - if (ceph_inode_set_size(dst_inode, endoff))
    > + if (ceph_inode_set_size(dst_inode, dst_off))
    > caps_flags |= CHECK_CAPS_AUTHONLY;
    > if (caps_flags)
    > ceph_check_caps(dst_ci, caps_flags, NULL);

    Hi Luis,

    I think this function still has short copy and file size issues:

    - do_splice_direct() may write fewer bytes than requested, including
    nothing at all (i.e. return 0). While we don't care about the second
    call much, handling the first call is crucial because proceeding to
    the copy-from loop with src/dst_off not at the object boundary will
    corrupt the destination file.

    - size is set after caps are acquired for the first time and never
    updated. But caps are dropped before do_splice_direct(), so by the
    time we get to dst_off > size check, it may be stale. Again, data
    loss if e.g. old-size < dst_off < new-size because the destination
    file will get truncated...

    Also, src/dst_oloc need to be freed with ceph_oloc_destroy() to avoid
    leaking memory on namespace layouts.

    It seems clear that this function needs to be split, with the new
    loop around do_splice_direct() and the copy-from loop each going into
    a separate functions with clear pre- and post-conditions.

    Thanks,

    Ilya

    \
     
     \ /
      Last update: 2020-02-10 19:38    [W:4.897 / U:0.408 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site