Re: [PATCH 06/10] ext4: switch to using blk_next_discard_bio directly

From: Keith Busch <kbusch@kernel.org>
To: Christoph Hellwig <hch@lst.de>
Cc: Jens Axboe <axboe@kernel.dk>,
	Chandan Babu R <chandanbabu@kernel.org>,
	linux-block@vger.kernel.org, linux-nvme@lists.infradead.org,
	linux-xfs@vger.kernel.org
Subject: Re: [PATCH 06/10] ext4: switch to using blk_next_discard_bio directly
Date: Thu, 7 Mar 2024 09:13:01 -0700	[thread overview]
Message-ID: <ZennjRhWR2PVtoGU@kbusch-mbp> (raw)
In-Reply-To: <20240307151157.466013-7-hch@lst.de>

On Thu, Mar 07, 2024 at 08:11:53AM -0700, Christoph Hellwig wrote:
> @@ -3840,12 +3840,16 @@ static inline int ext4_issue_discard(struct super_block *sb,
>  	trace_ext4_discard_blocks(sb,
>  			(unsigned long long) discard_block, count);
>  	if (biop) {

Does this 'if' case even need to exist? It looks unreachable since there
are only two callers of ext4_issue_discard(), and they both set 'biop'
to NULL. It looks like the last remaining caller using 'biop' was
removed with 55cdd0af2bc5ffc ("ext4: get discard out of jbd2 commit
kthread contex")

> -		return __blkdev_issue_discard(sb->s_bdev,
> -			(sector_t)discard_block << (sb->s_blocksize_bits - 9),
> -			(sector_t)count << (sb->s_blocksize_bits - 9),
> -			GFP_NOFS, biop);
> -	} else
> -		return sb_issue_discard(sb, discard_block, count, GFP_NOFS, 0);
> +		unsigned int sshift = (sb->s_blocksize_bits - SECTOR_SHIFT);
> +		sector_t sector = (sector_t)discard_block << sshift;
> +		sector_t nr_sects = (sector_t)count << sshift;
> +
> +		while (blk_next_discard_bio(sb->s_bdev, biop, &sector,
> +				&nr_sects, GFP_NOFS))
> +			;

This pattern is repeated often in this series, so perhaps a helper
function for this common use case.