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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4A358C433F5 for ; Wed, 30 Mar 2022 15:16:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 605B56B0072; Wed, 30 Mar 2022 11:16:18 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5B5748D0002; Wed, 30 Mar 2022 11:16:18 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4A66A8D0001; Wed, 30 Mar 2022 11:16:18 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0112.hostedemail.com [216.40.44.112]) by kanga.kvack.org (Postfix) with ESMTP id 3DCB16B0072 for ; Wed, 30 Mar 2022 11:16:18 -0400 (EDT) Received: from smtpin23.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id EA8668249980 for ; Wed, 30 Mar 2022 15:16:17 +0000 (UTC) X-FDA: 79301403594.23.5D4F176 Received: from heian.cn.fujitsu.com (mail.cn.fujitsu.com [183.91.158.132]) by imf04.hostedemail.com (Postfix) with ESMTP id B20B740016 for ; Wed, 30 Mar 2022 15:16:16 +0000 (UTC) IronPort-Data: =?us-ascii?q?A9a23=3AtwvaI6h4vvUoVt85m0RQnGMKX161CxEKZh0ujC4?= =?us-ascii?q?5NGQNrF6WrkUEmGUWCmHVOa6OZTCnKNAnPovk908P7ZPRndZqHAtsrHw8FHgiR?= =?us-ascii?q?ejtX4rAdhiqV8+xwmwvdGo+toNGLICowPkcFhcwnT/wdOixxZVA/fvQHOCkUra?= =?us-ascii?q?dYnkZqTJME0/NtzoywobVvaY42bBVMyvV0T/Di5W31G2NglaYAUpIg063ky6Di?= =?us-ascii?q?dyp0N8uUvPSUtgQ1LPWvyF94JvyvshdJVOgKmVfNrbSq+ouUNiEEm3lExcFUrt?= =?us-ascii?q?Jk57wdAsEX7zTIROTzHFRXsBOgDAb/mprjPl9b6FaNC+7iB3Q9zx14M9QvJqrW?= =?us-ascii?q?EEnOLbQsOoAURhECDw4NqpDkFPCCSHl6pTCnx2XIhMAxN0rVinaJ7Yw9u9pAG1?= =?us-ascii?q?m++YfLTcXZBGfwemxxdqTSuJsrsUlItPiMI4Wtjdn1z6xJfovR9bBBbrL4dtZ1?= =?us-ascii?q?TIrrsFIAfvaIcEebFJHYBbfZBtAElQaEpQzmKGvnHaXWzlZrk+F4K8yy2vNxQd?= =?us-ascii?q?ylr/3P7L9fMKGRMBQtkKZvX7duWD4BAwKctCS11Kt8Huqi6nEnT7TX5gbH7m1s?= =?us-ascii?q?PVthTW7wm0VFQ1TW0C3rOe0jmagVN9FbU8Z4Cwjqe417kPDZt38WQCo5X2JpBg?= =?us-ascii?q?RX/JOHOAgrgKA0KzZ50CeHGdsZjpAbsE28d84XhQ02VKT2dDkHzpitPuSU331y?= =?us-ascii?q?1s+hVteIgBMdSlbO3BCFlBDvrHeTEgIpkqnZr5e/GSd17UZwQ3N/g0=3D?= IronPort-HdrOrdr: =?us-ascii?q?A9a23=3AMHdCR6lcfaT+oQuP8yxoprJboKPpDfIQ3DAb?= =?us-ascii?q?v31ZSRFFG/Fw9vre+MjzsCWYtN9/Yh8dcK+7UpVoLUm8yXcX2/h1AV7BZniEhI?= =?us-ascii?q?LAFugLgrcKqAeQeREWmNQ86Y5QN4B6CPDVSWNxlNvG5mCDeOoI8Z2q97+JiI7l?= =?us-ascii?q?o0tQcQ=3D=3D?= X-IronPort-AV: E=Sophos;i="5.88,333,1635177600"; d="scan'208";a="123098899" Received: from unknown (HELO cn.fujitsu.com) ([10.167.33.5]) by heian.cn.fujitsu.com with ESMTP; 30 Mar 2022 23:16:14 +0800 Received: from G08CNEXMBPEKD05.g08.fujitsu.local (unknown [10.167.33.204]) by cn.fujitsu.com (Postfix) with ESMTP id AEC9B4D16FF5; Wed, 30 Mar 2022 23:16:11 +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; Wed, 30 Mar 2022 23:16:11 +0800 Received: from [10.167.201.8] (10.167.201.8) by G08CNEXCHPEKD09.g08.fujitsu.local (10.167.33.209) with Microsoft SMTP Server id 15.0.1497.23 via Frontend Transport; Wed, 30 Mar 2022 23:16:11 +0800 Message-ID: <894ed00b-b174-6a10-ee45-320007957ea4@fujitsu.com> Date: Wed, 30 Mar 2022 23:16:10 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH v11 7/8] xfs: Implement ->notify_failure() for XFS To: Christoph Hellwig CC: , , , , , , , , References: <20220227120747.711169-1-ruansy.fnst@fujitsu.com> <20220227120747.711169-8-ruansy.fnst@fujitsu.com> From: Shiyang Ruan In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed X-yoursite-MailScanner-ID: AEC9B4D16FF5.A4774 X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: ruansy.fnst@fujitsu.com X-Rspam-User: Authentication-Results: imf04.hostedemail.com; dkim=none; spf=none (imf04.hostedemail.com: domain of ruansy.fnst@fujitsu.com has no SPF policy when checking 183.91.158.132) smtp.mailfrom=ruansy.fnst@fujitsu.com; dmarc=fail reason="No valid SPF, No valid DKIM" header.from=fujitsu.com (policy=none) X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: B20B740016 X-Stat-Signature: prftuisjtdw5umxy61hk4mhgx6keikzj X-HE-Tag: 1648653376-423882 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: =E5=9C=A8 2022/3/30 14:00, Christoph Hellwig =E5=86=99=E9=81=93: >> @@ -1892,6 +1893,8 @@ xfs_free_buftarg( >> list_lru_destroy(&btp->bt_lru); >> =20 >> blkdev_issue_flush(btp->bt_bdev); >> + if (btp->bt_daxdev) >> + dax_unregister_holder(btp->bt_daxdev, btp->bt_mount); >> fs_put_dax(btp->bt_daxdev); >> =20 >> kmem_free(btp); >> @@ -1939,6 +1942,7 @@ xfs_alloc_buftarg( >> struct block_device *bdev) >> { >> xfs_buftarg_t *btp; >> + int error; >> =20 >> btp =3D kmem_zalloc(sizeof(*btp), KM_NOFS); >> =20 >> @@ -1946,6 +1950,14 @@ xfs_alloc_buftarg( >> btp->bt_dev =3D bdev->bd_dev; >> btp->bt_bdev =3D bdev; >> btp->bt_daxdev =3D fs_dax_get_by_bdev(bdev, &btp->bt_dax_part_off); >> + if (btp->bt_daxdev) { >> + error =3D dax_register_holder(btp->bt_daxdev, mp, >> + &xfs_dax_holder_operations); >> + if (error) { >> + xfs_err(mp, "DAX device already in use?!"); >> + goto error_free; >> + } >> + } >=20 > It seems to me that just passing the holder and holder ops to > fs_dax_get_by_bdev and the holder to dax_unregister_holder would > significantly simply the interface here. >=20 > Dan, what do you think? >=20 >> +#if IS_ENABLED(CONFIG_MEMORY_FAILURE) && IS_ENABLED(CONFIG_FS_DAX) >=20 > No real need for the IS_ENABLED. Also any reason to even build this > file if the options are not set? It seems like > xfs_dax_holder_operations should just be defined to NULL and the > whole file not supported if we can't support the functionality. Got it. These two CONFIG seem not related for now. So, I think I=20 should wrap these code with #ifdef CONFIG_MEMORY_FAILURE here, and add=20 `xfs-$(CONFIG_FS_DAX) +=3D xfs_notify_failure.o` in the makefile. >=20 > Dan: not for this series, but is there any reason not to require > MEMORY_FAILURE for DAX to start with? >=20 >> + >> + ddev_start =3D mp->m_ddev_targp->bt_dax_part_off; >> + ddev_end =3D ddev_start + >> + (mp->m_ddev_targp->bt_bdev->bd_nr_sectors << SECTOR_SHIFT) - 1; >=20 > This should use bdev_nr_bytes. OK. >=20 > But didn't we say we don't want to support notifications on partitioned > devices and thus don't actually need all this? >=20 >> + >> + /* Ignore the range out of filesystem area */ >> + if ((offset + len) < ddev_start) >=20 > No need for the inner braces. >=20 >> + if ((offset + len) > ddev_end) >=20 > No need for the braces either. Really no need? It is to make sure the range to be handled won't out of=20 the filesystem area. And make sure the @offset and @len are valid and=20 correct after subtract the bbdev_start. >=20 >> diff --git a/fs/xfs/xfs_notify_failure.h b/fs/xfs/xfs_notify_failure.h >> new file mode 100644 >> index 000000000000..76187b9620f9 >> --- /dev/null >> +++ b/fs/xfs/xfs_notify_failure.h >> @@ -0,0 +1,10 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2022 Fujitsu. All Rights Reserved. >> + */ >> +#ifndef __XFS_NOTIFY_FAILURE_H__ >> +#define __XFS_NOTIFY_FAILURE_H__ >> + >> +extern const struct dax_holder_operations xfs_dax_holder_operations; >> + >> +#endif /* __XFS_NOTIFY_FAILURE_H__ */ >=20 > Dowe really need a new header for this vs just sequeezing it into > xfs_super.h or something like that? Yes, I'll move it into xfs_super.h. The xfs_notify_failure.c was=20 splitted from xfs_super.c in the old patch. There is no need to create=20 a header file for only single line of code. >=20 >> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c >> index e8f37bdc8354..b8de6ed2c888 100644 >> --- a/fs/xfs/xfs_super.c >> +++ b/fs/xfs/xfs_super.c >> @@ -353,6 +353,12 @@ xfs_setup_dax_always( >> return -EINVAL; >> } >> =20 >> + if (xfs_has_reflink(mp) && !xfs_has_rmapbt(mp)) { >> + xfs_alert(mp, >> + "need rmapbt when both DAX and reflink enabled."); >> + return -EINVAL; >> + } >=20 > Right now we can't even enable reflink with DAX yet, so adding this > here seems premature - it should go into the patch allowing DAX+reflink= . >=20 Yes. I'll remove it for now. -- Thanks, Ruan.