[PATCH 22/48] xfsprogs: Add verifiers to libxfs buffer interfaces.

[Ben Myers]

Dave,

On Fri, Jun 07, 2013 at 10:25:45AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner at redhat.com>
> 
> Verifiers need to be used everywhere to enable calculation of CRCs
> during writeback of modified metadata. Add then to the libxfs buffer
> interfaces conver the internal use of devices to be buftarg aware.
> 
> Verifiers also require that the buffer has a back pointer to the
> struct xfs_mount. To make this source level comaptible between
> kernel and userspace, convert userspace to pass struct xfs_buftargs
> around rather than a "device".
> 
> Signed-off-by: Dave Chinner <dchinner at redhat.com>


> @@ -507,7 +527,7 @@ typedef struct xfs_inode {
>  	xfs_mount_t		*i_mount;	/* fs mount struct ptr */
>  	xfs_ino_t		i_ino;		/* inode number (agno/agino) */
>  	struct xfs_imap		i_imap;		/* location for xfs_imap() */
> -	dev_t			i_dev;		/* dev for this inode */
> +	struct xfs_buftarg			i_dev;		/* dev for this inode */

Got a little jumpy with the tabs there...

> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index e9cc7b1..f91a5d0 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -200,12 +200,15 @@ libxfs_log_header(
>  #undef libxfs_getbuf_flags
>  #undef libxfs_putbuf
>  
> -xfs_buf_t	*libxfs_readbuf(dev_t, xfs_daddr_t, int, int);
> -xfs_buf_t	*libxfs_readbuf_map(dev_t, struct xfs_buf_map *, int, int);
> +xfs_buf_t	*libxfs_readbuf(struct xfs_buftarg *, xfs_daddr_t, int, int,
> +				const struct xfs_buf_map *);

				const struct xfs_buf_ops *);

> +xfs_buf_t	*libxfs_readbuf_map(struct xfs_buftarg *, struct xfs_buf_map *,
> +				int, int, const struct xfs_buf_map *);

					  const struct xfs_buf_ops *);

> @@ -612,9 +622,9 @@ libxfs_purgebuf(xfs_buf_t *bp)
>  {
>  	struct xfs_bufkey key = {0};
>  
> -	key.device = bp->b_dev;
> +	key.buftarg = bp->b_target;
>  	key.blkno = bp->b_bn;
> -	key.bblen = bp->b_bcount >> BBSHIFT;
> +	key.bblen = bp->b_length;

Why was this change necessary?  b_bcount to b_length?  It doesn't seem to be
related to the rest of the patch.

> @@ -767,9 +803,42 @@ __write_buf(int fd, void *buf, int len, off64_t offset, int flags)
>  int
>  libxfs_writebufr(xfs_buf_t *bp)
>  {
> -	int	fd = libxfs_device_to_fd(bp->b_dev);
> +	int	fd = libxfs_device_to_fd(bp->b_target->dev);
>  	int	error = 0;
>  
> +	/*
> +	 * we never write buffers that are marked stale. This indicates they
> +	 * contain data that has been invalidated, and even if the buffer is
> +	 * dirty it must *never* be written. Verifiers are wonderful for finding
> +	 * bugs like this. Make sure the error is obvious as to the cause.
> +	 */
> +	if (bp->b_flags & LIBXFS_B_STALE) {
> +		bp->b_error = ESTALE;
> +		return bp->b_error;
> +	}

What led to this?

> +
> +	/*
> +	 * clear any pre-existing error status on the buffer. This can occur if
> +	 * the buffer is corrupt on disk and the repair process doesn't clear
> +	 * the error before fixing and writing it back.
> +	 */
> +	bp->b_error = 0;
> +	if (bp->b_ops) {
> +		bp->b_ops->verify_write(bp);
> +		if (bp->b_error) {
> +			fprintf(stderr,
> +	_("%s: write verifer failed on bno 0x%llx/0x%x\n"),
> +				__func__, (long long)bp->b_bn, bp->b_bcount);
> +			return bp->b_error;
> +		}
> +	}
> +
> +	if (bp->b_ops) {
> +		bp->b_ops->verify_write(bp);
> +		if (bp->b_error)
> +			return bp->b_error;
> +	}
> +

Calling the verifier twice?  Maybe I'm seeing double again...

> @@ -187,11 +184,7 @@ roundup_pow_of_two(uint v)
>  	NULL;						\
>  })
>  #define xfs_buf_relse(bp)		libxfs_putbuf(bp)
> -#define xfs_read_buf(mp,devp,blkno,len,f,bpp)	\
> -					(*(bpp) = libxfs_readbuf((devp), \
> -							(blkno), (len), 1), 0)

Yeah, nobody is using this macro anymore.

> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index a393607..3864932 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2487,13 +2488,19 @@ an AG size that is one stripe unit smaller, for example %llu.\n"),
>  		exit(1);
>  	}
>  
> +	/*
> +	 * XXX: this code is effectively shared with the kernel growfs code.
> +	 * These initialisations should be pulled into libxfs to keep the
> +	 * kernel/userspace header initialisation code the same.
> +	 */
>  	for (agno = 0; agno < agcount; agno++) {

Nice idea.

>  		/*
>  		 * Superblock.
>  		 */
> -		buf = libxfs_getbuf(xi.ddev,
> +		buf = libxfs_getbuf(mp->m_ddev_targp,
>  				XFS_AG_DADDR(mp, agno, XFS_SB_DADDR),
>  				XFS_FSS_TO_BB(mp, 1));
> +		buf->b_ops = &xfs_sb_buf_ops;
>  		memset(XFS_BUF_PTR(buf), 0, sectorsize);
>  		libxfs_sb_to_disk((void *)XFS_BUF_PTR(buf), sbp, XFS_SB_ALL_BITS);
>  		libxfs_writebuf(buf, LIBXFS_EXIT_ON_FAILURE);

...

> @@ -1353,7 +1353,8 @@ scan_ags(
>  	}
>  	memset(agcnts, 0, mp->m_sb.sb_agcount * sizeof(*agcnts));
>  
> -	create_work_queue(&wq, mp, scan_threads);
> +	create_work_queue(&wq, mp, 1);
> +	//create_work_queue(&wq, mp, scan_threads);

What's this all about?  Were you having trouble with a multithreaded scan?

Looks fine for the most part...  I did get a little uncomfortable with using
verifiers on reads in repair.  Not sure whether setting b_error = EFSCORRUPTED
would have ill effect later.

Reviewed-by: Ben Myers <bpm at sgi.com>

Regards,
Ben