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

[Christoph Hellwig <hch@xxxxxxxxxxxxx>]

> +	sector_t sector = start >> SECTOR_SHIFT;
> +	sector_t nr_sects = len >> SECTOR_SHIFT;
> +	struct bio *prev = NULL, *bio;
> +	int err;
> +
> +	if (!bdev_max_discard_sectors(bdev))
> +		return -EOPNOTSUPP;
> +
> +	if (!(file_to_blk_mode(cmd->file) & BLK_OPEN_WRITE))
> +		return -EBADF;
> +	if (bdev_read_only(bdev))
> +		return -EPERM;
> +	err = blk_validate_byte_range(bdev, start, len);
> +	if (err)
> +		return err;

Based on the above this function is misnamed, as it validates sector_t
range and not a byte range.

> +	if (nowait && nr_sects > bio_discard_limit(bdev, sector))
> +		return -EAGAIN;
> +
> +	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.

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

> +
>  #endif /* __LINUX_BIO_H */
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 753971770733..7ea41ca97158 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -208,6 +208,8 @@ struct fsxattr {
>   * (see uapi/linux/blkzoned.h)
>   */
>  
> +#define BLOCK_URING_CMD_DISCARD			_IO(0x12,137)

Whitespace after the comma please.  Also why start at 137?  A comment
would generally be pretty useful as well.

Also can we have a include/uapi/linux/blkdev.h for this instead of
bloating fs.h that gets included just about everywhere?