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
