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

Pavel Begunkov
2024-10-21T16:15:13Z

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