| 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 |
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, §or, > + &nr_sects, GFP_NOFS)) > + ; This pattern is repeated often in this series, so perhaps a helper function for this common use case.
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, §or, > > + &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 --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, §or, + &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,
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(-)