On Mon, Jun 20, 2016 at 10:21:07AM +1000, Dave Chinner wrote:
> On Thu, Jun 16, 2016 at 06:18:30PM -0700, Darrick J. Wong wrote:
> > Port various differences between xfsprogs and the kernel. This
> > cleans up both so that we can develop rmap and reflink on the
> > same libxfs code.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> Nak. I'm essentially trying to keep the little hacks needed in
> userspace out of the kernel libxfs tree. We quite regularly get
> people scanning the kernel tree and trying to remove things like
> exported function prototypes that are not used in kernel space,
> so the headers in userspace carry those simply to prevent people
> continually sending kernel patches that we have to look at and then
> ignore...
Fair enough, I merely diff'd the two libxfs and figured I'd remove
all the differences to try to develop atop as close to identical libxfs as I
could get. :)
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index 99b077c..58bdca7 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -2415,7 +2415,9 @@ xfs_alloc_read_agf(
> > be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNTi]);
> > spin_lock_init(&pag->pagb_lock);
> > pag->pagb_count = 0;
> > +#ifdef __KERNEL__
> > pag->pagb_tree = RB_ROOT;
> > +#endif
> > pag->pagf_init = 1;
> > }
> > #ifdef DEBUG
>
> e.g. this is an indication that reminds us that there is
> functionality in the libxfs kernel tree that isn't in userspace...
>
> > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h
> > index 4f2aed0..8ef420a 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.h
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.h
> > @@ -51,7 +51,7 @@ int xfs_attr_shortform_getvalue(struct xfs_da_args
> > *args);
> > int xfs_attr_shortform_to_leaf(struct xfs_da_args *args);
> > int xfs_attr_shortform_remove(struct xfs_da_args *args);
> > int xfs_attr_shortform_allfit(struct xfs_buf *bp, struct xfs_inode
> > *dp);
> > -int xfs_attr_shortform_bytesfit(xfs_inode_t *dp, int bytes);
> > +int xfs_attr_shortform_bytesfit(struct xfs_inode *dp, int bytes);
> > void xfs_attr_fork_remove(struct xfs_inode *ip, struct xfs_trans
> > *tp);
>
> Things like this are fine...
Ok.
> >
> > /*
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 932381c..499e980 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -1425,7 +1425,7 @@ xfs_bmap_search_multi_extents(
> > * Else, *lastxp will be set to the index of the found
> > * entry; *gotp will contain the entry.
> > */
> > -STATIC xfs_bmbt_rec_host_t * /* pointer to found extent
> > entry */
> > +xfs_bmbt_rec_host_t * /* pointer to found extent entry */
> > xfs_bmap_search_extents(
> > xfs_inode_t *ip, /* incore inode pointer */
> > xfs_fileoff_t bno, /* block number searched for */
> > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> > index 423a34e..79e3ebe 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.h
> > +++ b/fs/xfs/libxfs/xfs_bmap.h
> > @@ -231,4 +231,10 @@ int xfs_bmap_shift_extents(struct xfs_trans *tp,
> > struct xfs_inode *ip,
> > int num_exts);
> > int xfs_bmap_split_extent(struct xfs_inode *ip, xfs_fileoff_t
> > split_offset);
> >
> > +struct xfs_bmbt_rec_host *
> > + xfs_bmap_search_extents(struct xfs_inode *ip, xfs_fileoff_t bno,
> > + int fork, int *eofp, xfs_extnum_t *lastxp,
> > + struct xfs_bmbt_irec *gotp,
> > + struct xfs_bmbt_irec *prevp);
> > +
> > #endif /* __XFS_BMAP_H__ */
>
> But these are the sort of "clean up the kernel patches" that I was
> refering to. If there's a user in kernel space, then fine, otherwise
> it doesn't hurt to keep it only in userspace. There are relatively
> few of these....
>
> > diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> > index 1f88e1c..105979d 100644
> > --- a/fs/xfs/libxfs/xfs_btree.c
> > +++ b/fs/xfs/libxfs/xfs_btree.c
> > @@ -2532,6 +2532,7 @@ error0:
> > return error;
> > }
> >
> > +#ifdef __KERNEL__
> > struct xfs_btree_split_args {
> > struct xfs_btree_cur *cur;
> > int level;
> > @@ -2609,6 +2610,9 @@ xfs_btree_split(
> > destroy_work_on_stack(&args.work);
> > return args.result;
> > }
> > +#else /* !KERNEL */
> > +#define xfs_btree_split __xfs_btree_split
> > +#endif
>
> Same again -this is 4 lines of code that is userspace only. It's a
> tiny amount compared to the original difference that these
> kernel-only stack splits required, and so not a huge issue.
Will drop these two.
> > --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> > +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> > @@ -31,10 +31,16 @@
> > #include "xfs_cksum.h"
> > #include "xfs_trace.h"
> >
> > +/*
> > + * XXX: kernel implementation causes ndquots calc to go real
> > + * bad. Just leaving the existing userspace calc here right now.
> > + */
> > int
> > xfs_calc_dquots_per_chunk(
> > unsigned int nbblks) /* basic block units */
> > {
> > +#ifdef __KERNEL__
> > + /* kernel code that goes wrong in userspace! */
> > unsigned int ndquots;
> >
> > ASSERT(nbblks > 0);
> > @@ -42,6 +48,10 @@ xfs_calc_dquots_per_chunk(
> > do_div(ndquots, sizeof(xfs_dqblk_t));
> >
> > return ndquots;
> > +#else
> > + ASSERT(nbblks > 0);
> > + return BBTOB(nbblks) / sizeof(xfs_dqblk_t);
> > +#endif
> > }
>
> This is a clear case that we need to fix the code to be
> correct for both kernel and userspace without modification, not
> propagate the userspace hack back into the kernel code.
I /think/ it does this because libxfs/libxfs_priv.h's __do_div expects
to be passed a pointer to an unsigned long long (which is later dereferenced
and used as an unsigned long), whereas ndquots is an int?
I'm not sure why we need do_div either, since AFAICT we only ever process
quota in chunks of 1FSB, for which 32-bit division should be fine.
> > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > index 9d9559e..794fa66 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > @@ -56,6 +56,17 @@ xfs_inobp_check(
> > }
> > #endif
> >
> > +bool
> > +xfs_dinode_good_version(
> > + struct xfs_mount *mp,
> > + __u8 version)
> > +{
> > + if (xfs_sb_version_hascrc(&mp->m_sb))
> > + return version == 3;
> > +
> > + return version == 1 || version == 2;
> > +}
>
> This xfs_dinode_good_version() change needs to be a separate patch
Ok.
> > void xfs_inobp_check(struct xfs_mount *, struct xfs_buf *);
> > #else
> > diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> > index e8f49c0..e5baba3 100644
> > --- a/fs/xfs/libxfs/xfs_log_format.h
> > +++ b/fs/xfs/libxfs/xfs_log_format.h
> > @@ -462,8 +462,8 @@ static inline uint xfs_log_dinode_size(int version)
> > typedef struct xfs_buf_log_format {
> > unsigned short blf_type; /* buf log item type indicator */
> > unsigned short blf_size; /* size of this item */
> > - ushort blf_flags; /* misc state */
> > - ushort blf_len; /* number of blocks in this buf */
> > + unsigned short blf_flags; /* misc state */
> > + unsigned short blf_len; /* number of blocks in this buf */
> > __int64_t blf_blkno; /* starting blkno of this buf */
> > unsigned int blf_map_size; /* used size of data bitmap in words */
> > unsigned int blf_data_map[XFS_BLF_DATAMAP_SIZE]; /* dirty bitmap */
>
> The removal of ushort/uint from the kernel code needs to be a
> separate patch that addresses all the users, not just the couple in
> shared headers....
Ok.
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 12ca867..09d6fd0 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -261,6 +261,7 @@ xfs_mount_validate_sb(
> > /*
> > * Until this is fixed only page-sized or smaller data blocks work.
> > */
> > +#ifdef __KERNEL__
> > if (unlikely(sbp->sb_blocksize > PAGE_SIZE)) {
> > xfs_warn(mp,
> > "File system with blocksize %d bytes. "
> > @@ -268,6 +269,7 @@ xfs_mount_validate_sb(
> > sbp->sb_blocksize, PAGE_SIZE);
> > return -ENOSYS;
> > }
> > +#endif
> >
> > /*
> > * Currently only very few inode sizes are supported.
> > @@ -291,10 +293,12 @@ xfs_mount_validate_sb(
> > return -EFBIG;
> > }
> >
> > +#ifdef __KERNEL__
> > if (check_inprogress && sbp->sb_inprogress) {
> > xfs_warn(mp, "Offline file system operation in progress!");
> > return -EFSCORRUPTED;
> > }
> > +#endif
> > return 0;
> > }
>
> Again, I don't think this needs to be propagated back into the
> kernel code...
Will drop.
--D
>
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
|