> - if (!(cur->bc_flags & XFS_BTREE_LONG_PTRS) && cur->bc_ag.pag)
> + if (!(cur->bc_flags & XFS_BTREE_LONG_PTRS) &&
> + !(cur->bc_flags & XFS_BTREE_IN_XFILE) && cur->bc_ag.pag)
> xfs_perag_put(cur->bc_ag.pag);
> + if (cur->bc_flags & XFS_BTREE_IN_XFILE) {
> + if (cur->bc_mem.pag)
> + xfs_perag_put(cur->bc_mem.pag);
> + }
Btw, one thing I noticed is that we have a lot of confusion on what
part of the bc_ino/ag/mem union is used for a given btree. For
On-disk inodes we abuse the long ptrs flag, and then we through in
the xfile flags. If you're fine with it I can try to sort it out.
It's not really a blocker, but I think it would be a lot claner if
we used the chance to sort it out. This will become even more
important with the rt rmap/reflink trees that will further increase
the confusion here.
> + if (cur->bc_flags & XFS_BTREE_IN_XFILE)
> + return xfbtree_bbsize();
> + return cur->bc_mp->m_bsize;
> +}
One thing I've been wondering is if we should split
a strut xfs_btree outof struct xfbtree that contains most of the
fields from it minuts the space allocation (and the new fake header
from my patches) and also use that for the on-disk btrees.
That means xfs_btree.c can use the target from it, and the owner
and we can remove the indirect calls for calculcating maxrecs/minrecs,
and then also add a field for the block size like this one and remove
a lof of the XFS_BTREE_IN_XFILE checks.
> + if (cur->bc_flags & XFS_BTREE_IN_XFILE)
> + return 0;
> +
> if ((lr & XFS_BTCUR_LEFTRA) && left != NULLFSBLOCK) {
> xfs_btree_reada_bufl(cur->bc_mp, left, 1,
Should the xfile check go into xfs_buf_readahead instead? That would
execute a little more useles code for in-memory btrees, but keep this
check in one place (where we could also write a nice comment explaining
it :))
> + xfs_btree_buf_to_ptr(cur, bp, &bufptr);
> if (cur->bc_flags & XFS_BTREE_LONG_PTRS) {
> - if (be64_to_cpu(rptr.l) == XFS_DADDR_TO_FSB(cur->bc_mp,
> - xfs_buf_daddr(bp))) {
> + if (rptr.l == bufptr.l) {
> xfs_btree_mark_sick(cur);
> return -EFSCORRUPTED;
> }
> } else {
> - if (be32_to_cpu(rptr.s) == xfs_daddr_to_agbno(cur->bc_mp,
> - xfs_buf_daddr(bp))) {
> + if (rptr.s == bufptr.s) {
This almost screams for a xfs_btree_ptr_cmp helper, even if this
seems to be the only user so far..
> +static inline loff_t xfile_size(struct xfile *xf)
> +{
> + return i_size_read(file_inode(xf->file));
> +}
Despite looking over the whole patch for a while I only noticed
this one, and I think I could add it in my xfile diet series
instea dof open coding it in trace.h.
In general it would be really nice to split out patches that add
infrastructure in other parts of the XFS codebase to make them stick
out a bit more.
> +/* file block (aka system page size) to basic block conversions. */
> +typedef unsigned long long xfileoff_t;
> +#define XFB_BLOCKSIZE (PAGE_SIZE)
> +#define XFB_BSHIFT (PAGE_SHIFT)
> +#define XFB_SHIFT (XFB_BSHIFT - BBSHIFT)
> +
> +static inline loff_t xfo_to_b(xfileoff_t xfoff)
> +{
> + return xfoff << XFB_BSHIFT;
> +}
...
xfile.h feels like the wrong place for this - the encoding only really
makes sense fo the xfbtree. And in a way it feels redundant over
just using pgoff_t and the PAGE_* constants directly which should be
pretty obvious to everyone knowning the Linux MM and page cache APIs.
> +/* Return the number of sectors for a buffer target. */
> +xfs_daddr_t
> +xfs_buftarg_nr_sectors(
> + struct xfs_buftarg *btp)
> +{
> + if (btp->bt_flags & XFS_BUFTARG_XFILE)
> + return xfile_buftarg_nr_sectors(btp);
If we didn't add an ifdef around the struct xfile definition, this could
just be open coded and rely on the compiler eliminating dead code when
XFS_BUFTARG_XFILE isn't defined.
