Re: [PATCH 15/21] implement generic xfs_btree_rshift

[Christoph Hellwig <hch@lst.de>]

> > +{
> > +	ASSERT(from >= 0 && from <= 1000);
> > +	ASSERT(to >= 0 && to <= 1000);
> > +	ASSERT(numptrs >= 0);
> 
> Those numbers are not safe. I plucked them out of thin air to verify
> validity on 4k block size filesystem which had (IIRC) a max of about
> 500 ptrs to a block. It was throwaway debug code to find a problem.
> Larger block sizes can well exceed 1000. So realistically, the only
> valid assert there is this one:
> 
> 	ASSERT(numptrs >= 0);

Yeah, I've actually managed to trigger it now.  The >=0 checks
for from and to still make sense, although they might be overkill.

> How about:
> 
> 		union xfs_btree_ptr	*pp;
> 		xfs_caddr_t		*block = XFS_BUF_TO_BLOCK(bp);
> 		xfs_caddr_t		start;	/* first byte offset logged */
> 		xfs_caddr_t		end;	/* last byte offset logged */
> 
> 		pp = cur->bc_ops->ptr_addr(cur, 1, XFS_BUF_TO_BLOCK(bp));
> 
> 		if (cur->bc_flags & XFS_BTREE_LONG_PTRS) {
> 			__be64		*lpp = &pp->l;
> 
> 			start = (xfs_caddr_t)&lpp[first - 1] - block;
> 			end  = ((xfs_caddr_t)&lpp[last] - 1) - block;
> 		} else {
> 			__be32		*spp = &pp->s;
> 
> 			start = (xfs_caddr_t)&spp[first - 1] - block;
> 			end  = ((xfs_caddr_t)&spp[last] - 1) - block;
> 		}
> 
> 		xfs_trans_log_buf(cur->bc_tp, bp, (int)start, (int)end);
> 
> That makes it much easier to read (to me, anyway).

Yes, absolutely.  And there's also another set of useless braces.
I've also applied a similar cleanup to the log_keys and log_recs
implementations.

> > +	XFS_BTREE_TRACE_CURSOR(cur, XBT_ENTRY);
> > +	XFS_BTREE_TRACE_ARGBI(cur, bp, fields);
> > +
> > +	if (bp) {
> > +		xfs_btree_offsets(fields,
> > +				  (cur->bc_flags & XFS_BTREE_LONG_PTRS) ?
> > +				  	loffsets : soffsets,
>                                 ^^
> Some stray whitespace there.

Fixed.

> > +				  XFS_BB_NUM_BITS, &first, &last);
> > +		xfs_trans_log_buf(cur->bc_tp, bp, first, last);
> > +	} else {
> > +		/* XXX(hch): maybe factor out into a method? */
> > +		xfs_trans_log_inode(cur->bc_tp, cur->bc_private.b.ip,
> > +			XFS_ILOG_FBROOT(cur->bc_private.b.whichfork));
> 
> I don't think it is necessary at this point.

It's the only leakage of the detailed inode root implementation into
the generic code, so I'm still wondering whether a method would be
better.