Re: [PATCH v2 5/7] block: implement async discard as io_uring cmd

[Christoph Hellwig <hch@infradead.org>]

On Thu, Aug 22, 2024 at 04:35:55AM +0100, Pavel Begunkov wrote:
> io_uring allows to implement custom file specific operations via
> fops->uring_cmd callback. Use it to wire up asynchronous discard
> commands. Normally, first it tries to do a non-blocking issue, and if
> fails we'd retry from a blocking context by returning -EAGAIN to
> core io_uring.
> 
> Note, unlike ioctl(BLKDISCARD) with stronger guarantees against races,
> we only do a best effort attempt to invalidate page cache, and it can
> race with any writes and reads and leave page cache stale. It's the
> same kind of races we allow to direct writes.

Can you please write up a man page for this that clear documents the
expecvted semantics?

> +static void bio_cmd_end(struct bio *bio)

This is really weird function name.  blk_cmd_end_io or
blk_cmd_bio_end_io would be the usual choices.

> +	while ((bio = blk_alloc_discard_bio(bdev, &sector, &nr_sects,
> +					    GFP_KERNEL))) {

GFP_KERNEL can often will block.  You'll probably want a GFP_NOWAIT
allocation here for the nowait case.

> +		if (nowait) {
> +			/*
> +			 * Don't allow multi-bio non-blocking submissions as
> +			 * subsequent bios may fail but we won't get direct
> +			 * feedback about that. Normally, the caller should
> +			 * retry from a blocking context.
> +			 */
> +			if (unlikely(nr_sects)) {
> +				bio_put(bio);
> +				return -EAGAIN;
> +			}
> +			bio->bi_opf |= REQ_NOWAIT;
> +		}

And this really looks weird.  It first allocates a bio, potentially
blocking, and then gives up?  I think you're much better off with
something like:

	if (nowait) {
		if (nr_sects > bio_discard_limit(bdev, sector))
			return -EAGAIN;
		bio = blk_alloc_discard_bio(bdev, &sector, &nr_sects,
						    GFP_NOWAIT);
		if (!bio)
			return -EAGAIN
		bio->bi_opf |= REQ_NOWAIT;
		goto submit;
	}

	/* submission loop here */

> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 753971770733..0016e38ed33c 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			0

Is fs.h the reight place for this?

Curious:  how to we deal with conflicting uring cmds on different
device and how do we probe for them?  The NVMe uring_cmds
use the ioctl-style _IO* encoding which at least helps a bit with
that and which seem like a good idea.  Maybe someone needs to write
up a few lose rules on uring commands?