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

[David Sterba <dsterba@suse.cz>]

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.

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.

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