Re: [PATCH 9/9] xfs: connect in-memory btrees to xfiles

Christoph Hellwig
2024-01-04T06:54:15Z

On Sun, Dec 31, 2023 at 12:15:54PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Add to our stubbed-out in-memory btrees the ability to connect them with
> an actual in-memory backing file (aka xfiles) and the necessary pieces
> to track free space in the xfile and flush dirty xfbtree buffers on
> demand, which we'll need for online repair.

I guess this is is split from the last patch because of the size of
the changes? Because it feels they relong belong together. Maybe with
my patches for the diet and splitting out the new helpers outside the
btree code these could now become a single commit?

> +#ifdef CONFIG_XFS_BTREE_IN_XFILE
> +static inline unsigned long
> +xfbtree_ino(
> + struct xfbtree *xfbt)
> +{
> + return file_inode(xfbt->target->bt_xfile->file)->i_ino;
> +}
> +#endif /* CONFIG_XFS_BTREE_IN_XFILE */

This should probably move to xfile.h?

> + /* Make sure we actually can write to the block before we return it. */
> + pos = xfo_to_b(bt_xfoff);
> + error = xfile_prealloc(xfbtree_xfile(xfbt), pos, xfo_to_b(1));
> + if (error)
> + return error;

IFF we stick to always backing the buffers directly by the shmem
pages this won't be needed - the btree code does a buf_get right after
calling into ->alloc_blocks that will allocate the page.

> +int
> +xfbtree_free_block(
> + struct xfs_btree_cur *cur,
> + struct xfs_buf *bp)
> +{
> + struct xfbtree *xfbt = cur->bc_mem.xfbtree;
> + xfileoff_t bt_xfoff, bt_xflen;
> +
> + ASSERT(cur->bc_flags & XFS_BTREE_IN_XFILE);
> +
> + bt_xfoff = xfs_daddr_to_xfot(xfs_buf_daddr(bp));
> + bt_xflen = xfs_daddr_to_xfot(bp->b_length);
> +
> + trace_xfbtree_free_block(xfbt, cur, bt_xfoff);
> +
> + return xfboff_bitmap_set(&xfbt->freespace, bt_xfoff, bt_xflen);

Any reason this doesn't actually remove the page from shmem?

> +int
> +xfbtree_trans_commit(
> + struct xfbtree *xfbt,
> + struct xfs_trans *tp)
> +{
> + LIST_HEAD(buffer_list);
> + struct xfs_log_item *lip, *n;
> + bool corrupt = false;
> + bool tp_dirty = false;

Can we have some sort of flag on the xfs_trans structure that marks it
as fake for xfbtree, and assert it gets fed here, and add ansother
assert it desn't get fed to xfs_trans_commit/cancel?

> +/* Discard pages backing a range of the xfile. */
> +void
> +xfile_discard(
> + struct xfile *xf,
> + loff_t pos,
> + u64 count)
> +{
> + trace_xfile_discard(xf, pos, count);
> + shmem_truncate_range(file_inode(xf->file), pos, pos + count - 1);
> +}

This doesn't end up being used.

> +/* Ensure that there is storage backing the given range. */
> +int
> +xfile_prealloc(
> + struct xfile *xf,
> + loff_t pos,
> + u64 count)

If we end up needing this somewhere else in the end (and it really
should be a separate patch), we should be able to replace it with
a simple xfile_get_page/xfile_put_page pair.