On Thu, Nov 07, 2019 at 10:25:42AM -0800, Christoph Hellwig wrote:
> > bp = xfs_btree_get_bufs(tp->t_mountp, tp, agno, agbno);
> > - if (!bp) {
> > - XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, tp->t_mountp);
> > + if (XFS_IS_CORRUPT(tp->t_mountp, !bp)) {
> > return -EFSCORRUPTED;
> > }
>
> We can kill the braces here now. Same for various other spots later
> down.
>
> > + if (XFS_IS_CORRUPT(mp,
> > + ir.loaded != XFS_IFORK_NEXTENTS(ip, whichfork))) {
>
> Somewhat strange indentation here.
>
> > ASSERT(map && *map);
> > @@ -2566,14 +2551,16 @@ xfs_dabuf_map(
> > nirecs = 1;
> > }
> >
> > - if (!xfs_da_map_covers_blocks(nirecs, irecs, bno, nfsb)) {
> > - /* Caller ok with no mapping. */
> > - if (mappedbno == -2) {
> > - error = -1;
> > - goto out;
> > - }
> > + covers_blocks = xfs_da_map_covers_blocks(nirecs, irecs, bno, nfsb);
> > +
> > + /* Caller ok with no mapping. */
> > + if (mappedbno == -2 && !covers_blocks) {
> > + error = -1;
> > + goto out;
> > + }
> >
> > - /* Caller expected a mapping, so abort. */
> > + /* Caller expected a mapping, so abort. */
> > + if (XFS_IS_CORRUPT(mp, !covers_blocks)) {
>
> Why the restructure here?
>
> This could have just become:
>
> if (!XFS_IS_CORRUPT(mp != -2)) {
> error = -1;
> goto out;
> }
>
> not that I really like the current structure, but that change seems bit
> out of place in these semi-mechanical fixups, and once we touch the
> structure of this function and its callers there is so much more to
> fix..
>
> > index 7b845c052fb4..e1b9de6c7437 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -87,6 +87,10 @@ struct xfs_ifork {
> > #define XFS_IFORK_MAXEXT(ip, w) \
> > (XFS_IFORK_SIZE(ip, w) / sizeof(xfs_bmbt_rec_t))
> >
> > +#define XFS_IFORK_MAPS_BLOCKS(ip, w) \
> > + (XFS_IFORK_FORMAT((ip), (w)) == XFS_DINODE_FMT_EXTENTS || \
> > + XFS_IFORK_FORMAT((ip), (w)) == XFS_DINODE_FMT_BTREE)
>
> Why the double indentation? Also maybe XFS_IFORK_FORMAT_MAPS_BLOCKS
> is a better name? Or maybe even turn it into an inline function with
> a less shouting name? Also the addition of this helper is probably
> worth being split into a separate patch.
Ugh. I tried writing this as a static inline function but gcc then
tries to parse the function, which requires struct xfs_inode to be
defined prior to the helper. That in turn trips over files that include
xfs_inode_fork.h before they include xfs_inode.h, which is exacerbated
by struct xfs_inode requiring struct xfs_ifork which means that it's a
circular dependency mess.
So for now it's a macro because that's probably more efficient than
requiring a function call for a two line predicate, and less confusing
than stuffing it in xfs_inode.h.
Dave suggested xfs_ifork_has_extents as a shorter name.
--D
> > + head_block >= tail_block || head_cycle != (tail_cycle + 1)))
>
> no need for the inner most braces here if you touch the line anyway.
