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
