From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CB995C2D0DB for ; Thu, 30 Jan 2020 16:31:29 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A875A206D5 for ; Thu, 30 Jan 2020 16:31:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727369AbgA3Qb3 (ORCPT ); Thu, 30 Jan 2020 11:31:29 -0500 Received: from mx2.suse.de ([195.135.220.15]:36108 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727191AbgA3Qb2 (ORCPT ); Thu, 30 Jan 2020 11:31:28 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 70098AFDF; Thu, 30 Jan 2020 16:31:25 +0000 (UTC) Date: Thu, 30 Jan 2020 16:31:01 +0000 From: Luis Henriques To: Ilya Dryomov Cc: Jeff Layton , Sage Weil , "Yan, Zheng" , Gregory Farnum , Ceph Development , LKML Subject: Re: [PATCH 1/1] ceph: parallelize all copy-from requests in copy_file_range Message-ID: <20200130163101.GA22679@suse.com> References: <20200129182011.5483-1-lhenriques@suse.com> <20200129182011.5483-2-lhenriques@suse.com> <20200130153711.GA20170@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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