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

[Luis Henriques]

On Mon, Feb 10, 2020 at 07:38:10PM +0100, Ilya Dryomov wrote:
> On Thu, Feb 6, 2020 at 11:38 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.
> >
> > 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Luis Henriques <lhenriques@xxxxxxxx>
> > ---
> >  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.

Right, it makes sense to refactor this function and fix all these issues
you're pointing.  It'll be a pain because a lot of parameters will need to
be handed over into these new functions (maybe a new 'struct copy_desc'
can help making it a bit less messy).  Anyway, I'll try to spend some time
working on that and see what I can come up with.

Cheers,
--
Luís