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

[Mark Harmstone <maharmstone@meta.com>]

Thanks David.

On 21/10/24 14:50, David Sterba wrote:
>> +static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
>> +				   u64 start, u64 lockend,
>> +				   struct extent_state *cached_state,
>> +				   u64 disk_bytenr, u64 disk_io_size,
>> +				   size_t count, bool compressed,
>> +				   struct iovec *iov,
>> +				   struct io_uring_cmd *cmd)
>> +{
>> +	struct btrfs_inode *inode = BTRFS_I(file_inode(iocb->ki_filp));
>> +	struct extent_io_tree *io_tree = &inode->io_tree;
>> +	struct page **pages;
>> +	struct btrfs_uring_priv *priv = NULL;
>> +	unsigned long nr_pages;
>> +	int ret;
>> +
>> +	nr_pages = DIV_ROUND_UP(disk_io_size, PAGE_SIZE);
>> +	pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS);
>> +	if (!pages)
>> +		return -ENOMEM;
>> +	ret = btrfs_alloc_page_array(nr_pages, pages, 0);
> 
> The allocation sizes are derived from disk_io_size that comes from the
> outside, potentially making large allocatoins. Or is there some inherent
> limit on the maximu size?

Yes. It comes from btrfs_encoded_read, where it's limited to 
BTRFS_MAX_UNCOMPRESSED (i.e. 128KB).

>> +	if (ret) {
>> +		ret = -ENOMEM;
>> +		goto fail;
>> +	}
>> +
>> +	priv = kmalloc(sizeof(*priv), GFP_NOFS);
>> +	if (!priv) {
>> +		ret = -ENOMEM;
>> +		goto fail;
>> +	}
>> +
>> +	priv->iocb = *iocb;
>> +	priv->iov = iov;
>> +	priv->iter = *iter;
>> +	priv->count = count;
>> +	priv->cmd = cmd;
>> +	priv->cached_state = cached_state;
>> +	priv->compressed = compressed;
>> +	priv->nr_pages = nr_pages;
>> +	priv->pages = pages;
>> +	priv->start = start;
>> +	priv->lockend = lockend;
>> +
>> +	ret = btrfs_encoded_read_regular_fill_pages(inode, start, disk_bytenr,
>> +						    disk_io_size, pages,
>> +						    btrfs_uring_read_extent_cb,
>> +						    priv);
>> +	if (ret)
>> +		goto fail;
>> +
>> +	return -EIOCBQUEUED;
>> +
>> +fail:
>> +	unlock_extent(io_tree, start, lockend, &cached_state);
>> +	btrfs_inode_unlock(inode, BTRFS_ILOCK_SHARED);
>> +	kfree(priv);
> 
> Does this leak pages and priv->pages?

No, they get freed in btrfs_uring_read_finished.

>> +	return ret;
>> +}
>> +
>> +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?

Mark