Re: [PATCH 5/5] btrfs: add io_uring command for encoded reads

[Pavel Begunkov <asml.silence@gmail.com>]

On 10/21/24 14:50, David Sterba wrote:
> On Mon, Oct 14, 2024 at 06:18:27PM +0100, Mark Harmstone wrote:
>> Adds an io_uring command for encoded reads, using the same interface as
> 
> Add ...
> 
>> the existing BTRFS_IOC_ENCODED_READ ioctl.
> 
> This is probably a good summary in a changelog but the patch is quite
> long so it feels like this should be described in a more detail how it's
> done. Connecting two interfaces can be done in various ways, so at least
> mention that it's a simple pass through, or if there are any
> complications regardign locking, object lifetime and such.
> 
>> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
...
>> +
>> +	kfree(priv->pages);
>> +	kfree(priv->iov);
>> +	kfree(priv);
>> +}
>> +
>> +static void btrfs_uring_read_extent_cb(void *ctx, int err)
>> +{
>> +	struct btrfs_uring_priv *priv = ctx;
>> +
>> +	*(uintptr_t*)priv->cmd->pdu = (uintptr_t)priv;
> 
> Isn't there a helper for that? Type casting should be done in justified
> cases and as an exception.

FWIW, I haven't taken a look yet, but for this one, please use
io_uring_cmd_to_pdu(cmd, type), it'll check the size for you.
And there in no need to cast it to uintptr, would be much nicer
to

struct btrfs_cmd {
	struct btrfs_uring_priv *priv;
};

struct btrfs_cmd *bc = io_uring_cmd_to_pdu(priv->cmd, struct btrfs_cmd);
bc->priv = priv;

You get more type checking this way. You can also wrap around
io_uring_cmd_to_pdu() into a static inline helper, if that
looks better.

...>> +
>> +	start = ALIGN_DOWN(pos, fs_info->sectorsize);
>> +	lockend = start + BTRFS_MAX_UNCOMPRESSED - 1;
>> +
>> +	ret = btrfs_encoded_read(&kiocb, &iter, &args,
>> +				 issue_flags & IO_URING_F_NONBLOCK,
>> +				 &cached_state, &disk_bytenr, &disk_io_size);
>> +	if (ret < 0 && ret != -EIOCBQUEUED)
>> +		goto out_free;
>> +
>> +	file_accessed(file);
>> +
>> +	if (copy_to_user((void*)(uintptr_t)cmd->sqe->addr + copy_end,
>> +			 (char *)&args + copy_end_kernel,

Be aware, SQE data is not stable, you should assume that the user
space can change it at any moment. It should be a READ_ONCE, and
likely you don't want it to be read twice, unless you handle it /
verify values / etc. I'd recommend to save it early in the callback
and stash somewhere, e.g. into struct btrfs_cmd I mentioned above.

> 
> So many type casts again
> 


-- 
Pavel Begunkov