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
