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=-13.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable 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 57573C4338F for ; Fri, 20 Aug 2021 15:19:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4116960ED6 for ; Fri, 20 Aug 2021 15:19:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234323AbhHTPTp (ORCPT ); Fri, 20 Aug 2021 11:19:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46190 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240956AbhHTPTk (ORCPT ); Fri, 20 Aug 2021 11:19:40 -0400 Received: from mail-pj1-x102b.google.com (mail-pj1-x102b.google.com [IPv6:2607:f8b0:4864:20::102b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 40BDEC061757 for ; Fri, 20 Aug 2021 08:18:56 -0700 (PDT) Received: by mail-pj1-x102b.google.com with SMTP id qe12-20020a17090b4f8c00b00179321cbae7so7483572pjb.2 for ; Fri, 20 Aug 2021 08:18:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=e7Os9VAnizBXv51HC/2jWtdqVC6IT3MX2PEly7Hzllc=; b=M+ibRtOTQAZRmC0FnL3AMilGRh2mUgMJmWUfg9JNOtAKXFs9lFLPXucpaO7ELUpNDb EGbzGydfkHCtIjzQLeNz+1/fIOA2rFhRJv3yC4ZhYvpISJH4iNfkhazi5Hz/Nq0fpYXO QpGdn8L+8PdTFb0E+p78qZDHRXkUnXo7JNSYlGQecnF0IhE5VtdC/Y+7Y9yUsL+y7PV3 5x4ObvPPy0AxrVohJG1gL8vPnbME69k1gCKAA1GyMrEqgR+PWW/PMthly7voCniXVW59 FcwvvH0AWzl2X+1XFpdijT2XoPDvNcAXBc2pyggbrImBdT+vil40sbH7g0dNEEtf0bPA hjMA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=e7Os9VAnizBXv51HC/2jWtdqVC6IT3MX2PEly7Hzllc=; b=rz1YtarG2j7gzIIxnR6jTzE+cWnIOtoy58TmV8VTx+VdAnuse87oKI58iaOae1oT0K /HFq/lZeUsxs6/oFxkZKYSPXHYl7SjdYVwDar9v8wt+Xuzo59o6CxX9ALUou0GclZafE cpr1IyatX4ZB1GTSfSHAVZO1w0AZF64LFaaqxG97fgpIg58Gi/NHhUkTH1ZaAI9Ykwwt KG6Y7t7aajx8fZNMVS+eg41vZYRcJiR43xjxsWQk3JAnnS3ToXFPC84zkZBipVhLsjGc xFR6iQqWNLCAUGe6+cyOFVjeBqyrAW7zPRbbuMzzjSoEA4s4r9ueAjM6dxkb/pOlV1CN Lmvw== X-Gm-Message-State: AOAM5308dybEgtrWTm0gFkFO2CFgEKWO7RTnWP0XYXK8n+xRNoxAIe40 U6R9hihwPcU5Jg+S5p2YZhGYcMMSsTwBa09TnZ5HGA== X-Google-Smtp-Source: ABdhPJzh0Gic3915Mh1iFhrG9Et6Tc2pawPckB/akGEtXtIixYmIi3AnAExi5rU0t9vJgG3HUdy+9gqyxaTkb99WGQ0= X-Received: by 2002:a17:90a:708c:: with SMTP id g12mr5224172pjk.13.1629472735668; Fri, 20 Aug 2021 08:18:55 -0700 (PDT) MIME-Version: 1.0 References: <20210816060359.1442450-1-ruansy.fnst@fujitsu.com> <20210816060359.1442450-8-ruansy.fnst@fujitsu.com> In-Reply-To: From: Dan Williams Date: Fri, 20 Aug 2021 08:18:44 -0700 Message-ID: Subject: Re: [PATCH v7 7/8] fsdax: Introduce dax_iomap_ops for end of reflink To: "ruansy.fnst" Cc: "Darrick J. Wong" , Christoph Hellwig , linux-xfs , david , linux-fsdevel , Linux Kernel Mailing List , Linux NVDIMM , Goldwyn Rodrigues , Al Viro , Matthew Wilcox Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-xfs@vger.kernel.org On Thu, Aug 19, 2021 at 11:13 PM ruansy.fnst wrot= e: > > > > On 2021/8/20 =E4=B8=8A=E5=8D=8811:01, Dan Williams wrote: > > On Sun, Aug 15, 2021 at 11:05 PM Shiyang Ruan = wrote: > >> > >> After writing data, reflink requires end operations to remap those new > >> allocated extents. The current ->iomap_end() ignores the error code > >> returned from ->actor(), so we introduce this dax_iomap_ops and change > >> the dax_iomap_*() interfaces to do this job. > >> > >> - the dax_iomap_ops contains the original struct iomap_ops and fsdax > >> specific ->actor_end(), which is for the end operations of reflin= k > >> - also introduce dax specific zero_range, truncate_page > >> - create new dax_iomap_ops for ext2 and ext4 > >> > >> Signed-off-by: Shiyang Ruan > >> --- > >> fs/dax.c | 68 +++++++++++++++++++++++++++++++++++++---= -- > >> fs/ext2/ext2.h | 3 ++ > >> fs/ext2/file.c | 6 ++-- > >> fs/ext2/inode.c | 11 +++++-- > >> fs/ext4/ext4.h | 3 ++ > >> fs/ext4/file.c | 6 ++-- > >> fs/ext4/inode.c | 13 ++++++-- > >> fs/iomap/buffered-io.c | 3 +- > >> fs/xfs/xfs_bmap_util.c | 3 +- > >> fs/xfs/xfs_file.c | 8 ++--- > >> fs/xfs/xfs_iomap.c | 36 +++++++++++++++++++++- > >> fs/xfs/xfs_iomap.h | 33 ++++++++++++++++++++ > >> fs/xfs/xfs_iops.c | 7 ++--- > >> fs/xfs/xfs_reflink.c | 3 +- > >> include/linux/dax.h | 21 ++++++++++--- > >> include/linux/iomap.h | 1 + > >> 16 files changed, 189 insertions(+), 36 deletions(-) > >> > >> diff --git a/fs/dax.c b/fs/dax.c > >> index 74dd918cff1f..0e0536765a7e 100644 > >> --- a/fs/dax.c > >> +++ b/fs/dax.c > >> @@ -1348,11 +1348,30 @@ static loff_t dax_iomap_iter(const struct ioma= p_iter *iomi, > >> return done ? done : ret; > >> } > >> > >> +static inline int > >> +__dax_iomap_iter(struct iomap_iter *iter, const struct dax_iomap_ops = *ops) > >> +{ > >> + int ret; > >> + > >> + /* > >> + * Call dax_iomap_ops->actor_end() before iomap_ops->iomap_end= () in > >> + * each iteration. > >> + */ > >> + if (iter->iomap.length && ops->actor_end) { > >> + ret =3D ops->actor_end(iter->inode, iter->pos, iter->l= en, > >> + iter->processed); > >> + if (ret < 0) > >> + return ret; > >> + } > >> + > >> + return iomap_iter(iter, &ops->iomap_ops); > > > > This reorganization looks needlessly noisy. Why not require the > > iomap_end operation to perform the actor_end work. I.e. why can't > > xfs_dax_write_iomap_actor_end() just be the passed in iomap_end? I am > > not seeing where the ->iomap_end() result is ignored? > > > > The V6 patch[1] was did in this way. > [1]https://lore.kernel.org/linux-xfs/20210526005159.GF202144@locust/T/#m7= 9a66a928da2d089e2458c1a97c0516dbfde2f7f > > But Darrick reminded me that ->iomap_end() will always take zero or > positive 'written' because iomap_apply() handles this argument. > > ``` > if (ops->iomap_end) { > ret =3D ops->iomap_end(inode, pos, length, > written > 0 ? written : 0, > flags, &iomap); > } > ``` > > So, we cannot get actual return code from CoW in ->actor(), and as a > result, we cannot handle the xfs end_cow correctly in ->iomap_end(). > That's where the result of CoW was ignored. Ah, thank you for the explanation. However, this still seems like too much code thrash just to get back to the original value of iter->processed. I notice you are talking about iomap_apply(), but that routine is now gone in Darrick's latest iomap-for-next branch. Instead iomap_iter() does this: if (iter->iomap.length && ops->iomap_end) { ret =3D ops->iomap_end(iter->inode, iter->pos, iomap_length= (iter), iter->processed > 0 ? iter->processed : 0, iter->flags, &iter->iomap); if (ret < 0 && !iter->processed) return ret; } I notice that the @iomap argument to ->iomap_end() is reliably coming from @iter. So you could do the following in your iomap_end() callback: struct iomap_iter *iter =3D container_of(iomap, typeof(*iter), ioma= p); struct xfs_inode *ip =3D XFS_I(inode); ssize_t written =3D iter->processed; bool cow =3D xfs_is_cow_inode(ip); if (cow) { if (written <=3D 0) xfs_reflink_cancel_cow_range(ip, pos, length, true) }