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.
