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@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...
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
