On Thu, Jan 30, 2020 at 04:59:59PM +0100, Ilya Dryomov wrote:
...
> > > > @@ -2117,14 +2126,33 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
> > > > dout("ceph_osdc_copy_from returned %d\n", err);
> > > > if (!ret)
> > > > ret = err;
> > > > + /* wait for all queued requests */
> > > > + ceph_osdc_wait_requests(&osd_reqs);
> > > > goto out_caps;
> > > > }
> > > > + list_add(&req->r_private_item, &osd_reqs);
> > > > len -= object_size;
> > > > src_off += object_size;
> > > > dst_off += object_size;
> > > > ret += object_size;
> > >
> > > So ret is incremented here, but you have numerious tests where ret is
> > > assigned an error only if ret is 0. Unless I'm missing something, this
> > > interferes with returning errors from __ceph_copy_file_range().
> >
> > Well, the problem is that an error may occur *after* we have already done
> > some copies. In that case we need to return the number of bytes that have
> > been successfully copied instead of an error; eventually, subsequent calls
> > to complete the copy_file_range will then return the error. At least this
> > is how I understood the man page (i.e. similar to the write(2) syscall).
>
> AFAICS ret is incremented before you know that *any* of the copies were
> successful. If the first copy fails, how do you report that error?
Ah, got it. So, a solution would be to have the ceph_osdc_wait_requests
interface changed so that we can get the number of successful requests.
/me takes a deep breath and goes look at the *whole* thing to prevent
these mistakes. Thanks a lot for your review, Ilya.
>
> >
> > > > + if (--ncopies == 0) {
> > > > + err = ceph_osdc_wait_requests(&osd_reqs);
> > > > + if (err) {
> > > > + if (!ret)
> > > > + ret = err;
> > > > + goto out_caps;
> > > > + }
> > > > + ncopies = (src_fsc->mount_options->wsize /
> > > > + object_size) * 4;
> > >
> > > The object size is constant within a file, so ncopies should be too.
> > > Perhaps introduce a counter instead of recalculating ncopies here?
> >
> > Not sure I understood your comment. You would rather have:
> >
> > * ncopies initialized only once outside the loop
> > * have a counter counting the number of objects copied
> > * call ceph_osdc_wait_requests() when this counter is a multiple of
> > ncopies
>
> I was thinking of a counter that is initialized to ncopies and reset to
> ncopies any time it reaches 0. This is just a nit though.
Sure, no problem.
Cheers,
--
Luís
