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

[Mark Harmstone <maharmstone@meta.com>]

On 21/10/24 19:23, David Sterba wrote:
> > 
> On Mon, Oct 21, 2024 at 05:05:20PM +0000, Mark Harmstone wrote:
>>>> +static int btrfs_uring_encoded_read(struct io_uring_cmd *cmd,
>>>> +				    unsigned int issue_flags)
>>>> +{
>>>> +	size_t copy_end_kernel = offsetofend(struct btrfs_ioctl_encoded_io_args,
>>>> +					     flags);
>>>> +	size_t copy_end;
>>>> +	struct btrfs_ioctl_encoded_io_args args = {0};
>>>                                                   = { 0 }
>>>> +	int ret;
>>>> +	u64 disk_bytenr, disk_io_size;
>>>> +	struct file *file = cmd->file;
>>>> +	struct btrfs_inode *inode = BTRFS_I(file->f_inode);
>>>> +	struct btrfs_fs_info *fs_info = inode->root->fs_info;
>>>> +	struct extent_io_tree *io_tree = &inode->io_tree;
>>>> +	struct iovec iovstack[UIO_FASTIOV];
>>>> +	struct iovec *iov = iovstack;
>>>> +	struct iov_iter iter;
>>>> +	loff_t pos;
>>>> +	struct kiocb kiocb;
>>>> +	struct extent_state *cached_state = NULL;
>>>> +	u64 start, lockend;
>>>
>>> The stack consumption looks quite high.
>>
>> 696 bytes, compared to 672 in btrfs_ioctl_encoded_read.
>> btrfs_ioctl_encoded write is pretty big too. Probably the easiest thing
>> here would be to allocate btrfs_uring_priv early and pass that around, I
>> think.
>>
>> Do you have a recommendation for what the maximum stack size of a
>> function should be?
> 
> It depends from where the function is called. For ioctl callbacks, like
> btrfs_ioctl_encoded_read it's the first function using kernel stack
> leaving enough for any deep IO stacks (DM/NFS/iSCSI/...). If something
> similar applies to the io_uring callbacks then it's probably fine.

Thanks. Yes, the two should functions should be broadly equivalent.

> Using a separate off-stack structure works but it's a penalty as it
> needs the allcation. The io_uring is meant for high performance so if
> the on-stack allocation is safe then keep it like that.

Okay, I'll leave this bit as it is, then. I can revisit it if we start 
getting a spike of stack overflow crashes mentioning 
btrfs_uring_encoded_read.

> 
> I've checked on a release config the stack consumption and the encoded
> ioctl functions are not the worst:
> 
> tree-log.c:btrfs_sync_log                       728 static
> scrub.c:scrub_verify_one_metadata               552 dynamic,bounded
> inode.c:print_data_reloc_error                  544 dynamic,bounded
> uuid-tree.c:btrfs_uuid_scan_kthread             520 static
> tree-checker.c:check_root_item                  504 static
> file-item.c:btrfs_csum_one_bio                  496 static
> inode.c:btrfs_start_delalloc_roots              488 static
> scrub.c:scrub_raid56_parity_stripe              464 dynamic,bounded
> disk-io.c:write_dev_supers                      464 static
> ioctl.c:btrfs_ioctl_encoded_write               456 dynamic,bounded
> ioctl.c:btrfs_ioctl_encoded_read                456 dynamic,bounded