diff mbox series

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

Message ID 20240307151157.466013-7-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [01/10] block: remove the discard_granularity check in __blkdev_issue_discard | expand

Commit Message

Christoph Hellwig March 7, 2024, 3:11 p.m. UTC
This fixes fatal signals getting into the way and corrupting the bio
chain and removes the need to handle synchronous errors.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ext4/mballoc.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Keith Busch March 7, 2024, 4:13 p.m. UTC | #1
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.
Christoph Hellwig March 8, 2024, 3:21 p.m. UTC | #2
On Thu, Mar 07, 2024 at 09:13:01AM -0700, Keith Busch wrote:
> 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")

Yeah.  I didn't really want to dig so far into code I don't know well
for this series, though :)

> > +		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.

Well, it's 2-3 lines vs 1 line for a much better interface.
diff mbox series

Patch

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index e4f7cf9d89c45a..73437510bde26c 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -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) {
-		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))
+			;
+		return 0;
+	}
+	return sb_issue_discard(sb, discard_block, count, GFP_NOFS, 0);
 }
 
 static void ext4_free_data_in_buddy(struct super_block *sb,