| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2da95607-9b21-4d21-8926-9463021a6f33@gmail.com>
Date: Wed, 26 Nov 2025 17:14:44 +0800
From: Yongpeng Yang <yangyongpeng.storage@...il.com>
To: Chaitanya Kulkarni <chaitanyak@...dia.com>,
Yongpeng Yang <yangyongpeng.storage@...il.com>,
Chaitanya Kulkarni <ckulkarnilinux@...il.com>,
"axboe@...nel.dk" <axboe@...nel.dk>, "agk@...hat.com" <agk@...hat.com>,
"snitzer@...nel.org" <snitzer@...nel.org>,
"mpatocka@...hat.com" <mpatocka@...hat.com>,
"song@...nel.org" <song@...nel.org>, "yukuai@...as.com" <yukuai@...as.com>,
"hch@....de" <hch@....de>, "sagi@...mberg.me" <sagi@...mberg.me>,
"jaegeuk@...nel.org" <jaegeuk@...nel.org>, "chao@...nel.org"
<chao@...nel.org>, "cem@...nel.org" <cem@...nel.org>
Cc: "dm-devel@...ts.linux.dev" <dm-devel@...ts.linux.dev>,
"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
Johannes Thumshirn <johannes.thumshirn@....com>,
Yongpeng Yang <yangyongpeng@...omi.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
"linux-f2fs-devel@...ts.sourceforge.net"
<linux-f2fs-devel@...ts.sourceforge.net>,
"linux-raid@...r.kernel.org" <linux-raid@...r.kernel.org>,
"bpf@...r.kernel.org" <bpf@...r.kernel.org>,
"linux-xfs@...r.kernel.org" <linux-xfs@...r.kernel.org>
Subject: Re: [f2fs-dev] [PATCH V3 6/6] xfs: ignore discard return value
On 11/26/25 16:07, Chaitanya Kulkarni via Linux-f2fs-devel wrote:
> On 11/25/25 18:37, Yongpeng Yang wrote:
>>> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
>>> index 6917de832191..b6ffe4807a11 100644
>>> --- a/fs/xfs/xfs_discard.c
>>> +++ b/fs/xfs/xfs_discard.c
>>> @@ -108,7 +108,7 @@ xfs_discard_endio(
>>> * list. We plug and chain the bios so that we only need a single
>>> completion
>>> * call to clear all the busy extents once the discards are complete.
>>> */
>>> -int
>>> +void
>>> xfs_discard_extents(
>>> struct xfs_mount *mp,
>>> struct xfs_busy_extents *extents)
>>> @@ -116,7 +116,6 @@ xfs_discard_extents(
>>> struct xfs_extent_busy *busyp;
>>> struct bio *bio = NULL;
>>> struct blk_plug plug;
>>> - int error = 0;
>>> blk_start_plug(&plug);
>>> list_for_each_entry(busyp, &extents->extent_list, list) {
>>> @@ -126,18 +125,10 @@ xfs_discard_extents(
>>> trace_xfs_discard_extent(xg, busyp->bno, busyp->length);
>>> - error = __blkdev_issue_discard(btp->bt_bdev,
>>> + __blkdev_issue_discard(btp->bt_bdev,
>>> xfs_gbno_to_daddr(xg, busyp->bno),
>>> XFS_FSB_TO_BB(mp, busyp->length),
>>> GFP_KERNEL, &bio);
>>
>> If blk_alloc_discard_bio() fails to allocate a bio inside
>> __blkdev_issue_discard(), this may lead to an invalid loop in
>> list_for_each_entry{}. Instead of using __blkdev_issue_discard(), how
>> about allocate and submit the discard bios explicitly in
>> list_for_each_entry{}?
>>
>> Yongpeng,
>
>
> Calling __blkdev_issue_discard() keeps managing all the bio with the
> appropriate GFP mask, so the semantics stay the same. You are just
> moving memory allocation to the caller and potentially looking at
> implementing retry on bio allocation failure.
>
> The retry for discard bio memory allocation is not desired I think,
> since it's only a hint to the controller.
Agreed. I'm not trying to retry bio allocation inside the
list_for_each_entry{} loop. Instead, since blk_alloc_discard_bio()
returning NULL cannot reliably indicate whether the failure is due to
bio allocation failure, it could also be caused by 'bio_sects == 0', I'd
like to allocate the bio explicitly.
>
> This patch is simply cleaning up dead error-handling branches at the
> callers no behavioral changes intended.
>
> What maybe useful is to stop iterating once we fail to allocate the
> bio [1].
>
> -ck
>
> [1] Potential addition on the top of this to exit early in discard loop
> on bio allocation failure.
>
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index b6ffe4807a11..1519f708bb79 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -129,6 +129,13 @@ xfs_discard_extents(
> xfs_gbno_to_daddr(xg, busyp->bno),
> XFS_FSB_TO_BB(mp, busyp->length),
> GFP_KERNEL, &bio);
> + /*
> + * We failed to allocate bio instead of continuing the loop
> + * so it will lead to inconsistent discards to the disk
> + * exit early and jump into xfs_discard_busy_clear().
> + */
> + if (!bio)
> + break;
I noticed that as long as XFS_FSB_TO_BB(mp, busyp->length) is greater
than 0 and there is no bio allocation failure, __blkdev_issue_discard()
will never return NULL. I'm not familiar with this part of the xfs, so
I'm not sure whether there are cases where 'XFS_FSB_TO_BB(mp,
busyp->length)' could be 0. If such cases do not exist, then
checking whether the bio is NULL should be sufficient.
Yongpeng,
> }
>
> if (bio) {
> > If we keep looping after the first bio == NULL, the rest of the range is
> guaranteed to be inconsistent anyways, because every subsequent iteration
> will fall into one of three cases:
>
> - The allocator keeps returning NULL, so none of the remaining LBAs receive
> discard.
> - Rest of the allocator succeeds, but we’ve already skipped a chunk, leaving
> a hole in the discard range.
> - We get intermittent successes, which produces alternating chunks of
> discarded and undiscarded blocks.
>
> In each of those scenarios, the disk ends up with a partially discarded
> range, so the correct fix is to break out of the loop immediately and
> proceed to xfs_discard_busy_clear() once the very first allocation fails.
Powered by blists - more mailing lists