Received-SPF: pass (google.com: domain of linux-kernel+bounces-33587-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Naohiro Aota <Naohiro.Aota@wdc.com>
To: Johannes Thumshirn <Johannes.Thumshirn@wdc.com>
CC: Josef Bacik <josef@toxicpanda.com>, David Sterba <dsterba@suse.com>,
	"linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Damien Le Moal
	<dlemoal@kernel.org>
Subject: Re: Re: [PATCH 2/2] btrfs: zoned: wake up cleaner sooner if needed
Thread-Topic: Re: [PATCH 2/2] btrfs: zoned: wake up cleaner sooner if needed
Thread-Index: AQHaTUFOSU3kvBJL0EyMMqqSJ1tHJ7Dl9EwA
Date: Mon, 22 Jan 2024 15:26:45 +0000
Message-ID: <mhprsf62tm4n6e5lhcujucu6ejfaomexrud3prqxlv5k6rwlws@q6gn2lsueoyl>
References: <20240122-reclaim-fix-v1-0-761234a6d005@wdc.com>
 <20240122-reclaim-fix-v1-2-761234a6d005@wdc.com>
 <x6bi4u2u65q37tde3s357lzhce4wglpobfgp7qgzhun4iadg3m@2pewiu6xuts4>
 <f5d54836-5edf-4cd0-88c8-f2d474368ea9@wdc.com>
 <k7fiky6xm4hshkr5q2xukfjndcseiesfanlpc4oozztvuyclbw@ftrhgjeicsfs>
 <2fa9112d-a72a-440f-a32a-199b5350b59d@wdc.com>
In-Reply-To: <2fa9112d-a72a-440f-a32a-199b5350b59d@wdc.com>
Accept-Language: ja-JP, en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?IAY2PUaRjZrFJVe0TReJa3Wod7O/pQcLQrSc3jrOsE5pT0ClrLWFKYVohzHg?=
 =?us-ascii?Q?k0nz3iq11MClq6mHoBrTGfWT9b6av6sREUwAe2Z49OkGQoHMj6ukuDDDRxnK?=
 =?us-ascii?Q?Q547vNmqKVg+yjrT+dEw3lObJ0np9lYIN7IOoaxWfFg8kGgMrWcD4NzQp869?=
 =?us-ascii?Q?9cTAAmdEsrYoy2EClrzBAukt6Uk+6qvzK2YQKKDPK2TyJpqZC6+RZ6AUHUuj?=
 =?us-ascii?Q?lSCaQF2bREm+coiOakD70z6G0dvrsd/qGLiXHQgFoe2EBxF5paVz37ErCS+3?=
 =?us-ascii?Q?b0EsqitdmW7qJ7cg8DT6HZ+68+3ED5uoCD9EzRFzEksLAhJAEL43sus7qSxb?=
 =?us-ascii?Q?vXiaCZjpA8fI07AvyD8ANl7y88oLrKGckY0zmCFZeEHu1bKpV5H1MAkKiPq0?=
 =?us-ascii?Q?Z/+K9YcphQ+I1/Kc8tWCZBkDD3D74XK5/bH9ZRLY3VN+B0n3a+ZN9TBYItg3?=
 =?us-ascii?Q?+xMT3g7cCEaFSNoJI80KSWybS26RnbWH6h5d93MFqO57c8VLE6xs7hTXZMYB?=
 =?us-ascii?Q?wcI1lgoerMAv+Dtfz3Mfo7pHbx2eo8boUBfB32ZyhZyDjgw/hppuAPo2jt8S?=
 =?us-ascii?Q?O2dPl7eHgw3kf45LF+B3FbPMKGxnxOVD4zdW7iEDnzMu5dtY5t8tsWygYnep?=
 =?us-ascii?Q?2v5jAZgmitk1TU31HKNrVUpuklrd4daPUwNb4nAoRAE7OhpFNxoLS9Kj8v3Z?=
 =?us-ascii?Q?faN3wQcXxH9F2KB1DP05CjdqGSQp7E1uvi5BrlIhzV054KhCJrc1EXaayhl6?=
 =?us-ascii?Q?C1bCNb6Wf7+XCHw3E01lyFWG5mVCbhLAv9bNFhh6r8Sm3OnvHaTwPJtlvcfN?=
 =?us-ascii?Q?s90R/eIHUT6LanbjBzNksfUskpH877w+3znSa/ED1hRNpM2NvHREiT54cO9P?=
 =?us-ascii?Q?oHq2RE8P0fIPVWrhoYWTxWRmiV6gJ6RmN15n0RCpGaaRgTyboa/ZuKeLIT8K?=
 =?us-ascii?Q?pQHn28xwa2sjdfF/TZlx8AnwuFM5DWiu8HbS8mEtWz19wGiaWRRpUGGafIoH?=
 =?us-ascii?Q?NpX5riR1/4vSETPFfbxoYOaIS1sBiamPOiWKK5EORSyl4lNn5vhffDp8UyZ1?=
 =?us-ascii?Q?FhhZvBE/v+uIpPHFeZYBeLJkUfSzUzUSjkF1FEyf8ew0Gl5s5GvwvGRc9NRf?=
 =?us-ascii?Q?exM4YzR6NUfjAFFu9LwmvLtXo287jEmZiCcFE0rHFcwmfIZ0VMsasCwIkudE?=
 =?us-ascii?Q?mIosvE3MlPXhO+vXF+A+OWd0GFTNaRPqrQ2zTtkKKMh5s0lyo+GxSyZy7UUV?=
 =?us-ascii?Q?3P/gLWNTKhyUFgmj6m+DqBwwChxNc03JHMc1tF7tVmmv3gqpKmJ1RY9shzNj?=
 =?us-ascii?Q?mfNR07AAzNHSQmvObuF+NLtaqG+bDUfr+yCJSK1FKaFOyVhbT60tlLa9Uvtx?=
 =?us-ascii?Q?g5NX2EIu2+AxxxPyPHnZ/Fl+G0FjexDWNUevFfNkKvICuGDHmqpv3XQ9Nmkq?=
 =?us-ascii?Q?2RJ5XR+uekodrUeaOiuJb+Qykk1PEro2YXEDJSJxVpt9lxGXKQuNysTA0GuV?=
 =?us-ascii?Q?k7f/ehlr/YQuHhiQv13K6fsizQVtpKd2E14BrHW45GC+isOPLB/XPquzJwT1?=
 =?us-ascii?Q?cBu6mvnonj0pCUowwqvHkawF6gE2QM9E9Bmm2vR7h5UFRlJhVOpxWcdX2Ajn?=
 =?us-ascii?Q?LQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4A0885BBA15E6D43A297583C8EF90403@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0:
	tRCx+HM+fTMW5l63VUx/WTLr1QSuCSmQOjkq12c/v2qBlPaNYpTK7cBghYAMlzyt5qyT+GD5LijzRyALH0ZGjkvPMbkMguMtM3vAtjtEywPUW3/+q/etYjdfydo0JTLGNyFsMtQLeeI2CNCjOUJtZw5g1M2HkrTTcGE172MDDJ4bmuozULNR2OBk/3OfIYqVPvbOItHIgsB9+QLhm1E2tpz81P2a4JJQW6w5EoBvkxlWIlxaJ3gIrkH0h79dlBAcdj/AtwIOC614/uwtSTWfjXebzM3rXoARluVvQFYTlFOO/hCzMmgddgQ3zK7mAPAV3TbZ13kRZ3L99A3PyuGQjpo+vTvXNdr68TPJeFl0oozLFZFFCaIGDKfwdY+IAiaf2OPyRGmiaqSlnxEaERVhMUdO0aWqecS+4waTXlJvcx7WfxmOcdL1KT71KsIlRl9D79Hp6nxm40QSIeAzrbEFZSv410WL6QfTjERykQ6blI2nQav5354yL61uNrnz2fWnvDtB8yCBy5KpAW9tOfKAoaULpZFZYhmdK6kS3m0cAcAm7oXr0kPEhmapVqs29cgv6papd0Ltd3uMXTGumApP//XhtJf0dmZI8H3TO/vI9rlJED1wEiQMeVvw2cRSMgzq
X-OriginatorOrg: wdc.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR04MB7776.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3ea63925-de9d-42c5-95e5-08dc1b5e8b22
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Jan 2024 15:26:45.4695
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: iifPGURNbp+0WzSjgSHCISD9418UGQqUxqvYGz97h3qxv6uLMB7Y15xrjpx/BfRCTj4oxoJmKaE9E5GudCHP2w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR04MB8136

On Mon, Jan 22, 2024 at 02:43:03PM +0000, Johannes Thumshirn wrote:
> On 22.01.24 15:39, Naohiro Aota wrote:
> >>>
> >>> Also, looking into btrfs_zoned_should_reclaim(), it sums device->byte=
s_used
> >>> for each fs_devices->devices. And, device->bytes_used is set at
> >>> create_chunk() or at btrfs_remove_chunk(). Isn't it feasible to do th=
e
> >>> calculation only there?
> >>
> >> Oh sh*t! Right we should check bytes_used from all space_infos in
> >> btrfs_zoned_should_reclaim() and compare that to the disk total bytes.
> >=20
> > You mean device->bytes_used? space_info->bytes_used does not count free
> > space and zone_unusable in BGs, so using that changes the behavior. Eve=
n,
> > it won't kick the thread if there are many zone_unusable but small used
> > space.
> >=20
>=20
> I did mean btrfs_space_info_used():
>=20
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index b7e7b5a5a6fa..d5242c96c97c 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -2414,6 +2414,7 @@ bool btrfs_zoned_should_reclaim(struct=20
> btrfs_fs_info *fs_info)
>   {
>          struct btrfs_fs_devices *fs_devices =3D fs_info->fs_devices;
>          struct btrfs_device *device;
> +       struct btrfs_space_info *space_info;
>          u64 used =3D 0;
>          u64 total =3D 0;
>          u64 factor;
> @@ -2429,10 +2430,15 @@ bool btrfs_zoned_should_reclaim(struct=20
> btrfs_fs_info *fs_info)
>                          continue;
>=20
>                  total +=3D device->disk_total_bytes;
> -               used +=3D device->bytes_used;
>          }
>          rcu_read_unlock();
>=20
> +       list_for_each_entry(space_info, &fs_info->space_info, list) {
> +               spin_lock(&space_info->lock);
> +               used +=3D btrfs_space_info_used(space_info, true);
> +               spin_unlock(&space_info->lock);
> +       }
> +
>          factor =3D div64_u64(used * 100, total);
>          return factor >=3D fs_info->bg_reclaim_threshold;
>   }
>=20

This changes the behavior. btrfs_space_info_used() excludes unallocated
space.

Also, if we calculate it with device->disk_total_bytes, it screws up on
DUP/RAID* profile because btrfs_space_info_used() counts the logical space
vs disk_total_bytes counts the physical space.=