From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from heian.cn.fujitsu.com (mail.cn.fujitsu.com [183.91.158.132]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 3FDC43FC1 for ; Fri, 27 Aug 2021 03:30:02 +0000 (UTC) IronPort-HdrOrdr: =?us-ascii?q?A9a23=3AWKLzS6phbN9Sn/NnDITgk1MaV5oXeYIsimQD?= =?us-ascii?q?101hICG9E/bo8/xG+c536faaslgssQ4b8+xoVJPgfZq+z+8R3WByB8bAYOCOgg?= =?us-ascii?q?LBQ72KhrGSoQEIdRefysdtkY9kc4VbTOb7FEVGi6/BizWQIpINx8am/cmT6dvj?= =?us-ascii?q?8w=3D=3D?= X-IronPort-AV: E=Sophos;i="5.84,355,1620662400"; d="scan'208";a="113546863" Received: from unknown (HELO cn.fujitsu.com) ([10.167.33.5]) by heian.cn.fujitsu.com with ESMTP; 27 Aug 2021 11:30:02 +0800 Received: from G08CNEXMBPEKD05.g08.fujitsu.local (unknown [10.167.33.204]) by cn.fujitsu.com (Postfix) with ESMTP id 0B4FE4D0D9DD; Fri, 27 Aug 2021 11:30:01 +0800 (CST) Received: from G08CNEXCHPEKD09.g08.fujitsu.local (10.167.33.85) by G08CNEXMBPEKD05.g08.fujitsu.local (10.167.33.204) with Microsoft SMTP Server (TLS) id 15.0.1497.23; Fri, 27 Aug 2021 11:29:57 +0800 Received: from [192.168.22.65] (10.167.225.141) by G08CNEXCHPEKD09.g08.fujitsu.local (10.167.33.209) with Microsoft SMTP Server id 15.0.1497.23 via Frontend Transport; Fri, 27 Aug 2021 11:29:54 +0800 Subject: Re: [PATCH v7 7/8] fsdax: Introduce dax_iomap_ops for end of reflink To: Dan Williams CC: "Darrick J. Wong" , Christoph Hellwig , linux-xfs , david , linux-fsdevel , Linux Kernel Mailing List , Linux NVDIMM , Goldwyn Rodrigues , Al Viro , Matthew Wilcox References: <20210816060359.1442450-1-ruansy.fnst@fujitsu.com> <20210816060359.1442450-8-ruansy.fnst@fujitsu.com> From: Shiyang Ruan Message-ID: <32fa5333-b14e-2060-d659-d77f6c75ff16@fujitsu.com> Date: Fri, 27 Aug 2021 11:29:54 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 Precedence: bulk X-Mailing-List: nvdimm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-yoursite-MailScanner-ID: 0B4FE4D0D9DD.A549F X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: ruansy.fnst@fujitsu.com X-Spam-Status: No On 2021/8/20 23:18, Dan Williams wrote: > On Thu, Aug 19, 2021 at 11:13 PM ruansy.fnst wrote: >> >> >> >> On 2021/8/20 上午11: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 reflink >>>> - 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 iomap_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 = ops->actor_end(iter->inode, iter->pos, iter->len, >>>> + 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/#m79a66a928da2d089e2458c1a97c0516dbfde2f7f >> >> 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 = 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 = ops->iomap_end(iter->inode, iter->pos, iomap_length(iter), > iter->processed > 0 ? iter->processed : 0, As you can see, here is the same logic as the old iomap_apply(): the negative iter->processed won't be passed into ->iomap_end(). > 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 = container_of(iomap, typeof(*iter), iomap); > struct xfs_inode *ip = XFS_I(inode); > ssize_t written = iter->processed; The written will be 0 or positive. The original error code is ingnored. > bool cow = xfs_is_cow_inode(ip); > > if (cow) { > if (written <= 0) > xfs_reflink_cancel_cow_range(ip, pos, length, true) > } > -- Thanks, Ruan.