Re: [PATCH v4 5/8] block: implement async discard as io_uring cmd

[Christoph Hellwig <hch@infradead.org>]

On Tue, Sep 10, 2024 at 11:58:23AM +0100, Pavel Begunkov wrote:
> > Based on the above this function is misnamed, as it validates sector_t
> > range and not a byte range.
> 
> Start and len here are in bytes. What do you mean?

You are right, sorry.

> > > +
> > > +	err = filemap_invalidate_pages(bdev->bd_mapping, start,
> > > +					start + len - 1, nowait);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	while ((bio = blk_alloc_discard_bio(bdev, &sector, &nr_sects, gfp))) {
> > > +		if (nowait)
> > > +			bio->bi_opf |= REQ_NOWAIT;
> > > +		prev = bio_chain_and_submit(prev, bio);
> > > +	}
> > > +	if (!prev)
> > > +		return -EAGAIN;
> > 
> > If a user changes the max_discard value between the check above and
> > the loop here this is racy.
> 
> If the driver randomly changes it, it's racy either way. What do
> you want to protect against?

The discard limit shrinking and now this successfully returning while
not actually discarding the range.  The fix is pretty simple in that
the nowait case should simply break out of the loop after the first bio.

> > > +sector_t bio_discard_limit(struct block_device *bdev, sector_t sector);
> > 
> > And to be honest, I'd really prefer to not have bio_discard_limit
> > exposed.  Certainly not outside a header private to block/.
> 
> Which is the other reason why first versions were putting down
> a bio seeing that there is more to be done for nowait, which
> you didn't like. I can return back to it or narrow the scopre.

The above should also take care of that.

> 
> > Also why start at 137?  A comment
> > would generally be pretty useful as well.
> 
> There is a comment, 2 lines above the new define.
> 
> /*
>  * A jump here: 130-136 are reserved for zoned block devices
>  * (see uapi/linux/blkzoned.h)
>  */
> 
> Is that your concern?

But those are ioctls, this is not an ioctl and uses a different
number space.  Take a look at e.g. nvme uring cmds which also
don't try to use the same number space as the ioctl.

> > Also can we have a include/uapi/linux/blkdev.h for this instead of
> > bloating fs.h that gets included just about everywhere?
> I don't think it belongs to this series.

How would adding a proper header instead of bloating fs.h not be
part of the series adding the first ever block layer uring_cmds?
Just in case I wasn't clear - this isn't asking you to move anything
existing as we can't do that without breaking existing applications.
It is about adding the new command to the proper place.